fix: fail closed on the transferred upload chunk and on an unknown worker command

[FSM1]

Two fail-closed fixes in the engine worker, in one PR. Both live in
packages/client/src/worker/**, touch disjoint files, and share one review lens,
so they are reviewed together rather than as two branches of a hundred lines each.

---

Part 1 — #1037, wipe the transferred upload chunk

EngineHost.pushChunk receives the upload chunk by transfer, so the worker realm is its terminal owner — nothing upstream still holds the buffer. wasm-bindgen marshals the Uint8Array view into a Vec<u8> synchronously, before push_chunk returns its promise, so once the call has been made the JS-side copy is pure residue.

It was left for the collector. This wipes it.

What changed

• EngineHost grows one private scrubbing helper: it runs a call over a view of the buffer and fills the view with zeroes in a finally, so the failure path scrubs too. start already did this inline for the login secret; pushChunk now shares the same helper rather than repeating the idiom.
• EngineHostLike.pushChunk states the ownership transfer, so an implementer knows the buffer is the host's to scrub.

The engine keeps its own copy in a Zeroizing<Vec<u8>> for the duration of the push, so scrubbing after the call settles rather than between the marshal and the await opens no new window.

Tests

packages/client/src/worker/engineHost.test.ts gains two cases: a chunk handed to the host is all zeroes after a resolved push and after a rejected one, with the WASM-side copy asserted to have seen the real bytes so the wipe cannot pass by wiping too early.

Not in this PR

The crypto-privacy pass found the same residue class on the sending side: EngineClient.pushChunk and CorrelatedTransport.request drop a chunk they refuse before any transfer, so it is never detached and never wiped. Those files are outside this batch's ownership — filed as #1155.

Gates

pnpm typecheck 0, pnpm test 0, pnpm lint 0, pnpm lint:tracker-refs 0.

---

Part 2 — #1051, fail closed on an unknown or wrong-typed command

buildCommand switched over CommandDescriptor with no default arm, so an unknown kind fell out returning undefined and only failed because the wasm-bindgen glue happened to reject it — fail-closed by accident, and inconsistent with the three sibling switches in the same file that already fail closed deliberately.

The sharper case was a known kind carrying a wrong-typed field. { kind: 'rename', node: <16 bytes>, newName: 12345 } reached wasm.Command.rename(nodeId, 12345) and wasm-bindgen's USVString marshalling coerced it, renaming the node to "12345" instead of rejecting the command. Reachable from a version-skewed follower build: the private-port relay validates the command shape and deliberately leaves field validation to the codec, so that division only holds if the codec enforces it.

What changed

• Every field the builders read is checked against the type the protocol declares — byte arrays must be a real Uint8Array, strings a real string, opId a real bigint. The checkers take unknown, because the descriptor arrives as plain data across a realm boundary and its static type is a claim, not a guarantee.
• nodeKind and permission no longer fall through to Folder/Write on an unrecognised literal; they reject it. That was the same silent-coercion class, one layer down.
• A default arm binds the descriptor to never and throws. Verified by temporarily adding a seventeenth kind to CommandDescriptor: tsc fails with Type '{ kind: "probeNewKind"; … }' is not assignable to type 'never' at that binding, so a new kind cannot be added without a builder.

Valid commands are unaffected — every check passes through the value it validated.

Tests

packages/client/src/worker/commandCodec.test.ts gains five refusal cases driven against a permissive fake whose every builder succeeds, so only the codec's own checks can reject: an unknown kind, a numeric newName and a null name, a string where a public key belongs and an array where a node id belongs, an unrecognised nodeKind and permission, and a number opId. A sixth asserts the accept side of both mirror enums — 'folder' and 'write', not only the literal each mapper tests first.

Not in this PR

The byte and string arguments of beginWrite, snapshot, download and openContentStream reach WASM from the same untrusted message with the same coercion, and passArray8ToWasm0 turns a 16-character string into sixteen zero bytes rather than rejecting it. That is #1154, which reuses the checkers this PR introduces.

Gates

pnpm typecheck 0, pnpm test 0, pnpm lint 0, pnpm lint:tracker-refs 0.

---

Closes #1037
Closes #1051

Note

Fail closed on invalid worker commands and transferred upload chunks

• buildCommand in commandCodec.ts now strictly validates all descriptor fields, throwing descriptive errors for wrong-typed strings, non-Uint8Array byte fields, non-bigint opId, unknown node kinds, unknown permissions, and unknown command kinds instead of silently falling through or relying on wasm-bindgen coercion.
• EngineHost.pushChunk in engineHost.ts now zeros the provided ArrayBuffer after the WASM call completes, using a new scrubbing helper that guarantees zeroing even when the call rejects.
• Behavioral Change: callers passing wrong-typed fields to buildCommand or unknown command kinds will now receive thrown errors rather than silent misbehavior.

^{Macroscope summarized 45f85cc.}

Summary by CodeRabbit

• Security & Reliability
  • Strengthened command validation to reject unknown commands, invalid fields, unsupported values, and malformed operation IDs.
  • Upload data is now securely cleared from memory after processing, including when operations fail.
  • Updated upload behavior documentation to clarify that submitted plaintext buffers are scrubbed.

[coderabbitai]

Walkthrough

The worker command codec now validates descriptor fields and enum values before WASM binding. EngineHost now scrubs secret and upload buffers after successful or rejected engine operations. Tests cover validation, enum mapping, buffer copying, zeroization, and error propagation.

Changes

Worker boundary hardening

Layer / File(s)	Summary
Command descriptor validation packages/client/src/worker/commandCodec.ts, packages/client/src/worker/commandCodec.test.ts	buildCommand validates string, byte-array, bigint, node ID, node kind, and permission fields. Unknown command kinds and unsupported enum values now throw explicit errors. Tests cover secondary enum mappings and invalid inputs.
Engine buffer scrubbing packages/client/src/worker/engineHost.ts, packages/client/src/worker/engineHost.test.ts	start and pushChunk use asynchronous scrubbing that clears transferred buffers on success or rejection. Tests verify copied contents, zeroization, and preserved errors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Issue 1154: Both changes add strict runtime validation at the worker boundary, although Issue 1154 focuses on reusing these checks in EngineHost request handling.

Possibly related PRs

• FSM1/cipher-box#663: Introduces the WASM command builders consumed by the validated codec.
• FSM1/cipher-box#728: Directly overlaps in commandCodec.ts and engineHost.ts.
• FSM1/cipher-box#950: Adds cancelUpload handling that this change now validates.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issues [#1037] and [#1051] with failure-path scrubbing, strict command validation, exhaustive handling, and test coverage.
Out of Scope Changes check	✅ Passed	All production and test changes directly support the linked issue objectives and contain no unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes both primary fail-closed changes in the pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1037-zeroize-worker-upload-chunk

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/client/src/worker/engineHost.ts (1)

29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace behavior narration with rationale.

These comments state behavior that the API and helper names already show. State the plaintext-lifetime reason at the public boundary. Remove the redundant test-helper comment.

• packages/client/src/worker/engineHost.ts#L29-L29: State that transfer makes the worker the terminal owner, so it scrubs plaintext to reduce its lifetime.
• packages/client/src/worker/engineHost.test.ts#L45-L45: Remove the comment unless non-obvious test rationale is needed.

Proposed documentation cleanup

-  /** Takes ownership of `chunk`: the host scrubs the plaintext once it lands. */
+  /** Transfer makes the worker the terminal owner, so it limits plaintext lifetime. */

