Conversation
The response parser had 256 header slots. picohttpparser returns -1 when it runs out of slots, and `Response::parse_parts` reported every -1 as `Malformed_HTTP_Response`. A valid response with 257 fields failed with that error. - `MAX_RESPONSE_HEADERS` (2000) now sizes the parser scratch. It is a static in `.bss`, so a response still parses with no allocation. - `parse_parts` returns `ParseResponseError::TooManyHeaders` when the slots run out. fetch reports it as `ResponseHeadersTooLarge`. - Remove `content_encoding_i` and `did_set_content_encoding`. Nothing reads them, and the `u8` index cannot hold a position past 255.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe change adds explicit response-header overflow errors, applies a 1,000-header limit across HTTP/1, HTTP/2, and HTTP/3, updates content-encoding tracking, and adds boundary and fragmented-response tests. ChangesResponse header limits
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The response-header limit behavior is consistently handled and covered at the protocol boundaries. No current merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:02 AM PT - Sep 18th, 2026
✅ @robobun, your commit dee29fc2f7b7d6364ec59a5f95c2cdb03d91a065 passed in 🧪 To try this PR locally: bunx bun-pr 43181That installs a local version of the PR into your bun-43181 --bun |
|
Status: the fix is pushed (dee29fc) and waits for a maintainer. The open question is the number: the limit is 1000 fields, and it can go up after a release but not down. How I reproduced it: a raw PR: #43181 |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the TooManyHeaders classification in src/picohttp/lib.rs (picohttpparser resets *num_headers to 0 and only increments after a field parses, so num_headers == src.len() on -1 is the slot-exhaustion case), the larger SHARED_RESPONSE_HEADERS_BUF against the single-HTTP-thread RacyCell invariant (unchanged; only [0..num_headers] is exposed, so stale slots are never read), and the up-to-25 ms Headers build cost for a 2000-field response — that is the stated trade-off matching Node's maxHeadersCount default, not a bug.
Extended reasoning...
Two inline findings were reported (h2/h3 clients keep an uncapped field count; the WebSocket upgrade client keeps its own 128-slot buffer so it still rejects what fetch now accepts), so human review is already signalled. This note records what else was examined: the -1/num_headers == src.len() classification in parse_parts is correct given picohttpparser's counter semantics; the From<ParseResponseError> impl is now exhaustive so a future variant will fail to compile rather than silently fall through; the deleted content_encoding_i/did_set_content_encoding state had no reader beyond the block removed in the same diff; and the per-response cost of a 2000-field header block is a documented design choice rather than a defect.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/http/lib.rs— Users who opt into the HTTP/2 or HTTP/3 client still get an uncapped field count, so a server can force hundreds of milliseconds of quadratic Headers construction per response on the JS thread, the exact cost this PR's cap exists to bound. The h2 path enforces only LOCAL_MAX_HEADER_LIST_SIZE of 256 KB at src/http/H2Client.rs:23 and apply_multiplexed_headers at src/http/lib.rs:406 passes every field through unchanged. Fix: apply MAX_RESPONSE_HEADERS in apply_multiplexed_headers so one predicate covers HTTP/1.1, h2 and h3, returning ResponseHeadersTooLarge when the slice is longer.Extended reasoning...
src/http/lib.rs:1047 documents the reason for the new cap: building Headers grows with the square of the field count. That reasoning applies to every path that reaches WebCore__FetchHeaders__createFromPicoHeaders_ at src/jsc/bindings/bindings.cpp:2500. apply_multiplexed_headers at src/http/lib.rs:406 builds a picohttp::Response from a caller-supplied slice with no length check, and the only bound upstream is the HPACK list size of 256 KB at src/http/H2Client.rs:23. HPACK counts each field as name plus value plus 32 bytes, so about 7000 short fields fit. By the PR's own table 4096 distinct names cost 105 ms and 16,384 cost 1.8 s in release, so roughly 300 ms per response is reachable, and the debug/ASAN build is orders of magnitude slower. The population is every fetch with the h2 or h3 protocol option or BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT; the rate is per response from a hostile server. The dismissing finder relied on the PR text listing this path as not changed rather than on the cost argument the PR itself makes. Remedy: check headers.len() against MAX_RESPONSE_HEADERS at…
Verification: pre-existing — acknowledged in diff: the PR description's "Not changed" section states "The HTTP/2 and HTTP/3 clients cap header bytes (256 KB), not the field count"; that claim is accurate, and the base branch already has the same gap by the same route (the diff touches none of src/http/H2Client.rs, src/http/h2_client/, src/http/h3_client/). Triggering condition: the user opts into h2…
|
On the HTTP/2 and HTTP/3 finding: the fact is correct. I kept that check out of this PR for two reasons:
|
… HTTP/3 - `MAX_RESPONSE_HEADERS` is 1000, the count that Node's HTTP client enforces. The comment states it as a fixed limit on input that the server controls. - `apply_multiplexed_headers` applies the same limit to HTTP/2 and HTTP/3 responses. Both callers already fail only the stream on `Err`. - Tests: the 1000 and 1001 boundaries on all three protocols.
There was a problem hiding this comment.
I reviewed the updated push (limit unified at 1000 and extended to HTTP/2 and HTTP/3) and didn't find any bugs. Because this picks a new hard limit that HTTP/2 and HTTP/3 responses previously did not have, a maintainer should still weigh the number itself.
What was reviewed:
- The
-1classification insrc/picohttp/lib.rs:504: picohttpparser checks for the blank terminator line before the slot check, so exactly 1000 fields parse and only the 1001st tripsTooManyHeaders; a malformed field leavesnum_headersbelow capacity soMalformed_HTTP_Responseis preserved (the 290-field test covers this). - Every
ParseResponseErrormatch site:error.rs,WebSocketUpgradeClient.rs, and theErr(e) => e.into()arm atsrc/http/lib.rs:3730all handle the new variant; no remaining references to the deletedcontent_encoding_i/did_set_content_encoding. - HTTP/2 and HTTP/3 both route through
apply_multiplexed_headers, whose callers (h2_client/ClientSession.rs:1137,h3_client/ClientSession.rs:343) already fail only the stream onErr.
Extended reasoning...
Overview
The source diff is small (about 22 lines across src/http/lib.rs, src/http/error.rs, src/http/InternalState.rs, src/picohttp/lib.rs, and src/http_jsc/websocket_client/WebSocketUpgradeClient.rs): a MAX_RESPONSE_HEADERS = 1000 constant sizes the shared HTTP/1 scratch slot array and is checked in apply_multiplexed_headers for HTTP/2 and HTTP/3; a new ParseResponseError::TooManyHeaders variant is produced when picohttpparser returns -1 with every slot consumed and mapped to Error::ResponseHeadersTooLarge; the dead content_encoding_i u8 index and its flag are removed. Since the prior review the limit was changed from 2000 to 1000 and extended to the multiplexed clients, with matching h2/h3 tests.
Security risks
This is response-parsing code on network bytes the server controls. The change tightens rather than loosens: HTTP/1 previously rejected at 256 (as malformed) and now rejects at 1001 with a typed error; HTTP/2 and HTTP/3 previously had only a byte cap and now also have a field-count cap. The static scratch buffer grows from 256 to 1000 picohttp::Header entries (a fixed zeroed static, no per-response allocation). The Response slice construction still uses num_headers.min(src.len()), so the enlarged buffer cannot be over-indexed. No credential, TLS, or auth paths are involved.
Level of scrutiny
Moderate. The code is mechanically straightforward and I traced each piece: the picohttpparser control flow (terminator check precedes the slot check, count increments only after a field parses), the exhaustive From impl in error.rs, the catch-all Err(e) => e.into() in the HTTP/1 read path, the WebSocket arm, and both multiplexed callers' error handling. What I cannot settle from the code is the policy: 1000 is a maintainer decision (the description says as much), and applying it to HTTP/2/HTTP/3 is a new user-visible restriction on paths that previously accepted any count within 256 KB. That is why a human look is worthwhile rather than approving outright.
Other factors
The tests cover the exact boundaries on all three protocols (at the limit resolves, one past rejects), a header block split across two reads, and the malformed-line case, and use raw TCP / local Bun.serve only. The WebSocket upgrade client's separate 128-slot array was raised in the prior run as pre-existing and remains unchanged in this push; I did not repeat it. No CODEOWNERS entry covers src/http. The hunt exited on dry_streak with no findings.
At the maximum, every parser slot is in use when the first read ends. picohttpparser must then ask for more bytes (-2), not report -1. Cut the header block before and inside the blank line, and cut a block that is one field over the maximum after its last slot.
Problem
fetch()rejects a valid HTTP/1.1 response with more than 256 header fields. The error isMalformed_HTTP_Response. Node and curl accept it.SHARED_RESPONSE_HEADERS_BUF,src/http/lib.rs:1048), not a policy. picohttpparser returns -1 when the slots run out, andResponse::parse_parts(src/picohttp/lib.rs:501) reports every -1 as malformed.Fix
MAX_RESPONSE_HEADERS(1000) sizes the slots, a zeroed static, so no response allocates to parse. Past it,parse_partsreturns the newParseResponseError::TooManyHeadersandfetch()reportsResponseHeadersTooLarge, the error of the 1 MB byte cap. The deadcontent_encoding_i: u8index is deleted: au8cannot hold a position past 255.apply_multiplexed_headersapplies the same limit to HTTP/2 and HTTP/3, which had no field-count limit. Both callers already fail only the stream onErr.Background
fetch()for HTTP/1.1. The caller gives it an array of header slots. It returns -1 for a malformed message and -2 for an incomplete one.fetch()traffic.clone_metadatacopies the parsed fields out before the next response.apply_multiplexed_headersis where the HTTP/2 and HTTP/3 clients hand their decoded fields to the HTTP/1.1 response pipeline.Notes
Repro (loopback only, run with bun and with node):
1.4.3-canary.1+c6b7fcb5b: 254 resolves (256 fields with the two framing fields). 255 and 300 reject withMalformed_HTTP_Response.This was found by fuzzing. There is no user report.
Where the 256 came from. cd9b473 (2022, "Reduce memory usage of HTTP requests by 8 KB") replaced a
[128]array in each client with one shared[256]static. Nothing chose 256 as a limit on responses.Why a fixed limit and not an unbounded count. #34923 (closed unmerged by a cleanup of stale PRs) grew an overflow
Vecto the line count of the header block, so only the 1 MB byte cap bounded the field count. A fixed limit is the shape this code base already uses for input that the peer controls: the 1 MB byte cap in the same function ("a generous fixed cap"), and the request-side count cap inpackages/bun-uws/src/HttpParser.h, which #26130 raised as a constant. It also needs no growable container and no newunsafe.What other clients do.
MAX_HTTP_RESP_HEADER_COUNT) and the bytes at 300 KB.maxHeadersCount). The docs said 2000 until v26.9.0 corrected them. Since http: reject responses exceeding header limit nodejs/node#65010 it rejects a response past the limit. Before that it kept the first 1000 fields and dropped the rest with no error. This PR rejects, because a dropped field can beContent-LengthorTransfer-Encoding.fetch(undici) has no count limit and caps header bytes at 16 KB (UND_ERR_HEADERS_OVERFLOW). It accepts 3000 short fields, and it rejects the 1000-field response of this PR's test, which is larger than 16 KB.http.client(100), HAProxy (tune.http.maxhdr101) and Envoy (max_headers_count100) cap the count. Go and Chromium cap bytes only.Cost of a response at the limit.
HTTPHeaderMap, the store behindHeaders, finds a name with a linear scan, so the time to buildHeadersgrows with the square of the number of distinct names. The limit does not fix that, and this PR does not claim to. #35087 (closed) and #42250 (open) are changes toHTTPHeaderMapitself. These numbers are from the release build (best of 5),new Headers()plusappend, which uses the same map as a fetch response:The last column is the worst case that fits in the 1 MB byte cap. It grows in a straight line with the limit, which is one reason to start at 1000 and not higher. The work runs on the JS thread when the
Responseis made.The -1 classification. In
parse_headers(vendor/picohttpparser/picohttpparser.c) the field count goes up only after a field parses. A malformed field at index k returns -1 with the count at k, which is less than the capacity. The count equals the capacity on -1 in two cases: the slot check, and a bare CR directly after the line in the last slot. In the second case the response is malformed and also has no free slot, soTooManyHeadersis still a true statement.fetch()rejects in both cases.HTTP/2 and HTTP/3. Those clients bound header bytes (256 KB) and had no field-count limit, so this PR adds a restriction there: a response with more than 1000 fields now fails with
ResponseHeadersTooLarge. Pseudo-header fields such as:statusdo not count, because neither client puts them in the decoded list. On HTTP/2 the client resets the stream withCANCELand the session stays usable. Both clients are behindBUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENTor an explicitprotocoloption.Self-review. A review of this PR returned "merge after changes" with three required changes. All three are in: the limit is stated as a resource limit and its sources are corrected (Node enforces 1000, curl caps the count at 5000), the same limit covers HTTP/2 and HTTP/3, and the limit is 1000 with the number left to a maintainer. The verdict also named two further should-fix items. Their text was cut off in the result I received, and two attempts to recover it failed. From the reviewers' scratch files I reconstructed one: no test covered the limit across split reads. Those cases are now in the HTTP/1.1 file. I could not recover the second.
Not changed. The
WebSocketupgrade client has its own[picohttp::Header; 128](src/http_jsc/websocket_client/WebSocketUpgradeClient.rs). That is a separate bug with a separate fix. Here it only gains the match arm for the new variant, with the same result as before (Invalid response).Related PRs.
fetch-header-count-limit.test.ts. This PR only appends to that file.unsafefromsrc/http. On that branch the scratch isRefCell<Box<[picohttp::Header; 256]>>on the HTTP thread (src/http/HTTPThread.rs). The port of this PR to that branch is the constant, in that declaration and in the heap fallback for re-entry. That branch still hascontent_encoding_i.MAX_RESPONSE_HEADER_BUFFER. The two diffs do not touch the same lines.Tests. The new cases use
Set-Cookielines as filler.Headersstores them in a vector, so the cost is linear. The debug build with ASAN takes 22 s to buildHeadersfrom 2000 distinct names (release: 14 ms), which is too slow for a test.test/js/bun/http/fetch-header-count-limit.test.ts(HTTP/1.1): 256, 257 and 1000 fields resolve, with every cookie, theContent-Lengththat comes last, and the body. 1001 fields reject withResponseHeadersTooLarge. A header block that arrives in two reads resolves in three cases: 300 fields cut after 280 complete lines (BUN_DEBUG_fetch=1showshandleShortReadand then the second read), and exactly 1000 fields cut before and inside the blank line. In the last two every parser slot is in use when the buffer ends, so they pin that picohttpparser asks for more bytes (-2) before it checks the slots. 1001 fields cut after the 1000th line reject. A malformed line after 290 fields still rejects withMalformed_HTTP_Response.test/js/web/fetch/fetch-http2-adversarial.test.ts: a raw HTTP/2 server sends 1000 and 1001 fields in HEADERS plus CONTINUATION.test/js/web/fetch/fetch-http3-adversarial.test.ts:Bun.servewithhttp3: true. The test first reads how many fields the server adds on its own, then asks for exactly 1000 and 1001 fields in total.Without the fix (
USE_SYSTEM_BUN=1,1.4.3-canary.1+c6b7fcb5b): in the HTTP/1.1 file 7 of the 9 new cases fail, all withMalformed_HTTP_Response. The 256-field case and the malformed-line case pass. In the HTTP/2 and HTTP/3 files the 1001 cases fail, because the response resolves.Suites run on the debug build of this branch:
fetch-header-count-limit.test.ts(12),fetch-http2-adversarial.test.ts(22),fetch-http3-adversarial.test.ts(28),fetch-http3-client.test.ts(63),client-fetch.test.ts(34),fetch-redirect.test.ts(30),fetch-gzip.test.ts(80),fetch-compress.test.ts(32),fetch.brotli.test.ts(2),fetch_headers.test.js(10),proxy.test.ts(92),websocket-client-short-read.test.ts(4),websocket-handshake-event.test.ts(1),websocket-accept-header-validation.test.ts(2). All pass.fetch-http2-client.test.ts: 72 of 73 pass.POST: request body is delivered as DATA framestimed out at 5 s in the full run and passes alone in 4.4 s. It does not touch response headers.cargo clippyis clean onbun_picohttp,bun_httpandbun_http_jsc.[human-review] gate passed · iteration 0 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file