feat(msmsg): decrypt Meta AI / fbid bot replies (<enc type="msmsg">) - #650
Conversation
Required for Meta AI / fbid bot reply decryption (`<enc type="msmsg">`). WA Web `WAWebBotMessageSecret.decryptMsmsgBotMessage` looks the secret up by (chat, target_sender, target_id) where target_id is the id of our original outbound stanza. whatsmeow does the same via `cli.Store.MsgSecrets`. - `MsgSecretStore` trait in wacore (added to Backend composition). - InMemoryBackend impl with composite-key map. - SqliteStore impl backed by new `msg_secrets` table + Diesel migration, scoped by device_id for multi-account isolation. - Tests: round-trip, miss returns None, composite-key independence, upsert overwrites, multi-device isolation.
Implements the dual-HKDF + AES-256-GCM open used by Meta AI / fbid bot
replies. Cross-verified against:
WA Web `WAWebBotMessageSecret.js`
k1 = HKDF-SHA256(messageSecret, info="Bot Message", L=32)
k2 = HKDF-SHA256(k1, info=msgId||target_user||bot_user, L=32)
AAD = msgId || 0x00 || bot_user
whatsmeow `decryptBotMessage` / `generateMsgSecretKey` (msgsecret.go:42-160)
Identical info concat + AAD layout (modificationType="" for msmsg).
`BotMessageContext` exposes msg_id / target_sender_user_jid / bot_user_jid
so callers (Phase 5 inbound dispatch) can plug in `info.id` or
`bot_info.edit_target_id` for the edit chain.
Tests (13):
- HKDF determinism + secret-size validation
- Per-message key sensitivity to every input field
- AAD layout matches WA Web `msgId + "\0" + bot_user_jid`
- Encrypt/decrypt round-trip
- Reject tag tampering, ciphertext tampering, wrong secret, wrong msg_id,
wrong bot JID, short IV, short payload
- Known-input vector covering every binding (key + AAD) at once
Recognises `<enc type="msmsg">` as a distinct envelope (neither a Signal session message nor a sender-key group message). is_session() stays false; is_bot_secret() identifies the new variant. - `categorize_enc_nodes` now routes msmsg into a new `bot_enc` bucket so callers can dispatch to the bot_message decrypt path (Phase 5). - features::signal::decrypt_message rejects EncType::MessageSecret with a clear error pointing at the bot_message helper. - Fixed existing unknown-enc tests that piggy-backed on "msmsg" as an example of an unknown type -- switched them to "frskmsg" so the semantics survive the recognition change. Tests: - categorize_msmsg_goes_into_bot_bucket (new) - enc_type_msmsg_round_trip: from_wire/as_wire_str/is_session/is_bot_secret
…send `send_message_impl` now calls `persist_outbound_msg_secret` after the stanza lands on the wire when `message.message_context_info.message_secret` is set. Key shape `(chat_non_ad, own_pn_non_ad, request_id)` matches the inbound lookup (Phase 5) which derives target_sender from `<meta target_sender_jid>` defaulting to our PN. No alloc of the secret -- the helper reads it directly from the message ref after send; only the request_id is cloned because the branch builders move it. Tests (4): - writes under (chat, ownPN, id) with the exact non-AD form - skips when MessageContextInfo.message_secret is absent - skips when get_pn() returns None - chat-with-device is stored under the non-AD chat form so the inbound lookup (which uses the bare chat from `<meta target_chat_jid>`) hits
`classify_incoming_message` now routes `<enc type="msmsg">` to a new
`handle_msmsg_payload` helper on Client. The helper:
1. Decodes the MessageSecretMessage proto (enc_iv, enc_payload).
2. Resolves target_sender from `<meta target_sender_jid>` (fallback to
our LID when the stanza sender is on the bot server, else our PN --
mirrors whatsmeow `decryptBotMessage`).
3. Looks up the stored secret by (target_chat or chat, target_sender,
target_id). Missing secret => nack 495.
4. Runs the dual-HKDF AES-256-GCM open via wacore::bot_message.
5. Decodes the plaintext as wa::Message and dispatches via
`dispatch_parsed_message` (delivery receipt fires from there).
Parser additions:
- `MsgMetaInfo.target_chat` field for `<meta target_chat_jid>`.
- `parse_message_info` populates target_id / target_sender / target_chat
from `<meta>`.
- `BotEditType::from_wire` and `parse_message_info` populates `bot_info`
from `<bot edit="..." edit_target_id="..." sender_timestamp_ms="..."/>`.
Tests (4):
- happy path: encrypt with the symmetric helper, route through classify,
observe Event::Message on the bus.
- missing stored secret: nack `error=495`, no Message event.
- tampered GCM tag: nack 495.
- meta without target_id: nack 495.
When `<bot edit="inner|last">` is present alongside `edit_target_id`, swap in that id as the HKDF input so the edited bot reply decrypts under the same per-message key as the message it edits. Mirrors whatsmeow's `decryptBotMessage` and WA Web `decryptMsmsgFbidBotMessage`. `first` and absent edit fall through to the stanza's own id. Tests (3): - edit=inner with edit_target_id → HKDF uses edit_target_id, dispatch works - no <bot> → HKDF stays on info.id, mismatched key fails GCM tag → nack 495 - edit=first → HKDF stays on info.id (must NOT swap)
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds msmsg support: storage trait + backends and DB migration, bot-message HKDF+AES-GCM crypto, wire parsing and classification for "msmsg", inbound msmsg decrypt/dispatch with NACK 495, and outbound message_secret capture and persistence. ChangesBot Message Secret Encryption & Persistence
Sequence Diagram: inbound msmsg flowsequenceDiagram
participant Wire as Stanza
participant Classify
participant Handler as handle_msmsg_payload
participant Store as MsgSecretStore
participant Crypto as bot_message
participant Dispatcher as EventBus
Wire->>Classify: receive stanza (enc type="msmsg")
Classify->>Classify: route payload -> bot_payloads
Classify->>Handler: call handle_msmsg_payload(payload)
Handler->>Store: get_msg_secret(chat,sender,msg_id)
Store-->>Handler: secret or None
alt secret present
Handler->>Crypto: decrypt_bot_message(secret, iv, payload, ctx)
Crypto-->>Handler: plaintext wa::Message
Handler->>Dispatcher: dispatch wa::Message
else missing or decrypt fails
Handler->>Wire: emit NACK (495 MissingMessageSecret / ParsingError)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
I expect this to work; review crypto, DB scoping, and NACK semantics carefully. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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: d3768daba9
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 746-754: The msmsg decrypt/dispatch is being offloaded to
self.outbound_flush which breaks inbound ordering; instead route msmsg through
the inbound ordering path by removing the outbound_flush.spawn call around
handle_msmsg_payload and either (a) acquire the per-chat message_enqueue_locks
(or appropriate session_locks for sender) and call
client.handle_msmsg_payload(&info_arc, payload).await inline so it runs under
the inbound enqueue lock, or (b) if you must spawn, acquire the same
message_enqueue_locks before spawning so the handler executes serialized with
other incoming messages; update the code around outbound_flush,
handle_msmsg_payload, message_enqueue_locks and session_locks accordingly.
- Around line 172-210: The lookup uses the raw resolved target_sender
(target_sender_str) but secrets were stored under the client's persistence key
(a different identity family), so normalize the sender to the same identity
family used when secrets are persisted before calling
persistence_manager.backend().get_msg_secret: take the target_sender from
resolve_msmsg_target_sender, convert it with the same helper used when storing
outbound secrets (the persistence key normalization function used elsewhere in
the codebase / the persistence manager's JID->key helper), and pass that
normalized key (instead of target_sender_str) into get_msg_secret (keep
chat_for_lookup and target_id unchanged).
- Around line 222-227: The Err(e) branch that handles backend read failure for
message_secret currently only logs and returns, leaving the recognized msmsg
stanza unacknowledged so it can be replayed; modify that branch (the Err(e)
block where info.id and msmsg are in scope) to send a terminal NACK for the
msmsg stanza using the existing nack/negative-acknowledgement mechanism used
elsewhere in this module (call the same function you use to terminal-nack other
msmsg stanzas), include the error details in the log, and then return so the
stanza is removed/terminally failed instead of being silently dropped.
In `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 2509-2553: The put_msg_secret implementation currently spawns a
blocking task directly and can surface transient SQLITE_BUSY/LOCKED errors;
change it to use the same retry/serialization helper (self.with_retry) used by
other SQLite writers so the insert_into(msg_secrets::table) +
on_conflict(...).do_update().set(...) execute path is retried/serialized.
Concretely: move the Diesel insert/update code (the closure that acquires
pool.get(), builds values for
msg_secrets::chat/sender/msg_id/secret/device_id/created_at and calls .execute)
into a closure passed to self.with_retry (ensuring you clone device_id, chat,
sender, msg_id, secret, now into the closure as done today) instead of calling
tokio::task::spawn_blocking directly; return and propagate errors via the
existing StoreError mapping so SQLITE_BUSY is retried rather than bubbled up.
- Around line 3257-3284: The test msg_secret_isolated_per_device_id is
incorrectly validating DB isolation because store_a and store_b use separate
in-memory DBs; change the test to use the same underlying DB connection while
differing only the device_id so the device_id filter is actually exercised.
Specifically, obtain a second Store that shares the same DB backend as store_a
(e.g., construct store_b from the same connection or provide a helper like
clone_with_db_and_device_id) then set store_b.device_id = store_a.device_id + 1,
call store_a.put_msg_secret("c","s","M",...) and assert
store_b.get_msg_secret("c","s","M").await.unwrap().is_none(); keep references to
create_test_store, device_id, put_msg_secret, and get_msg_secret to locate the
relevant code.
In `@wacore/src/types/message.rs`:
- Around line 158-176: Replace the manual string mapping with a WireEnum-driven
definition: derive WireEnum on BotEditType, add #[wire = "..."] attributes for
each variant's wire value (e.g., "first", "inner", "last"), remove
serde::Serialize from the enum per guideline, and reimplement from_wire as a
thin wrapper that calls BotEditType::try_from(s) (or maps the WireEnum try_from
result to Option) so the wire values are the single source of truth; update any
imports to bring WireEnum/try_from into scope and keep the public API
BotEditType and from_wire names unchanged.
🪄 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: a526c79b-02ab-4d03-a034-c85803982e16
📒 Files selected for processing (15)
src/appstate_sync.rssrc/features/signal.rssrc/message.rssrc/send.rsstorages/sqlite-storage/migrations/2026-05-28-000000_add_msg_secrets/down.sqlstorages/sqlite-storage/migrations/2026-05-28-000000_add_msg_secrets/up.sqlstorages/sqlite-storage/src/schema.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/bot_message.rswacore/src/lib.rswacore/src/message_processing.rswacore/src/messages.rswacore/src/store/in_memory.rswacore/src/store/traits.rswacore/src/types/message.rs
…identity, retries) 1. wacore-binary encoder: AD_JID was being emitted for any server with a `:device` suffix, even servers (bot/group/broadcast/newsletter/…) whose domain_type the decoder maps back to Pn, silently dropping the real server. Restrict AD_JID to the 4 servers the decoder round-trips (Pn/Lid/Hosted/HostedLid); everything else falls through to JID_PAIR, matching whatsmeow `writeJID` and WA Web `WAWap.De`. 2. send: capture the messageSecret that `prepare_dm/group_stanza` mints on its own clone of the message, not the caller's `&wa::Message` (which never has `message_context_info.message_secret` for ordinary sends). `PreparedDmStanza` / `PreparedGroupStanza` now carry an `Option<[u8; 32]>` so the post-send hook actually persists every outbound bot prompt. 3. msmsg ordering: route bot payloads through `process_classified_message` like session/group payloads instead of spawning on `outbound_flush`, so the handler runs under the same global semaphore + per-chat enqueue lock as the rest of the inbound pipeline. 4. msmsg identity: WA Web fbid path echoes our LID as `<meta target_sender_jid>`, but `persist_outbound_msg_secret` was storing every secret under our PN. For chats whose server is Bot, store under our LID so the inbound lookup hits. 5. msmsg backend error: terminal-nack with 495 when the SQLite read fails instead of returning silently (otherwise the server replays). 6. sqlite store: `put_msg_secret` now goes through `with_retry` for SQLITE_BUSY handling, matching the other writers. 7. wacore types: derive `WireEnum` on `BotEditType` so `"first"`/`"inner"`/ `"last"` are the single source of truth per AGENTS.md. Tests: - `test_bot_jid_with_device_round_trips_via_jid_pair` (encoder regression) - `prepared_dm_stanza_exposes_generated_message_secret` (P1 regression) - `persist_outbound_msg_secret_uses_lid_for_bot_chats` (identity) - `msmsg_outbound_put_and_inbound_get_match_for_lid_bot` (end-to-end LID) - `msmsg_with_bot_device_suffix_round_trips` (encoder + msmsg) - `msg_secret_isolated_per_device_id` rewritten to share the DB and only vary `device_id`, so the column filter is actually exercised.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/message.rs (1)
191-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate the persistence lookup key from the HKDF sender value.
This still breaks PN-tagged bot replies. Bot-chat secrets are persisted under the normalized bot-chat identity, but
get_msg_secret()is keyed with the rawtarget_sender_strfrom<meta>. When<meta target_sender_jid>comes back as our PN, the lookup misses and we nack a reply we could otherwise decrypt. Keeptarget_sender_strforBotMessageContext, but derive a second sender key for storage lookup using the same normalization path aspersist_outbound_msg_secret().🤖 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.rs` around lines 191 - 211, The persistence lookup is using the raw target_sender_str which fails for PN-tagged bot replies; keep target_sender_str for BotMessageContext but derive a second normalized sender key using the same normalization logic as persist_outbound_msg_secret() (i.e., the path used when storing bot-chat secrets) and use that normalized key in the call to get_msg_secret(&chat_for_lookup, &normalized_sender_key, target_id); update the code around target_sender_str, get_msg_secret, and BotMessageContext to pass the original target_sender_str to BotMessageContext but the normalized_sender_key to persistence_manager.backend().get_msg_secret so PN JIDs resolve to the stored secret.
🤖 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.
Duplicate comments:
In `@src/message.rs`:
- Around line 191-211: The persistence lookup is using the raw target_sender_str
which fails for PN-tagged bot replies; keep target_sender_str for
BotMessageContext but derive a second normalized sender key using the same
normalization logic as persist_outbound_msg_secret() (i.e., the path used when
storing bot-chat secrets) and use that normalized key in the call to
get_msg_secret(&chat_for_lookup, &normalized_sender_key, target_id); update the
code around target_sender_str, get_msg_secret, and BotMessageContext to pass the
original target_sender_str to BotMessageContext but the normalized_sender_key to
persistence_manager.backend().get_msg_secret so PN JIDs resolve to the stored
secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb64c243-fc51-4056-8816-ec9315371575
📒 Files selected for processing (6)
src/message.rssrc/send.rsstorages/sqlite-storage/src/sqlite_store.rswacore/binary/src/encoder.rswacore/src/send.rswacore/src/types/message.rs
WA Web `WAWebBotMessageSecret.js` has two dispatch arms:
* `h()` (fbid bot, Meta AI) pre-resolves the HKDF msg_id to either
`externalId` or `edit_target_id` based on edit_type, then makes a
single AES-GCM attempt.
* `f()` (regular bot) always tries `externalId` first, falls back to
`edit_target_id` on AES-GCM failure.
We don't have an `isFbidBot()` check, so unify: the fbid-style id is the
primary attempt and the OTHER id (if available) is the fallback. This is
a strict superset:
* INNER/LAST stanzas: primary = edit_target_id, fallback = info.id.
Matches fbid outcome on first try; covers the regular-path scenario
where the bot encrypted under `externalId` instead.
* Other stanzas: primary = info.id, fallback = edit_target_id (when the
parser populated it). Matches the regular path's first attempt.
`bot_info.edit_target_id` is still parsed only for INNER/LAST (matches
whatsmeow `parseMsgBotInfo`), so for non-edit stanzas there's no
fallback id — single attempt, nack 495 on failure.
Tests (2):
- `msmsg_falls_back_to_info_id_when_primary_uses_edit_target`: stanza
declares `<bot edit="inner" edit_target_id="...">` but the ciphertext
was minted under `info.id`. Primary tries the edit target, fails, and
the fallback succeeds.
- `msmsg_no_fallback_when_no_edit_target_present`: no `<bot>` node →
parser leaves `edit_target_id = None` → wrong key → no second attempt
→ nack 495 (single failure must not silently mask).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e601af018
ℹ️ 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 8251-8398: Add a new async test that mirrors
msmsg_falls_back_to_info_id_when_primary_uses_edit_target but exercises the
inverse branch: store the secret under edit_target_id (use
persistence_manager.backend().put_msg_secret with edit_target_id), set the
stanza id (info.id) to one value (e.g., REPLY_INV) and set the <bot edit
edit_target_id="EDIT_INV"> to another (edit_target_id), then encrypt the payload
under edit_target_id by constructing BotMessageContext with msg_id =
edit_target_id and call encrypt_bot_message/encode_message_secret_message (same
helpers used in the diff) so the primary attempt using info.id fails and the
fallback to edit_target_id succeeds; send the NodeBuilder message (include
meta.target_id/outbound id and bot edit as above) to
client.handle_incoming_message and assert you receive the decrypted message
(match on wacore::types::events::Event::Message where info.id == stanza id and
msg.conversation == "fallback ok" or similar) to confirm the info.id ->
edit_target_id fallback path is covered.
🪄 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: 40b34d26-e246-4acd-8a5a-ab42f22146e2
📒 Files selected for processing (1)
src/message.rs
…tionally P2 (Codex): LID-mode groups were persisting messageSecret under our PN because the helper branched on chat.server (Bot -> LID, else -> PN), but `prepare_group_stanza` already picks LID or PN based on the group's addressing_mode. `<meta target_sender_jid>` echoes whatever identity the server addressed, so for a LID-mode group the inbound msmsg lookup keyed under our LID would miss the row we stored under PN. - PreparedGroupStanza carries the addressing identity it actually used (`sender_identity: Jid`). - send_message_impl threads it through to a refactored `persist_outbound_msg_secret(chat, sender, msg_id, secret)` that takes the identity explicitly instead of deriving it from chat.server. - A new `dm_sender_identity_for` helper keeps the previous DM heuristic (LID for bot chats, PN otherwise) for the DM path. Loosened the `<bot>` parser to read `edit_target_id` unconditionally (WA Web `f()` accesses it via `?.botEditTargetId` regardless of edit_type; restricting to INNER/LAST blocked the regular-bot try-then-fallback from ever firing for non-fbid bots). Tests: - `persist_uses_group_sender_identity_for_lid_mode_groups` - `dm_sender_identity_picks_lid_for_bot_else_pn` - `msmsg_falls_back_to_edit_target_when_primary_uses_info_id` (inverse of the existing primary→fallback test, exercises the regular bot path)
|
@claude review |
|
Claude finished @jlucaso1's task in 3m 54s —— View job Claude Code Review
Overall this is solid. The crypto layout matches WA Web / whatsmeow, the test matrix is thorough (HKDF input-sensitivity, AAD layout, every-field-mutation vector, fallback both directions, encoder round-trip, PUT/GET coherence), and the prior P1/P2/Major review threads were properly folded in (secret threaded out of Four findings worth a look — none blocking, two are minor/preexisting. 1.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8bdfd0149
ℹ️ 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/send.rs`:
- Around line 1477-1482: The helper persist_outbound_msg_secret currently
accepts a generic &[u8], which lets callers pass wrong-sized secrets; change its
signature to require a fixed 32-byte secret (e.g., & [u8; 32] or a [u8; 32]
value) so misuse is a compile-time error, update any call sites to
construct/borrow a [u8;32], and adjust internal usage to treat the parameter as
a 32-byte array (no length checks needed). Ensure the change propagates to any
trait impls or tests referencing persist_outbound_msg_secret so callers provide
the exact-size message_secret expected by the reporting-token/msmsg flow.
🪄 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: 48ed05e3-c71b-4b40-8f36-8a30d0c49284
📒 Files selected for processing (4)
src/message.rssrc/send.rswacore/src/messages.rswacore/src/send.rs
Codex P2 (encoder): the AD_JID restriction landed only on the string path
(`parse_jid_meta`). Typed JIDs encoded via `write_jid_owned` /
`write_jid_ref` (and their size estimators) still emitted AD_JID for any
`device > 0`, so a programmatically built `Jid { server: Bot, device: 5 }`
would round-trip back to Pn. Centralised the check in a single
`server_supports_ad_jid(Server)` helper and applied it in:
* `parse_jid_meta` (via `Server::try_from`)
* `write_jid_ref`, `write_jid_owned`
* `owned_jid_encoded_size_with_cache`, `jid_ref_encoded_size_with_cache`
Codex (send.rs): tightened `persist_outbound_msg_secret`'s secret param
from `&[u8]` to `&[u8; reporting_token::MESSAGE_SECRET_SIZE]` so misuse
is a compile-time error. All call sites already pass a 32-byte array.
Claude P3 (dedup): after the parser started reading `edit_target_id`
unconditionally, a stanza that happens to set `edit_target_id == info.id`
would otherwise run two identical decrypt attempts before nacking.
`.filter(|fb| *fb != primary_msg_id)` collapses the duplicate.
Skipped (with reason):
* Nack code for GCM tag failure stays `495` (`MissingMessageSecret`):
matches whatsmeow `decryptMessages` exactly, which uses that code for
every msmsg failure path. Diverging to 500 would split parity for a
purely cosmetic gain.
* HKDF info concatenation stays separator-less: matches WA Web's
`v(msgId, target_jid, bot_jid)` literally. Changing it would break
interop.
Tests:
* `test_typed_non_ad_jid_with_device_round_trips_via_jid_pair`: typed
`Jid` with `device > 0` for Bot/broadcast/newsletter must NOT emit
AD_JID and must round-trip with the server preserved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadb0d9fe1
ℹ️ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/message.rs (2)
184-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize msmsg JIDs exactly like the outbound secret store path.
This path still feeds
to_non_ad().to_string()into the secret lookup and HKDF context. Outbound persistence strips device IDs, so a reply that round-trips asuser:NN@botortarget_sender_jid=...:NN@lidcan miss the stored secret or derive the wrong key and get nacked even though we persisted the right secret. We need one canonical form on both sides.Suggested fix
+ let normalize_msmsg_jid = |jid: &Jid| { + let mut jid = jid.to_non_ad(); + jid.device = 0; + jid.to_string() + }; + let chat_for_lookup = info .meta_info .target_chat .as_ref() .unwrap_or(&info.source.chat) - .to_non_ad() - .to_string(); - let target_sender_str = target_sender.to_non_ad().to_string(); + .pipe(normalize_msmsg_jid); + let target_sender_str = normalize_msmsg_jid(&target_sender); @@ - let bot_user_jid = info.source.sender.to_non_ad().to_string(); + let bot_user_jid = normalize_msmsg_jid(&info.source.sender);Also applies to: 208-211, 233-275
🤖 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.rs` around lines 184 - 191, The chat_for_lookup and target_sender_str JID normalization currently uses to_non_ad().to_string(), which differs from the canonical form used by outbound secret-store paths and can cause missed lookups or wrong HKDF keys; update the normalization in chat_for_lookup, target_sender_str (and the other mentioned sites) to use the exact same helper used when constructing outbound secret-store paths (the canonical secret-path JID normalizer used by the outbound persistence code) instead of to_non_ad().to_string() so both sides strip device IDs and produce the identical canonical JID string for secret lookup and HKDF context.
816-829:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat
msmsgplus an unknown sibling as “unknown-only”.This guard ignores
bot_payloads. So a stanza with one validmsmsgand one unknown/empty<enc>hits the fallback transport-ack path and never reacheshandle_msmsg_payload. That's silent message loss on a real bot reply.Suggested fix
if session_payloads.is_empty() && group_payloads.is_empty() + && bot_payloads.is_empty() && had_unknown_enc && !had_custom_handler {🤖 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.rs` around lines 816 - 829, The early-return condition treats stanzas with a valid msmsg (bot_payloads) plus an unknown sibling as “unknown-only”; update the guard that currently checks session_payloads.is_empty(), group_payloads.is_empty(), had_unknown_enc, and !had_custom_handler to also require bot_payloads.is_empty() (or otherwise ensure there are no msmsg/bot payloads) so that messages with a valid msmsg still reach handle_msmsg_payload instead of taking the transport-ack path (spawn_node_transport_ack); keep the rest of the logic intact and reference variables session_payloads, group_payloads, bot_payloads, had_unknown_enc, had_custom_handler, handle_msmsg_payload, and spawn_node_transport_ack when making the change.
🤖 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.
Outside diff comments:
In `@src/message.rs`:
- Around line 184-191: The chat_for_lookup and target_sender_str JID
normalization currently uses to_non_ad().to_string(), which differs from the
canonical form used by outbound secret-store paths and can cause missed lookups
or wrong HKDF keys; update the normalization in chat_for_lookup,
target_sender_str (and the other mentioned sites) to use the exact same helper
used when constructing outbound secret-store paths (the canonical secret-path
JID normalizer used by the outbound persistence code) instead of
to_non_ad().to_string() so both sides strip device IDs and produce the identical
canonical JID string for secret lookup and HKDF context.
- Around line 816-829: The early-return condition treats stanzas with a valid
msmsg (bot_payloads) plus an unknown sibling as “unknown-only”; update the guard
that currently checks session_payloads.is_empty(), group_payloads.is_empty(),
had_unknown_enc, and !had_custom_handler to also require bot_payloads.is_empty()
(or otherwise ensure there are no msmsg/bot payloads) so that messages with a
valid msmsg still reach handle_msmsg_payload instead of taking the transport-ack
path (spawn_node_transport_ack); keep the rest of the logic intact and reference
variables session_payloads, group_payloads, bot_payloads, had_unknown_enc,
had_custom_handler, handle_msmsg_payload, and spawn_node_transport_ack when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e792793a-0680-42ca-900d-2993901d3b89
📒 Files selected for processing (3)
src/message.rssrc/send.rswacore/binary/src/encoder.rs
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Inbound fanout capture (gap noted in earlier review): WA Web `processRenderableMessages` caches `messageSecret` from any bot-targeted renderable (`$ && (P || N || w || A) && !isForwarded`), regardless of fromMe. Mirrored in `Client::maybe_capture_inbound_msg_secret`, called from `dispatch_parsed_message`. Without it phone-initiated Meta AI conversations would still nack 495 even though the fanout from the primary carries the outbound secret. Performance: 32-byte stack copy + `Arc<MessageInfo>` clone (refcount bump) + a single `outbound_flush.spawn`. No heap alloc for the secret. Retention (Codex P2): the `created_at` column existed but no path used it, so long-running deployments accumulated one row per outbound. Added `MsgSecretStore::delete_expired_msg_secrets(cutoff)` to the trait, wired the SQLite impl through `with_retry`, and plugged the call into the keepalive cleanup loop (next to `delete_expired_sent_messages`) with a 14-day TTL — covers bot edits / late reactions / poll votes without unbounded growth. Codex inline P2 (msmsg sibling of an unknown enc): the unknown-only fallback guard ignored `bot_payloads`, so a stanza carrying BOTH a valid msmsg AND an unknown enc would short-circuit through `spawn_node_transport_ack` and silently drop the msmsg. Added `bot_payloads.is_empty()` to the guard. Codex inline (chat/sender canonical form) — skipped: outbound and inbound both use `.to_non_ad().to_string()` against the same identity family (verified at src/send.rs:1484-1485 vs src/message.rs:224/231). The canonical forms already match. Tests: - `maybe_capture_inbound_msg_secret_persists_for_bot_chats` - `maybe_capture_inbound_msg_secret_skips_non_bot_chats` - `maybe_capture_inbound_msg_secret_skips_when_secret_absent` - `fanout_capture_lets_subsequent_msmsg_decrypt` (end-to-end) - `mixed_msmsg_and_unknown_enc_still_decrypts_msmsg` - `delete_expired_msg_secrets_removes_only_old_rows` (in-memory) - `delete_expired_msg_secrets_deletes_only_below_cutoff` (sqlite)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b61a22e2
ℹ️ 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".
| // target_chat_jid) to look up the parent messageSecret. | ||
| meta_info.target_id = ma.optional_string("target_id").map(|s| s.into_owned()); | ||
| meta_info.target_sender = ma.optional_jid("target_sender_jid"); | ||
| meta_info.target_chat = ma.optional_jid("target_chat_jid"); |
There was a problem hiding this comment.
When a bot reply carries <meta target_chat_jid> (for example a Meta AI reply in a group), this only records the value in meta_info; source.chat was already set to the bot JID earlier and is never corrected. The new msmsg lookup uses target_chat, so decryption can succeed, but the dispatched Event::Message is attributed to the bot DM instead of the target group/chat, which breaks consumers that route messages by info.source.chat. After parsing this attr, bot-sourced stanzas should update MessageSource (and group context as needed) before returning MessageInfo.
Useful? React with 👍 / 👎.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/keepalive.rs (1)
153-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDecouple
msg_secretsretention from the sent-message TTL gate.I need this fixed: Line 153 currently gates both cleanups on
sent_msg_ttl > 0. When sent-message retention is disabled,delete_expired_msg_secretsnever runs andmsg_secretscan grow unbounded.Suggested fix
- if sent_msg_ttl > 0 && cleanup_counter >= 12 { + if cleanup_counter >= 12 { cleanup_counter = 0; - let backend = self.persistence_manager.backend(); - let cutoff = wacore::time::now_secs() - - sent_msg_ttl as i64; - self.runtime.spawn(Box::pin(async move { - if let Err(e) = backend.delete_expired_sent_messages(cutoff).await { - log::debug!(target: "Client/Keepalive", "Sent message cleanup error: {e}"); - } - })).detach(); + if sent_msg_ttl > 0 { + let backend = self.persistence_manager.backend(); + let cutoff = wacore::time::now_secs() - sent_msg_ttl as i64; + self.runtime.spawn(Box::pin(async move { + if let Err(e) = backend.delete_expired_sent_messages(cutoff).await { + log::debug!(target: "Client/Keepalive", "Sent message cleanup error: {e}"); + } + })).detach(); + } // msg_secrets retention: 14 days covers bot // edits / late reactions / poll votes while // bounding growth on long-running deployments. const MSG_SECRETS_TTL_SECS: i64 = 14 * 86_400; let backend = self.persistence_manager.backend();🤖 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/keepalive.rs` around lines 153 - 181, The sent-message cleanup block currently guarded by sent_msg_ttl > 0 also contains the msg_secrets cleanup, causing delete_expired_msg_secrets to be skipped when sent_msg_ttl is 0; separate the two: keep the existing condition and runtime.spawn call for backend.delete_expired_sent_messages(cutoff) inside the sent_msg_ttl > 0 && cleanup_counter >= 12 branch (and reset cleanup_counter there), then unconditionally (or under its own schedule if desired) create a separate runtime.spawn that calls persistence_manager.backend().delete_expired_msg_secrets(secret_cutoff) using MSG_SECRETS_TTL_SECS and the same logging on error so msg_secrets cleanup runs independent of sent_msg_ttl.src/send.rs (1)
1410-1424: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider a trace log when sender identity is unavailable.
Look, the connection's working, the stanza's going out, but if we can't resolve a sender identity here, the bot's reply is gonna fail to decrypt and nobody's gonna know why. That's not a great user experience. A quick
log::trace!whensenderisNonewould help us track down those "why can't I decrypt the Meta AI reply?" issues without spamming production logs.That said, this is a narrow edge case since bot interactions happen after LID is typically set. Not blocking.
📝 Optional trace log
if let Some(secret) = outbound_msg_secret.as_ref() { let sender = match outbound_group_sender_identity { Some(s) => Some(s), None => self.dm_sender_identity_for(&tc_issue_target).await, }; if let Some(sender) = sender { self.persist_outbound_msg_secret( &tc_issue_target, &sender, &outbound_id_clone, secret, ) .await; + } else { + log::trace!( + "Skipping messageSecret persistence for {}: sender identity unavailable", + outbound_id_clone + ); } }🤖 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/send.rs` around lines 1410 - 1424, Add a trace log when no sender identity can be resolved so we can track decryption failures: inside the branch handling outbound_msg_secret where you compute sender (using outbound_group_sender_identity and dm_sender_identity_for), when the final `sender` is None, call log::trace! with context (e.g., tc_issue_target and outbound_id_clone) before skipping persist_outbound_msg_secret; leave existing behavior unchanged and keep the log at trace level to avoid spamming production.
♻️ Duplicate comments (1)
src/message.rs (1)
164-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the bot-secret sender key on both write and read.
This now persists under
dm_sender_identity_for(bot_chat), buthandle_msmsg_payload()still looks up with the rawtarget_sender_jid/fallback. If the stanza echoes our PN while the secret was stored under our LID, the lookup misses and we nack a decryptable reply with 495.🤖 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.rs` around lines 164 - 170, The write path stores the secret under the normalized DM sender identity returned by client.dm_sender_identity_for(...) in persist_outbound_msg_secret, but the read path in handle_msmsg_payload still looks up using the raw target_sender_jid/fallback and can miss matches; update the read-side lookup in handle_msmsg_payload to first normalize the sender JID via client.dm_sender_identity_for(&chat) (or equivalent normalization helper) and try that key before falling back to the raw target_sender_jid, or alternatively make both persist_outbound_msg_secret and the lookup use the same normalization function so the secret is always written and read under the same normalized sender key.
🤖 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 164-170: The outbound persist call is being detached via
outbound_flush.spawn (using self.runtime) which makes bot-secret persistence
eventually consistent and allows a subsequent inbound processed by
handle_msmsg_payload() to miss the secret; instead perform the
persist_outbound_msg_secret write inline (await it) under the appropriate
per-chat/per-sender lock so the state update is visible before returning: remove
the detached spawn of client.persist_outbound_msg_secret(&info.source.chat,
&sender, &info.id, &secret).await and ensure you hold the relevant
session_locks/message_enqueue_locks while awaiting the persist so secret capture
is synchronous for the incoming message path.
---
Outside diff comments:
In `@src/keepalive.rs`:
- Around line 153-181: The sent-message cleanup block currently guarded by
sent_msg_ttl > 0 also contains the msg_secrets cleanup, causing
delete_expired_msg_secrets to be skipped when sent_msg_ttl is 0; separate the
two: keep the existing condition and runtime.spawn call for
backend.delete_expired_sent_messages(cutoff) inside the sent_msg_ttl > 0 &&
cleanup_counter >= 12 branch (and reset cleanup_counter there), then
unconditionally (or under its own schedule if desired) create a separate
runtime.spawn that calls
persistence_manager.backend().delete_expired_msg_secrets(secret_cutoff) using
MSG_SECRETS_TTL_SECS and the same logging on error so msg_secrets cleanup runs
independent of sent_msg_ttl.
In `@src/send.rs`:
- Around line 1410-1424: Add a trace log when no sender identity can be resolved
so we can track decryption failures: inside the branch handling
outbound_msg_secret where you compute sender (using
outbound_group_sender_identity and dm_sender_identity_for), when the final
`sender` is None, call log::trace! with context (e.g., tc_issue_target and
outbound_id_clone) before skipping persist_outbound_msg_secret; leave existing
behavior unchanged and keep the log at trace level to avoid spamming production.
---
Duplicate comments:
In `@src/message.rs`:
- Around line 164-170: The write path stores the secret under the normalized DM
sender identity returned by client.dm_sender_identity_for(...) in
persist_outbound_msg_secret, but the read path in handle_msmsg_payload still
looks up using the raw target_sender_jid/fallback and can miss matches; update
the read-side lookup in handle_msmsg_payload to first normalize the sender JID
via client.dm_sender_identity_for(&chat) (or equivalent normalization helper)
and try that key before falling back to the raw target_sender_jid, or
alternatively make both persist_outbound_msg_secret and the lookup use the same
normalization function so the secret is always written and read under the same
normalized sender key.
🪄 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: 89848ade-701f-4b64-b1fe-4f956583080e
📒 Files selected for processing (8)
src/appstate_sync.rssrc/keepalive.rssrc/message.rssrc/send.rsstorages/sqlite-storage/src/sqlite_store.rswacore/binary/src/encoder.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
The msg_secrets prune was nested inside `if sent_msg_ttl > 0`, so a deployment that disabled sent-message pruning (sent_message_ttl_secs = 0) but opted into msg_secret_ttl_secs would never run the secret cleanup — the advertised opt-in was a no-op in that combination. Share the ~5min tick gate but give each retention setting its own `ttl > 0` guard so they enable/disable independently.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/keepalive.rs (1)
159-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis cutoff math can break retention correctness under large TTL values.
Line 161 and Line 174 cast
u64TTLs withas i64; if config exceedsi64::MAX, the cast wraps and computes a bogus cutoff, so cleanup can target the wrong rows. This needs a safe conversion before subtraction.Suggested fix
if sent_msg_ttl > 0 { - let backend = self.persistence_manager.backend(); - let cutoff = now - sent_msg_ttl as i64; - self.runtime.spawn(Box::pin(async move { - if let Err(e) = backend.delete_expired_sent_messages(cutoff).await { - log::debug!(target: "Client/Keepalive", "Sent message cleanup error: {e}"); - } - })).detach(); + if let Ok(ttl_i64) = i64::try_from(sent_msg_ttl) { + let backend = self.persistence_manager.backend(); + let cutoff = now.saturating_sub(ttl_i64); + self.runtime.spawn(Box::pin(async move { + if let Err(e) = backend.delete_expired_sent_messages(cutoff).await { + log::debug!(target: "Client/Keepalive", "Sent message cleanup error: {e}"); + } + })).detach(); + } else { + log::warn!(target: "Client/Keepalive", "sent_message_ttl_secs exceeds i64::MAX; skipping cleanup tick"); + } } @@ let secret_ttl = self.cache_config.msg_secret_ttl_secs; if secret_ttl > 0 { - let backend = self.persistence_manager.backend(); - let secret_cutoff = now - secret_ttl as i64; - self.runtime.spawn(Box::pin(async move { - if let Err(e) = backend - .delete_expired_msg_secrets(secret_cutoff) - .await - { - log::debug!( - target: "Client/Keepalive", - "msg_secrets cleanup error: {e}" - ); - } - })) - .detach(); + if let Ok(ttl_i64) = i64::try_from(secret_ttl) { + let backend = self.persistence_manager.backend(); + let secret_cutoff = now.saturating_sub(ttl_i64); + self.runtime.spawn(Box::pin(async move { + if let Err(e) = backend + .delete_expired_msg_secrets(secret_cutoff) + .await + { + log::debug!( + target: "Client/Keepalive", + "msg_secrets cleanup error: {e}" + ); + } + })) + .detach(); + } else { + log::warn!(target: "Client/Keepalive", "msg_secret_ttl_secs exceeds i64::MAX; skipping cleanup tick"); + } }Use this read-only check to confirm the type mismatch and risky cast sites:
#!/bin/bash set -euo pipefail echo "== CacheConfig TTL field types ==" rg -n --type=rust -C3 'struct\s+CacheConfig|sent_message_ttl_secs|msg_secret_ttl_secs' echo echo "== now_secs signature ==" rg -n --type=rust -C2 'fn\s+now_secs\s*\(' echo echo "== Keepalive cutoff cast sites ==" rg -n --type=rust -C2 'now\s*-\s*[a-zA-Z_][a-zA-Z0-9_]*\s+as\s+i64|as\s+i64'Expected confirmation: TTL fields are unsigned (typically
u64) while cutoff math in this block usesas i64, proving wrap risk for oversized config values.🤖 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/keepalive.rs` around lines 159 - 175, The TTL-to-cutoff subtraction currently casts unsigned TTLs with "as i64" (for sent_msg_ttl and msg_secret_ttl), which can wrap on large values; change to a checked conversion (use TryInto::<i64>() on sent_msg_ttl and secret_ttl), handle the Err case by saturating to i64::MAX or skipping the cleanup, then compute cutoff = now - ttl_i64; update both places that spawn async tasks around backend.delete_expired_sent_messages(...) and the msg-secret cleanup to use the safe ttl_i64 conversion and avoid direct "as i64" casts.
🤖 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.
Duplicate comments:
In `@src/keepalive.rs`:
- Around line 159-175: The TTL-to-cutoff subtraction currently casts unsigned
TTLs with "as i64" (for sent_msg_ttl and msg_secret_ttl), which can wrap on
large values; change to a checked conversion (use TryInto::<i64>() on
sent_msg_ttl and secret_ttl), handle the Err case by saturating to i64::MAX or
skipping the cleanup, then compute cutoff = now - ttl_i64; update both places
that spawn async tasks around backend.delete_expired_sent_messages(...) and the
msg-secret cleanup to use the safe ttl_i64 conversion and avoid direct "as i64"
casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9286008-7516-4ba4-8e53-65f939741716
📒 Files selected for processing (7)
src/client.rssrc/features/message_edit.rssrc/features/polls.rssrc/keepalive.rssrc/message.rssrc/send.rswacore/binary/src/jid.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8dc241bc0
ℹ️ 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".
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
… TTL hardening Codex P2 (fanout ordering): the inbound messageSecret capture spawned the DB write on outbound_flush and returned, releasing the per-chat worker to dequeue the next stanza. A bot <enc type="msmsg"> reply queued right behind its own fanout (offline replay) could run handle_msmsg_payload before the write landed → MissingMessageSecret. Made the capture awaited inline: `maybe_capture_inbound_msg_secret` and `dispatch_parsed_message` are now async, so the write completes within the per-chat worker's serial section before the next stanza is processed. Also drops the spawn's 32-byte stack copy + Arc clones — the secret is borrowed straight through. Codex P2 (retention only after ping): the cleanup lived in the keepalive ping-success branch, but busy connections skip the ping when recent traffic already proves liveness, so they never reached the 12-tick gate. Moved the sweep to fire on the interval tick itself (before the idle early-return), extracted into `spawn_retention_cleanup`. Codex (TTL `as i64` wrap): converted the u64→i64 TTL cast to `now.saturating_sub(i64::try_from(ttl).unwrap_or(i64::MAX))` so an absurd TTL clamps (pruning nothing) instead of wrapping the cutoff negative.
- CacheConfig's manual Debug impl omitted the new msg_secret_ttl_secs field; added it so the runtime switch shows up in logs. - mentions_any_bot compared the JID domain to the literal "bot", missing the legacy PN-form Meta bot. Parse each mentioned_jid and use the canonical Jid::is_bot() contract (matches WA Web jid.isBot()). Only runs on the rare group-mention path with a short list. Skipped (already fixed in 2380433 / stale review commit): - keepalive TTL u64→i64 overflow: now i64::try_from(..).unwrap_or(MAX) + saturating_sub in spawn_retention_cleanup. - persist_outbound_msg_secret on outbound_flush.spawn race: now awaited inline in maybe_capture_inbound_msg_secret / dispatch_parsed_message. Skipped (not feasible by design): - "normalize PUT sender to handle_msmsg_payload's target_sender_jid": the capture runs on our outbound prompt, which carries no target_sender_jid meta (that exists only on the bot's reply). dm_sender_identity_for is the capture-time identity; alternate_msg_secret_lookup (WA Web getAlternateMsgKey mirror) reconciles LID↔PN skew at GET time, with e2e coverage. Test: mentions_any_bot_true_for_legacy_pn_form_bot.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message.rs (1)
165-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis cache key breaks real group bot traffic.
Line 172 always persists the secret under our own sender identity, and the gate at Lines 165-167 skips bot-authored replies in groups. That means flows like
peer mentions bot -> bot reply/edit via msmsgwon't match the laterget_msg_secret(chat, meta.target_sender_jid, target_id)lookup and we end up nack'ing a decryptable reply with 495.Possible fix
- let chat_is_bot = info.source.chat.server == wacore_binary::Server::Bot; - if !chat_is_bot && !msg.mentions_any_bot() { + let chat_is_bot = info.source.chat.server == wacore_binary::Server::Bot; + let sender_is_bot = info.source.sender.server == wacore_binary::Server::Bot; + if !chat_is_bot && !sender_is_bot && !msg.mentions_any_bot() { return; } if msg.is_forwarded() { return; } - let Some(sender) = self.dm_sender_identity_for(&info.source.chat).await else { + let sender = if !info.source.is_from_me && info.source.is_group && !sender_is_bot { + Some(info.source.sender.to_non_ad()) + } else { + self.dm_sender_identity_for(&info.source.chat).await + }; + let Some(sender) = sender else { return; }; self.persist_outbound_msg_secret(&info.source.chat, &sender, &info.id, secret_arr) .await;🤖 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.rs` around lines 165 - 176, The secret is being cached under our own DM sender identity (dm_sender_identity_for) which, combined with the early skip for non-bot messages (msg.mentions_any_bot), causes later lookups (get_msg_secret(chat, meta.target_sender_jid, target_id)) to miss replies authored by the bot in group flows; change the persist path in message.rs so persist_outbound_msg_secret stores the secret under the actual target sender JID used by recipients (use meta.target_sender_jid / original message sender identity) instead of the value returned by dm_sender_identity_for, and ensure the store key aligns with get_msg_secret's lookup key; keep the existing guards (mentions_any_bot/is_forwarded) but update the identity passed to persist_outbound_msg_secret to match get_msg_secret.
🤖 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.
Outside diff comments:
In `@src/message.rs`:
- Around line 165-176: The secret is being cached under our own DM sender
identity (dm_sender_identity_for) which, combined with the early skip for
non-bot messages (msg.mentions_any_bot), causes later lookups
(get_msg_secret(chat, meta.target_sender_jid, target_id)) to miss replies
authored by the bot in group flows; change the persist path in message.rs so
persist_outbound_msg_secret stores the secret under the actual target sender JID
used by recipients (use meta.target_sender_jid / original message sender
identity) instead of the value returned by dm_sender_identity_for, and ensure
the store key aligns with get_msg_secret's lookup key; keep the existing guards
(mentions_any_bot/is_forwarded) but update the identity passed to
persist_outbound_msg_secret to match get_msg_secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e55c4b32-f049-4aad-9e75-eb938b376711
📒 Files selected for processing (2)
src/keepalive.rssrc/message.rs
The contents API only returns .content (base64) for files up to 1MB. dev/bench/data.js grew past that (~1.3MB), so .content came back empty, base64 -d produced an empty file, and bench-comment.py crashed on json.loads. Use Accept: application/vnd.github.raw instead (works to 100MB) and guard load_baseline against empty input.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
|
Actionable comments posted: 0 |
Two WA Web group-bot divergences, both surfaced by review: 1. Secret capture identity (Codex P2 + outside-diff): the secret was always keyed under our own identity (dm_sender_identity_for). When ANOTHER group participant invokes the bot, their prompt carries the messageSecret but the bot reply's <meta target_sender_jid> echoes that participant — so the lookup missed and the reply nacked 495. Now key non-self captures under info.source.sender (the prompt author), keeping the self path on dm_sender_identity_for (bot DM echoes our LID). Mirrors WA Web's msgKey, which keys the cache by author/participant. 2. Receipt shape (review divergence): WA Web `sendAggregateReceipts` emits a bare `<ack class="message">` (sendBotInvokeResponseAcks) for a DELIVERY where the chat is not a bot but the author is — i.e. a bot reply inside a group. A 1:1 bot chat keeps the normal `<receipt>` (its `v` gate is false). ack_received_message now routes bot-authored non-bot-chat messages to the transport ack instead of a delivery receipt. Tests: - maybe_capture_inbound_msg_secret_keys_under_other_participant - bot_reply_in_group_acks_with_bare_ack_not_receipt - bot_dm_reply_keeps_delivery_receipt (regression: 1:1 bot keeps receipt)
The msmsg happy path was silent (only warns on failure). Add a debug line when the bot messageSecret is cached and an info line on successful msmsg decrypt, matching the existing "Successfully decrypted" style, so a production run can confirm the full flow from the logs.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
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 199-203: The logs are currently emitting raw JIDs
(sender.to_non_ad_string()) which leaks identifiers; replace direct use of
sender.to_non_ad_string() in the log::debug! call (and the similar
decrypt-success info-level log around the decrypt path referenced at lines
407-411) with a redacted or hashed representation (e.g., compute hashed_sender =
hash_or_redact_jid(sender.to_non_ad_string()) via a new helper like
redact_jid/hash_jid) and log that instead; also update the decrypt-success log
site (where info.id and sender are used) to use the same redacted/hashed helper
(and consider lowering its level if it’s currently info) so raw JIDs are never
written to logs.
🪄 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: 252cbf89-ca80-435b-b026-6667d05f27b3
📒 Files selected for processing (3)
.github/scripts/bench-comment.py.github/workflows/benchmark-comment.ymlsrc/message.rs
| log::debug!( | ||
| "[msg:{}] cached bot messageSecret under sender={}", | ||
| info.id, | ||
| sender.to_non_ad_string() | ||
| ); |
There was a problem hiding this comment.
Don’t ship raw JIDs into these logs.
This leaks full sender identifiers on the inbound msmsg path. The capture log can record another participant’s PN/LID, and the decrypt-success log does it at info level on every hit. Redact or hash the JID before logging.
Also applies to: 407-411
🤖 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.rs` around lines 199 - 203, The logs are currently emitting raw
JIDs (sender.to_non_ad_string()) which leaks identifiers; replace direct use of
sender.to_non_ad_string() in the log::debug! call (and the similar
decrypt-success info-level log around the decrypt path referenced at lines
407-411) with a redacted or hashed representation (e.g., compute hashed_sender =
hash_or_redact_jid(sender.to_non_ad_string()) via a new helper like
redact_jid/hash_jid) and log that instead; also update the decrypt-success log
site (where info.id and sender are used) to use the same redacted/hashed helper
(and consider lowering its level if it’s currently info) so raw JIDs are never
written to logs.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
A group bot invocation initiated by the PRIMARY device carries its messageSecret only in the bot-addressed copy the primary sends directly to the bot; that copy is not mirrored to companions (we only receive the group skmsg, which lacks the secret). So a companion legitimately never holds the secret for such replies — the lookup miss is expected and we nack 495 (confirmed in prod: no replay loop, no disconnect). Log it at debug for group chats; keep warn for a 1:1 bot chat where a miss is genuinely unexpected. Verified in production: DM with Meta AI decrypts end-to-end including the streaming edit chain; group-from-phone gracefully nacks without any stream:error or reconnect.
WA Web is itself a companion and DOES decrypt group bot replies, so the secret must be reachable by companions — our group capture is missing it. Add a temporary INFO log dumping has_secret / is_from_me / is_group / chat_is_bot / mentions_bot / forwarded / mentioned_jids at the capture gate so a prod repro reveals exactly which condition fails (likely the bot mentioned as a LID, which is_bot() doesn't recognize). To be reverted once the root cause is fixed.
There was a problem hiding this comment.
💡 Codex Review
Line 1403 in 48033a3
Cache the msmsg secret before sending the stanza
When send_message targets a bot, this writes the encrypted stanza to the socket before persisting outbound_msg_secret a few lines later. A fast bot reply can be read and processed by the inbound loop during that await/scheduling window, so handle_msmsg_payload looks up the original target_id before the row exists and nacks 495 even though this send generated the correct secret. Persist the generated secret before exposing the stanza to the server, or otherwise make the write visible before replies can be handled.
ℹ️ 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".
| log::info!( | ||
| "[msg:{}] msmsg-capture gate: has_secret={} secret_len={} is_from_me={} is_group={} chat={} sender={} chat_is_bot={} mentions_bot={} forwarded={} mentioned_jids={:?}", |
There was a problem hiding this comment.
In production this path runs for every successfully parsed message before the secret/mention gates return, and the info log includes real chat JIDs, sender JIDs, message ids, and mentioned JIDs. That makes ordinary message traffic and group membership data land in default application logs even when no msmsg secret is captured; this temporary diagnostic should be removed or dropped to a gated trace/debug log with redaction.
Useful? React with 👍 / 👎.
Prod diagnostic on a group @metaai prompt showed: has_secret=true, mentions_bot=false, mentioned_jids=[] — our own group bot prompt carries the messageSecret but no mention, so the (P||N) gate dropped it and the reply nacked 495. WA Web caches it via the `w`/`A` group-participant gates. - Add `message_context_info.bot_metadata` (the bot-invocation envelope WA Web reads) as a capture trigger, covering bot prompts with no mentioned JID. Gate is now P || N || bot_metadata. - Key the secret by chat type, not is_from_me: bot DM resolves to our LID (sender there is our PN device JID); group/regular use info.source.sender, which already equals the addressing identity the reply echoes in <meta target_sender_jid> (our LID in a LID group; the other participant's JID for their prompt). Removes reliance on the alternate lookup for the common group path. Diagnostic gate log still in place (now includes has_bot_metadata) to confirm the next prod run caches + decrypts the group reply. Tests: - maybe_capture_inbound_msg_secret_via_bot_metadata_without_mention
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4a744daf7
ℹ️ 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".
| return Err(e.into()); | ||
| } | ||
|
|
||
| if let Some(secret) = outbound_msg_secret.as_ref() { |
There was a problem hiding this comment.
When the outbound bot prompt is encoded as a bot_invoke_message wrapper (protobuf tag 67, which the send path already unwraps for stanza typing), prepare_dm_stanza/prepare_group_stanza return message_secret: None because wacore/src/reporting_token.rs's REPORTING_FIELDS does not include field 67. This new guard then skips persistence entirely, so the ciphertext contains no cached secret for a subsequent <enc type="msmsg"> reply referencing that prompt and the inbound lookup will nack with MissingMessageSecret; add the bot-invoke future-proof field to reporting-token extraction or otherwise ensure these bot prompts still produce and persist a messageSecret.
Useful? React with 👍 / 👎.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Prod confirmed the group bot flow works end-to-end: our own prompt caches via bot_metadata (keyed under our LID), another participant's prompt caches via mention, and all bot replies decrypt — 12 successes, 0 nack 495, 0 stream errors. Remove the temporary capture-gate INFO log; keep the concise "cached bot messageSecret" debug and "Successfully decrypted msmsg" info.
|
Actionable comments posted: 0 |
1 participant
Summary
Implements end-to-end
<enc type="msmsg">decryption so the client renders Meta AI / fbid bot replies instead of dropping them and looping the offline queue. Closes the residual disconnect cause from #648 (the070AB633…stream:error loop).Verified in production (companion of a personal Android number):
bot_metadata, all replies decrypted.Cross-referenced WA Web
WAWebBotMessageSecret.js+processRenderableMessagesand whatsmeowmsgsecret.go/message.go.Crypto
HKDF
msgIDunifies WA Web's two arms (fbidh()pre-resolvesedit_target_idfor INNER/LAST; regularf()triesinfo.idthen falls back) into a primary + fallback attempt — a strict superset of both.How it works
Capture (PUT). Every dispatched message carrying
message_context_info.message_secretis persisted when it's a bot context — mirroring WA WebprocessRenderableMessages$ && (P || N || w || A) && !isForwarded:Pchat is a bot,Na mentioned JID is a bot,w/Aproxied bymessage_context_info.bot_metadata(the bot-invocation envelope; present on our own group prompt even with no mention).<meta target_sender_jid>: bot DM → our LID (the raw sender there is our PN device JID); group/regular →info.source.sender(already our LID in a LID group / the other participant's JID). Awaited inline before the per-chat worker dequeues the next stanza, so a reply queued behind its own prompt can't race the write.Lookup + decrypt (GET).
<enc type="msmsg">is bucketed (EncType::MessageSecret) and decrypted inline inprocess_classified_messageunder the global permit + per-chat lock. Secret looked up by(target_chat|chat, target_sender_jid, target_id), with an alternate PN↔LID lookup (WA WebgetAlternateMsgKey). Missing secret / GCM fail / malformed proto / backend error → nack 495 (whatsmeow parity), so the server stops replaying — no loop.Receipts. DM bot reply → normal delivery
<receipt>. Group bot reply (chat not bot, author is bot) → bare<ack class="message">(WA WebsendBotInvokeResponseAcks), not a receipt.Retention.
MsgSecretStorepersists secrets in a newmsg_secretstable. Pruning is opt-in viaCacheConfig.msg_secret_ttl_secs(default0= no prune, matching whatsmeow / WA Web), driven by an independent keepalive tick.Notable supporting changes
EncType::MessageSecret+bot_encbucket;BotEditTypederivesWireEnum.<meta target_id/target_sender_jid/target_chat_jid>and<bot edit/edit_target_id/sender_timestamp_ms>(edit_target_id read unconditionally).PreparedDmStanza/PreparedGroupStanzasurface the generatedmessageSecret(+ groupsender_identity) so the outbound send path persists it too.writeJID/ WA WebWAWap.De.Jid::to_non_ad_string()(single-alloc) replacingto_non_ad().to_string()across 23 sites.MessageExt::mentions_any_bot/is_forwarded(canonicalJid::is_bot()).Test plan
cargo fmt --all,cargo clippy --all --tests(clean)cargo test -p wacore -p whatsapp-rust -p whatsapp-rust-sqlite-storage -p wacore-binary— all green (787 + 600 + 28 + 88 unit tests)Review threads folded in
P1 secret threading; P2 LID-group sender identity; P2 encoder typed path; P2 retention; P2 msmsg+unknown sibling; P2 fanout ordering (awaited inline); P2 group-participant secret key; backend-error nack; SQLite
with_retry;BotEditTypeWireEnum; primary/fallback dedup;&[u8; 32]signature; CacheConfig Debug field; canonicalis_bot(); group bot capture viabot_metadata.Skipped (with reason)
495(whatsmeow parity, not 500).v(...)literally; interop-load-bearing).