Repository navigation
Conversation
|
Status
|
|
Warning Review limit reached
Next review available in: 5 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 (2)
WalkthroughHTTP/2 single-value header handling now ignores empty arrays during validation and encoding. Non-empty duplicate values remain invalid. Tests cover request, response, additional headers, push streams, and trailers. ChangesHTTP/2 header handling
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: 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/runtime/api/bun/h2_frame_parser.rs`:
- Around line 8009-8013: Update Http2Stream.sendTrailers() to detect when all
string-keyed trailer values are empty arrays and route that input through
noTrailers() before invoking the native trailer encoder. Preserve the existing
native validation path for undefined, null, non-array values, and objects
containing any non-empty value; the encoder’s value_iter handling should remain
unchanged.
🪄 Autofix
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: 493a92f9-943e-4992-b258-e19062bab88f
📒 Files selected for processing (3)
src/js/node/http2.tssrc/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
|
On the zero-length trailer block: the new test row does not fail. The The framing itself is also not something this PR introduces. On the current release, Node does differ on that pre-existing point: its native trailer submit sends an empty DATA frame when the encoded list is empty, so a node server's peer gets no |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow Node-compat fix that stops an empty-array header value from tripping the single-value rule.
What was reviewed:
assertSingleValueHeaders: the newfieldCountbranch preserves the multi-element and already-seen throws; only[]is newly skipped.- The three native
continues land right afterarray_iterator()and before thesingle_value_headers[idx]bookkeeping — nothing else in those loop bodies would have executed for a zero-length iterator, so no per-key side effect is skipped. - Tests cover all five entry points, both key orders, the still-throwing rows, and use fresh peers per row to avoid HPACK desync on the mid-encode-throw cases.
Extended reasoning...
Overview
The PR relaxes the HTTP/2 single-value-header check so that a header property whose value is an empty array is not counted as an occurrence of that header. It touches assertSingleValueHeaders() in src/js/node/http2.ts (the pre-encode JS guard used by request, respond, additionalHeaders, pushStream) and the three array branches in the native encoders in src/runtime/api/bun/h2_frame_parser.rs (send_trailers, push_promise, and the object walk in request). A new describe.concurrent block in test/js/node/http2/node-http2.test.js runs six header shapes through all five entry points against a live peer and asserts either the wire-decoded content-type values or the thrown error.
Security risks
None. The rule being relaxed is a compatibility guard, not a security invariant, and the relaxation applies only to [], which already sent nothing on the wire. Multi-element arrays and repeated case-variant names still throw exactly as before.
Level of scrutiny
Moderate — it touches native HTTP/2 header encoding, but the change at each of the three Rust sites is a single if value_iter.len == 0 { continue; } inserted immediately after the iterator is created and before the single_value_headers[idx] bookkeeping. I traced each loop body: for a zero-length array the subsequent while let Some(item) = value_iter.next()? loop would have done nothing anyway, so the only observable difference is skipping the single_value_headers[idx] = true write, which is exactly the fix. Name validation, pseudo-header checks, and (in push_promise) the never_index lookup all happen before the array branch and still run. On the JS side, refactoring to fieldCount = $isArray(value) ? value.length : 1 keeps the > 1 throw and the seen-set throw identical to before while adding only the === 0 skip.
Other factors
The PR description explains why both layers must change together (JS pre-check protects HPACK table sync; native is the only check for sendTrailers), and the tests reflect that by spinning up fresh peers per row so a mid-encode throw in the sendTrailers case can't poison later rows. Tests wire error/sessionError to reject, use port: 0, and race the failure promise against the client work. The change is a strict relaxation matching Node v26.3.0's buildNgHeaderString, covers the whole class (all five entry points), and includes negative rows proving the rule still fires when it should. This is small, mechanical, and well-tested.
|
Updated 12:54 AM PT - Aug 13th, 2026
❌ @robobun, your commit 1bd3e52 has some failures in 🧪 To try this PR locally: bunx bun-pr 37945That installs a local version of the PR into your bun-37945 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a focused Node-compat fix that stops counting empty-array header values as an occurrence of a single-value field.
What was reviewed:
assertSingleValueHeadersin http2.ts: thefieldCountrefactor preserves every existing throw path and only adds thefieldCount === 0skip;.lengthwas already read on the array before, so no new observable access.- All three native array branches (
send_trailers,push_promise, request object walk) get the same earlycontinuebeforesingle_value_headers[idx]is set — verified nothing after the array/non-array branch in each loop body is skipped. The fourthsingle_value_index_checkedsite (~L8906) is the raw[name, value, ...]form with no array-value handling, correctly left for #37796. - Tests cover all five entry points × six rows (including two that must still throw) against a real peer, and the CodeRabbit zero-length-trailer concern was checked and is pre-existing behavior this PR does not change.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap in node:http2: an empty-array value on a single-value header field (e.g. { "content-type": [], "Content-Type": "text/plain" }) was being counted as an occurrence, causing a spurious ERR_HTTP2_HEADER_SINGLE_VALUE throw. Node's buildNgHeaderString returns before touching its singles set for a zero-length array, so an empty array should send nothing and not block a case-variant key from supplying the actual value. The fix touches assertSingleValueHeaders() in src/js/node/http2.ts (the JS pre-encode guard) and the three native object-walk encoders in src/runtime/api/bun/h2_frame_parser.rs (send_trailers, push_promise, and the request/respond path), plus a comprehensive new test block in test/js/node/http2/node-http2.test.js.
Security risks
None. This relaxes an over-strict validation check for a case that already put nothing on the wire (the pre-existing zero-iteration loop encoded no bytes). No new user-controlled data reaches the encoder; the change only moves the continue earlier so the single_value_headers[idx] bookkeeping bit is not set. The .length property read on the user's array was already present in the pre-change condition, so no new observable side effect or prototype-pollution surface is introduced.
Level of scrutiny
Moderate — this is a Node-compat behavioral change in HTTP/2 header encoding, and the PR description explains the HPACK-desync risk of changing only one layer, so both layers had to move together. I verified: (1) the JS refactor is behavior-preserving for every non-empty case (scalar → fieldCount 1, array of 1 → fieldCount 1, array of >1 → throws, already-seen → throws) and only adds the fieldCount === 0 → continue; (2) each native continue sits immediately after array_iterator() and before any state mutation, and the array/non-array branch is the last thing in each loop body so nothing else is skipped; (3) the fourth single_value_index_checked call site at ~L8906 is the raw flat-pair form which has no per-value array branch and is explicitly deferred to #37796. The four assertSingleValueHeaders call sites (request, respond, additionalHeaders, pushStream) all gate on kStrictSingleValueFields unchanged.
Other factors
The tests are strong: five entry points each run six header objects against a fresh server/client pair and assert either the exact wire-decoded content-type list or the exact {name, code, message} of the throw, cross-checked against Node v26.3.0. Two rows are negative controls that must still throw, guarding against over-relaxation. CI passed on all lanes except a known unrelated bake dev-server flake. The CodeRabbit finding about zero-length trailer HEADERS frames was investigated and withdrawn — that framing is pre-existing (a non-single-value { "x-none": [] } already produced it on main via the zero-iteration loop) and both bun and nghttp2 accept it; a Node-shaped empty-encoding check is tracked separately. The comment-cop bot flags were addressed in 31ffd3b (comments trimmed to one line). All review threads are resolved.
…ue header In the object header form an empty array sends no field, but both the JS single-value pre-check and the native encoders (request, pushPromise, sendTrailers) still recorded it as an occurrence, so a second key for the same field (a case variant) threw ERR_HTTP2_HEADER_SINGLE_VALUE. Node's buildNgHeaderString returns before touching its singles set for a zero-length array; do the same in all four places.
31ffd3b to
1bd3e52
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is small and well-tested, but since it touches the native HTTP/2 HPACK encoding path in h2_frame_parser.rs alongside the node:http2 compat layer, a human look would still be worthwhile.
What was reviewed:
- The three native
continuesites match every object-form array branch that touchessingle_value_headers; the raw-pairs branch at ~8851 is intentionally out of scope per the description. - Confirmed the early
continueonly skips the single-value bookkeeping — the pre-existing zero-iterationwhilealready encoded nothing for[], so wire bytes are unchanged. assertSingleValueHeadersrefactor preserves the prior semantics for scalar values, one-element arrays, and multi-element arrays; only the empty-array case now skipsseen.add.- CodeRabbit's zero-length-trailer concern was checked and is pre-existing behavior this PR does not alter.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap in node:http2: an empty-array header value (e.g. { "content-type": [], "Content-Type": "text/plain" }) was being counted as an occurrence of a single-value field, causing ERR_HTTP2_HEADER_SINGLE_VALUE to fire when a case-variant key supplied the actual value. Node skips the empty array before touching its singles set. The fix is applied in two layers that must agree: the JS pre-check assertSingleValueHeaders() in src/js/node/http2.ts, and the three object-form array branches in the native encoders in src/runtime/api/bun/h2_frame_parser.rs (send_trailers, push_promise, and the request/respond object walk). ~16 lines of production code plus a ~200-line test block covering all five entry points × six header shapes.
Security risks
None identified. This is a relaxation of an over-strict validation guard, not a loosening of a security check. The relaxed case ([]) already sent zero bytes on the wire before this change (the while loop iterated zero times); the only difference is that the single-value bookkeeping bit is no longer set. No new untrusted-input parsing, no allocation, no change to what reaches the peer for inputs that were previously accepted.
Level of scrutiny
Moderate-to-high. The production diff is mechanical (three identical four-line early-continue insertions and a two-line JS refactor), and the test coverage is thorough and verified against Node v26.3.0. But h2_frame_parser.rs is a ~9k-line protocol state machine where the JS pre-check and native encoders must stay in lockstep to avoid HPACK table desync — the PR description itself explains why. That coupling, plus REVIEW.md's guidance that Node-compat changes warrant the situational checklist, puts this just outside the auto-approve bar for me even though I found nothing wrong with it.
Other factors
CI was green on all lanes except an unrelated Windows bake/deinitialization.test.ts teardown flake. The CodeRabbit finding about zero-length trailer HEADERS frames was investigated and withdrawn — that framing is pre-existing (a non-single-value { "x-none": [] } already produced it on main) and is tracked separately. The comment-cop feedback was addressed by trimming the inline comments to one line each. No prior claude[bot] reviews on this PR.
This branch has not been deployed
1 participant
Problem
client.request({ ":path": "/", "content-type": [], "Content-Type": "text/plain" })throwsTypeError [ERR_HTTP2_HEADER_SINGLE_VALUE]: Header field "content-type" must only have a single value. Node v26.3.0 sends onecontent-typefield.stream.respond(),stream.additionalHeaders(),stream.pushStream()andstream.sendTrailers()with the same object, in either key order.assertSingleValueHeaders()insrc/js/node/http2.tsadds the name to its seen set for any value that is not a multi-element array, and the array branches of the three native encoders insrc/runtime/api/bun/h2_frame_parser.rs(send_trailers(),push_promise(), the object walk inrequest()) set the field'ssingle_value_headersbit before looking at the array's length. Node'sbuildNgHeaderStringreturns for a zero-length array before it touches its singles set.Fix
assertSingleValueHeaders()skips a property whose value is an empty array; the three native array branchescontinueon a zero-length array before the single-value bookkeeping. Nothing else about the rule changes: a one-element array is still one occurrence, a multi-element array or a repeated name still throws, andstrictSingleValueFields: falsestill disables all of it.sendTrailers()has no JS check, so for it the native change is the whole fix.[name, value, ...]form (it has no array handling on main; node:http2: send array values in raw-form header lists as one field per element #37796 adds it together with this same skip),undefined/nullproperty values (bun rejects them, node skipsundefinedand sendsnullas "null"; tracked separately), the zero-length trailer block a trailers object made only of empty arrays still produces (pre-existing, accepted by nghttp2; node sends an empty DATA frame instead; tracked separately), and the fact that bun still validates the name of an empty-array field while node's early return skips that too.test/js/node/http2/node-http2.test.js, describehttp2 object-form headers: an empty array is not an occurrence of a single-value field. Six header objects (empty array first, empty array second, empty array plus a one-element array, two empty arrays, and two that must still throw) are run through each of the five entry points against a real peer, asserting on the content-type fields the peer decoded from the wire or the error thrown. All five tests fail on the current release (the first four rows throw on every entry point) and pass with this change; the same rows were run through node v26.3.0 and produce the listed outcomes.node-http2.test.js(362 pass),h2-conformance.test.ts(61 pass), and the portedtest-http2-{multiheaders,multiheaders-raw,single-headers-validation,single-headers-validation-disabled,sent-headers,sensitive-headers,raw-headers,raw-headers-defaults,info-headers,trailers,cookies,server-push-stream,server-push-stream-head,compat-serverresponse-headers,misc-util}.js, all passing.Background
content-type,content-length, every pseudo-header, ...;kSingleValueHeadersin http2.ts). The check is per lowercased name, so the only way to name the same field twice in an object is with two spellings of the key.strictSingleValueFields: falseturns the rule off.[]means no field. (An empty array is what header-building code such as{ "set-cookie": cookies }naturally produces.)request(),respond(),additionalHeaders()andpushStream()runassertSingleValueHeaders()in JS before calling into native, so a violation is reported before anything is encoded; each native encoder then applies the rule again while it walks the block, and forsendTrailers()that native walk is the only check.Probe output, before and after (debug build) and node v26.3.0
Input:
{ "content-type": [], "Content-Type": "text/plain" }(the reversed key order gives the same results).client.request()ERR_HTTP2_HEADER_SINGLE_VALUEcontent-type: text/plainoncestream.respond()stream.additionalHeaders()(102)'headers'block has it oncestream.pushStream()stream.sendTrailers(){ "content-type": ["text/plain"], "Content-Type": "text/html" }still throwsERR_HTTP2_HEADER_SINGLE_VALUEfrom all five entry points on this PR, as on main and on node.