fix(tctoken): address all 7 PR #386 review findings
All 7 inline review comments validated as real bugs (zero false positives). Fixes applied: #1 Codex P1 (CRITICAL) — process-message.ts: storeTcTokensFromHistorySync ran BEFORE storeLIDPNMappings. On a fresh device with empty mapping cache, resolveTcTokenJid(PN) returned null for every chat → tokens stored under PN keys → send path resolves PN→LID, misses them → error 463 on first multi-device send. Defeated the purpose of the whole TIER 1.3 backport. Reordered: mappings first, tctokens after. #3 Copilot (MAJOR) — messages-send.ts: PSA/bot gating relied on `destinationJid === PSA_WID` (which is '0@c.us') and isJidBot (also @c.us-only), but destinationJid arrives normalized to @s.whatsapp.net. Issuance was leaking to PSA/bot contacts. Replaced with `!isRegularUser(destinationJid)` — the same Wid. isRegularUser() port the store path already uses, which handles @c.us / @s.whatsapp.net / @hosted / @hosted.lid / @lid uniformly. #4 Copilot (MAJOR) — tc-token-utils.ts: resolveIssuanceJid used strict isLidUser() so hosted LIDs (@hosted.lid) skipped resolution, and passed un-normalized JIDs to getLIDForPN (which early-returns unless isAnyPnUser, breaking @c.us inputs). Fixed by normalizing upfront with jidNormalizedUser and using isAnyLidUser/isAnyPnUser. Added 3 new tests covering @c.us → normalize → resolve, hosted.lid passthrough on issueToLid=true, and hosted.lid → getPNForLID call on issueToLid=false. #6 Copilot (MAJOR) — messages-recv.ts: scheduleTcTokenIndexSave wrote the index from the in-memory tcTokenKnownJids set, OVERWRITING any JIDs added by cross-layer paths (messages-send issuance, process-message history sync) that wrote via buildMergedTcTokenIndexWrite without updating the in-memory set. Each debounced flush silently dropped those JIDs. Same bug in the connection.update flush path. Both now use buildMergedTcTokenIndexWrite so the persisted index is preserved. #7 CodeRabbit Minor — process-message.ts: storeTcTokensFromHistorySync's loop captured `existing` once before the loop, so when two candidates resolved to the same storageJid (PN+LID alias collision through resolveTcTokenJid, or duplicate chunks across syncs), the second iteration read the original persisted entry instead of the in-progress entries[storageJid] from iter 1. A lower-ts entry could overwrite a higher-ts one. Fixed with `entries[c.storageJid] ?? existing[c.storageJid]`. #5 Copilot (DEFENSIVE) — identity-change-handler.ts: onBeforeSessionRefresh was called outside the try/catch around assertSessions. A throwing consumer callback would abort identity-change recovery and prevent the session refresh entirely. Wrapped in try/catch with warn-log; assertSessions still runs. #2 Copilot (TYPO) — messages-recv.ts: 'racets' → 'races' in the reissueTcTokenAfterIdentityChange docstring. Tests: 35/35 suites, 824/824 (+3 new for resolveIssuanceJid normalization). Zero diff in src/Utils/messages.ts (carousel/buttons/lists), src/Socket/groups.ts, WAProto/* — customizations untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+25
-10
@@ -74,6 +74,7 @@ import {
|
|||||||
recordMessageRetry
|
recordMessageRetry
|
||||||
} from '../Utils/prometheus-metrics.js'
|
} from '../Utils/prometheus-metrics.js'
|
||||||
import {
|
import {
|
||||||
|
buildMergedTcTokenIndexWrite,
|
||||||
isTcTokenExpired,
|
isTcTokenExpired,
|
||||||
resolveIssuanceJid,
|
resolveIssuanceJid,
|
||||||
resolveTcTokenJid,
|
resolveTcTokenJid,
|
||||||
@@ -209,16 +210,25 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
|
|||||||
}
|
}
|
||||||
})()
|
})()
|
||||||
|
|
||||||
/** Debounced save of the tctoken JID index (5s) */
|
/**
|
||||||
|
* Debounced save of the tctoken JID index (5s).
|
||||||
|
*
|
||||||
|
* Merges with the persisted index instead of overwriting — other layers
|
||||||
|
* (messages-send fire-and-forget issuance, process-message history sync) may
|
||||||
|
* write JIDs to the index via `buildMergedTcTokenIndexWrite` without updating
|
||||||
|
* `tcTokenKnownJids`. Without the merge those JIDs would be silently dropped
|
||||||
|
* the next time this debounced save fires.
|
||||||
|
*/
|
||||||
const scheduleTcTokenIndexSave = () => {
|
const scheduleTcTokenIndexSave = () => {
|
||||||
if (tcTokenIndexSaveTimer) clearTimeout(tcTokenIndexSaveTimer)
|
if (tcTokenIndexSaveTimer) clearTimeout(tcTokenIndexSaveTimer)
|
||||||
tcTokenIndexSaveTimer = setTimeout(async () => {
|
tcTokenIndexSaveTimer = setTimeout(async () => {
|
||||||
try {
|
try {
|
||||||
const arr = Array.from(tcTokenKnownJids)
|
const merged = await buildMergedTcTokenIndexWrite(authState.keys, tcTokenKnownJids)
|
||||||
await authState.keys.set({
|
await authState.keys.set({
|
||||||
tctoken: {
|
tctoken: {
|
||||||
|
...merged,
|
||||||
[TC_TOKEN_INDEX_KEY]: {
|
[TC_TOKEN_INDEX_KEY]: {
|
||||||
token: Buffer.from(JSON.stringify(arr), 'utf8'),
|
...merged[TC_TOKEN_INDEX_KEY],
|
||||||
timestamp: unixTimestampSeconds().toString()
|
timestamp: unixTimestampSeconds().toString()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -1453,7 +1463,7 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
|
|||||||
* Why parallel and not sequential:
|
* Why parallel and not sequential:
|
||||||
* - WA Web invokes this BEFORE assertSessions to maximise the chance the contact
|
* - WA Web invokes this BEFORE assertSessions to maximise the chance the contact
|
||||||
* has a fresh tctoken by the time the next outbound send executes.
|
* has a fresh tctoken by the time the next outbound send executes.
|
||||||
* - Running after assertSessions (the fork's previous behaviour) racets with the
|
* - Running after assertSessions (the fork's previous behaviour) races with the
|
||||||
* next send and risks error 463 when the contact reinstalls and the user replies
|
* next send and risks error 463 when the contact reinstalls and the user replies
|
||||||
* immediately afterwards.
|
* immediately afterwards.
|
||||||
*
|
*
|
||||||
@@ -3238,19 +3248,24 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
|
|||||||
// Await index load first — prevents overwriting a more complete persisted index
|
// Await index load first — prevents overwriting a more complete persisted index
|
||||||
// if the connection closes before the initial load finishes.
|
// if the connection closes before the initial load finishes.
|
||||||
tcTokenIndexLoaded
|
tcTokenIndexLoaded
|
||||||
.then(() => {
|
.then(async () => {
|
||||||
Promise.resolve(
|
try {
|
||||||
authState.keys.set({
|
// Same merge-with-persisted invariant as scheduleTcTokenIndexSave —
|
||||||
|
// other layers may have written cross-layer JIDs to the index since
|
||||||
|
// our in-memory set was last updated.
|
||||||
|
const merged = await buildMergedTcTokenIndexWrite(authState.keys, tcTokenKnownJids)
|
||||||
|
await authState.keys.set({
|
||||||
tctoken: {
|
tctoken: {
|
||||||
|
...merged,
|
||||||
[TC_TOKEN_INDEX_KEY]: {
|
[TC_TOKEN_INDEX_KEY]: {
|
||||||
token: Buffer.from(JSON.stringify([...tcTokenKnownJids]), 'utf8'),
|
...merged[TC_TOKEN_INDEX_KEY],
|
||||||
timestamp: unixTimestampSeconds().toString()
|
timestamp: unixTimestampSeconds().toString()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
).catch(() => {
|
} catch {
|
||||||
/* non-critical */
|
/* non-critical */
|
||||||
})
|
}
|
||||||
})
|
})
|
||||||
.catch(() => {
|
.catch(() => {
|
||||||
/* non-critical */
|
/* non-critical */
|
||||||
|
|||||||
@@ -47,6 +47,7 @@ import { metrics, recordMessageFailure, recordMessageSent } from '../Utils/prome
|
|||||||
import { getMessageReportingToken, shouldIncludeReportingToken } from '../Utils/reporting-utils'
|
import { getMessageReportingToken, shouldIncludeReportingToken } from '../Utils/reporting-utils'
|
||||||
import {
|
import {
|
||||||
buildMergedTcTokenIndexWrite,
|
buildMergedTcTokenIndexWrite,
|
||||||
|
isRegularUser,
|
||||||
isTcTokenExpired,
|
isTcTokenExpired,
|
||||||
resolveIssuanceJid,
|
resolveIssuanceJid,
|
||||||
resolveTcTokenJid,
|
resolveTcTokenJid,
|
||||||
@@ -66,14 +67,12 @@ import {
|
|||||||
isHostedPnUser,
|
isHostedPnUser,
|
||||||
isJidBot,
|
isJidBot,
|
||||||
isJidGroup,
|
isJidGroup,
|
||||||
isJidMetaAI,
|
|
||||||
isLidUser,
|
isLidUser,
|
||||||
isPnUser,
|
isPnUser,
|
||||||
jidDecode,
|
jidDecode,
|
||||||
jidEncode,
|
jidEncode,
|
||||||
jidNormalizedUser,
|
jidNormalizedUser,
|
||||||
type JidWithDevice,
|
type JidWithDevice,
|
||||||
PSA_WID,
|
|
||||||
S_WHATSAPP_NET
|
S_WHATSAPP_NET
|
||||||
} from '../WABinary'
|
} from '../WABinary'
|
||||||
import { USyncQuery, USyncUser } from '../WAUSync'
|
import { USyncQuery, USyncUser } from '../WAUSync'
|
||||||
@@ -1845,7 +1844,11 @@ export const makeMessagesSocket = (config: SocketConfig) => {
|
|||||||
// rapid sends to the same contact only triggers a single IQ before senderTimestamp
|
// rapid sends to the same contact only triggers a single IQ before senderTimestamp
|
||||||
// is persisted.
|
// is persisted.
|
||||||
const isProtocolMsg = !!normalizeMessageContent(message)?.protocolMessage
|
const isProtocolMsg = !!normalizeMessageContent(message)?.protocolMessage
|
||||||
const isBotOrPSA = destinationJid === PSA_WID || isJidBot(destinationJid) || isJidMetaAI(destinationJid)
|
// Use isRegularUser (the same Wid.isRegularUser() port that gates the store
|
||||||
|
// path) so we filter PSA/bot/MetaAI consistently regardless of JID server
|
||||||
|
// (@c.us vs @s.whatsapp.net) and device suffix. The previous PSA_WID/isJidBot
|
||||||
|
// checks only matched @c.us forms — destinationJid arrives normalized.
|
||||||
|
const isBotOrPSA = !isRegularUser(destinationJid)
|
||||||
if (
|
if (
|
||||||
is1on1Send &&
|
is1on1Send &&
|
||||||
!isProtocolMsg &&
|
!isProtocolMsg &&
|
||||||
|
|||||||
@@ -183,7 +183,15 @@ export async function handleIdentityChange(
|
|||||||
// Fire-and-forget side effects (e.g. tctoken re-issuance) BEFORE the session is
|
// Fire-and-forget side effects (e.g. tctoken re-issuance) BEFORE the session is
|
||||||
// re-established. WA Web runs these in parallel with the session refresh —
|
// re-established. WA Web runs these in parallel with the session refresh —
|
||||||
// running afterwards would race with the next outbound send and risk error 463.
|
// running afterwards would race with the next outbound send and risk error 463.
|
||||||
|
//
|
||||||
|
// Wrapped in try/catch so a misbehaving consumer callback cannot abort identity
|
||||||
|
// change recovery. We log and continue — assertSessions still runs so the E2E
|
||||||
|
// session always gets refreshed.
|
||||||
|
try {
|
||||||
ctx.onBeforeSessionRefresh?.(from)
|
ctx.onBeforeSessionRefresh?.(from)
|
||||||
|
} catch (error) {
|
||||||
|
ctx.logger.warn({ error, jid: from }, 'onBeforeSessionRefresh callback threw — continuing with session refresh')
|
||||||
|
}
|
||||||
|
|
||||||
// Attempt session refresh/creation
|
// Attempt session refresh/creation
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -114,7 +114,11 @@ async function storeTcTokensFromHistorySync(
|
|||||||
const entries: Record<string, { token: Buffer; timestamp?: string; senderTimestamp?: number }> = {}
|
const entries: Record<string, { token: Buffer; timestamp?: string; senderTimestamp?: number }> = {}
|
||||||
|
|
||||||
for (const c of candidates) {
|
for (const c of candidates) {
|
||||||
const existingEntry = existing[c.storageJid]
|
// Same-batch dedup: when two chats resolve to the same storageJid (e.g. PN+LID
|
||||||
|
// aliases collapsing through resolveTcTokenJid, or duplicate chunks across
|
||||||
|
// retries), prefer the value already written by an earlier iteration so a
|
||||||
|
// lower-ts entry can't overwrite a higher-ts one captured from `existing`.
|
||||||
|
const existingEntry = entries[c.storageJid] ?? existing[c.storageJid]
|
||||||
const existingTs = existingEntry?.timestamp ? Number(existingEntry.timestamp) : 0
|
const existingTs = existingEntry?.timestamp ? Number(existingEntry.timestamp) : 0
|
||||||
// Strict > guard: equal timestamps are skipped so we never clobber
|
// Strict > guard: equal timestamps are skipped so we never clobber
|
||||||
// senderTimestamp written by other layers (issuance after send, etc).
|
// senderTimestamp written by other layers (issuance after send, etc).
|
||||||
@@ -496,13 +500,14 @@ const processMessage = async (
|
|||||||
|
|
||||||
const data = await downloadAndProcessHistorySyncNotification(histNotification, options, logger)
|
const data = await downloadAndProcessHistorySyncNotification(histNotification, options, logger)
|
||||||
|
|
||||||
// Persist tctokens carried by history-sync chats BEFORE emitting messaging-history.set
|
|
||||||
// — listeners may immediately fire outbound sends that need the tctoken, and the store
|
|
||||||
// has to be populated first to avoid an error 463 on the first multi-device send.
|
|
||||||
await storeTcTokensFromHistorySync(data.chats, signalRepository, keyStore, logger)
|
|
||||||
|
|
||||||
// Emit LID-PN mappings from history sync
|
// Emit LID-PN mappings from history sync
|
||||||
// This is how WhatsApp Web learns mappings for chats with non-contacts
|
// This is how WhatsApp Web learns mappings for chats with non-contacts
|
||||||
|
//
|
||||||
|
// MUST run BEFORE storeTcTokensFromHistorySync — otherwise resolveTcTokenJid()
|
||||||
|
// can't resolve PN→LID for fresh-device chats (mapping cache is empty), tokens
|
||||||
|
// get persisted under PN keys, and the send path (which resolves to LID first)
|
||||||
|
// misses them — exactly the error 463 scenario this whole change is meant to
|
||||||
|
// prevent. Catches Codex P1 review on PR #386.
|
||||||
if (data.lidPnMappings?.length) {
|
if (data.lidPnMappings?.length) {
|
||||||
logger?.debug({ count: data.lidPnMappings.length }, 'processing LID-PN mappings from history sync')
|
logger?.debug({ count: data.lidPnMappings.length }, 'processing LID-PN mappings from history sync')
|
||||||
// eslint-disable-next-line max-depth
|
// eslint-disable-next-line max-depth
|
||||||
@@ -527,6 +532,12 @@ const processMessage = async (
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Persist tctokens carried by history-sync chats BEFORE emitting messaging-history.set
|
||||||
|
// — listeners may immediately fire outbound sends that need the tctoken, and the store
|
||||||
|
// has to be populated first to avoid an error 463 on the first multi-device send.
|
||||||
|
// Runs AFTER storeLIDPNMappings (see comment above) so LID resolution works.
|
||||||
|
await storeTcTokensFromHistorySync(data.chats, signalRepository, keyStore, logger)
|
||||||
|
|
||||||
ev.emit('messaging-history.set', {
|
ev.emit('messaging-history.set', {
|
||||||
...data,
|
...data,
|
||||||
isLatest: histNotification.syncType !== proto.HistorySync.HistorySyncType.ON_DEMAND ? isLatest : undefined,
|
isLatest: histNotification.syncType !== proto.HistorySync.HistorySyncType.ON_DEMAND ? isLatest : undefined,
|
||||||
|
|||||||
@@ -3,6 +3,8 @@ import type { BinaryNode } from '../WABinary'
|
|||||||
import {
|
import {
|
||||||
getBinaryNodeChild,
|
getBinaryNodeChild,
|
||||||
getBinaryNodeChildren,
|
getBinaryNodeChildren,
|
||||||
|
isAnyLidUser,
|
||||||
|
isAnyPnUser,
|
||||||
isHostedLidUser,
|
isHostedLidUser,
|
||||||
isHostedPnUser,
|
isHostedPnUser,
|
||||||
isJidMetaAI,
|
isJidMetaAI,
|
||||||
@@ -141,6 +143,11 @@ export async function resolveTcTokenJid(
|
|||||||
* the LID; when off, it goes to the PN. Returns the original JID if no
|
* the LID; when off, it goes to the PN. Returns the original JID if no
|
||||||
* mapping is found in either direction.
|
* mapping is found in either direction.
|
||||||
*
|
*
|
||||||
|
* Normalizes the JID upfront and uses the `isAny*` helpers so callers can pass
|
||||||
|
* `@c.us`, `@s.whatsapp.net`, `@hosted`, `@hosted.lid`, `@lid` or device-specific
|
||||||
|
* forms — `LIDMappingStore.getLIDForPN` early-returns unless `isAnyPnUser`,
|
||||||
|
* so unnormalized inputs would silently bypass routing.
|
||||||
|
*
|
||||||
* Reference: WAWebTrustedContactsManager.issuePrivacyTokens
|
* Reference: WAWebTrustedContactsManager.issuePrivacyTokens
|
||||||
*/
|
*/
|
||||||
export async function resolveIssuanceJid(
|
export async function resolveIssuanceJid(
|
||||||
@@ -149,19 +156,22 @@ export async function resolveIssuanceJid(
|
|||||||
getLIDForPN: (pn: string) => Promise<string | null>,
|
getLIDForPN: (pn: string) => Promise<string | null>,
|
||||||
getPNForLID?: (lid: string) => Promise<string | null>
|
getPNForLID?: (lid: string) => Promise<string | null>
|
||||||
): Promise<string> {
|
): Promise<string> {
|
||||||
|
const normalized = jidNormalizedUser(jid)
|
||||||
|
|
||||||
if (issueToLid) {
|
if (issueToLid) {
|
||||||
if (isLidUser(jid)) return jid
|
if (isAnyLidUser(normalized)) return normalized
|
||||||
const lid = await getLIDForPN(jid)
|
if (!isAnyPnUser(normalized)) return normalized
|
||||||
return lid ?? jid
|
const lid = await getLIDForPN(normalized)
|
||||||
|
return lid ?? normalized
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!isLidUser(jid)) return jid
|
if (!isAnyLidUser(normalized)) return normalized
|
||||||
if (getPNForLID) {
|
if (getPNForLID) {
|
||||||
const pn = await getPNForLID(jid)
|
const pn = await getPNForLID(normalized)
|
||||||
return pn ?? jid
|
return pn ?? normalized
|
||||||
}
|
}
|
||||||
|
|
||||||
return jid
|
return normalized
|
||||||
}
|
}
|
||||||
|
|
||||||
type TcTokenParams = {
|
type TcTokenParams = {
|
||||||
|
|||||||
@@ -956,6 +956,34 @@ describe('resolveIssuanceJid', () => {
|
|||||||
expect(await resolveIssuanceJid(LID, false, getLIDForPN)).toBe(LID)
|
expect(await resolveIssuanceJid(LID, false, getLIDForPN)).toBe(LID)
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
describe('JID normalization (handles @c.us and hosted forms)', () => {
|
||||||
|
const PN_CUS = '5511999999999@c.us'
|
||||||
|
const PN_NORMALIZED = '5511999999999@s.whatsapp.net'
|
||||||
|
const HOSTED_LID = '999999@hosted.lid'
|
||||||
|
|
||||||
|
it('normalizes @c.us before resolving (issueToLid=true)', async () => {
|
||||||
|
const fn = jest.fn<(pn: string) => Promise<string | null>>(async (pn: string) => {
|
||||||
|
if (pn === PN_NORMALIZED) return LID
|
||||||
|
return null
|
||||||
|
})
|
||||||
|
const result = await resolveIssuanceJid(PN_CUS, true, fn, getPNForLID)
|
||||||
|
expect(result).toBe(LID)
|
||||||
|
// Mapping store called with NORMALIZED form
|
||||||
|
expect(fn).toHaveBeenCalledWith(PN_NORMALIZED)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('treats hosted LID as LID input (issueToLid=true returns it unchanged)', async () => {
|
||||||
|
expect(await resolveIssuanceJid(HOSTED_LID, true, getLIDForPN, getPNForLID)).toBe(HOSTED_LID)
|
||||||
|
expect(getLIDForPN).not.toHaveBeenCalled()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('treats hosted LID as LID input (issueToLid=false converts via getPNForLID)', async () => {
|
||||||
|
const fn = jest.fn<(lid: string) => Promise<string | null>>(async () => null)
|
||||||
|
await resolveIssuanceJid(HOSTED_LID, false, getLIDForPN, fn)
|
||||||
|
expect(fn).toHaveBeenCalledWith(HOSTED_LID)
|
||||||
|
})
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
// ─── readTcTokenIndex / buildMergedTcTokenIndexWrite ───────────────────
|
// ─── readTcTokenIndex / buildMergedTcTokenIndexWrite ───────────────────
|
||||||
|
|||||||
Reference in New Issue
Block a user