Skip to content

Bun.serve: fix HEAD and 204 response framing - #32800

Merged
Jarred-Sumner merged 8 commits into
mainfrom
farm/d01b7291/serve-head-204-framing
Jun 28, 2026
Merged

Jarred-Sumner merged 8 commits into
mainfrom
farm/d01b7291/serve-head-204-framing

Conversation

@robobun

@robobun robobun commented Jun 27, 2026 •

Copy link
Copy Markdown
Collaborator

Three RFC violations in Bun.serve's HTTP/1 response framing, all with the same shape: the HEAD path, the 204 path, and the empty-file path each derived framing independently of what GET puts on the wire. Review turned up a fourth member of the class: the static routes: paths (StaticRoute, FileRoute) put a null-body status's Response body bytes on the wire, which GET's render() drops.

Repro

// 1. Handler-supplied Content-Length: corrected on GET, forwarded verbatim on HEAD
using a = Bun.serve({ port: 0, fetch: () => new Response("hi", { headers: { "Content-Length": "999" } }) });
(await fetch(a.url)).headers.get("content-length");                     // "2"
(await fetch(a.url, { method: "HEAD" })).headers.get("content-length"); // "999"

// 2. Default-200 Response with an empty file body is rewritten to 204, but not on HEAD
require("fs").writeFileSync("/tmp/e", "");
using b = Bun.serve({ port: 0, fetch: () => new Response(Bun.file("/tmp/e")) });
(await fetch(b.url)).status;                     // 204
(await fetch(b.url, { method: "HEAD" })).status; // 200

// 3. Every 204 carries Content-Length: 0
using c = Bun.serve({ port: 0, fetch: () => new Response(null, { status: 204 }) });
// raw socket: HTTP/1.1 204 No Content\r\nDate: ...\r\nContent-Length: 0\r\n\r\n

// 4. A static route puts a null-body status's body bytes on the wire
using d = Bun.serve({ port: 0, routes: { "/": new Response("data", { status: 204 }) }, fetch: () => new Response(null) });
// raw socket: HTTP/1.1 204 No Content\r\n...Content-Length: 4\r\n\r\ndata

