Skip to content

webcore/Blob: default FormData Blob entries to filename "blob" - #32431

Closed
robobun wants to merge 4 commits into
mainfrom
farm/a68d7af9/formdata-blob-filename
Closed

robobun wants to merge 4 commits into
mainfrom
farm/a68d7af9/formdata-blob-filename

Conversation

@robobun

@robobun robobun commented Jun 16, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #14725.
Fixes #22762.
Fixes #21788.

Repro

const formData = new FormData();
formData.set("fname", new Blob(["hello, world"], { type: "text/plain" }));
console.log(await new Response(formData).text());

Before:

Content-Disposition: form-data; name="fname"; filename=""

Node, Chrome, Firefox, Safari:

Content-Disposition: form-data; name="fname"; filename="blob"

And formData.get("fname").name was undefined instead of "blob".

Cause

XHR spec "create an entry" step 3 says that when the value is a Blob that is not a File, it becomes a File whose name is "blob". Blob__getFileNameString (called from FormData.append/set when no filename argument is given) returned an empty string for plain Blobs, so both the multipart filename attribute and the retrieved entry's .name were empty.

Additionally, Blob__setAsFile stored the FormData-assigned filename on the Blob store's stored_name, which is shared with the user's original Blob. With a non-empty default this would have leaked the name into the original Blob, and the stored_name.is_empty() guard prevented overriding the name via a later set(name, blob, filename).

Fix

  • Blob__getFileNameString now returns "blob" when the value is a plain Blob (not a File) with no existing name.
  • Blob__setAsFile now stores the filename on the per-Blob name field instead of the shared store, so it applies to the FormData entry (and the File returned from get()) without affecting the original Blob, and can be overridden by a later set/append with an explicit filename.

This also makes formData.set("k", Bun.file(path), "custom.bin") return .name === "custom.bin" from get(), which previously returned the path.

Verification

bun bd test test/js/web/html/FormData.test.ts
bun bd test test/js/web/html/FormData-multipart-serialization.test.ts

Per https://xhr.spec.whatwg.org/#create-an-entry, when FormData.append or
FormData.set receives a Blob that is not a File and no filename argument,
the entry becomes a File named "blob". Bun was leaving the name empty, so
multipart serialization emitted filename="" and the retrieved entry's
.name was undefined.

Also switch Blob__setAsFile to store the FormData-assigned filename on
the per-Blob name field instead of the shared store's stored_name, so it
can be overridden by a subsequent set()/append() with an explicit
filename and does not leak into the user's original Blob.

Fixes #14725
@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Blob__setAsFile now writes the filename to the per-Blob name field instead of the store's stored_name. Blob__getFileNameString checks the per-Blob name first, falls back to get_file_name(), and defaults to "blob" for non-jsdom Blobs when neither is set. jsdom_file_construct_ sets blob.name unconditionally from its constructor argument. FormData and multipart serialization tests are updated to expect "blob" as the default filename.

Changes

Blob filename defaulting to "blob"

Layer / File(s) Summary
Blob filename state: setAsFile, getFileNameString, jsdom_file_construct_
src/runtime/webcore/Blob.rs
Blob__setAsFile writes to the per-Blob name field instead of the store's stored_name. Blob__getFileNameString checks the per-Blob name first, falls back to get_file_name(), then returns "" for jsdom Files or "blob" for non-jsdom Blobs. jsdom_file_construct_ sets blob.name unconditionally from the constructor argument and removes the conditional store-level assignment for S3/File variants.
FormData and multipart serialization test updates
test/js/web/html/FormData.test.ts, test/js/web/html/FormData-multipart-serialization.test.ts
Existing tests updated to expect name: "blob" (instead of undefined) and filename="blob" (instead of filename="") for nameless Blobs. New regression tests for issue #14725 cover FormData.set/append for nameless Blobs, multipart Content-Disposition output, and a fetch-level integration assertion.

Possibly related PRs

  • oven-sh/bun#30955: Also modifies src/runtime/webcore/Blob.rs and the per-Blob name field — fixes borrowed_view() ownership and clone behavior for the same name field this PR now writes to via setAsFile.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately describes the main change: setting FormData Blob entries to default filename 'blob'.
