feat(client): hand over a decrypted payload before it is decoded - #1240
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) 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:
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 |
|
| Filename | Overview |
|---|---|
| src/message/receive.rs | Records encryption-node indices during unified classification, preserves them through session and group decryption, and emits unpadded plaintext before decoding. |
| src/message/msg_secret.rs | Extends pre-decode payload forwarding to successful bot-secret decryptions using the exact bytes supplied to the protobuf decoder. |
| src/message.rs | Adds encryption-node index metadata to classified and deferred payload structures with direct-then-local-fan-out ordering. |
| src/client.rs | Adds the weak-reference forwarding lease and atomic consumer counter without extending client lifetime. |
| src/client/accessors.rs | Exposes lease acquisition and the inexpensive forwarding-enabled check. |
| src/client/lifecycle.rs | Initializes the new forwarding counter consistently during client construction. |
| src/plugins/mod.rs | Generalizes gated event leases so plugin subscriptions independently acquire and retire raw-node and decrypted-payload forwarding. |
| wacore/src/messages.rs | Separates unpadding from unpadded decoding while preserving the established two-argument public decoder. |
| wacore/src/types/events.rs | Defines the decrypted-payload event contract and documents its payload and encryption-node ordering semantics. |
| src/message/tests.rs | Adds focused regression coverage for forwarding gates, undecodable payloads, lease lifetimes, index semantics, fan-out ordering, and bot-secret messages. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Encrypted stanza] --> B[Classify enc nodes and record index]
B --> C{Encryption type}
C -->|msg / pkmsg / skmsg| D[Signal decrypt and unpad]
C -->|msmsg| E[Bot-secret decrypt]
D --> F{Forwarding lease held?}
E --> F
F -->|Yes| G[Dispatch DecryptedPayload]
F -->|No| H[Skip forwarding]
G --> I[Decode protobuf]
H --> I
I -->|Success| J[Normal message pipeline]
I -->|Failure| K[Return decode error; forwarded bytes remain available]
Reviews (9): Last reviewed commit: "fix(client): forward the bot-secret payl..." | Re-trigger Greptile
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: New public event forwards decrypted plaintext (including undecodable payloads) to subscribers; this is a privacy/data-exposure and public API decision that needs human sign-off.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Confidence score: 4/5
- In
src/client/accessors.rs, the new public accessor returnsDecryptedPayloadLeasebut that type is not re-exported throughwhatsapp_rust::prelude::*, so prelude-only consumers can’t name the return type and may hit compile-time integration breakage—re-exportDecryptedPayloadLeasein the prelude alongsideRawNodeLeaseto keep the public API surface consistent.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/client/accessors.rs">
<violation number="1" location="src/client/accessors.rs:70">
P2: Consumers using the documented `whatsapp_rust::prelude::*` surface cannot name `DecryptedPayloadLease`, even though this new public accessor returns it and the equivalent `RawNodeLease` is in the prelude. Re-exporting the new lease from the prelude would keep the forwarding APIs consistent for typed fields and helper functions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// is dropped. Until then nothing is emitted and nothing is cloned. | ||
| /// | ||
| /// [`Event::DecryptedPayload`]: wacore::types::events::Event::DecryptedPayload | ||
| pub fn acquire_decrypted_payload_forwarding(self: &Arc<Self>) -> DecryptedPayloadLease { |
There was a problem hiding this comment.
P2: Consumers using the documented whatsapp_rust::prelude::* surface cannot name DecryptedPayloadLease, even though this new public accessor returns it and the equivalent RawNodeLease is in the prelude. Re-exporting the new lease from the prelude would keep the forwarding APIs consistent for typed fields and helper functions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/client/accessors.rs, line 70:
<comment>Consumers using the documented `whatsapp_rust::prelude::*` surface cannot name `DecryptedPayloadLease`, even though this new public accessor returns it and the equivalent `RawNodeLease` is in the prelude. Re-exporting the new lease from the prelude would keep the forwarding APIs consistent for typed fields and helper functions.</comment>
<file context>
@@ -61,6 +61,32 @@ impl Client {
+ /// is dropped. Until then nothing is emitted and nothing is cloned.
+ ///
+ /// [`Event::DecryptedPayload`]: wacore::types::events::Event::DecryptedPayload
+ pub fn acquire_decrypted_payload_forwarding(self: &Arc<Self>) -> DecryptedPayloadLease {
+ let incremented = self
+ .decrypted_payload_forwarding
</file context>
There was a problem hiding this comment.
Valid, fixed: DecryptedPayloadLease is in the prelude alongside RawNodeLease.
2e673f1 to
56212e3
Compare
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
fbc8e7d to
9466415
Compare
9466415 to
25da877
Compare
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
25da877 to
f418ad1
Compare
A plaintext that decrypts but does not decode is currently lost. The client turns it into `wa::Message` and dispatches that; if the decode fails — a field this build predates, a message type it does not model — `handle_decrypted_plaintext` returns an error, the caller logs a warning, and the bytes are gone. Nothing can ask for them back: the decryption already advanced the ratchet, so the same ciphertext will never decrypt again. Add `Event::DecryptedPayload`, emitted after unpadding and before decoding, carrying the plaintext, the `<enc>` type and its position in the stanza. It fires whether or not the decode goes on to succeed, which is what makes the failing case observable at all. Gated by `Client::acquire_decrypted_payload_forwarding()`, following `RawNodeLease`: while no lease is held the path costs one relaxed atomic load and nothing is cloned. Under a lease the payload is the same `Bytes` decoding receives, so forwarding it is a refcount bump. `decode_plaintext_detached_history_sync` is split in two — `unpad_plaintext` and the decode — because the two fail for unrelated reasons and the bytes in between are what the event hands over.
…t wants it A gated kind produces nothing until someone leases it. The host handled that for `RawNode` inline; a second gated kind would have made a plugin subscribe successfully and receive nothing. The two leases move behind `GatedForwarding`, so a future gated kind is one entry there rather than another pair of branches in the interest path.
`enc_index` was counted with `enumerate()` over the per-kind payload bucket the decrypt loop walks. Classification splits a stanza's `<enc>` nodes into session, group and bot buckets and skips any that yield no payload, so that count is not a position in the stanza. The common group shape — a pkmsg carrying the sender key followed by the skmsg it unlocks — reported index 0 for both, which is the one thing the field exists to answer. Record the position during classification, where the stanza is still being walked, and carry it on `EncPayload`. The decrypt loops read it instead of re-deriving it, so both paths agree by construction. `Event::DecryptedPayload` also moves to the end of `Event`, for the reason `EventKind` already documents for itself: a binary `Serialize` format writes the variant index, and inserting in the middle renumbers every variant after it.
Splitting the function changed its arity, which `cargo semver-checks` correctly reports as a breaking change to a published crate. The split is still what the new event needs — the plaintext in between is the thing it hands over — but it does not have to cost the existing signature. `decode_plaintext_detached_history_sync` is back to taking the padded plaintext and its version, now composed of `unpad_plaintext` and the new `decode_unpadded_detached_history_sync`. All three are public, and the names say which one takes what.
…ex counts The `msmsg` path opens its payload with AES-GCM rather than Signal and decodes it in its own function, so it was the one place a plaintext reached `Message` without the event seeing it. Its secret is single-use, which makes a payload it drops as unrecoverable as a ratcheted one — the same reason the event exists. `enc_index` was also documented as a position among the stanza's children, which it is not. The client enumerates direct `<enc>` children first and then this device's under `<participants><to>`, and the index is a position in that concatenation. A consumer resolving it back to a node has to walk the same two groups in the same order, so the field now says so. Two tests pin the order: one for the fan-out shape, one for the bot path.
f418ad1 to
a30bb05
Compare
Stacked on #1239 — review only the last commit; the base merges first.
The gap
A plaintext that decrypts but does not decode is lost.
handle_decrypted_plaintextturns it intowa::Message; when that decode fails — a field this build predates, a message type it does not model — it returns an error, the caller logs a warning, and the bytes disappear. Nothing can ask for them back: the decryption already advanced the ratchet, so the same ciphertext will never decrypt again.The bytes cost a real decryption and are the only copy that will ever exist. Today the only way to see one is to add a log line and rebuild.
What this adds
Event::DecryptedPayload, emitted after unpadding and before decoding, carrying:payload— the plaintext, unpadded, exactly as decoding receives itenc_type—msg,pkmsg,skmsg, …enc_index— which<enc>of the stanza produced it, since a message to several devices carries severalinfo— theMessageInfothe message was parsed intoIt fires whether or not the decode goes on to succeed. That is the point: the failing case is the one with nothing else to look at.
Reasons to want it beyond that: recording traffic for faithful replay (re-encoding a decoded
Messagedoes not reproduce the original bytes), and decoding with a newer protobuf than the build carries.Cost
Gated by
Client::acquire_decrypted_payload_forwarding(), followingRawNodeLease. While no lease is held the path is one relaxed atomic load and nothing is cloned. Under a lease the payload is the samebytes::Bytesthe decoder receives, so forwarding it is a refcount bump — no copy of the plaintext.enc_indexrides onDeferredPlaintext, which already exists, so nothing new is allocated per<enc>.Incidental
decode_plaintext_detached_history_syncis split intounpad_plaintext+ the decode. They fail for unrelated reasons — a payload that unpads cleanly but does not decode is a protocol change, one that fails to unpad is a corrupt frame — and the bytes in between are what the event hands over. Callers compose the two.Tests
Four in
src/message/tests.rs: nothing emitted without a lease; the payload and itsenc_indexunder one; still emitted when the protobuf decode fails; and forwarding stopping only when the last of several leases drops.