Repository navigation
Conversation
|
Updated 7:20 AM PT - Jul 29th, 2026
❌ @robobun, your commit 0691f8e has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33458That installs a local version of the PR into your bun-33458 --bun |
WalkthroughThis PR adds outbound HTTP/2 connection-specific header validation across JS-side stream send paths (sendTrailers, additionalHeaders, respond, request) via a reworked ChangesHTTP/2 header validation hardening
Possibly related PRs
🚥 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/http2.ts`:
- Around line 3219-3221: Guard the file-response paths in respondWithFile() and
respondWithFD() the same way as the other outbound header encoders: after
headers are normalized/defaulted and before any file I/O or native send-file
call, invoke assertNoConnectionHeaders() on the final headers object. This
should ensure connection-specific fields, http2-settings, and invalid te values
cannot bypass validation through file responses.
In `@test/js/node/http2/node-http2.test.js`:
- Around line 2594-2598: The inline rationale added around the HTTP/2
invalid-headers case is too long and exceeds the repo’s 3-line comment limit.
Shorten the comment in the HTTP2 test around the request/session.request(h)
check to a concise 3-line summary, or move the extra RFC and retry-context
explanation into the PR description while keeping only the essential note in the
test.
- Around line 2652-2659: Add the missing undefined coverage to the HTTP/2 header
test in node-http2.test.js. The existing assertion around the tryRequest cases
in the HTTP/2 test block documents that undefined values should not be
forbidden, but only verifies empty arrays and other inputs. Update the same
expect(...) call near the te/trailers cases to include a connection: undefined
case (using tryRequest with the same helper) so the test explicitly guards
against regressions in handling undefined header values.
🪄 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: d0b5a802-8ea2-4ace-8914-58ae2a37f8c3
📒 Files selected for processing (3)
src/js/node/http2.tssrc/runtime/api/bun/h2_frame_parser.rstest/js/node/http2/node-http2.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Thanks both. I checked each finding at the wire level; two of the four are real, two are based on code paths that don't run. Pushed 1930c53 with the fixes that were warranted. AddressedComment over the 3-line limit (
So I did not take the suggested synchronous Not applied, with evidence
That is generic header-value handling in the native encoder, unchanged before and after this PR. The
Captured at the wire with a raw socket client against a Bun Identical for the indexed encoding, and no GOAWAY frame in either. The session survives, the follow-up request is delivered, and the test passes deterministically (full file: 304 tests, 298 pass, 0 fail). I left the dead branch alone rather than delete code I can't prove unreachable from every caller. |
There was a problem hiding this comment.
Thanks for the wire-capture evidence on the :status inbound path — that resolved my earlier concern. I didn't find any remaining issues, but since this touches HTTP/2 header-block validation across both the JS layer and the native frame encoder (relaxing outbound pseudo-header checks), I'd like a human maintainer to sign off.
Extended reasoning...
Overview
This PR aligns Bun's node:http2 outbound header validation with Node.js in two directions:
- JS (
src/js/node/http2.ts): addsassertNoConnectionHeaders()calls to the four sites that encode a header block but previously skipped the check —request(),respond(),additionalHeaders(),sendTrailers()— and refines the helper to match Node's value semantics (skipundefined/empty-array values, compareteviaString(value)so["trailers"]unwraps and["trailers","gzip"]fails). Also relocates a misplaced doc comment. - Rust (
src/runtime/api/bun/h2_frame_parser.rs): adds anis_valid_pseudo_headerpredicate holding all six spec-defined pseudo-headers (Node'skValidPseudoHeaders) and swaps three outbound-encoding sites (client request object form, client request raw-array form, server PUSH_PROMISE) from the request-only set to it. Server response encoding and inbound decoding are unchanged. - Tests: four new tests (~260 lines) covering the full matrix — every forbidden connection-specific field,
tevalue shapes, raw-array headers,:statusin a client request,respond/additionalHeaders/sendTrailers, andrespondWithFile/respondWithFD.
Security risks
None material. The JS side tightens validation (rejects RFC 9113 §8.2.2 connection-specific headers earlier, before they reach the wire). The Rust side relaxes the outbound client/PUSH_PROMISE pseudo-header check to allow :status through — but that only lets a client send a request the peer will RST; it is Node's documented behavior and does not affect what Bun's server accepts inbound. No auth, crypto, or permissions surface is touched.
Level of scrutiny
Moderate-to-high. HTTP/2 frame encoding is production-critical network code, and the interactions here (sync throw vs. async stream error, RST_STREAM vs. GOAWAY, which validation predicate runs at which site) are subtle — subtle enough that my own earlier static-reading of decode_header_block reached a conclusion the author refuted with a wire capture and an eprintln probe. The changes themselves are small and mechanical (four one-line insertions, one predicate swap at three sites), but the correctness argument depends on understanding which encoder/decoder paths are live, and that warrants a maintainer familiar with h2_frame_parser.rs.
Other factors
All prior review feedback is resolved: the three CodeRabbit comments were addressed in commit 1930c53 (comment shortened, undefined-value case covered, respondWithFile/respondWithFD test added), and my inline concern about the :status test was answered with empirical evidence (raw-socket capture showing RST_STREAM, not GOAWAY, from Bun's server; full-file run of 304 tests with 0 failures; all four new tests fail on an unpatched build). The PR description also reports no change across the 272 ported test-http2-* Node parallel tests. No CODEOWNERS entry covers these paths. Given the subsystem, I'm deferring rather than approving.
1057436 to
79d3645
Compare
There was a problem hiding this comment.
Thanks — 8f7089d addresses the send-file error-path teardown and I found nothing new; leaving final sign-off to a human since this changes HTTP/2 wire behavior on both the JS validation side and the native outbound pseudo-header check.
Extended reasoning...
Overview
This PR wires assertNoConnectionHeaders() into the four outbound header-block sites that were missing it (request(), respond(), additionalHeaders(), sendTrailers()) in src/js/node/http2.ts, reworks that helper's value semantics (own-key iteration via ObjectKeys, skip undefined/empty-array, String(value) coercion for te), guards the two doSendFileFD error branches so a throwing respond() cannot skip stream teardown, and — on the native side — adds is_valid_pseudo_header() in h2_frame_parser.rs and swaps three outbound encode sites (request object path, request raw-pairs path, PUSH_PROMISE) from the request-only set to the full spec set, so :status in a request is encoded rather than thrown. Five new tests cover client-side sync rejection, the :status-on-request wire shape, server-side respond/additionalHeaders/sendTrailers, the respondWithFile/FD async-error shape, and the directory + connection-header teardown regression.
Security risks
The net effect is stricter outbound validation on the JS side (connection-specific fields are rejected before encoding, closing the case where http2-settings previously reached the wire) and looser outbound validation on the native side (:status in a request is now sent). The relaxation matches Node's kValidPseudoHeaders and the peer is expected to RST the stream per RFC 9113 §8.1.1; the inbound decoder and server-response pseudo-header check are unchanged. I do not see an injection, auth, or data-exposure vector, but any change to what an HTTP/2 client will emit on the wire is worth a maintainer's eye.
Level of scrutiny
Medium-high. This is a focused Node-compat fix with strong test coverage and wire-level verification tables against Node v26.3.0, and the ported test-http2-* suite is reported unchanged. But it is a behavioral change in a network protocol path spanning both the JS builtin and the native frame encoder — not a mechanical or config-only change — so it falls outside what I would auto-approve.
Other factors
Both of my earlier inline findings are resolved: the :status → GOAWAY concern was refuted empirically (the cited decode_header_block() branch is not on the live inbound path), and the doSendFileFD error-branch hang was fixed in 8f7089d with a dedicated regression test. All CodeRabbit threads are resolved, and the bug-hunting pass on this revision found nothing. No CODEOWNERS entry matches the touched files.
CI status: diff is green, one lane failing on an infra flakeThe latest run (build #69355, sha That shard ran zero tests. It died during setup: Evidence it's infra, not the diff:
I've already used my one allowed CI re-roll on this flake, so I'm not pushing another retrigger (that would just be noise). The diff itself is complete and verified: |
|
Another report of the trailer-block face of this landed today; I worked it before finding this PR. Pushed the branch for reference: Same core fix; a few deltas that might be worth folding in here:
Not opening a separate PR. |
8f7089d to
129b8e5
Compare
CI status: diff is green; all red lanes are unrelated and already ownedBuild #74378 (sha
The diff itself is complete and verified: I am not pushing another retrigger; a maintainer re-run of those lanes should clear them. |
c79ce99 to
d81b991
Compare
CI status: diff is green; both red tests are unrelated ASAN flakes already being handledBuild #74581 (sha
Both pass repeatedly on my ASAN debug build locally, both are being addressed separately, and this PR touches only
|
d81b991 to
90aac4f
Compare
Diff is ready; internal QA build is failing on infraThe internal QA gate bounced on
I reproduced both builds locally on
I am not pushing another empty commit to re-queue the gate (one was already pushed earlier for the same failure pattern, and each push also queues a full CI run). |
…utbound header blocks Node validates every outbound header block through buildNgHeaderString, which rejects connection-specific fields (connection, upgrade, keep-alive, proxy-connection, transfer-encoding, http2-settings, and te with any value other than "trailers") with a synchronous ERR_HTTP2_INVALID_CONNECTION_HEADERS. Bun only wired that check into pushStream(), so client request(), respond(), additionalHeaders() and sendTrailers() let those fields reach the wire: the request came back as a stream that failed asynchronously, and http2-settings was encoded into the HEADERS frame of a request node never sends. Call assertNoConnectionHeaders() from the four missing sites, and give it node's value semantics: undefined values and empty arrays encode no header at all, and a multi-element te array is illegal. The outbound pseudo-header set is the opposite problem. Node's kValidPseudoHeaders accepts every spec-defined pseudo-header on a request or PUSH_PROMISE block, so a request carrying :status is encoded and the peer answers with a stream error. Bun validated against the request-only set and threw ERR_HTTP2_INVALID_PSEUDOHEADER synchronously instead. The inbound decoder keeps the strict request/response split.
respondWithFile()/respondWithFD() encode their header block through respond(), so they pick up the connection-specific check; node validates at the same point (processRespondWithFD) and destroys the stream instead of throwing, because the block is built after the fstat completes. Assert that shape on both entry points. Also pin the undefined-value behavior: an undefined value encodes no header, so it is not a connection-specific violation and fails the same way any undefined-valued header does.
…spond doSendFileFD's two error branches (fstat failure, and a directory / non-seekable target) called this.respond(headers, options) before this.destroy(). respond() validates headers and can throw, so a connection-specific header on one of these paths skipped the destroy and leaked the stream, hanging the client. Guard both respond() calls so the stream is always torn down, matching node, which never encodes headers on the send-file error path.
90aac4f to
00fe292
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Closing: superseded by #41614, which adds the connection-specific header check (with node's te rules) to request(), respond(), additionalHeaders() and sendTrailers(). This branch no longer merges. |
Repro
connection/upgrade/keep-alive/proxy-connection/transfer-encoding/te: gzipSYNC-THROW ERR_HTTP2_INVALID_CONNECTION_HEADERSERR_HTTP2_STREAM_ERROR,rst=1http2-settingsSYNC-THROW ERR_HTTP2_INVALID_CONNECTION_HEADERS200, sent on the wire:statusin a requestPROTOCOL_ERRORSYNC-THROW ERR_HTTP2_INVALID_PSEUDOHEADERThe node-idiomatic
try { session.request(h) } catchnever fires for a request that can never succeed, so the error surfaces later as what looks like a peer failure and retry logic keeps replaying a permanently-invalid request. Acceptinghttp2-settingsputs a connection-specific field on the wire, which conforming HTTP/2 peers may treat as a protocol error for the whole session (RFC 9113 §8.2.2).Cause
Node routes every outbound header block through
buildNgHeaderString, which callsisIllegalConnectionSpecificHeaderon each non-pseudo field. Bun has the equivalent helper,assertNoConnectionHeaders, but it was only wired intopushStream(). The other four sites that encode a header block never called it:session.request()stream.respond()stream.additionalHeaders()stream.sendTrailers()stream.pushStream()The
:statuscase is the same validation read in the other direction. Node'skValidPseudoHeadersholds all six spec-defined pseudo-headers and is what both request andPUSH_PROMISEblocks validate against, so:statusis encoded and the peer reports the protocol error. Bun's native encoder validated an outbound request against the request-only set (h2_frame_parser.rs,is_valid_request_pseudo_header) and threwERR_HTTP2_INVALID_PSEUDOHEADERsynchronously.Fix
src/js/node/http2.ts: callassertNoConnectionHeaders()fromrequest(),respond(),additionalHeaders()andsendTrailers(), and give it node's value semantics: anundefinedvalue or an empty array encodes no header at all, a single-element array unwraps, and a multi-elementtearray is illegal. Header names are matched case-insensitively, as before.src/runtime/api/bun/h2_frame_parser.rs: addis_valid_pseudo_header(node'skValidPseudoHeaders) and use it for outbound request andPUSH_PROMISEblocks. Server responses keepis_valid_response_pseudo_header, and the inbound decoder keeps the strict request/response split, so a received request carrying:statusis still malformed.pushStream()is unchanged; it already had the check.respondWithFile/respondWithFDinherit it throughrespond()and destroy the stream on the throw, matching node'sprocessRespondWithFD. The twodoSendFileFDerror branches (fstat failure, directory/non-seekable target) calledrespond()beforedestroy(), so a throwingrespond()skipped the teardown; both are now guarded so the stream is always torn down.Verification
bun bd test test/js/node/http2/node-http2.test.jspasses; the five new tests fail on an unpatched build.The repro above now prints the same table as node v26.3.0 for all ten shapes (the six connection-specific fields,
te: gzip,te: trailers,http2-settings,:status), including the raw[name, value, ...]headers form and the value-shape matrix (te: ["trailers"]allowed,te: ["trailers", "gzip"]rejected,connection: []skipped).Ported node http2 suite: no change
All 272
test/js/node/test/parallel/test-http2-*.jsfiles were run before and after. The same set passes;test-http2-misused-pseudoheaders,test-http2-info-headers,test-http2-server-push-stream-errors-argsandtest-http2-invalidheaderfield(which assert on these error codes) pass both ways.test-http2-util-headers-list,test-http2-util-assert-valid-pseudoheaderandtest-http2-respond-file-fd-leakfail identically on stock bun 1.4.0 and are unrelated (they requireinternalBinding("http2")/internal/http2/util, or cover an fd leak onrespondWithFile's error path).Rebase note
Main wrapped
assertSingleValueHeaders(headers)in akStrictSingleValueFields !== falseconditional at the same three call sites this change insertsassertNoConnectionHeaders(headers)after. Node'sbuildNgHeaderStringgates the single-value check on that flag but callsisIllegalConnectionSpecificHeaderunconditionally, so the resolution keeps main's conditional for the single-value check and callsassertNoConnectionHeadersunconditionally after it.[review] gate passed · iteration 16 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 16
evidence per changed file