Description check ✅ Passed The PR description comprehensively addresses both template sections with detailed repro steps, root cause analysis, fix explanation, and verification commands.
Linked Issues check ✅ Passed All three linked issues (#14725, #22762, #21788) are fully resolved: multipart serialization now includes filename='blob' and formData.get().name returns 'blob'.
Out of Scope Changes check ✅ Passed All code changes are directly scoped to Blob.rs and FormData test files, focusing solely on Blob filename handling per the linked issue requirements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:34 PM PT - Jun 16th, 2026

❌ @robobun, your commit 7499288 has 1 failures in Build #63006 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32431

That installs a local version of the PR into your bun-32431 executable, so you can run:

bun-32431 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. Multipart uploads to Cloudflare Workers fail because Blob in FormData serialized to multipart/form-data without filename (unlike Node) #22762 - Multipart uploads to Cloudflare Workers fail because Blob in FormData is serialized with filename="" instead of filename="blob"
  2. Inconsistent file uploading behaviour between Node and Bun. (fetch API w/ FormData) #21788 - Inconsistent file uploading behaviour between Node and Bun when using fetch with FormData (missing filename on Blob entries)
  3. Multipart formdata justs fails for file upload #21467 - Multipart FormData file upload fails due to malformed serialization of Blob entries without filename

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #22762
Fixes #21788
Fixes #21467

🤖 Generated with Claude Code

@robobun

robobun commented Jun 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI is blocked on infrastructure: linux build agents fail with Image not found: linux-*-v37 across builds 62935, 62938, 62977, and 63006. On build 63006 one lane (alpine 3.23 aarch64-musl) got agents and all 20 test shards passed with no failures. The change also passes the full FormData, structured-clone, and blob suites locally under bun bd test. Ready for review once the remaining CI agents are back.

Comment thread src/runtime/webcore/Blob.rs Resolved
Files retrieved from FormData carry their filename in the per-Blob name
field (set by Blob__setAsFile), not the store's stored_name. Re-appending
such a File to a FormData without a filename argument was returning empty
because Blob__getFileNameString only looked at the store. Check the
per-Blob name first, matching get_name_string.
Comment thread src/runtime/webcore/Blob.rs Resolved
When the parts array is a single Blob, Blob::get returns a dupe that
inherits the source's per-Blob name. The Bytes arm previously only wrote
the constructor's filename into stored_name, so the inherited name won in
get_name_string/Blob__getFileNameString. Set blob.name unconditionally so
the new File's .name always reflects the constructor argument.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/webcore/Blob.rs (1)

5652-5657: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid writing constructor names into a shared byte store.

Line 5657 still mutates bytes.stored_name even though this path can be a dupe() over a shared StoreRef as noted above. That can leak new File([blob], "x.txt") back into the original Blob, causing a later FormData.append("k", blob) to derive "x.txt" from get_file_name() instead of defaulting to "blob". Keep the constructor name on blob.name, or only populate stored_name when the byte store is uniquely owned.

Proposed fix
         blob.name.set(name_value_str.dupe_ref());
         if let Some(store_) = blob.store.get() {
-            match store_.data_mut() {
-                store::Data::Bytes(bytes) => {
+            if store_.has_one_ref() {
+                if let store::Data::Bytes(bytes) = store_.data_mut() {
                     // `get::<_, true>` on a single-Blob sequence returns
                     // `dupe()` (a shared StoreRef), so this `Bytes` may already
                     // carry an owned `stored_name` from the source blob; the
                     // assignment drops (frees) the previous `Box<[u8]>`.
                     bytes.stored_name = name_value_str.to_owned_slice().into_boxed_slice();
                 }
-                store::Data::S3(_) | store::Data::File(_) => {}
             }
         } else if !name_value_str.is_empty() {

As per coding guidelines, “Store state on the object whose lifetime matches it.”

🤖 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/runtime/webcore/Blob.rs` around lines 5652 - 5657, The mutation of
bytes.stored_name on line 5657 in the store::Data::Bytes branch can affect a
shared StoreRef when bytes is obtained via dupe(), leaking the constructor name
into the original Blob. To fix this, either avoid mutating bytes.stored_name
when it could be shared and instead store the constructor name on the blob
object itself whose lifetime matches the name's usage, or add a check to only
populate bytes.stored_name when the byte store is uniquely owned (not a shared
reference). Follow the principle that state should be stored on the object whose
lifetime matches it.

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.

Outside diff comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 5652-5657: The mutation of bytes.stored_name on line 5657 in the
store::Data::Bytes branch can affect a shared StoreRef when bytes is obtained
via dupe(), leaking the constructor name into the original Blob. To fix this,
either avoid mutating bytes.stored_name when it could be shared and instead
store the constructor name on the blob object itself whose lifetime matches the
name's usage, or add a check to only populate bytes.stored_name when the byte
store is uniquely owned (not a shared reference). Follow the principle that
state should be stored on the object whose lifetime matches it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2c096985-afa1-4f49-8dea-5aa1025bdd2b

📥 Commits

Reviewing files that changed from the base of the PR and between 91ce3b4 and 7499288.

📒 Files selected for processing (3)
  • src/runtime/webcore/Blob.rs
  • test/js/web/html/FormData-multipart-serialization.test.ts
  • test/js/web/html/FormData.test.ts

@robobun

robobun commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Re the CodeRabbit outside-diff note on jsdom_file_construct_ writing stored_name on a shared store: that mutation is pre-existing (same behavior in the current release) and is not changed by this PR. The .name getter and FormData filename are now correct via the per-Blob name field, which both get_name_string and Blob__getFileNameString check first. Gating the stored_name write on has_one_ref() would also change what get_file_name() returns for new File([blob], name) in the single-Blob fast path, which feeds loader detection for blob imports; I'd rather audit those callers separately than fold it into this fix.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both regressions I flagged are addressed and I have nothing further on this revision; deferring to a human since this reworks where Blob filenames live (per-Blob name vs shared store stored_name) across several call sites and CI hasn't yet run against the final diff.

Extended reasoning...

Overview

This PR changes FormData's default filename for plain-Blob entries from "" to "blob" per the XHR spec, and in doing so reworks how Blob filenames are stored: Blob__setAsFile now writes to the per-Blob name field instead of the shared store's stored_name, Blob__getFileNameString now consults name before the store and falls back to "blob", and jsdom_file_construct_ now sets blob.name unconditionally so the File constructor's name argument wins over anything inherited via dupe(). Tests in FormData.test.ts and FormData-multipart-serialization.test.ts are updated and extended with re-append and new File([f], name) round-trip cases.

Security risks

None apparent. The change affects which filename string is emitted in multipart bodies and returned from .name; it does not touch parsing of untrusted input, escaping, or any auth/permission path.

Level of scrutiny

Moderate-to-high. The diff is small but sits in core src/runtime/webcore/Blob.rs and shifts the source of truth for Blob filenames between two storage locations that are read by several distinct paths (get_name_string, get_file_name, Blob__getFileNameString, the File constructor's single-Blob fast path, FormData serialization). I flagged two separate regressions on earlier revisions of this PR — re-appending a FormData-retrieved File losing its name, and new File([f], newName) keeping the inherited name — both now fixed and covered by tests. That it took two iterations to converge is itself a signal that the per-Blob-name vs shared-stored_name interaction is subtle enough to warrant a human pass.

Other factors

CI failed on infrastructure both times (build agents could not be provisioned), so no automated tests have run against the current head; the author reports the FormData suite passes locally. The author also notes the pre-existing shared-store stored_name leak in jsdom_file_construct_ is left in place (kept for get_file_name() callers like loader detection / auto Content-Disposition) but is no longer observable via .name/FormData — that trade-off seems reasonable but is the kind of judgment call a maintainer should sign off on.

@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up: #32915 touches the same area from a different angle (the name getter returned undefined for any File with an empty name, e.g. new File([], "") and multipart parts with filename="", which throws in file.name.endsWith(...) handlers). The two fixes are complementary, but they both edit the name: undefined assertions in FormData.test.ts and FormData-multipart-serialization.test.ts; after both land those become "blob", which is the value your PR already asserts. Whichever merges second just needs a trivial rebase of those lines.

@robobun

robobun commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

A FormData differential pass flagged the same filename="" divergence, so I re-verified this PR against today's main and Node across the entry matrix (plain Blob and File, with and without a filename argument, append and set). It matches Node in every case, including two that the linked issues don't mention and that may be worth a line in the description:

  • form.append("a", blob, "custom.txt") followed by form.get("a") renames the caller's blob on main (blob.name becomes "custom.txt"), because Blob__setAsFile writes into the shared store. Moving the name to the per-Blob field fixes that too. The existing "should use the correct filenames" test can't catch a regression here: it reads blob.name before the get(), and the cached name getter then hides the later mutation. A test that reads the source's name for the first time after the get() does:

    const blob = new Blob(["bar"]) as any;
    const form = new FormData();
    form.append("foo", blob, "custom.txt");
    expect((form.get("foo") as File).name).toBe("custom.txt");
    expect(blob.name).toBeUndefined();
    expect(blob instanceof File).toBe(false);
  • form.append("a", file, "override.txt") ignores the override on main (the stored_name.is_empty() guard) and keeps the File's original name. Also fixed here.

Two things worth folding in while this function is open:

1. The no-filename arm still hands C++ a borrowed view of the store's bytes, and that is a reachable use-after-free on main.

BunString::from_bytes(get_file_name()) plus toWTFString(BunString::ZeroCopy) ends in StringImpl::createWithoutCopying over Bytes.stored_name, so the entry's m_fileName points into a Box<[u8]> owned by the (shared) store. new File([f], name) replaces that box and frees the old one, and the entry then serializes freed memory:

const f = new File(["x"], "aaaa.txt");
const fd = new FormData();
fd.append("k", f);
new File([f], "bbbb.txt"); // frees the allocation m_fileName points into
console.log(await new Response(fd).text());

On 1.4.0 the part's filename comes out as heap garbage that changes every run, e.g. filename="\u0006¤\u0010u\u0003\u0000\u0000", so freed heap bytes end up in outgoing request bodies. Delegating to get_name_string() fixes that as part of this change, because that helper caches an owned copy on the blob (so the C++ String refs an owned StringImpl) and already implements the name-field-first precedence this PR adds by hand:

#[unsafe(no_mangle)]
pub extern "C" fn Blob__getFileNameString(this: &Blob) -> BunString {
    if let Some(name) = this.get_name_string() {
        return name;
    }
    if this.is_jsdom_file.get() {
        // `new File(bits, "")`: keep the explicit empty name.
        return BunString::empty();
    }
    BunString::static_(b"blob")
}

With that, the snippet above prints filename="aaaa.txt" on every run.

2. lastModified of a wrapped Blob. Step 3.1 of "create an entry" makes a new File, so it should get a fresh lastModified. With the PR as-is, form.get() of a plain Blob entry reports lastModified === 0; Node and Chrome report the wrap time. Blob__setAsFile already knows whether it is wrapping a non-File:

pub extern "C" fn Blob__setAsFile(this: &mut Blob, path_str: &mut BunString) {
    if !this.is_jsdom_file.replace(true) {
        // Wrapping a plain `Blob` creates a new `File`: stamp `lastModified`.
        this.last_modified
            .set(bun_core::time::milli_timestamp() as f64);
    }
    this.name.set(path_str.dupe_ref());
}

A real File keeps its own lastModified (the flag is already set), and the value is stable across repeated get() calls.

I also confirmed the jsdom_file_construct_ hunk here is load-bearing rather than incidental: I tried the fix without it and new File([form.get("a")], "renamed.txt").name comes back "blob" once Blob__setAsFile starts populating name, so please keep that assertion.

Other cases verified against Node that are cheap to assert: append("k", blob, "") produces a File named "", a File's own lastModified survives the entry ({ lastModified: 1234 } round-trips), and Bun.file(path) entries keep the path as the filename, so the "blob" default must only apply when the blob has no name of its own. I pushed these as tests on farm/cc79e482/formdata-blob-entry-name if you want to lift them; note that branch's src side is missing this PR's jsdom_file_construct_ change, so this PR is the one to finish.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#14725) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant