fix: three parity defects the differential fuzzers found - #47
Conversation
Two findings from the bridge fuzzer, both in the event dispatch.
An event that arrived with no data slot threw a TypeError on 30 of the 58
declared types — `data.code` on `undefined`. `adapt.ts` documents the
opposite ("null on unrecoverable shape mismatch"), and the throw does not
stay local: it propagates into the socket's event dispatch and takes down
the whole event loop rather than the one event.
The slot is normalised rather than rejected, because rejecting would
change behaviour the adapters already define. Measured on those 30 with
an empty slot: 16 return null of their own accord, 11 return a `noop`,
and three return the fallback their own author wrote — `stream_error` its
`?? 'unknown'`, `pair_error` its 'Unknown pairing error', and
`offline_sync_completed` its `count: 0`. Nothing is invented; the throw
becomes what the adapter already does with `{}`. 30 throws to 0.
The table is a plain object literal indexed by a string the server
chooses, so `constructor` and `toString` resolved through
`Object.prototype`, were called, and had their return values handed on as
canonical events — an object and a string respectively. `__proto__` and
`valueOf` reached the other failure mode and threw. The lookup now
requires an own property, so every inherited name drops as unrecognised.
Both are pinned in adapt.test.ts, and removing either fix fails those
pins. The two registry entries that documented them are deleted along
with the helpers they needed: an allowlist that outlives its divergence
is how the same bug comes back unnoticed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
📝 WalkthroughWalkthroughBridge event adaptation now rejects prototype-chain event types and normalizes missing data before adapter dispatch. Tests cover inherited names, absent or null data, required-data events, and fallback adapters across all known event types. ChangesBridge event adaptation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0a1cbff78
ℹ️ 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".
| // `?? 'unknown'`, `pair_error` its 'Unknown pairing error', and | ||
| // `offline_sync_completed` its `count: 0`. Nothing is invented here; the throw | ||
| // simply becomes whatever the adapter already does with `{}`. | ||
| const data = typed.data ?? {} |
There was a problem hiding this comment.
Reject absent history-sync payloads before normalization
When a history_sync event has omitted or null data—the malformed input this change is intended to handle—typed.data ?? {} turns it into a valid plain object. The history-sync adapter consequently bypasses its !isObject(data) rejection, constructs an empty final batch, and src/Socket/events.ts emits a spurious messaging-history.set; before this change the same shape returned null. Preserve the distinction between an absent payload and an actual {} for adapters whose object guard determines whether the event is recoverable.
Useful? React with 👍 / 👎.
…y slot Review of #47 caught a regression in the previous commit. Substituting `{}` for a missing data slot removed the throws, but `{}` is a perfectly good object — so it walked straight past the `isObject(data)` guard several adapters use to decide a payload is unrecoverable. `history_sync` then built an empty final batch and the socket emitted a spurious `messaging-history.set` where it had previously dropped the event. The slot is passed through as it arrived and every adapter in the table reads it optionally instead, so the "never throw" property is kept while each adapter keeps the last word on its own payload. Verified across all 58 declared types with both `undefined` and `null`: 0 throw, 26 return null, 21 return a noop, and 11 emit — the six that never read a slot plus five with a fallback their own author wrote. `history_sync` is back to null and pinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
`generateForwardMessageContent` shallow-cloned the outer message map and then assigned `contextInfo` onto the *nested* object, which that clone still shares with the caller. And it replaced rather than merged, so forwarding a quoted message stripped `stanzaId` and `participant` from the caller's own object. Upstream leaves the argument untouched. Only the one object being written is rebuilt, rather than paying for upstream's `proto.Message.decode(proto.Message.encode(content))` round trip on a send path. Allocation count is unchanged on the plain-text path — the `extendedTextMessage` it builds there is already ours, so the context goes straight into the literal — and one shallow spread higher elsewhere, which is the irreducible cost of not writing into someone else's object. Measured with each input consumed once, because the old implementation mutates its argument and a reused pool would have it take a different path from the second pass on: conversation 540-556 ns/op -> 530-538 ns/op extendedTextMessage 221-253 ns/op -> 155-188 ns/op imageMessage 228-302 ns/op -> 198-201 ns/op Two of the three registry entries on this helper stay: `copy-shape` and `int64-truncation` come from upstream's round trip converting through Long and reshaping the key set, not from the aliasing, and matching them means buying that round trip. The fuzzer confirms it — after the fix those two still excuse findings and the mutation entry excuses nothing, so it is deleted along with the three helpers only it used. An earlier version of the fix spread the nested slot unconditionally and invented a property named "undefined" on a message with no content key, where upstream raises a TypeError. The differential caught it; that path now keeps the original assignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg
|
Two commits pushed. The review finding was a real regression in my previous commit, and the PR now also carries a second fix. The
|
| shape | before | after |
|---|---|---|
| conversation | 540–556 ns/op | 530–538 ns/op |
| extendedTextMessage | 221–253 ns/op | 155–188 ns/op |
| imageMessage (8 fields) | 228–302 ns/op | 198–201 ns/op |
Two of the three registry entries on this helper stay open, deliberately. copy-shape and int64-truncation come from upstream's round trip converting through Long and reshaping the key set — not from the aliasing — so matching them means buying that round trip. The fuzzer settles it rather than my reading: after the fix those two still excuse findings and the mutation entry excuses nothing, so only that one is deleted. I had earlier guessed this change would close all three; it closes one.
An earlier version of the fix spread the nested slot unconditionally and invented a property named "undefined" on a message with no content key, where upstream raises a TypeError. The differential caught it (89 pass / 3 fail against a 90 / 2 baseline); that path now keeps the original assignment.
Verification
tsc --noEmit, oxlint, oxfmt --check clean. npm test 1181 pass / 0 fail. npm run fuzz 185 pass / 0 fail. Deep pure-differential back to 90 pass / 2 fail, matching the pre-change baseline exactly. New pins fail when their fix is removed: 3 failures for the forwarding aliasing, 2 for the bridge guards.
Generated by Claude Code
The first draft asked whether some surviving entry shared the mention's leading segment. That reads as a heuristic and behaves as a blind spot: four of the 26 ids are the only one carrying their prefix, so deleting poll-vote-aggregation-order, binary-node-messages-tolerates-bad-payload, event-buffer-merge-precedence or to-number-high-word would have taken the check for their own references down with them. Verified by deleting the first and adding a reference to it: the prefix version ignores the mention, this one names the file. It also hid a live one. generators/bridge-event.ts says "the registry already carries `bridge-adapter-prototype-chain-lookup` for this class of defect", and the registry has not carried it since #47 — which removed the entry because it fixed the defect. `adaptBridgeEvent` guards its table with Object.hasOwn and src/Bridge/__tests__/adapt.test.ts pins it, so the comment was pointing readers at a lease on a bug that no longer exists. Rewritten to say where the fix lives. Two names in these sources are spelled like an entry id and are not one. They are listed rather than pattern-matched: an unregistered id is a question somebody answers, and a heuristic answers it silently.
Three defects the fuzzers from #43 left open in the registry. Two in the bridge's event dispatch, one in the send path.
1. A missing data slot threw instead of dropping the event
30 of the 58 declared event types threw a
TypeErrorwhen an event arrived with no data slot —data.codeonundefined.adapt.tsdocuments the opposite contract ("Result isnullon unrecoverable shape mismatch"), and the throw does not stay local: it propagates into the socket's event dispatch and takes down the whole event loop rather than the one event.Every adapter in the table now reads its slot optionally, so the dispatch passes it through exactly as it arrived. Across all 58 types, with both
undefinedandnull:Six of the eleven never read a slot at all; the other five return a fallback their own author already wrote (
stream_errorits?? 'unknown',pair_errorits'Unknown pairing error',offline_sync_completeditscount: 0).2. The event type resolved through the prototype chain
The adapter table is a plain object literal indexed by a string the runtime takes from the server, so an untrusted value was indexing a prototype-bearing lookup.
constructorandtoStringresolved to inherited functions, were called, and had their return values handed on as canonical events — an object and a string respectively.__proto__andvalueOfreached the other failure mode and threw.The lookup now requires an own property.
Events that were already fine are untouched:
connectedstill adapts,qrwith a code still adapts,qrwithout one still drops.3. Forwarding rewrote the caller's own message
generateForwardMessageContentshallow-cloned the outer message map and then assignedcontextInfoonto the nested object, which that clone still shares with the caller. And it replaced rather than merged, so forwarding a quoted message strippedstanzaIdandparticipantfrom the caller's own object. Upstream leaves the argument untouched.Only the one object being written is rebuilt, rather than buying upstream's
proto.Message.decode(proto.Message.encode(content))round trip on a send path. Allocation count is unchanged on the plain-text path — theextendedTextMessagebuilt there is already ours, so the context goes straight into the literal — and one shallow spread higher elsewhere, which is the irreducible cost of not writing into someone else's object.Benchmarked with each input consumed exactly once, because the old implementation mutates its argument and a reused pool would have it take a different path from the second pass onward (the forwarding score accumulates):
Two of the three registry entries on this helper stay open, deliberately.
copy-shapeandint64-truncationcome from upstream's round trip converting throughLongand reshaping the key set — not from the aliasing — so matching them means buying exactly the round trip this avoids. The fuzzer settles which is which rather than a reading of the code: after the fix those two still excuse findings and the mutation entry excuses nothing.Registry
Three entries deleted, along with the helpers only they used:
bridge-adapter-throws-on-missing-data,bridge-adapter-prototype-chain-lookup,forward-message-content-mutates-input. An allowlist that outlives its divergence is how the same bug comes back unnoticed. Open findings go from 25 to 22.Verification
New pins in
src/Bridge/__tests__/adapt.test.tsandsrc/__tests__/forward-message-content.test.ts, each of which fails when its fix is removed:tsc --noEmit,oxlint,oxfmt --checkclean.npm test1181 pass / 0 fail.npm run fuzz185 pass / 0 fail. Deeppure-differentialat 90 pass / 2 fail, matching the pre-change baseline exactly; bridge fuzzer clean on seedsfixed d1 d5 d9 y1.Not in this change
proto-decode-above-max-safe-integeris the most severe finding in the registry — any 64-bit field outside ±(2^53−1) fails the whole message decode — but the throw comes from@oxidezap/whatsapp-rust-bridge, so it needs a change there rather than here.🤖 Generated with Claude Code
https://claude.ai/code/session_01RvtvVVWQs8AeBqSzS1JpCg