fix: self-DM stops working, WA Web compliance for fanout and BadMac - #634
Merged
Conversation
In a self-DM the fanout merged `recipient_cached` (LID, from `recipient_bare`) with `own_cached` (PN, from `device_snapshot.pn`). Both lookups resolve to the same backend record but project it in different namespaces, and `sort_dedup_by_device` keys on `user` so it cannot collapse LID/PN copies of the same physical device. Each companion device ended up in the stanza twice and the server replied `ack error="400"`, blocking the bot's response. Mirror `WAWebDBDeviceListFanout.getFanOutList`: when the recipient is the user themselves, skip the own-device lookup; `recipient_cached` already lists every device in a single addressing mode.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAvoid duplicate device fanout for self-directed messages via ChangesSelf-DM Device Fanout Correction
Signal Session Preservation & Tests
Sequence Diagram(s)sequenceDiagram
participant Client
participant process_session_enc_batch
participant signal_cache
participant handle_decrypt_failure
participant retry_receipt_spawner
Client->>process_session_enc_batch: deliver ciphertext (pkmsg or msg)
process_session_enc_batch->>signal_cache: lookup session (do not delete on BadMac/InvalidMessage)
signal_cache-->>process_session_enc_batch: session (preserved)
process_session_enc_batch->>handle_decrypt_failure: call(RetryReason, label, msg) on decrypt failure
handle_decrypt_failure->>retry_receipt_spawner: spawn retry receipt
handle_decrypt_failure-->>process_session_enc_batch: dispatch UndecryptableMessage event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Note: verify tests run cleanly and session archival path still archives previous session state after the preserved session is later replaced. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff148bbe5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
WAWebMsgProcessingDecryptionHandler classifies every SignalDecryptionError that isn't `errDuplicateMsg` (BadMac, InvalidMessage, NoSession, BadKey, FutureMessage, ...) as `SignalRetryable` -> `sendRetryReceipt`, with no session manipulation. The only sites where WA Web touches the session are reg-id mismatch and same-base-key on the OUTGOING retry path (WAWebUpdateLocalSignalSession), both already mirrored in `src/retry.rs`. Our inbound branch in `src/message.rs` was deleting the session on BadMac/InvalidMessage. When the sender's next retry finally landed as pkmsg, `process_prekey_bundle` ran against an empty record, so `archive_current_state_inner` had nothing to archive and `previous_sessions[0]` stayed empty. Any other in-flight messages from the old ratchet then failed permanently — exactly the loop observed in the user's prod log (`AC724...` retry-thrashing across reconnects). Empirical state on the user's prod DB (1703 sessions): 95.1% had `previous_sessions = 0`; the device-0 session for the user's own account was a fresh shell with no archived state. Fix: drop the `signal_cache.delete_session(...)` call on the BadMac/InvalidMessage arm. When the sender retries as pkmsg, `promote_state` archives the stale current session into `previous_sessions[0]`, which is what libsignal's `decrypt_message_with_record` walks for in-flight backlog. Tests: - `test_badmac_preserves_session`: real session, tampered MAC -> BadMac branch; session is still in cache afterwards. - `test_invalid_message_preserves_session`: same invariant on the InvalidMessage arm of the matches!() block. - `test_prod_scenario_pkmsg_archives_old_session_after_badmac`: end-to-end repro of the prod loop -- v1 session established, BadMac on a tampered message preserves it, fresh X3DH (pkmsg_v2) archives session_v1 into previous_sessions[0] with the original base key intact.
jlucaso1
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e53ffb1bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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/message.rs`:
- Around line 2449-2545: Add an assertion in test_badmac_preserves_session that
the retry path ran by checking message_retry_counts for the message's cache key
after calling process_session_enc_batch: compute the same cache_key used by the
retry logic (matching how the code derives it in process_session_enc_batch),
then call client.persistence_manager.message_retry_counts.get(&cache_key).await
and assert it equals Some(1); if the retry task is spawned asynchronously, wait
briefly or poll until the count becomes Some(1) before asserting so the test
doesn't race. Ensure you reference the same cache_key generation logic and use
the client.message_retry_counts (or persistence manager) APIs rather than
duplicating internal state.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3a8686ea-fbe2-437d-844c-e9f2fe0fc9c9
📒 Files selected for processing (1)
src/message.rs
Two targeted follow-ups on PR #634. src/send.rs — Codex P2: restore own-device fallback for self-DM when recipient_cached is None Only skip the own_cached lookup in self-DM when recipient_cached actually contains data. If the recipient lookup misses (cache eviction + warmup failed) we still need the PN-keyed own_cached as a fallback, otherwise the fanout collapses to the bare-JID path and drops every companion device. Pinned with the new `self_dm_with_no_recipient_cache_still_appends_own_devices` test. src/message.rs — CodeRabbit critical: discriminate BadMac branch from the parse-error path Session preservation alone is insensitive: the parse-error path also dispatches an undecryptable event and keeps the session, so the previous assertions would pass even if the BadMac arm regressed. Added an `await_retry_count_eq` helper that polls `message_retry_counts` (only bumped via `handle_decrypt_failure -> spawn_retry_receipt`) so the three tests now prove they took the BadMac/InvalidMessage arm. The byte-tamper approach in `test_badmac_preserves_session` and `test_prod_scenario_pkmsg_archives_old_session_after_badmac` was silently hitting the pkmsg parse-error branch (the trailing byte of a `PreKeySignalMessage` lives inside a tail varint, not the embedded MAC). Both tests now clear Alice's unacknowledged-pkmsg flag before the tamper so her next encrypt is a plain `SignalMessage`, whose final byte is squarely in the MAC region. Tests: - 519 unit tests pass (was 518; +1 send-side fallback test) - cargo fmt / clippy clean
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124c03ad09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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/message.rs`:
- Line 1049: Remove the blocking eprintln! in the decrypt error path and route
the message through the project's logging framework instead: replace
eprintln!("DEBUG decrypt err for msg:{}: {:?}", info.id, e) with a log macro
(e.g., debug! or warn!) that includes info.id and the error (e) so the message
is non-blocking and respects log levels; locate the statement in the
decrypt/error handling code (the place referencing info.id and e) and use debug!
or warn! with the same formatted text to preserve context.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ddfaba72-9d48-45c5-a68f-1aa1f67035b1
📒 Files selected for processing (2)
src/message.rssrc/send.rs
Leftover debug print from the diagnostic that surfaced the pkmsg-tamper bug in the BadMac tests. Bypasses the `log` pipeline (no level gating, formatting, redaction) and would spam stderr in production failure bursts — exactly the scenario this branch targets. Routed nothing else because the surrounding code already logs through `log::warn!` / `debug!`.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/message.rs`:
- Around line 2348-2374: The test is mutating Device state directly via
device_guard.store_prekey(...) and reading via device_arc.read(), bypassing the
production mutation/read paths; replace the direct store and read with the
PersistenceManager APIs by creating the appropriate DeviceCommand (e.g.,
DeviceCommand::StorePreKey with pk_id_u32 and pk_struct) and calling
client.persistence_manager.process_command(command). For reads, call
client.persistence_manager.get_device_snapshot().await (or the provided
get_device_snapshot helper) to obtain device state instead of
device_arc.read().await, and remove direct calls to store_prekey and direct
access to device_guard so the test exercises PersistenceManager::process_command
and the snapshot path.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: daf8f314-4553-4925-9ea2-4841b0570717
📒 Files selected for processing (1)
src/message.rs
- Route the synthetic prekey for `bobs_prekey_bundle` through `client.signal_adapter()` (`PreKeyStore::save_pre_key` / `SignedPreKeyStore::get_signed_pre_key`) instead of poking `device_guard.store_prekey(...)` directly. This is the same trait surface production goes through in `signal_adapter.rs:212`. - Drop `test_invalid_message_preserves_session`. The construction (`SignalMessage::new` with random keys) ends up triggering libsignal's BadMac path, not InvalidMessage, because any candidate state can still derive a message key off the random ratchet and then fail MAC verification. Both errors share the same `matches!()` arm in `process_session_enc_batch`, so the test was indistinguishable from `test_badmac_preserves_session` and the docstring was misleading. Left a comment explaining why the InvalidMessage arm doesn't get a dedicated test (genuinely triggering it requires session-state scaffolding far beyond what tests should own). Tests: 518 passed (was 519; -1 deleted), clippy clean.
jlucaso1
changed the title
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/message.rs`:
- Around line 2171-2180: The test must assert the concrete retry reason is
RetryReason::NoSession rather than just that the session still exists; add an
assertion that the recorded resend/retry reason equals RetryReason::NoSession
(e.g. check the value returned from whatever records retry reasons in this test
— the resend tracker or the function that produced the retry result) after the
decrypt failure; use the existing symbols like RetryReason::NoSession,
signal_cache.has_session, signal_address and
client.persistence_manager.backend() to locate the relevant spot and add a
strict equality/assertion for the retry reason so regressions to BadMac or
InvalidMessage will fail the test.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c4540c4-6797-4f0c-8e35-e453f6fde1bd
📒 Files selected for processing (1)
src/message.rs
Codex/CodeRabbit flagged that the session-preservation assertions can't tell a NoSession path apart from a BadMac/InvalidMessage path after the fix (both preserve the session, both bump `message_retry_counts`). Counting alone is also not enough to rule out a hypothetical regression that takes a different arm of `process_session_enc_batch`. Adds a small parallel cache `recent_retry_reasons: Cache<String, RetryReason>` populated atomically alongside the count inside `increment_retry_count`. It exposes the same key shape as `message_retry_counts`, so tests can read it with `make_retry_cache_key` without any new plumbing. Doubles as a tiny diagnostic surface in prod (last retry reason per message), capped at the same TTL/size as the counter cache. - `await_retry_receipt(count, reason)` replaces `await_retry_count_eq`. - `test_empty_session_record_treated_as_session_not_found` now asserts `RetryReason::NoSession` (was previously indistinguishable from BadMac post-fix). - `test_badmac_preserves_session` and `test_prod_scenario_pkmsg_archives_old_session_after_badmac` now assert `RetryReason::BadMac`. Tests: 518 passed (3 runs, stable), clippy clean.
oonid
pushed a commit
to oonid/whatsapp-rust-sqlx
that referenced
this pull request
May 21, 2026
This was referenced May 28, 2026
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
1 participant
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.
Summary
Two related WA Web compliance bugs that combined to break self-DM delivery. Outgoing replies were rejected with
ack error="400"because of duplicate participants, and on the inbound side anyBadMacratchet desync wiped the session so the in-flight backlog could never recover.Verified live against WhatsApp: self-DM replies now return
<ack>withouterror, andDeleted stale sessionno longer appears in the runtime log.Bug 1: duplicate self-DM device fanout
src/send.rsbuilt the fanout list by concatenatingrecipient_cached(LID form devices fromrecipient_bare) withown_cached(PN form devices fromdevice_snapshot.pn). For a self-DM both lookups hit the same backendDeviceListRecordbut project it in different namespaces, andsort_dedup_by_devicecompares(user, server, agent, device): LID and PN users differ so duplicates were never collapsed. Every companion device appeared in<participants>twice and the server returnedack error="400".Fix: when the recipient is the user themselves, skip the second lookup.
recipient_cachedalready lists every own device in a single addressing mode, matchingWAWebDBDeviceListFanout.getFanOutList. The skip is gated onrecipient_cached.is_some()so the existing own-device fallback still kicks in when the recipient lookup misses.Bug 2: session deleted on inbound BadMac/InvalidMessage
src/message.rswas callingsignal_cache.delete_session(...)on every inboundBadMacorInvalidMessage. WA Web'sWAWebMsgProcessingDecryptionHandlerclassifies everySignalDecryptionErrorother thanerrDuplicateMsgasSignalRetryableand routes it straight toWAWebSendRetryReceiptJob.sendRetryReceiptwith no session manipulation. The only places WA Web touches a session are reg-id mismatch and same-base-key collision on the outgoing retry path (WAWebUpdateLocalSignalSession), and both are already mirrored insrc/retry.rs.After our delete, when the sender's next retry arrived as pkmsg,
process_prekey_bundleran against an empty record soarchive_current_state_innerhad nothing to archive.previous_sessions[0]stayed empty and any other in-flight messages on the old ratchet failed permanently. That matched the loop seen in the user's prod log (same message id retry-thrashing across reconnects, 28MAC verification failed/Deleted stale sessionpairs in a 5h window).Empirical state on the user's prod SQLite (1703 sessions): 95.1% had
previous_sessions = 0; the device-0 entry for the user's own account was a fresh shell with no archived state.Fix: drop the
delete_sessioncall. The retry receipt path stays. When the sender retries as pkmsg, libsignal'spromote_statearchives the stale current session intoprevious_sessions[0]anddecrypt_message_with_recordwalks it for in-flight backlog.Test plan
cargo fmt --allcargo clippy -p whatsapp-rust --tests(clean)cargo test -p whatsapp-rust --lib(518 passed)src/send.rs:self_dm_lid_recipient_matches_own_lidself_dm_pn_recipient_matches_own_pnself_dm_pn_recipient_self_dm_even_without_own_lidnon_self_lid_recipient_is_not_self_dmlid_recipient_without_own_lid_is_not_self_dmgroup_or_broadcast_recipient_is_not_self_dmself_dm_with_no_recipient_cache_still_appends_own_devices(own-cache fallback)old_merge_produced_lid_pn_duplicates_for_self_dm(regression pin: 2N to N entries)src/message.rs:test_badmac_preserves_session: real session, tampered MAC, BadMac branch, session still in cache,message_retry_countsbumped to discriminate from the parse-error path.test_prod_scenario_pkmsg_archives_old_session_after_badmac: end-to-end repro. v1 session, BadMac preserves it, fresh X3DH archives v1 intoprevious_sessions[0]with the original base key intact.k8awqjsgww2lnkt89urp3de1: self-DM ping/pong round-trip succeeds, server returns<ack>withouterror, no recurringDeleted stale sessionlines.