FormData: preserve filename when parsing a zero-byte multipart file part - #36435
FormData: preserve filename when parsing a zero-byte multipart file part#36435robobun wants to merge 4 commits into
Conversation
Parsing a multipart/form-data body that contains a file part with a filename but an empty body produced a File whose .name was undefined. The multipart parser builds a Blob from the part body and hands it to DOMFormData along with the filename. When JS later reads the entry, toJS() calls Blob__setAsFile() to attach the filename to the Rust-side Blob. Blob__setAsFile wrote the name onto the Blob's byte store, but a zero-byte Blob has no store, so the name was silently dropped. When there is no store, create an empty Bytes store to carry the name, matching what the File constructor already does for new File([], name).
|
Status: diff is green; ready for review. Repro (fails on main, passes here): bun bd test test/js/web/html/FormData.test.ts -t "zero-byte file part"CI build #85678: no failures in |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesMultipart file part handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
On the linked issues:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that mirrors the existing jsdom_file_construct empty-store pattern.
What was reviewed:
- Confirmed the new branch in
Blob__setAsFileis byte-for-byte the same store-creation pattern asjsdom_file_construct(Blob.rs:5627-5636), so ownership/refcount semantics are already proven. - Checked both C++ call sites of
Blob__setAsFilein blob.cpp — thepath_stris copied viato_owned_slice(), no borrow escapes. - Verified the
!path_str.is_empty()guard intentionally leavesfilename=""alone (per the #16096 note). - Structured-clone deserialize path was checked for the same bug class and ruled out.
Extended reasoning...
Overview
This PR fixes a bug where parsing a zero-byte multipart file part loses the filename. The change adds a 10-line else if branch to Blob__setAsFile in src/runtime/webcore/Blob.rs: when the blob has no store (the zero-byte case) and a non-empty filename is supplied, it creates an empty Bytes store to carry stored_name. Four new test cases in test/js/web/html/FormData.test.ts cover Request/Response parsing, round-tripping through Response.formData(), and FormData.append(name, emptyBlob, filename).
Security risks
None. This is purely about attaching a filename string to an already-constructed empty Blob. No untrusted-size arithmetic, no new parsing, no allocation sized from external input.
Level of scrutiny
Low-to-medium. The Rust change is a verbatim copy of the pattern already used in jsdom_file_construct (lines 5627-5636) for new File([], name) — same StoreRef::from(Store::new(...)), same init_empty_with_name, same ThreadSafeRefCount::init(), same MimeType::NONE. Since that pattern is already exercised on every new File([], "name") call, the memory-ownership semantics are proven. The branch is only reachable when this.store() is None, so store.set(Some(...)) cannot leak a prior store. path_str.to_owned_slice().into_boxed_slice() copies the C++-owned BunString into a Rust-owned Box<[u8]> freed by Bytes::Drop, matching the existing branch's ownership comment.
Other factors
Tests are well-constructed: they include a 1-byte control case to prove the assertions isolate the zero-byte path, they cover both Request and Response, and the PR description confirms all three fail on main with Received: undefined. The comment-cop feedback about the long comment was addressed (trimmed to one line). The author correctly declined to add Fixes #16096 since that concerns filename="", which this PR intentionally leaves unchanged via the !path_str.is_empty() guard. A finder flagged structured-clone deserialize as a possible sibling site with the same bug class; verifiers ruled it out.
|
Heads up: #37680 stops |
2 participants
Problem
Parsing a
multipart/form-databody where a file part has a non-emptyfilenamebut an empty body produces aFilewhose.nameisundefined. Bun cannot round-trip its own serialization of an emptyFile:Node returns the filename in all of these cases.
Cause
The multipart parser builds a
Blobfrom the part body and passes it toDOMFormDataalong with the filename. When JS reads the entry,toJS()callsBlob__setAsFile()to attach the filename to the Rust-sideBlob.Blob__setAsFilewrote the name onto the blob's byte store viabytes.stored_name, but a zero-byteBlobhas no store (Blob::create_with_bytes_and_allocatoronly allocates one whenlen > 0), so the name was dropped.Fix
When
Blob__setAsFileis given a non-empty name and the blob has no store, create an emptyBytesstore to carry the name. This is the same thing theFileconstructor (jsdom_file_construct) already does fornew File([], name).Verification
All three fail on
mainwithReceived: undefinedand pass with this change. FullFormData.test.ts(152 tests) passes.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file