-/** A host whose WASM `pushChunk` hands the view it was given to `onPush`. */
 function pushingHost(onPush: (chunk: Uint8Array) => Promise<void>): EngineHost {

🤖 Prompt for 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.

In `@packages/client/src/worker/engineHost.ts` at line 29, Update the
public-boundary comment in packages/client/src/worker/engineHost.ts at line 29
to explain that transfer makes the worker the terminal owner, so it scrubs
plaintext to reduce its lifetime. Remove the redundant comment in
packages/client/src/worker/engineHost.test.ts at line 45 unless it provides
non-obvious test rationale.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@packages/client/src/worker/engineHost.ts`:
- Line 29: Update the public-boundary comment in
packages/client/src/worker/engineHost.ts at line 29 to explain that transfer
makes the worker the terminal owner, so it scrubs plaintext to reduce its
lifetime. Remove the redundant comment in
packages/client/src/worker/engineHost.test.ts at line 45 unless it provides
non-obvious test rationale.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 87de0388-92c2-49dd-ac77-0927a77ac73c

📥 Commits

Reviewing files that changed from the base of the PR and between 8c69c17 and ca37edd.

📒 Files selected for processing (4)

• packages/client/src/worker/commandCodec.test.ts
• packages/client/src/worker/commandCodec.ts
• packages/client/src/worker/engineHost.test.ts
• packages/client/src/worker/engineHost.ts

[FSM1]

Nitpick disposition

Posted late — this PR merged without one. Verified against origin/main.

The review's single nitpick had two parts.

• packages/client/src/worker/engineHost.ts:29 — replace the behaviour narration with the plaintext-lifetime rationale. Taken, in 45f85cc44, the only commit after the review. It now reads "the host is its terminal owner, so it scrubs the plaintext to bound the lifetime of a copy no caller can reach".
• packages/client/src/worker/engineHost.test.ts:45 — delete the fake's docstring. Declined. That comment carries the one non-obvious property of the fake: it forwards the exact view it was handed rather than a copy. The transfer and scrub assertions rest on that, and the name pushingHost does not convey it. Cosmetic either way.

Nothing outstanding.