Repository navigation
Bun.serve: Date/Content-Length on parser-tier 4xx/5xx; 414 for oversize URI; 501 for unknown methods - #35299
Bun.serve: Date/Content-Length on parser-tier 4xx/5xx; 414 for oversize URI; 501 for unknown methods#35299robobun wants to merge 4 commits into
Conversation
…ze URI; 501 for unknown methods The request-parser rejection path wrote only a status line and Connection: close. RFC 9110 6.6.1 says an origin server with a clock MUST send Date on every response, and the absence of Content-Length left the body framing to connection close; the handler path already sends both. The status-line table is now just status lines, and writeHttpErrorAndClose() appends Connection: close, Content-Length: 0 and the loop's cached Date before shutting the socket. An oversize request-target was answered 431 (Request Header Fields Too Large). 431 is for header fields; 414 is the status for a request-target that alone exceeds the server's limit (RFC 9110 15.5.15). The parser now distinguishes the two: after a parse the request-target length is compared to the limit, and on a short-read overflow a request-line that has not yet terminated (no LF in the buffered bytes) is classified 414. The node:http clientError code is unchanged (llhttp reports HPE_HEADER_OVERFLOW either way). A syntactically valid method token outside the router's method table (lowercase "get", "FROB", "BREW", "GETS") closed the connection with zero response bytes: Bun.serve expands the wildcard handler to supportedHttpMethods, so the ANY fallback node has no handler and the route miss went straight to us_socket_close. A miss now writes 501 Not Implemented (RFC 9110 9.1) with the same Date/Content-Length headers before closing.
|
Warning Review limit reached
Next review available in: 23 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 (1)
WalkthroughChangesHTTP error response handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
|
Reproduced with the raw-socket script (exit 86 on canary). With this change: Tests added to |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/HttpParser.h`:
- Line 1561: Update the overflow handling around headOverflowStatus so the scan
uses the bounded concatenation of fallback and data: scan fallback first, then
only the valid data prefix that fits within maxFallbackSize, bounded by length.
Avoid reading beyond buffered bytes and preserve detection of newlines already
present in fallback.
In `@test/js/bun/http/serve.test.ts`:
- Around line 3697-3699: Update the rawRequest error handler so socket or
connection errors reject the helper promise instead of resolving it. Preserve
normal resolution for successful responses, and propagate the original transport
error through the rejection path.
- Around line 3760-3766: Make the fragmented request setup in the rawRequest
test deterministic by introducing an explicit parser boundary or other
observable synchronization barrier between the 10,000-byte prefix, big, and HTTP
headers. Ensure the test reliably exercises the fallback-buffer overflow path
rather than allowing write/flush coalescing to produce a single read.
🪄 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: 378d04ff-e31e-4361-ae6e-4f1444090ab6
📒 Files selected for processing (4)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpErrors.hpackages/bun-uws/src/HttpParser.htest/js/bun/http/serve.test.ts
…headOverflowStatus Client and server share one event loop in-process, so back-to-back conn.write() calls all queued before the server's onData fired and the trickled request arrived as one read, taking the same post-parse classifier as the single-write case. Yielding between writes lets each piece reach onData separately, so the first two land in the fallback buffer and the third fills it, and headOverflowStatus() is what returns 414. Verified by breaking the helper to always return 431: trickled now fails while uriOnce still passes.
There was a problem hiding this comment.
The ccb5e81 fix addresses the trickled-write coverage gap I flagged; no further issues found. Deferring to a maintainer since this changes user-visible Bun.serve behavior (501 for unrouted methods, 414 vs 431 classification) in the core uWS parser/context path.
What was reviewed:
writeHttpErrorAndClose: buffer sizing (worst case 123 < 160),%.29sbounds the LoopData date read, andLoopData()initializesdatein its ctor.- Router-miss 501: node:http registers on
ANY_METHOD_TOKEN(itsuseStrictMethodValidationskips thesupportedHttpMethodsexpansion inonHttp) and rejects unknown methods at parse time, so itsclientErrorpath is unaffected; Bun.serve always registers a/*handler on every supported method. headOverflowStatus(data, maxFallbackSize)at HttpParser.h:1561 is bounded (length >= maxFallbackSizein that branch,fallbackalready cleared).
Extended reasoning...
Overview
Touches packages/bun-uws/src/{HttpErrors,HttpContext,HttpParser}.h and adds a describe block to test/js/bun/http/serve.test.ts. Three behavioral changes to the Bun.serve parser-rejection path: (1) parser-tier 4xx/5xx responses now carry Date and Content-Length: 0 via a new writeHttpErrorAndClose helper; (2) request-target overflow is classified as 414 (both post-parse and via a new headOverflowStatus LF scan for the fallback-buffer paths); (3) a router miss now writes 501 instead of silently closing.
Security risks
This is HTTP protocol handling on untrusted input. Checked: the snprintf into char buf[160] cannot overflow (longest status line is 44 bytes; total worst case ~123); %.29s bounds the date read and LoopData::date[32] is initialized in the ctor. The memchr in headOverflowStatus at both call sites reads only within valid bytes (the second call site is in the length >= maxFallbackSize branch with fallback already cleared, as robobun noted). The HttpParserError reported to node:http clientError is unchanged, so the node compat default 431 response and HPE_HEADER_OVERFLOW mapping are preserved.
Level of scrutiny
High — this is the core uWS HTTP server path shared by Bun.serve and node:http, and it changes wire-visible behavior. The 501-on-router-miss is a design decision: it improves on the current silent close but does not dispatch custom methods to fetch() (which #21566 asks for), so a maintainer should confirm 501 is the intended intermediate step. The 414/431 heuristic (no LF in the buffered window ⇒ request-line not terminated ⇒ 414) is reasonable but is a classification choice worth a human eye.
Other factors
My earlier inline comment (the trickled test not exercising headOverflowStatus) was fixed in ccb5e81 with an await Bun.sleep(0) yield between writes, and the author verified by mutation-testing the helper. All CodeRabbit threads are resolved (two withdrawn as not-a-bug, one addressed by the same commit). Tests cover 400/431/505 headers, 414 single-read + trickled, 431 for oversize header field, and 501 for four unknown method tokens plus dispatch of two known extension methods. The ref-count / cork behavior of the router-miss path is unchanged from the previous us_socket_close-only version.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-23 and it conflicts with main. This is not a judgment on the fix itself. The linked issue (#6556) 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
Three defects in the
Bun.serveparser-rejection path, all observable with a raw TCP client:Cause
httpErrorResponses[](packages/bun-uws/src/HttpErrors.h) held full responses containing only the status line andConnection: close; every handler-path response writesDate(RFC 9110 6.6.1: MUST on any response from a server with a clock) andContent-Length.consumeRequestLineand the fallback-buffer overflow paths inHttpParser.hreported any request-head overflow asHTTP_ERROR_431_REQUEST_HEADER_FIELDS_TOO_LARGE, with no way to tell a request-target overflow (414, RFC 9110 15.5.15) from header-field overflow (431).HttpContext.hcalledus_socket_close()with no write whenrouter->route()missed. Bun.serve'sany()registration expands"*"to the fixedsupportedHttpMethodslist, so theANY_METHOD_TOKENfallback node carries no handler and a method token outside that list (FROB, lowercaseget,GETS,BREW) routes nowhere.Fix
httpErrorResponses[]now holds just the status lines; a newwriteHttpErrorAndClose()appendsConnection: close,Content-Length: 0, and the loop's cachedDatebefore shutdown/close. Both the parser-error path and the route-miss path use it.HttpParserErrorreported toonClientErroris unchanged (HEADER_FIELDS_TOO_LARGE), matching Node/llhttp which only hasHPE_HEADER_OVERFLOW;node:http's default 431 response is therefore unchanged.HTTP/1.1 501 Not Implemented(RFC 9110 9.1) with the same headers before closing.Verification
New
describe("parser-tier error responses")block intest/js/bun/http/serve.test.ts:Date(IMF-fixdate) andContent-Length: 0, body is empty.FROB/get/BREW/GETS-> 501 with Date/CL;PROPFIND/PURGE-> dispatched tofetch()(200).All three tests fail on
mainand pass with this change.test/js/bun/http/request-smuggling.test.tsand thenode:httptest-http-header-overflow.js/test-http-server-client-error.js/test-http-server-destroy-socket-on-client-error.jsparallel tests are unchanged.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 #6556
Related: #21566 (the Bun.serve side no longer closes silently; requests now get 501 instead, though custom methods are still not dispatched to
fetch()), #27512 (that 413 is themaxRequestBodySizeearly rejection insrc/runtime/server/mod.rs, which useswrite_status+end_without_bodyrather than the parser-error table touched here; not changed by this PR).