Repository navigation
Conversation
|
Updated 12:37 AM PT - Jul 8th, 2026
❌ @robobun, your commit 2a64bde has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33605That installs a local version of the PR into your bun-33605 --bun |
|
Reproduced with: bun -e 'console.log(new Blob([], {type:"text/plain"}).type)' # text/plain;charset=utf-8
bun -e 'console.log(new Blob(["x"], {type:"text/plain"}).slice(0,1).type)' # text/plain;charset=utf-8
bun -e 'console.log(new Blob(["x"], {type:"a/b"}).slice(0,1).type)' # "" (inconsistent)All three now match Node. Tests in CI: the diff is green. Build #70275 fails only on |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the earlier feedback — I don't see any remaining issues, but since this changes user-visible Content-Type behavior across Blob/File/Bun.file/S3/serve (dropping the auto-appended ;charset=utf-8) and adds a vendored-test skip, a maintainer should sign off on the behavior change.
Extended reasoning...
Overview
This PR removes MIME-table canonicalization at every site where a user explicitly supplies a type option — new Blob({type}), new File({type}), blob.slice(_, _, type), Bun.file(path, {type}), Bun.file().write(data, {type}), s3file.writer({type}), and both S3File constructors — and stores the lowercased string verbatim instead. It also removes the parent-type inheritance branch from get_slice_from, so a typeless slice() now has type === "" per the File API spec. Six test files are updated to expect the verbatim type, a new describe("Blob type") block is added to blob.test.ts, and one vendored Elysia test is skipped via test/vendor.json.
Security risks
None identified. The change replaces a table lookup with a straight lowercase copy of a validated string; is_valid_blob_type still gates the input, and the allocation/ownership pattern (leaked Box<[u8]> with content_type_allocated = true, freed via free_content_type) is the same one already used by the fallthrough branch that this PR promotes to the only path. No new untrusted-input surface.
Level of scrutiny
High. This is a deliberate user-visible behavior change to Web-standard APIs with broad ripple effects: new Response(new Blob([...], {type: "text/html"})) now serves Content-Type: text/html (previously text/html;charset=utf-8), S3 uploads send the un-canonicalized type, multipart bodies emit the verbatim type on the wire, and slice() no longer inherits. The change is spec-correct and matches Node/browsers, but it's the kind of default-behavior shift a maintainer should explicitly approve — especially the Bun.serve static-route Content-Type change and the vendored Elysia test skip.
Other factors
- I previously left four inline comments on an earlier revision; all four were addressed in 7b4e100 and f7e8072 (S3 inspect test updated,
do_write/get_writerde-canonicalized, SAFETY comment tightened, FormData round-trip trace corrected by the author). All threads are resolved. - The
get_slice_fromchange also flipscontent_type_was_setfrom "inherit parent OR explicit" to "explicit only", which affects downstream consumers gated on that flag (get_mime_type_or_content_type,ByteBlobLoader,Body.rsstring-body defaulting). I traced the obvious ones and they look correct for in-memory blobs, but a maintainer familiar with theBun.file(...).slice()→ Response body path should confirm. - CI shows one failure (
node-net-server.test.tson Windows x64-baseline) that appears unrelated to this change. - Memory-management shape is unchanged — the removed branches were the ones that stored a borrowed
&'staticpointer withcontent_type_allocated = false; every remaining explicit-type path now heap-allocates and setscontent_type_allocated = true, whichfree_content_typeand the existing dupe/drop paths already handle.
There was a problem hiding this comment.
The earlier review feedback has been addressed and I found nothing new, but this is a user-visible behavior change to Blob.type/File.type that flows into served Content-Type headers (static routes, S3 uploads, FormData) and required updating six test files plus skipping a vendored Elysia test — worth a maintainer sign-off on the compat call.
Extended reasoning...
Overview
Removes MIME-table canonicalization at every site that accepts an explicit user-supplied type (Blob/File constructors, slice(), Bun.file(path,{type}), file.write(data,{type}), file.writer({type}), and both S3 construct_s3_file_* paths), and stops get_slice_from() from inheriting the parent's content_type on a typeless slice. Eight parallel hunks in Blob.rs/S3File.rs collapse the vm.mime_type() branch into a single lowercased heap copy; content_type_was_set on a slice is now driven by whether the caller supplied a non-empty type. Six test files are updated to expect the verbatim (non-;charset=utf-8) type, a new describe("Blob type") block is added, and an Elysia vendored test is skipped in test/vendor.json.
Security risks
None identified. The change narrows what goes into content_type (always a fresh lowercased heap copy or the static empty literal) and simplifies the ownership model — the removed static-mime-table pointer branch was the source of the odd "inherit only when non-allocated" behavior. No new untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Medium-high. The mechanical transformation is uniform and the memory-lifecycle change is a simplification (fewer pointer provenance cases, free_content_type() still called before reassignment, content_type_allocated correctly tracked). But the semantic change is user-visible across a lot of surface: new Response(new Blob([...], {type: 'text/html'})) now serves Content-Type: text/html instead of text/html;charset=utf-8, S3 uploads send the verbatim type, and typeless .slice() of a file/S3 blob no longer carries a type. That's spec-correct and matches Node, but it's a compat break that a maintainer should sign off on.
Other factors
All four of my prior inline comments were resolved in 7b4e100 (the do_write/get_writer sites, both S3File constructors, the S3 inspect test, and the stale SAFETY comment). The one comment robobun pushed back on (FormData round-trip at line 140) was correctly refuted — that path re-derives type from the filename extension. Test coverage for the new behavior is thorough (test.each matrix over Blob/File/slice + Bun.file construct/write, plus the negative slice-inheritance cases). No outstanding reviewer comments.
new Blob({type}), new File({type}), blob.slice(_, _, type),
Bun.file(path, {type}), Bun.file().write(data, {type}),
s3file.writer({type}), and both S3 client.file(path, {type}) paths
now store the user's type string lowercased instead of canonicalizing
through the MIME table, so new Blob([], {type: 'text/plain'}).type
is 'text/plain' rather than 'text/plain;charset=utf-8'.
blob.slice() with no/empty/invalid contentType now has type '' per
the File API spec instead of inheriting the parent's type when the
parent's type happened to be a static table entry. Bun-inferred
defaults (Bun.file extension sniffing, Response string bodies,
data URLs) are unchanged.
Fixes #15078
d8f2f6f to
1a67989
Compare
…wline
A typeless slice of a File- or S3-backed blob has an empty content_type
but has_content_type_from_user() still returns true (because of the
store kind), so blob_content_type() returned Some("") and fetch/serve
emitted an empty Content-Type header. Filter empty in headers_ref.rs so
the header is omitted, matching the Fetch 'extract a body' algorithm
for a Blob with an empty type. This path was already reachable on main
via Bun.file(path, {type: "custom/x"}).slice(); the previous commit
made the extension-sniffed case land here too.
Also: S3File::write_format only emitted the newline before offset: inside
the type: block, so an empty-type slice with a nonzero offset glued
offset: to the opening brace. Move the newline into the offset: block.
…t_type_opt doc get_slice() was short-circuiting on size==0 before reading the contentType argument, so new Blob([]).slice(0, 0, 'text/plain').type was '' instead of 'text/plain'. Drop the early return; get_slice_from() already handles a zero-size parent (relative_start/end both clamp to 0). Also update the any_blob_content_type_opt doc comment to match its siblings.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/webcore/Blob.rs:2441-2443— Nit (same #19603 family, distinct site): after this change,new Response(new Blob([...], {type: "text/html"}))served from a static route sendsContent-Type: text/html(this PR's own updated assertion at bun-serve-static.test.ts:211), but the same Response returned from a dynamicfetchhandler still sendstext/html;charset=utf-8—RequestContext::get_content_typeroutes the blob's now-verbatim"text/html"throughMimeType::by_name→MimeType::init→ theHTMLconstant. Before this PR the two paths agreed. Nothing regresses (dynamic-serve wire is unchanged), just flagging a siblingMimeType::initsite that now observably diverges from the paths this PR made verbatim.Extended reasoning...
What diverges
For
const r = new Response(new Blob(["<p>hi</p>"], {type: "text/html"}))(no init arg, soinit.headersstaysNone), theContent-Typeheader now depends on how the Response is served:Before PR After PR r.headers.get("content-type")(in-process)text/html;charset=utf-8text/htmlStatic route wire ( routes: {"/a": r})text/html;charset=utf-8text/htmlDynamic route wire ( fetch: () => r)text/html;charset=utf-8text/html;charset=utf-8Before this PR all three agreed; after, the dynamic-serve path disagrees with the other two.
Step-by-step trace
Blob construction — after this PR,
new Blob([...], {type: "text/html"})at Blob.rs:2441-2443 storescontent_type = Owned("text/html")verbatim (wasStatic("text/html;charset=utf-8")before, viavm.mime_type()).Static route —
StaticRoute::from_responsecallsany_blob_content_type(&blob)(headers_ref.rs:11-17), which returns the blob'scontent_type()verbatim →Some("text/html")→from_fetch_headerswritesContent-Type: text/html. Confirmed by this PR's own updated assertion at bun-serve-static.test.ts:211.Dynamic route —
render_metadata(RequestContext.rs) callsget_content_type(response.get_init_headers_mut(), &self.blob). With no init arg,get_init_headers_mut()returnsNone, so the headers branch is skipped and it falls through toby_name(blob.content_type())=MimeType::init("text/html", false, None). InMimeType::init,category_ == "text" && str == "html"matches and returns theHTMLconstant, whose.valueis"text/html;charset=utf-8".render_metadatathen writes that as thecontent-typeheader.In-process —
Response::get_or_create_headerswritesblob.content_type_slice()verbatim →"text/html", matching the static route but not the dynamic route.There's an additional wrinkle: for a dynamic route, if the caller reads
r.headersbefore returning (which materializes the verbatim"text/html"onto the Response),do_write_headerswrites that andneeds_content_type=falseskips theby_namefallback — so the same dynamic Response now servestext/htmlortext/html;charset=utf-8depending on whether.headerswas touched. Before this PR both cases produced the canonical form.Scope
Only affects the specific set
MimeType::initspecial-cases (text/plain,text/html,text/css,text/javascript,application/json,application/geo+json). Non-table types like"a/b"are unaffected becauseMimeType::initfalls through and preserves the input verbatim.Why not caught
serve.test.ts's "does propagate type for Blob" test passes the full canonical stringtype: "text/plain;charset=utf-8", soby_namereturns the same string and no change is observable. No test compares the same typed-Blob Response across static and dynamic registration.Why nit
The dynamic-serve wire behavior is unchanged from before this PR — no user regresses, no test breaks. The root cause is the pre-existing
by_name()canonicalization inRequestContext::get_content_type, which is the same #19603 /MimeType::initfamily the PR description explicitly scopes out. This is a distinct code site from the already-flaggedBody.rs:2173nit (that one is.blob()'s return type; this one is the served header), worth noting per "fix the whole class" as another parallel path that now observably disagrees with the paths this PR made verbatim.Possible fix
Either fold into the #19603 follow-up, or have
RequestContext::get_content_typewriteblob.content_type()verbatim into the header (usingMimeType::initonly to derive the.categoryneeded byautoset_filename) so static/dynamic/.headersagree.
…type RequestContext::get_content_type reads blob.content_type() directly (not through headers_ref.rs), so a typeless slice of Bun.file() fell through to MimeType::OTHER and the response got Content-Type: application/octet-stream plus a Content-Disposition header. Fall back to content_type_or_mime_type() so the sliced file is served with the same extension-sniffed type as the unsliced file (which is what the pre-existing TODO at that site was asking about).
Mirror RequestContext::get_content_type so a static route registered with new Response(Bun.file(p).slice(a, b)) serves the same Content-Type as the dynamic handler path and the unsliced file. Inline the fallback at the FileRoute call site (content_type_or_mime_type) and drop the now-unused blob_content_type helper; headers_ref.rs stays spec-shaped for the fetch-client body path. Extend the serve test to cover both the static and dynamic route.
There was a problem hiding this comment.
Thanks — 2a64bde addresses the last FileRoute sibling and everything I flagged is now resolved. I'm not auto-approving only because this is a user-visible Content-Type behavior change (six test files updated, one vendored test skipped) with a couple of intentional scope boundaries (the #19603 Body.rs/by_name follow-ups, dynamic-serve still canonicalizing while static/in-process now verbatim) that a maintainer should confirm.
Extended reasoning...
Overview
Removes MIME-table canonicalization at eight explicit-type call sites in Blob.rs/S3File.rs so new Blob({type})/new File({type})/slice(_,_,type)/Bun.file(path,{type})/.write({type})/s3.file({type}) store the user's string verbatim (lowercased), and drops the Static-only parent-type inheritance in get_slice_from so a typeless slice() has type === "" per the File API spec. Follow-on adjustments in headers_ref.rs (filter empty), RequestContext::get_content_type and FileRoute::headers_from (fall back to the File/S3 store's extension-sniffed mime for typeless file slices), and S3File::write_format (offset-line newline). Six test files updated to expect verbatim types; one Elysia vendored test skipped.
Security risks
None identified. No auth/crypto/permissions surface. The changes narrow what user-supplied strings are transformed (less rewriting, not more), and validation (is_valid_blob_type) is unchanged.
Level of scrutiny
Medium-high. The core transformation is mechanical, but the ripple into HTTP Content-Type emission across three separate serve/fetch consumers took four review rounds to converge (empty header, dynamic-handler octet-stream+Content-Disposition, static-route sibling). Each was fixed with a targeted change and a covering test. The size == 0 early-return removal and the new Any::content_type_or_mime_type helper are small but non-trivial.
Other factors
- All ten of my prior inline threads are resolved (nine addressed by code, one correctly refuted).
- New tests cover the spec matrix (Blob/File/slice × table/non-table types, empty parent, fetch body header omission, dynamic + static serve fallback,
.write({type})). - The PR description explicitly documents the remaining #19603-family sites (
Body.rs.blob(),RequestContextby_name, data-URL) as out of scope, and the resulting static-vs-dynamic-serve canonicalization difference. Those scope boundaries are reasonable but are the kind of judgment call a maintainer should confirm. - CI is reported green on the diff (only an unrelated darwin artifact-download infra flake).
|
This also fixes the Blob/File constructor half of #34717 (explicit |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-08, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#15078) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
new Blob({type}),new File({type}),blob.slice(_, _, type),Bun.file(path, {type}),Bun.file().write(data, {type}),s3file.writer({type}), and the S3client.file(path, {type})paths were canonicalizing an explicit user-suppliedtypethrough Bun's MIME table and, as a side effect of how the canonicalized value was stored, a typelessslice()of such a blob inherited the parent's type instead of being empty.The inheritance was inconsistent: whether
blob.slice(0, 1).typewas empty depended on whether the parent's type string happened to be in Bun's static MIME table.Why
When a user's
typematched a table entry,vm.mime_type()returned the canonicalMimeType(e.g.text/plain→text/plain;charset=utf-8) and the blob stored it asBlobContentType::Static.get_slice_from()then copied the parent'scontent_typeonto a typeless slice specifically when it wasStatic, so a table-matched parent bled its type into the slice while an unrecognised parent did not.Fix
typenow callsBlobContentType::from_lowercased(slice)directly instead of going through the MIME table, matching the File API spec and Node.get_slice_from()no longer inherits the parent'scontent_type; a slice's type is exactly the caller-suppliedcontentType(or""if omitted/empty/invalid) per the slice algorithm. Thesize == 0early return is dropped sonew Blob([]).slice(0, 0, type)also honors the caller's type.content_type:headers_ref.rsfilters empty so a typeless slice body omits theContent-Typeheader rather than sending an empty value;RequestContext::get_content_typefalls back to the File/S3 store's extension-sniffedmime_typesonew Response(Bun.file(p).slice(a, b))from a dynamic handler keeps the sameContent-Typeas the unsliced file (no spuriousContent-Disposition);S3File::write_formatemits theoffset:line correctly when the type line is absent.Bun-inferred defaults are unchanged:
Bun.file("x.txt").typeis still"text/plain;charset=utf-8"(extension-sniffed),new Response("s")still defaults totext/plain;charset=utf-8, and data-URL handling still canonicalizes.Rebased onto #33599 (the
BlobContentTypeenum refactor), which reduced the source change to dropping thevm.mime_type()match at eight call sites plus the slice-inheritance block.Verification
Six existing test files that asserted the canonicalized type are updated to expect the verbatim type; in each case the test's actual invariant (clone preserves type, serve is stable across registrations, multipart round-trips, S3 inspect shows the slice's own type) is preserved. One vendored Elysia test that asserts the old behavior is skipped via
test/vendor.json.Fixes #15078
Related: #19603 is the same family but goes through
MimeType::init/by_name, which this PR does not touch. Remaining sites that still canonicalize for a #19603 follow-up:Body.rsres.blob()path (MimeType::initcopying a responseContent-Typeonto the returned blob), and its!content_type_was_set && store.is_some()heuristic at line 2173 that stampstext/plain;charset=utf-8onto.blob()of an untyped in-memory body (now also reachable fromnew Response(typedBlob.slice()), where it was previously masked by the inherited flag).RequestContext::get_content_type'sby_namecall, so a dynamicfetchhandler returningnew Response(new Blob([...], {type: "text/html"}))still servestext/html;charset=utf-8while the static route and in-process.headersnow reporttext/htmlverbatim. The dynamic-serve wire behavior is unchanged from before this PR.fetch.rs::data_url_response(MimeType::initon the data URL's media type).