Skip to content

fetch: derive blob() type and the formData() boundary from the Content-Type header per the spec - #33128

Closed
robobun wants to merge 8 commits into
mainfrom
farm/5c7c54a8/fetch-blob-mime-type
Closed

robobun wants to merge 8 commits into
mainfrom
farm/5c7c54a8/fetch-blob-mime-type

Conversation

@robobun

@robobun robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator

Response.prototype.blob() and Request.prototype.blob() must resolve to a plain Blob whose type is the result of extracting a MIME type from the header list, serialized per the MIME Sniffing standard and normalized like any Blob type (ASCII-lowercased, or "" if any character falls outside U+0020-U+007E).

Bun returned the body's own Blob instead: a File body came back as a File, name and all, its raw type shadowed the Content-Type header, and when the header was consulted it went through bun_http_types::MimeType::init, a category lookup table rather than a MIME parser.

const r = new Response(new File(["x"], "n.bin", { type: "My/Type; A=B" }));
const b = await r.blob();
b.type;            // Node: "my/type;a=b"   Bun: "my/type; a=b"
b instanceof File; // Node: false           Bun: true, with b.name === "n.bin"

const r2 = new Response("x", { headers: { "content-type": "TEXT/Plain ; Charset=UTF-8" } });
(await r2.blob()).type; // Node: "text/plain;charset=utf-8"   Bun: "TEXT/Plain ; Charset=UTF-8"

In a 15-case probe of the body/header matrix, 14 cases diverged from Node (table in the details block below). Beyond the examples above:

  • Bun.serve request bodies resolve on a native path that received no headers, so request.blob().type was always "text/plain;charset=utf-8" no matter what the client sent.

  • When the body was a typed Blob and a different Content-Type header was also given, the blob's type won; the spec says the header does.

  • Content-Type: application/json from a server was rewritten to application/json;charset=utf-8 by the lookup table.

  • A ReadableStream body never consulted the header at all, so its blob() had an empty type.

  • formData() read its boundary from the raw combined header value, so a response (or Request/Response init) carrying two Content-Type lines (boundary=B1, then boundary=B2) yielded a boundary matching neither framing and formData() rejected with ERR_FORMDATA_PARSE_ERROR; Node parses the B2-framed body.

  • The header-derived type was written into the body blob's shared Store, so blob() could retype an object the caller still holds:

    const s = new Blob(["x"]);
    await new Response(s, { headers: { "content-type": "b/b" } }).blob();
    s.type; // Bun: "b/b"   Node: ""
