fetch: accept HTTP/1.1 responses with more than 256 header fields - #34923
fetch: accept HTTP/1.1 responses with more than 256 header fields#34923robobun wants to merge 3 commits into
Conversation
picohttpparser returns -1 when the caller-supplied header array fills before the terminating CRLF, which the Rust wrapper reported as Malformed_HTTP_Response. The HTTP client passed a fixed 256-slot scratch array, so any origin sending more than 256 Set-Cookie / rate-limit / debug header lines was rejected as malformed even though curl, Node/undici and bun's own node:http.get all accept it. Distinguish the buffer-full case as ParseResponseError::TooManyHeaders and, when it fires, reparse into an HTTP-thread scratch Vec sized to the line count of the received bytes. The existing 1 MB byte cap on the response header block remains the only hard limit, matching llhttp's byte-only bound.
|
Warning Review limit reached
Next review available in: 1 minute 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 (5)
Comment |
|
Status: diff is green; waiting on a maintainer. CI build 76842: 194/196 lanes passed; |
|
Updated 7:08 AM PT - Jul 21st, 2026
❌ @robobun, your commit 6b2fb97 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34923That installs a local version of the PR into your bun-34923 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
On the linked issues:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits: checked that every ParseResponseError match site handles the new TooManyHeaders variant (three WebSocket arms updated; http::Error::from has a _ => catch-all so a second TooManyHeaders on reparse surfaces as Response_Headers_Too_Large), and that amount_read stays 0 across the first-parse → reparse (the -1 branch in parse_parts doesn't touch *offset). Traced the overflow-Vec borrow through the 1xx continue and clone_metadata paths — no UAF, just the stale SAFETY comment noted inline.
Extended reasoning...
The two inline findings are nits (documentation accuracy on an unsafe block; bounded over-sizing of a process-lifetime scratch Vec). This note records the additional exhaustiveness/soundness checks so a human reviewer doesn't re-derive them: all four ParseResponseError consumers cover the new variant, the retry's last_len is correctly 0, and the lifetime-erased borrow into the growable overflow Vec is reset before any subsequent resize(). Not approving — the change adds unsafe lifetime erasure into a growable container on the HTTP client hot path, which per REVIEW.md's memory-safety section warrants a maintainer look.
…FETY Count newlines only up to the first CRLF CRLF so a newline-dense body delivered in the same read as a >256-field header block does not inflate the overflow scratch. Update the detach_lifetime SAFETY comment to name the overflow Vec and the actual invariant.
There was a problem hiding this comment.
Both earlier concerns (SAFETY comment, body-newline over-sizing) are addressed in bc80950. Beyond the inline nit, I also checked: a second TooManyHeaders from the reparse falls into Err(e) => close_and_fail(e.into()) and surfaces as Response_Headers_Too_Large (no unhandled variant); all four picohttp::Response::parse* call sites (http/lib.rs, three WebSocketUpgradeClient arms, error.rs From impl) handle the new variant; and the picohttpparser -1 && num_headers == max_headers ⇔ overflow invariant holds against the reference impl (the max_headers check runs before parsing each header, so every non-overflow -1 leaves num_headers strictly below capacity).
Extended reasoning...
Prior review's two findings were addressed in bc80950 and the threads are resolved. This run's bug-hunt surfaced one nit (content_encoding_i: u8 can now wrap on >255-field responses), which is dead state — grep confirms its only reader is the vestigial block at lib.rs:3874-3882 and actual decompression is driven by state.encoding, not the index. The note above records the additional checks done this pass so a human reviewer doesn't re-derive them; the change still touches unsafe lifetime-detached HTTP-thread scratch, so leaving the final call to a human.
The field was a u8 header index whose only reader was a no-op block that rewrote itself and re-stored pending_response with an unchanged value. With responses now able to carry more than 255 header fields the u8 narrowing would silently wrap, so delete the field, the flag, the four writes, and the vestigial block instead of widening a value nothing consumes. Decompression is driven by state.encoding, which is unchanged.
There was a problem hiding this comment.
All three rounds of prior feedback are addressed as of 6b2fb97 (SAFETY comment names the overflow Vec and the reset-before-resize invariant; overflow slot count is bounded to the header block via index_of("\r\n\r\n"); the dead content_encoding_i/did_set_content_encoding state is fully removed — grep confirms no remaining references). I found nothing new this pass, but this touches the HTTP client's unsafe detach_lifetime path with borrows into a new growable process-lifetime static, so a human sign-off is warranted.
What was reviewed:
- picohttpparser
-1overflow classification:num_headers == max_headersonly on the header-array-full path; every other-1(including status-line failures, which resetnum_headers = 0before parsing) leaves it strictly below. The!src.is_empty()guard covers the degenerate zero-slot case. - Reparse fallback: if the overflow reparse itself returns
TooManyHeaders(shouldn't — newline count ≥ field count), it falls through toErr(e) => close_and_fail(e.into())via the existing wildcardFrom<ParseResponseError>. - Loop invariant:
pending_response = Some(Response::default())at line 3730 runs before anyoverflow.resize()on the next iteration, so the erased borrow into the Vec's old buffer is never read after a realloc. - Dead-state removal: no remaining readers of
content_encoding_i/did_set_content_encodinganywhere insrc/.
Extended reasoning...
Overview
This PR lifts the fixed 256-field cap on HTTP/1.1 response headers in fetch(). It adds a ParseResponseError::TooManyHeaders variant so picohttp::Response::parse_parts can distinguish header-array overflow from a genuinely malformed response, then has handle_on_data_headers (src/http/lib.rs) reparse into a new HTTP-thread Vec<Header> static sized to the header-block line count. The WebSocket upgrade client's three parse sites map the new variant to the existing InvalidResponse code. A follow-on commit deletes the vestigial content_encoding_i: u8 / did_set_content_encoding state whose u8 value space this change would otherwise have overflowed.
Security risks
The new path is bounded: the overflow Vec is sized from newlines in buf[..index_of("\r\n\r\n")], and the header block itself is capped at 1 MB by the pre-existing MAX_RESPONSE_HEADER_BUFFER check on the ShortRead branch. Worst case a hostile server can induce is ~8 MB of retained scratch (≈250K four-byte header lines × 32 B/slot), which is bounded and requires the 256-slot fixed buffer to overflow first — not a DoS vector of note. No new untrusted-input parsing is introduced (same picohttpparser call, larger scratch).
Level of scrutiny
High. The change threads a borrowed &[Header] into a growable Vec's heap buffer through unsafe { response.detach_lifetime() } and stores it in self.state.pending_response across a loop that can resize() that Vec on the next iteration. I traced the invariant (line 3730 overwrites pending_response with Response::default() before any subsequent resize; clone_metadata() deep-copies before the fn returns; the 1xx-continue path nulls pending_response before continue) and it holds — but this is exactly the class of pointer-into-growable-container hazard REVIEW.md's memory-safety section flags, and the SAFETY comment now correctly documents it. A maintainer should confirm they're comfortable with the shape.
Other factors
- All three prior review findings (stale SAFETY comment, body-newline over-sizing,
u8index wraparound) were addressed in bc80950 and 6b2fb97; each fix is verified in the current diff. - Test coverage is good: 200/255/300/1000-field responses with first/last-field spot-checks, an 8 KB LF-dense body in the same write, and a malformed-response negative case. The PR evidence shows the new tests fail on main (Malformed_HTTP_Response at 255+) and pass on the branch under both ASAN-debug and release.
- The dead-state deletion in
InternalState.rsandhandle_response_metadatais complete (grep-clean) and decompression is unaffected (state.encodingdrives it).
1 participant
Repro
255 fields resolve, 256 and up are rejected as malformed even though the bytes are valid HTTP/1.1. A single 1 MB header value is accepted, so this is a field-count cap, not a byte guard. curl, Node/undici, and bun's own
node:http.getall accept 1000+ fields. This hits real origins that send manySet-Cookielines or per-item cache/rate-limit/debug headers.Cause
picohttpparser returns
-1both for genuinely invalid input and whennum_headersreachesmax_headersbefore the terminating CRLF;picohttp::Response::parse_partsmapped every-1toParseResponseError::MalformedHttpResponse. The HTTP client passes a fixed[picohttp::Header; 256]scratch (SHARED_RESPONSE_HEADERS_BUF), so the 257th field fills it and the response is rejected with a misleading error.Fix
parse_partsdistinguishes the overflow case (on-1picohttpparser writesnum_headers == max_headersonly when the array filled; every other-1path leaves it strictly below) and returns a newParseResponseError::TooManyHeaders.handle_on_data_headersretries into an HTTP-thread scratchVec<picohttp::Header>sized to the\ncount of the received bytes (a strict upper bound on the field count). The existing 1 MB byte cap (MAX_RESPONSE_HEADER_BUFFER) stays as the only hard limit on the header block, which matches llhttp's byte-only bound in Node.TooManyHeadersthe same asMalformedHttpResponse(a 101 handshake with >128 fields is still rejected, just via the existingInvalidResponsecode rather than a compile break).The common case (≤256 fields) is unchanged: one parse into the fixed array, no allocation, no line-count scan.
Verification
New
describeintest/js/bun/http/fetch-header-count-limit.test.tscovers 200/255/300/1000 response header fields plus a genuinely malformed response that must still reject withMalformed_HTTP_Response.fetch-redirect.test.ts,headers.test.ts,fetch_headers.test.js, andclient-fetch.test.ts(151 tests) pass unchanged.[review] gate passed · iteration 1 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 1
evidence per changed file