Validate transfer lists before serializing instead of silently dropping invalid entries#32809
Conversation
…er semantics structuredClone, the worker-global postMessage, and the Worker transferList option silently skipped transfer entries that are not objects, so a call that node and browsers reject with a TypeError instead succeeded and detached the caller's buffers. Convert the transfer list per WebIDL (sequence<object> / StructuredSerializeOptions) before any serialization instead. MessagePort.postMessage now checks the transfer list before serializing: transferring the source port throws with no side effects, and transferring the entangled peer is allowed but dooms the channel per the HTML spec. Serializing a MessagePort that is not in the transfer list now fails with a DataCloneError instead of a TypeError.
|
Updated 1:11 AM PT - Jun 27th, 2026
❌ @robobun, your commit 2307c10 has some failures in 🧪 To try this PR locally: bunx bun-pr 32809That installs a local version of the PR into your bun-32809 --bun |
|
Status: reproduced against bun 1.4.0 and cross-checked against node v26 and the HTML spec before fixing. Scope was narrowed after the duplicate check: the MessagePort postMessage semantics and the untransferred-port DataCloneError are already implemented in #31216, so this PR only keeps the transfer-list validation (structuredClone, worker-global postMessage, Worker transferList option). Verification:
CI state (build 65329, finished): every lane that ran tests is green, including all Windows and Linux test lanes. The only two failed jobs are macOS |
|
Warning Review limit reached
More reviews will be available in 1 minute and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughWorker construction, structuredClone, and postMessage now convert transfer-related inputs through shared IDL helpers. Tests add coverage for invalid entries, malformed options, iterable transfers, and buffer detachment behavior. ChangesTransfer option conversion
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 547-560: Update the worker_threads test to await worker cleanup
instead of calling terminate() without waiting, so the fallback cleanup in the
failure path fully completes before the test ends. Keep the existing try/finally
around the Worker creation in test("an invalid transferList entry throws before
anything is detached"), but make the cleanup path async-aware by awaiting the
Worker termination result. Also remove the inline comment in that test that
restates the regression context, since the test name and assertions already
describe the intent.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 0fc3180e-4892-48f6-b5e7-90438efd787c
📒 Files selected for processing (9)
src/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/MessagePort.cppsrc/jsc/bindings/webcore/SerializedScriptValue.cppsrc/jsc/bindings/webcore/StructuredClone.cppsrc/jsc/bindings/webcore/Worker.cpptest/js/node/worker_threads/worker_threads.test.tstest/js/web/workers/message-channel.test.tstest/js/web/workers/structured-clone.test.tstest/js/web/workers/worker-postmessage-transfer.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes user-facing behavior across several spec-sensitive transfer/serialization paths (including flipping the entangled-peer-transfer case from throw to doomed-channel and rewriting existing test assertions), so it's worth a human look.
Extended reasoning...
Overview
This PR replaces hand-rolled transfer-list parsing in structuredClone, the worker-global postMessage, and the Worker constructor's transferList option with the WebIDL convertDictionary<StructuredSerializeOptions> / convert<IDLSequence<IDLObject>> machinery, so invalid entries throw TypeError before any buffer is detached. It also reorders the source-port / entangled-peer checks in MessagePort::postMessage to run before serialization (per the HTML spec's message-port-post-message-steps), changes the entangled-peer case from throwing DataCloneError to silently dooming the channel, and reclassifies an untransferred MessagePort from ValidationError to DataCloneError in SerializedScriptValue.cpp. Five C++ binding files and four test files are touched; two pre-existing assertions in message-channel.test.ts are inverted.
Security risks
None apparent. The change tightens validation (rejecting more inputs earlier) rather than loosening it, and does not touch auth, crypto, or permission boundaries. The doomed-channel path drops the message and disentangles the peer rather than delivering anything new.
Level of scrutiny
Moderate-to-high. These are core, spec-defined Web/Node compatibility surfaces where subtle ordering matters (e.g., the new pre-serialization loop in MessagePort::postMessage compares port->pipe() against m_pipe.ptr() to detect both this port and its peer, then defers the doomed drop until after disentanglePorts). The change also intentionally flips observable behavior — calls that used to succeed now throw, and one that used to throw now succeeds — and rewrites existing test expectations to match. That combination merits a human reviewer confirming the spec/Node alignment and that no other call sites depended on the old lenient parsing.
Other factors
Test coverage is good (11 new tests, fail-before/pass-after verified per the PR description), and the bug-hunting system found nothing. The implementation reuses existing WebIDL converters rather than adding new logic, which limits risk. Still, the breadth (five C++ files across structured clone, Worker, and MessagePort) and the deliberate behavioral inversions push this past the threshold for auto-approval.
The doomed-port semantics and the DataCloneError for an untransferred MessagePort are already implemented in #31216 together with the rest of that MessagePort work, so this PR keeps only the transfer-list validation fixes.
robobun
changed the title
|
Checked both:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/webcore/SerializedScriptValue.cpp`:
- Around line 1781-1783: The MessagePort validation path in
SerializedScriptValue.cpp is mapping an untransferred MessagePort to
ValidationError, which later turns into TypeError; update the failure handling
in the serialization check so this case reports the DataCloneError-style
serialization failure instead. Adjust the code around the MessagePort
lookup/validation in SerializedScriptValue::serialize to use the correct return
code so maybeThrowExceptionIfSerializationFailed() and
exceptionForSerializationFailure() surface DataCloneError for an unserializable
MessagePort.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: cff1cc94-da00-42a4-8674-11ee14b414ea
📒 Files selected for processing (3)
src/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/node/worker_threads/worker_threads.test.tstest/js/web/workers/structured-clone.test.ts
💤 Files with no reviewable changes (1)
- test/js/web/workers/structured-clone.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@src/jsc/bindings/webcore/SerializedScriptValue.cpp`:
- Around line 1781-1783: The MessagePort validation path in
SerializedScriptValue.cpp is mapping an untransferred MessagePort to
ValidationError, which later turns into TypeError; update the failure handling
in the serialization check so this case reports the DataCloneError-style
serialization failure instead. Adjust the code around the MessagePort
lookup/validation in SerializedScriptValue::serialize to use the correct return
code so maybeThrowExceptionIfSerializationFailed() and
exceptionForSerializationFailure() surface DataCloneError for an unserializable
MessagePort.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: cff1cc94-da00-42a4-8674-11ee14b414ea
📒 Files selected for processing (3)
src/jsc/bindings/webcore/SerializedScriptValue.cpptest/js/node/worker_threads/worker_threads.test.tstest/js/web/workers/structured-clone.test.ts
💤 Files with no reviewable changes (1)
- test/js/web/workers/structured-clone.test.ts
🛑 Comments failed to post (1)
src/jsc/bindings/webcore/SerializedScriptValue.cpp (1)
1781-1783: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return
DataCloneErrorfor untransferredMessagePort.Line 1782 still reports
ValidationError, which later becomesTypeErrorinmaybeThrowExceptionIfSerializationFailed()/exceptionForSerializationFailure(). An unserializableMessagePorthere should surfaceDataCloneError, notTypeError.Suggested fix
- code = SerializationReturnCode::ValidationError; + code = SerializationReturnCode::DataCloneError;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// MessagePort object could not be found in transferred message ports code = SerializationReturnCode::DataCloneError; return true;🤖 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 `@src/jsc/bindings/webcore/SerializedScriptValue.cpp` around lines 1781 - 1783, The MessagePort validation path in SerializedScriptValue.cpp is mapping an untransferred MessagePort to ValidationError, which later turns into TypeError; update the failure handling in the serialization check so this case reports the DataCloneError-style serialization failure instead. Adjust the code around the MessagePort lookup/validation in SerializedScriptValue::serialize to use the correct return code so maybeThrowExceptionIfSerializationFailed() and exceptionForSerializationFailure() surface DataCloneError for an unserializable MessagePort.
The previous commit accidentally picked up the detached-ArrayBuffer hunk that already landed on main as #32799. This PR does not change SerializedScriptValue.cpp.
worker_threads.test.ts has unrelated tests that hit their timeout under debug builds, so the transferList regression test gets a dedicated file next to it (plus a positive control that a valid transferList still detaches).
…red-clone-transfer-validation
There was a problem hiding this comment.
Thanks for tightening this up — the metadata and stray SerializedScriptValue.cpp hunk are gone and the diff now matches the description. I didn't find any issues, but since this swaps the argument-conversion path for structuredClone, the worker-global postMessage, and the Worker constructor (a user-visible strictness change in core serialization), I'd like a human to sign off.
Extended reasoning...
Overview
Three hand-rolled transfer-list parsers are replaced with calls to the existing generated WebIDL converters:
StructuredClone.cpp/jsFunctionStructuredClone: now goes throughconvertDictionary<StructuredSerializeOptions>(the same generated converterJSStructuredSerializeOptions.cppalready provides).Worker.cpp/jsFunctionPostMessage(the worker-globalself.postMessage): the sequence overload now usesconvert<IDLSequence<IDLObject>>and the dictionary overload usesconvertDictionary<StructuredSerializeOptions>— exactly what the generatedjsWorkerPrototypeFunction_postMessage{1,2}BodyinJSWorker.cppalready do for the parent-sideWorker#postMessage.JSWorker.cpp/JSWorkerDOMConstructor::construct:options.transferListnow goes throughconvert<IDLSequence<IDLObject>>.
Net effect is ~40 lines of C++ removed and ~15 added, plus 7 new tests across three files. Since my last comment the author dropped the unrelated SerializedScriptValue.cpp hunk (already merged in #32799), moved the worker_threads test into its own file with await worker?.terminate(), and updated the title/description.
Security risks
None identified. The change tightens input validation (rejects more inputs) and reuses converters that are already on the hot path for Worker.prototype.postMessage and MessagePort.prototype.postMessage. No new parsing of untrusted data; RETURN_IF_EXCEPTION is present after every converter call.
Level of scrutiny
Medium-high. The change itself is mechanical and follows the established pattern in the same files, but structuredClone, postMessage, and the Worker constructor are core runtime entry points, and this is a deliberate behavior change: calls that previously succeeded (with silent data loss) now throw TypeError. That's the spec-correct, Node- and browser-matching behavior, but a maintainer should confirm they're comfortable with the strictness bump (e.g. { transfer: null } and primitive options now throw).
Other factors
- CI is green except
test-docker-build-debian.tson macOS aarch64, which is unrelated to this diff. - The bug-hunting system found nothing.
- The CodeRabbit nit about awaiting
worker.terminate()was addressed. - I verified
convertDictionary<StructuredSerializeOptions>(JSStructuredSerializeOptions.cpp:32-56) handles null/undefined options, non-object options, and undefinedtransferthe way the new tests expect.
Jarred-Sumner
deleted the
farm/5864dcaf/structured-clone-transfer-validation
branch
2 participants
Problem
structuredClone, the worker-globalpostMessage, and thetransferListworker option each hand-roll their transfer-list parsing and silently skip entries that are not objects. The surviving entries are still transferred, so a call that node and every browser reject with aTypeErrorinstead succeeds and destroys the caller's buffer:transfer: [undefined],[42],["x"],transfer: 5,transfer: "abc", and a non-objectoptionsargument are all silently accepted too, and the same silent-skip loop exists inself.postMessage(msg, [buf, null])inside a worker and innew Worker(path, { workerData, transferList }).Fix
Convert the transfer list per WebIDL before anything is serialized, using the converters the generated
Worker.prototype.postMessageandMessagePort.prototype.postMessagebindings already use, so an invalid transfer list throws aTypeErrorwith no side effects:StructuredClone.cpp:structuredCloneconverts its options throughconvertDictionary<StructuredSerializeOptions>(WebIDLsequence<object>) beforeSerializedScriptValue::create. This also makes non-array iterables (Set, generators) usable intransfer, which the generated postMessage bindings already supported.Worker.cpp(jsFunctionPostMessage, the worker-globalpostMessage): same conversion for both thesequence<object>and{ transfer }overloads.JSWorker.cpp(thetransferListoption of the Worker constructor): same conversion, so an invalid entry throws beforeworkerDatais serialized and nothing is detached.Verification
7 new tests across
test/js/web/workers/structured-clone.test.ts,test/js/web/workers/worker-postmessage-transfer.test.ts, andtest/js/node/worker_threads/worker-transfer-list.test.ts. All fail on bun 1.4.0 and pass with this change; the surrounding worker and structured-clone suites pass.Related PRs
The first version of this PR also made
MessagePort.postMessagefollow the HTML "message port post message steps" (source port in the transfer list throws before serialization, transferring the entangled peer dooms the channel instead of throwing) and changed the untransferred-MessagePort serialization error toDataCloneError. Both are already implemented in #31216 together with the rest of that MessagePort work, so they were dropped here to avoid duplicating it. Thecloseevent and transferable streams have their own PRs (#32565, #32401). #29177 adds a JS-levelmarkAsUntransferableregistry innode:worker_threadsand does not validate transfer entries or touch these binding paths.