Bun.serve: write Response.statusText as the reason phrase and drop the HM placeholder - #36003
Bun.serve: write Response.statusText as the reason phrase and drop the HM placeholder#36003robobun wants to merge 4 commits into
Conversation
…e HM placeholder
Bun.serve's status-line writer only consulted the built-in reason-phrase
table; the statusText a handler sets on its Response never reached the wire,
and any code not in the table (419/420/444/499, Cloudflare 520-530, 599)
was sent as 'HTTP/1.1 NNN HM'.
HTTPStatusText gains a format() helper that emits '<code> <reason>' for an
app-supplied statusText (after the RFC 9112 reason-phrase byte check, so a
CR/LF in statusText cannot split the response), then falls back to the table,
then to an empty reason phrase. RequestContext::render_metadata now reads the
Response's statusText and threads it into do_write_status; StaticRoute
snapshots it alongside status_code so routes: { ... } sees it too. FileRoute
and the range-override paths pass an empty slice and keep the canonical
phrase.
|
Warning Review limit reached
Next review available in: 7 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 (3)
WalkthroughChangesThe server now formats HTTP status lines from validated custom reason phrases, stores response status text for static routes, propagates it through dynamic and file responses, and tests custom, fallback, and CR/LF-filtered reason phrases. HTTP status reason phrases
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:32 PM PT - Jul 26th, 2026
❌ @robobun, your commit b6948d2 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 36003That installs a local version of the PR into your bun-36003 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The diff is ready; the new Build 82812's only hard failure is the binary-size check, which is comparing against a stale canary (#79916 at ae4b17d, the last passed main build). This branch is based on 44f6469, twelve commits later, and the ~540 KB delta comes from #32602 (node:quic on lsquic), #31823 (inspector DevTools server) and #31827 (node:repl on acorn) on main, not from the ~200-line status-text change here. The four test-file entries ( |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
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 (1)
src/runtime/server/FileRoute.rs (1)
144-218: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
FileRoutestill drops customResponse.statusTextforBun.file()routes.from_jsnever capturesresponse.get_init_status_text(),FileRoutehas nostatus_textfield, andwrite_status_codealways passes&[]to the HTTP/1.x writers. Sonew Response(Bun.file(...), { statusText: "Custom" })will still emit the default/canned reason phrase instead of the caller’s text. MirrorStaticRoutehere and add a regression test for aBun.file()-backed route.🤖 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/FileRoute.rs` around lines 144 - 218, Update FileRoute to preserve custom Response status text: add a status_text field, capture response.get_init_status_text() in from_js when constructing a response-backed route, and pass it through write_status_code to the HTTP/1.x writers instead of always using an empty slice. Mirror StaticRoute’s implementation and add a regression test covering a Bun.file()-backed Response with a custom statusText.
🤖 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/server/FileRoute.rs`:
- Around line 144-218: Update FileRoute to preserve custom Response status text:
add a status_text field, capture response.get_init_status_text() in from_js when
constructing a response-backed route, and pass it through write_status_code to
the HTTP/1.x writers instead of always using an empty slice. Mirror
StaticRoute’s implementation and add a regression test covering a
Bun.file()-backed Response with a custom statusText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c34063ce-f83f-4aa2-9a7a-35233d3adf16
📒 Files selected for processing (7)
src/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/HTTPStatusText.rssrc/runtime/server/RequestContext.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rstest/js/bun/http/serve.test.ts
Mirrors StaticRoute so a Bun.file()-backed routes: entry honours the handler's statusText. The route-computed 206/304/412/416 overrides keep the canonical phrase. Also updates serve-stream-body-error.test.ts which asserted the old 'HM' placeholder for status 597.
There was a problem hiding this comment.
LGTM — both earlier findings (the FileRoute sibling gap and the stale 597 HM assertion) are addressed in b6948d2, and this pass found nothing new.
What was reviewed:
HTTPStatusText::formatbuffer arithmetic (256-byte buf vs. max 5-digit code + truncated reason) — no overflow.is_valid_reason_phraseagainst RFC 9112 §4 — correctly rejects CR/LF/NUL, so response splitting viastatusTextis blocked (covered by the/injectioncases).- Borrow lifetime of
status_text_sliceinrender_metadata—swap_init_headersonly touchesinit.headers, andBunStringisCopywith noDrop, so the +0 view stays valid throughdo_write_status. - All
write_statusfree-function callers updated; server-computed overrides (206/304/405/412/416) still get the canonical phrase via&[].
Extended reasoning...
Overview
Threads Response.statusText through Bun.serve's status-line writer and replaces the HM placeholder with an empty reason phrase for unknown codes. New HTTPStatusText::format centralizes the "<code> <reason>" construction; RequestContext::do_write_status, StaticRoute::do_write_status, FileRoute::write_status_code, and the shared server::write_status all route through it. StaticRoute and FileRoute gain a status_text: Box<[u8]> snapshot (populated in from_js, defaulted elsewhere, cloned/counted in clone/memory_cost); HTMLBundle's construction site defaults it. Tests add a raw-socket describe block covering dynamic/static/file routes, CR-LF injection, and a fetch() round-trip; the ASAN-gated serve-stream-body-error assertion is updated from 597 HM to 597 .
Security risks
The one risk is HTTP response splitting via a statusText containing CR/LF. is_valid_reason_phrase implements the RFC 9112 §4 byte set (HTAB / SP / VCHAR / obs-text) and rejects any control byte other than HTAB, so a statusText with \r\n falls through to the canned/empty phrase. The /injection and /injection-unknown test cases assert this on the wire. No other security-sensitive surface is touched.
Level of scrutiny
Moderate. render_metadata is on the per-response hot path, but the added work is a BunString bitwise copy, a possible Latin-1→UTF-8 transcode of a short string, and a bounded copy_from_slice into a 256-byte stack buffer — no allocation on the common all-ASCII path (to_utf8_without_ref returns a never_free borrowed slice). No new unsafe, no refcount changes, no GC-visible fields. The struct-field additions are plain owned Box<[u8]> with default Drop.
Other factors
This is a re-review after b6948d2 addressed both of my earlier comments: FileRoute now snapshots status_text alongside status_code and threads it through only when the route's own status is emitted (206/304/412/416 keep the canonical phrase), and the stale 597 HM snapshot assertion is updated. I verified all callers of the free write_status were updated (only FileRoute/StaticRoute call it), that swap_init_headers mutates only init.headers so the +0 status_text borrow in render_metadata is sound, and that bun_core::String is Copy with no Drop so the +0 handle is not spuriously deref'd. The buffer math in format is safe for any u16 code. CI build 82812 is still queued (earlier builds failed at agent provisioning, not at compile/test), but the author reports local green including the ASAN-gated test.
What does this PR do?
Bun.servewrote the placeholder reason phraseHMfor every status code missing from its built-in table, and never serialized the handler'sstatusText, so applications could not override it.Repro
Before:
The
statusTextnever appears; even for 201 the cannedCreatedwins.Cause
RequestContext::do_write_status(andStaticRoute/server::write_status) only looked upHTTPStatusText::get(code)and fell back to"{code} HM".Response.init.status_textwas stored but never read on the server side.Fix
HTTPStatusText::format(buf, code, status_text)now builds the status-line token: it writes a non-emptystatus_textthat passes the RFC 9112 reason-phrase byte check (so a CR/LF cannot split the response), otherwise falls back to the table, otherwise emits an empty reason phrase ("<code> ").RequestContext::render_metadatareads theResponse'sstatus_textand threads it throughdo_write_status;StaticRoutesnapshots it next tostatus_codesoroutes: { "/": new Response(...) }honours it too. Range/precondition overrides (206/304/405/412/416) keep passing an empty slice and get the canonical phrase.After:
and with no
statusText:NodeHTTPResponse.rsis left alone; thenode:httpJS layer already defaults an unsetstatusMessagetoSTATUS_CODES[code] || "unknown", and #35017 is touching that file.Verification
Both new tests fail on main (
HTTP/1.1 599 HM,statusText === "HM") and pass here. The existingshould return <code> <phrase>loop,bun-serve-routes.test.ts,bun-serve-file.test.ts,bun-serve-headers.test.tsand the static-route stress tests continue to pass.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts
Fixes #13817