Repository navigation
Conversation
|
Status
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughHTTP/2 header processing now skips undefined values across request, response, push-promise, informational, and trailer paths. Empty trailers end streams with an empty DATA frame. Tests cover transmission, validation, and session behavior. ChangesHTTP/2 header handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — well-scoped Node compat fix with thorough wire-level tests.
What was reviewed:
- The three native object walks now skip
undefinedbefore name validation, matching node'sbuildNgHeaderString;nullhandling is unchanged on every path. - The empty-trailers early return in
send_trailers()mirrorsno_trailers()exactly (wait_for_trailers = false+ empty DATA/END_STREAM viasend_data);&mut streamderef-coercesGuardedStream → &mut Streamcorrectly. - All five JS pre-checks that mirror the encoder's classification skip
undefinedin step;writeEarlyHints()now validates hints itself (assertValidHeader + token check on trimmed/lowercased name) and delegates towriteInformation(), preserving the compat-layer rejection that test-http2-compat-write-early-hints-invalid-header.js pins. - Seven new tests assert on peer-decoded field lists and wire error events to reject;
pushedHeaderBlockshelper exists at line 3783.
Extended reasoning...
Overview
This PR aligns Bun's node:http2 outbound-header-object handling with Node's: a property whose value is undefined is skipped before its name is validated or counted, rather than rejected. It touches three files: src/runtime/api/bun/h2_frame_parser.rs (three native object walks + an empty-trailers fallback), src/js/node/http2.ts (five JS pre-checks + a writeEarlyHints() refactor), and seven new tests in test/js/node/http2/node-http2.test.js.
Security risks
None. The change only removes rejections of undefined-valued properties (which are then not sent at all); nothing that was previously sent is sent differently, and no call throws that did not before. null continues to be rejected exactly as on main. Header name/value validation for properties that are sent is unchanged.
Level of scrutiny
Medium. This is Node-compat behavior in the http2 header-encoding path, which has an HPACK-desync failure mode if the JS pre-checks and native encoder disagree — but the PR explicitly addresses that by updating all five JS pre-checks in lockstep with the three native walks, and the tests assert on what the peer actually decoded (so a desync would fail them). The 49-row behavior matrix in the description shows every changed row moves to node v26.3.0's result. The send_trailers() empty-case addition is a byte-for-byte copy of no_trailers()'s body against a GuardedStream (which DerefMuts to &mut Stream).
Other factors
- Tests are well-constructed: they wire every session/stream error to reject the awaited promise, assert on
rawHeadersdecoded from the wire (not the sender's view), cover the raw-array form alongside the object form, and include a follow-up request to prove the session's HPACK table stayed in sync. - The
writeEarlyHints()refactor was needed because it was relying on the native rejection this PR removes; the new form matches Node's compat layer (validate each hint, trim+lowercase the name, delegate towriteInformation()) and is pinned by both the new compat test and the existing portedtest-http2-compat-write-early-hints-invalid-header.js. bun bd test test/js/node/http2/reports 472 pass / 0 fail and all 256 portedtest-http2-*.jspass per robobun's status comment.- The description explicitly scopes out
null(unchanged, deliberately) and notes textual overlap with #37945/#33458, both of which compose with this.
|
Updated 6:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit c911973 has some failures in 🧪 To try this PR locally: bunx bun-pr 37953That installs a local version of the PR into your bun-37953 --bun |
|
Updated 10:31 AM PT - Aug 13th, 2026
❌ @robobun, your commit 07492b1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37953That installs a local version of the PR into your bun-37953 --bun |
|
Overlap note: #37951 (opened a little earlier) is a standalone fix for the all-empty trailer block ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites writeEarlyHints() (adding rejections as well as removing them), touches the HPACK encoder's validate-vs-skip ordering across three native loops plus five JS pre-checks, and overlaps with #37951 on the empty-trailers fallback, a human look is still worthwhile. There are also seven unresolved comment-cop flags on comment length that a maintainer should call on.
Checked: the three native iter.value.is_undefined() skips sit before name validation and single-value accounting, matching node's buildNgHeaderString position; null stays on its old paths (is_null() narrowing only). Verified assertValidHeader() rejects undefined/null and writeInformation() carries the headersSent/closed guard the old code had. The encoded_headers.is_empty() fallback mirrors no_trailers()'s DATA+END_STREAM. Tests wire session/stream errors to reject and assert on the peer's decoded field list.
Extended reasoning...
Overview
Node.js compat fix for node:http2: properties with undefined values in outbound header objects are now skipped before name validation and single-value accounting, matching node's buildNgHeaderString. Three files: src/runtime/api/bun/h2_frame_parser.rs (three object-walk loops in send_trailers/push_promise/request hoist iter.value.is_undefined() to the top; remaining checks narrowed to is_null(); send_trailers gains an empty-block fallback to DATA+END_STREAM), src/js/node/http2.ts (five JS pre-checks skip undefined; writeEarlyHints() rewritten to validate hints via assertValidHeader + checkIsHttpToken and delegate to writeInformation()), and seven new tests in test/js/node/http2/node-http2.test.js.
Security risks
None identified. The change loosens validation only for undefined (dropping the field, never sending it), which cannot inject header content. null handling is unchanged. The writeEarlyHints() rewrite tightens validation (pseudo-headers now rejected, invalid values rejected before the link check) and trims/lowercases names before the token check; assertValidHeader still rejects space-containing names.
Level of scrutiny
High. HPACK encoding is stateful per-connection: the JS pre-checks and the native encoder must agree on which properties count as fields, or a half-encoded block desyncs the session (the PR description itself calls this out as a bug on main). The writeEarlyHints() change is a behavior rewrite — it now throws in cases that used to return false (bad hint with link: [], or after headersSent) and accepts cases that used to throw (padded names). Every changed row in the 50-call matrix is claimed to match node v26.3.0, but that's exactly the kind of claim a maintainer familiar with this file should verify.
Other factors
- Seven unresolved
comment-copinline flags from github-actions on comment length; the second commit (70ca4a97) reworded some but the flags remain on the current diff. A maintainer should decide whether the comments (which explain node parity, not workarounds) are acceptable. - Acknowledged overlap with #37951 on the
send_trailersempty-block fallback; merge order needs coordinating. - Test coverage is thorough: seven tests, each block builder covered, failure paths wired to reject, peer-decoded fields asserted, session-health check after the request that used to desync HPACK. All 256 ported
test-http2-*files reported passing. claude-labeled (AI-authored) PR touching a ~9000-line native file and a ~6000-line JS file; the changes are surgical but the surrounding code is dense enough that a maintainer sign-off is appropriate.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The change is well-scoped and the behavior matrix in the description is thorough, but since it touches native HPACK encoding across three object-walk loops and rewrites writeEarlyHints() with behavior changes in both directions, a human look would still be worthwhile.
What was reviewed:
- The three native
iter.value.is_undefined()hoists insend_trailers/push_promise/request— the value is populated byiter.next()withinclude_value: true, so reading it before name validation is sound; the narrowedis_null()checks preserve the existingnullbehavior on every path. - The
encoded_headers.is_empty()fallback matchesno_trailers()line-for-line (wait_for_trailers = false+ empty DATA with END_STREAM); noted the overlap with #37951. - The five JS pre-checks (
assertSingleValueHeaders,assertNoConnectionHeaders, request()'s name loop, additionalHeaders()' pseudo-header loop, the no-payload content-length check) each skip onlyundefined, keeping them in step with the encoder. writeEarlyHints()now routes throughassertValidHeader+writeInformation(); verifiedassertValidHeaderrejectsundefined/null/pseudo-headers so the compat-layer boundary the tests pin holds.
Extended reasoning...
Overview
This PR aligns Bun's node:http2 outbound header handling with Node's buildNgHeaderString: a property whose value is undefined is skipped before its name is validated or counted, rather than rejected. It touches three files: src/runtime/api/bun/h2_frame_parser.rs (three object-walk loops in send_trailers, push_promise, and request, plus an empty-trailers fallback), src/js/node/http2.ts (five JS-side pre-checks that must classify properties the same way the encoder does, and a rewrite of Http2ServerResponse#writeEarlyHints()), and seven new tests in test/js/node/http2/node-http2.test.js.
Security risks
None identified. The change relaxes a rejection to a skip, which is what Node does; no untrusted input reaches new sinks. The writeEarlyHints() rewrite actually tightens validation in some cases (a :status hint now throws ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED instead of being sent). The narrowed is_null() checks keep null rejected exactly as before.
Level of scrutiny
Moderate-to-high. HTTP/2 HPACK encoding is a protocol-correctness-critical path — the PR description itself notes that a half-encoded block desyncs the connection's HPACK table. The individual edits are small and mechanical (hoist iter.value, add an is_undefined() guard, add headers[name] === undefined continues), but they span three native loops and five JS pre-checks that must all agree. The writeEarlyHints() rewrite is the least mechanical piece: it changes behavior in both directions (new rejections for pseudo-headers and post-headersSent bad hints; new acceptances for whitespace-padded names and Link alongside link), all documented as matching Node v26.3.0 in the second behavior matrix.
Other factors
The PR description is unusually thorough (50-row and 7-row behavior matrices against Node v26.3.0), the seven new tests each assert on wire-decoded field lists with error paths wired to reject, and CI passed on all jobs that ran. The comment-cop bot flags are automated length heuristics; the author already shortened comments in two follow-up commits and the remaining flagged comments (3-4 lines each) explain genuinely non-obvious rationale. The encoded_headers.is_empty() fallback overlaps with open PR #37951 (noted by robobun). Given the breadth of the protocol surface touched and the writeEarlyHints() behavior changes, deferring to a human maintainer rather than auto-approving.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches HTTP/2 header encoding across both the JS pre-checks and three native HPACK object walks, plus a semantic rewrite of writeEarlyHints() that adds new throw sites, a human look would still be worthwhile.
What was reviewed:
- The three native
iter.value.is_undefined()hoists inrequest()/push_promise()/send_trailers()—iter.valueis set byiter.next()before the loop body, so reading it early is safe;nullhandling is narrowed tois_null()and unchanged. - The empty-trailers fallback in
send_trailers()matchesno_trailers()line-for-line (wait_for_trailers = false+ empty DATA with END_STREAM). writeEarlyHints()now delegates towriteInformation(), which preserves the removedheadersSent || closedguard; the candidate concern aboutassertSingleValueHeadersrunning before pseudo-header defaulting was ruled out.
Extended reasoning...
Overview
This PR makes Bun's node:http2 skip properties whose value is undefined in outbound header blocks, matching Node's buildNgHeaderString behavior. It touches three files: five JS pre-checks in src/js/node/http2.ts (assertSingleValueHeaders, assertNoConnectionHeaders, the request() name loop, the additionalHeaders() pseudo-header loop, the no-payload content-length check), a rewrite of Http2ServerResponse#writeEarlyHints() to validate hints via assertValidHeader and delegate to writeInformation(), and three native object walks in src/runtime/api/bun/h2_frame_parser.rs (request(), push_promise(), send_trailers()) plus an empty-block fallback in send_trailers(). Seven new tests (~380 lines) cover every builder and the compat boundary.
Security risks
None identified. The change relaxes rejection (previously-throwing calls now succeed by omitting the field), which cannot introduce header injection since the field is not sent at all. The one path that gains rejections, writeEarlyHints(), adds Node's own assertValidHeader + checkIsHttpToken validation. HPACK state consistency is improved (the old code could desync the encoder table by throwing mid-block).
Level of scrutiny
High. This is protocol-level HTTP/2 frame encoding across two languages with HPACK state implications, and writeEarlyHints() gains new throw sites (:status hint, bad hint after writeHead). The PR is exceptionally well-documented (50-row node/main/PR behavior matrix, node v26.3.0 verification) and thoroughly tested, but it is not a mechanical change.
Other factors
CI (build 93693) passed on all agents that ran; the two macOS 26 jobs were cancelled by infrastructure. The comment-cop bot flagged long comments on an earlier revision; commits b9180b1 and 463da77 shortened them and the current diff has one-line comments. There is a known textual overlap with #37951 (same empty-trailers rule) and #37945 (assertSingleValueHeaders neighbouring lines), noted in the PR. The deliberate decision to leave null rejected (deviating from Node, which stringifies it) is called out and deferred to #37579 — that design boundary is worth a maintainer's eye.
node's buildNgHeaderString drops a property whose value is undefined before validating its name or counting it towards a single-value field. The native encoders behind request()/respond()/additionalHeaders() and sendTrailers() rejected such a property with ERR_HTTP2_INVALID_HEADER_VALUE (asynchronously for a client request, leaving the HPACK table out of step with the peer), and pushPromise() skipped it only after validating its name. All three now skip it first, and the JS checks that run before encoding (single-value rule, request() name validation, the pushStream() connection-header check and the no-payload content-length check) ignore it as well. A trailer block in which every field was skipped now ends the stream with an empty DATA frame, the frame noTrailers() and node send for an empty trailer list, instead of a zero-length HEADERS block. Http2ServerResponse#writeEarlyHints() was relying on the native rejection; it now validates each hint the way node's compat layer does (assertValidHeader plus a token check on the trimmed, lowercased name) and goes through writeInformation(). null values are left as they were on every path.
…ts rejections The sendTrailers() comment claimed that nghttp2 peers reject a zero-length trailer block; node's client and curl both accept one. The empty DATA frame is sent because that is what node sends, so say that instead. The trailers test now includes a pseudo-header name with an undefined value, the one block where the value has to be looked at before the name, and the compat test pins the rejections writeEarlyHints() gains from node's validation order (a :status hint, a bad hint with an empty link list) and the shape of the blocks it hands down.
… the request(), pushStream() and sendTrailers() tests
171a4a3 to
e942735
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes header-block handling across three native encoder loops, six JS pre-check sites, and refactors writeEarlyHints() (which both gains and loses rejections), plus carries an inline copy of #37951's empty-trailers fallback that needs a merge-order decision, a human look would still be worthwhile.
Checked: the is_undefined() skip is placed before name validation and single-value accounting in all three native loops, matching node's buildNgHeaderString order; the narrowed is_null() checks preserve the existing null rejection; the new encoded_headers.is_empty() fallback in send_trailers() matches no_trailers() line-for-line (wait_for_trailers = false + empty DATA with END_STREAM); the now-unconditional scalar branch in request() is safe because undefined continues at the top and null throws before reaching it.
Extended reasoning...
Overview
This PR makes Bun's node:http2 skip object properties whose value is undefined when building outbound header blocks, matching Node's buildNgHeaderString. It touches three files: src/runtime/api/bun/h2_frame_parser.rs (three encoder loops in send_trailers(), push_promise(), and request() — each gains a top-of-loop is_undefined() skip and narrows the later check to is_null(); send_trailers() also gains an empty-block fallback), src/js/node/http2.ts (six JS-level pre-checks skip undefined-valued keys; writeEarlyHints() is refactored to validate each hint via assertValidHeader() + checkIsHttpToken() and route through writeInformation()), and ~380 lines of new tests in test/js/node/http2/node-http2.test.js covering each block builder plus the compat boundary.
Security risks
None identified. The change loosens validation only by silently dropping undefined-valued properties (which are never sent, so no injection surface). null is deliberately unchanged on every path. The writeEarlyHints() refactor actually adds validation (pseudo-header rejection, token check on trimmed/lowercased names) to match Node's compat layer.
Level of scrutiny
Moderate-to-high. This is HTTP/2 protocol handling — the PR itself notes that the pre-fix bug could desync the HPACK encoder table and corrupt every subsequent request on the session. The change spans native Rust and built-in JS, and the writeEarlyHints() refactor changes behavior in both directions (6 of 7 rows in the compat matrix change: some inputs newly throw, others newly succeed). While the work is exceptionally well-documented (behavior matrices verified against Node v26.3.0) and thoroughly tested, the surface area — three native encoder loops, six JS pre-checks, one compat-layer refactor, one new native fallback path — is beyond what I'd approve without human review.
Other factors
CI passed on the first push (build 93693, 179/181 jobs; the two skipped were infra-cancelled macOS 26). All comment-cop feedback about long comments was addressed in follow-up commits. There is a stated overlap with #37951 (the standalone empty-trailers fix) that carries the same encoded_headers.is_empty() hunk and needs a merge-order decision from a maintainer. The tests are strong: they assert on wire-level fields decoded by the peer, wire error events on both sessions to fail the test, and cover the defined-then-undefined ordering that distinguishes a skip placed before vs after single-value accounting.
node-http2.test.js has a dozen pre-existing tests that run at 3-5s on a debug build, so the whole file cannot be run reliably under ASAN on a busy machine; the tests for this change are self-contained and the other open header PRs all insert at the same spot in that file, so they get their own file next to it. The pushStream() case now collects the pushed blocks through the shared session helper instead of the big file's helper.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes HTTP/2 header encoding across both the native encoder (h2_frame_parser.rs) and the JS pre-checks, rewrites writeEarlyHints() with validation that both adds and removes rejections, and changes wire behavior for empty trailer blocks, a human look would still be worthwhile.
Checked: the removed is_empty_or_undefined_or_null() guards are safe since JSPropertyIterator::next() already skips JSEmpty internally (JSPropertyIterator.rs:207); the encoded_headers.is_empty() fallback matches no_trailers() exactly; writeInformation() carries the same headersSent || closed guard the old inline path had; the JS-side skips (assertSingleValueHeaders, assertNoConnectionHeaders, the request()/additionalHeaders() name loops, the content-length check) each align with the encoder's new skip position.
Extended reasoning...
Overview
This PR makes bun's node:http2 skip properties with undefined values in outbound header blocks, matching Node's buildNgHeaderString (if (value === undefined || key === '') continue;). It touches three layers:
- Native (
src/runtime/api/bun/h2_frame_parser.rs): three object-walk loops (send_trailers,push_promise,request) now hoistiter.valueand skip onis_undefined()before name validation and single-value accounting; the residual value checks narrow fromis_undefined_or_null()/is_empty_or_undefined_or_null()tois_null().send_trailersgains anencoded_headers.is_empty()fallback that ends the stream via an empty DATA+END_STREAM frame (mirroringno_trailers()and Node'sSubmitTrailers). - JS built-in (
src/js/node/http2.ts): five pre-encoder checks (assertSingleValueHeaders,assertNoConnectionHeaders, therequest()name-validation loop, theadditionalHeaders()pseudo-header loop, the content-length-on-no-payload check) each add a=== undefinedskip so they agree with the encoder.writeEarlyHints()is rewritten to validate each hint viaassertValidHeader()+checkIsHttpToken()(likesetHeader()) and then delegate towriteInformation(). - Tests: a new 403-line file with seven end-to-end tests, one per block builder plus the empty-trailers case and the compat boundary, asserting on the field list the peer decoded and pinning the expected values against Node v26.3.0.
Security risks
None identified. The change loosens validation only for undefined values (which are dropped, not sent), and the writeEarlyHints() rewrite adds Node's hint validation before anything reaches additionalHeaders(). No untrusted-input parsing paths are widened.
Level of scrutiny
High. This is production Node compat code in the HTTP/2 stack: the native side is the HPACK encoder where a mid-block throw already caused an HPACK-table desync (called out in the PR), and writeEarlyHints() is a user-facing API whose rewrite both adds rejections (:status hint now throws ERR_HTTP2_PSEUDOHEADER_NOT_ALLOWED; bad hints throw even when link is empty or headers were already sent) and removes some (whitespace-padded names now trimmed; a Link hint is no longer dropped). The empty-trailers change alters what goes on the wire. This is not a mechanical change.
Other factors
- The PR description includes a 50-row behavior matrix run against Node v26.3.0; every changed row moves to Node's result and
nullis deliberately unchanged on every path. - The removed
is_emptyguard on the scalar branch inrequest()and the narrowed check inpush_promise()are sound becauseJSPropertyIterator::next()itself doesif current.is_empty() { continue; }, soiter.valueis neverJSEmptywhen a name is yielded. - The
encoded_headers.is_empty()fallback is byte-for-byte whatno_trailers()does (wait_for_trailers = false; send_data(b"", true, UNDEFINED, false, false)). writeInformation()already checksstream.headersSent || this[kState].closed, so delegating to it preserves that guard; the validation loop now runs before the empty-link early return, which is the intended Node behavior per the second matrix.- All comment-cop inline comments are resolved; the PR reports the full
test/js/node/http2/suite and all 256 portedtest-http2-*.jsfiles pass. - There is an acknowledged overlap with open PR #37951 (the standalone empty-trailers fix at the same site); whichever lands second drops its copy. That coordination is a merge-order decision a maintainer should make.
Given the multi-layer scope, the user-facing semantic changes to writeEarlyHints(), the wire-format change for trailers, and the interaction with #37951, I'm deferring rather than approving.
… bound like nghttp2 Also complete a pending close() when the last request throws during submission, and drop the undefined-value test rows that #37953 changes.
… bound like nghttp2 Also complete a pending close() when the last request throws during submission, and drop the undefined-value test rows that #37953 changes.
Problem
undefinedis rejected.stream.respond({ ":status": 200, "content-type": maybeUndefined }),additionalHeaders()andsendTrailers()throwERR_HTTP2_INVALID_HEADER_VALUE(Invalid value for header "content-type") synchronously;session.request()returns a stream that emits the same error a tick later.buildNgHeaderStringin lib/internal/http2/util.js:if (value === undefined || key === '') continue;), so it is not sent, its name is never validated, and it does not count as an occurrence of a single-value field. Passing possibly-undefined values and relying on that is common in node code.H2FrameParser::request()encodes the pseudo-headers in its first pass and throws in the second, so the connection's HPACK encoder is one block ahead of the peer and the next request on the session fails too.src/runtime/api/bun/h2_frame_parser.rs): the object walks inrequest()(behindrequest(),respond()andadditionalHeaders()) andsend_trailers()throw onis_undefined_or_null()after validating the name;push_promise()skipsundefined, but also only after validating the name. The raw[name, value, ...]walk already skippedundefinedslots, so the two forms disagreed.src/js/node/http2.ts): the checks that run before encoding count the property as well.assertSingleValueHeaders()throwsERR_HTTP2_HEADER_SINGLE_VALUEfor{ "content-type": undefined, "Content-Type": "text/plain" },request()'s name validation throws for{ ":bogus": undefined },additionalHeaders()'s pseudo-header loop likewise,pushStream()'sassertNoConnectionHeaders()for{ connection: undefined }, and the no-payload check rejects{ "content-length": undefined }on a GET.Fix
iter.value.is_undefined()next to the existing empty-name check at the top of the loop, before name and pseudo-header validation and before single-value accounting, which is the position node's skip has. The scalar branch inrequest()loses its guard, which this makes unconditional.nullis left exactly as it was on every path: the narrowed checks areis_null(), sorequest()/respond()/additionalHeaders()/sendTrailers()still throw on it andpushStream()still skips it. Node stringifies it; bun rejectsnullelements on purpose (node:http2: send array-valued pushStream() headers as one field per element #37579), and whether scalars should follow node or the elements is a separate decision from this one, so nothing about it changes here.send_trailers(): when every field was skipped it ends the stream with the same empty DATA+END_STREAM framenoTrailers()sends forsendTrailers({}), instead of a zero-length HEADERS block. That is what node'sSubmitTrailersdoes for an empty list, and parity is the only reason for it: the peer then sees the stream end without a'trailers'event, as it does with a node server. (The comment next to the JS short-cut used to say that nghttp2 peers reject a zero-length trailer block; node v26's client and curl with nghttp2 1.64 both accept one, checked against the current release, so that comment is gone.) node:http2: end the stream with an empty DATA frame when sendTrailers() encodes no fields #37951 is the standalone fix for this rule, with wire-level tests, sincesendTrailers({ x: [] })already reaches an empty block on main; the skip adds another way to get there, so this PR carries the same few lines inline, and whichever of the two lands second drops its copy.pushStream()even adds an undefined:authorityof its own after its checks have run, which is whypush_promise()already skipped it), because stripping would mean deleting properties from the copied object that backssentHeaders(node keeps the undefined-valued key there, and the file avoidsdeleteon these objects for the dictionary-mode reason noted in it), and because the pre-checks have to be kept in step with the encoder either way.Http2ServerResponse#writeEarlyHints()was relying on the native rejection: node's compat layer validates each hint itself and its test-http2-compat-write-early-hints-invalid-header.js expectsERR_HTTP2_INVALID_HEADER_VALUEfor{ "x-custom": undefined }, which this change would otherwise have started sending. It now does what node's does:assertValidHeader()plus the token check on the trimmed, lowercased name, thenwriteInformation(). This is the one function here that gains rejections as well as losing some, all of them node's (second table below): a:statushint throwsERR_HTTP2_PSEUDOHEADER_NOT_ALLOWEDinstead of being sent, and a bad hint throws even when the link list is empty or the response headers are already out, where the old code returnedfalse; on the other side{ " X-Hint ": "v" }is sent asx-hint(it used to throw), aLinkhint is sent alongsidelink(it used to be dropped), andwriteInformation(103, { x: undefined })passes the value through toadditionalHeaders()(it used to throw).writeEarlyHints()is node's current code, and the rows that still differ from node all involvenullor array elements, which are unchanged on every path.test/js/node/http2/node-http2-undefined-header-values.test.js, one per block builder plus the all-skipped trailers case and the compat boundary. They assert on the field list the peer decoded from the wire and fail if either session reports an error; the expected values were produced by running the same blocks under node v26.3.0. Each builder's block includes a pseudo-header name with an undefined value (in the trailers block that is the case where the skip, not the name check, has to win, since trailers reject every pseudo-header) and the single-value pair spelled undefined-then-defined;request()(which runs the shared JS pre-check),pushStream()andsendTrailers()(the two builders with their own native accounting) also send the pair in the other order, which is the only input that tells a skip placed before the occurrence check from one placed after it. The compat test pins thewriteEarlyHints()rejections, thelink: []ordering and the shape ofsentInfoHeaders. All seven fail on main (six on the rejection, the compat one onwriteInformation()) and pass with the fix.bun bd test test/js/node/http2/: 472 pass, 0 fail. All 256 portedtest/js/node/test/parallel/test-http2-*.jsfiles pass with the fix (test-http2-compat-write-early-hints-invalid-header.js is the one that caughtwriteEarlyHints()).assertSingleValueHeaders()and the array branches; the two rules are independent and compose in either order, the overlap is textual. node:http2: fix connection-specific and pseudo-header validation on outbound header blocks #33458 carries the sameundefinedskip inassertNoConnectionHeaders(). node:http2: coerce outbound header values before taking a stream id #37568 passes primitives through to the encoder untouched and node:http2: send array values in raw-form header lists as one field per element #37796's raw-form pre-check already skipsundefinedslots, so both keep working with this.Background
{ name: value }, an array value meaning one field per element) or as a flat[name, value, ...]list. This PR is about the object form;request(),respond(),additionalHeaders(),pushStream()andsendTrailers()all accept it.H2FrameParser, which walks the object, validates each field and HPACK-encodes it as it goes. HPACK keeps a table per connection that the peer's decoder mirrors entry for entry, so a block that is rejected after some of its fields were encoded leaves the connection's table ahead of the peer's and every later block is misdecoded. The JS layer therefore repeats some of the validation before calling into native (assertSingleValueHeaders()and friends); those checks are only correct if they classify properties exactly as the encoder does.content-type, the pseudo-headers, ...); a second occurrence, or a multi-element array, isERR_HTTP2_HEADER_SINGLE_VALUE.Http2Stream::SubmitTrailersin node_http2.cc), so the peer gets no'trailers'event; bun'snoTrailers()path sends the same frame.Http2ServerResponse(the node:http-shaped compat layer) validates header values when they are set, and rejectsundefinedandnullthere, in node as well (setHeader(),writeEarlyHints()); the stream-level API underneath is the one that skipsundefined. The compat test in this PR pins that boundary.Behavior matrices: node v26.3.0 / main / this PR (stream-level builders: 50 calls, 20 change; compat layer: 7 calls, 6 change; every change is to node's result)
Each row is one call made against an
http2.createServer()/http2.connect()pair on a fresh session; the listed fields are thex-*andcontent-typefields the peer decoded. The[undefined]/[null]rows are array elements and the:status nullrow is the JS status default; none of those change.Http2ServerResponserows (7 calls, 6 change, again every change is to node's result).linkis"</s.css>; rel=preload"; the listed fields are the:status,x-*andlinkfields of the informational block the client received.