Repository navigation
fix(socket): reject an invalid enum argument before it reaches the bridge - #40
Conversation
A bot called `groupParticipantsUpdate(from, [id], '☠️')` from a `setTimeout`,
with no `await` and no `.catch`. The value crossed into the bridge untouched,
the core's deserializer rejected it, and the consumer got an unhandled
rejection whose frames are all `wasm://wasm/<hash>`. Nothing in it names the
line that made the call, which is the only thing its author needed.
These drive the real public socket against a port nothing listens on, so a
value that passes validation fails downstream as "not connected" instead of
reaching a server. That separates rejected-by-us from not-rejected-by-us, and
lets one table cover every closed-domain argument on the surface.
Today 26 of them fail, in three ways:
- a wasm rejection with no JS frame: groupParticipantsUpdate,
groupRequestParticipantsUpdate, groupMemberAddMode, updateBlockStatus,
sendPresence, sendChatState, sendPresenceUpdate, profilePictureUrl and
communityParticipantsUpdate
- nothing at all: groupJoinApprovalMode takes anything but 'on' as off, the
privacy wrappers put the typo on the wire, sendReceipt/sendReceipts log a
debug line and resolve, downloadMediaMessage returns a stream where a
buffer was asked for
- a message about something else: groupSettingUpdate throws "Cannot read
properties of undefined", waUploadToServer names no parameter,
newsletterMetadata lists no accepted values
The source scan and the bridge .d.ts comparison pass already: they exist so a
method added later cannot skip this without saying why, and so a value the
core starts accepting shows up here instead of being rejected in production.
…idge
A value outside a closed set now fails at the method that took it, with a
message naming the method, the parameter, what arrived and everything that is
accepted, and with the caller's own frame in the stack:
Error: groupParticipantsUpdate: "action" must be one of "add", "remove",
"promote", "demote", "modify", received "☠️"
at Object.groupParticipantsUpdate (src/Socket/groups.ts:142:4)
at Timeout._onTimeout (bot.js:9:4)
Where the check runs is the whole point. The bridge already rejects these, but
it builds the rejection inside wasm, where there is no JS frame to capture, so
a fire-and-forget call surfaces an unhandled rejection made entirely of
`wasm://wasm/<hash>` frames. Running in the synchronous prefix of the public
method, ahead of the first `await`, is what puts the call site in the stack:
V8 stitches async frames onto an awaiting caller, and a fire-and-forget call
has none.
Values are never a second list. Each domain is the array its type is derived
from (`type ParticipantAction = (typeof PARTICIPANT_ACTIONS)[number]`), so the
compiler and the runtime check read the same definition, and the group-setting
domain comes off the alias table the method already dispatches through. What
the bridge accepts is compared against those arrays in the tests, so a value
the core starts taking shows up as a failing test rather than as a valid call
this layer refuses.
Three of these were not reaching the bridge at all: `groupJoinApprovalMode`
took anything but 'on' as off, `sendReceipt`/`sendReceipts` logged a debug line
and resolved, and `downloadMediaMessage` returned a stream to a caller asking
for a buffer. Those failed silently rather than loudly, which is worse.
The public signatures are unchanged: the audit reports the same 97.92%
(21896/22361) as before. `newsletterMetadata` already refused an unknown key
type, and its bespoke message is now the shared one, which lists the accepted
values it used to omit.
…t tests Two tests now read `whatsapp_rust_bridge.d.ts`, each with its own copy of the candidate paths, so a bridge that moves its declarations again would have to be found twice. One loader, one list of places to look, plus the union reader the new test needed.
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on September 2. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe pull request centralizes string-value domains, adds runtime validation for socket and media APIs, derives public types from shared constants, and adds comprehensive closed-domain validation tests. ChangesClosed-domain validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 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: 89960bc928
ℹ️ 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".
…ient Two entry points reported the wrong thing. `sock.downloadMedia` awaits the bridge client while building the context it passes down, so the check inside `downloadMediaMessage` ran past that await: an invalid `type` with no client available surfaced the connection error instead, and a fire-and-forget call lost the frame the check exists to keep. It now checks in the wrapper too, before the await. The helper keeps its own check because it is exported and called directly as well. The four community methods that are the group operation delegated straight to it, so an invalid argument to `communityRequestParticipantsUpdate` came back as a failure of `groupRequestParticipantsUpdate`, a method the consumer never called. They validate under their own names now and delegate afterwards.
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 4 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 4 unresolved issues from previous reviews.
Re-trigger cubic
`String(value)` throws on a null-prototype object and on anything whose conversion throws, so rendering the value for the message could replace the 400 with a TypeError raised inside the report itself. That is the failure this whole path exists to remove, so it falls back to the object tag. While in there: the bridge union reader matched a single physical line, and the generator already wraps long unions across lines elsewhere in the same file. A body-shaped match survives that, instead of failing as "the bridge no longer declares X" over a formatting change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69a760008d
ℹ️ 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.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Focused bug fix: adds synchronous closed-domain argument validation across the socket API so invalid enum values produce caller-readable 400 errors instead of wasm-only rejections or silent no-ops. Tests pin the behavior; valid inputs and existing contracts are unchanged.
Re-trigger cubic
`MEDIA_TYPES` is exported from the package root and is what `waUploadToServer` checks against, but it was typed as a mutable `MediaType[]`: a consumer could edit the list and change what every upload is allowed to send. Every other domain is declared `as const`, so this was the one that did not say readonly. The assertions fail the build if either guarantee goes away.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92942a7ffc
ℹ️ 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".
The fallback added for a value that cannot be converted still read the value: `Object.prototype.toString` looks up `Symbol.toStringTag`, which throws on a revoked proxy and on a trap that throws, so the caller got that error instead of the 400 naming the parameter. `typeof` reads nothing off the value, so reporting it cannot fail on it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f4d5be672
ℹ️ 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".
Same gap as `MEDIA_TYPES`, in the domain next to it: `GROUP_SETTINGS` is exported, reachable through the package's `./lib/*` entry, and read live by `groupSettingUpdate` and `communitySettingUpdate`. Editing it would make a valid setting start failing for every caller. It carries an assertion alongside the other two, so the whole class is pinned rather than the instance that happened to be reviewed.
There was a problem hiding this comment.
0 issues found across 5 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Auto-approved: Focused bug fix: adds synchronous closed-domain argument validation to socket methods so invalid enum values fail with readable 400 Boom errors instead of silent or wasm-only failures, with tests pinning the behavior. No valid-call behavior or operational tradeoff changes remain.
Re-trigger cubic
Summary
A closed-domain argument that is outside its set now fails at the method that took it.
Before,
groupParticipantsUpdate(jid, [p], '☠️')produced this, and nothing else:Now:
It is a
Boom(400)carryingdata: { parameter, value, accepted }, so the reason is readable by ahuman and branchable by code. Thirty arguments across the socket surface report this way: nine used
to fail inside the bridge with a wasm-only stack, thirteen used to not fail at all, and three failed
with a message about something else.
Reproduction
src/__tests__/closed-domain-arguments.test.ts, committed failing first (91defe4). It drives thereal public socket against a port nothing listens on, so a value that passes validation fails
downstream as "not connected" rather than reaching a server: rejected-by-us and not-rejected-by-us
stay distinguishable. The headline test makes the call the way the report did, inside a
setTimeoutwith no
awaitand no.catch.Red, 26 of 65 failing:
Green after 8399e33, with the stack pointing at the timer callback that made the call:
The three failure shapes the red run recorded, which is what these arguments looked like before:
groupParticipantsUpdate,communityParticipantsUpdate,groupRequestParticipantsUpdate,groupMemberAddMode,updateBlockStatus,sendPresence,sendChatState,sendPresenceUpdate,profilePictureUrlgroupJoinApprovalMode(anything but'on'meant off), the eight privacy wrappers (the typo went on the wire),cleanDirtyBits(same),sendReceipt/sendReceipts(debug line, then resolve),downloadMediaMessage(a stream where a buffer was asked for)groupSettingUpdate("Cannot read properties of undefined"),waUploadToServer(named no parameter),newsletterMetadata(listed no accepted values)Root cause
The value crossed the bridge boundary unchecked. The core rejects it correctly, and the bridge turns
that into a proper rejection naming the field, but the
Erroris constructed inside wasm: the framesit captures are the wasm ones plus the shim, and none of them belong to the caller.
V8 does stitch async frames onto an awaiting caller, so a consumer doing
await sock.…()would atleast see its own function. The reported call was made from a
setTimeoutwith noawaitand no.catch, which is the common shape for fire-and-forget bot commands. There is no awaiting frame, sothe rejection carries nothing but wasm addresses.
That is why the check has to run in the synchronous prefix of the public method, ahead of the
first
await: theErroris then constructed on the caller's own stack. A check placed afterawait ctx.getClient()would produce a better message and the same useless stack.Not a regression
This case never worked. Upstream Baileys uses
actionas the tag name of the node it sends:With
'☠️'that builds<☠️>on the wire, the server answers with no node of that name,getBinaryNodeChildreturnsundefined,getBinaryNodeChildren(undefined, 'participant')returns[], and the call resolves with an empty array. The participant was never removed and nothingsaid so. baileyrs did not introduce the defect; it made a latent one visible, badly.
Worth noting for anyone auditing their own bot:
'modify'is a real value here. The core takesadd | remove | promote | demote | modify, one more than the classic upstreamParticipantAction,and the tests below pin our set to the bridge's.
Scope
Every closed-domain argument on the socket surface, from a source scan rather than a search by name.
Socket/groups.tsgroupParticipantsUpdateactionParticipantActionSocket/groups.tsgroupRequestParticipantsUpdateactionGroupRequestActionSocket/groups.tsgroupSettingUpdatesettingSocket/groups.tsgroupMemberAddModemodeMemberAddModeSocket/groups.tsgroupJoinApprovalModemodeJoinApprovalModeSocket/communities.tscommunityParticipantsUpdateactionParticipantActionSocket/communities.tscommunityRequestParticipantsUpdateactionGroupRequestActionSocket/communities.tscommunitySettingUpdatesettingSocket/communities.tscommunityMemberAddModemodeMemberAddModeSocket/communities.tscommunityJoinApprovalModemodeJoinApprovalModeSocket/privacy.tsupdateLastSeenPrivacyvalueWAPrivacyValueSocket/privacy.tsupdateProfilePicturePrivacyvalueWAPrivacyValueSocket/privacy.tsupdateStatusPrivacyvalueWAPrivacyValueSocket/privacy.tsupdateOnlinePrivacyvalueWAPrivacyOnlineValueSocket/privacy.tsupdateReadReceiptsPrivacyvalueWAReadReceiptsValueSocket/privacy.tsupdateGroupsAddPrivacyvalueWAPrivacyGroupAddValueSocket/privacy.tsupdateCallPrivacyvalueWAPrivacyCallValueSocket/privacy.tsupdateMessagesPrivacyvalueWAPrivacyMessagesValueSocket/blocking.tsupdateBlockStatusactionBlockActionSocket/presence.tssendPresencestatusWAPresenceStatusSocket/presence.tssendChatStatestateWAChatStateSocket/contacts.tsprofilePictureUrltypeProfilePictureTypeSocket/newsletter.tsnewsletterMetadatatypeNewsletterKeyTypeSocket/server-queries.tscleanDirtyBitstypeDirtyBitTypeSocket/index.tssendPresenceUpdatetypeWAPresenceSocket/index.tswaUploadToServeropts.mediaTypeMediaTypeSocket/index.tsdownloadMediatypeMediaDownloadTypeSocket/messages.tssendReceipttypeMessageReceiptTypeSocket/messages.tssendReceiptstypeMessageReceiptTypeUtils/messages.tsdownloadMediaMessagetypeMediaDownloadTypeThe community methods that are the group operation check under their own names before delegating, so
a refusal never points at a method the consumer did not call; same for
downloadMedia, which checksbefore awaiting the client and then delegates to
downloadMediaMessage, itself an exported entrypoint with its own check.
The scan also found four that are deliberately not validated, each recorded with its reason in the
test's
EXEMPTtable:internals.ts:resyncAppState:collectionsserver-queries.ts:createCallLink:_typeinternals.ts:upsertMessage:typebusiness.ts:minutesPastMidnight:whichupdatePrivacySetting(category, value)stays open on purpose and is documented as such: both halvesare
stringat the bridge and the core's wire enums carry a fallback, so it is the escape hatch fora category the eight wrappers do not name yet.
Design
One check, called at each boundary.
assertArgumentDomain(method, parameter, value, domain)insrc/Utils/argument-domain.tsis the only place that builds this error. It is called from thesynchronous prefix of each method rather than from a wrapper around the socket, because that is the
only position that captures the caller's frame, and because a
Proxyover the socket would sit onthe hot path of every property read for the sake of thirty arguments.
The values are the definition, not a copy of it. Each domain is the array its type is derived
from:
Same for the privacy values, presence, chat states and receipt types.
MediaTypealready derivedfrom
MEDIA_HKDF_KEY_MAPPING, so its domain isObject.keysof that map;GroupSettingnow derivesfrom the alias table the method dispatches through, so the accepted set and the mapping cannot
disagree. No domain is written down twice.
When the core changes. The risk of a check here is refusing a value the core has started
accepting, which would be worse than the bug. Three things guard it:
.d.tsunions, exactly, forGroupParticipantAction,GroupRequestAction,MemberAddMode,BlockAction,PresenceStatus,ChatStateandPictureType; a bridge bump that adds a variant fails CI here firstwhose bridge types carry a
| stringfallback), the tests assert ours is a subset of what the coreknows, so the narrowing stays intentional
waUploadToServeracceptsNot forgetting the next one. A source scan resolves every closed-domain type in the package
(literal unions,
(typeof X)[number],keyof typeof X) and then requires every socket parametertyped by one of them to be either in the test table or in
EXEMPTwith a reason. A method addedlater with an off-domain argument fails that test rather than shipping unchecked.
Nothing coerces or falls back.
'REMOVE'is refused rather than lowercased,nulland''arerefused, and an omitted optional argument still takes its default (
profilePictureUrl(jid)is'preview');undefinedis accepted only where the domain contains it, which is how upstream spellsa delivery receipt in
MessageReceiptType.Validation
compat:auditreports 97.92% (21896/22361), identical to the score on the unmodified tree: thepublic signatures did not change, only what they refuse.
Every guard site was reverted one at a time, with the suite re-run after each: all of them fail a
test when removed, none is dead code. Reverting the single line in
groupParticipantsUpdatefailsthe reported call rejects with the call site in the stackandgroupParticipantsUpdate rejects an off-domain action; reverting the whole change is the red commit above, at 26 failures.npm run test:e2ewas not run locally (it needs the mock server); CI runs it and it is green.One existing test changed.
newsletter-compatibilityasserted the wording of the bespoke checknewsletterMetadataalready had (unknown key type 'nope'), which is now the shared message; theassertion still pins that the call is refused rather than treated as a jid, and now also that the
accepted values are listed.