fix(signal): lock on wire address (not raw JID) — addresses bot review
Bots (Codex P1, Copilot, CodeRabbit) flagged a real defect in the previous
commit (79844a9d29): locking on the raw `jid` doesn't actually serialize
concurrent ops on the same session, because `signalStorage.loadSession`/
`storeSession` internally call `resolveLIDSignalAddress(id)` which canonicalizes
PN→LID before hitting `keys.get/set('session', ...)`. So:
- Op A arrives as PN → lock(PN_addr) → storage rewrites to LID_addr
- Op B arrives as LID → lock(LID_addr) → storage stays at LID_addr
- Two locks, SAME session record → ratchet interleaving → Bad MAC.
This was the same drift the original `canonicalJid` was trying to prevent —
just expressed at a different layer. Pedro's snapshot doesn't trip it because
in Feb/6 there were no PN→LID mappings populated yet (WhatsApp hadn't fully
rolled out LID-addressed DSM), so storage canonicalization was a no-op and lock
key == storage key by accident.
Fix: extract `resolveLIDSignalAddress` to a top-level helper (parameterized by
`lidMapping`) and use it in BOTH places —
1. `signalStorage.loadSession`/`storeSession` (unchanged behavior, just no
longer a closure).
2. `decryptMessage`/`encryptMessage` to compute the *wire address* and lock
on it. Lock key now matches the storage key exactly.
This eliminates both directions of the PN/LID drift. PN/LID mapping latency on
the hot path is unchanged from the previous commit (one `getLIDForPN` lookup
per decrypt — same as before — but now correctly placed).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+62
-39
@@ -341,6 +341,14 @@ export function makeLibSignalRepository(
|
|||||||
},
|
},
|
||||||
async decryptMessage({ jid, type, ciphertext }) {
|
async decryptMessage({ jid, type, ciphertext }) {
|
||||||
const addr = jidToSignalProtocolAddress(jid)
|
const addr = jidToSignalProtocolAddress(jid)
|
||||||
|
// Wire address = the EXACT key that signalStorage's loadSession/storeSession
|
||||||
|
// uses internally (it canonicalizes PN→LID via resolveLIDSignalAddress).
|
||||||
|
// We MUST lock on this same key — locking on the raw `jid` lets two
|
||||||
|
// concurrent ops for the same logical contact (one arriving as PN, one as
|
||||||
|
// LID) acquire DIFFERENT mutex slots and interleave on the same session
|
||||||
|
// record, corrupting the ratchet → perpetual `Bad MAC` on own DSM after
|
||||||
|
// the WhatsApp PN→LID DSM rollout.
|
||||||
|
const wireAddr = await resolveLIDSignalAddress(addr.toString(), lidMapping)
|
||||||
const session = new libsignal.SessionCipher(storage, addr)
|
const session = new libsignal.SessionCipher(storage, addr)
|
||||||
|
|
||||||
async function doDecrypt() {
|
async function doDecrypt() {
|
||||||
@@ -389,29 +397,23 @@ export function makeLibSignalRepository(
|
|||||||
return result
|
return result
|
||||||
}
|
}
|
||||||
|
|
||||||
// Lock on the raw JID — same string libsignal uses to derive the
|
|
||||||
// SessionCipher address (line above). Locking on a *resolved* canonical
|
|
||||||
// JID would mean the lock key and the session storage key drift apart,
|
|
||||||
// letting concurrent ops on the same session interleave and corrupt the
|
|
||||||
// ratchet (observed as a perpetual Bad MAC loop on own DSM after the
|
|
||||||
// PN→LID rollout). PN/LID race protection lives upstream in
|
|
||||||
// migrateSession + storeLIDPNMappings, not here.
|
|
||||||
return parsedKeys.transaction(async () => {
|
return parsedKeys.transaction(async () => {
|
||||||
return await doDecrypt()
|
return await doDecrypt()
|
||||||
}, jid)
|
}, wireAddr)
|
||||||
},
|
},
|
||||||
|
|
||||||
async encryptMessage({ jid, data }) {
|
async encryptMessage({ jid, data }) {
|
||||||
const addr = jidToSignalProtocolAddress(jid)
|
const addr = jidToSignalProtocolAddress(jid)
|
||||||
|
// See decryptMessage above for the rationale: lock on the wire address,
|
||||||
|
// not the raw JID, so concurrent PN/LID ops on the same session serialize.
|
||||||
|
const wireAddr = await resolveLIDSignalAddress(addr.toString(), lidMapping)
|
||||||
const cipher = new libsignal.SessionCipher(storage, addr)
|
const cipher = new libsignal.SessionCipher(storage, addr)
|
||||||
|
|
||||||
// Lock on raw JID — same string used to derive `addr` above. See comment
|
|
||||||
// in decryptMessage for the rationale (lock key must match session storage key).
|
|
||||||
return parsedKeys.transaction(async () => {
|
return parsedKeys.transaction(async () => {
|
||||||
const { type: sigType, body } = await cipher.encrypt(data)
|
const { type: sigType, body } = await cipher.encrypt(data)
|
||||||
const type = sigType === 3 ? 'pkmsg' : 'msg'
|
const type = sigType === 3 ? 'pkmsg' : 'msg'
|
||||||
return { type, ciphertext: Buffer.from(body, 'binary') }
|
return { type, ciphertext: Buffer.from(body, 'binary') }
|
||||||
}, jid)
|
}, wireAddr)
|
||||||
},
|
},
|
||||||
|
|
||||||
async encryptGroupMessage({ group, meId, data }) {
|
async encryptGroupMessage({ group, meId, data }) {
|
||||||
@@ -719,6 +721,48 @@ const jidToSignalSenderKeyName = (group: string, user: string): SenderKeyName =>
|
|||||||
return new SenderKeyName(group, jidToSignalProtocolAddress(user))
|
return new SenderKeyName(group, jidToSignalProtocolAddress(user))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 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', [...])`.
|
||||||
|
*
|
||||||
|
* 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
|
||||||
|
* address (not the raw JID), otherwise the lock key and storage key drift
|
||||||
|
* 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.
|
||||||
|
*
|
||||||
|
* Top-level (not inside `signalStorage`) so `makeLibSignalRepository` can
|
||||||
|
* compute the same wire address for transaction locking.
|
||||||
|
*/
|
||||||
|
const resolveLIDSignalAddress = async (id: string, lidMapping: LIDMappingStore): Promise<string> => {
|
||||||
|
if (id.includes('.')) {
|
||||||
|
const [deviceId, device] = id.split('.')
|
||||||
|
if (!deviceId) {
|
||||||
|
throw new Error('Missing device ID')
|
||||||
|
}
|
||||||
|
|
||||||
|
const [user, domainType_] = deviceId.split('_')
|
||||||
|
const domainType = parseInt(domainType_ || '0')
|
||||||
|
|
||||||
|
if (domainType === WAJIDDomains.LID || domainType === WAJIDDomains.HOSTED_LID) return id
|
||||||
|
|
||||||
|
const pnJid = `${user!}${device !== '0' ? `:${device}` : ''}@${domainType === WAJIDDomains.HOSTED ? 'hosted' : 's.whatsapp.net'}`
|
||||||
|
|
||||||
|
const lidForPN = await lidMapping.getLIDForPN(pnJid)
|
||||||
|
if (lidForPN) {
|
||||||
|
const lidAddr = jidToSignalProtocolAddress(lidForPN)
|
||||||
|
return lidAddr.toString()
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return id
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Extended SignalStorage with identity key management
|
* Extended SignalStorage with identity key management
|
||||||
* This type adds identity key operations to the standard Signal storage
|
* This type adds identity key operations to the standard Signal storage
|
||||||
@@ -750,30 +794,9 @@ function signalStorage(
|
|||||||
ev?: BaileysEventEmitter,
|
ev?: BaileysEventEmitter,
|
||||||
logger?: ILogger
|
logger?: ILogger
|
||||||
): ExtendedSignalStorage {
|
): ExtendedSignalStorage {
|
||||||
// Shared function to resolve PN signal address to LID if mapping exists
|
// Bind the top-level `resolveLIDSignalAddress` to this instance's lidMapping
|
||||||
const resolveLIDSignalAddress = async (id: string): Promise<string> => {
|
// so call sites below stay readable.
|
||||||
if (id.includes('.')) {
|
const resolveAddr = (id: string) => resolveLIDSignalAddress(id, lidMapping)
|
||||||
const [deviceId, device] = id.split('.')
|
|
||||||
if (!deviceId) {
|
|
||||||
throw new Error('Missing device ID')
|
|
||||||
}
|
|
||||||
|
|
||||||
const [user, domainType_] = deviceId.split('_')
|
|
||||||
const domainType = parseInt(domainType_ || '0')
|
|
||||||
|
|
||||||
if (domainType === WAJIDDomains.LID || domainType === WAJIDDomains.HOSTED_LID) return id
|
|
||||||
|
|
||||||
const pnJid = `${user!}${device !== '0' ? `:${device}` : ''}@${domainType === WAJIDDomains.HOSTED ? 'hosted' : 's.whatsapp.net'}`
|
|
||||||
|
|
||||||
const lidForPN = await lidMapping.getLIDForPN(pnJid)
|
|
||||||
if (lidForPN) {
|
|
||||||
const lidAddr = jidToSignalProtocolAddress(lidForPN)
|
|
||||||
return lidAddr.toString()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return id
|
|
||||||
}
|
|
||||||
|
|
||||||
// Delayed PreKey deletion: grace period to handle race conditions
|
// Delayed PreKey deletion: grace period to handle race conditions
|
||||||
// where two pkmsg with the same preKeyId arrive nearly simultaneously.
|
// where two pkmsg with the same preKeyId arrive nearly simultaneously.
|
||||||
@@ -785,7 +808,7 @@ function signalStorage(
|
|||||||
return {
|
return {
|
||||||
loadSession: async (id: string) => {
|
loadSession: async (id: string) => {
|
||||||
try {
|
try {
|
||||||
const wireJid = await resolveLIDSignalAddress(id)
|
const wireJid = await resolveAddr(id)
|
||||||
const { [wireJid]: sess } = await keys.get('session', [wireJid])
|
const { [wireJid]: sess } = await keys.get('session', [wireJid])
|
||||||
|
|
||||||
if (sess) {
|
if (sess) {
|
||||||
@@ -798,7 +821,7 @@ function signalStorage(
|
|||||||
return null
|
return null
|
||||||
},
|
},
|
||||||
storeSession: async (id: string, session: libsignal.SessionRecord) => {
|
storeSession: async (id: string, session: libsignal.SessionRecord) => {
|
||||||
const wireJid = await resolveLIDSignalAddress(id)
|
const wireJid = await resolveAddr(id)
|
||||||
await keys.set({ session: { [wireJid]: session.serialize() } })
|
await keys.set({ session: { [wireJid]: session.serialize() } })
|
||||||
},
|
},
|
||||||
isTrustedIdentity: () => {
|
isTrustedIdentity: () => {
|
||||||
@@ -875,7 +898,7 @@ function signalStorage(
|
|||||||
const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'load' })
|
const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'load' })
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const wireJid = await resolveLIDSignalAddress(id)
|
const wireJid = await resolveAddr(id)
|
||||||
|
|
||||||
// Check cache first
|
// Check cache first
|
||||||
const cached = identityKeyCache.get(wireJid)
|
const cached = identityKeyCache.get(wireJid)
|
||||||
@@ -909,7 +932,7 @@ function signalStorage(
|
|||||||
const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'save' })
|
const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'save' })
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const wireJid = await resolveLIDSignalAddress(id)
|
const wireJid = await resolveAddr(id)
|
||||||
const currentFingerprint = generateKeyFingerprint(identityKey)
|
const currentFingerprint = generateKeyFingerprint(identityKey)
|
||||||
|
|
||||||
// Load existing key (from cache or storage)
|
// Load existing key (from cache or storage)
|
||||||
|
|||||||
Reference in New Issue
Block a user