fix(message)!: decrypt secret encrypted edits on receive - #665
Conversation
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extracts per-message message_secret from history-sync blobs and inbound non-forwarded messages, persists secrets to the backend, and decrypts secret-encrypted message edits (with primary and alternate LID↔PN lookups), persisting recovered secrets for chained edits before dispatch. ChangesMessage Secret Persistence and Decryption Flow
Sequence Diagram(s)sequenceDiagram
participant DispatchMsg as dispatch_parsed_message
participant CaptureSecret as maybe_capture_inbound_msg_secret
participant PersistSecret as persist_msg_secret_bytes
participant DecryptSecret as maybe_decrypt_secret_encrypted_message
participant Backend as backend.get_msg_secret / put_msg_secrets
participant EventBus as Event::Message
DispatchMsg->>CaptureSecret: parsed message
CaptureSecret->>CaptureSecret: extract message_context_info.message_secret (if not forwarded)
alt secret exists
CaptureSecret->>PersistSecret: persist secret_bytes
PersistSecret->>Backend: put_msg_secret(chat, sender, msg_id)
end
DispatchMsg->>DecryptSecret: secret_encrypted_message present
DecryptSecret->>DecryptSecret: resolve own_jid (LID/PN)
DecryptSecret->>Backend: get_msg_secret(primary lookup)
alt secret found
Backend-->>DecryptSecret: secret_bytes
else not found
DecryptSecret->>DecryptSecret: compute alternate identity
DecryptSecret->>Backend: get_msg_secret(alternate lookup)
Backend-->>DecryptSecret: secret_bytes
end
DecryptSecret->>PersistSecret: persist newly-obtained secret via put_msg_secrets
DecryptSecret-->>DispatchMsg: decrypted message (or original)
DispatchMsg->>EventBus: dispatch
sequenceDiagram
participant HistoryTask as process_history_sync_task
participant ProcessFunc as process_history_sync
participant StoreSecrets as store_history_sync_msg_secrets
participant SendersHelper as history_msg_secret_senders
participant Backend as backend.put_msg_secrets
HistoryTask->>ProcessFunc: call process_history_sync(retain_history_blob)
ProcessFunc->>StoreSecrets: return msg_secret_records
loop each message_secret record
StoreSecrets->>SendersHelper: derive candidate senders
SendersHelper-->>StoreSecrets: list of sender JIDs
StoreSecrets->>Backend: put_msg_secrets(entries)
end
StoreSecrets-->>HistoryTask: count stored secrets
HistoryTask->>HistoryTask: check has_listeners
alt no listeners
HistoryTask->>HistoryTask: return after secrets persisted
else listeners present
HistoryTask->>HistoryTask: dispatch Event::HistorySync
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Mark: verify persistence semantics, alternate-identity lookups, and chained-edit secret refresh paths; these must be correct and covered by tests. 🚥 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 |
![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: e4333f2f7f
ℹ️ 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".
Benchmark Results1 regression(s) detected (>2% threshold):
66 unchanged benchmark(s)
|
![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
🤖 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/history_sync.rs`:
- Around line 252-301: store_history_sync_msg_secrets currently calls
wa::HistorySync::decode and iterates every conversation/message on the async
executor; move the CPU-bound decode + traversal into a
tokio::task::spawn_blocking closure that returns a Vec of lightweight records
(e.g., tuples of (chat: Jid string, sender: SenderType, msg_id: String, secret:
Vec<u8>)) by using wa::HistorySync::decode + the existing conversation/message
walking and history_msg_secret_senders logic, then await that spawn_blocking
result inside store_history_sync_msg_secrets and perform only the async
persistence calls (persist_msg_secret_bytes) over the returned records; keep
error handling (decode failure -> return 0) inside the spawn_blocking task or
propagate a clear empty Vec so the async path does only await and call
persist_msg_secret_bytes for each record.
In `@src/message.rs`:
- Around line 385-397: The current branch only persists refreshed edit secrets
under the (chat, original_sender, target_id) alias in persist_msg_secret_bytes,
which leaves the alternate identity alias stale and can cause subsequent chained
edits to fail; update the logic so that when message_context_info.message_secret
is present you persist the secret for both aliases (primary and alternate) —
either by invoking persist_msg_secret_bytes twice (once for the primary alias
and once for the alternate alias derived from the message/identity lookup) or by
extending persist_msg_secret_bytes to accept and write both PN/LID keys
internally; ensure you reference message_context_info, message_secret,
original_sender, target_id, persist_msg_secret_bytes and
alternate_msg_secret_lookup when locating where to add the second write.
🪄 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: dfb7c6cf-4869-4e39-a907-d2ad65c3377e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.tomlsrc/history_sync.rssrc/message.rs
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/history_sync.rs`:
- Around line 223-224: process_history_sync() currently decodes and walks the
decompressed history blob, but you then decode it again in
extract_history_sync_msg_secret_records(); instead, perform message-secret
extraction during the first parse/walk in process_history_sync() and pass the
resulting secret records to store_history_sync_msg_secrets() (which should be
reduced to an async persistence-only function). Update process_history_sync() to
return or yield the extracted secret records, remove the second decode in
extract_history_sync_msg_secret_records() (or eliminate that function and merge
its logic into the initial parse), and change the call site that currently does
self.store_history_sync_msg_secrets(decompressed.clone()).await to pass the
extracted records so the protobuf decode and conversation walk only happen once
(apply same refactor at the other duplicated locations mentioned).
🪄 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: 5b63b74e-dbc7-4676-b9a9-24c9145874b1
📒 Files selected for processing (2)
src/history_sync.rssrc/message.rs
|
@claude review |
|
Claude finished @jlucaso1's task in 4m 8s —— View job Claude PR Review
OverallThis is a solid fix. The previously-flagged concerns from Codex and CodeRabbit have been correctly addressed:
Findings🟡 P2 —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e6496cf06
ℹ️ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/history_sync.rs (1)
247-270: 🧹 Nitpick | 🔵 TrivialOn a big InitialBootstrap this writes secrets one row at a time — that won't scale.
Look,
store_history_sync_msg_secretsawaitspersist_msg_secret_bytessequentially per sender per record. A real bootstrap can carry thousands of messages, and every secret here is a separateput_msg_secretround-trip (each its ownspawn_blocking+ retry). That serializes the entire sync worker on DB latency. Move it forward with a single batched/transactional write so the backfill stays fast.🤖 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/history_sync.rs` around lines 247 - 270, store_history_sync_msg_secrets currently calls persist_msg_secret_bytes sequentially for every sender and record, causing N separate DB round-trips; instead collect all secret write entries (include chat Jid, sender Jid, msg_id, secret and any metadata derived from device_snapshot/own_pn/own_lid) and call a single batched/transactional persistence API (e.g., add a new PersistenceManager method like put_msg_secrets_batch or persist_msg_secrets_tx) to perform one bulk write/transaction; ensure you build the entries using history_msg_secret_senders(...) as now, handle parse/skip of invalid chat Jids, perform the batch write once, translate the batch result into the stored count to return, and keep error handling/retries inside the new batched persistence implementation.
🤖 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/history_sync.rs`:
- Around line 247-270: store_history_sync_msg_secrets currently calls
persist_msg_secret_bytes sequentially for every sender and record, causing N
separate DB round-trips; instead collect all secret write entries (include chat
Jid, sender Jid, msg_id, secret and any metadata derived from
device_snapshot/own_pn/own_lid) and call a single batched/transactional
persistence API (e.g., add a new PersistenceManager method like
put_msg_secrets_batch or persist_msg_secrets_tx) to perform one bulk
write/transaction; ensure you build the entries using
history_msg_secret_senders(...) as now, handle parse/skip of invalid chat Jids,
perform the batch write once, translate the batch result into the stored count
to return, and keep error handling/retries inside the new batched persistence
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54a5b163-7430-48fe-9955-b6e945283951
📒 Files selected for processing (1)
src/history_sync.rs
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 `@storages/sqlite-storage/src/sqlite_store.rs`:
- Around line 2555-2596: The current put_msg_secrets implementation inserts
entries one-by-one inside a transaction; refactor it to follow the chunked
bulk-insert pattern used by put_app_state_mutation_macs_for_device: collect
value tuples from the Arc<[MsgSecretEntry]> (include chat, sender, msg_id,
secret, device_id, created_at), split into chunks (e.g., 100), and for each
chunk call
diesel::insert_into(msg_secrets::table).values(&chunk).on_conflict((msg_secrets::chat,
msg_secrets::sender, msg_secrets::msg_id,
msg_secrets::device_id)).do_update().set((msg_secrets::secret.eq(...),
msg_secrets::created_at.eq(...))).execute(conn) inside the existing
immediate_transaction closure, summing and returning the total stored; preserve
use of device_id, now, and the with_retry("put_msg_secrets") wrapper.
🪄 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: 26d2b108-79b0-4c27-80fc-b8c2e321ba1d
📒 Files selected for processing (4)
src/history_sync.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e1117495
ℹ️ 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 `@wacore/src/history_sync.rs`:
- Around line 427-512: base_message() only peels each wrapper once in a fixed
order, so nested wrappers like view_once -> ephemeral -> extended_text stop
early; change base_message() to iteratively unwrap wrappers until no wrapper
fields point to an inner message (i.e., loop and replace current with the inner
message whenever any of device_sent_message, ephemeral_message,
view_once_message, view_once_message_v2, document_with_caption_message,
edited_message contain a nested .message), then return the final innermost
&Self; keep is_forwarded() as-is to call the fixed base_message() result. Ensure
you preserve lifetimes/types and use the same field names (device_sent_message,
ephemeral_message, view_once_message, view_once_message_v2,
document_with_caption_message, edited_message) when implementing the loop.
🪄 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: 422349a9-1287-4a6d-96b9-f3abcd379f1d
📒 Files selected for processing (5)
src/history_sync.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/history_sync.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
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/history_sync.rs`:
- Around line 247-303: The history sync path stores secrets only under the
senders returned by history_msg_secret_senders(), but must also persist the
bot-DM alternate sender alias (as live receive does) so later lookups using that
alias succeed; in store_history_sync_msg_secrets, after computing senders
(history_msg_secret_senders) and before pushing MsgSecretEntry items, detect
when a bot-DM alternate identity exists (i.e., the DM alias differs from the
primary sender/non-ad form) and push a duplicate MsgSecretEntry for that
alternate sender with the same chat, msg_id and secret (use the same
chat_id/msg_id/secret cloning/taking logic used for other senders); ensure the
duplicate uses sender.to_non_ad_string() for the alias and include it in entries
so put_msg_secrets persists both keys.
🪄 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: 4269d92b-8e83-4828-8c9b-a7c910444b1a
📒 Files selected for processing (5)
src/history_sync.rsstorages/sqlite-storage/src/sqlite_store.rswacore/src/history_sync.rswacore/src/store/in_memory.rswacore/src/store/traits.rs
Edits to a message authored by another user never decrypted: they stayed as `secret_encrypted_message` and were never rewrapped to the legacy `protocol_message.edited_message`, so `Event::Message` consumers that key off edits never observed them. Edits to our own messages worked. Root cause: `maybe_decrypt_secret_encrypted_message` resolved the parent author with `resolve_target_sender(target_message_key, my_jid)`. For an incoming peer edit the `target_message_key` is written in the editor's frame -- `from_me = true`, and no `participant` even in groups -- so it resolved to `my_jid`. The parent `messageSecret` is stored under the real author, so the lookup missed, decrypt returned `None`, and the still -encrypted envelope was dispatched unchanged. Self-synced edits worked only because there the editor is us. Fix: add `SecretEncrypted::original_sender_for_dispatch`. A message can only be edited by its author, so for MESSAGE_EDIT resolve the original sender from the envelope frame (my_jid when from-me, else the envelope sender) instead of trusting `target_message_key.from_me`. Poll/event kinds can be modified by a non-author, so they keep target-key resolution. The existing `original_sender_jid` is retained for the manual `extract_envelope` API and the poll path. Adds unit tests for the peer-edit, self-synced, and poll cases. Follow-up to oxidezap#665.
1 participant
What
WhatsApp now sends message edits as
message.secret_encrypted_messagewithMESSAGE_EDIT, encrypted with the parent message'smessageSecret. This wires that path into receive dispatch:message_context_info.message_secretfor every non-forwarded message, while preserving the existing bot-DM LID alias for msmsgsecret_encrypted_messageadd-ons during message dispatch, includingMESSAGE_EDITrewrap into the legacyprotocol_message.edited_messageshapemessageSecretunder the parent id so edit-of-edit decrypts workWhy
The crypto and store primitives already existed, but the receive path still surfaced encrypted edit envelopes as raw
Event::Message. The root cause was twofold: message-secret capture was gated to bot contexts, and dispatch never attempted the add-on decrypt forsecret_encrypted_message.That meant normal user messages often never had their parent secret stored, and edits that did arrive could not be converted into the edit shape downstream consumers expect.
Breaking Change
BREAKING CHANGE: incoming
secret_encrypted_messageedits that decrypt successfully are no longer dispatched as raw encrypted envelopes. They are dispatched as the decrypted legacy edit shape:protocol_message.type == MESSAGE_EDITwithprotocol_message.edited_messagepopulated.To migrate, handle message edits through the existing
ProtocolMessage::MESSAGE_EDITpath and treat rawsecret_encrypted_messageas a decrypt-failure fallback only. Consumers with custom msg-secret stores do not need code changes becauseput_msg_secretshas a default implementation, but storage backends should override it to get transactional/batched history-sync persistence.Tests
cargo fmt --allcargo clippy --all --testscargo test -p wacore -p whatsapp-rust -p whatsapp-rust-sqlite-storage