Node vs. Bun 1.4.0 across the body/header matrix
case Node Bun 1.4.0
File(type:"My/Type; A=B") body "my/type;a=b", plain Blob "my/type; a=b", a File named n.bin
Blob(type:"My/Type; A=B") body "my/type;a=b" "my/type; a=b"
string body + header TEXT/Plain ; Charset=UTF-8 "text/plain;charset=utf-8" "TEXT/Plain ; Charset=UTF-8"
string body, no header "text/plain;charset=utf-8" "text/plain;charset=utf-8"
Uint8Array body, no header "" "text/plain;charset=utf-8"
Blob(type:"a/a") body + header b/b "b/b" "a/a"
header */* "" "application/octet-stream"
header invalid "" "invalid"
ReadableStream body + header "text/plain;charset=utf-8" ""
Request with a File body "my/type;a=b", plain Blob "my/type; a=b", a File
Request string body + header "text/plain;charset=utf-8" "TEXT/Plain ; Charset=UTF-8"
fetch() of a TEXT/Plain ; Charset=UTF-8 response "text/plain;charset=utf-8" "TEXT/Plain ; Charset=UTF-8"
Bun.serve request.blob() (any Content-Type) normalized header always "text/plain;charset=utf-8"

Fix

src/http_types/mime_sniff.rs implements the WHATWG "parse a MIME type" and "serialize a MIME type" algorithms plus Fetch's "extract a MIME type", including the get-decode-split step over comma-combined values and the charset carry-over. It was validated byte for byte against Node v26 across 102 header vectors; the only intentional divergences are two inputs containing a backtick in a token, where undici's HTTP-token regex is missing U+0060 (`) and the MIME Sniffing standard includes it.

Action::GetBlob now carries the normalized type, computed once in get_blob_with_this_value where the Request/Response and its headers are in hand, so every resolution path agrees:

  • the direct path (body already materialized);
  • readableStreamToBlob, for a JS ReadableStream body; the builtin gains an optional contentType parameter, mirroring readableStreamToFormData. It stamps the type through an internal setBlobType native helper rather than new Blob(chunks, { type }), because the Blob constructor canonicalizes well-known essences through the interned MIME table (application/json becomes application/json;charset=utf-8, Normalizing blob type with charset causing bug on type validation #15078) and would have put this path out of step with the others. The helper keeps the constructor's validation and ASCII-lowercasing (so CR/LF can never reach a Content-Type header through it), and a non-string contentType throws ERR_INVALID_ARG_TYPE;
  • the native buffering path, which previously got its headers from Value::resolve's headers argument. That argument is now unused and removed. The Bun.serve caller passed None for it, which is what broke request.blob() there.

The result blob drops the source body's File identity (is_jsdom_file, name, lastModified), and nothing writes into the shared store anymore. Blob.type falls back to the store's mime_type when the blob's own type is empty, and Blob::from_url_search_params was the one body constructor that set it, so a blob() result whose header extracted to nothing (or a slice() of the result) still read back application/x-www-form-urlencoded; that constructor now puts the type on the blob only, like every other one. Bun materializes the body-derived Content-Type header lazily, so for the purposes of extraction the "header list" is the explicit header, else what "extract a body" derives from the body: the blob's own type, or text/plain;charset=UTF-8 for a string body. A stream the caller provided (or a network body) derives nothing.

A string body's identity only lives in the body value, and three value transformations used to erase it there, so blob() reported "" after any of them:

  • reading .body converts the body into a ReadableStream of plain bytes; to_readable_stream now captures the derived type on the pending value (PendingValue::source_content_type) and blob() reads it back,
  • recovering the body from that stream (to_blob_if_possible, reached by clone()) returned the bytes with was_string cleared; it now restores the flag from the captured type,
  • clone() converts an InternalBlob body into an untyped shared-store Blob (a never-streamed non-ASCII string body takes this path too); that blob now keeps the text/plain;charset=utf-8 its was_string implied.

The pending value owning these captures is torn down by Value::reset() on Response::destroy, which never runs drop glue (it resets, then deallocates the allocation raw), so reset() now releases the captured type and a pending action's payload. The CI ASAN lane's LeakSanitizer caught that for responses finalized while still holding their .body stream, which also covers the pre-existing Action::GetFormData case.

The three Value::resolve call sites whose signature this changes (two in RequestContext.rs, one in html_rewriter.rs) each discarded the result with a let _ =, which leaves a thrown exception pending on the VM. They now report it through report_uncaught_exception_from_error, the idiom RuntimeTranspilerStore already uses for an exception escaping a void native callback.

Multipart boundary

Making blob().type conformant forces one further change. The File API requires Blob.type to read back ASCII-lowercased, so a multipart boundary containing uppercase letters can never survive request.blob() followed by fetch(url, { body }): the re-posted Content-Type names a lowercased boundary that the body bytes do not contain. Node, Gecko, and Deno all generate lowercase-safe boundaries, which is the only reason the round-trip works there; an existing test for #21011 exercises exactly that round-trip and would otherwise regress.

Bun's generated boundary prefix therefore changes from ----WebKitFormBoundary to ----formdata-bun-. The leading-dash count and 32-hex suffix, which are what #29630 actually needed, are unchanged.

formData()

Request/Response get_form_data_encoding now go through form_data::encoding_from_header, which runs the same "extract a MIME type" over the combined header value before Encoding::get, so a stacked Content-Type yields the last valid value's boundary. Boundaries containing non-token characters (=, :, space) come back from the serializer as a quoted parameter, which get_boundary already unquotes; tests pin that. Blob.prototype.formData() is unchanged: Blob.type is a single normalized value, not a header list.

Tests

test/js/web/fetch/body.test.ts gains, for both Request and Response: a table of Content-Type header values and their expected blob().type, each pair generated by running the same header through Node v26; a second table of stacked (repeated) Content-Type headers; the duplicated-boundary formData() case (B2-framed body parses, B1-framed rejects) and the quoted/non-token boundary cases; the plain-Blob and File-identity checks; the header-over-body-type precedence; the shared-store non-mutation; the string-body and BufferSource defaults; the ReadableStream body; a Bun.file body; the .body-before-blob() cases (typed, untyped, and empty Blob, string, URLSearchParams, FormData, an explicit header, .headers first, clone() after .body for Blob and string bodies, clone() of ASCII and non-ASCII string bodies, and a body built from another body's stream), all verified against Node v26. Over the network: a fetch() round trip through Bun.serve, a raw-socket server sending repeated Content-Type lines (checked through headers.get, blob(), and formData()), Bun.serve request.blob() with a normalized header, and the #32801 repro (a posted File with four different types round-trips each one). Bun.readableStreamToBlob is covered with and without the new parameter.

The file is 641 tests; all pass with this branch. On the released bun the stacked-header, formData(), network, and the original blob() cases fail.

Two existing assertions encoded the old behavior and are updated:

Not changed

  • Content-Type: "" (present but explicitly empty) still falls through to the body-implied type, because FetchHeaders::fastGet cannot distinguish an absent header from a present-but-empty one. Pre-existing, not a regression, and it also affects formData() and WebAssembly.compileStreaming.
  • Response.json() sets application/json;charset=utf-8 where Node sets application/json; blob() now faithfully reflects whichever header is there.
  • new Blob(["x"], { type: "text/plain" }).type returning "text/plain;charset=utf-8" (Normalizing blob type with charset causing bug on type validation #15078) comes from the Blob constructor's interned MIME table, a separate code path that blob() no longer goes through.

Branch state

origin/main (f426a8e) is merged in. Conflicts were in html_rewriter.rs (the RewriterPipe refactor), Body.rs (pub(crate) narrowing) and the body.test.ts imports. Lints that landed in the meantime required three adjustments, all in the merge commit: mime_sniff.rs searches bytes through bun_core::strings, Blob__setType is a pub(crate) unsafe export built on ffi::slice, and the inherent Response::get_fetch_headers (whose only caller was the removed resolve argument) is deleted, which also lets FetchTasklet::on_body_received hold a plain &mut to the body instead of a raw pointer. cargo clippy -p bun_http_types -p bun_runtime is clean; body.test.ts, blob.test.ts, client-fetch.test.ts, the FormData suites, and the HTMLRewriter suites pass locally.

Earlier rebase notes (onto 4924862)

Rebased onto main at 4924862 (was 132 commits behind). Three refactors landed under this branch that forced non-trivial conflict resolution:

The eleven commits are squashed into one on top of main; every change from the prior tip (e14cd7e) is carried forward. test/js/web/fetch/body.test.ts is 517 tests, 147 red on the released bun and all green with the build; adjacent body, clone, stream, FormData, and readableStreamToBlob suites also pass.

Related PRs

Fixes #32801
Fixes #19603
Fixes #35284


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/fetch/blob.test.ts test/js/web/fetch/body.test.ts test/js/web/html/FormData-multipart-serialization.test.ts

@robobun
robobun requested a review from alii as a code owner June 30, 2026 04:52
@robobun

robobun commented Jun 30, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:05 AM PT - Aug 13th, 2026

❌ @robobun, your commit 290d9de has some failures in Build #94138 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 33128

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

bun-33128 --bun

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. server: preserve request Content-Type in request.blob() #32806 - Fixes the same issue (Bug: Bun server loses File MIME type when reading request body as Blob #32801) by preserving request Content-Type in request.blob(); fully superseded by this PR's more comprehensive MIME sniffing approach
  2. webcore: fix Content-Type lost when Request/Response body is read before headers #32913 - Fixes the same Content-Type-lost-during-blob-read problem in Body.rs; superseded by this PR's refactor of blob type normalization

🤖 Generated with Claude Code

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Not duplicates, but both are related:

@coderabbitai

coderabbitai Bot commented Jun 30, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds MIME parsing and extraction, carries normalized content type data through Body blob creation, extends readableStreamToBlob with an optional contentType, and updates multipart boundary generation, blob name handling, and related tests.

Changes

Blob type normalization and multipart boundaries

Layer / File(s) Summary
MIME sniff helpers and extraction
src/http_types/lib.rs, src/http_types/mime_sniff.rs
New MIME parsing helpers and exported MIME extraction logic are added, including byte predicates, trimming, quoted-string handling, comma-split decoding, record serialization, and Fetch-style extraction.
Body blob resolution and content type propagation
src/runtime/webcore/Body.rs, src/runtime/webcore/fetch.rs, test/js/web/fetch/body.test.ts, test/js/web/fetch/client-fetch.test.ts, test/js/web/fetch/blob.test.ts
Body blob materialization now carries normalized content type bytes through resolution, and the tests cover blob type normalization, request/response/server behavior, data URLs, and empty-file-name handling.
Body mixin implementations and resolve call sites
src/runtime/webcore/Request.rs, src/runtime/webcore/Response.rs, src/runtime/webcore/fetch/FetchTasklet.rs, src/runtime/server/RequestContext.rs, src/runtime/api/html_rewriter.rs
Request and Response now provide BodyMixin content-type access, and resolve call sites were updated to the new signature with explicit error handling.
ReadableStream blob contentType plumbing
src/js/builtins/ReadableStream.ts, src/codegen/generate-js2native.ts, src/jsc/bindings/ZigGlobalObject.h, src/jsc/bindings/webcore/ReadableStream.cpp, src/jsc/bindings/webcore/JSReadableStream.cpp, src/jsc/JSGlobalObject.rs, packages/bun-types/deprecated.d.ts
readableStreamToBlob accepts an optional contentType, and the JS, Rust, C++, C API, and deprecated type declarations are updated to pass the extra argument through.
FormData boundary prefix and Blob name behavior
src/runtime/webcore/Blob.rs, test/js/bun/http/form-data-set-append.test.js, test/js/web/html/FormData-multipart-serialization.test.ts
Multipart form-data now uses a Bun-specific boundary prefix, and empty blob names on non-File blobs now resolve to no name; the multipart tests were updated accordingly.

Possibly related PRs

  • oven-sh/bun#29631: Changes the same multipart FormData boundary expectations and related tests.
  • oven-sh/bun#31379: Also modifies Blob::from_dom_form_data multipart serialization and boundary-related test coverage.
  • oven-sh/bun#32975: Touches multipart serialization in Blob::from_dom_form_data and related tests in the same file.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation satisfies #32801 and #19603 by deriving blob types from Content-Type; #39 is unrelated and can be ignored.
Out of Scope Changes check ✅ Passed The diff stays centered on the blob MIME-type fix and its supporting plumbing, with no clearly unrelated feature work.
Title check ✅ Passed The title clearly summarizes the primary blob type and FormData boundary changes described in the pull request.
Description check ✅ Passed The description explains the changes, implementation, related issues, scope, and extensive verification results.

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

@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.

Actionable comments posted: 2

🤖 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/runtime/webcore/Blob.rs`:
- Around line 2162-2167: The name-handling logic in Blob::get_name is conflating
an explicitly empty filename with the absence of a name, which breaks File
values created with an empty string. Update the Blob.rs name fallback so
BunString::empty() is preserved as a real name for File instances, and only
suppress the store-derived fallback for the plain-Blob path using a distinct
sentinel or a guard such as is_jsdom_file. Keep the behavior consistent with
Body.rs and any structured-clone/multipart code that relies on Blob::get_name
returning an explicit empty string rather than undefined.

In `@test/js/web/fetch/body.test.ts`:
- Around line 741-753: The comment above the `bodyTypes` loop in `body.test.ts`
is too long and mostly historical test provenance instead of a durable
invariant. Trim it to a short spec-facing note near the `describe(...blob())`
test, and remove the PR-history details about Bun behavior and the Node v26
vector generation. Keep only the non-obvious invariant that the test is
verifying, leaving the rationale/history out of the source comment.
🪄 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: 7b5d0f59-3d85-475a-87ea-27e9e9e8c34f

📥 Commits

Reviewing files that changed from the base of the PR and between a5d122d and cf09069.

📒 Files selected for processing (19)
  • packages/bun-types/deprecated.d.ts
  • src/http_types/lib.rs
  • src/http_types/mime_sniff.rs
  • src/js/builtins/ReadableStream.ts
  • src/jsc/JSGlobalObject.rs
  • src/jsc/bindings/ZigGlobalObject.h
  • src/jsc/bindings/webcore/JSReadableStream.cpp
  • src/jsc/bindings/webcore/ReadableStream.cpp
  • src/runtime/api/html_rewriter.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/webcore/Blob.rs
  • src/runtime/webcore/Body.rs
  • src/runtime/webcore/Request.rs
  • src/runtime/webcore/Response.rs
  • src/runtime/webcore/fetch/FetchTasklet.rs
  • test/js/bun/http/form-data-set-append.test.js
  • test/js/web/fetch/body.test.ts
  • test/js/web/fetch/client-fetch.test.ts
  • test/js/web/html/FormData-multipart-serialization.test.ts

Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread test/js/web/fetch/body.test.ts Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/js/builtins/ReadableStream.ts Outdated
Comment thread src/runtime/webcore/Blob.rs Outdated

@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.

Additional findings (outside current diff — PR may have been updated during review):

  • 🟡 src/runtime/webcore/Blob.rs:876-883 — nit: this comment block is 7 lines, exceeding the repo's 3-line limit (CLAUDE.md rule 13). You already trimmed two similar over-length blocks in body.test.ts per CodeRabbit feedback in 526d583; this one looks like it was just missed. The lowercase invariant is the durable bit worth keeping — the undici/Gecko comparison and #29630 reference are already in the PR description.

    Extended reasoning...

    What the issue is. The repo's root CLAUDE.md rule 13 (line 268) states: "Keep code comments to 3 lines max — Comments must be concise. If the code needs more explanation than that, it belongs in docs." The new comment block above BOUNDARY_PREFIX in Blob::from_dom_form_data (Blob.rs:876-882) is 7 lines. The same guidelines elsewhere (and the CodeRabbit feedback already applied in this PR) note that bug history / provenance belongs in the PR description, not source.

    Why this is worth mentioning here. In commit 526d583 of this same PR, the author responded to CodeRabbit's identical "trim this comment block" feedback on test/js/web/fetch/body.test.ts with "Trimmed both blocks to three lines in 526d583; the rationale and the vector provenance now live only in the PR description." So the author is actively applying this guideline within the PR — this block in Blob.rs was added in the earlier commit (65055cb) and CodeRabbit's rate-limited review didn't flag it, so it was simply missed rather than intentionally kept long.

    Step-by-step. Counting the lines at src/runtime/webcore/Blob.rs:876-882:

    1. // Prefix + 32 lowercase-hex chars of a fresh UUID. The boundary must
    2. // not contain uppercase ASCII: the File API requires \Blob.type` to`
    3. // read back ASCII-lowercased, so an uppercase boundary could never
    4. // survive a \request.blob()` -> `fetch(url, { body })` round-trip.`
    5. // undici and Gecko generate lowercase-safe boundaries for the same
    6. // reason. The leading-dash count and hex suffix are what downstream
    7. // multipart parsers actually key on (#29630) and are unchanged.

    That's 7 lines vs. the 3-line cap. Lines 1-4 carry the durable non-obvious invariant (why lowercase-only). Lines 5-7 are cross-impl provenance ("undici and Gecko do the same") and bug history ("#29630 … unchanged"), both of which the PR description already covers verbatim under the Multipart boundary section.

    Why existing code doesn't already prevent it. Enforcement of this rule is review-based, not lint-based. The same file already has an 8-line // block immediately below (lines 903-910), so the file itself isn't a clean baseline — but that block is pre-existing and out of scope. What makes this one worth a nit is purely consistency with the author's own trims elsewhere in this PR.

    Impact. None functional — pure style. This is the lowest-severity finding possible: a comment-length guideline. It's flagged only because (a) the rule is explicit in CLAUDE.md, and (b) the author already applied the same trim to two sibling blocks in this PR, so leaving this one long is an inconsistency they'd likely want to fix before merge rather than an intentional exception.

    How to fix. Condense to the invariant and drop the provenance/history, e.g.:

    // Prefix + 32 lowercase-hex chars of a fresh UUID. Lowercase-only so the
    // boundary survives a `request.blob()` -> `fetch({ body })` round-trip
    // (the File API ASCII-lowercases `Blob.type`). Dash count/hex unchanged.
    const BOUNDARY_PREFIX: &[u8; 17] = b"----formdata-bun-";

    The undici/Gecko comparison and the #29630 reference already live in the PR description ("Node, Gecko, and Deno all generate lowercase-safe boundaries… The leading-dash count and 32-hex suffix, which are what #29630 actually needed, are unchanged") and in test/js/bun/http/form-data-set-append.test.js's file-level comment, so nothing is lost.

@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.

Actionable comments posted: 2

🤖 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/http_types/mime_sniff.rs`:
- Around line 312-323: The carried charset logic in mime_sniff::parse_mime_type
only updates on a new essence and misses the case where the same essence
supplies a different charset. Adjust the same-essence branch so it also
refreshes the carried charset when parsed.parameter(b"charset") is present,
while still inheriting it only when the charset parameter is absent. Use the
existing essence/charset handling in mime_sniff.rs to keep the final combined
value aligned with the most recent charset for that essence.

In `@src/js/builtins/ReadableStream.ts`:
- Around line 336-350: The readableStreamToBlob API currently accepts non-string
contentType values and only fails later inside setBlobTypeVerbatim, after the
stream may already be consumed. Add upfront validation in readableStreamToBlob
for contentType when it is provided, using the existing typed-argument pattern
($ERR_INVALID_ARG_TYPE or the appropriate validation helper) so non-string
values throw immediately before any stream work starts. Keep the current
behavior unchanged for undefined, and preserve the existing Blob type-setting
path for valid string inputs.
🪄 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: 942d3cce-abb2-4cee-ada2-e90f57341510

📥 Commits

Reviewing files that changed from the base of the PR and between cf09069 and a04e10b.

📒 Files selected for processing (21)
  • packages/bun-types/deprecated.d.ts
  • src/codegen/generate-js2native.ts
  • src/http_types/lib.rs
  • src/http_types/mime_sniff.rs
  • src/js/builtins/ReadableStream.ts
  • src/jsc/JSGlobalObject.rs
  • src/jsc/bindings/ZigGlobalObject.h
  • src/jsc/bindings/webcore/JSReadableStream.cpp
  • src/jsc/bindings/webcore/ReadableStream.cpp
  • src/runtime/api/html_rewriter.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/webcore/Blob.rs
  • src/runtime/webcore/Body.rs
  • src/runtime/webcore/Request.rs
  • src/runtime/webcore/Response.rs
  • src/runtime/webcore/fetch/FetchTasklet.rs
  • test/js/bun/http/form-data-set-append.test.js
  • test/js/web/fetch/blob.test.ts
  • test/js/web/fetch/body.test.ts
  • test/js/web/fetch/client-fetch.test.ts
  • test/js/web/html/FormData-multipart-serialization.test.ts

Comment thread src/http_types/mime_sniff.rs
Comment thread src/js/builtins/ReadableStream.ts Outdated
@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Good catch on the comment length, trimmed to three lines in d2b864a (and tightened the new set_blob_type_verbatim doc while there). The undici/Gecko comparison and the #29630 reference stay in the PR description and in form-data-set-append.test.js's header comment.

Comment thread test/js/web/fetch/client-fetch.test.ts
Comment thread src/js/builtins/ReadableStream.ts Outdated
Comment thread src/runtime/webcore/Blob.rs Outdated

@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.

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/runtime/webcore/Blob.rs`:
- Around line 89-92: The doc comment for setBlobType in Blob.rs is too long and
exceeds the repository’s 3-line comment limit. Compress the existing explanation
into three lines or fewer while keeping the key points: it applies only to
readableStreamToBlob, it validates and ASCII-lowercases the type like the Blob
constructor, and it must not canonicalize through the interned MIME table. Refer
to the setBlobType comment block directly when editing.
🪄 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: 95213837-c9d3-4aa8-b64e-36cec12783da

📥 Commits

Reviewing files that changed from the base of the PR and between d2b864a and 077c791.

📒 Files selected for processing (4)
  • src/js/builtins/ReadableStream.ts
  • src/runtime/webcore/Blob.rs
  • src/runtime/webcore/fetch.rs
  • test/js/web/fetch/body.test.ts

Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated

@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.

All my prior findings have been addressed and the latest revision looks clean, but this is a substantial change — a new ~330-line WHATWG MIME parser, a rewrite of the blob() resolution paths across Body/Request/Response, a user-visible multipart boundary prefix change, and a new public contentType parameter on Bun.readableStreamToBlob — so it warrants a human sign-off.

Extended reasoning...

Overview

This PR makes Request/Response.prototype.blob() spec-conformant by deriving the result's type from the header list via WHATWG MIME extraction. It touches 22 files: a new src/http_types/mime_sniff.rs (~330 lines implementing "parse/serialize a MIME type" and Fetch's "extract a MIME type"), a substantial rewrite of Body.rs (into_consumed_blob, normalize_blob_type, Action::GetBlob now carries the normalized type, PendingValue::source_content_type, Value::resolve signature change), Blob.rs (set_blob_type native helper, get_name_string empty-name suppression, multipart boundary prefix changed from ----WebKitFormBoundary to ----formdata-bun-), C++/Rust FFI signature updates for readableStreamToBlob, a new optional contentType parameter on the public Bun.readableStreamToBlob, and ~340 lines of new tests in body.test.ts.

Security risks

Low. The new setBlobType path was hardened during review to validate/lowercase like the Blob constructor (rejecting CR/LF and bytes outside U+0020–U+007E), so user-supplied contentType cannot inject into outgoing Content-Type headers. The MIME parser operates on byte slices with Rust bounds-checked indexing and no unsafe. The boundary change keeps the same entropy (32 hex chars from a fresh UUID). The three Value::resolve call sites that previously discarded errors with let _ = now report them via report_uncaught_exception_from_error, which is strictly an improvement.

Level of scrutiny

High. This is core web-platform behavior on a hot path (fetch, Bun.serve, Body mixin), with multiple resolution paths (direct, JS ReadableStream, native buffering, .body-then-recovery, clone()) that must agree. The PR went through seven rounds of bot review that surfaced real bugs (stale interned-pointer leak, Blob-constructor canonicalization on the stream path, File("") name regression, data:, URL CI failure, .body-before-blob() regression, clone()-after-.body string-body gap, invalid-contentType fast-path leak), each fixed with a targeted commit and test. That iteration history itself argues for a human pass over the final shape.

Other factors

The PR also makes design-level choices a maintainer should ratify: changing the generated multipart boundary prefix (user-visible in wire bytes and downstream parsers), adding a public API parameter to a deprecated helper, and explicitly superseding #32806 while overlapping textually with #32913/#32915/#32430. Test coverage is extensive (102-vector header table verified against Node v26, plus the regression rows added for each review finding), and the latest bug-hunting pass found nothing — but the breadth of the change and the number of subtle interactions uncovered during review put this outside what I'd approve without a human look.

@robobun

robobun commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

CI status on e14cd7e (build 67208): 283 jobs passed, including all 20 ASAN test shards. The LeakSanitizer failure from the previous build is fixed: Value::reset now frees the pending body's owned payloads, which the Response::destroy reset-then-raw-dealloc teardown never ran drop glue for.

The two remaining red lanes are pre-existing, tracked CI issues with no overlap with this diff:

Re-running just those BuildKite jobs is enough to get a green check; a fresh build would re-roll all 286 jobs against the same two known-bad lanes.

Response.prototype.blob() and Request.prototype.blob() now resolve to a plain
Blob whose type is the result of Fetch's "extract a MIME type" over the header
list, serialized per the MIME Sniffing standard and normalized like any Blob
type. The File identity of a body source is dropped, an explicit Content-Type
header wins over the body blob's type, and the type agrees across the direct,
stream-resolved, and Bun.serve resolution paths.

src/http_types/mime_sniff.rs implements the WHATWG parse/serialize algorithms
and Fetch's extraction step (get-decode-split over comma-combined values with
charset carry-over). Action::GetBlob now carries the normalized type, computed
once where the headers are in hand; PendingValue::source_content_type captures
the body-derived type before .body turns the body into a stream, and clone()'s
tee propagates it so clone() after .body keeps the type.

readableStreamToBlob (now in BunStreamConsumers.cpp after the C++ ReadableStream
rewrite) gains an optional contentType that is stamped onto the result via a
Blob__setType FFI, skipping the Blob constructor's interned-MIME
canonicalization. Value::reset releases the pending body's owned payloads so a
Response finalized while Locked does not leak them.

The generated multipart boundary prefix becomes ----formdata-bun-: Blob.type
reads back ASCII-lowercased, so the prior mixed-case prefix could not survive
request.blob() followed by fetch({body}). An empty data: URL mediatype defaults
to text/plain. client-fetch.test.ts restores undici's original expectation.

Fixes #32801
Fixes #19603
@robobun
robobun force-pushed the farm/5c7c54a8/fetch-blob-mime-type branch from e14cd7e to 6749f1d Compare July 9, 2026 00:05
Comment thread src/runtime/webcore/Body.rs
Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread test/js/web/fetch/body.test.ts
BunString::clone_utf8 returns a +1-ref WTFStringImpl and to_js bumps it for
the JS side; BunString is Copy with no Drop, so the temporary's +1 was never
released. Use BunString::init (a borrow, like the GetFormData arm) and fix
the Blob__setType SAFETY comment to describe the actual caller shape.

@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.

All prior review feedback has been addressed and no new issues were found on this revision, but this is a substantial change (a new 328-line MIME parser, Body/Blob ownership plumbing across Rust/C++ FFI, and a user-visible multipart boundary prefix change) that warrants a human sign-off.

Extended reasoning...

Overview

This PR reworks how Response.prototype.blob() / Request.prototype.blob() derive the result Blob's type to match the Fetch spec and Node. It adds a full WHATWG MIME parser (src/http_types/mime_sniff.rs, 328 lines), threads a normalized content type through Action::GetBlob and PendingValue::source_content_type in Body.rs, adds a contentType parameter to readableStreamToBlob across the Rust/C++/TS/type-declaration surfaces, adds a Blob__setType FFI export, changes the multipart boundary prefix from ----WebKitFormBoundary to ----formdata-bun-, and updates ~350 lines of tests. It also removes the headers parameter from Value::resolve and converts three let _ = swallowed-error call sites to report_uncaught_exception_from_error.

Security risks

Low but non-zero. The new MIME parser processes untrusted header bytes; it operates on &[u8] with Rust-bounds-checked slicing and no unsafe, so there is no memory-safety exposure there. The Blob__setType FFI entry point takes a raw pointer/len pair; the SAFETY invariant (single caller passes a stack-owned WTF::CString) is documented and holds. The contentType argument to the public Bun.readableStreamToBlob is validated (non-string rejected, bytes outside U+0020..U+007E dropped) so CR/LF cannot reach an outgoing Content-Type header — this was specifically hardened during review.

Level of scrutiny

High. This touches core webcore body-resolution and Blob ownership paths (Body.rs, Blob.rs, Response.rs, Request.rs, fetch.rs, RequestContext.rs), adds owned payloads to the Action enum and PendingValue struct with corresponding cleanup in Value::reset(), and changes FFI signatures across four languages. The PR went through 8+ review iterations in which real bugs were found each round — a stale-interned-pointer leak, a File("") name regression, a per-call WTFStringImpl leak, a .body→clone()→blob() type-erasure regression, and a data-URL default-mediatype regression that broke CI on all platforms. All of those are now resolved with tests, but the density of issues found is itself a signal that the change surface is subtle.

Other factors

The boundary-prefix change (----WebKitFormBoundary → ----formdata-bun-) is a user-visible behavior change with a well-argued rationale (lowercase-safety for the blob().type → fetch({body}) round-trip), but it's the kind of compatibility-vs-correctness tradeoff a maintainer should explicitly ratify. The PR also supersedes #32806 and interacts with three other open PRs (#32913, #32915, #32430). Test coverage is very thorough (517 tests in body.test.ts, 147 red on release Bun and green on the branch), and the PR description is exceptionally detailed. No outstanding unresolved review threads remain.

@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

CI on aaa72a5 (build 70760, the rebase onto main at 4924862): 282 jobs passed including all 20 ASAN test shards, so the rebased ownership paths (Value::reset releasing the pending body's payloads, and the ZigString::init borrow in set_promise) are clean under LeakSanitizer.

The two red lanes are pre-existing CI flakes with no overlap with this diff:

  • :windows: 2019 x64-baseline on test/js/sql/postgres-binary-array-bounds.test.ts: the test received ERR_POSTGRES_CONNECTION_REFUSED (Postgres service unavailable on the Windows host). 7 of the 10 most recent failed builds on other branches have the same failure.
  • :darwin: 14 x64 on test/js/bun/terminal/terminal.test.ts: "creates subprocess with terminal attached" timed out at 90s; also seen on an unrelated recent PR build.

Neither touches fetch, Body, Blob, or streams. Re-running just those two jobs should be enough to get a green check.

Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread src/runtime/webcore/Blob.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/Body.rs Outdated
Comment thread src/runtime/webcore/FormData.rs Outdated
Comment thread src/runtime/webcore/fetch.rs Outdated
@robobun robobun changed the title fetch: derive blob() type from the Content-Type header per the spec fetch: derive blob() type and the formData() boundary from the Content-Type header per the spec Aug 13, 2026
Comment thread src/runtime/webcore/Body.rs
Blob.type falls back to the store's mime_type when the blob's own type is
empty, and from_url_search_params was the one body constructor that wrote
it, so a blob() result whose header extracted to nothing (and a slice() of
the result) still read back application/x-www-form-urlencoded. The blob's
own content_type already carries the value; drop the store write.

@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.

290d9de addresses the last note (the vestigial store.mime_type write in from_url_search_params is gone, and the URLSearchParams + */*/invalid header cases are now pinned in the test). This pass found nothing further; the Bun.file() variant of the same store-fallthrough was also checked and ruled out. Given the scope — a new spec MIME parser, FFI signature changes across the Rust/C++ stream boundary, a new Bun.readableStreamToBlob parameter, and the user-visible multipart boundary prefix change — a human sign-off is still warranted.

