node:http2: don't let a header block that fails validation reach the HPACK encoder

Closed
RO
2mo ago
+377-195

Rebased past #33909 (the per-crate thiserror migration) and squashed: the three consolidated encode_header_into_list error handlers now match crate::Error::Alloc(_) instead of the old bun_core::err!("OutOfMemory"). No other textual overlap.

Overlaps #32995, which hoists request()'s options validation above the encode to fix the same desync from the other side. This PR splits the header walk into a validate pass and an encode pass, which is the follow-up #32995 names under "Intentionally excluded, same bug class, different fix", and then moves that encode pass below the options too, so the whole class is closed in one place. #32995 keeps its own value (parent: 0 is accepted, weight is clamped, PADDED+PRIORITY frame layout) and the two conflict textually in request() and in node-http2.test.js; whichever lands first, the other gets rebased.

Repro

import http2 from "node:http2";
// server: a plain node http2.createServer answering 200 to everything
const ses = http2.connect(`http://127.0.0.1:${port}`);
let sessionErrors = 0;
ses.on("error", e => { sessionErrors++; console.log("SESSION 'error':", e.code); });
const plain = tag => new Promise(r => {
  const ev = [], st = ses.request({ ":path": "/" + tag }, { endStream: true });
  st.on("response", h => ev.push("response:" + h[":status"])); st.on("data", () => {});
  st.on("error", e => ev.push("error:" + e.code));
  st.on("close", () => { ev.push("rst=" + st.rstCode); r(ev); });
});
console.log("#1", await plain("one"));
try { ses.request({ ":path": "/bad", ":bogus": "x" }, { endStream: true }); }
catch (e) { console.log("threw synchronously:", e.code, "(caught)"); }
console.log("#2", await plain("two"));
console.log("#3", await plain("three"));
console.log("session 'error' events:", sessionErrors);
node v26.3.0bun 1.4.0 / main
the throwERR_HTTP2_INVALID_PSEUDOHEADER (caught)ERR_HTTP2_INVALID_PSEUDOHEADER (caught)
session 'error' events01, ERR_HTTP2_INVALID_PSEUDOHEADER
#2 / #3response:200, rst=0error:ERR_HTTP2_STREAM_ERROR, rst=1

A caught argument-validation error from request() permanently breaks the session: every later request on it is reset by the peer, and re-reporting the caught error as a session 'error' takes down a process that has no session error handler. The same trigger applies to stream.sendTrailers() and stream.pushStream().

Cause

Two independent defects, both on the path a throwing request() takes.

The HPACK encoder is advanced before the request is known to be sendable. One encoder, and so one dynamic table, serves every header block on a session, and the peer's decoder mirrors it entry for entry (RFC 7541 §4). request() in h2_frame_parser.rs validated and encoded each field in the same step, so when :bogus throws, :path: /bad has already been inserted into the encoder's dynamic table. The peer never receives that block, so its decoder's table is one entry behind from then on: every later block resolves its indexed references to the wrong entries, and the peer answers with RST_STREAM(PROTOCOL_ERROR) or GOAWAY(COMPRESSION_ERROR).

The desync only happens when a field encoded before the rejection inserts into the dynamic table, which is why the existing rejects header names longer than 4096 bytes test passes today: its :path: / is a static-table hit. Swapping the value to /bad breaks the session, and moving :bogus ahead of :path (nothing encoded before the throw) leaves it healthy.

request() rejects a request in four more places, and every one of them ran after the block was encoded:

rejectiontrigger
a bad request optionsilent, endStream, exclusive, parent, weight, signal, or a throwing getter
maxSessionMemory exceededENHANCE_YOUR_CALM
a field lshpack cannot encodename + value over its 65536-byte scratch buffer
maxSendHeaderBlockLength exceededmeasured on the encoded block

sendTrailers() and pushStream() encode field-by-field and have the header half of the defect.

The throw is also emitted on the session. ClientHttp2Session.request()'s catch re-dispatched the error through emitErrorNT. Node treats a request() that fails its own validation as purely the caller's problem.

