Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughThe connection now tracks peer-initiated stream IDs independently from local stream IDs. ChangesHTTP/2 promised stream validation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The client now rejects reused or non-increasing promised stream IDs with a protocol error, preventing invalid stream reuse. Current test results show no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced on bun 1.4.3 with a raw server against Fix: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 changes the h2 engine's stream state machine on untrusted network input (and closes a path the description notes could make stream ids repeat under the embedder's free in rewrite_read), a human look would still be worthwhile.
What was reviewed:
handle_headers: the newignoredgate is checked beforerefused, socan_open_stream()is not consulted for old ids;last_peer_stream_idis only written when!ignored, so it never regresses.finish_header_block:stream_ignoredreturns after the HPACK decode loop and thefatalcheck but before any RST/sink callback, so the dynamic table stays in sync and nothing is surfaced or sent — the CONTINUATION test covers this.handle_push_promise:promised <= last_peer_stream_idsubsumes the oldcontains_keycheck across eviction;last_peer_stream_idis client-only here (server never receives PUSH_PROMISE), so it cannot collide with the server-side HEADERS watermark.
Extended reasoning...
Overview
The PR adds a per-peer stream-id high-water mark (last_peer_stream_id) to Connection in src/runtime/api/bun/h2/connection.rs and uses it in two places: (1) on a server, an inbound HEADERS whose id has no entry and is at or below the mark is decoded for HPACK sync and otherwise ignored (no stream, no callback, no wire response), matching nghttp2's NGHTTP2_ERR_IGN_HEADER_BLOCK; (2) on a client, a PUSH_PROMISE whose promised id is at or below the mark is a connection PROTOCOL_ERROR. A new header_stream_ignored flag threads the ignored case through the CONTINUATION-assembly path into finish_header_block. Six raw-socket tests are added to test/js/node/http2/h2-conformance.test.ts.
Security risks
This is the HTTP/2 frame parser processing untrusted network bytes. The change tightens behaviour (rejects/ignores previously-accepted frames) rather than loosens it, and the PR description points out that the pre-fix behaviour let a client make a stream id repeat under an embedder that frees keyed on id (rewrite_read), so the fix itself is security-positive. I checked that the new watermark is monotone (only assigned when hdr.stream_id > last_peer_stream_id is implied by !ignored), that ignored blocks still run through HPACK so §4.3 table sync is preserved, and that a fatal HPACK decode still tears the connection down before the stream_ignored early return.
Level of scrutiny
High. This is core protocol state-machine logic in a network-facing parser, and the chosen behaviour (silently ignore rather than GOAWAY) is a deliberate deviation from the RFC's literal text, justified by nghttp2 parity and the §5.1 late-trailer race. That reasoning is sound and well-documented in-source, but it is exactly the kind of non-obvious protocol judgement a maintainer should sign off on.
Other factors
Test coverage is thorough: lower-id after higher, replay on a completed id, HPACK dynamic-table sync across an ignored HEADERS+CONTINUATION, re-send on a refused id, a guard that the server's own even push ids do not raise the client-side watermark, and the client PUSH_PROMISE ordering case with wire-level GOAWAY and ERR_HTTP2_ERROR assertions. The tests reuse the file's existing RawH2/RawH2Server harness and follow its conventions (the { id, sync: undefined } vs { id } toEqual pattern already appears in exhaustedSession). No CODEOWNERS entry covers this path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 9:29 AM PT - Oct 1st, 2026
✅ @robobun, your commit 875fbf94b68b7a9eb76545369b8a7f2363509919 passed in 🧪 To try this PR locally: bunx bun-pr 37563That installs a local version of the PR into your bun-37563 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the HTTP/2 engine's stream state machine on a network-facing path (and deliberately follows nghttp2's ignore behaviour rather than the RFC's literal connection error), a human look would still be worthwhile.
Checked: the ignored guard is server-only so client response HEADERS are unaffected; header_stream_ignored is threaded through CONTINUATION and reset alongside the other header_* flags; the early return in the decode path leaves header_end_stream/header_target set but they're overwritten by the next block (same as the existing refused path); the now-redundant contains_key guard in handle_push_promise is harmless.
Extended reasoning...
Overview
Adds a last_peer_stream_id high-water mark to the h2 Connection and a header_stream_ignored flag. On a server, an inbound HEADERS with no map entry and an id at or below the mark is decoded (for HPACK §4.3 sync, CONTINUATION included) and then dropped — no stream opened, nothing surfaced, nothing sent. On a client, a PUSH_PROMISE whose promised id does not exceed the mark is a connection PROTOCOL_ERROR. Nine raw-socket conformance tests are added, seven of which fail on the unfixed binary and two of which pin the per-peer semantics that distinguish the new mark from last_stream_id.
Security risks
The bug being fixed had a security angle: the embedder assumes stream ids never repeat within a session (the description points at a free in rewrite_read), and a hostile client could previously make them repeat by replaying a completed id. The fix closes that. The new behaviour is strictly more restrictive on the server and equally restrictive on the client (the contains_key check is subsumed by <= last_peer_stream_id). I did not find a way for the new code to loosen validation. The only user-controlled input flowing into the new state is the 31-bit stream id, already masked.
Level of scrutiny
High. This is a protocol state-machine change in production Rust on a network-facing path, with an intentional deviation from RFC 9113's literal wording (ignore vs GOAWAY) justified by nghttp2 precedent and the §5.1 late-frames tolerance rule. That is a design decision a maintainer should sign off on rather than a mechanical fix. The interaction between ignored, refused, stream_closed, HPACK decoding, CONTINUATION assembly, and last_stream_id advancement is subtle enough that it warrants a human pass over the state machine.
Other factors
The test coverage is thorough (both directions, HPACK sync across CONTINUATION, refused-then-replayed, late trailers, per-peer vs last_stream_id) and follows the file's existing raw-socket harness conventions. The comment-cop threads were all resolved after the author shortened the comments; the remaining two- and three-line comments record the nghttp2-vs-RFC decision, which is exactly the kind of non-obvious protocol choice that should be documented. No human review has landed yet.
|
Heads up: the server-side half of this (request HEADERS on a stream id the client already used → decode for HPACK, then drop) landed independently as #37985, rebased on top of #37637's What #37985 does not cover is the client direction here — rejecting a PUSH_PROMISE whose promised id is at or below the previous one once the earlier promised stream has been evicted. If you rebase this down to just that |
|
Thanks, will do: rebasing this onto current main and cutting it down to the PUSH_PROMISE check plus its tests. |
…he previous one RFC 9113 5.1.1 numbers every new promised stream above all earlier ones. The client only caught a reused or lower id while the earlier promised stream still had an engine entry; once that stream closed and was evicted the promise was accepted and delivered again. Track the highest promised id (the peer high-water mark, declared as in #37985 so the two merge cleanly in either order) and fail the session like nghttp2 does.
64c2368 to
3a6fe2d
Compare
robobun
changed the title
|
Done in 3a6fe2d: rebased onto main and cut down to the Two server-side scenarios from the earlier revision that I did not see in #37985's tests, in case they are useful there: a block re-sent on an id that was refused for |
…he promised id tests concurrent
|
Pushed d7488a3 on top of the main merge: the On the node question: node v26.3.0 rejects both bad promises with The CI failures at de8ab37 are not related to this change: |
|
@robobun please run the tests this PR adds or changes on Node.js v26.3.0 and report whether each one passes or fails. |
…-stream-id-high-water-mark
…he GOAWAY code node v26.3.0 rejects the same PUSH_PROMISE frames with ERR_HTTP2_ERROR (errno -505, "Protocol error") but writes INTERNAL_ERROR in its GOAWAY, because session.destroy(err) runs before nghttp2 sends its own. Assert the error both runtimes surface so the tests pass on node too.
|
Ran the four tests this PR adds under Node.js v26.3.0. Method: the Results with the tests as they were on the PR (d7488a3):
The two node failures were the same line: Fixed in 064097e: Results after the change:
The bun 1.4.3 column is the unfixed binary, so the two rejection tests still exercise the |
This branch has not been deployed
3 participants
Problem
http2.connect()accepts a PUSH_PROMISE whose promised id is below an earlier one, or equal to one whose stream has closed. For promise 4 then 2, bun 1.4.3 delivers both streams. node v26.3.0 fails the session withERR_HTTP2_ERROR.Connection::handle_push_promise(src/runtime/api/bun/h2/connection.rs) only checksstreams.contains_key(promised). A lower id was never in that map, and a closed stream has left it.Fix
Connectionkeeps the highest peer-initiated stream id inlast_peer_stream_id.handle_push_promiserejects a promised id at or below it with the existing GOAWAY(PROTOCOL_ERROR).test/js/node/http2/h2-conformance.test.ts. Two of four tests fail on bun 1.4.3. All four pass with the fix and on node v26.3.0.Background
last_stream_id. That mark also rises when a response arrives on the client's own stream. It then rejects a legal push below that id (last test).Downsides
Connectiongrows from 448 to 456 bytes per session (size_of). Each received PUSH_PROMISE does two more integer comparisons and one store. Release binary size: not measured.Notes
last_peer_stream_idfield and raises it for request ids. The declarations match, so the two merge in either order. nghttp2 keeps the same mark aslast_recv_stream_id.onSessionInternalErrorcallssession.destroy(err), anddestroywith an Error always encodes INTERNAL_ERROR before nghttp2 sends its own GOAWAY. The tests therefore assert the session error ({ code: "ERR_HTTP2_ERROR", errno: -505, message: "Protocol error" }) andclient.destroyed, not the GOAWAY code. That makes them pass on both runtimes.last_stream_id=3(node: 0). With requests sent but no response yet it writes 0, andsession.close()writes 0.local_connection_errorwriteslast_stream_id, whichhandle_headersraises for a response on the client's own stream. A bun or node server that receives such a GOAWAY fails the session withERR_HTTP2_ERROR. This PR does not change the GOAWAY payload. node:http2: name only peer streams in GOAWAY and never raise the id (RFC 9113 6.8) #37588 changes that shared connection-error path to write the peer-initiated mark.RawH2Server,requestHeaderBlock, frame constants) are copied verbatim into one.mtsfile. A smallexpect/testshim overnode:assertruns them. Results per test are in the PR thread.last_stream_id.size_of::<Connection>()read from a compile error (const _: [(); 0] = [(); size_of::<Connection>()];) on main (448) and on this branch (456), x86_64 Linux. The release binary size delta needs two release builds and was not measured. The change is one condition and one store in one function, with no new allocation, syscall or host function.h2-conformance.test.ts(88 tests) andnode-http2.test.json every lane. On a local debug build the four tests pass. One RST_STREAM flood test from node:http2: rate-limit stream resets per connection (CVE-2023-44487 rapid reset, CVE-2025-8671 MadeYouReset) #36230 and nine "DATA payload survives its ArrayBuffer being detached" tests time out at the 5 s default there.clientWithRawServerdoes not release its session and raw server when its own setupwaitFortimes out. The callers'finallyis not registered until the helper returns. The fix definesclose()before the wait and calls it in acatchbefore the rethrow. It is verified on node and bun, and held so that CI does not restart on a green head. It goes in with the next push.