Extended reasoning...

Overview

24 files across src/http_types/, src/runtime/webcore/{Body,Blob,Request,Response,FormData,fetch}.rs, the C++ ReadableStream consumers, Bun.serve's RequestContext, html_rewriter, and five test files. The core change routes Request/Response .blob().type and the .formData() boundary through a new WHATWG-conformant extract_mime_type (mime_sniff.rs, ~300 lines), threads the normalized type through Action::GetBlob(Option<BlobContentType>) and a new PendingValue::source_content_type field so every resolution path (direct, native buffered, JS-stream via readableStreamToBlob) agrees, and drops the shared-store mutation that used to retype caller-owned Blobs. It also changes the generated multipart boundary prefix to a lowercase-safe ----formdata-bun- and adds an optional contentType argument to Bun.readableStreamToBlob.

Since the last review

My 2026-08-13 note flagged that into_consumed_blob(_, None) left store.mime_type visible for a URLSearchParams body. 290d9de removes the store.mime_type write from from_url_search_params (now sets only blob.content_type to a Static value) and adds urlSearchParams / urlSearchParamsInvalid / formData / urlSearchParamsSlice rows to the "header wins even when it extracts to nothing" test. 94c861d trimmed the multi-line comments the comment-cop action flagged. Nothing else has changed.