Fix

  • src/runtime/api/bun/h2_frame_parser.rs: PendingHeaderBlock collects each validated name/value/never-index triple, and the encoder runs in a single pass afterwards, once the whole block is known good. Used by request() (which also backs respond() and additionalHeaders()), send_trailers() and push_promise(). In request() that pass now sits below every rejection listed above, so nothing can abandon an encoded block. The walk order, validation order and error codes are unchanged, so the same input still fails the same way; it just fails before anything is encoded. The three duplicated encode_header_into_list error handlers collapse into one, and inline capacity (SmallVec) keeps an ordinary block off the heap.
  • maxSendHeaderBlockLength is now measured against an upper bound on the encoded size rather than the encoded block, which is both what nghttp2 measures it against (nghttp2_hd_deflate_bound) and the only way to check it without advancing the encoder. Bun's behavior now matches node v26.3.0 on the repro below.
  • src/js/node/http2.ts: drop the session 'error' emit from request()'s catch, keeping the #connections-- that balances the slot streamStart took, and the graceful-close completion (destroy() once the last slot is handed back on a closing session) that every sibling decrement site makes. emitErrorNT had no other caller and is deleted.

send_trailers() and push_promise() get the same oversized-field pre-check, so no site advances the encoder for a field it cannot encode. What each one then does is unchanged: request() resets the stream, push_promise() throws, and send_trailers() resets the stream and takes the session down with a graceful GOAWAY, because nghttp2 measures a trailer block that large against its own 64KB limit and treats overrunning it as fatal (test-http2-exceeds-server-trailer-size asserts the client sees the session close). Keeping the encoder clean there still buys something: the GOAWAY is graceful, so streams still draining under it do not encode against a dirty table.

An encode_header_into_list failure that survives those pre-checks is an lshpack fault rather than bad input, and leaves the encoder in an unknown state, so all three sites take the session down with the graceful GOAWAY send_trailers() already used.

Verification

Two tests in test/js/node/http2/node-http2.test.js, alongside the existing header-validation coverage. Every :path is chosen to be absent from the HPACK static table so that encoding it inserts into the dynamic table, and after each rejection the next request on the session must still get a 200.

  1. six rejected client requests (bad pseudo-header name, bad name token, bad value, raw [name, value, ...] array form, a bad request option, an oversized field) plus a rejected server trailer block, and no session 'error' at the end.
  2. maxSendHeaderBlockLength: an oversized request is refused with a frameError(FRAME_SIZE_ERROR) and the session stays usable, matching node v26.3.0 exactly.

The oversized-trailer path is already covered by the ported test-http2-exceeds-server-trailer-size.js, which sends a 64KiB trailer field and asserts the frameError plus the session close. One commit here got that case wrong (resetting only the stream) and that test caught it.

$ USE_SYSTEM_BUN=1 bun test test/js/node/http2/node-http2.test.js -t "HPACK encoder"
Expected: 200
Received: "error:ERR_HTTP2_STREAM_ERROR"
(fail) http2 a request the session rejects never reaches its HPACK encoder

$ USE_SYSTEM_BUN=1 bun test test/js/node/http2/node-http2.test.js -t "maxSendHeaderBlockLength refuses"
error: Stream closed with error code NGHTTP2_COMPRESSION_ERROR
(fail) http2 maxSendHeaderBlockLength refuses a request without desyncing the session

$ bun bd test test/js/node/http2/node-http2.test.js -t "HPACK encoder"
(pass) http2 a request the session rejects never reaches its HPACK encoder
(pass) http2 maxSendHeaderBlockLength refuses a request without desyncing the session

bun bd test test/js/node/http2/ passes (372 pass, 1 timeout on user-controlled options getters, which also times out on an unmodified debug+ASAN build on this host; it sits at ~90% of its budget at rest).

Ported node http2 suite: no change

