Repository navigation
Conversation
The Blob serializer tagged an S3 store as Bytes while writing the File shaped body for it, so structuredClone(), bun:jsc/node:v8 serialize() and postMessage() of an S3File produced a record that failed to read back with "Unable to deserialize data." (postMessage lost the message and threw on the receiver instead). An S3 store is bound to the credentials and options of the client that made it, which the record has no form for, so refuse it at serialize time with a DataCloneError DOMException. The store kind to wire tag mapping is now an exhaustive match shared by the tag and the body, Store::serialize becomes File::serialize (its S3 arm was the other half of this bug and its Bytes arm was unreachable), and the deserializer rejects a File record naming an s3:// path instead of minting an S3File under the receiver's environment credentials, since no serializer writes one.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughStructured-clone serialization rejects S3-backed Blobs with a ChangesS3File structured-clone handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the primary change: throwing DataCloneError when structured-cloning an S3File. |
| Description check | |
The description provides detailed problem, fix, verification, scope, compatibility behavior, and known limitations. It does not use the template headings exactly, but it contains the required informat… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description provides detailed problem, fix, verification, scope, compatibility behavior, and known limitations. It does not use the template headings exactly, but it contains the required information for what the PR does and how it was verified.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
|
Updated 7:47 AM PT - Oct 17th, 2026
❌ @robobun, your commit b3a90c3 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 37905That installs a local version of the PR into your bun-37905 --bun |
|
Status: head 5ae84cb (plus one empty CI retrigger). Reproduced on a release build of main (1.4.3 canary, 367d939) with const options = { accessKeyId: "a", secretAccessKey: "b", bucket: "b", endpoint: "http://127.0.0.1:1" };
const f = Bun.s3.file("key.txt", options);
structuredClone(f); // TypeError: Unable to deserialize data.
f.size; // 0 after the failed clone, NaN before it
structuredClone(Bun.s3.file("key.txt", options).slice(0, 5)); // no error: a Blob of the bytes 00 00 00 00 01
With this branch each of these throws a The diff is ready for review. The new test file passes on every CI lane. The remaining CI failures in build 127404 are on tests this PR does not touch (an elysia stream cancel test on one ASAN lane, and retry-pass flakes elsewhere). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It makes a user-facing API decision (S3File is not structured-clonable, throwing DataCloneError) and adds a security-relevant deserialize guard, so a maintainer sign-off on the design would be worthwhile.
What was reviewed:
structured_clone_store_tag()is exhaustive overDataTag; theas_file()unwrap in the non-Bytes branch is reachable only when the tag isFile.check_s3: falsein the deserializer is safe because the news3://prefix guard runs first, matching the same case-sensitive checkfind_or_create_file_from_pathuses.- The deleted
Store::serializeBytes arm was confirmed unreachable (the Blob serializer inlines the sliced window itself); the S3 arm was the bug. throw_dom_exceptionfrom the void hook relies onCloneSerializer's post-hookRETURN_IF_EXCEPTION, which the PR description verified underBUN_JSC_validateExceptionChecks=1.
Extended reasoning...
Overview
The PR fixes structured cloning of S3File, which previously wrote a malformed wire record (S3 store tagged as Bytes with a File-shaped body) that every reader rejected with "Unable to deserialize data." The fix refuses to serialize S3 stores at all, throwing a spec-compliant DataCloneError at the sender. It touches src/runtime/webcore/Blob.rs (serialize hook, deserialize File arm, new structured_clone_store_tag helper), src/runtime/webcore/blob/Store.rs (Store::serialize collapsed to File::serialize, dead S3/Bytes arms removed), one doc sentence, and a substantial new test block.
Security risks
Two security-adjacent changes: (1) the deserializer now rejects File records whose path starts with s3://, closing a vector where a hand-crafted bun:jsc/node:v8 buffer could produce a live S3File signed with the receiving process's environment credentials; (2) the design explicitly avoids putting S3Credentials on the wire, which would otherwise embed secrets in every serialize() output. Both are hardenings, not new attack surface. The s3:// check is case-sensitive but so is the only consumer (find_or_create_file_from_path), and check_s3 is now false there anyway, so a mixed-case scheme just becomes a local file path.
Level of scrutiny
Medium-high. The wire-format code is subtle (tag/body must agree, unreachable arms were silently wrong before), and the change is user-visible: code that previously got a confusing deserialize-time TypeError now gets a serialize-time DataCloneError with a specific message. The refactor of Store::serialize → File::serialize deletes code, which I traced: the Bytes arm was genuinely unreachable because _on_structured_clone_serialize writes bytes inline for memory-backed stores and never called store.serialize for them.
Other factors
Test coverage is comprehensive — every S3File constructor, every clone entry point, nested graphs, the for-storage/for-transfer flags, a guard matrix that other Blob kinds still clone, and the crafted-record rejection. The PR description states 14/15 new tests fail on the unfixed build and the whole file plus adjacent structured-clone suites pass with it. The bug hunting system found nothing. I'm deferring rather than approving because the "refuse to clone" decision is a deliberate API stance a maintainer should confirm (the alternatives — carrying credentials on the wire or rebinding to the receiver's client — are argued against convincingly in the description, but it's still their call), and the deserialize guard touches untrusted-input handling near credentials.
|
For the maintainer who makes the design call: this PR makes "an S3File cannot be structured-cloned" a documented contract. There are two open questions.
|
|
This came up again in a structured-clone sweep, with one extra symptom worth folding into the description: the failed first attempt also mutates the source. I had an equivalent branch before finding this PR: main...robobun/22c566ef/s3file-structured-clone. The one difference is that it makes the generated |
…oneerror Conflicts in src/runtime/webcore/Blob.rs and src/runtime/webcore/blob/Store.rs. Main changed the string view and path types around the Blob structured clone code (Utf8Bytes, PathLike::owned, the JsThread receiver of FileExt::unlink). The resolution keeps those and carries the store tag parameter, File::serialize, and the s3:// record guard over unchanged.
The tests now clone a 5-byte S3 slice (it cloned into 5 wrong bytes with no error), new Blob([s3file]), new File([s3file], name) and formData.get() of an appended S3File. They also assert that a refused clone leaves the size of the source unchanged. The docs say that structured cloning is not supported, and no longer call it the only exception. The comments this change adds in Blob.rs and Store.rs are one line each.
The block comment states the invariant in one line. The two slice rows are named by their windows.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 1 finding from earlier reviews is still open above.
Still open from earlier reviews (1):
- 🔴
src/runtime/webcore/Blob.rs:3906—If any persisted serialized buffer (v8.serialize, bun:jsc serialize, forStorage records) was written by a Bun that pred…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
The deserializer passes check_s3 = false, so a File record that names an s3:// path builds a local File store, which is what a record written before S3 support meant. No hard error for that record.
There was a problem hiding this comment.
I reviewed the latest push (5ae84cb) and found no new bugs; because this sets a new user-facing contract (S3File, and any Blob/File/FormData entry sharing its store, now throws DataCloneError from structuredClone/postMessage/serialize where the base emitted a broken record), a maintainer should still confirm that is the intended behavior before merging.
What was reviewed:
- The DataCloneError throw in
on_structured_clone_serialize:CloneSerializerruns RETURN_IF_EXCEPTION right after the hook and the outer loop maps it to ExistingExceptionError, so the throw propagates and no record bytes follow the tag. - The
store.data.as_file()accessor: provably a File store at that point (Bytes is taken by the if-let, S3 returns None before the writer is built, andresolve_size()never swaps the store variant). - The deserializer with
check_s3 = false: the latest commit drops the hards3://rejection, so such a File record now builds a local File store instead of an env-credential S3File, which addresses the deserialization concern raised in the earlier review. - New tests assert the exact DOMException shape across every S3File constructor, slices, the three wrappers, each entry point, forStorage, the
{}cross-process result, and the crafteds3://record; no S3 I/O occurs.
Extended reasoning...
The change touches src/runtime/webcore/Blob.rs (structured-clone serialize/deserialize hooks), src/runtime/webcore/blob/Store.rs (moves the File record writer from StoreExt to FileExt and deletes the S3 and dead Bytes arms), a docs paragraph, and a new 19-test describe block. The security-relevant surface is the deserializer: with check_s3 = false a File record naming an s3:// path no longer yields an S3File signed with the receiving process's environment credentials. No CODEOWNERS entry covers the changed files and the exit reason was dry_streak. Defer rather than approve because the PR introduces a new user-visible contract (postMessage(s3file) now throws at the sender) that the author explicitly asks a maintainer to confirm, and the error message still names S3File for plain Blob wrappers, a wording choice the author kept after self-resolving that thread.
Problem
structuredClone(s3file)throwsTypeError: Unable to deserialize data.postMessage(s3file)returns, and the receiver getsmessageerror. A 5-byte slice clones into 5 wrong bytes with no error.sizeof the source to0. Thenfile.text()reads 1 byte of 10 andclient.write(key, file)uploads 1 byte. No user report exists._on_structured_clone_serialize(src/runtime/webcore/Blob.rs:726) tags an S3 store asBytes, writes aFilebody, and callsresolve_size(), which sets the size to 0.Fix
structured_clone_store_tag()returnsNonefor an S3 store. The serialize hook then throws aDataCloneErrorbefore it writes a byte or callsresolve_size(). ABlobbuilt from an S3File throws too.Store::serializebecomesFile::serialize, without the S3 andBytesarms. The deserializer passescheck_s3 = false: aFilerecord with ans3://path builds a local File store, never an S3File with the environment credentials.test/js/web/structured-clone-blob-file.test.ts(18 of 19 new tests fail without the fix). Notes list the other suites.Background
Blobstore isBytes,FileorS3. The clone record has noS3tag and no field for credentials.Downsides
postMessage(s3file)now throws at the sender.expect(s3file).toHaveLength()andtoBeEmpty()still setsizeto 0 throughresolve_size(). s3: leave the size of an S3 blob unknown in resolve_size #43934 changes that function.match. Deserialize drops one prefix compare.Notes
Questions for a maintainer.
Filerecord that comes from raw bytes. Only reconstruct file-backed Blobs from in-process structured clone #31329 covers that class.Measured on a release build of main (1.4.3 canary, 367d939) and on a debug build of this branch. The S3 endpoint is a
Bun.serveon loopback that serves a 10-byte object.file = client.file("x.txt")sizeisNaN,text()reads 10 bytes,client.write("copy.txt", file)uploads 10 bytesstructuredClone(file)TypeErrorsizeis0,text()reads 1 byte (Range: bytes=0-0),write()uploads 1 byteDataCloneError, then the same as the controlport.postMessage(file)messageerrorDataCloneError, then the same as the controlOther results on main, with no I/O:
structuredClone(file.slice(0, 5))returns aBlobof the bytes00 00 00 00 01, andsizeof the slice goes from 5 to 0. The reader takes the low 32 bits of the u64 size as the byte length. The other windows that were tried throw theTypeError, exceptslice(2, 7), which returns 3 wrong bytes.serialize(file)returns 49 bytes thatdeserialize()rejects.new Blob([file]),new File([file], "n.txt")andformData.get()of an appended file are notS3Fileobjects. They share the S3 store, throw theTypeError, and getsize0.Filerecord with the paths3://bucket/key, written by hand, deserializes to[object S3File]signed with the environment credentials.On this branch each of these throws, and
sizedoes not change. The hand-built record deserializes to a plain[object Blob]over a local File store.Why the clone is refused and not made to work. An S3 store is a key plus refcounted
S3Credentials(endpoint, bucket, region, keys) and the upload options (ACL, storage class, requester pays, multipart settings). The old record carried only the key as the user typed it. A receiver that opens that key again uses the credentials of its own environment, and a bare key (client.file("key")) would open a localBun.file("key"). ADataCloneErrorat serialize time is what the structured clone algorithm specifies for a platform object that cannot be serialized. Bun already throws it for aResponseand, since #32799, for a detachedArrayBuffer.A third design keeps the same behavior and makes the generated serialize hook return
JsResult<()>(branchrobobun/22c566ef/s3file-structured-clone). It changessrc/codegen/generate-classes.tsandBlockList.rstoo. This PR throws from the existing hook, as 52 otherlet _ = global.throw_*()sites insrc/runtimeandsrc/jscdo.CloneSerializerchecks for a pending exception after each hook.The two arms of
Store::serializethat are gone. The S3 arm wrote a path record under theBytestag, which is the wrong record of this bug. TheBytesarm had no caller: the Blob serializer writes the bytes of its own window inline.Self-review, the 5 concerns with no change here. Each one is about a route that this PR does not touch.
resolve_size()still sets an unknown S3 size to 0 for its other callers (s3: leave the size of an S3 blob unknown in resolve_size #43934).Related PRs.
resolve_size()and removes thesizechange for every caller. Thesizeassertions here pass with or without it. Without this PR the rows still fail, because they expect theDataCloneError.Not changed on purpose.
process.send) turns each Blob into{}before it reaches the Blob serializer. An S3File still arrives as{}, the same asBun.file(). A test asserts this.Blob::resolve_size()(see Downsides).new Blob(["x", s3file])with more than one part. It is aBytesstore that holds onlyx, and it clones as before.Receiver side. The deserializer passes
check_s3 = falsetofind_or_create_file_from_path. AFilerecord with ans3://path builds a local File store with that path. That is what a Bun from before S3 support wrote forBun.file("s3://..."), so such a record still deserializes. An earlier revision rejected the record with a hard error. A review pointed out that this broke old persisted records, so the reject is gone.Tests. The block "S3File structured clone" has 19 tests. It covers five ways to make an S3File, two slices (one of 5 bytes), three plain wrappers over an S3 store (
new Blob([s3file]),new File([s3file], name),formData.get()), each entry point (structuredClone,serializeofbun:jscandnode:v8,postMessageofMessagePortandWorker, the for-storage flag), an S3File nested in a larger graph, the{}result for cross-process transfer, a matrix of the other stores that still clone, and the hand-builts3://record (a local File store, never an S3File). The rows assert thatsizeof the source does not change. No test does S3 I/O.Suites run with the debug build of this branch (merged with main at 7c96a12).
test/js/web/structured-clone-blob-file.test.ts: 61 pass. 18 fail on the release build of main.test/js/web/workers/structured-clone.test.ts: 244 pass (with--timeout 180000, the 2 GiB ArrayBuffer tests need more than 5 s on a debug build).test/js/web/workers/structuredClone-classes.test.ts: 57 pass.test/js/web/structured-clone-fastpath.test.ts: 93 pass.test/js/bun/jsc/bun-jsc.test.ts: 41 pass.test/internal/source-lints/: 103 pass.BUN_JSC_validateExceptionChecks=1: no report.cargo clippy -p bun_runtime --no-deps: clean.bun run rust:check-all x86_64-pc-windows-msvc: ok.tsc --noEmitintest/: no error in the test file.