fix: validate worker request fields and wipe a refused upload chunk - #1241
Merged
FSM1 merged 4 commits intoAug 11, 2026
Conversation
The engine host read `beginWrite`, `snapshot`, `download`, `openContentStream` and `readStream` arguments straight off an untrusted worker message. wasm-bindgen coerces rather than refuses, so a 16-character string reached `NodeId::from_bytes` as sixteen zero bytes and a string or NaN ToInt32'd into a byte count: a malformed request became a valid one against a different node or window. Every field the host reads now passes the codec's checkers, with a finite non-negative check for the counts. The sending side also dropped an upload chunk it refused before any transfer, leaving readable plaintext in a failed-over or closing tab's heap. `EngineClient.pushChunk` now scrubs the chunk its unknown-handle refusal never hands on, and `CorrelatedTransport.request` scrubs what a send would have transferred on every route to a pre-send rejection — a terminal error, a refused readiness gate, or a throwing send. Closes #1154 Closes #1155
WalkthroughThe client now scrubs rejected secret and chunk buffers. Transport layers propagate transfer lists through dispatch. Worker request fields use strict type validation before WASM calls. ChangesSecurity and boundary validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Name the transfer scrub `wipeTransfer` so it no longer collides with the relay's payload-shaped `wipeCarried`, state each ownership rationale once at its home, and cover the transport wiring that declares what a send would have moved: without a test, undoing either `LocalTransport` argument left every suite green.
Security-review follow-ups on the same two boundaries. `start` and `pushChunk` took their payload straight into `new Uint8Array(buffer)`, which coerces: a view is copied rather than referenced, so the scrub afterwards zeroes the copy and leaves the sender's plaintext, and a transferable that is not an `ArrayBuffer` yields a zero-length push. Both now go through a buffer check like every other field. `EngineClient.start` refused a closed client before scrubbing the secret it had already decided not to forward — the same refuse-before-transfer shape as the chunk. The seam contract now says outright that a buffer is consumed on every outcome, so a retry re-reads its source rather than re-sending one a retryable rejection scrubbed.
FSM1
marked this pull request as ready for review
August 10, 2026 20:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client/src/correlatedTransport.ts`:
- Around line 75-85: Update wipeTransfer to detect ArrayBuffer values without
relying on instanceof, so buffers from other JavaScript realms are wiped before
the existing Uint8Array fill operation. Preserve the current handling for
undefined transfers, already-empty buffers, and other transferable types.
In `@packages/client/src/worker/commandCodec.ts`:
- Around line 31-45: Update buildCommand to validate the command envelope with
record and validate its kind with text before reading kind or entering the
switch, so null, primitive, and non-string values consistently raise invalid
request field errors. Add regression coverage confirming malformed envelopes do
not invoke any WASM command factory.
In `@packages/client/src/worker/engineHost.ts`:
- Around line 138-139: Validate every WriteHandle and StreamHandle with a shared
runtime bigint validator before invoking WASM in pushChunk, commitWrite,
abortWrite, and closeStream. Ensure malformed handles reject at the worker
boundary without reaching the WASM call, and add behavior tests covering invalid
handles for each affected operation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7895dcdf-b6db-4bb6-bfb5-804107b77074
📒 Files selected for processing (12)
packages/client/src/broadcastTransport.test.tspackages/client/src/broadcastTransport.tspackages/client/src/correlatedTransport.test.tspackages/client/src/correlatedTransport.tspackages/client/src/engineClient.test.tspackages/client/src/engineClient.tspackages/client/src/transport.test.tspackages/client/src/transport.tspackages/client/src/worker/commandCodec.test.tspackages/client/src/worker/commandCodec.tspackages/client/src/worker/engineHost.test.tspackages/client/src/worker/engineHost.ts
FSM1
marked this pull request as draft
August 10, 2026 20:54
… worker Three fields reached past the boundary this PR exists to guard. wipeTransfer branded transferables with instanceof ArrayBuffer, which is false for one minted in another realm, so a secret arriving from a worker or a frame skipped the scrub. Branded by the byteLength getter now, which answers across realms and returns null for everything else. buildCommand read descriptor.kind before validating the envelope, so a null or primitive command answered with a TypeError or an unknown-kind error naming undefined, rather than the invalid-field refusal every other malformed input gets. Write and stream handles crossed to WASM unchecked while their sibling fields were validated. minted() is the shared bigint check, applied at pushChunk, commitWrite, abortWrite, readStream and closeStream. commitWrite becomes async so its refusal rejects like the other four rather than throwing synchronously out of the call.
Owner
Author
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
wipeTransfer brands with instanceof ArrayBuffer, false across realms |
Taken. Reproduced the realm behaviour locally rather than trusting the script: instanceof false, getter brand 4, null for {}/null/MessagePort, and the fill does zero foreign bytes. Now branded by the byteLength getter. Chose it over Object.prototype.toString because Symbol.toStringTag is writable — a buffer could be made to answer something else and skip the scrub, the wrong way to fail on a plaintext-clearing path. |
buildCommand reads kind before validating the envelope |
Taken. An inconsistency with this file's own header, which says every field the worker reads passes a checker — beginWrite does, the envelope did not, and serve.ts:53 hands it straight from the wire. null gave a TypeError; a primitive or missing kind gave unknown command kind: undefined. Both answer invalid request field … now. |
| write and stream handles forwarded to WASM unchecked | Taken at all five sites. pushChunk validated chunk but not handle; readStream validated offset/length but not handle; commitWrite, abortWrite, closeStream validated nothing. One shared minted() checker now, replacing the private opId so there is one bigint check rather than two. |
One thing the review did not name, found while fixing it
commitWrite was the only one of the five not async, so its refusal threw synchronously out of the call while the other four rejected. A boundary should refuse in one shape; it is async now. This surfaced only because the new test asserted rejects uniformly and commitWrite alone failed — it was not visible from reading the diff.
Coverage added
7 cases, each verified as a true negative control (reverting the guard fails it, restoring it passes):
- 5 ×
refuses a {pushChunk,commitWrite,abortWrite,readStream,closeStream} carrying a handle the engine never minted— each also assertscallsis empty, so an empty WASM call log is the evidence none ran refuses an envelope that is not a command before it reads a kind off it—null, a number, a string, a non-stringkind, and a missingkindwipes a chunk minted in another realm, which instanceof does not answer for— a real foreign buffer vianode:vm, assertinginstanceofis false for it and that the bytes end zeroed
Gates on b718e2f
pnpm lint 0 · pnpm typecheck 0 · pnpm --filter @cipherbox/client test 0 (29 files, 441 tests) · pnpm lint:tracker-refs 0.
Generated by Claude Code
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
1 participant
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
packages/clientboundary fixes: the worker validates the request fields it reads instead of letting wasm-bindgen coerce them, and the sending side scrubs an upload chunk it refuses before any transfer.Validate the worker's read and write request fields
EngineHost.beginWrite,snapshot,download,openContentStreamandreadStreamread their arguments straight off the worker message, exactly ascommanddid before its codec gained checkers. wasm-bindgen coerces rather than throws, so a 16-character string reachedNodeId::from_bytesas sixteen zero bytes, and a string orNaNToInt32'd into a byte count — a malformed request became a different valid request rather than a refusal.WriteTarget.parent/name/node,beginWritesize,snapshotfolder,download/openContentStreamnode,readStreamoffset and length. Byte fields go throughnodeId, which is thebyteschecker composed withNodeId.fromBytes— the same check, spelled once.count: a non-negative safe integer, sincetypeofalone still admitsNaN,1.5and-1.recordrefuses aWriteTargetthat is not an object, so the variant discriminator cannot throw a bareTypeError.bufferguards the two transferred payloads (start's secret,pushChunk's chunk), which went straight intonew Uint8Array(payload)— a coercion with a sharper edge than the rest, since aUint8Arraythere is copied, so the scrub afterwards zeroes the copy and leaves the sender's plaintext.beginWrite/openContentStreambecameasync, so a refused field rejects rather than throwing synchronously — the rest of the surface already did.invalid request field …rather thaninvalid command field …: the same checkers now guard requests that carry no command.Wipe an upload chunk the sender refuses before it is transferred
A chunk refused before any transfer is never detached, so the refusing frame is its terminal owner and nothing downstream will ever scrub it (AGENTS.md rule 7). A failed-over or closing tab was leaving up to one chunk of file plaintext in its heap indefinitely.
EngineClient.pushChunkscrubs the chunk its unknown-handle refusal declines to forward, mirroring howstarthandles a secret it declines.CorrelatedTransport.requestscrubs what the send would have transferred on every route to a pre-send rejection: an already-latched terminal error, one latched while the readiness gate was awaited, a rejected gate, and a throwingsend. A transferred buffer reads as empty, so a send that did run leaves the scrub a no-op — and the happy path is asserted to keep its bytes, since wiping a buffer the receiver is about to seal would corrupt the upload.EngineClient.startrefused a closed client before scrubbing the secret it had already declined to forward — the same refuse-before-transfer shape, found by the security pass.EngineTransportnow states outright that a buffer it takes is consumed on every outcome, transferred away or scrubbed in place. A retryable rejection (leader changed; retry) hands back a zeroed buffer, so a retry must re-read its source; both upload callers already do, and the contract was the only thing left implicit.Wiring the two transports to declare what they transfer is 3 lines outside the issues' stated files (
transport.tsstart/command/pushChunk,broadcastTransport.tsoverPort); without it the base-class scrub would never see a real chunk.Verification
Each behaviour was broken deliberately and the failing test recorded, then restored — 20 mutants, 20 caught, including an over-wipe mutant that scrubs after a successful send and two that undo the transport wiring. The wipes are asserted on buffer contents after the rejection, never on a spy.
Closes #1154
Closes #1155
Note
Validate worker request fields and zero-fill refused upload chunks
EngineHostmethods (start,beginWrite,pushChunk,commitWrite,abortWrite,readStream,closeStream,snapshot,download,openContentStream) using new helpers (buffer,count,minted,record,nodeId) in commandCodec.ts.CorrelatedTransport.requestto accept a transfer list and zero-fill anyArrayBufferentries on pre-send failure paths (terminal error, gate rejection, or send exception); successful transfers are unaffected.EngineClient.pushChunknow scrubs the chunk buffer when the write handle is unknown or stale;EngineClient.startscrubs the secret when called on a closed or non-leader client.'invalid request field'across the worker codec.Macroscope summarized b718e2f.
Summary by CodeRabbit
Security
Bug Fixes
Validation