expose the frame bytes and decrypted payloads to hooks - #5
Open
jlucaso1 wants to merge 5 commits into
Open
Conversation
Both are values the library already computes on the receive path and then discards, and neither is reachable by a proxy that owns part of the protocol state. RawNodeHandler now takes a RawNode carrying the decoded node and the buffer it was decoded from. handleFrame had that buffer in scope and let it fall out of scope right after Unmarshal. A proxy forwarding stanzas elsewhere wants those bytes rather than a re-encoding: this library and whoever wrote the frame may encode one value differently and both are valid, so re-encoding yields a stanza that means the same and is not the same bytes. This changes the RawNodeHandler signature. A struct so that later additions do not change it again. DecryptedPayloadHandler is new and fires for every <enc> the library decrypts, before anything interprets the plaintext. Decryption is irreversible: by the time the plaintext exists the ratchet has advanced and a prekey may have been spent, so a payload the library then fails to unmarshal is currently lost behind a warning at message.go, and retrying yields the same failure. The hook runs ahead of that, so a caller gets the bytes whether or not this library can read them. Both take their argument by value. A pointer escapes through the indirect call and costs one heap allocation per stanza; the benchmarks in hooks_bench_test.go pin that at zero. With the hooks unset the cost is one nil check. Tests reach the decryption path through the event buffer, which returns a buffered plaintext before calling into Signal, so no session pair is needed.
![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: 586dc0dae9
ℹ️ 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".
…t first Two review findings on the DecryptedPayloadHandler placement. Unpadding runs inside decryptDM and decryptGroupMsg, after Signal has already advanced the ratchet. A plaintext whose padding the library refuses left through the decryption-error branch, so the hook never saw bytes that by then were the only copy in existence. That is the failure the hook was added to prevent, one layer below where it was placed. Both functions now return the raw Signal output alongside the unpad error, and the loop hands it over before reporting the failure. A decryption that produced nothing still notifies nothing. The hook also ran before cancelDelayedRequestFromPhone, so a handler that blocked could let the pending phone rerequest fire for a message that had arrived and decrypted. Cancellation happens first now. Both are covered, and both tests fail if the fix is reverted.
jlucaso1
added a commit
to oxidezap/wa-wire
that referenced
this pull request
Aug 8, 2026
The third engine, and the first the core cannot reach. `whatsapp-rust` links the Rust natively and `zapo` runs it through WebAssembly; Go can do neither, because Rust in Go means cgo and cgo in the per-stanza hot path is the cost the boundary exists to avoid. So the envelope format is written out a third time. That is the case the design was made for rather than duplication for its own sake: an adapter runs inside its engine, and this one is written by someone who could not have used our code even if they wanted to — which is the difference between a specification and a library with three callers. The Go encoder has no Rust to check itself against, so it emits fixtures the Rust side reads back. Nine assertions, passing on the first run, and failing three ways when a path is written big-endian. The engine gives this adapter something the other two lack. Both of them are told which `<enc>` of a stanza decrypted, counting `<enc>` nodes, and must resolve that to a child index — ambiguous the moment a stanza carries anything else, unresolvable for a fan-out `<message>`, and both emit those as L0-wire rather than risk a plaintext on the wrong node. hypermeow reports the child index directly and nothing is inferred. That is what contributing the observation point buys over working around one. Writing the joiner a third time exposed a defect in the shared design: the lookahead counts later stanzas, and the first cut counted only the ones it held, so a receive path carrying nothing but acks would have held a message for ever. Worth checking in the other two. D-022 set this directory aside as MPL-2.0, expecting patched whatsmeow files. It carries none — the hooks went upstream, where they are MPL-2.0 already, and what is here only imports the engine, which §3.3 allows under other terms. NOTICE.md says so and says what would change it. Built against polymorfa/hypermeow#5 through a `replace`, which says plainly that this is not built against anything published.
`notifyDecryptedPayload` misses the inliner by one point (cost 81 against a budget of 80), so every `<enc>` was paying a call to reach a nil check. Testing at the call site puts the cost back where the hook's documentation claims it is. Under callgrind: 5 instructions per `<enc>` against 35. Nothing else moved -- the same benchmarks compiled on `main` and here, ten alternating runs, show no significant difference on either path. The other two shapes were worse. Handing the helper a built `DecryptedPayload` does inline, but the struct is then materialised before the test rather than after it: 22. Turning the inner test into an early return costs 82 and does not inline at all.
The set case returned `drop: true` and left before `enqueueNode`, while the unset case went on to it. The two were measuring different amounts of work, so their matching numbers meant nothing. Both let the node through now, and both drain the queue. 488 B/op and 10 allocs/op on each, which is what the comparison was supposed to show.
Member
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7af3c9f353
ℹ️ 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".
Author
|
Evil Codex 😞 |
The doc promised "the plaintext exactly as Signal produced it" and the callback gets the unpadded bytes, which is a real contradiction and worth resolving in the doc rather than in the code. Stripping first is right. The padded form is not the message, every caller would strip it again, and what fires here is what the protobuf unmarshal is about to read. whatsapp-rust and Baileys hand over the unpadded bytes too, so changing this side would make one library disagree with the others about what a plaintext is. Padding that will not strip is the exception, and the reason is that no unpadded form exists there — the alternative is handing over nothing.
|
🤔 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two values the library already computes on the receive path and then discards. Neither is reachable by a proxy that owns part of the protocol state.
The frame bytes
RawNodeHandlernow takes aRawNodecarrying the decoded node and the buffer it was decoded from.handleFramehad that buffer in scope and let it fall out right afterUnmarshal.A proxy forwarding stanzas elsewhere wants those bytes rather than a re-encoding. This library and whoever wrote the frame may encode one value differently and both are valid, so re-encoding yields a stanza that means the same and is not the same bytes.
Breaking: the
RawNodeHandlersignature changes. It takes a struct now so that later additions do not change it again.The decrypted payloads
DecryptedPayloadHandleris new. It fires for every<enc>the library decrypts, before anything interprets the plaintext.Decryption is irreversible. By the time the plaintext exists the ratchet has advanced and a prekey may have been spent, so a payload that then fails
proto.Unmarshalis lost behind a warning inmessage.goand retrying yields the same failure. The hook runs ahead of that, so a caller gets the bytes whether or not this library can read them.ChildIndexis the position of the<enc>among the stanza's children rather than an ordinal among<enc>nodes, which would be ambiguous once a stanza carries<participants>or<device-identity>between them.Cost
Unset, each hook is one compare. Measured against
main, the same benchmarks compiled on both branches, ten alternating runs:handleFramedecryptMessages, 1<enc>decryptMessages, 8<enc>Allocation counts are equal in every sample and no time difference reaches significance.
Keeping it a compare took one thing.
notifyDecryptedPayloadmisses the inliner by a point (cost 81 exceeds budget 80), so the call sites test the handler themselves: 5 instructions per<enc>under callgrind rather than 35 for the call.Set, both hooks add no allocation of their own —
hooks_bench_test.go, with the hook letting the node through so that both cases pay the same dispatch:That is what taking the argument by value buys. By pointer it escapes through the indirect call and costs one allocation per stanza (13 → 14) and one per
<enc>(301 → 309 for a stanza carrying eight).Tests
hooks_test.gocovers both, including that a plaintext survives a failedproto.Unmarshal. Moving the hook below the unmarshal fails three of them.The decryption path is reached through the event buffer, which returns a buffered plaintext before calling into Signal, so no session pair is needed. That seam should make the rest of the decryption path testable too.