RFC 9110 9.3.2: a HEAD response carries the same header fields a GET of the same target would have. RFC 9110 8.6: a server MUST NOT send Content-Length in a response with a 1xx or 204 status (case 3 is issue #20676, where such a 204 is rejected as a framing error by a proxy). RFC 9112 6.3: a 1xx/204/304 is terminated by the blank line after the header fields regardless of what follows, so case 4's stray body bytes desync the next keep-alive response.

Causes

  1. do_render_head_response forwarded a handler-supplied Content-Length / Transfer-Encoding header before looking at the body. GET strips both (render_metadata -> do_write_headers) and frames from the body's byte count.
  2. render_metadata rewrote 200 -> 204 when size == 0 && !blob.is_detached(). Every in-memory empty body ("", Uint8Array(0), new Blob([])) happens to report is_detached() == true, so only the file / Blob-store forms got the rewrite, and the HEAD path (whose self.blob is never populated) never did. StaticRoute and FileRoute each carried a copy of the same rewrite.
  3. Bun.serve never marked the uWS response as a no-body status, and uWS's tryEnd -> internalEnd path (unlike end()) never consulted noBodyStatus, so render_bytes's try_end("", 0) always emitted Content-Length: 0.
  4. The WHATWG null-body-status set {101, 103, 204, 205, 304} was spelled out inline at three sites and absent from two: render() dropped the body, but StaticRoute unconditionally sent its cached blob, and FileRoute's bodiless list (204 | 205 | 304 | 307 | 308) was missing 1xx.

Fix

  • uws::HttpResponse::writeStatus records noBodyStatus for 1xx / 204 statuses (the existing flag node:http already sets for 204/304). internalEnd then drops the Content-Length, the data, and the totalSize, exactly what end() already does on its noBodyStatus short-circuit; writeHeader(key, uint64_t) skips an integer Content-Length too. 205 (MUST indicate a zero-length body, RFC 9110 15.3.6) and 304 (MAY carry one, 15.4.5) keep Content-Length: 0.
  • do_render_head_response short-circuits null-body statuses to the same render_metadata() + empty try_end that GET's render() uses, instead of framing from the dropped body or the user headers.
  • do_render_head_response honors a handler-supplied Content-Length / Transfer-Encoding only when the Response's body is null. A null body carries no framing of its own, so the header is the only description of what a GET would have sent; that is the escape hatch issue Bun always sets Content-Length to 0 for HEAD response #15355 added for HEAD handlers and it is preserved (its tests still pass). A real body's byte count now wins for both methods.
  • The 200 -> 204-on-empty rewrite is removed from render_metadata, StaticRoute, and FileRoute. An empty file is a valid zero-byte representation. The rewrite was undocumented, applied to only one of six empty body forms, never applied on HEAD, and produced the spec-violating 204 + Content-Length: 0. Every empty body now frames as 200 with Content-Length: 0, as new Response("") already did.
  • StaticRoute drops the body for a null-body status and its HEAD path reports Content-Length: 0 (mirroring GET); FileRoute's bodiless early return gains 1xx. FileResponseStream ships the file via sendfile / write(), neither of which goes through internalEnd, so FileRoute needs its own guard. The set now lives in one HTTPStatusText::is_null_body predicate used by all four paths.

Existing tests updated

  • bun-server.test.ts "transfer-encoding / content-length whose StringImpl is held only by the header map" used new Response("hello", ...) with duplicate Transfer-Encoding / Content-Length headers and asserted HEAD forwarded them, which is the behavior item 1 removes. The test exists to catch a use-after-free in the header fast path (fastGet -> render_metadata frees the StringImpl it borrowed); that path is now only reachable with a null body, so the fixture bodies became null and the assertions are unchanged.
  • bun-serve-file.test.ts "serves empty file" and "returns 204 for empty files with 200 status" now assert 200 + Content-Length: 0, for GET and HEAD, on both the route and fetch-handler paths.

Verification

Full framing matrix (raw socket, body form x GET/HEAD) before and after:

Details
# before (bun 1.4.0 / main)
body                         GET                          HEAD
Response("hi")               200 CL=2                     200 CL=2
Response("")                 200 CL=0                     200 CL=0
Response(null)               200 CL=0                     200 CL=0
Response(Uint8Array(0))      200 CL=0                     200 CL=0
Response(new Blob([]))       200 CL=0                     200 CL=0
Bun.file(empty)              204 CL=0                     200 CL=0   <<<
Bun.file(full)               200 CL=10                    200 CL=10
"hi" + CL:999                200 CL=2                     200 CL=999 <<<
null + CL:999                200 CL=0                     200 CL=999
null status 204              204 CL=0                     204 CL=0   <<<
"body" status 204            204 CL=0                     204 CL=4   <<<
null status 304              304 CL=0                     304 CL=0
null status 205              205 CL=0                     205 CL=0
null status 101              101 CL=0                     101 CL=0   <<<

# after
Bun.file(empty)              200 CL=0                     200 CL=0
"hi" + CL:999                200 CL=2                     200 CL=2
null + CL:999                200 CL=0                     200 CL=999   (#15355, unchanged)
null status 204              204 CL=-                     204 CL=-
"body" status 204            204 CL=-                     204 CL=-
null status 304              304 CL=0                     304 CL=0
null status 205              205 CL=0                     205 CL=0
null status 101              101 CL=-                     101 CL=-

# routes vs fetch handler, Response("data", { status }), before -> after
204 routes  GET   204 CL=4 body="data"   ->  204 CL=-  body=""
204 routes  HEAD  204 CL=4               ->  204 CL=-
205 routes  GET   205 CL=4 body="data"   ->  205 CL=0  body=""
304 routes  GET   304 CL=4 body="data"   ->  304 CL=0  body=""

The new describe("response framing") in serve.test.ts has 24 assertions that fail on the unmodified build and 7 deliberate positive controls (205/304 keep Content-Length: 0 on the fetch path; the null-body escape hatch still forwards the user header). bun-serve-file.test.ts (67), bun-serve-static.test.ts (34), bun-serve-routes.test.ts (41), node-http.test.ts (127), proxy-stress-adversarial.test.ts (151), serve-http3.test.ts (45) all pass.

Fixes #20676

Related: #32794 fixes the fourth framing divergence the same fuzzer found (Bun.file(p).slice(a, b) treated as a..EOF). Different root cause, but both touch render_metadata, so whichever lands second needs a small rebase.

Three RFC violations in Bun.serve's HTTP/1 response framing, all with
the same shape: the HEAD path, the 204 path, and the empty-file path
each derived framing independently of what GET puts on the wire.

1. A handler-supplied Content-Length was corrected on GET but forwarded
   verbatim on HEAD. HEAD now frames from the body's byte count like
   GET does, except for null-body Responses, where the header is the
   only description of what GET would have sent (issue #15355).

2. new Response(Bun.file(emptyFile)) was rewritten from 200 to 204, but
   only on GET and only for file/Blob-store bodies. The rewrite is
   removed from render_metadata, StaticRoute, and FileRoute: every
   empty body now frames as 200 + Content-Length: 0, like
   new Response("") already did.

3. Every 204 carried Content-Length: 0 (RFC 9110 8.6 MUST NOT;
   issue #20676). uws::HttpResponse::writeStatus now records
   noBodyStatus for 1xx / 204, and internalEnd / writeHeader(uint64_t)
   honor it. 205 and 304 keep the explicit Content-Length: 0.

do_render_head_response also short-circuits the same no-body statuses
render() drops the body for on GET, so a body or user header on a 204
Response no longer leaks into HEAD's Content-Length.

Fixes #20676
@coderabbitai

coderabbitai Bot commented Jun 27, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: de2b5cd3-c543-4723-aa4f-f5558d716532

📥 Commits

Reviewing files that changed from the base of the PR and between 88fc713 and 57fecf4.

📒 Files selected for processing (4)
  • packages/bun-uws/src/HttpResponse.h
  • src/runtime/server/FileRoute.rs
  • src/runtime/server/RequestContext.rs
  • test/js/bun/http/serve.test.ts

Walkthrough

HTTP response handling now uses shared null-body status rules across core framing, HEAD rendering, route responses, and tests. Empty-file routes now stay on 200, and the framing tests were updated to match the new status and header behavior.

Changes

HTTP response framing

Layer / File(s) Summary
No-body status bookkeeping
packages/bun-uws/src/HttpResponse.h, packages/bun-uws/src/HttpResponseData.h, src/runtime/server/HTTPStatusText.rs
HttpResponse records no-body statuses, blocks Content-Length writes, and clears pending body framing; the status helper and comment definitions describe the shared null-body status set.
HEAD framing and header precedence
src/runtime/server/RequestContext.rs, test/js/bun/http/bun-server.test.ts
RequestContext adds null-body handling to HEAD rendering, reworks body-vs-header framing selection, and uses the shared null-body predicate in render paths; the regression test keeps duplicate Transfer-Encoding handling on a bodiless response.
Route null-body rendering
src/runtime/server/FileRoute.rs, src/runtime/server/StaticRoute.rs
FileRoute and StaticRoute use the shared null-body predicate when choosing status codes, ending responses, and computing metadata or content length for route-backed responses.
Framing and file-serving tests
test/js/bun/http/serve.test.ts, test/js/bun/http/bun-serve-file.test.ts
serve.test.ts adds raw framing checks for null-body statuses and HEAD behavior, and bun-serve-file.test.ts updates empty-file expectations to 200 for GET and HEAD plus fetch-backed empty files.

Suggested reviewers

  • alii
  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main framing fix for HEAD and 204 responses.
Description check ✅ Passed The description covers what changed and how it was verified, matching the template's intent.
Linked Issues check ✅ Passed The changes stop Content-Length on 204 responses and make the 204 framing RFC-compliant, satisfying #20676.
Out of Scope Changes check ✅ Passed The additional HEAD, empty-file, static route, and test changes all support the same framing fix and are in scope.

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

@robobun

robobun commented Jun 27, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 4:51 AM PT - Jun 27th, 2026

❌ @robobun, your commit 02910c7 has some failures in Build #65377 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 32800

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

bun-32800 --bun

@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 `@packages/bun-uws/src/HttpResponse.h`:
- Around line 428-433: Update the shared noBodyStatus documentation so it no
longer says 304 responses must not carry Content-Length; the new behavior leaves
304 to callers, while only 1xx and 204 are universally no-body statuses. Adjust
the comment near the noBodyStatus contract in HttpResponseData.h to match the
logic in HttpResponse and prevent future callers from re-suppressing 304 by
mistake.

In `@test/js/bun/http/bun-server.test.ts`:
- Around line 1104-1109: Shorten the new explanatory comment in
bun-server.test.ts to fit the 3-line max while preserving the key point about
HEAD bodiless responses and the Malloc=1 ASAN use-after-free check. Update the
comment near the test case in the HTTP server test so it is concise, removes
extra RFC elaboration, and keeps only the essential rationale tied to the
affected HEAD/body framing behavior and allocator behavior.
🪄 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: 2d7fabf9-01ce-457c-8786-70a81e72a116

📥 Commits

Reviewing files that changed from the base of the PR and between 96a7627 and 1217d5e.

📒 Files selected for processing (7)
  • packages/bun-uws/src/HttpResponse.h
  • src/runtime/server/FileRoute.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/server/StaticRoute.rs
  • test/js/bun/http/bun-serve-file.test.ts
  • test/js/bun/http/bun-server.test.ts
  • test/js/bun/http/serve.test.ts
💤 Files with no reviewable changes (1)
  • src/runtime/server/FileRoute.rs

Comment thread packages/bun-uws/src/HttpResponse.h Outdated
Comment thread test/js/bun/http/bun-server.test.ts Outdated
The HttpResponseData.h comment described noBodyStatus as a 204/304
Content-Length prohibition; writeStatus() now sets it automatically
for 1xx and 204 only, and 304 stays a caller (node:http) opt-in.
Comment thread packages/bun-uws/src/HttpResponse.h Outdated
Comment thread src/runtime/server/FileRoute.rs
A Response body on a null-body status must never reach the wire:
RFC 9112 6.3 terminates a 1xx/204/304 at the blank line after the
header fields regardless of what follows, so stray body bytes desync
the next keep-alive response. `render()` already dropped the body on
the fetch-handler path, but `StaticRoute` and `FileRoute` did not, and
the `noBodyStatus` change in the previous commit suppressed their
Content-Length too, turning a self-consistently-framed (if
RFC-violating) `204 + Content-Length: N + body` into an unframed
`204 + body`.

- `uws::internalEnd` now drops the data and totalSize alongside the
  Content-Length when `noBodyStatus` is set, the same thing `end()`
  already does on its noBodyStatus short-circuit.
- `StaticRoute::do_render_blob_corked` drops the body for a null-body
  status and its HEAD path reports the zero bytes GET actually sends.
- `FileRoute`'s bodiless-status early return gains 1xx, which its
  `204 | 205 | 304 | 307 | 308` list was missing; FileResponseStream
  ships the file via sendfile/write(), neither of which goes through
  `internalEnd`, so it needs its own guard.
- The WHATWG null-body-status set `{101, 103, 204, 205, 304}` appeared
  inline at four sites; it is now `HTTPStatusText::is_null_body`.

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

f476d0a addresses both of my earlier inline findings (the internalEnd data-write and the FileRoute 101 gap), with tests covering each — nothing further from me there. Still worth a human pass given this rewires HTTP/1 framing across uWS + three route paths and drops the empty-file 200→204 rewrite.

Extended reasoning...

Overview

This PR fixes three RFC 9110/9112 framing violations in Bun.serve: HEAD responses forwarding handler-supplied Content-Length/Transfer-Encoding instead of mirroring GET; 1xx/204 responses carrying Content-Length: 0; and an undocumented 200→204 rewrite for empty file bodies that applied inconsistently. It touches packages/bun-uws/src/HttpResponse.h (writeStatus auto-sets noBodyStatus for 1xx/204; internalEnd and writeHeader(key, uint64_t) honor it), HttpResponseData.h (comment only), and the Rust RequestContext / StaticRoute / FileRoute render paths, plus a new shared HTTPStatusText::is_null_body() helper. ~190 lines of new tests in serve.test.ts exercise the raw-socket framing matrix.

Prior review

I left two 🔴 inline comments on earlier revisions identifying keep-alive desync regressions (StaticRoute → tryEnd writing body bytes after a 204 with no framing; FileRoute on status 101 reaching sendfile after its Content-Length was suppressed). Commit f476d0a addresses both: internalEnd now clears data/totalSize when noBodyStatus is set, StaticRoute::do_render_blob_corked short-circuits null-body statuses, and FileRoute switched its bodiless guard to HTTPStatusText::is_null_body() || matches!(.., 307 | 308). The new describe.each(["routes", "fetch"]) and Bun.file route on a null-body status tests in serve.test.ts directly cover the cases I raised. The bug-hunting system found nothing further on the current revision.

Security risks

HTTP/1 message framing is exactly where response-smuggling / keep-alive desync bugs live; the earlier revisions of this PR introduced two such regressions, now fixed. The current revision moves toward stricter RFC compliance and adds defensive drops at the uWS layer, so the net direction is risk-reducing — but the surface area (three independent route renderers each deciding framing, plus a shared C++ layer that node:http also uses) means a missed path is plausible.

Level of scrutiny

High. This is production-critical wire-protocol code in both the vendored uWS C++ and the Rust server, with a user-visible behavior change (empty-file responses now 200 instead of 204). The PR description's framing matrix and the new raw-socket tests are thorough, and node-http / proxy-stress / serve-http3 suites are reported passing, but a maintainer familiar with the Bun.serve render paths (cirospaciari is the suggested reviewer) should sign off on the design choices — particularly the 200→204 removal and the decision to leave 304's Content-Length to callers.

@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

Caution

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

⚠️ Outside diff range comments (1)
src/runtime/server/RequestContext.rs (1)

2445-2449: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shorten this comment to the 3-line cap.

Proposed rewrite
-        // A body, when present, decides the framing: GET puts its byte count
-        // on the wire and strips any handler-supplied Content-Length /
-        // Transfer-Encoding, so HEAD must report the same (RFC 9110 §9.3.2).
-        // Only a bodiless Response leaves those headers as the sole
-        // description of what a GET would have sent (issue `#15355`).
+        // A body decides framing; HEAD reports the same as GET (RFC 9110 §9.3.2).
+        // Only a bodiless Response lets handler Content-Length /
+        // Transfer-Encoding describe what GET would send (issue `#15355`).

As per coding guidelines, “Keep code comments to 3 lines max.”

🤖 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/server/RequestContext.rs` around lines 2445 - 2449, The
explanatory comment in RequestContext::RequestContext response framing logic
exceeds the 3-line comment limit and should be condensed. Shorten the multi-line
note around the body/HEAD/GET framing behavior to a concise 3-line version while
preserving the key rule about body-present responses and bodiless responses, and
keep it attached to the same conditional logic so it remains clear where the
behavior applies.

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.

Inline comments:
In `@src/runtime/server/FileRoute.rs`:
- Around line 491-498: The early null-body guard in FileRoute::serve is
returning before the Content-Length handling, which causes 205/304 responses to
miss the required Content-Length: 0 behavior. Adjust the early-return condition
so it only bypasses body emission for true null-body/redirect cases, and let
205/304 continue through the existing header-writing path or explicitly set
Content-Length: 0 before returning. Use the existing FileResponseStream response
flow and HTTPStatusText::is_null_body/status_code checks to keep the fix
localized.

In `@test/js/bun/http/serve.test.ts`:
- Around line 1271-1274: Shorten the explanatory comment in the null-body status
test to fit the 3-line limit while preserving the key point about 204/304
responses and the static routes path; update the comment near the
`serve.test.ts` case so it briefly states that a Response body on a null-body
status never reaches the wire, and that the `routes:` path should drop it like
`render` already does. Keep the reference to RFC 9112 and the `render`/`routes:`
behavior, but remove extra detail to stay within the comment length guideline.

---

Outside diff comments:
In `@src/runtime/server/RequestContext.rs`:
- Around line 2445-2449: The explanatory comment in
RequestContext::RequestContext response framing logic exceeds the 3-line comment
limit and should be condensed. Shorten the multi-line note around the
body/HEAD/GET framing behavior to a concise 3-line version while preserving the
key rule about body-present responses and bodiless responses, and keep it
attached to the same conditional logic so it remains clear where the behavior
applies.
🪄 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: 822e2896-6931-4768-aa61-94ac7053c5a0

📥 Commits

Reviewing files that changed from the base of the PR and between 1217d5e and 88fc713.

📒 Files selected for processing (8)
  • packages/bun-uws/src/HttpResponse.h
  • packages/bun-uws/src/HttpResponseData.h
  • src/runtime/server/FileRoute.rs
  • src/runtime/server/HTTPStatusText.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/server/StaticRoute.rs
  • test/js/bun/http/bun-server.test.ts
  • test/js/bun/http/serve.test.ts

Comment thread src/runtime/server/FileRoute.rs Outdated
Comment thread test/js/bun/http/serve.test.ts Outdated
Comment thread src/runtime/server/FileRoute.rs Outdated
@robobun

robobun commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator Author

Status: the diff is done and green; the only remaining red is a Buildkite infrastructure failure that never runs a test.

Build #65322 ran the current tree (57fecf4c51) to completion: 283 test lanes passed, 1 failed. The one failed job is :darwin: 26 aarch64 - test-bun, and its entire log is 252 lines ending in

Error: buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'.
Refusing to continue with a partial download (would silently fall back to the wrong binary).

so it dies fetching the build artifact before executing a single test. That same failure, on that same lane, has hit all five of this PR's builds (#65229, #65255, #65303, #65322, #65377) and also an unrelated PR's build (#65305), so it is a darwin-26-aarch64 agent-pool problem, not anything this diff can reach. Every lane that actually runs the changed code (Linux, Linux ASAN, Windows, darwin 14) passes, including the new 31-case response framing matrix in serve.test.ts. An earlier set of Windows failures was this branch missing main's #32797 and went away after merging main.

I have already used a ci: retrigger (#65377); both darwin 26 shards hit the identical artifact-download timeout again, so I am not going to keep retriggering.

Review state: all threads are resolved. claude[bot] confirmed both of its earlier findings fixed and had nothing further; CodeRabbit's last pass reported no actionable comments.

This needs a maintainer to merge over (or re-run) the single infra-red darwin lane.

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

The two desync regressions and the comment-length nits I flagged earlier are all addressed — nothing further from me. This still warrants a human look though: it changes HTTP/1 wire framing across uWS writeStatus/internalEnd and three Rust render paths, drops the empty-file 200→204 rewrite (a user-visible behavior change), and Build #65322 is currently red.

Extended reasoning...

Overview

This PR fixes RFC 9110/9112 framing violations in Bun.serve: 204/1xx responses no longer carry Content-Length, HEAD now derives framing from the same source GET does (body byte-count beats handler-supplied Content-Length/Transfer-Encoding when a body exists), the undocumented 200→204 rewrite for empty file bodies is removed, and StaticRoute/FileRoute now drop body bytes for null-body statuses. The change spans packages/bun-uws/src/HttpResponse.h (writeStatus auto-sets noBodyStatus for 1xx/204; internalEnd clears data/totalSize/Content-Length when set; writeHeader(key, uint64_t) skips Content-Length), a comment-only edit in HttpResponseData.h, a new is_null_body predicate in HTTPStatusText.rs, and logic changes in RequestContext.rs (do_render_head_response, render_metadata, render), StaticRoute.rs, and FileRoute.rs. Three test files are updated/expanded with a full status×method×path matrix and raw-socket assertions.

Security risks

HTTP response-framing correctness is desync-adjacent: getting body delimitation wrong on a keep-alive connection behind a proxy is the classic request-smuggling vector. My earlier review of this PR found two such regressions (StaticRoute 204 body emitted with no framing; FileRoute 101 body shipped via sendfile after Content-Length suppression), both confirmed and fixed in f476d0a with regression tests. The current diff looks correct to me on those paths and the new internalEnd data-drop is the layer-owning invariant, but framing changes of this breadth deserve a second pair of eyes.

Level of scrutiny

High. This is production-critical wire-protocol behavior in the core HTTP server, touching the vendored uWS C++ layer plus three independent Rust render paths that must agree. It also removes a long-standing (if inconsistent) user-visible behavior — empty Bun.file responses now return 200 instead of 204 — which the PR body justifies well but is still a deliberate behavior change a maintainer should sign off on.

Other factors

  • All my prior inline findings (two 🔴 desync regressions, one 🟡 comment-length nit covering three blocks) were addressed by the author and are resolved.
  • CodeRabbit raised two trivial nits (addressed) and one functional concern on FileRoute 205/304 that the author correctly rebutted as pre-existing behavior locked by an existing test.
  • robobun reports Build #65322 failing on the head commit 57fecf4; that needs to go green or be explained before merge.
  • Test coverage is thorough: the new describe("response framing") matrix in serve.test.ts exercises GET/HEAD × {101,204,205,304} × {routes,fetch} via raw sockets, plus the FileRoute 101 case, and the PR body lists six existing suites passing locally.
  • CodeRabbit suggested alii and cirospaciari as reviewers (HTTP server owners).

Jarred-Sumner pushed a commit that referenced this pull request Jun 28, 2026
…te objects (#32822)

A `routes` value of the per-method object form never answers HEAD unless
a `HEAD` key is spelled out. A HEAD request to that path is served by
whatever matches next, so HEAD and GET on the same URL return different
representations, or it 404s when nothing else matches.

### Repro

```js
using srv = Bun.serve({
  port: 0,
  routes: {
    "/m": { GET: () => new Response("hello-get") },
    "/*": () => new Response("from-catch-all"),
  },
});
// GET  /m -> 200, Content-Length: 9  ("hello-get")
// HEAD /m -> 200, Content-Length: 14 (the "/*" route's "from-catch-all")
// ...and with no "/*" route, HEAD /m -> 404
```

RFC 9110 section 9.3.2 requires HEAD to return the same header fields a
GET of the same target would. Every other `Bun.serve` route form already
derives HEAD from GET: plain function routes (registered for any
method), static `Response` / `Bun.file` routes (which register a
dedicated HEAD handler), and the `fetch` fallback. Only the per-method
object form is missing it, and an intermediary that caches based on a
HEAD probe gets a different answer than GET.

### Cause

Three sites, same class. Note that `HttpRouter::add` removes an existing
handler for the same method, pattern, and priority before inserting, so
the last registration for a method and path wins, and `set_routes`
registers static routes after user routes.

1. `ServerConfig::from_js` turns each key of a per-method route object
into a user route registered for that one method only. Nothing registers
the path under HEAD, so the router never matches it.
2. `apply_static_route` registered a HEAD handler for every static entry
regardless of its declared methods, so a static `Response` under a
non-GET key both answers HEAD on its own and captures HEAD away from a
sibling GET handler:

```js
using a = Bun.serve({ port: 0, routes: {
  "/m": { GET: () => new Response("hello-get"), POST: new Response("static-post-response") },
}});
// GET  /m -> Content-Length: 9
// HEAD /m -> Content-Length: 20   (the POST response's framing)

using b = Bun.serve({ port: 0, routes: { "/p": { POST: new Response("p") } } });
// GET /p -> 404,  HEAD /p -> 200
```

3. For the same reason, a static `Response` under the GET key silently
displaced an explicit callable HEAD handler on the same path:

```js
using c = Bun.serve({ port: 0, routes: {
  "/m": { GET: new Response("g"), HEAD: () => new Response(null, { headers: { "x-h": "1" } }) },
}});
// HEAD /m -> Content-Length: 1, no x-h header; the HEAD handler never runs
```

### Fix

- When a per-method route object has a callable GET handler and no HEAD
entry, register the GET handler under HEAD as well. The request context
is created with `method == HEAD`, so the existing HEAD rendering path
strips the body and reports the Content-Length GET would have produced;
the handler observes `req.method === "HEAD"`, the same as a plain
function route does today.
- `apply_static_route` (and its HTTP/3 twin) register the implicit HEAD
handler only for an entry that serves any method, GET, or HEAD, and
never for a path that already has an explicit HEAD handler route. A
static `Response` under a non-GET, non-HEAD key no longer answers or
captures HEAD, and a static GET `Response` no longer displaces a
declared HEAD handler.

With that, an explicit `HEAD` key, handler or static `Response`, always
takes precedence, and other methods are unchanged: they still fall
through to later routes, which is how per-method routes compose with a
`"/*"` catch-all.

### Verification

New `describe("implicit HEAD for per-method route objects")` in
`test/js/bun/http/bun-serve-routes.test.ts` with nine tests. Six fail on
the unmodified build:

- HEAD falls through to the `"/*"` catch-all instead of the GET handler
- HEAD 404s when there is no catch-all
- the `req.method` and Content-Length the GET handler observes
- `{ GET: handler, POST: new Response(...) }` answers HEAD with the POST
response's framing
- `{ POST: new Response(...) }` answers HEAD at all
- `{ GET: new Response(...), HEAD: handler }` drops the explicit HEAD
handler

The other three are positive controls: an explicit HEAD handler over a
GET handler, an explicit static HEAD `Response`, and a route object with
no GET handler.

`bun-serve-routes.test.ts` (50), `bun-serve-static.test.ts` (34),
`serve-http3.test.ts` (45), `bun-serve-file.test.ts` (66),
`serve-if-none-match.test.ts` (17), and
`bun-serve-html-manifest.test.ts` (4) all pass. `serve.test.ts` and
`bun-server.test.ts` have the same failures as the unmodified build
(environment dependent: IPv6, privileged ports).

Related: #32800 fixes HEAD response framing (what goes on the wire for a
HEAD response). This fixes HEAD route dispatch (which handler a HEAD
request reaches). The two change disjoint files. #32823 fixes the
route-parameter percent-decoder, reported together with this but
independent.

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
@Jarred-Sumner
Jarred-Sumner merged commit 8f6a7c6 into main Jun 28, 2026
77 of 79 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/d01b7291/serve-head-204-framing branch June 28, 2026 01:52
robobun added a commit that referenced this pull request Jun 28, 2026
…32800

8f6a7c6 (#32800) removed render_metadata's 200-to-204 rewrite for an
empty body, so an empty Bun.file().slice() is now a plain 200 with
Content-Length: 0 on both GET and HEAD. That also makes GET and HEAD
fully agree for an empty slice, so the two separate empty-slice tests
collapse into the GET/HEAD parity matrix.
robobun added a commit that referenced this pull request Jun 30, 2026
…32800

8f6a7c6 (#32800) removed render_metadata's 200-to-204 rewrite for an
empty body, so an empty Bun.file().slice() is now a plain 200 with
Content-Length: 0 on both GET and HEAD. That also makes GET and HEAD
fully agree for an empty slice, so the two separate empty-slice tests
collapse into the GET/HEAD parity matrix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun wrongly sends "Content-Length" header with HTTP 204 responses, breaking compliance with HTTP spec.

2 participants