From 586dc0dae9eca7aeed53ec22dfa968e4fcded3c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas=20de=20Oliveira=20Lopes?= Date: Sat, 8 Aug 2026 04:02:03 -0300 Subject: [PATCH 1/5] expose the frame bytes and decrypted payloads to hooks 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 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. --- client.go | 85 +++++++++++- hooks_bench_test.go | 67 ++++++++++ hooks_test.go | 306 ++++++++++++++++++++++++++++++++++++++++++++ message.go | 15 ++- 4 files changed, 470 insertions(+), 3 deletions(-) create mode 100644 hooks_bench_test.go create mode 100644 hooks_test.go diff --git a/client.go b/client.go index f8f13f737..67652b230 100644 --- a/client.go +++ b/client.go @@ -59,7 +59,80 @@ type EventHandlerWithSuccessStatus func(evt any) bool // invariants, and other stateful parts of the protocol. Intended for // proxy implementations where an external system owns part of the // protocol state machine (see also [DisabledFeatures]). -type RawNodeHandler func(ctx context.Context, node *waBinary.Node) (modified *waBinary.Node, drop bool) +type RawNodeHandler func(ctx context.Context, raw RawNode) (modified *waBinary.Node, drop bool) + +// RawNode is one inbound stanza as it arrived, handed to a +// [RawNodeHandler]. +// +// A struct rather than separate parameters so that later additions do not +// change the handler signature again. +type RawNode struct { + // Node is the decoded stanza. + Node *waBinary.Node + + // Frame is the buffer Node was decoded from: after Noise decryption + // and after decompression, before parsing. + // + // A proxy that forwards stanzas elsewhere wants these rather than a + // re-encoding of Node. The two are not interchangeable: this library + // and whoever wrote the frame may encode one value differently + // (a token here, bytes there) and both are valid, so re-encoding + // produces a stanza that means the same and is not the same bytes. + // + // Only valid for the duration of the call: an uncompressed frame is a + // window into the transport's own read buffer rather than a buffer of + // its own. A handler that keeps the slice must copy it first. Handing + // the buffer over as it stands is what keeps this hook free for the + // callers that only look at Node. + Frame []byte +} + +// DecryptedPayloadHandler is called for every child this library +// decrypts, with the plaintext exactly as Signal produced it and before +// anything interprets it. +// +// It exists because decryption is irreversible. The ratchet has advanced +// and a prekey may have been spent by the time the plaintext exists, so a +// payload the library then fails to interpret (an unknown protobuf +// version, a malformed message) is gone for good: retrying yields the +// same failure and the sender will not send it again. This fires before +// that interpretation, so a caller that wants the bytes gets them whether +// or not this library can make sense of them. +// +// Fires once per successfully decrypted , in the order the children +// appear in the stanza. A failed decryption produces nothing here; +// [events.UndecryptableMessage] reports that. +// +// Runs on the goroutine handling that stanza, which the handler queue +// waits on. Blocking here therefore does not stall the receive loop, but +// it does stall the queue behind it: past 30 seconds the queue starts +// logging, and once the queue fills the client force-reconnects. +type DecryptedPayloadHandler func(ctx context.Context, payload DecryptedPayload) + +// DecryptedPayload is one plaintext, addressed to the node it came from. +type DecryptedPayload struct { + // Info is the enclosing message's metadata. + Info *types.MessageInfo + + // Node is the stanza the belongs to. + Node *waBinary.Node + + // ChildIndex is the position of the among Node's children, so a + // caller can address the plaintext to the node it came from without + // re-deriving which this was. Counting nodes separately + // would be ambiguous the moment a stanza carries anything else. + ChildIndex int + + // EncType is the node's type attribute: pkmsg, msg, skmsg, msmsg. + EncType string + + // Plaintext is what Signal produced, unparsed. + // + // Only valid for the duration of the call: the buffer belongs to the + // decryption that produced it and is passed on to be parsed. A handler + // that keeps it must copy. + Plaintext []byte +} // DisabledFeatures lets callers turn off built-in whatsmeow processing // paths. Used by proxies where an external system owns parts of the @@ -217,6 +290,11 @@ type Client struct { // after decoding but before standard dispatch. See [RawNodeHandler]. RawNodeHandler RawNodeHandler + // DecryptedPayloadHandler, if non-nil, is called for every this + // library decrypts, before the plaintext is interpreted. + // See [DecryptedPayloadHandler]. + DecryptedPayloadHandler DecryptedPayloadHandler + // DisabledFeatures controls which built-in processing paths are // skipped. See [DisabledFeatures]. DisabledFeatures DisabledFeatures @@ -886,7 +964,10 @@ func (cli *Client) handleFrame(ctx context.Context, data []byte) { return } if h := cli.RawNodeHandler; h != nil { - modified, drop := h(ctx, node) + // `decompressed` is the buffer `node` was decoded from, and it is + // about to go out of scope. Handing it over costs nothing and is + // the only chance anyone has at the original bytes. + modified, drop := h(ctx, RawNode{Node: node, Frame: decompressed}) if drop { cli.recvLog.Debugf("RawNodeHandler dropped node: %s", node) return diff --git a/hooks_bench_test.go b/hooks_bench_test.go new file mode 100644 index 000000000..51d3b049f --- /dev/null +++ b/hooks_bench_test.go @@ -0,0 +1,67 @@ +// Copyright (c) 2026 Tulir Asokan +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package whatsmeow + +import ( + "context" + "testing" + + waBinary "go.mau.fi/whatsmeow/binary" + "go.mau.fi/whatsmeow/store" + waLog "go.mau.fi/whatsmeow/util/log" +) + +// Both hooks take their argument by value rather than by pointer, and these +// exist to keep it that way. A pointer would escape through the indirect call +// and cost one heap allocation per stanza. Small, and on the receive path of +// a long-running process, which is where small and per-stanza adds up. +// +// Compare the two: the numbers have to match. +// +// go test -run XXX -bench BenchmarkRawNodeHook -benchmem +func benchmarkFrame(b *testing.B) []byte { + b.Helper() + frame, err := waBinary.Marshal(waBinary.Node{ + Tag: "receipt", + Attrs: waBinary.Attrs{"id": "ABCD1234", "type": "read"}, + }) + if err != nil { + b.Fatalf("failed to marshal fixture: %v", err) + } + return frame +} + +func BenchmarkRawNodeHookUnset(b *testing.B) { + cli := NewClient(&store.Device{}, waLog.Noop) + // Drained, so the queue depth does not become the thing being measured. + go func() { + for range cli.handlerQueue { + } + }() + frame := benchmarkFrame(b) + + b.ReportAllocs() + for b.Loop() { + cli.handleFrame(context.Background(), frame) + } +} + +func BenchmarkRawNodeHookSet(b *testing.B) { + cli := NewClient(&store.Device{}, waLog.Noop) + cli.RawNodeHandler = func(_ context.Context, raw RawNode) (*waBinary.Node, bool) { + if len(raw.Frame) == 0 { + b.Fatal("the hook was handed no frame") + } + return nil, true + } + frame := benchmarkFrame(b) + + b.ReportAllocs() + for b.Loop() { + cli.handleFrame(context.Background(), frame) + } +} diff --git a/hooks_test.go b/hooks_test.go new file mode 100644 index 000000000..47dc45549 --- /dev/null +++ b/hooks_test.go @@ -0,0 +1,306 @@ +// Copyright (c) 2026 Tulir Asokan +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package whatsmeow + +import ( + "bytes" + "context" + "testing" + "time" + + "github.com/polymorfa/libsignal-protocol-go/ecc" + "github.com/polymorfa/libsignal-protocol-go/keys/identity" + "github.com/polymorfa/libsignal-protocol-go/protocol" + + waBinary "go.mau.fi/whatsmeow/binary" + "go.mau.fi/whatsmeow/store" + "go.mau.fi/whatsmeow/types" + waLog "go.mau.fi/whatsmeow/util/log" +) + +func testClient(t *testing.T) *Client { + t.Helper() + return NewClient(&store.Device{}, waLog.Noop) +} + +// --- RawNodeHandler --------------------------------------------------------- + +func TestRawNodeHandlerGetsTheBytesTheNodeWasDecodedFrom(t *testing.T) { + // A proxy that forwards stanzas elsewhere wants the bytes that arrived, + // not a re-encoding: this library and whoever wrote the frame may encode + // one value differently and both are valid. + cli := testClient(t) + node := waBinary.Node{ + Tag: "receipt", + Attrs: waBinary.Attrs{"id": "ABCD1234", "type": "read"}, + } + marshaled, err := waBinary.Marshal(node) + if err != nil { + t.Fatalf("failed to marshal fixture: %v", err) + } + + var got RawNode + var frameCopy []byte + cli.RawNodeHandler = func(_ context.Context, raw RawNode) (*waBinary.Node, bool) { + got = raw + frameCopy = bytes.Clone(raw.Frame) + return nil, true + } + cli.handleFrame(context.Background(), marshaled) + + if got.Node == nil || got.Node.Tag != "receipt" { + t.Fatalf("unexpected node: %+v", got.Node) + } + // Marshal writes a leading format byte that the decoder consumes, so the + // frame is everything after it. Pinning this is the point: a consumer + // replaying the bytes has to know exactly which buffer it was handed. + if want := marshaled[1:]; !bytes.Equal(frameCopy, want) { + t.Fatalf("frame is not the decoded buffer:\n got %x\nwant %x", frameCopy, want) + } + + // And the frame really does decode back to the same stanza. + unpacked, err := waBinary.Unpack(marshaled) + if err != nil { + t.Fatalf("failed to unpack: %v", err) + } + if !bytes.Equal(frameCopy, unpacked) { + t.Fatal("frame is not what Unpack produced") + } +} + +func TestRawNodeHandlerCanStillDropAndReplace(t *testing.T) { + // The behaviour the signature change must not have disturbed. + marshaled, err := waBinary.Marshal(waBinary.Node{Tag: "receipt", Attrs: waBinary.Attrs{}}) + if err != nil { + t.Fatalf("failed to marshal fixture: %v", err) + } + + cli := testClient(t) + cli.RawNodeHandler = func(_ context.Context, _ RawNode) (*waBinary.Node, bool) { + return nil, true + } + cli.handleFrame(context.Background(), marshaled) + if len(cli.handlerQueue) != 0 { + t.Fatal("a dropped node still reached the dispatch queue") + } + + replaced := testClient(t) + replaced.RawNodeHandler = func(_ context.Context, _ RawNode) (*waBinary.Node, bool) { + return &waBinary.Node{Tag: "iq", Attrs: waBinary.Attrs{}}, false + } + replaced.handleFrame(context.Background(), marshaled) + select { + case queued := <-replaced.handlerQueue: + if queued.Tag != "iq" { + t.Fatalf("the replacement did not reach dispatch: %s", queued.Tag) + } + case <-time.After(time.Second): + t.Fatal("nothing reached the dispatch queue") + } +} + +func TestNoRawNodeHandlerCostsNothing(t *testing.T) { + marshaled, err := waBinary.Marshal(waBinary.Node{Tag: "receipt", Attrs: waBinary.Attrs{}}) + if err != nil { + t.Fatalf("failed to marshal fixture: %v", err) + } + cli := testClient(t) + cli.handleFrame(context.Background(), marshaled) + if len(cli.handlerQueue) != 1 { + t.Fatal("the node did not reach dispatch with no handler set") + } +} + +// --- DecryptedPayloadHandler ------------------------------------------------ + +// bufferedPlaintext is an [store.EventBuffer] that hands back a plaintext +// without any Signal work. +// +// [Client.bufferedDecrypt] returns a buffered plaintext before calling the +// decrypt closure, which is what makes the decryption path reachable in a test +// at all: standing up a real double-ratchet session pair would take more +// scaffolding than the thing under test. +type bufferedPlaintext struct { + store.NoopStore + plaintext []byte + cleared int +} + +func (b *bufferedPlaintext) GetBufferedEvent(context.Context, [32]byte) (*store.BufferedEvent, error) { + return &store.BufferedEvent{Plaintext: b.plaintext, InsertTime: time.Now()}, nil +} + +func (b *bufferedPlaintext) ClearBufferedEventPlaintext(context.Context, [32]byte) error { + b.cleared++ + return nil +} + +func (b *bufferedPlaintext) DeleteOldBufferedHashes(context.Context) error { return nil } + +// signalCiphertext builds a structurally valid SignalMessage. +// +// Never decrypted: the buffered plaintext short-circuits that. It has to parse, +// because `decryptDM` parses before it consults the buffer. +func signalCiphertext(t *testing.T) []byte { + t.Helper() + ratchet, err := ecc.GenerateKeyPair() + if err != nil { + t.Fatalf("failed to generate ratchet key: %v", err) + } + senderKey, err := ecc.GenerateKeyPair() + if err != nil { + t.Fatalf("failed to generate sender key: %v", err) + } + receiverKey, err := ecc.GenerateKeyPair() + if err != nil { + t.Fatalf("failed to generate receiver key: %v", err) + } + + msg, err := protocol.NewSignalMessage( + 3, 0, 0, make([]byte, 32), + ratchet.PublicKey(), []byte("ciphertext"), + identity.NewKey(senderKey.PublicKey()), identity.NewKey(receiverKey.PublicKey()), + pbSerializer.SignalMessage, + ) + if err != nil { + t.Fatalf("failed to build signal message: %v", err) + } + return msg.Serialize() +} + +// padded appends the padding `unpadMessage` strips for a v2 message. +func padded(plaintext []byte) []byte { + const pad = 4 + return append(append([]byte{}, plaintext...), bytes.Repeat([]byte{pad}, pad)...) +} + +// decryptFixture wires a client whose decryption yields `plaintext`, plus the +// `` stanza to feed it. +func decryptFixture(t *testing.T, plaintext []byte, children ...waBinary.Node) (*Client, *types.MessageInfo, *waBinary.Node) { + t.Helper() + cli := testClient(t) + cli.EnableDecryptedEventBuffer = true + buffer := &bufferedPlaintext{plaintext: padded(plaintext)} + cli.Store.EventBuffer = buffer + + node := &waBinary.Node{ + Tag: "message", + Attrs: waBinary.Attrs{"id": "ABCD1234"}, + Content: children, + } + info := &types.MessageInfo{ + ID: "ABCD1234", + MessageSource: types.MessageSource{ + // A `@lid` sender skips the PN-to-LID migration lookup, which + // would need a store this test does not stand up. + Sender: types.JID{User: "1234", Server: types.HiddenUserServer}, + Chat: types.JID{User: "1234", Server: types.HiddenUserServer}, + }, + Timestamp: time.Now(), + } + return cli, info, node +} + +func encNode(t *testing.T, encType string) waBinary.Node { + t.Helper() + return waBinary.Node{ + Tag: "enc", + Attrs: waBinary.Attrs{"type": encType, "v": "2"}, + Content: signalCiphertext(t), + } +} + +func TestDecryptedPayloadHandlerSeesEveryPlaintext(t *testing.T) { + cli, info, node := decryptFixture(t, []byte("not-a-protobuf"), encNode(t, "msg")) + + var seen []DecryptedPayload + cli.DecryptedPayloadHandler = func(_ context.Context, payload DecryptedPayload) { + clone := payload + clone.Plaintext = bytes.Clone(payload.Plaintext) + seen = append(seen, clone) + } + cli.decryptMessages(context.Background(), info, node) + + if len(seen) != 1 { + t.Fatalf("expected one payload, got %d", len(seen)) + } + got := seen[0] + if !bytes.Equal(got.Plaintext, []byte("not-a-protobuf")) { + t.Fatalf("plaintext is not what decryption produced: %q", got.Plaintext) + } + if got.EncType != "msg" { + t.Fatalf("unexpected enc type: %q", got.EncType) + } + if got.ChildIndex != 0 { + t.Fatalf("unexpected child index: %d", got.ChildIndex) + } + if got.Info == nil || got.Info.ID != "ABCD1234" { + t.Fatal("the payload does not carry its message's info") + } + if got.Node == nil || got.Node.Tag != "message" { + t.Fatal("the payload does not carry its stanza") + } +} + +func TestDecryptedPayloadHandlerFiresEvenWhenTheLibraryCannotReadThePlaintext(t *testing.T) { + // The reason the hook exists. `not-a-protobuf` fails `proto.Unmarshal` + // below the hook, and before this the plaintext went nowhere: the ratchet + // had already advanced, so nobody could ever get those bytes again. + cli, info, node := decryptFixture(t, []byte("not-a-protobuf"), encNode(t, "msg")) + + var fired bool + cli.DecryptedPayloadHandler = func(_ context.Context, payload DecryptedPayload) { + fired = true + if !bytes.Equal(payload.Plaintext, []byte("not-a-protobuf")) { + t.Errorf("unexpected plaintext: %q", payload.Plaintext) + } + } + cli.decryptMessages(context.Background(), info, node) + + if !fired { + t.Fatal("a plaintext the library could not interpret was dropped without anyone seeing it") + } +} + +func TestChildIndexAddressesTheEncItCameFrom(t *testing.T) { + // Counting `` nodes separately would be ambiguous the moment a + // stanza carries anything else, and real ones do. + cli, info, node := decryptFixture( + t, + []byte("not-a-protobuf"), + waBinary.Node{Tag: "participants", Attrs: waBinary.Attrs{}}, + encNode(t, "msg"), + waBinary.Node{Tag: "device-identity", Attrs: waBinary.Attrs{}}, + encNode(t, "msg"), + ) + + var indices []int + cli.DecryptedPayloadHandler = func(_ context.Context, payload DecryptedPayload) { + indices = append(indices, payload.ChildIndex) + children, ok := payload.Node.Content.([]waBinary.Node) + if !ok { + t.Error("the stanza does not carry its children") + return + } + if payload.ChildIndex >= len(children) || children[payload.ChildIndex].Tag != "enc" { + t.Errorf("child index %d does not address an ", payload.ChildIndex) + } + } + cli.decryptMessages(context.Background(), info, node) + + if len(indices) != 2 { + t.Fatalf("expected two payloads, got %d", len(indices)) + } + if indices[0] != 1 || indices[1] != 3 { + t.Fatalf("indices do not address the children: %v", indices) + } +} + +func TestNoDecryptedPayloadHandlerIsHarmless(t *testing.T) { + cli, info, node := decryptFixture(t, []byte("not-a-protobuf"), encNode(t, "msg")) + cli.decryptMessages(context.Background(), info, node) +} diff --git a/message.go b/message.go index 328056d82..05a493c0d 100644 --- a/message.go +++ b/message.go @@ -354,7 +354,7 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, } } var recognizedStanza, protobufFailed bool - for _, child := range children { + for childIndex, child := range children { if child.Tag != "enc" { continue } @@ -434,6 +434,19 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, }) return } + // Before anything reads the plaintext, including the protobuf + // unmarshal below: decryption already advanced the ratchet, so a + // payload this library cannot interpret is one nobody can recover. + if h := cli.DecryptedPayloadHandler; h != nil { + h(ctx, DecryptedPayload{ + Info: info, + Node: node, + ChildIndex: childIndex, + EncType: encType, + Plaintext: decrypted, + }) + } + retryCount := ag.OptionalInt("count") cli.cancelDelayedRequestFromPhone(info.ID) From d0d93a13ee200713aedc213e5f536a712446e748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas=20de=20Oliveira=20Lopes?= Date: Sat, 8 Aug 2026 04:21:32 -0300 Subject: [PATCH 2/5] hand over a plaintext that unpadding refuses, and cancel the rerequest 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. --- hooks_test.go | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++ message.go | 65 ++++++++++++++++++++++++++++++++++----------- 2 files changed, 122 insertions(+), 16 deletions(-) diff --git a/hooks_test.go b/hooks_test.go index 47dc45549..ea90b04a6 100644 --- a/hooks_test.go +++ b/hooks_test.go @@ -9,6 +9,7 @@ package whatsmeow import ( "bytes" "context" + "errors" "testing" "time" @@ -304,3 +305,75 @@ func TestNoDecryptedPayloadHandlerIsHarmless(t *testing.T) { cli, info, node := decryptFixture(t, []byte("not-a-protobuf"), encNode(t, "msg")) cli.decryptMessages(context.Background(), info, node) } + +func TestPaddingThatTheLibraryRejectsStillReachesTheHook(t *testing.T) { + // Unpadding runs after Signal, so a plaintext with padding the library + // refuses has already cost a ratchet step. Before this it left through + // the decryption-error branch and nobody ever saw it. + cli, info, node := decryptFixture(t, nil, encNode(t, "msg")) + // Not `padded`: a trailing byte that claims more padding than there is. + cli.Store.EventBuffer = &bufferedPlaintext{plaintext: []byte{'a', 'b', 0x7f}} + + var seen [][]byte + cli.DecryptedPayloadHandler = func(_ context.Context, payload DecryptedPayload) { + seen = append(seen, bytes.Clone(payload.Plaintext)) + } + cli.decryptMessages(context.Background(), info, node) + + if len(seen) != 1 { + t.Fatalf("a plaintext rejected by unpadding was dropped without anyone seeing it (got %d)", len(seen)) + } + if !bytes.Equal(seen[0], []byte{'a', 'b', 0x7f}) { + t.Fatalf("the hook got something other than the Signal output: %q", seen[0]) + } +} + +func TestNothingReachesTheHookWhenDecryptionItselfFails(t *testing.T) { + // The other side of the same rule: no plaintext existed, so there is + // nothing to hand over and the hook must stay quiet. + cli, info, node := decryptFixture(t, nil, encNode(t, "msg")) + cli.Store.EventBuffer = &failingBuffer{} + + called := false + cli.DecryptedPayloadHandler = func(_ context.Context, _ DecryptedPayload) { + called = true + } + cli.decryptMessages(context.Background(), info, node) + + if called { + t.Fatal("the hook fired for a decryption that never produced anything") + } +} + +type failingBuffer struct { + store.NoopStore +} + +func (f *failingBuffer) GetBufferedEvent(context.Context, [32]byte) (*store.BufferedEvent, error) { + return nil, errors.New("no buffered event, and no session to decrypt with") +} + +func TestTheRerequestTimerIsCancelledBeforeTheHookRuns(t *testing.T) { + // A handler that blocks must not let the phone be asked to resend a + // message that did arrive and did decrypt. + cli, info, node := decryptFixture(t, []byte("not-a-protobuf"), encNode(t, "msg")) + cli.AutomaticMessageRerequestFromPhone = true + + cancelled := false + cli.pendingPhoneRerequestsLock.Lock() + cli.pendingPhoneRerequests = map[types.MessageID]context.CancelFunc{ + info.ID: func() { cancelled = true }, + } + cli.pendingPhoneRerequestsLock.Unlock() + + cli.DecryptedPayloadHandler = func(_ context.Context, _ DecryptedPayload) { + if !cancelled { + t.Error("the hook ran while the phone rerequest was still pending") + } + } + cli.decryptMessages(context.Background(), info, node) + + if !cancelled { + t.Fatal("the pending rerequest was never cancelled") + } +} diff --git a/message.go b/message.go index 05a493c0d..f189a30ce 100644 --- a/message.go +++ b/message.go @@ -309,6 +309,30 @@ func (cli *Client) handlePlaintextMessage(ctx context.Context, info *types.Messa return cli.dispatchEvent(evt.UnwrapRaw()) } +// notifyDecryptedPayload hands one plaintext to [Client.DecryptedPayloadHandler]. +// +// Called from both the path where the library goes on to interpret the +// plaintext and the path where something between Signal and that interpretation +// refused it, because the hook promises the bytes in either case. +func (cli *Client) notifyDecryptedPayload( + ctx context.Context, + info *types.MessageInfo, + node *waBinary.Node, + childIndex int, + encType string, + plaintext []byte, +) { + if h := cli.DecryptedPayloadHandler; h != nil { + h(ctx, DecryptedPayload{ + Info: info, + Node: node, + ChildIndex: childIndex, + EncType: encType, + Plaintext: plaintext, + }) + } +} + func (cli *Client) migrateSessionStore(ctx context.Context, pn, lid types.JID) { err := cli.Store.Sessions.MigratePNToLID(ctx, pn, lid) if err != nil { @@ -410,6 +434,13 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, cli.Log.Warnf("Ignoring message %s from %s: %v", info.ID, info.SourceString(), err) continue } else if err != nil { + // A non-nil plaintext here means Signal succeeded and something + // after it refused the result, so this is the last moment those + // bytes exist. Handing them over is the whole reason the hook is + // not simply placed on the success path. + if decrypted != nil { + cli.notifyDecryptedPayload(ctx, info, node, childIndex, encType, decrypted) + } cli.Log.Warnf("Error decrypting message %s from %s: %v", info.ID, info.SourceString(), err) if ctx.Err() != nil || errors.Is(err, context.Canceled) { return @@ -434,21 +465,16 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, }) return } + retryCount := ag.OptionalInt("count") + // Cancelled before the hook runs: a handler that blocks would + // otherwise let the pending phone rerequest fire for a message that + // did arrive and did decrypt. + cli.cancelDelayedRequestFromPhone(info.ID) + // Before anything reads the plaintext, including the protobuf // unmarshal below: decryption already advanced the ratchet, so a // payload this library cannot interpret is one nobody can recover. - if h := cli.DecryptedPayloadHandler; h != nil { - h(ctx, DecryptedPayload{ - Info: info, - Node: node, - ChildIndex: childIndex, - EncType: encType, - Plaintext: decrypted, - }) - } - - retryCount := ag.OptionalInt("count") - cli.cancelDelayedRequestFromPhone(info.ID) + cli.notifyDecryptedPayload(ctx, info, node, childIndex, encType, decrypted) var msg waE2E.Message var handlerFailed bool @@ -628,10 +654,15 @@ func (cli *Client) decryptDM(ctx context.Context, child *waBinary.Node, from typ return nil, nil, fmt.Errorf("failed to decrypt normal message: %w", err) } } + // The raw Signal output is returned alongside the error when unpadding + // rejects it. The ratchet has already advanced at this point, so those + // bytes are the only copy that will ever exist; discarding them here + // would put them out of reach of [Client.DecryptedPayloadHandler]. + raw := plaintext var err error - plaintext, err = unpadMessage(plaintext, child.AttrGetter().Int("v")) + plaintext, err = unpadMessage(raw, child.AttrGetter().Int("v")) if err != nil { - return nil, nil, fmt.Errorf("failed to unpad message: %w", err) + return raw, &ciphertextHash, fmt.Errorf("failed to unpad message: %w", err) } return plaintext, &ciphertextHash, nil } @@ -655,9 +686,11 @@ func (cli *Client) decryptGroupMsg(ctx context.Context, child *waBinary.Node, fr if err != nil { return nil, nil, fmt.Errorf("failed to decrypt group message: %w", err) } - plaintext, err = unpadMessage(plaintext, child.AttrGetter().Int("v")) + // Returned alongside the error for the same reason as in decryptDM. + raw := plaintext + plaintext, err = unpadMessage(raw, child.AttrGetter().Int("v")) if err != nil { - return nil, nil, err + return raw, &ciphertextHash, err } return plaintext, &ciphertextHash, nil } From 3fe5d8fd5c3c48c82f80e20c68b3ab30d4ec7fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas=20de=20Oliveira=20Lopes?= Date: Sat, 8 Aug 2026 19:43:42 -0300 Subject: [PATCH 3/5] test the handler at the call site so an unset hook costs a compare `notifyDecryptedPayload` misses the inliner by one point (cost 81 against a budget of 80), so every `` 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 `` 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. --- message.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/message.go b/message.go index f189a30ce..8324c11bd 100644 --- a/message.go +++ b/message.go @@ -314,6 +314,12 @@ func (cli *Client) handlePlaintextMessage(ctx context.Context, info *types.Messa // Called from both the path where the library goes on to interpret the // plaintext and the path where something between Signal and that interpretation // refused it, because the hook promises the bytes in either case. +// +// Both call sites test the handler themselves before calling. That reads like +// a duplicate of the test below and is not: this function misses the inliner +// by a point (cost 81, budget 80), so without the outer test an unset hook +// pays a call per instead of a compare. The test stays here too because +// nothing about the signature says the caller has to make it. func (cli *Client) notifyDecryptedPayload( ctx context.Context, info *types.MessageInfo, @@ -438,7 +444,7 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, // after it refused the result, so this is the last moment those // bytes exist. Handing them over is the whole reason the hook is // not simply placed on the success path. - if decrypted != nil { + if decrypted != nil && cli.DecryptedPayloadHandler != nil { cli.notifyDecryptedPayload(ctx, info, node, childIndex, encType, decrypted) } cli.Log.Warnf("Error decrypting message %s from %s: %v", info.ID, info.SourceString(), err) @@ -474,7 +480,9 @@ func (cli *Client) decryptMessages(ctx context.Context, info *types.MessageInfo, // Before anything reads the plaintext, including the protobuf // unmarshal below: decryption already advanced the ratchet, so a // payload this library cannot interpret is one nobody can recover. - cli.notifyDecryptedPayload(ctx, info, node, childIndex, encType, decrypted) + if cli.DecryptedPayloadHandler != nil { + cli.notifyDecryptedPayload(ctx, info, node, childIndex, encType, decrypted) + } var msg waE2E.Message var handlerFailed bool From 7af3c9f353cfd21b9e0c94dec532e146da5bc81d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas=20de=20Oliveira=20Lopes?= Date: Sat, 8 Aug 2026 19:43:42 -0300 Subject: [PATCH 4/5] let the benchmarked hook through, so both cases pay the same dispatch 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. --- hooks_bench_test.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/hooks_bench_test.go b/hooks_bench_test.go index 51d3b049f..e52847eef 100644 --- a/hooks_bench_test.go +++ b/hooks_bench_test.go @@ -15,12 +15,15 @@ import ( waLog "go.mau.fi/whatsmeow/util/log" ) -// Both hooks take their argument by value rather than by pointer, and these -// exist to keep it that way. A pointer would escape through the indirect call -// and cost one heap allocation per stanza. Small, and on the receive path of -// a long-running process, which is where small and per-stanza adds up. +// [RawNodeHandler] takes its argument by value rather than by pointer, and +// these exist to keep it that way. A pointer would escape through the indirect +// call and cost one heap allocation per stanza. Small, and on the receive path +// of a long-running process, which is where small and per-stanza adds up. // -// Compare the two: the numbers have to match. +// Compare the two: the allocation counts have to match. Both let the node +// through, so that the difference between them is the hook and not the work +// downstream of it — a hook that drops would skip the dispatch the unset case +// still pays for, and the two would not be comparable. // // go test -run XXX -bench BenchmarkRawNodeHook -benchmem func benchmarkFrame(b *testing.B) []byte { @@ -52,11 +55,16 @@ func BenchmarkRawNodeHookUnset(b *testing.B) { func BenchmarkRawNodeHookSet(b *testing.B) { cli := NewClient(&store.Device{}, waLog.Noop) + // Drained, as above. + go func() { + for range cli.handlerQueue { + } + }() cli.RawNodeHandler = func(_ context.Context, raw RawNode) (*waBinary.Node, bool) { if len(raw.Frame) == 0 { b.Fatal("the hook was handed no frame") } - return nil, true + return nil, false } frame := benchmarkFrame(b) From ebfccca4eb6f44c32699347ad4c75fb35a18290c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas=20de=20Oliveira=20Lopes?= Date: Sun, 9 Aug 2026 22:14:14 -0300 Subject: [PATCH 5/5] Say what the payload hook actually hands over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- client.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/client.go b/client.go index 67652b230..42df932fa 100644 --- a/client.go +++ b/client.go @@ -88,8 +88,15 @@ type RawNode struct { } // DecryptedPayloadHandler is called for every child this library -// decrypts, with the plaintext exactly as Signal produced it and before -// anything interprets it. +// decrypts, with the plaintext before anything interprets it. +// +// "Before anything interprets it" is the promise, not "byte for byte as the +// cipher emitted it". Signal's padding is stripped first, because the padded +// form is not the message and every caller would strip it again; what fires +// here is what the protobuf unmarshal below is about to read. The one +// exception is padding that will not strip, where no unpadded form exists and +// the padded bytes are handed over instead — the alternative there is nothing +// at all. // // It exists because decryption is irreversible. The ratchet has advanced // and a prekey may have been spent by the time the plaintext exists, so a