fetch: send text/plain Content-Type for string request bodies#33677
fetch: send text/plain Content-Type for string request bodies#33677robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 10 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 (7)
WalkthroughThis PR changes how implicit ChangesImplicit Content-Type derivation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:05 AM PT - Jul 8th, 2026
❌ @robobun, your commit 911b2a6 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33677That installs a local version of the PR into your bun-33677 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/Request.rs (1)
1522-1539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the deferred-header case is actually covered end-to-end.
This block only writes
Content-Typewhenreq.headers.get().is_some()— i.e., headers were already materialized earlier inconstruct_into(from an explicitheadersinit or a cloned Request/Response). When headers are stillNonehere, the implicit content type is deferred toensure_fetch_headers(lines 250-301), which duplicates this same "only if not already present" logic via a different code path (raw match on body variants vs.content_type()). This is not a bug per se, but it's the second near-duplicate implementation of "derive implicit Content-Type from body and insert if absent" in this file. Worth consolidating into one helper both call, to avoid the two diverging later.🤖 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/Request.rs` around lines 1522 - 1539, The deferred-header path and the materialized-header path both implement the same implicit Content-Type insertion logic in Request::construct_into and ensure_fetch_headers, so the handling is duplicated and can drift. Extract the shared “derive Content-Type from body and insert if absent” behavior into one helper and have both code paths call it, preserving the existing borrowck-safe handling around req.headers_mut() and body_value().content_type().
🤖 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/Body.rs`:
- Around line 720-735: String body content-type values are hardcoded with the
wrong charset casing, so update both `Body::content_type()` and
`Internal::content_type()` to return the fetch-spec/browser wire value
`text/plain;charset=UTF-8` for string bodies. Make the change in the string-body
paths (`Value::WTFStringImpl` and the corresponding `Internal` logic) together
so exact header comparisons stay consistent.
---
Outside diff comments:
In `@src/runtime/webcore/Request.rs`:
- Around line 1522-1539: The deferred-header path and the materialized-header
path both implement the same implicit Content-Type insertion logic in
Request::construct_into and ensure_fetch_headers, so the handling is duplicated
and can drift. Extract the shared “derive Content-Type from body and insert if
absent” behavior into one helper and have both code paths call it, preserving
the existing borrowck-safe handling around req.headers_mut() and
body_value().content_type().
🪄 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: e6b25484-3ba1-4121-9eea-99d5b7f6bc8c
📒 Files selected for processing (7)
src/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rstest/js/bun/util/inspect.test.jstest/js/web/fetch/fetch.test.tstest/vendor.json
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runtime/webcore/Body.rs (1)
725-735: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the string content type from
MimeType::TEXTinstead of a duplicated literal.Line 731 hardcodes
b"text/plain;charset=utf-8", while the sibling fix inclone_with_readable_stream(lines 1649-1651) sources the same value frombun_http_types::MimeType::TEXT.value.as_ref(). Keeping two literal copies of this spec string invites drift if the constant ever changes.♻️ Proposed fix
- Value::WTFStringImpl(_) => Some(b"text/plain;charset=utf-8"), + Value::WTFStringImpl(_) => Some(bun_http_types::MimeType::TEXT.value.as_ref()),🤖 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/Body.rs` around lines 725 - 735, The Body::content_type match arm for Value::WTFStringImpl currently hardcodes the text/plain charset string, creating a duplicated MIME literal. Update that branch to source the value from bun_http_types::MimeType::TEXT.value.as_ref() just like clone_with_readable_stream does, so the string content type stays centralized and consistent.test/js/web/fetch/fetch.test.ts (1)
2899-2946: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSocket errors are swallowed instead of rejecting the awaited promise.
socket.on("error", () => {})discards failures silently, so a connection failure leavesnextHead.promisepending until the suite's default timeout instead of failing fast with a clear error. Also note the connection closure reads the outer, reassignablenextHead/lastHeadvariables rather than capturing them per-connection — safe today only because calls are sequential and no error/close handling exists yet; adding rejection naively on the shared variable risks a stale connection rejecting a later call's promise.🔧 Proposed fix
await using server = net.createServer(socket => { + const resolver = nextHead; let buf = Buffer.alloc(0); socket.on("data", d => { buf = Buffer.concat([buf, d]); if (buf.indexOf("\r\n\r\n") < 0) return; lastHead = buf.toString("latin1").split("\r\n\r\n")[0]; socket.end("HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); - nextHead.resolve(); + resolver.resolve(); }); - socket.on("error", () => {}); + socket.on("error", err => resolver.reject(err)); });As per coding guidelines, "Wire EVERY failure event (
error,close,abort, process exit) to reject the awaited promise — never throw inside event callbacks."🤖 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 `@test/js/web/fetch/fetch.test.ts` around lines 2899 - 2946, The fetch socket test currently swallows connection failures because socket.on("error", () => {}) ignores errors and leaves doFetch waiting on nextHead.promise until timeout. Update the server callback in fetch.test.ts so each connection captures its own resolver state and wires socket error/close (and any other failure path used by the test) to reject the current Promise.withResolvers instance instead of relying on shared outer nextHead/lastHead variables. Keep the existing assertions in doFetch and contentTypeHeader, but make the awaited promise fail fast with the actual socket failure for the specific request being tested.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/Body.rs`:
- Around line 725-735: The Body::content_type match arm for Value::WTFStringImpl
currently hardcodes the text/plain charset string, creating a duplicated MIME
literal. Update that branch to source the value from
bun_http_types::MimeType::TEXT.value.as_ref() just like
clone_with_readable_stream does, so the string content type stays centralized
and consistent.
In `@test/js/web/fetch/fetch.test.ts`:
- Around line 2899-2946: The fetch socket test currently swallows connection
failures because socket.on("error", () => {}) ignores errors and leaves doFetch
waiting on nextHead.promise until timeout. Update the server callback in
fetch.test.ts so each connection captures its own resolver state and wires
socket error/close (and any other failure path used by the test) to reject the
current Promise.withResolvers instance instead of relying on shared outer
nextHead/lastHead variables. Keep the existing assertions in doFetch and
contentTypeHeader, but make the awaited promise fail fast with the actual socket
failure for the specific request being tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9df9d7dd-718d-4892-bad3-7d14f2c88887
📒 Files selected for processing (2)
src/runtime/webcore/Body.rstest/js/web/fetch/fetch.test.ts
|
Addressing the two outside-diff review notes:
|
|
CI status: the diff is green on every lane that exercises it. Build 70344 (first rebase onto e10d444) completed 285 passed / 1 failed on Build 70491 (second rebase onto 332f744): the only hard failure is The pre-rebase builds 69953/69991/70016 each hit The one failure this diff did cause ( All 11 new tests ( |
robobun
force-pushed
the
farm/08ef360e/fetch-string-body-content-type
branch
from
166a2f2 to
5a59f26
Compare
There was a problem hiding this comment.
All prior review threads are resolved and I found nothing new, but this is a user-visible Web-compat behavior change across the Request/Response/fetch hot paths (plus a deliberate O(M) clone trade-off for non-ASCII string bodies and a vendor-test skip), so a maintainer should sign off.
Extended reasoning...
Overview
This PR makes string request/response bodies emit Content-Type: text/plain;charset=utf-8 per the WHATWG fetch spec's "extract a body" step, matching Node/undici, Deno, and browsers. It touches four core webcore Rust files (Blob.rs, Body.rs, Request.rs, Response.rs) — adding body::Value::content_type(), updating Any::has_content_type_from_user(), rewiring six header-derivation sites in Request/Response, reworking construct_json to avoid the string body's implied type shadowing application/json, and changing clone_with_readable_stream to preserve was_string through .clone(). Tests, an inspect snapshot update, and a vendor-test skip round it out.
Security risks
None. No auth/crypto/permissions surface. The change adds a header value derived from a compile-time constant; no user-controlled bytes reach the header emission path beyond what already did.
Level of scrutiny
High. This is a behavioral change to production-critical Web API surface (fetch, new Request, new Response, Response.json) that every Bun.serve handler and HTTP client touches. It changes what goes on the wire for the most common body type. The change went through four rounds of my own review with substantive reworks (the .clone() fix, then its over-correction on .blob().type, then the current InternalBlob-preserving approach). The final shape carries a deliberate perf trade-off — non-ASCII string bodies now deep-copy on .clone() instead of sharing a refcounted store — which the author has documented and tied to #33128 as the path back to O(1). That trade-off, the two acknowledged deferred follow-ups (empty-string bodies, consume-before-headers order-dependence via #32913), and the elysia vendor-test skip are all reasonable calls, but they're the kind of calls a maintainer should ratify rather than a bot.
Other factors
Test coverage is thorough (wire-level TCP capture, Request/Response .headers, .clone() on both original and clone, .blob().type non-shadowing guards, ArrayBuffer negative controls). CI is reported green on the lanes that exercise the diff. No outstanding unresolved review threads. The lowercase charset=utf-8 casing choice is intentional and consistent with MimeType::TEXT repo-wide. Given the scope and the number of interacting code paths (lazy vs eager header materialization, clone normalization, .blob() precedence, Response.json), this warrants a human look before merge.
fetch(url, {method: 'POST', body: '<string>'}) was sending the request with
no Content-Type header on the wire, while node/undici, deno, and browsers
all send the spec-mandated text/plain;charset=UTF-8. The same omission made
new Request(...).headers.get('content-type') and new Response('...').headers
return null for string bodies.
The fetch spec's 'extract a body' step assigns text/plain;charset=UTF-8 to
scalar-value-string bodies just as it assigns the urlencoded/multipart types
to URLSearchParams/FormData bodies. Bun's header-building paths gated the
body-derived Content-Type on has_content_type_from_user(), which returned
false for the WTFStringImpl / InternalBlob{was_string} variants a string
body becomes, so nothing was emitted.
Add body::Value::content_type() returning the spec-assigned type for each
body variant and use it at every site that copies the body's type into the
header list (Request construct_into / ensure_fetch_headers / get_content_type,
Response constructor / get_or_create_headers / get_content_type). Update
AnyBlob::has_content_type_from_user() so the fetch wire serializer and
headers_ref helpers see string bodies as having a type. Response.json() now
materializes its headers before the body-derived type is considered so the
JSON type is not overwritten by text/plain.
ArrayBuffer / typed-array bodies (InternalBlob{was_string: false}) keep
returning no type, matching the spec.
elysia 1.4.28's 'automatically handle HEAD request for GET dynamic path'
asserts response.headers.toJSON() is exactly {content-length: '11'} for a
handler returning a plain string. new Response('string').headers now exposes
content-type: text/plain;charset=utf-8 per the fetch spec, matching Node and
browsers, so the assertion sees an extra key.
Value::clone_with_readable_stream converts a non-ASCII string body
(WTFStringImpl -> InternalBlob{was_string:true} -> Blob) without carrying
the was_string bit forward, so the resulting Blob has an empty content_type
and Value::content_type() returns None for both the clone and the mutated
original. Set the Blob's content_type to MimeType::TEXT when was_string,
matching create_with_bytes_and_allocator; dupe_with_content_type copies it
verbatim to the clone.
robobun
force-pushed
the
farm/08ef360e/fetch-string-body-content-type
branch
from
5a59f26 to
911b2a6
Compare
1 participant
Problem
fetch(url, {method: "POST", body: "<string>"})sends the request with noContent-Typeheader on the wire. node/undici, deno, and browsers all sendtext/plain;charset=UTF-8for the identical call, as the WHATWG fetch spec's "extract a body" step mandates for scalar-value-string bodies.The same omission made
new Request("http://x/", {method:"POST", body:"hello"}).headers.get("content-type")andnew Response("hello").headers.get("content-type")both returnnull.Servers that dispatch their body parser on
Content-Type(express.text(), Rails, many WAFs and API gateways) see an untyped POST from bun: bodies silently not parsed, or rejected with 415.Cause
A string body is stored as
body::Value::WTFStringImpl(orInternalBlob { was_string: true }after UTF-8 conversion).AnyBlob::content_type()already returnstext/plain;charset=utf-8for these, but every site that copies the body's type into the header list is gated one of two ways:AnyBlob::has_content_type_from_user(), which returnedfalsefor both string variants;RequestandResponseconstructor / lazy-headers paths match onlyBodyValue::Blob(_).So the string body's implicit type was never written to the header list. URLSearchParams and FormData bodies already worked because they are stored as
Value::Blobwithcontent_type_was_set.Fix
Add
body::Value::content_type()returning the type the spec assigns to each body variant: aBlob's explicit content type if set,text/plain;charset=utf-8forWTFStringImplandInternalBlob { was_string: true }, andNonefor ArrayBuffer / ReadableStream / null bodies (which the spec assigns no type). Use it at every site that copies the body's type into the header list:Request::construct_into/ensure_fetch_headers/get_content_typeResponse::constructor/get_or_create_headers/get_content_typeUpdate
AnyBlob::has_content_type_from_user()to returntrueforWTFStringImplandInternalBlob { was_string: true }so the fetch wire serializer (from_fetch_headersviaany_blob_content_type_opt) and theheaders_refhelpers see string bodies as having a type.InternalBlob { was_string: false }(ArrayBuffer / typed-array bodies) keeps returningfalse, so those still correctly get noContent-Type.Response.json()previously routed throughget_or_create_headers()and thenput_default(ContentType, json). With the change above a fresh header list would first receive the string body'stext/plain, making theput_defaulta no-op.construct_jsonnow materializesinit.headersitself and writes the JSON type before the body-derived type is considered.Bun's existing value
text/plain;charset=utf-8(lowercase) is used, matching what Bun already emits on the response wire for string bodies and on the request wire forBun.file()bodies.Verification
New tests in
test/js/web/fetch/fetch.test.tsunder"Content-Type implied by the body init per fetch spec":fetch()with a string body putstext/plain;charset=utf-8on the wire (captured by a raw TCP server), both with no headers and with unrelated headers; a non-ASCII string body exercises theInternalBlob { was_string }path; an explicitcontent-typeheader wins; URLSearchParams still sends urlencoded; ArrayBuffer / Uint8Array bodies send noContent-Type.fetch(new Request(...))with a string body sends it on the wire.new Request(...)/new Response(...)with a string, non-ASCII string, or URLSearchParams body expose the type on.headers; an explicit header wins; ArrayBuffer / typed-array bodies do not get a type.Against unmodified main the four string-body tests fail and the URLSearchParams / ArrayBuffer control tests pass; with the fix all six pass.
test/js/bun/util/inspect.test.jsis updated for thenew Response("Hello")headers now includingcontent-type, matching Node.body.test.ts,bun-serve-static.test.ts,fetch-file-upload.test.ts,FormData.test.ts, and theResponse.jsontests all pass unchanged.#32913 touches adjacent lines for a different issue (body-read-before-headers ordering for FormData/URLSearchParams/typed Blob) and explicitly left plain strings alone; this PR addresses the string case.
Fixes #17085
Fixes #8530