Conversation
…for <32 fields Node's parserOnHeadersComplete passes the full raw header list to _addHeaderLines with a separately clamped count, so req.rawHeaders stays complete while req.headers is clamped to server.maxHeadersCount. Only once the llhttp binding's 32-pair flush batch routes fields through parserOnHeaders does the raw list itself get clamped. The native server's lazy rawHeaders getter was slicing the raw list unconditionally, so proxies/loggers/signature verifiers that read req.rawHeaders lost fields that Node would have delivered. Clamp only the processed count for <32 fields and slice the raw list at >=32 fields, matching Node v26.3.0 exactly.
|
Status: diff is green; ready for review. Repro: CI (#77269): the new |
|
Updated 6:49 PM PT - Jul 21st, 2026
❌ @robobun, your commit e52e102 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35015That installs a local version of the PR into your bun-35015 --bun |
|
No related open issues found for this PR. 🤖 Generated with Claude Code |
Walkthrough
ChangesHTTP header clamping
Proxy test loopback routing
Possibly related PRs
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: 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 `@src/js/node/_http_incoming.ts`:
- Around line 221-228: Reduce the comment immediately before the _addHeaderLines
call to exactly three lines, preserving only the invariant that raw headers are
flushed in 32-pair batches and fewer than 32 pairs bypass parserOnHeaders.
Remove the surrounding explanations about parser limits, native delivery, and
proxy/logger behavior.
- Around line 230-236: Update the maxHeadersCount handling near fakeSocketSymbol
so the value is truncated or rejected when non-integer before calculating
maxHeaderPairs. Preserve the existing positive-value guard and ensure doubling
matches Node’s integer-sized << 1 behavior, keeping rawHeaders and kHeadersCount
on an even boundary.
In `@test/js/node/http/node-http.test.ts`:
- Around line 1927-1935: Add a complementary rawHeaders-first test case
alongside the existing headers-first coverage, reading and capturing
req.rawHeaders before accessing req.headers; then assert that headers remains
clamped to the configured maxHeadersCount and headersDistinct reflects the
expected clamped view.
🪄 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: d81a50e6-63c2-4d70-a3fc-77cfec49619c
📒 Files selected for processing (2)
src/js/node/_http_incoming.tstest/js/node/http/node-http.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node compat fix with matrix coverage across the 32-field boundary.
What was reviewed:
rawHeadersgetter now stores the full list and clamps onlykHeadersCount; slicing atcount >= 64reproduces Node's llhttp 32-pair flush;<< 1matches Node's fractional coercion.headers/headersDistinctstill iterate tokHeadersCount, so parsed views remain clamped in both access orders (matrix is rawHeaders-first, targeted test is headers-first).- Client/llhttp path (
_http_common.ts) already matches Node and is untouched; fresh server per test viabeforeEachsomaxHeadersCountdoesn't leak between matrix cases.
Extended reasoning...
Overview
This PR changes ~10 lines in the rawHeaders lazy getter in src/js/node/_http_incoming.ts (native-server path only) so that req.rawHeaders is no longer truncated to maxHeadersCount * 2 when fewer than 32 header fields arrive. Instead, the full raw list is stored and only kHeadersCount (which drives req.headers/req.headersDistinct iteration) is clamped. When ≥32 pairs arrive, the raw list is sliced to maxHeaderPairs, reproducing Node's llhttp kMaxHeaderFieldsCount batch flush through parserOnHeaders. New test.each cases in node-http.test.ts pin the exact (headerKeys, rawHeaders.length) pairs observed in Node v26.3.0 across the 31/32 boundary and both access orders.
Security risks
None introduced. The change retains more data in rawHeaders (matching Node) rather than exposing anything new; req.headers clamping is unchanged. maxHeaderPairs > 0 preserves the "0/negative means unlimited" semantics from before.
Level of scrutiny
Low-to-medium. This is a narrow Node.js compat fix in a single accessor, driven by empirical comparison against Node v26.3.0. The << 1 coercion matches Node's parser.maxHeaderPairs = server.maxHeadersCount << 1 (and Bun's own client path at _http_client.ts:989), including fractional truncation — covered by the max: 3.7 matrix row. The headers/headersDistinct getters read this.rawHeaders then loop to this[kHeadersCount], so the headers-first order still sees the clamped count while rawHeaders returns the full stored list; both orders are tested.
Other factors
All three CodeRabbit comments (comment length, << 1 vs * 2, rawHeaders-first coverage) were addressed in 7cce553 and marked resolved. The client/llhttp path in _http_common.ts already implements the batched clamp and is untouched, so there's no sibling site left unfixed. Tests use the existing HTTP Server Security Tests - Advanced harness (fresh new Server() per test via beforeEach, sendRequest wires socket error to reject), so server.maxHeadersCount mutations don't leak across matrix rows. Vendored test-http-rawheaders-limit.js and test-http-max-headers-count.js both exist and per the PR continue to pass.
On hosts where localhost resolves to ::1 first, listen(0, "localhost") binds IPv6-only while the client request lands on 127.0.0.1 and is refused. exampleSite already pins 127.0.0.1; do the same here so the fixture is deterministic regardless of the host's dual-stack ordering.
There was a problem hiding this comment.
LGTM — focused Node compat fix with a verified test matrix.
What was reviewed:
- The
rawHeadersgetter change:kHeadersCountis still clamped soreq.headers/headersDistinctstay limited; only the raw list retention widens, and only below the 32-pair boundary — matches the Node v26.3.0 table in the description. - Checked the llhttp client path in
_http_common.ts(parserOnHeadersCompletealready passes the full list to_addHeaderLineswith a clampedn) — correctly left unchanged. - Confirmed
test-http-rawheaders-limit.js(66 headers, max=50) still holds under the newcount >= 64slice, and the new matrix covers both access orders plus the fractionalmaxHeadersCountcoercion via<< 1.
Extended reasoning...
Overview
This PR fixes IncomingMessage.prototype.rawHeaders on the native node:http server path so that req.rawHeaders is not truncated to server.maxHeadersCount * 2 for requests with fewer than 32 header fields, matching Node's actual behavior where only the count fed to _addHeaderLines is clamped in that case. The change is a ~13-line rewrite inside one accessor in src/js/node/_http_incoming.ts, plus a new describe block in node-http.test.ts and an unrelated localhost → 127.0.0.1 bind in the proxy fixture.
Security risks
None. The fix widens what rawHeaders retains (up to 31 pairs) while keeping req.headers and headersDistinct clamped to maxHeadersCount via the unchanged kHeadersCount iteration bound. The >= 32 pairs slice path is preserved, so the DoS-protection intent of maxHeadersCount is intact — verified against the vendored test-http-rawheaders-limit.js (66 headers → rawHeaders.length still ≤ 100). The << 1 coercion matches Node's own parser.maxHeaderPairs = server.maxHeadersCount << 1, so fractional/negative/zero inputs behave identically to before (the maxHeaderPairs > 0 guard preserves the old "0 means no limit" semantics).
Level of scrutiny
Medium — Node compat correctness in a hot server path, but the change is mechanical and well-localized. The 32-pair threshold is a magic number, but it's documented against kMaxHeaderFieldsCount in node_http_parser.cc and the PR encodes a test matrix of (sent, max) → (headerKeys, rawLen) pairs that were empirically verified against Node v26.3.0, which is exactly how the repo's Node-compat guidance says to derive behavior. The headers/headersDistinct getters iterate to kHeadersCount, so their clamping is unaffected. The client-side llhttp path in _http_common.ts already had the correct split (full headers list, clamped n to _addHeaderLines) and was correctly left alone.
Other factors
All three CodeRabbit findings were addressed in follow-up commits (comment tightened, << 1 coercion, rawHeaders-first ordering in the matrix). The test.each matrix reads rawHeaders first and the standalone test reads headers first, so both materialization orders are covered. The beforeEach creates a fresh Server per case so there's no listener leakage across the matrix. The node-http-proxy.js change is a benign test-fixture hardening (explicit IPv4 bind). No open reviewer threads remain.
|
Closing in favor of #42532. Node no longer truncates a request that is over What #42532 carries from this PR:
I sent the requests of the test matrix in this PR (12 to 66 fields, both access orders, |
2 participants
What does this PR do?
With
server.maxHeadersCountset, Bun'snode:httpserver was truncatingreq.rawHeaderstomaxHeadersCount * 2entries. Node only clamps the count fed to_addHeaderLines(which buildsreq.headers), soreq.rawHeadersstays complete for requests with fewer than 32 header fields; proxies, request loggers, and header-signature verifiers that readreq.rawHeaderswere silently losing fields.Cause
IncomingMessage.prototype.rawHeaders(the native server's lazy materialization path in_http_incoming.ts) sliced the raw list tomaxHeadersCount * 2before storing it, instead of storing the full list and only clampingkHeadersCount.Node's actual behavior has a split: the llhttp binding flushes headers to
parserOnHeadersin 32-pair batches (kMaxHeaderFieldsCountinnode_http_parser.cc), andparserOnHeadersclamps the accumulated list. Fewer than 32 pairs arrive in a singleparserOnHeadersCompletecall whoseheadersargument is the full list and bypassesparserOnHeaders, so only the processed count is clamped. Verified against Node v26.3.0:req.headerskeysreq.rawHeaders.lengthFix
In the
rawHeadersgetter, store the full raw list and clampkHeadersCounttomaxHeadersCount * 2. Slice the raw list itself only when it carries 32 or more pairs, reproducing Node's split.req.headersandreq.headersDistinctiterate tokHeadersCountso they remain clamped. The llhttp-based client path (_http_common.ts) already matches Node and is unchanged.How did you verify your code works?
New
server.maxHeadersCountcases intest/js/node/http/node-http.test.tsassert the exact(headers, rawHeaders.length)pairs observed in Node v26.3.0 across the 32-field boundary and for both access orders. Vendoredtest-http-rawheaders-limit.jsandtest-http-max-headers-count.jscontinue to pass.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file