Security risks

The MIME parser runs over untrusted Content-Type header bytes; I traced the token/quoted-string/whitespace predicates and the get_decode_split loop for bounds and termination and found no unbounded growth or index past end. Blob__setType validates its input with is_valid_blob_type (rejects anything outside U+0020–U+007E), so a caller-supplied contentType cannot inject CR/LF into an outgoing Content-Type header — the "a/b\rx: y" test pins this. No auth/crypto/permission surfaces are touched.

Level of scrutiny

High. This reworks spec-mandated behavior on the fetch/Body hot path, changes an FFI signature across four C++ files and one Rust extern block, adds owning fields to PendingValue/Action (with matching releases in Value::reset), removes Value::resolve's headers parameter at three call sites, and makes two user-visible API changes (the boundary prefix, and the new readableStreamToBlob parameter). It has been through several review rounds already (each of which surfaced a real issue that was fixed), CI has been green modulo tracked flakes, and the 641-case test file covers the header/body/stream/clone matrix against Node v26 — but the breadth of behavioral change and the API-surface additions put it outside what an automated review should approve alone.

Other factors

The finder-raised "Bun.file() body's store.mime_type still leaks through when the header extracts to nothing" was examined and refuted this run. All prior inline threads (the clone_utf8 leak, fast-path type shadowing, .body/clone() type loss, the SAFETY comment mismatch, the unused import) are resolved in the current diff.