All 59 test/js/node/test/parallel/test-http2-*.js files covering header, trailer, push, pseudo-header, option, priority, abort and continuation behavior were run against the patched build: 50 pass, 9 fail. The 9 (test-http2-client-set-priority, test-http2-info-headers-errors, test-http2-options-max-headers-exceeds-nghttp2, test-http2-server-push-stream-errors, test-http2-server-push-stream-head, test-http2-single-headers-validation-disabled, test-http2-util-assert-valid-pseudoheader, test-http2-util-headers-list, test-http2-util-update-options-buffer) fail identically on stock bun 1.4.0 and are unrelated: they need internalBinding("http2"), internal/http2/util, the strictSingleValueFields option, or stream.state.weight reporting. test-http2-options-max-headers-block-length (the one that exercises the changed limit), test-http2-misused-pseudoheaders, test-http2-invalidheaderfield, test-http2-multiheaders-raw, test-http2-raw-headers, test-http2-trailers and the sensitive-header tests all pass both ways.


[review] gate passed · iteration 8 · 3 files touched

fails on main (without fix)
ASAN without fix: 2 failed, 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (0475cb022)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [865.53ms]
(pass) node none > Client Basics > should be able to send a POST request [612.75ms]
(pass) node none > Client Basics > constants [18.48ms]
(pass) node none > Client Basics > getDefaultSettings [7.80ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.40ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [5.66ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.17ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [5.01ms
... (truncated)

release without fix: 8 failed, 6 skipped
bun test v1.4.0-canary.1 (1498d7b77)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > constants [0.76ms]
(pass) node none > Client Basics > getDefaultSettings [0.17ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [0.41ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [0.12ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [0.04ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [0.08ms]
(pass) node none > Client Basics > headers cannot be bigger than 65536 bytes [1.73ms]
(pass) node none > Client Basics > is possible to abort request [1.00ms]
(pass) node none > Client Basics > aborted event should work with abortController [0.70ms]
(pass) node none > Client Basics > aborted event should work with aborted signal [0.60ms]
(pass) node none > Client Basics > signal validation matches node: non-signal objects throw, duck-typed { aborted } is accepted [0.92ms]
(skip) node none > Client Basics > should not leak memory
(pass) node none > Client Basics > close callback [15.
... (truncated)
passes on PR (with fix)
ASAN with fix: 6 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/node/http2/node-http2.test.js"
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (0475cb022)

test/js/node/http2/node-http2.test.js:
(pass) node none > Client Basics > should be able to send a GET request [734.59ms]
(pass) node none > Client Basics > should be able to send a POST request [499.40ms]
(pass) node none > Client Basics > constants [18.10ms]
(pass) node none > Client Basics > getDefaultSettings [7.10ms]
(pass) node none > Client Basics > getPackedSettings/getUnpackedSettings [22.05ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is too small [5.08ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a multiple of 6 bytes [3.17ms]
(pass) node none > Client Basics > getUnpackedSettings should throw if buffer is not a buffer [4.92ms
... (truncated)

release with fix: 6 skipped
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     0475cb0220
  features     (none)

22 deps, 106 codegen, 1168 objects in 1016ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1231] install /workspace/bun
bun install v1.4.0-canary.1 (1498d7b77)

Checked 124 installs across 170 packages (no changes) [17.00ms]
[2/1231] install /workspace/bun/packages/bun-error
bun install v1.4.0-canary.1 (1498d7b77)

Checked 1 install across 2 packages (no changes) [1.00ms]
[3/1231] install /workspace/bun/src/node-fallbacks
bun install v1.4.0-canary.1 (1498d7b77)

Checked 129 installs across 147 packages (no changes) [7.00ms]
[4/1231] gen ErrorCode+*.h
[5/1231] gen bindgenv2
[6/1231] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[7/1231] fetch picohttpparser
[picohttpparser] up to date

... (truncated)
diff hotspot
src/js/node/http2.ts                   |  20 +-
 src/runtime/api/bun/h2_frame_parser.rs | 391 ++++++++++++++++++---------------
 test/js/node/http2/node-http2.test.js  | 161 +++++++++++++-
 3 files changed, 377 insertions(+), 195 deletions(-)

gate history · 1 passed · 0 rejected · iteration 8

evidence per changed file
file                                    reads  edits  tests
src/js/node/http2.ts                        3      4      0
src/runtime/api/bun/h2_frame_parser.rs     22     34      0
test/js/node/http2/node-http2.test.js       9     13      0