Repository navigation
Conversation
FormData.append(name, blob, filename) stored the filename on the C++ wrapper, and the multipart serializer read it from there. get(), getAll() and the iterators returned a dupe of the original blob, and Blob__setAsFile kept the dupe's own name when it had one. So the filename override reached the wire but not JS. The entry now owns the name. Blob__setAsFile always sets it on the dupe that toJS returns. A Blob with no name becomes a File named "blob", as the create-an-entry algorithm says, so its part serializes with filename="blob" instead of filename="". An empty filename argument gives the File an empty name.
A Blob that is not a FormData entry (a WebSocket message) has no filename, so it keeps having no name. DOMFormData stores a non-null filename for every entry, so the two cases can be told apart. The default entry name "blob" is a static string: no allocation per append.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughFormData Blob entries now receive the filename ChangesFormData Blob Filename Handling
Blob URL Loader Selection
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
The FormData and Blob filename changes support #14725. However, Blob::get_loader() now changes import loader selection for all Blobs with an unrecognized or missing filename extension. The added `te… |
Remove the unrelated Blob::get_loader() behavior change and its dedicated import-loader tests, or link and define an issue requirement that requires this behavior. |
-
Check name Status Explanation Linked Issues check Issue #14725requires an unnamed Blob in multipart FormData to serialize withfilename="blob".entryFileName()supplies that default forappend()andset(). The entry name is applied to a dupl…Title check The title clearly summarizes the main changes: defaulting unnamed Blob entries to "blob" and applying the filename argument to the returned File entry. Description check The description explains the problem, implementation, scope, tradeoffs, and verification results. It does not use the exact template headings, but it provides equivalent content for both required sect…
Full details: Out of Scope Changes check
-
Explanation
The FormData and Blob filename changes support
#14725. However,Blob::get_loader()now changes import loader selection for all Blobs with an unrecognized or missing filename extension. The addedtest/js/web/fetch/blob.test.tscases cover this separate import behavior. The linked issue and current PR description do not require this behavior.
- Fix all pre-merge checks with AI
- 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.
|
Updated 1:05 PM PT - Oct 15th, 2026
❌ @robobun, your commit 6e2f30e has some failures in 🧪 To try this PR locally: bunx bun-pr 45373That installs a local version of the PR into your bun-45373 --bun |
|
Status: ready for review. How I reproduced it. On main 5f5799e (debug and release builds) and on Bun 1.4.2: const fd = new FormData();
fd.set("fname", new Blob(["hello, world"], { type: "text/plain" }));
console.log((await new Response(fd).text()).match(/Content-Disposition:[^\r\n]*/)[0]);
// main: Content-Disposition: form-data; name="fname"; filename=""
// this PR: Content-Disposition: form-data; name="fname"; filename="blob" (Node v26.3.0 prints the same)
const form = new FormData();
form.append("f", new File(["x"], "orig.txt"), "over.txt");
console.log(form.get("f").name);
// main: "orig.txt" this PR: "over.txt"
The second PR on top of this one is the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the BunString ownership in the new paths: Blob__getFileNameString returns a +1 clone that transferToWTFString() consumes, and the Dead branch holds no reference; in toJS, Bun::toString borrows the entry name and Blob__setAsFile clones it into the dupe, which Blob__create then owns. I also looked at Bun.file()/S3 blobs appended with an explicit filename argument — the override naming the dupe is the intended entry semantics and does not touch the caller's blob.
Extended reasoning...
The change touches FormData entry naming across the C++ DOMFormData/JSDOMFormData bindings, blob.cpp toJS, and the Rust Blob__setAsFile/Blob__getFileNameString FFI helpers, with no security-sensitive surface. Inline findings already flag the set/append surrogate inconsistency, the un-updated toJSNewlyCreated caller, the changed multipart-parse entry point, and the Bun.file path-as-filename behavior, so a human should weigh those; the refcount and ownership paths across the FFI boundary were checked and are balanced.
append() replaced each lone surrogate in the filename. set() stored the filename as it came. get() returns the entry filename, so the two gave different names for the same File. Both now use one helper. toJSNewlyCreated maps a null filename to no name, the same as toJS. Tests: a lone surrogate through append() and set(), a parsed part with filename="", and the Content-Disposition filename that Bun.serve sends for an entry File.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the reordered toJS in src/jsc/bindings/blob.cpp (dupe first, then Blob__setAsFile on the dupe): Blob__dupe's +1 is still consumed by Blob__create, and the entry's own impl is no longer mutated, so the caller's File keeps its name as the description claims. The points raised in the prior review (set-path sanitization, toJSNewlyCreated null-to-Dead mapping, parse-side filename="" and Bun.serve content-disposition coverage) are addressed in 1a2245b.
Extended reasoning...
The latest push adds sanitizedFileName() and fileNameOrDead() helpers shared by both append/set and both toJS overloads, and extends the test suite with the parse-side, Bun.serve and WebSocket variants. The remaining inline finding is a behavioral regression for createObjectURL imports of FormData entries, which is not a nit, so approval is not appropriate.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
…wn extension get_loader used the TSX loader for every Blob that has a name with no known extension, and ignored the type. A FormData entry made from an unnamed Blob is now a File named "blob", so an import of that entry through URL.createObjectURL parsed JSON as TSX. The extension decides when the name has one that maps to a loader. If not, the type decides. Also shorten the comments of the FormData entry helpers to one line.
They cover an empty File and an empty Blob with a filename argument, one File appended under two names, and a File over Bun.file() that is appended with no filename argument.
There was a problem hiding this comment.
I re-reviewed the latest push (commits 1a2245b..6e2f30e) and found no bugs; the new commits address the points raised in the earlier inline threads. Because this changes user-visible Web API behavior (an unnamed Blob entry now serializes and reads back as "blob") and widens get_loader fallback beyond FormData, a human maintainer should still sign off on the behavior change.
What was reviewed:
- Ownership in
toJS(Blob&):Blob__dupereturns a fresh +1 allocation thatBlob__createconsumes, same transfer as before;Blob__setAsFileclones the borrowedBunStringview, so no leak or double-free. - All callers of
Blob__getFileNameString/Blob__setAsFile(bothtoJSoverloads,entryFileName) now share the Dead-means-no-name convention; theset()path and the multipart parser'sfilename=""case are now sanitized and tested. get_loaderrestructure:Path::loaderreturnsNonefor an unknown extension, so a name like "blob" or "data" falls through to the MIME type rather than forcing Tsx; this also changesblob:imports of any extension-less File, which the newblob.test.tscase covers.
Extended reasoning...
The diff touches FormData entry naming across three C++ binding files and Blob.rs, plus three test files; it changes the default entry filename to "blob", makes an explicit filename argument win over a File's own name, dupes the entry before naming so the caller's File is not mutated, and restructures loader selection for named Blobs. No security-sensitive surface (no auth, crypto, or path handling beyond the pre-existing Bun.file name behavior). The refcount transfer, the Dead/Empty/null distinction across every caller, and the get_loader fallback were verified from source and are covered by new tests. Defer rather than approve because the change deliberately alters user-visible wire output and blob: import loader selection, which is a product decision a maintainer should confirm.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
|
On the out-of-scope check for An entry made from an unnamed Blob is now a File named |
Fixes #14725
Part of #43691 and #35439.
Problem
fd.append("f", new Blob(["x"]))serializes asfilename="". The spec, Node and browsers sendfilename="blob". Strict servers reject the part.fd.append("f", file, "renamed.txt")keeps the File's own name on the value thatfd.get()returns.JSDOMFormData.cpp:311defaults the entry filename to the Blob's name or an empty string.Blob__setAsFile(src/runtime/webcore/Blob.rs:3997) applies it only to a Blob with no name.Fix
entryFileName()follows create an entry: the filename argument, else the Blob's own name, else"blob".toJSnames a dupe of the entry's Blob, so the caller's File keeps its name.get_loader(Blob.rs:1957) uses the type when the name has no known extension, so an entry named"blob"still imports by its type.test/js/web/html/FormData.test.ts,FormData-multipart-serialization.test.tsandtest/js/web/fetch/blob.test.ts. 13 tests fail on main.Background
DOMFormData(C++) holds each file entry as a dupe of the user's Blob plus the entry filename.get()returns the entry throughtoJS(blob.cpp).get_loaderpicks the loader forimport(URL.createObjectURL(blob)). It used TSX for each name with no known extension.Downsides
filename="blob", andfd.get()returns a File named"blob". A server that read the part as a text field now sees a file part.Bun.servesends that name incontent-disposition.fd.set(name, blob): 664 to 701 instructions.fd.append(name, blob): 544 to 545. Allocations +0,.text+0 bytes,.data+64 bytes.Notes
Before and after (Node v26.3.0 gives the "after" column, except for the two
importrows, which Node does not have)fd.append("f", new Blob(["x"])), multipart partfilename=""filename="blob"fd.get("f").nameundefined"blob"fd.append("f", new File(["x"], "orig.txt"), "over.txt"),fd.get("f").name"orig.txt""over.txt"fd.append("f", new Blob(["x"]), ""),fd.get("f").nameundefined""fd.append("f", file),fd.get("f").namefd.set("f", new File(["x"], "a\uD800.txt")),fd.get("f").name"a\uD800.txt""a\uFFFD.txt"filename="",.nameundefined""binaryType = "blob"message,.nameundefinedundefinedfilename=""filename="blob"Bun.servewithnew Response(fd.get("f")), entry from a Blob of typeapplication/octet-streamwith no nameContent-Dispositionfilename="blob"import(URL.createObjectURL(fd.get("f"))), entry from a JSON Blob with no name{ a: 1 }{ a: 1 }import(URL.createObjectURL(new File([json], "data", { type: "application/json" })))BuildMessage(parsed as TSX){ a: 1 }The
Content-Dispositionrow.Bun.servesends the name of a File body as the filename. A File thatreq.formData()parses from a part withfilename="blob"(what a browser and Node send for an unnamed Blob) gets that header on main too. A test pins the header for an entry File.The
get_loaderchange.get_loader(Blob.rs:1957) is used only for an import of ablob:URL. The extension decides when the name has one that maps to a loader. If not, the type decides, as it does for a Blob with no name. Without this change, the import in the second to last row fails with this PR, because the entry has the name"blob".Issue scripts
filename="blob".new Blob([file])returns a File, andFormData.set(name, value, filename)then ignores the filename #43691: rows 3 and 4 of the script printname="renamed.txt". Row 1 (new Blob([file]) instanceof File) is not part of this PR.file.slice()) and item 3 (thetyperewrite) are not part of this PR.Measurements. Release builds of the merge base 5f5799e and of this PR (6e2f30e). The instruction counts are a gdb
stepicount of one native call after 30 warm-up calls. The counts repeat exactly.fd.append(name, blob), Blob with no namefd.append(name, file)fd.set(name, blob), Blob with no namefd.set(name, file)fd.get(name)set()now replaces a lone surrogate in the entry filename, asappend()does. That is the call that adds the instructions tofd.set."blob". A named File entry pays the same today. The default name is a static string, so there is no allocation.slice(),text()andstream()of a Blob: 399, 543 and 408 instructions on both builds.size:.text106,006,149 -> 106,006,149 bytes,.data115,160 -> 115,224 bytes.Not changed here
fd.get()Files reportlastModified0 where Node gives the current time (FormData: give the Files it creates a lastModified of now #41972).Bun.file()entry sends its full path as the filename (FormData: send only basename, not full path, as Bun.file() multipart filename #35523).instanceof File(Give File its own prototype that inherits from Blob.prototype #30328 changes that).new File(["x"], "a\uD800.txt").namekeeps the lone surrogate (File: convert the name argument to a USVString #41216 changes that). This PR makes only the entry filename a scalar value string.Content-Typeheader when that header followsContent-Disposition(FormData: honor an explicit Content-Type on multipart file parts #33013).Other open PRs in the same lines
Blob__setAsFilechange and nothing else. Its two tests are in this PR.Blob__setAsFilechange. Its other half (new Blob([file])) is not in this PR. Blob: a copy of a File is a plain Blob (file.slice(), new Blob([file]), response.blob()) #45401 covers it.DOMFormData.cppand rewritestoJSinblob.cpp. The two PRs conflict in those two files.Blob__getFileNameString. After this PR it must keep returning a Dead string for a Blob with no name.Review. This change was reviewed together with a larger Blob change (
file.slice()returns a plain Blob). The review asked to land this part alone, because it needs no design decision. The Blob change is #45401, on top of this one. The later commits answer the review comments on this PR:set()andappend()store the same filename, the tests for a parsed part and forBun.serve, and theget_loaderchange.no test proof · iteration 11 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/html/FormData-multipart-serialization.test.ts