node:http2: reject request header blocks that fail RFC 9113 8.3.1 (empty/missing :path, :method, :scheme; CONNECT shape) - #34736
Conversation
…pty/missing :path, :method, :scheme; CONNECT shape) The h2 server's header-block validator checked for duplicate, unknown and late pseudo-headers but never enforced the per-request presence rules in RFC 9113 8.3.1: a HEADERS block with :path set to the empty string (or missing any of :method/:scheme/:path, or a CONNECT carrying :scheme/:path, or a non-CONNECT carrying :protocol) was delivered to the 'stream' handler with req.url === '' / undefined. Node (via nghttp2's nghttp2_http_on_request_headers) answers all of these with a stream PROTOCOL_ERROR and never dispatches. A compliant peer cannot produce these shapes, so nothing legitimate is rejected; the only callers that can reach the handler with them are raw-frame clients. The fix is nghttp2-equivalent: an empty pseudo-header value is malformed inline, and after the decode loop a request block (a server-received initial HEADERS or a PUSH_PROMISE) is held to the 8.3.1 requirements before being surfaced.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 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)
Comment |
|
Updated 5:38 PM PT - Jul 19th, 2026
❌ @autofix-ci[bot], your commit 26eb1c8 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34736That installs a local version of the PR into your bun-34736 --bun |
|
This PR may be a duplicate of:
Generated with Claude Code <!~~ find-duplicate-prs-bot:marker ~~> |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this is protocol-validation logic in the h2 engine with a stated overlap with #33191, so it's worth a maintainer look to confirm the CONNECT / extended-CONNECT / :authority-vs-Host shape matches what #33191 expects to rebase over.
What was reviewed:
- Traced the new §8.3.1 bitmask check against every test case (plain/extended CONNECT, missing/empty pseudo-headers,
hostfallback) — each branch reaches the intended verdict. header_is_request = is_server && is_newcorrectly excludes client-side response HEADERS and server-side trailers; PUSH_PROMISE sets it explicitly.- The new
value_b.is_empty()check also fires on client-side:status— that's a strict tightening matching nghttp2'scheck_pseudo_header, not a regression. - Existing Rust unit tests were updated to send complete pseudo-header sets; no other in-tree callers of
encode_header/finish_header_blockneeded changes.
Extended reasoning...
Overview
Adds RFC 9113 §8.3.1 request pseudo-header validation to the h2 engine's finish_header_block in src/runtime/api/bun/h2/connection.rs: empty pseudo-header values are marked malformed inline in the decode loop, and after decode a request block (server-received initial HEADERS or client-received PUSH_PROMISE) is checked for the required combination of :method/:scheme/:path/:authority-or-Host, plain-CONNECT shape, and extended-CONNECT (RFC 8441) shape. A new header_is_request connection field distinguishes request blocks from trailers and client-side response blocks. The magic bit constants were lifted into a pseudo module. Four existing Rust unit tests were updated to send complete pseudo-header sets. 20 new cases (16 rejections + 4 acceptances) were added to test/js/node/http2/h2-conformance.test.ts.
Security risks
This is validation of untrusted network input. The change strictly tightens what reaches the JS 'stream' handler — previously an empty :path or missing :method reached user code, which is a request-smuggling / handler-confusion surface. The rejection path is the pre-existing malformed-block path (RST_STREAM PROTOCOL_ERROR, counted against maxSessionInvalidFrames), so no new DoS surface is introduced. I checked that the empty-value check is applied per-pseudo-header (matching nghttp2's check_pseudo_header) and that seen_pseudo is still set even when malformed, which is harmless because the request-shape check is gated on !malformed.
Level of scrutiny
Medium-high. This is not a mechanical change: the three-branch bitmask check (plain CONNECT vs extended CONNECT vs ordinary request) encodes several spec paragraphs and the interaction between saw_connect, extended_connect, saw_host, and the AUTHORITY bit is subtle. I traced each of the 20 test inputs through the logic and they all land where intended, including the extended-CONNECT-with-host-but-no-:authority case (rejected via the extended_connect && (seen_pseudo & AUTHORITY) == 0 clause even though saw_host would satisfy the ordinary-request clause). The is_server && is_new gate on header_is_request is essential and correctly commented — without is_server, every client-side response would be misclassified as a request since the engine only tracks inbound-created streams.
Other factors
The PR description explicitly positions this as a carve-out from #33191 designed to rebase cleanly under it, and the duplicate-PR bot flagged the overlap. That coordination decision — whether to land this standalone or fold it into #33191, and whether the exact validation shape here (e.g. no :path-starts-with-/ check, no empty/duplicate Host check) is the right cut point — is a maintainer call. The test coverage is thorough and the robobun gate confirms the tests fail without the fix and pass with it on both ASAN and release builds.
|
CI on build #75867: the two red tests are unrelated to this diff (
|
2 participants
What
A raw-frame h2 client that sends a request with
:pathset to the empty string reaches the'stream'handler withheaders[':path'] === ''(andreq.url === ''in the compat layer). RFC 9113 8.3.1 says:path"MUST NOT be empty" for http/https and that every non-CONNECT request must carry exactly one non-empty:method,:schemeand:path; node (via nghttp2'snghttp2_http_on_request_headers) answers withRST_STREAM(PROTOCOL_ERROR)and never dispatches.Repro (node RSTs, Bun on main dispatches):
The same validator gap meant requests with
:method/:scheme/:pathmissing entirely, a plainCONNECTcarrying:scheme/:path, aCONNECTwithout:authority, or:protocolon a non-CONNECT all reached the handler too. All are rejected by node as a streamPROTOCOL_ERROR.Cause
finish_header_blockin the h2 engine (src/runtime/api/bun/h2/connection.rs) tracks which pseudo-headers were seen (for the duplicate check) but never checks that the required ones are present, and never checks for empty values. The decode loop validated per-field shape (duplicate, unknown, late, connection-specific, CR/LF/NUL) but not the per-block 8.3.1 requirements.Fix
nghttp2-equivalent, one block in
finish_header_block:check_pseudo_headersemantics), so":path": ""never counts as present.HEADERSor a client-receivedPUSH_PROMISE) is held to the 8.3.1 requirements::method,:scheme,:pathand:authority-or-Hostfor ordinary requests;:authorityand no:scheme/:pathfor plainCONNECT;:method CONNECT+:authorityfor extended CONNECT (:protocol, RFC 8441).A
header_is_requestflag distinguishes a request block from a trailer section (which 8.1 already forbids from carrying pseudo-headers) and from a client-received response block (which the 8.3.1 rules do not apply to).The rejection takes the existing malformed-block path (
RST_STREAM(PROTOCOL_ERROR), counted againstmaxSessionInvalidFrames, never surfaced to'stream'), so the JS-visible behavior matches node exactly.Verification
16 new rejection cases plus 4 positive cases (valid block,
hostin place of:authority, plainCONNECT, extendedCONNECT) intest/js/node/http2/h2-conformance.test.ts. All 16 rejection cases fail on the unfixed build (the request is dispatched) and pass with the fix; all 4 positive cases pass on both. The fullh2-conformancesuite (58 tests),node-http2.test.js(305 tests) and node's upstreamtest-http2-connect-method*/test-http2-misused-pseudoheaderspass on the fixed build.Related
#33191 carries a broader version of this validation (including nghttp2's
check_path():path-starts-with-/rule,Hostempty/repeated handling, andPUSH_PROMISEconnection-error semantics) as item 1 of a larger hardening round. This PR is the minimal, standalone carve-out of the 8.3.1 presence/emptiness rules against current main, shaped so #33191 rebases cleanly over it.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file