From 6c09b14776c557fa7c489be3334ac14cf1266062 Mon Sep 17 00:00:00 2001 From: Renato Alcara Date: Wed, 29 Apr 2026 00:36:20 -0300 Subject: [PATCH] =?UTF-8?q?fix(signal):=20bot-review=20round=202=20?= =?UTF-8?q?=E2=80=94=20injectE2ESession=20lock,=20doc/error=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses bot review on commit 4bdcd5769d: 1) [Major — CodeRabbit + Copilot] `injectE2ESession` was the last session-mutating path still locking on raw `jid` while writing through `storeSession()` (which canonicalizes via `resolveLIDSignalAddress`). Same PN/LID lock-vs-storage drift as the original bug — fixed by computing `wireAddr` once and locking on it (same pattern as encrypt/decrypt). 2) [Minor — Copilot] `resolveLIDSignalAddress` docstring described the input as `userDomain.device` but the actual format from `jidToSignalProtocolAddress` is `signalUser.device`, where `signalUser` is either the bare user (WhatsApp domain) or `user_domainType` for LID/hosted. Updated to match. 3) [Minor — Copilot] `Missing device ID` error fired when the user portion (before the `.`) was empty, which was misleading. Split into two specific errors and added validation for the empty-device-portion case (e.g. trailing `.`). Each now points at the actual missing part. 4) [Doc — CodeRabbit residual race concern] Added a paragraph explaining the theoretical race between our `wireAddr` resolution and storage's internal re-resolution if `lidMapping` mutates between the two awaits. In production this is mitigated by the `migrateSession()` call upstream in `messages-recv.ts` running synchronously BEFORE `decryptMessage`, so the mapping is populated by the time the lock is acquired. Eliminating the residual entirely requires storage to skip re-resolution — bigger refactor reserved for a follow-up if the residual ever bites in prod. 5) [Out of scope — Copilot PR-metadata staleness] PR title still says "raw JID" from the original commit. Will be retitled on the GitHub side after this lands. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Signal/libsignal.ts | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/Signal/libsignal.ts b/src/Signal/libsignal.ts index 6c93819f..31fa0fbb 100644 --- a/src/Signal/libsignal.ts +++ b/src/Signal/libsignal.ts @@ -441,10 +441,16 @@ export function makeLibSignalRepository( async injectE2ESession({ jid, session }) { logger.trace({ jid }, 'injecting E2EE session') - const cipher = new libsignal.SessionBuilder(storage, jidToSignalProtocolAddress(jid)) + const addr = jidToSignalProtocolAddress(jid) + // Same wire-address locking as encrypt/decrypt — `SessionBuilder.initOutgoing` + // writes via `storeSession` which canonicalizes, so the lock key must be + // the wire address for the mutex to actually serialize concurrent ops on + // the same logical session. + const wireAddr = await resolveLIDSignalAddress(addr.toString(), lidMapping) + const cipher = new libsignal.SessionBuilder(storage, addr) return parsedKeys.transaction(async () => { await cipher.initOutgoing(session) - }, jid) + }, wireAddr) }, jidToSignalProtocolAddress(jid) { return jidToSignalProtocolAddress(jid).toString() @@ -722,9 +728,11 @@ const jidToSignalSenderKeyName = (group: string, user: string): SenderKeyName => } /** - * Resolve a Signal protocol address (string form `userDomain.device`) to its - * canonical *wire* address — the exact key that `loadSession`/`storeSession` - * uses when reading/writing `keys.get('session', [...])`. + * Resolve a Signal protocol address (string form `signalUser.device`, where + * `signalUser` is `user` for the WhatsApp domain or `user_domainType` for any + * other domain — e.g. `12345_1.0` for a LID device 0) to its canonical *wire* + * address — the exact key that `loadSession`/`storeSession` uses when + * reading/writing `keys.get('session', [...])`. * * This is the SAME key the storage layer uses internally, so callers that need * a transaction lock around session mutations MUST lock on this resolved @@ -732,9 +740,20 @@ const jidToSignalSenderKeyName = (group: string, user: string): SenderKeyName => * apart and concurrent ops on the same session interleave → ratchet * corruption → perpetual `Bad MAC` errors. * - * Behavior: if the input already targets a LID domain, returns it unchanged. - * Otherwise looks up the PN→LID mapping; if found, returns the LID-form - * Signal address. If no mapping exists, returns the input unchanged. + * Behavior: if the input already targets a LID domain, returns it unchanged + * (cheap fast path — no awaits). Otherwise looks up the PN→LID mapping; if + * found, returns the LID-form Signal address. If no mapping exists, returns + * the input unchanged. + * + * Residual race window: the lock and the storage's internal re-resolution + * each call this function once. If a PN→LID mapping is added between those + * two calls (microsecond window), the lock could be on the PN address while + * storage canonicalizes to LID. In production this is mitigated upstream: + * `messages-recv.ts` runs `migrateSession()` synchronously BEFORE + * `decryptMessage()` is called, so the mapping is already populated by the + * time we lock. Eliminating the residual race entirely would require storage + * to NOT re-resolve, which is a bigger refactor (other call sites depend on + * the current contract). * * Top-level (not inside `signalStorage`) so `makeLibSignalRepository` can * compute the same wire address for transaction locking. @@ -743,7 +762,11 @@ const resolveLIDSignalAddress = async (id: string, lidMapping: LIDMappingStore): if (id.includes('.')) { const [deviceId, device] = id.split('.') if (!deviceId) { - throw new Error('Missing device ID') + throw new Error(`Malformed signal address (empty user portion before '.'): "${id}"`) + } + + if (device === undefined || device === '') { + throw new Error(`Malformed signal address (empty device portion after '.'): "${id}"`) } const [user, domainType_] = deviceId.split('_')