fix: decrypt poll votes across PN and LID parent authors - #230
Open
digaovaa wants to merge 3 commits into
Open
Conversation
Poll voters encrypt with the creator JID from pollCreationMessageKey, which is typically our LID after the addressing migration. Outgoing secrets were persisted under meJid (PN), so tryDecryptAddon derived the wrong HKDF info and AES-GCM failed with "unable to authenticate data". tryDecryptAddon now prefers the parent author from the target key (same as whatsmeow getOrigSenderFromKey) and retries stored/PN/LID sender candidates on auth failure. New polls also persist the secret under meLid when available so the common path matches what peers encrypt with.
📝 WalkthroughWalkthroughAddon decryption now resolves multiple normalized sender candidates and retries aligned sender pairs after authentication failures. Outgoing message secrets now persist a valid LID, normalized JID, or empty sender value. Tests cover resolution, normalization, fallback, and persistence. ChangesAddon sender resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WaMessageCoordinator
participant addon-crypto
participant AddonDecryption
WaMessageCoordinator->>addon-crypto: Collect sender candidates
addon-crypto->>AddonDecryption: Retry aligned sender pairs
AddonDecryption-->>addon-crypto: Return plaintext or authentication failure
addon-crypto-->>WaMessageCoordinator: Return plaintext or final error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/message/crypto/__tests__/addon-crypto.test.ts (1)
290-335: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a modification-sender retry case.
This test retries only
parentMsgOriginalSenderCandidates. Add a case where the first modification sender is PN and the second is LID.modificationSenderCandidatesaffect both secret derivation and poll-vote additional authenticated data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/message/crypto/__tests__/addon-crypto.test.ts` around lines 290 - 335, Add a second test for decryptAddonPayloadWithSenderFallback where parent sender candidates remain valid, but modificationSenderCandidates contains PN first and LID second; encrypt the poll-vote payload using the LID sender, verify direct decryption with PN rejects, then verify fallback recovers the plaintext and covers both secret derivation and poll-vote authenticated data.src/message/crypto/addon-crypto.ts (1)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExpose the deduped JID list as readonly.
collectUniqueUserJidsproduces an immutable candidate list from the caller perspective, so returnreadonly string[]instead ofstring[]to match the guidelines and preserve array invariants for future call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/message/crypto/addon-crypto.ts` around lines 52 - 54, Update the return type of collectUniqueUserJids from string[] to readonly string[], preserving its existing deduplication behavior and implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/coordinators/WaMessageDispatchCoordinator.ts`:
- Around line 603-612: Update the senderForSecret construction near
messageSecretStore.set to catch malformed credentials.meLid and fall back to
normalizing credentials.meJid. Preserve the existing preference for a valid LID,
persist the normalized JID when the LID is invalid, and use the existing empty
sender value when both credentials are invalid.
In `@src/message/crypto/addon-crypto.ts`:
- Around line 79-93: Update resolveAddonParentSenderFromKey to validate the
selected modificationSender, participant, or remoteJid through
collectUniqueUserJids before returning it, preserving the existing branch
precedence and trimming behavior. Return the validated JID when available,
otherwise null, so malformed target-key values do not throw and allow
tryDecryptAddon to use parentEntry.senderJid as fallback.
---
Nitpick comments:
In `@src/message/crypto/__tests__/addon-crypto.test.ts`:
- Around line 290-335: Add a second test for
decryptAddonPayloadWithSenderFallback where parent sender candidates remain
valid, but modificationSenderCandidates contains PN first and LID second;
encrypt the poll-vote payload using the LID sender, verify direct decryption
with PN rejects, then verify fallback recovers the plaintext and covers both
secret derivation and poll-vote authenticated data.
In `@src/message/crypto/addon-crypto.ts`:
- Around line 52-54: Update the return type of collectUniqueUserJids from
string[] to readonly string[], preserving its existing deduplication behavior
and implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72ec6c62-ef09-464e-9a60-c270e9dbd05b
📒 Files selected for processing (4)
src/client/coordinators/WaMessageCoordinator.tssrc/client/coordinators/WaMessageDispatchCoordinator.tssrc/message/crypto/__tests__/addon-crypto.test.tssrc/message/crypto/addon-crypto.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
vinikjkkj
requested changes
Aug 11, 2026
vinikjkkj
left a comment
vinikjkkj
left a comment
Owner
There was a problem hiding this comment.
Validated against wa-web.
- Premise confirmed – the encrypt path normalizes both JIDs to LID, so peers bind our LID into the HKDF and persisting under
meLidis right. Rebuilding the AAD per attempt is correct too. - Blocking: unguarded
toUserJid–resolveAddonParentSenderFromKeycalls it directly and the call site doesn't catch, so a malformedpollCreationMessageKeythrows before the first attempt and loses a vote thatparentEntry.senderJidalone would have decrypted.collectUniqueUserJidsalready wraps it in try/catch; returning the raw JID would fix it. - Fallback shape –
wa-web'sdecryptAddOnis three rungs that transform both sides together (both→LID, both→PN, both as-is) and never builds a mixed pair deliberately. The cross product here is 2 × up to 7 = 14 attempts, each with its own HKDF + GCM, with the mixed pairs landing before the PN pair. Pairing by addressing mode gives 3 worst case and still covers the asymmetric peer on the as-is rung. - Minor – in groups
attrs.fromis the group JID, andcollectUniqueUserJidsonly checks theuser@servershape, so it enters as a sender candidate that can never succeed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/coordinators/WaMessageDispatchCoordinator.ts`:
- Around line 1495-1508: Update the meLid handling near the parent-sender
resolution logic in WaMessageDispatchCoordinator.ts: normalize meLid, return it
only when isLidJid() confirms it is a LID, and otherwise fall back to meJid. In
src/client/coordinators/__tests__/coordinators.test.ts lines 243-271, add
coverage for a valid PN-shaped meLid differing from meJid and assert the
normalized meJid is returned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 745dad95-f061-47aa-9ad4-432dbb5ff8ec
📒 Files selected for processing (6)
.gitignoresrc/client/coordinators/WaMessageCoordinator.tssrc/client/coordinators/WaMessageDispatchCoordinator.tssrc/client/coordinators/__tests__/coordinators.test.tssrc/message/crypto/__tests__/addon-crypto.test.tssrc/message/crypto/addon-crypto.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/coordinators/WaMessageCoordinator.ts
Comment on lines
+1495
to
+1508
| if (meLid && meLid.includes('@')) { | ||
| try { | ||
| return toUserJid(meLid) | ||
| } catch (error) { | ||
| this.deps.logger.trace('ignoring malformed me lid jid', { | ||
| meLid, | ||
| message: toError(error).message | ||
| }) | ||
| } | ||
| } | ||
| const meJid = credentials?.meJid | ||
| if (meJid && meJid.includes('@')) { | ||
| try { | ||
| return toUserJid(meJid) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a LID before preferring meLid.
toUserJid() accepts PN JIDs. A PN-shaped credentials.meLid can therefore win over
credentials.meJid and persist the wrong parent sender. Later addon decryption can then
lack the correct stored sender fallback.
src/client/coordinators/WaMessageDispatchCoordinator.ts#L1495-L1508: Normalize
meLid, then return it only whenisLidJid()is true. Otherwise fall back tomeJid.src/client/coordinators/__tests__/coordinators.test.ts#L243-L271: Add a case where
meLidis a valid PN JID that differs frommeJid, and assert that the normalized
meJidis returned.
📍 Affects 2 files
src/client/coordinators/WaMessageDispatchCoordinator.ts#L1495-L1508(this comment)src/client/coordinators/__tests__/coordinators.test.ts#L243-L271
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/coordinators/WaMessageDispatchCoordinator.ts` around lines 1495 -
1508, Update the meLid handling near the parent-sender resolution logic in
WaMessageDispatchCoordinator.ts: normalize meLid, return it only when isLidJid()
confirms it is a LID, and otherwise fall back to meJid. In
src/client/coordinators/__tests__/coordinators.test.ts lines 243-271, add
coverage for a valid PN-shaped meLid differing from meJid and assert the
normalized meJid is returned.
There was a problem hiding this comment.
4 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".gitignore">
<violation number="1" location=".gitignore:19">
P3: Adding `/docs/` to `.gitignore` is unrelated to this decrypt fix and would silently hide any future top-level docs directory. The repo currently keeps all documentation at the root (README.md, AGENTS.md, CLAUDE.md) and no `docs/` path exists or is tracked, so consider dropping this line from this PR or documenting why `docs/` should be ignored.</violation>
</file>
<file name="src/message/crypto/addon-crypto.ts">
<violation number="1" location="src/message/crypto/addon-crypto.ts:106">
P2: Hosted-device messages can now lose addon decryption because valid `@hosted`/`@hosted.lid` candidates are discarded before pairing. Hosted variants should be classified with their PN/LID rung (while preserving the exact JID used for HKDF) or canonicalized consistently before this filter.</violation>
<violation number="2" location="src/message/crypto/addon-crypto.ts:129">
P2: The paired fallback can miss a valid sender combination that the previous cross-product would have recovered. `buildAddonSenderPairs` only produces (LID,LID), (PN,PN), and the 'first-of-each' rung; the third is deduped away whenever the first parent and first modifier are both LID. So with parents = [Lid, Pn] and modifiers = [Lid, Pn] where the real encryption rung is (Lid parent, Pn modifier), all three generated rungs fail authentication and the vote is dropped, because the mixed rung is never built. Consider adding an explicit mixed rung (LID parent + PN modifier, and optionally PN parent + LID modifier) so a valid combination is still attempted after the principal LID rung fails.</violation>
</file>
<file name="src/client/coordinators/WaMessageDispatchCoordinator.ts">
<violation number="1" location="src/client/coordinators/WaMessageDispatchCoordinator.ts:1495">
P2: resolveOutgoingSecretSenderJid only checks that `meLid` contains '@' before normalizing and returning it, without verifying it is actually a LID JID (e.g. via `isLidJid`). If `credentials.meLid` is ever populated with a PN-shaped value, this will persist the PN as the parent sender instead of falling back to `meJid`, which can cause the stored sender fallback used during addon decryption to be wrong. Consider normalizing `meLid` first and only returning it when `isLidJid()` is true, falling back to `meJid` otherwise.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| readonly parentCandidates: readonly string[] | ||
| readonly modificationCandidates: readonly string[] | ||
| }): readonly WaAddonSenderPair[] { | ||
| const parents = input.parentCandidates.filter((jid) => isLidJid(jid) || isUserJid(jid)) |
There was a problem hiding this comment.
P2: Hosted-device messages can now lose addon decryption because valid @hosted/@hosted.lid candidates are discarded before pairing. Hosted variants should be classified with their PN/LID rung (while preserving the exact JID used for HKDF) or canonicalized consistently before this filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/message/crypto/addon-crypto.ts, line 106:
<comment>Hosted-device messages can now lose addon decryption because valid `@hosted`/`@hosted.lid` candidates are discarded before pairing. Hosted variants should be classified with their PN/LID rung (while preserving the exact JID used for HKDF) or canonicalized consistently before this filter.</comment>
<file context>
@@ -81,21 +81,55 @@ export function resolveAddonParentSenderFromKey(
+ readonly parentCandidates: readonly string[]
+ readonly modificationCandidates: readonly string[]
+}): readonly WaAddonSenderPair[] {
+ const parents = input.parentCandidates.filter((jid) => isLidJid(jid) || isUserJid(jid))
+ const modifiers = input.modificationCandidates.filter((jid) => isLidJid(jid) || isUserJid(jid))
+ const pairs: WaAddonSenderPair[] = []
</file context>
| parents.find((jid) => isUserJid(jid)), | ||
| modifiers.find((jid) => isUserJid(jid)) | ||
| ) | ||
| addPair(parents[0], modifiers[0]) |
There was a problem hiding this comment.
P2: The paired fallback can miss a valid sender combination that the previous cross-product would have recovered. buildAddonSenderPairs only produces (LID,LID), (PN,PN), and the 'first-of-each' rung; the third is deduped away whenever the first parent and first modifier are both LID. So with parents = [Lid, Pn] and modifiers = [Lid, Pn] where the real encryption rung is (Lid parent, Pn modifier), all three generated rungs fail authentication and the vote is dropped, because the mixed rung is never built. Consider adding an explicit mixed rung (LID parent + PN modifier, and optionally PN parent + LID modifier) so a valid combination is still attempted after the principal LID rung fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/message/crypto/addon-crypto.ts, line 129:
<comment>The paired fallback can miss a valid sender combination that the previous cross-product would have recovered. `buildAddonSenderPairs` only produces (LID,LID), (PN,PN), and the 'first-of-each' rung; the third is deduped away whenever the first parent and first modifier are both LID. So with parents = [Lid, Pn] and modifiers = [Lid, Pn] where the real encryption rung is (Lid parent, Pn modifier), all three generated rungs fail authentication and the vote is dropped, because the mixed rung is never built. Consider adding an explicit mixed rung (LID parent + PN modifier, and optionally PN parent + LID modifier) so a valid combination is still attempted after the principal LID rung fails.</comment>
<file context>
@@ -81,21 +81,55 @@ export function resolveAddonParentSenderFromKey(
+ parents.find((jid) => isUserJid(jid)),
+ modifiers.find((jid) => isUserJid(jid))
+ )
+ addPair(parents[0], modifiers[0])
+ return pairs
+}
</file context>
| private resolveOutgoingSecretSenderJid(): string { | ||
| const credentials = this.deps.getCurrentCredentials() | ||
| const meLid = credentials?.meLid | ||
| if (meLid && meLid.includes('@')) { |
There was a problem hiding this comment.
P2: resolveOutgoingSecretSenderJid only checks that meLid contains '@' before normalizing and returning it, without verifying it is actually a LID JID (e.g. via isLidJid). If credentials.meLid is ever populated with a PN-shaped value, this will persist the PN as the parent sender instead of falling back to meJid, which can cause the stored sender fallback used during addon decryption to be wrong. Consider normalizing meLid first and only returning it when isLidJid() is true, falling back to meJid otherwise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/coordinators/WaMessageDispatchCoordinator.ts, line 1495:
<comment>resolveOutgoingSecretSenderJid only checks that `meLid` contains '@' before normalizing and returning it, without verifying it is actually a LID JID (e.g. via `isLidJid`). If `credentials.meLid` is ever populated with a PN-shaped value, this will persist the PN as the parent sender instead of falling back to `meJid`, which can cause the stored sender fallback used during addon decryption to be wrong. Consider normalizing `meLid` first and only returning it when `isLidJid()` is true, falling back to `meJid` otherwise.</comment>
<file context>
@@ -1487,6 +1482,40 @@ export class WaMessageDispatchCoordinator {
+ private resolveOutgoingSecretSenderJid(): string {
+ const credentials = this.deps.getCurrentCredentials()
+ const meLid = credentials?.meLid
+ if (meLid && meLid.includes('@')) {
+ try {
+ return toUserJid(meLid)
</file context>
| *.log | ||
| ___* No newline at end of file | ||
| ___* | ||
| /docs/ |
There was a problem hiding this comment.
P3: Adding /docs/ to .gitignore is unrelated to this decrypt fix and would silently hide any future top-level docs directory. The repo currently keeps all documentation at the root (README.md, AGENTS.md, CLAUDE.md) and no docs/ path exists or is tracked, so consider dropping this line from this PR or documenting why docs/ should be ignored.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .gitignore, line 19:
<comment>Adding `/docs/` to `.gitignore` is unrelated to this decrypt fix and would silently hide any future top-level docs directory. The repo currently keeps all documentation at the root (README.md, AGENTS.md, CLAUDE.md) and no `docs/` path exists or is tracked, so consider dropping this line from this PR or documenting why `docs/` should be ignored.</comment>
<file context>
@@ -15,4 +15,5 @@ bench-profiles-*/
-___*
\ No newline at end of file
+___*
+/docs/
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Poll voters encrypt with the creator JID from pollCreationMessageKey, which is typically our LID after the addressing migration. Outgoing secrets were persisted under meJid (PN), so tryDecryptAddon derived the wrong HKDF info and AES-GCM failed with "unable to authenticate data".
tryDecryptAddon now prefers the parent author from the target key (same as whatsmeow getOrigSenderFromKey) and retries stored/PN/LID sender candidates on auth failure. New polls also persist the secret under meLid when available so the common path matches what peers encrypt with.
Summary by CodeRabbit
Bug Fixes
Tests