@robobun

robobun commented Aug 13, 2026 •

Copy link
Copy Markdown
Collaborator Author

Self-review notes after folding #35901 / #32806 into this branch. Three things survived a closer look; none of them is introduced by the fold itself (the first one is pre-existing accessor behavior that the fold's "last valid value wins" claim runs into), but they affect what the description promises, so listing them here for whoever picks the branch up next.

  1. Bun.serve requests read uws' first Content-Type line, not the combined header list. Request::get_content_type (Request.rs, unchanged by this PR) returns req.header("content-type") while the uws request is still attached, and uws getHeader returns the first matching line (packages/bun-uws/src/HttpParser.h, getHeader). Both new consumers go through it: get_blob_content_type and get_form_data_encoding -> encoding_from_header. So for a request carrying boundary=B1 and boundary=B2 lines, request.formData() / request.blob() called synchronously (or after a microtask) in the handler see B1, while request.headers.get("content-type") returns the combined value and the same calls after a macrotask (uws request detached, FetchHeaders used) see B2; HTTP/3 never has the uws request and always sees B2. extract_mime_type is fed a single line on that path, so the get-decode-split step is a no-op there. The JS-constructed and fetch-client paths (what the tests cover) are fine. Options: have get_content_type read through the FetchHeaders when the uws request is attached (formData()/blob() always return promises, so a handler that awaits them goes through to_async, which builds the FetchHeaders anyway, i.e. no new allocation in practice), or keep the uws fast path and list this under "Not changed". Either way a Bun.serve test with two request Content-Type lines over a raw socket (sync call and post-macrotask call) would pin whichever is chosen.

  2. The body-derived fallback is only wired into blob(). get_blob_content_type adds the was_string / PendingValue::source_content_type fallback on top of get_content_type(), but get_form_data_encoding still calls the bare get_content_type(), which has no Locked arm. Observable split on a FormData/URLSearchParams body after r.body; const c = r.clone(): c.blob().type is the multipart type (pinned by "clone() after accessing .body keeps the type"), while c.formData() rejects with ERR_FORMDATA_PARSE_ERROR because the tee'd clone's get_content_type() sees Locked and returns None (Node parses it). Value::source_content_type() also re-evaluates the Blob arm that get_content_type() already handled one line earlier. Suggestion: put the body fallback in one place (a BodyMixin default over the inherent explicit-header lookup) and have both blob() and get_form_data_encoding use it, then get_blob_content_type becomes get_content_type().and_then(normalize_blob_type); add .body; clone().formData() for Request and Response. Related: the clone() path in Body.rs now stamps text/plain;charset=utf-8 onto the body blob, which get_or_create_headers / ensure_fetch_headers will then emit, so headers.get("content-type") of a cloned string body differs from the original's (null); worth a test either way.

  3. readableStreamToBlob skips tryUseReadableStreamBufferedFastPath whenever a contentType string is passed. That makes blob() the one consumer whose buffering path depends on the Content-Type header, and it moves the bundled node-fetch shim's blob() onto the JS chunk-array path: that shim touches .body before calling super.blob(), so it always goes through the ReadableStream branch in Body.rs, which now passes the derived type. Stamping the type onto the fast-path result with Blob__setType (the same helper the slow path uses) would keep the fast path for both cases and satisfy the existing tests, including "a blob-backed stream's own type never shadows the contentType".

