From 8b5818be31f1ed6e9a8946b3403e71e3aef7f957 Mon Sep 17 00:00:00 2001 From: Renato Alcara Date: Sat, 25 Apr 2026 18:28:13 -0300 Subject: [PATCH] fix(tctoken): address all 7 PR #386 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/Socket/messages-recv.ts | 35 ++++++++++++++++++++-------- src/Socket/messages-send.ts | 9 ++++--- src/Utils/identity-change-handler.ts | 10 +++++++- src/Utils/process-message.ts | 23 +++++++++++++----- src/Utils/tc-token-utils.ts | 24 +++++++++++++------ src/__tests__/Utils/tc-token.test.ts | 28 ++++++++++++++++++++++ 6 files changed, 102 insertions(+), 27 deletions(-) diff --git a/src/Socket/messages-recv.ts b/src/Socket/messages-recv.ts index f9e95788..04665b2d 100644 --- a/src/Socket/messages-recv.ts +++ b/src/Socket/messages-recv.ts @@ -74,6 +74,7 @@ import { recordMessageRetry } from '../Utils/prometheus-metrics.js' import { + buildMergedTcTokenIndexWrite, isTcTokenExpired, resolveIssuanceJid, 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 = () => { if (tcTokenIndexSaveTimer) clearTimeout(tcTokenIndexSaveTimer) tcTokenIndexSaveTimer = setTimeout(async () => { try { - const arr = Array.from(tcTokenKnownJids) + const merged = await buildMergedTcTokenIndexWrite(authState.keys, tcTokenKnownJids) await authState.keys.set({ tctoken: { + ...merged, [TC_TOKEN_INDEX_KEY]: { - token: Buffer.from(JSON.stringify(arr), 'utf8'), + ...merged[TC_TOKEN_INDEX_KEY], timestamp: unixTimestampSeconds().toString() } } @@ -1453,7 +1463,7 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => { * Why parallel and not sequential: * - WA Web invokes this BEFORE assertSessions to maximise the chance the contact * 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 * immediately afterwards. * @@ -3238,19 +3248,24 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => { // Await index load first — prevents overwriting a more complete persisted index // if the connection closes before the initial load finishes. tcTokenIndexLoaded - .then(() => { - Promise.resolve( - authState.keys.set({ + .then(async () => { + try { + // 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: { + ...merged, [TC_TOKEN_INDEX_KEY]: { - token: Buffer.from(JSON.stringify([...tcTokenKnownJids]), 'utf8'), + ...merged[TC_TOKEN_INDEX_KEY], timestamp: unixTimestampSeconds().toString() } } }) - ).catch(() => { + } catch { /* non-critical */ - }) + } }) .catch(() => { /* non-critical */ diff --git a/src/Socket/messages-send.ts b/src/Socket/messages-send.ts index 2356313b..b2122105 100644 --- a/src/Socket/messages-send.ts +++ b/src/Socket/messages-send.ts @@ -47,6 +47,7 @@ import { metrics, recordMessageFailure, recordMessageSent } from '../Utils/prome import { getMessageReportingToken, shouldIncludeReportingToken } from '../Utils/reporting-utils' import { buildMergedTcTokenIndexWrite, + isRegularUser, isTcTokenExpired, resolveIssuanceJid, resolveTcTokenJid, @@ -66,14 +67,12 @@ import { isHostedPnUser, isJidBot, isJidGroup, - isJidMetaAI, isLidUser, isPnUser, jidDecode, jidEncode, jidNormalizedUser, type JidWithDevice, - PSA_WID, S_WHATSAPP_NET } from '../WABinary' 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 // is persisted. 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 ( is1on1Send && !isProtocolMsg && diff --git a/src/Utils/identity-change-handler.ts b/src/Utils/identity-change-handler.ts index d3a81111..e445d05c 100644 --- a/src/Utils/identity-change-handler.ts +++ b/src/Utils/identity-change-handler.ts @@ -183,7 +183,15 @@ export async function handleIdentityChange( // 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 — // running afterwards would race with the next outbound send and risk error 463. - ctx.onBeforeSessionRefresh?.(from) + // + // 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) + } catch (error) { + ctx.logger.warn({ error, jid: from }, 'onBeforeSessionRefresh callback threw — continuing with session refresh') + } // Attempt session refresh/creation try { diff --git a/src/Utils/process-message.ts b/src/Utils/process-message.ts index 0b47ad28..562a0268 100644 --- a/src/Utils/process-message.ts +++ b/src/Utils/process-message.ts @@ -114,7 +114,11 @@ async function storeTcTokensFromHistorySync( const entries: Record = {} 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 // Strict > guard: equal timestamps are skipped so we never clobber // senderTimestamp written by other layers (issuance after send, etc). @@ -496,13 +500,14 @@ const processMessage = async ( 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 // 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) { logger?.debug({ count: data.lidPnMappings.length }, 'processing LID-PN mappings from history sync') // 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', { ...data, isLatest: histNotification.syncType !== proto.HistorySync.HistorySyncType.ON_DEMAND ? isLatest : undefined, diff --git a/src/Utils/tc-token-utils.ts b/src/Utils/tc-token-utils.ts index 392326ff..008f1e2d 100644 --- a/src/Utils/tc-token-utils.ts +++ b/src/Utils/tc-token-utils.ts @@ -3,6 +3,8 @@ import type { BinaryNode } from '../WABinary' import { getBinaryNodeChild, getBinaryNodeChildren, + isAnyLidUser, + isAnyPnUser, isHostedLidUser, isHostedPnUser, isJidMetaAI, @@ -141,6 +143,11 @@ export async function resolveTcTokenJid( * the LID; when off, it goes to the PN. Returns the original JID if no * 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 */ export async function resolveIssuanceJid( @@ -149,19 +156,22 @@ export async function resolveIssuanceJid( getLIDForPN: (pn: string) => Promise, getPNForLID?: (lid: string) => Promise ): Promise { + const normalized = jidNormalizedUser(jid) + if (issueToLid) { - if (isLidUser(jid)) return jid - const lid = await getLIDForPN(jid) - return lid ?? jid + if (isAnyLidUser(normalized)) return normalized + if (!isAnyPnUser(normalized)) return normalized + const lid = await getLIDForPN(normalized) + return lid ?? normalized } - if (!isLidUser(jid)) return jid + if (!isAnyLidUser(normalized)) return normalized if (getPNForLID) { - const pn = await getPNForLID(jid) - return pn ?? jid + const pn = await getPNForLID(normalized) + return pn ?? normalized } - return jid + return normalized } type TcTokenParams = { diff --git a/src/__tests__/Utils/tc-token.test.ts b/src/__tests__/Utils/tc-token.test.ts index 924542d2..1bce0362 100644 --- a/src/__tests__/Utils/tc-token.test.ts +++ b/src/__tests__/Utils/tc-token.test.ts @@ -956,6 +956,34 @@ describe('resolveIssuanceJid', () => { 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>(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>(async () => null) + await resolveIssuanceJid(HOSTED_LID, false, getLIDForPN, fn) + expect(fn).toHaveBeenCalledWith(HOSTED_LID) + }) + }) }) // ─── readTcTokenIndex / buildMergedTcTokenIndexWrite ───────────────────