Bun.serve: treat empty header values as absent so auto Content-Type/Date are not duplicated - #35336
Bun.serve: treat empty header values as absent so auto Content-Type/Date are not duplicated#35336robobun wants to merge 7 commits into
Conversation
…ate are not duplicated An empty or whitespace-only Content-Type on a Response was written to the wire as 'Content-Type: ' and, because FetchHeaders.fast_get treats a zero-length value as not present, render_metadata then appended the auto-derived 'content-type: text/plain;charset=utf-8' as a second line. The Date case inverted: the commonHeaders loop set the wrote-date flag on key presence and emitted 'Date: ', so the auto Date was suppressed and only the empty line remained. Skip empty-valued common headers in writeFetchHeadersToUWSResponse and its H3 sibling before any wrote-this-header state is touched. FetchHeaders already normalises leading/trailing HTTP whitespace, so a single isEmpty() check covers both '' and whitespace-only.
WalkthroughChangesEmpty header values are now treated as absent when applying response defaults and are skipped during HTTP/1, HTTP/3, file, and static response emission. Raw socket tests cover duplicate prevention, header omission, preserved values, and ETag generation. Empty header semantics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:12 PM PT - Jul 23rd, 2026
❌ @robobun, your commit 9976fc8 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35336That installs a local version of the PR into your bun-35336 --bun |
|
CI: the only hard failure on the latest build (79129, at 9976fc8) is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
StaticRoute and FileRoute snapshot the Response's FetchHeaders through from_fetch_headers/copyTo and replay the snapshot per request via their own write loops, bypassing writeFetchHeadersToUWSResponse. An empty user Date there still marked the wrote-date state and wrote 'Date: ' with the auto-Date suppressed; an empty Content-Type was written verbatim. - FetchHeaders::put_default now gates on fast_get().is_some() (empty is None) instead of fast_has, so Response.json and StaticRoute's was_string path overwrite an empty Content-Type with the real default. - StaticRoute::do_write_headers and FileRoute::write_headers skip zero-length values. - has_date / has_date_header (and the other FileRoute presence bits) treat an empty value as absent so the auto-header fires.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/js/bun/http/bun-serve-headers.test.ts`:
- Around line 140-146: Add sibling FileRoute tests alongside “file route: date
empty” for empty and whitespace-only Last-Modified, Content-Length, and
Content-Range headers, including a Range request for Content-Range. Assert each
response uses the corresponding automatic fallback and covers every changed
FileRoute presence-flag state.
- Around line 21-28: Update both raw-head socket helpers in
test/js/bun/http/bun-serve-headers.test.ts at lines 21-28 and 94-101 so their
awaited response-read promises reject when socket emits an error, rather than
relying on the no-op error listener. Preserve data accumulation and close
resolution, wiring the socket failure event to promise rejection at both sites.
🪄 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: be00d320-7492-4f29-bb1a-d00c06e1b0d7
📒 Files selected for processing (5)
src/jsc/FetchHeaders.rssrc/jsc/bindings/NodeHTTP.cppsrc/runtime/server/FileRoute.rssrc/runtime/server/StaticRoute.rstest/js/bun/http/bun-serve-headers.test.ts
The snapshot that StaticRoute/FileRoute replay carries no common/uncommon distinction (copyTo flattens both), so its value.length == 0 skip applies to every entry. Apply the same skip to the uncommon loop in writeFetchHeadersToUWSResponse and the H3 variant so an empty x-custom is dropped on every Bun.serve response path rather than only on static routes.
There was a problem hiding this comment.
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 `@test/js/bun/http/bun-serve-headers.test.ts`:
- Around line 149-157: The test named “empty custom header is dropped on both
paths” currently omits the file-backed FileRoute path. Extend its response
matrix using the existing temporary-file setup and FileRoute writer so it
exercises an empty “x-custom” header, then assert that header is absent while
preserving the existing content-type assertion across all paths.
🪄 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: 34b88a7f-9d64-4236-a5ea-acbbb790f05a
📒 Files selected for processing (3)
src/jsc/bindings/NodeHTTP.cppsrc/runtime/server/FileRoute.rstest/js/bun/http/bun-serve-headers.test.ts
…ing siblings FetchHeaders::put_default is called at Response construction time by Response.json, so overwriting an empty Content-Type there is a JS-observable Fetch-spec deviation (the spec's 'header list contains' is key-presence; Node/Chrome/Firefox leave the empty value). Revert it to fast_has and move the empty-as-absent check to the Bun-specific StaticRoute was_string call site, where mutating the registered Response is acceptable. Close the remaining serializer-layer siblings: - getSetCookieHeaders() loops in both writeFetchHeadersTo* variants: skip empty values so the dynamic and static paths agree. - StaticRoute etag gate: treat empty as absent so the auto content-hash ETag is still generated. - RequestContext render_metadata / user_handles_range: gate has_content_disposition / has_content_range on fast_get().is_some() so an empty user value does not both suppress the auto-header and be dropped by the serializer (a 206 must carry Content-Range).
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)
test/js/bun/http/bun-serve-headers.test.ts (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
test.eachfor the content-type matrix.This manually generated matrix should follow the repository’s Bun test convention so each variant remains a first-class, independently named test.
Proposed refactor
- for (const [label, value] of [ + test.each([ ["empty", ""], ["whitespace", " \t "], - ] as const) { - test(`content-type: ${label}`, async () => { + ] as const)("content-type: %s", async (label, value) => { const head = await rawHead(() => new Response("x", { headers: { "content-type": value } })); const ct = lines(head, "content-type"); expect(ct).toHaveLength(1); + });As per coding guidelines, tests should use
test.eachfor matrices.🤖 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/bun/http/bun-serve-headers.test.ts` around lines 41 - 45, Refactor the content-type test matrix in the surrounding test block to use Bun’s test.each convention instead of manually iterating with a for loop. Preserve the existing “empty” and “whitespace” labels, values, assertions, and independently named test cases.Source: Coding guidelines
src/jsc/bindings/NodeHTTP.cpp (1)
1539-1572: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd HTTP/3 coverage for the empty-value header branches.
serve-http3.test.tsexercises Date/Alt-Svc, but it does not drivewriteFetchHeadersToH3Responsethrough the emptySet-Cookie, common, and uncommon header paths. Add a focused H3 case for those empty values.🤖 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/jsc/bindings/NodeHTTP.cpp` around lines 1539 - 1572, The HTTP/3 tests lack coverage for empty header values in writeFetchHeadersToH3Response. Add a focused case in serve-http3.test.ts that sends empty Set-Cookie, common, and uncommon headers, then verifies the response handles them without writing those headers while preserving normal H3 behavior.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/jsc/bindings/NodeHTTP.cpp`:
- Around line 1539-1572: The HTTP/3 tests lack coverage for empty header values
in writeFetchHeadersToH3Response. Add a focused case in serve-http3.test.ts that
sends empty Set-Cookie, common, and uncommon headers, then verifies the response
handles them without writing those headers while preserving normal H3 behavior.
In `@test/js/bun/http/bun-serve-headers.test.ts`:
- Around line 41-45: Refactor the content-type test matrix in the surrounding
test block to use Bun’s test.each convention instead of manually iterating with
a for loop. Preserve the existing “empty” and “whitespace” labels, values,
assertions, and independently named test cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6749658-a432-4c0f-b049-1435570c5133
📒 Files selected for processing (4)
src/jsc/bindings/NodeHTTP.cppsrc/runtime/server/RequestContext.rssrc/runtime/server/StaticRoute.rstest/js/bun/http/bun-serve-headers.test.ts
append_etag pushes a second entry via Headers::append, so an empty user etag plus the auto content-hash leaves two snapshot entries. do_write_headers skips the empty one so the wire looks right, but render_precondition reads headers.get(b"etag") (first match, the empty one) and filters it to None, so the served tag is never honoured for If-None-Match/If-Match. Keep the original .is_none() gate: an empty user etag is dropped by the write loop and the route simply has no ETag, which is valid and consistent.
There was a problem hiding this comment.
No new issues found after 9976fc8, but this touches the hot Bun.serve response-header serialization path across four writers (uWS/H3/StaticRoute/FileRoute) and makes a user-observable design choice — dropping all empty-valued headers on the wire, not just the auto-header ones — so a maintainer should sign off on the semantics.
What was reviewed: the six isEmpty()/value.length == 0 skips are placed before any state bits are set, so auto Date/Content-Length/TE flags stay unset for empty values; the fast_has → fast_get(...).is_some() swap in render_metadata/user_handles_range was traced through the 206/Content-Range logic and now correctly falls back to the auto Content-Range instead of emitting a 206 with none; the reverted etag gate keeps render_precondition consistent with the wire; the was_string StaticRoute override is scoped to the Bun-specific static: API and Response.json's spec-observable Headers are unchanged (test asserts ""); node:http's flat-array path is untouched.
Extended reasoning...
Overview
The PR fixes an RFC 9110 violation in Bun.serve: a user-supplied empty header value (content-type: "", date: "", whitespace-only) was written to the wire and the matching auto-header was appended (or, for Date, the empty line suppressed the auto-Date entirely). The fix treats empty values as absent at each of the three response-header serializers — writeFetchHeadersToUWSResponse / writeFetchHeadersToH3Response (NodeHTTP.cpp), StaticRoute::do_write_headers, and FileRoute::write_headers — and converts the presence checks that gate each auto-header (has_date, has_content_range, has_content_disposition, the four FileRoute has_* bits, and user_handles_range) from key-presence to value-non-empty. 22 new raw-socket test cases cover the dynamic/static/file paths, "" and whitespace-only, headers.set, Response.json, custom headers, Set-Cookie, and non-empty preservation.
Security risks
None identified. This is output-side header serialization; the empty-value skip cannot inject or reorder bytes, and no untrusted input is parsed differently. The Content-Range change avoids emitting a 206 without a Content-Range header (which the pre-PR code combined with the new serializer skip would have produced).
Level of scrutiny
High. Bun.serve's header writer is a per-request hot path with wide blast radius, and the change is a user-observable behavioral shift: after 3d70b96/a0fef71f, every empty-valued header — including custom ones and Set-Cookie — is dropped from the wire, not just the ones that collide with an auto-header. That was a deliberate consistency decision (dynamic and static/file paths now agree) reached during review, but it is a design call a maintainer should ratify. The PR went through five fix commits addressing sibling sites, a Fetch-spec deviation in put_default (reverted), a missed Set-Cookie loop, and an ETag/precondition regression (reverted in 9976fc8) — each round exposed a subtle interaction, which is exactly the profile that benefits from a human read.
Other factors
All seven prior inline findings (from me and CodeRabbit) are resolved. The final diff keeps put_default on fast_has (spec-compliant Response.json Headers), scopes the empty-CT overwrite to the Bun-specific StaticRoute was_string registration path, and leaves the etag gate on .is_none() so render_precondition and the wire agree. The remaining known gap — a Blob-body static route with content-type: "" ends up with no CT rather than the blob's type — was explicitly scoped out (touching from_fetch_headers also affects the outgoing fetch() client) and is strictly better than the pre-PR lone empty line. Test coverage is thorough and follows the harness conventions (port: 0, using, tempDir, error wired to reject).
2 participants
Repro
RFC 9110 forbids duplicate
Content-Type. TheDatecase inverts:{ date: "" }emits a lone emptyDate:and suppresses the auto Date, so the origin sends no valid Date at all. Same for" "/"\t",headers.set("content-type", ""),Response.json(v, { headers: { "content-type": "" } }), and the static/file-route siblings (static: { "/": new Response("x", { headers: { date: "" } }) }).Cause
Two serializers write a
FetchHeadersto the wire, and both let an empty value through while the presence check that gates the matching auto-header disagrees about whether empty means present:writeFetchHeadersToUWSResponse,src/jsc/bindings/NodeHTTP.cpp): writes every header unconditionally and sets wrote-this-header state as it goes.render_metadatadecides whether to append the autocontent-type:viaFetchHeaders::fast_get, which returnsNonefor a zero-length value, so forcontent-type: ""the serializer writes the empty line and the auto value is appended as a second line. The Date check is key-presence, so the empty entry setsHTTP_WROTE_DATE_HEADERand only the empty line remains.StaticRoute::do_write_headers,FileRoute::write_headers): snapshot theFetchHeadersviafrom_fetch_headers/copyToat registration time and replay the snapshot per request. The snapshot preserves empty entries,has_dateisheaders.get(b"date").is_some()(true forb""), and the replay loop writes every entry, so an emptyDate:is written with the auto-Date suppressed.Fix
Treat an empty value as absent at each serializer-layer site.
FetchHeadersalready strips leading/trailing HTTP whitespace, soisEmpty()/len == 0covers whitespace-only too.writeFetchHeadersToUWSResponseand its HTTP/3 sibling:continuepast empty values in all three loops (Set-Cookie, common, uncommon) before any state bit is touched.StaticRoute::do_write_headers,FileRoute::write_headers: skip zero-length values in the replay loop.StaticRoute/FileRoutehas_date*and the other header-presence bits:.is_some_and(|v| !v.is_empty()).render_metadata'shas_content_disposition/has_content_rangeand theuser_handles_rangecheck: gate onfast_get(...).is_some()so an empty user value does not both suppress the auto-header and be dropped by the serializer (a 206 must carryContent-Range).text/plainat the Bun-specificstatic:registration call site rather than insideput_default, soResponse.json(v, { headers: { "content-type": "" } }).headers.get("content-type")stays""per the Fetch spec.node:http'ssetHeader("content-type", "")still writes the single empty line it does today; that path serializes through the flat name/value array rather thanFetchHeadersand is unchanged.After:
Verification
test/js/bun/http/bun-serve-headers.test.tsgains a describe block that reads the raw response head over a plain socket and asserts exactly one non-emptycontent-type/dateline for""and whitespace-only values, via the init dict,headers.set,Response.json, a static route, and aBun.file()route; that an empty custom header and an emptyset-cookieare dropped on both the dynamic and static paths; that an emptyetagon a static route is dropped rather than written as an empty line; and that non-empty user values are unchanged. TheResponse.jsoncase also asserts the Headers object itself keeps the empty value. 22 cases total; the behaviour-asserting ones fail on main and all pass with the fix.serve.test.ts,bun-serve-date.test.ts,bun-serve-routes.test.ts,bun-serve-file.test.ts,bun-serve-cookies.test.ts, andresponse.test.tsare unchanged.[review] gate passed · iteration 2 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file