Verified item 1 against the uws source and the accessor; items 2 and 3 against the current branch head (290d9de). Happy for any of these to be resolved as "documented, not changed" if that is the call, but the description currently reads as if all three paths agree.

@robobun

robobun commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

#42016 now carries the precedence and plumbing part of this change on current main: the Content-Type header wins over the body's own type on every path (in-memory, pending native, ReadableStream), Bun.serve requests see their header, and the shared-Store write is gone (#32801, #35284). It keeps MimeType::init for the value.

What remains unique to this PR is the WHATWG "extract a MIME type" parser and serializer, the File to plain Blob identity for blob(), and the multipart boundary handling. If those are still wanted they would rebase on top of #42016 as a much smaller diff; as it stands this branch conflicts with main since the August refactors.

@robobun

robobun commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

#41922 carries a narrower version of the Action::GetBlob / readableStreamToBlob(contentType) plumbing from this PR (the MIME type is read when blob() is called and applied on the buffered, pending and stream paths), without the mime_sniff parser, the File-identity change or the boundary change. If that lands first, this PR would rebase down to the spec-parsing and store-retyping parts.

@robobun

robobun commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #42016, which carries the precedence rule and the Action::GetBlob / readableStreamToBlob plumbing from this PR on current main: a present Content-Type header types blob() on every path (in-memory, pending native, ReadableStream, served requests) and the shared-Store write is gone, so it fixes #32801 and #35284. #41922 is stacked on it for the Bun.serve side (formData() and Range read through req.headers).

This branch has conflicted with main since the August refactors. What remains unique to it is the WHATWG "extract a MIME type" parser and serializer (#19603), the File to plain Blob identity for blob(), and the multipart boundary handling. Those are better re-proposed as a smaller PR on top of main once #42016 lands. The branch stays available for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

1 participant