node:http2: reject a PUSH_PROMISE whose promised id does not exceed the previous one

RO
1mo ago
+169-3

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 with ERR_HTTP2_ERROR.
  • Cause: Connection::handle_push_promise (src/runtime/api/bun/h2/connection.rs) only checks streams.contains_key(promised). A lower id was never in that map, and a closed stream has left it.

Fix

  • Connection keeps the highest peer-initiated stream id in last_peer_stream_id. handle_push_promise rejects a promised id at or below it with the existing GOAWAY(PROTOCOL_ERROR).
  • RFC 9113 section 5.1.1 requires each new stream id to exceed every id its initiator used. nghttp2 fails the same frames.
  • Verified: describe "promised stream ids" in 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

  • A server reserves even stream ids with PUSH_PROMISE and must number them upward. The engine drops a stream's map entry when the stream closes.
  • Considered a check against the existing 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

  • Behavior change: a server can reuse or lower a promised id. Its clients now get a session error, not the pushed stream. node does the same.
  • Cost: Connection grows 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
  • Relation to #37985 (server direction, open): it declares the same last_peer_stream_id field and raises it for request ids. The declarations match, so the two merge in either order. nghttp2 keeps the same mark as last_recv_stream_id.
  • GOAWAY code on the wire: bun writes PROTOCOL_ERROR (the RFC's connection error). node writes INTERNAL_ERROR, because onSessionInternalError calls session.destroy(err), and destroy with 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" }) and client.destroyed, not the GOAWAY code. That makes them pass on both runtimes.
  • GOAWAY Last-Stream-ID, measured with release bun 1.4.3: a client connection error after a response on its own stream 3 writes last_stream_id=3 (node: 0). With requests sent but no response yet it writes 0, and session.close() writes 0. local_connection_error writes last_stream_id, which handle_headers raises for a response on the client's own stream. A bun or node server that receives such a GOAWAY fails the session with ERR_HTTP2_ERROR. This PR does not change the GOAWAY payload. #37588 changes that shared connection-error path to write the peer-initiated mark.
  • Node run method: the describe block and its helpers (RawH2Server, requestHeaderBlock, frame constants) are copied verbatim into one .mts file. A small expect/test shim over node:assert runs them. Results per test are in the PR thread.
  • The two acceptance tests (a higher promise after a reset one, a promise numbered below the client's own streams) pass on unfixed bun too. They guard against an over-strict check and against pointing the check at last_stream_id.
  • Measurements: 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.
  • Other suites: CI passes the full h2-conformance.test.ts (88 tests) and node-http2.test.js on every lane. On a local debug build the four tests pass. One RST_STREAM flood test from #36230 and nine "DATA payload survives its ArrayBuffer being detached" tests time out at the 5 s default there.
  • Review nit not applied yet: clientWithRawServer does not release its session and raw server when its own setup waitFor times out. The callers' finally is not registered until the helper returns. The fix defines close() before the wait and calls it in a catch before 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.
  • Earlier revision: this PR also made the server ignore request HEADERS on a client stream id at or below the highest one already accepted. That half is superseded by #37985 and was removed at the maintainer's request.