fetch: send every request header instead of silently dropping past the 250th - #36433
fetch: send every request header instead of silently dropping past the 250th#36433robobun wants to merge 7 commits into
Conversation
…e 250th build_request() wrote user headers into a fixed 256-slot per-thread scratch array (minus 6 reserved for defaults), silently dropping any header past the 250th. The request then resolved 200 with those fields missing on the wire, so a late Authorization or signature header just vanished with no error. Mirror the response-side fix: keep the 256-slot inline array for the common case and spill to a per-HTTP-thread Vec sized from the user header count when it would overflow. Every user header is now written, matching Node/undici which have no request-side field-count cap. The will_append guard and MAX_USER_HEADERS bound are gone since the buffer is always large enough.
|
Status Reproduced with the loopback capture in the PR body: 300 user headers in, 250 on the wire (255 fields), no error. After the fix all 300 arrive.
|
WalkthroughChangesHTTP header buffering
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/http/lib.rs`:
- Around line 2372-2383: Audit request header validation and construction paths
surrounding the header buffer sizing, including JS Headers/fetch option handling
and proxy or tunnel requests, to confirm whether a header-count or
aggregate-size cap is required after the request-side field-count cap removal.
If an upstream limit exists, enforce or reuse it before `header_names` reaches
the sizing logic; otherwise preserve the current `needed` calculation and
document the validated unbounded 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: 4b5a203a-b5d8-4c45-b91f-bd42f3a9d3ef
📒 Files selected for processing (2)
src/http/lib.rstest/js/web/fetch/fetch_headers.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that swaps the fixed 256-slot request-header scratch for an overflow Vec when header_names.len() + 6 exceeds it, removing the silent 250-header truncation.
What was reviewed:
- Bounds: post-loop appends max out at 6 defaults (Connection/UA/Accept/Host/Accept-Encoding + one of Content-Length|Transfer-Encoding), so
header_count <= neededand the slice index can't overrun. - Lifetime erasure over the Vec-backed slice: all four
build_request()call sites (lib.rs:2985, lib.rs:3475, H2Client.rs:136→ClientSession.rs:448, h3_client/encode.rs:48) serialize the returnedRequestimmediately before any subsequentbuild_request()could resize; same invariant the pre-existing static-arraydetach_lifetimealready relied on. will_appendremoval is safe now that the buffer is always large enough; override flags revert to unconditional as they were before the cap was added.- Tests cover the boundary (251) and well past it (300), assert exact header set equality, and were verified to fail on the released binary.
Extended reasoning...
Overview
The PR fixes silent request-header truncation in fetch(): build_request() in src/http/lib.rs previously wrote user headers into a fixed 256-slot per-HTTP-thread scratch array, reserving 6 slots for defaults and dropping anything past index 250 with no error. The fix keeps the inline array as the fast path and spills to a per-HTTP-thread Vec<picohttp::Header> sized to header_names.len() + MAX_DEFAULT_HEADERS when needed. The now-redundant will_append guard and MAX_USER_HEADERS bound are removed. Two it.each([251, 300]) regression tests in test/js/web/fetch/fetch_headers.test.js capture the raw wire bytes via a net.Server and assert every x-NNNN header arrives.
Security risks
None introduced. The change removes a silent-drop path; there is no new parsing of untrusted input, and the request-side has no field-count cap (matching Node/undici — servers enforce their own limits). The overflow Vec is sized from the caller-supplied header count, which is already bounded by whatever the JS side constructed.
Level of scrutiny
Moderate. This is core HTTP client code with an unsafe { detach_lifetime(...) } over the returned header slice, and the backing storage now includes a growable Vec whose buffer moves on resize. However, the SAFETY invariant is unchanged from the pre-existing static-array case: the erased 'static slice was already only valid until the next build_request() overwrote the array. The Vec resize is the same hazard in a different shape. I traced all four call sites (h1 initial send, h1 on_writable, h2 ClientSession::attach, h3 encode) and each serializes via write_request/encode::write_request immediately, on the single HTTP thread, before another build_request() can run. The updated SAFETY comment documents this correctly.
Other factors
- Bounds are provably safe: the user-header loop writes at most
header_names.len()entries (somecontinueearly), then at most 6 defaults are appended — I read lines 2463–2531 to confirm the sixth slot is Content-Length XOR Transfer-Encoding, never both. Soheader_count <= neededalways holds. - The
scratch::request_headers_overflow()accessor follows the exact pattern of the four existing accessors inmod scratchand is covered by the same module-level INVARIANT comment (single HTTP thread, sole live borrow). - The tests follow the file's existing raw-socket capture pattern (same as the latin-1 wire test just above), use
Promise.withResolverswith error wired to reject, close the server infinally, and assert exact set equality on the received header lines rather than just a count. - Minor: the SAFETY comment in
H2Client.rs:129-135still names onlySHARED_REQUEST_HEADERS_BUF, not the overflow Vec — a doc-staleness nit, not a correctness issue, since the invariant holds identically for both buffers.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The test at test/js/bun/http/fetch-header-count-limit.test.ts asserted the old behaviour (300 headers in, 250 on the wire). Update it to expect every header to reach the origin and retitle the overflow-path test for what it now exercises.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/http/fetch-header-count-limit.test.ts (1)
96-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the custom special-header values, not only their names.
These checks also pass if the user values are dropped and default
Host,User-Agent, andAcceptheaders are emitted. Return the parsed values from the server and assert the exact custom values.As per coding guidelines, tests must prove the intended reason and assert the strongest invariant.
🤖 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 `@test/js/bun/http/fetch-header-count-limit.test.ts` around lines 96 - 99, Update the assertions in the fetch header-limit test to validate the parsed header values returned by the server, not just the presence of “host”, “user-agent”, and “accept” names. Assert each header matches the exact custom value configured by the test, preserving the existing header-name checks if useful.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 `@test/js/bun/http/fetch-header-count-limit.test.ts`:
- Around line 76-87: Align the test description with the setup in “user-supplied
Host/User-Agent/Accept are sent alongside >250 other headers” by either renaming
it to state exactly 250 other headers or changing the loop to generate 251; keep
the existing boundary-path assertions unchanged.
---
Outside diff comments:
In `@test/js/bun/http/fetch-header-count-limit.test.ts`:
- Around line 96-99: Update the assertions in the fetch header-limit test to
validate the parsed header values returned by the server, not just the presence
of “host”, “user-agent”, and “accept” names. Assert each header matches the
exact custom value configured by the test, preserving the existing header-name
checks if useful.
🪄 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: decd27cc-a97e-42a0-baed-d13c41af3f65
📒 Files selected for processing (3)
src/http/H2Client.rssrc/http/lib.rstest/js/bun/http/fetch-header-count-limit.test.ts
The previous assertions only checked the header names were present, which also passes under the old 250-cap behaviour (the defaults are appended instead). Return the parsed values from the raw server and assert the user's custom values. Also bump the filler to 251 so the test title is literally accurate.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/http/fetch-header-count-limit.test.ts:94-99— The renamed title claims "user-supplied Host/User-Agent/Accept are sent", but the assertions still only checkheaderNames.toContain("host"|"user-agent"|"accept")— presence of the name, not the user-supplied value. Under the pre-PR behavior these names were also present (as Bun's defaults, since the override flags weren't set for dropped headers), so this test passes both with and without the fix. Either assert the received values (custom-host.example.com,custom-agent,text/html) or drop the test — thecustomCount === 300case above and the newit.each([251, 300])cases already cover the overflow path.Extended reasoning...
What the bug is
The PR renamed this test from "default headers preserved when user headers overflow the buffer" to "user-supplied Host/User-Agent/Accept are sent alongside >250 other headers", and rewrote the setup comment to say the special headers "exercise the overflow path". But the assertions were left byte-identical to
main:expect(headerNames).toContain("host"); expect(headerNames).toContain("user-agent"); expect(headerNames).toContain("accept");
These check only that a header named
host/user-agent/acceptreached the origin. They cannot distinguish the user-suppliedHost: custom-host.example.comfrom Bun's defaultHost: 127.0.0.1:<port>. The raw server helper (makeRawHttpServer) only returnsheaderNames, not values, so the valuescustom-host.example.com/custom-agent/text/htmlset on lines 88–90 are never observed anywhere in the test.Why the assertions can't fail on the pre-PR behavior
Tracing the removed
will_appendguard from the diff:- The 250
a-*headers fill user slots 0–249. Host/User-Agent/Acceptiterate withheader_count >= 250, sowill_append = false.- The override flags were wrapped in
if will_append { override_host_header = true; }etc., so they stay false. if !will_append { continue; }drops the user headers.- After the loop, because the override flags are false, the default Host/User-Agent/Accept are appended.
Result on
main:headerNamescontains"host","user-agent","accept"(the defaults) → test passes. This is exactly what the original title ("default headers preserved when user headers overflow") documented, and why these assertions existed.After this PR: the user headers are written via the overflow Vec, override flags are set, defaults are suppressed →
headerNamescontains"host","user-agent","accept"(the user's) → test passes.Step-by-step proof
- Run this test with
USE_SYSTEM_BUN=1 bun test test/js/bun/http/fetch-header-count-limit.test.ts -t "user-supplied Host"(released binary, no fix). - 250
a-*headers + defaults arrive; user Host/UA/Accept are dropped; default Host/UA/Accept appended. headerNames=["a-0000", …, "a-0249", "connection", "user-agent", "accept", "host", "accept-encoding"].- All three
.toContain()assertions pass. - Run with
bun bd test …— same three assertions pass for the same reason.
Per REVIEW.md: "a test that passes both ways is worse than no test" and "assert the strongest invariant … actual bytes not lengths". CLAUDE.md's testing section is explicit: "Your test is NOT VALID if it passes with
USE_SYSTEM_BUN=1."Impact
Low — this is a test-quality issue, not a runtime bug. The overflow fix itself is guarded elsewhere in this PR: the updated
customCount === 300assertion in the same file (line 56) and the newit.each([251, 300])cases infetch_headers.test.jsboth fail on the released binary withReceived: 250. So merging as-is doesn't leave the fix untested; it just leaves one test whose title promises something its body doesn't verify.How to fix
Either:
- Assert the values. Extend
makeRawHttpServerto also return the raw header lines (it already has them inlines), then assert e.g.expect(headerLines).toContain("host: custom-host.example.com")/"user-agent: custom-agent"/"accept: text/html". This makes the test fail on the released binary (which would sendhost: 127.0.0.1:<port>etc.) and actually verify what the title says. - Or delete the test. The scenario it was originally written for — "override flags set but header dropped, leaving Host entirely absent" — is structurally impossible now that
will_appendis gone, and the overflow path is already covered by the two other tests.
- The 250
|
Re the additional finding on |
There was a problem hiding this comment.
No issues found, but this touches the unsafe lifetime-erased scratch storage in the HTTP client hot path — worth a human confirming the new overflow-Vec invariant.
What was reviewed:
- Sizing:
header_countis bounded byheader_names.len() + 6on every path (2continues, 5 conditional defaults, 1 mutually-exclusive CL/TE) — no OOB into either the 256-slot inline array or theneeded-sized overflow slice. - Lifetime: verified all four
build_requestcallers (h1on_writable×2, h2ClientSession::attach, h3encode) serialize the returnedRequestsynchronously before any subsequent call could resize the overflowVec, so the erased'staticborrow never dangles. - Pattern mirrors the response-side overflow from #34923; the
RacyCellINVARIANT (single HTTP thread) covers the new accessor.
Extended reasoning...
Overview
Removes the silent 250-header cap in fetch() request building. src/http/lib.rs build_request() previously wrote into a fixed 256-slot SHARED_REQUEST_HEADERS_BUF and continued past MAX_USER_HEADERS = 250, dropping headers with no error. The fix keeps the 256-slot inline array for the common case and adds a per-HTTP-thread SHARED_REQUEST_HEADERS_OVERFLOW: RacyCell<Vec<Header>> sized to header_names.len() + 6 when needed. The will_append guard and MAX_USER_HEADERS constant are removed; override flags (Host/UA/Accept/etc.) are now set unconditionally since the header is always written. H2Client.rs updates its SAFETY comment to name the overflow storage. Two test files gain/tighten coverage.
Security risks
None identified. This is outbound request construction — the header count is controlled by the caller who has already materialized every header in self.header_entries/self.header_buf, so the overflow Vec is constant-factor scratch on already-committed memory. No new parsing of untrusted input.
Level of scrutiny
Medium-high. The change itself is small and mirrors #34923's response-side pattern, but it modifies unsafe { detach_lifetime(..) } semantics: the returned Request<'static> may now borrow a Vec buffer that moves on the next resize(), whereas the fixed array never moved. The SAFETY comment documents the new invariant ("Callers serialize the returned Request before the next build_request() call resizes"). I traced all four call sites — lib.rs:2983/3473 (h1), ClientSession.rs:448 (h2), h3_client/encode.rs:48 (h3) — and each serializes into a writer/QPACK buffer synchronously on the single HTTP thread before returning, so the invariant holds today. A maintainer familiar with the HTTP thread model should confirm this is the intended contract going forward.
Other factors
- I verified the sizing arithmetic: the loop writes ≤
header_names.len()entries (Content-Length and non-streaming Transfer-Encodingcontinue), then appends ≤ 5 conditional-once defaults plus exactly one CL/TE branch — worst caseheader_names.len() + 6 = needed, sorequest_headers_buf[header_count]never indexes past the slice. - Test coverage is solid:
it.each([251, 300])infetch_headers.test.jsasserts exact wire contents against a raw TCP server;fetch-header-count-limit.test.tsnow asserts the user-supplied Host/UA/Accept values (not just presence) reach the origin through the overflow path. PR evidence shows both fail on main withReceived: 250. - My earlier inline comment (accidental revert in cd1bc69) was addressed by 290e308; all bot threads are resolved.
- The overflow Vec is never shrunk, so a single large request leaves per-thread scratch allocated — same trade-off as #34923's response side, and proportional to memory the caller already committed.
2 participants
What
fetch()silently dropped every user request header past the 250th. The request still went out and resolved 200, but with those fields missing on the wire. A lateAuthorization, signature, or forwarded header just vanished with no error and no warning.Repro
Before:
status 200: sent 300 user headers, origin received 250 (255 fields total)Node/undici:
status 200: sent 300 user headers, origin received 300 (307 fields total)Cause
src/http/lib.rsbuild_request()wrote user headers into the fixed 256-slotSHARED_REQUEST_HEADERS_BUFscratch array withMAX_USER_HEADERS = 256 - 6reserved for defaults, and the loop did// Silently drop excess headers to stay within the fixed-size request header buffer.on overflow. Request-side sibling of the response-side cap fixed in #34923, except this one never surfaced an error.Fix
Mirror #34923: keep the 256-slot inline array for the common case and spill to a per-HTTP-thread
Vec<picohttp::Header>sized fromheader_names.len() + MAX_DEFAULT_HEADERSwhen that would overflow. Every user header is written. Thewill_appendguard andMAX_USER_HEADERSbound are gone since the buffer is always large enough now. No request-side field-count cap, matching Node/undici.Verification
The new cases fail on the released binary with
Expected: 251 / Received: 250andExpected: 300 / Received: 250.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file