From f92bb0d3673c054a5cd65aca88075cf2511dec05 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 11 Aug 2026 17:54:06 +0000 Subject: [PATCH 1/5] node:http2: validate goaway() arguments the same way on client and server sessions Node implements goaway() once on Http2Session: opaqueData must be a Buffer/TypedArray/DataView when given, code and lastStreamID must be numbers, and a code outside the uint32 range is converted on the way to the wire rather than rejected. Bun's two session classes had drifted in both directions. ClientHttp2Session#goaway() validated nothing, so a bad code surfaced as a plain Error without a code and a string or null opaqueData was silently dropped while the GOAWAY still went out. ServerHttp2Session#goaway() range-checked the code with validateInteger, so goaway(-1), goaway(1.5) and goaway(NaN) threw ERR_OUT_OF_RANGE, and because destroy(error, code) routes its code through goaway(), a server destroy() with such a code threw halfway through and left the session with destroyed === false. Move the validation into one shared validateGoawayArguments() using Node's validators and call it from both sessions. --- src/js/node/http2.ts | 31 +++-- test/js/node/http2/node-http2.test.js | 160 ++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 11 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 86e278a77c7f..9da730d7544f 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -421,7 +421,6 @@ function isEconnresetAfterGoaway(session, error: Error): boolean { return (error as NodeJS.ErrnoException)?.code === "ECONNRESET" && session[kGoawayCode] !== undefined; } const kMaxStreams = 2 ** 32 - 1; -const kMaxUint32 = 4294967295; const kMaxInt = 4294967295; const kMaxWindowSize = 2 ** 31 - 1; const { @@ -2184,6 +2183,20 @@ function validateWindowSize(windowSize) { } hideFromStack(validateWindowSize); +// Node's Http2Session#goaway (one implementation for both session types) only type-checks the +// two numbers; a code outside the uint32 range is converted on the way to the wire (-1 goes out +// as 0xffffffff), not rejected. destroy(error, code) routes its caller's code through here too, +// so a range check would make it throw mid-teardown where Node destroys the session. +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js (Http2Session#goaway) +function validateGoawayArguments(code, lastStreamID, opaqueData) { + if (opaqueData !== undefined) { + validateBuffer(opaqueData, "opaqueData"); + } + validateNumber(code, "code"); + validateNumber(lastStreamID, "lastStreamID"); +} +hideFromStack(validateGoawayArguments); + function pushToStream(stream, data) { if (data && stream[bunHTTP2StreamStatus] & StreamState.Closed) { if (!stream._readableState.ended) { @@ -4793,12 +4806,7 @@ class ServerHttp2Session extends Http2Session { } goaway(code = NGHTTP2_NO_ERROR, lastStreamID = 0, opaqueData) { if (this.destroyed) throw $ERR_HTTP2_INVALID_SESSION(); - - if (opaqueData !== undefined) { - validateBuffer(opaqueData, "opaqueData"); - } - validateInteger(code, "code", 0, kMaxUint32); - validateNumber(lastStreamID, "lastStreamID"); + validateGoawayArguments(code, lastStreamID, opaqueData); return this.#parser?.goaway(code, lastStreamID, opaqueData); } @@ -4962,7 +4970,7 @@ class ServerHttp2Session extends Http2Session { } } catch (e) { // A throwing destroy did not destroy: argument validation (goaway's - // validateInteger, the native session rejecting a non-numeric error + // validateNumber, the native session rejecting a non-numeric error // code) throws mid-teardown, and a corrected retry must still run. this.#destroying = false; throw e; @@ -5717,9 +5725,10 @@ class ClientHttp2Session extends Http2Session { } return true; } - goaway(errorCode = constants.NGHTTP2_NO_ERROR, lastStreamId = 0, opaqueData) { + goaway(code = NGHTTP2_NO_ERROR, lastStreamID = 0, opaqueData) { if (this.destroyed) throw $ERR_HTTP2_INVALID_SESSION(); - return this.#parser?.goaway(errorCode, lastStreamId, opaqueData); + validateGoawayArguments(code, lastStreamID, opaqueData); + return this.#parser?.goaway(code, lastStreamID, opaqueData); } setLocalWindowSize(windowSize) { @@ -6049,7 +6058,7 @@ class ClientHttp2Session extends Http2Session { } } catch (e) { // A throwing destroy did not destroy: argument validation (goaway's - // validateInteger, the native session rejecting a non-numeric error + // validateNumber, the native session rejecting a non-numeric error // code) throws mid-teardown, and a corrected retry must still run. this.#destroying = false; throw e; diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index aa4900ab329e..45db367e84c9 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -1838,6 +1838,166 @@ it("http2 session.goaway() validates input types", async done => { }); }); +// node defines goaway() once, on Http2Session, so a client session rejects the same arguments +// as a server session. Every rejected call below must throw synchronously without putting a +// GOAWAY frame on the wire. +it("http2 client and server sessions validate goaway() arguments identically", async () => { + const server = http2.createServer(); + const { promise: serverSessionOpened, resolve: onServerSession } = Promise.withResolvers(); + server.once("session", onServerSession); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const client = http2.connect(`http://127.0.0.1:${server.address().port}`); + try { + await new Promise(resolve => client.once("connect", resolve)); + const serverSession = await serverSessionOpened; + + function thrown(fn) { + try { + fn(); + } catch (e) { + return `${e.name} ${e.code}: ${e.message}`; + } + return "did not throw"; + } + + for (const [label, session] of [ + ["client", client], + ["server", serverSession], + ]) { + const actual = [label]; + const expected = [label]; + for (const input of [true, "1", {}, [], null, new Date(), new ArrayBuffer(1)]) { + const received = invalidArgTypeHelper(input); + actual.push( + thrown(() => session.goaway(input)), + thrown(() => session.goaway(0, input)), + thrown(() => session.goaway(0, 0, input)), + ); + expected.push( + `TypeError ERR_INVALID_ARG_TYPE: The "code" argument must be of type number.${received}`, + `TypeError ERR_INVALID_ARG_TYPE: The "lastStreamID" argument must be of type number.${received}`, + `TypeError ERR_INVALID_ARG_TYPE: The "opaqueData" argument must be an instance of Buffer, TypedArray, or DataView.${received}`, + ); + } + expect(actual).toEqual(expected); + } + + // Frames are ordered on the wire, so the first GOAWAY the server sees being the valid one + // proves none of the rejected client calls sent anything (a string opaqueData used to). + const { promise: goawayReceived, resolve: onGoaway } = Promise.withResolvers(); + serverSession.once("goaway", (code, lastStreamID, opaqueData) => + onGoaway({ code, lastStreamID, opaqueData: opaqueData.toString() }), + ); + client.goaway(http2.constants.NGHTTP2_ENHANCE_YOUR_CALM, 0, Buffer.from("bye")); + expect(await goawayReceived).toEqual({ + code: http2.constants.NGHTTP2_ENHANCE_YOUR_CALM, + lastStreamID: 0, + opaqueData: "bye", + }); + + // Like node, the destroyed check runs before argument validation. + client.destroy(); + expect(thrown(() => client.goaway(true))).toBe("Error ERR_HTTP2_INVALID_SESSION: The session has been destroyed"); + } finally { + client.destroy(); + server.close(); + } +}); + +// node only type-checks the code: any number is accepted and converted to a uint32 on the way +// out, and destroy(error, code) hands its code to the same path. node v26.3.0 produces exactly +// this table from either side. The server session used to throw ERR_OUT_OF_RANGE on all four of +// its rows, which for destroy() also left the session half torn down with destroyed === false. +it("http2 session.goaway() and destroy() accept any numeric code like node and send it as a uint32", async () => { + const server = http2.createServer(); + server.on("sessionError", () => {}); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + + // Runs `issue` against the chosen side of a fresh connection and reports what the other side's + // 'goaway' event carried. One connection per call: the receiver tears the session down right + // after emitting a non-zero GOAWAY, so a connection can only observe one of them. + async function issueAndReadPeer(sender, issue) { + const { promise: serverSessionOpened, resolve: onServerSession } = Promise.withResolvers(); + server.once("session", onServerSession); + const client = http2.connect(`http://127.0.0.1:${server.address().port}`); + client.on("error", () => {}); + try { + await new Promise(resolve => client.once("connect", resolve)); + const serverSession = await serverSessionOpened; + const { promise: codeReceived, resolve: onGoaway } = Promise.withResolvers(); + const [session, peer] = sender === "client" ? [client, serverSession] : [serverSession, client]; + peer.once("goaway", onGoaway); + try { + issue(session); + } catch (e) { + return { threw: e.code, destroyed: session.destroyed }; + } + const destroyed = session.destroyed; + return { code: await codeReceived, destroyed }; + } finally { + client.destroy(); + } + } + + const issues = { + "goaway(-1)": session => session.goaway(-1), + "goaway(1.5)": session => session.goaway(1.5), + "goaway(NaN)": session => session.goaway(NaN), + "destroy(error, -1)": session => session.destroy(new Error("boom"), -1), + }; + try { + const results = {}; + for (const sender of ["client", "server"]) { + for (const [name, issue] of Object.entries(issues)) { + results[`${sender}.${name}`] = await issueAndReadPeer(sender, issue); + } + } + expect(results).toEqual({ + "client.goaway(-1)": { code: 0xffffffff, destroyed: false }, + "client.goaway(1.5)": { code: 1, destroyed: false }, + "client.goaway(NaN)": { code: 0, destroyed: false }, + "client.destroy(error, -1)": { code: 0xffffffff, destroyed: true }, + "server.goaway(-1)": { code: 0xffffffff, destroyed: false }, + "server.goaway(1.5)": { code: 1, destroyed: false }, + "server.goaway(NaN)": { code: 0, destroyed: false }, + "server.destroy(error, -1)": { code: 0xffffffff, destroyed: true }, + }); + } finally { + server.close(); + } +}); + +it("http2 client session.goaway() sends opaqueData given as any Buffer, TypedArray, or DataView view", async () => { + const bytes = Buffer.from("__bye"); + const views = { + Buffer: bytes.subarray(2), + Uint8Array: new Uint8Array(bytes.buffer, bytes.byteOffset + 2, 3), + DataView: new DataView(bytes.buffer, bytes.byteOffset + 2, 3), + }; + const server = http2.createServer(); + await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + try { + const received = {}; + for (const [name, view] of Object.entries(views)) { + const { promise: goawayReceived, resolve: onGoaway } = Promise.withResolvers(); + server.once("session", session => + session.once("goaway", (code, lastStreamID, opaqueData) => onGoaway(opaqueData)), + ); + const client = http2.connect(`http://127.0.0.1:${server.address().port}`); + try { + await new Promise(resolve => client.once("connect", resolve)); + client.goaway(http2.constants.NGHTTP2_NO_ERROR, 0, view); + received[name] = (await goawayReceived).toString(); + } finally { + client.destroy(); + } + } + expect(received).toEqual({ Buffer: "bye", Uint8Array: "bye", DataView: "bye" }); + } finally { + server.close(); + } +}); + it("http2 stream.close() validates input types and ranges", async () => { const server = http2.createServer(); From 25e282fe849bfac06b0a8c4eb7755273ce8d966a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:26:08 +0000 Subject: [PATCH 2/5] node:http2: shorten the validateGoawayArguments comment --- src/js/node/http2.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 9da730d7544f..472f75ec2a27 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2183,10 +2183,8 @@ function validateWindowSize(windowSize) { } hideFromStack(validateWindowSize); -// Node's Http2Session#goaway (one implementation for both session types) only type-checks the -// two numbers; a code outside the uint32 range is converted on the way to the wire (-1 goes out -// as 0xffffffff), not rejected. destroy(error, code) routes its caller's code through here too, -// so a range check would make it throw mid-teardown where Node destroys the session. +// Node only type-checks the numbers (the code is converted to a uint32 on the way out), and +// destroy(error, code) comes through here too, so a range check would make destroy() throw: // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js (Http2Session#goaway) function validateGoawayArguments(code, lastStreamID, opaqueData) { if (opaqueData !== undefined) { From 93bc3b2be05866b9c324fe8eb561567ba0170fd3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:29:09 +0000 Subject: [PATCH 3/5] node:http2: cite node for validateGoawayArguments instead of arguing the case --- src/js/node/http2.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 472f75ec2a27..99ba424a3e69 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2183,9 +2183,8 @@ function validateWindowSize(windowSize) { } hideFromStack(validateWindowSize); -// Node only type-checks the numbers (the code is converted to a uint32 on the way out), and -// destroy(error, code) comes through here too, so a range check would make destroy() throw: -// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js (Http2Session#goaway) +// Same checks, in the same order, as Http2Session#goaway (validateNumber, not a range check): +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js function validateGoawayArguments(code, lastStreamID, opaqueData) { if (opaqueData !== undefined) { validateBuffer(opaqueData, "opaqueData"); From 47823a7fbc23f379fffff480a9e1e7aadc78eda5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:32:24 +0000 Subject: [PATCH 4/5] node:http2: drop the validateGoawayArguments comment, the test carries the node reference --- src/js/node/http2.ts | 2 -- test/js/node/http2/node-http2.test.js | 9 +++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 99ba424a3e69..2d41ac54cc7b 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -2183,8 +2183,6 @@ function validateWindowSize(windowSize) { } hideFromStack(validateWindowSize); -// Same checks, in the same order, as Http2Session#goaway (validateNumber, not a range check): -// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js function validateGoawayArguments(code, lastStreamID, opaqueData) { if (opaqueData !== undefined) { validateBuffer(opaqueData, "opaqueData"); diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 45db367e84c9..77374e4d6c70 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -1904,10 +1904,11 @@ it("http2 client and server sessions validate goaway() arguments identically", a } }); -// node only type-checks the code: any number is accepted and converted to a uint32 on the way -// out, and destroy(error, code) hands its code to the same path. node v26.3.0 produces exactly -// this table from either side. The server session used to throw ERR_OUT_OF_RANGE on all four of -// its rows, which for destroy() also left the session half torn down with destroyed === false. +// node's Http2Session#goaway (https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js) +// only type-checks the code: any number is accepted and converted to a uint32 on the way out, and +// destroy(error, code) hands its code to the same path. node v26.3.0 produces exactly this table +// from either side. The server session used to throw ERR_OUT_OF_RANGE on all four of its rows, +// which for destroy() also left the session half torn down with destroyed === false. it("http2 session.goaway() and destroy() accept any numeric code like node and send it as a uint32", async () => { const server = http2.createServer(); server.on("sessionError", () => {}); From 1cfab5ffcd849cbd222b5dd08bd0eb232b06c14c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Wed, 12 Aug 2026 00:51:19 +0000 Subject: [PATCH 5/5] test(http2): reject the goaway test waits when the session errors or closes first --- test/js/node/http2/node-http2.test.js | 54 ++++++++++++++++----------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 77374e4d6c70..117e4758e87d 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -1838,18 +1838,28 @@ it("http2 session.goaway() validates input types", async done => { }); }); +// Resolves with the arguments of the next `event`; rejects instead if the emitter errors or closes +// first, so a frame that never shows up fails with the reason rather than with the test timeout. +// The listeners are removed once settled, so errors a test expects later stay its own business. +function nextEvent(emitter, event) { + const { promise, resolve, reject } = Promise.withResolvers(); + const onEvent = (...args) => resolve(args); + const onClose = () => reject(new Error(`'close' was emitted before '${event}'`)); + emitter.once(event, onEvent).once("error", reject).once("close", onClose); + return promise.finally(() => emitter.off(event, onEvent).off("error", reject).off("close", onClose)); +} + // node defines goaway() once, on Http2Session, so a client session rejects the same arguments // as a server session. Every rejected call below must throw synchronously without putting a // GOAWAY frame on the wire. it("http2 client and server sessions validate goaway() arguments identically", async () => { const server = http2.createServer(); - const { promise: serverSessionOpened, resolve: onServerSession } = Promise.withResolvers(); - server.once("session", onServerSession); await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); + const serverSessionOpened = nextEvent(server, "session"); const client = http2.connect(`http://127.0.0.1:${server.address().port}`); try { - await new Promise(resolve => client.once("connect", resolve)); - const serverSession = await serverSessionOpened; + await nextEvent(client, "connect"); + const [serverSession] = await serverSessionOpened; function thrown(fn) { try { @@ -1884,12 +1894,10 @@ it("http2 client and server sessions validate goaway() arguments identically", a // Frames are ordered on the wire, so the first GOAWAY the server sees being the valid one // proves none of the rejected client calls sent anything (a string opaqueData used to). - const { promise: goawayReceived, resolve: onGoaway } = Promise.withResolvers(); - serverSession.once("goaway", (code, lastStreamID, opaqueData) => - onGoaway({ code, lastStreamID, opaqueData: opaqueData.toString() }), - ); + const goawayReceived = nextEvent(serverSession, "goaway"); client.goaway(http2.constants.NGHTTP2_ENHANCE_YOUR_CALM, 0, Buffer.from("bye")); - expect(await goawayReceived).toEqual({ + const [code, lastStreamID, opaqueData] = await goawayReceived; + expect({ code, lastStreamID, opaqueData: opaqueData.toString() }).toEqual({ code: http2.constants.NGHTTP2_ENHANCE_YOUR_CALM, lastStreamID: 0, opaqueData: "bye", @@ -1918,23 +1926,25 @@ it("http2 session.goaway() and destroy() accept any numeric code like node and s // 'goaway' event carried. One connection per call: the receiver tears the session down right // after emitting a non-zero GOAWAY, so a connection can only observe one of them. async function issueAndReadPeer(sender, issue) { - const { promise: serverSessionOpened, resolve: onServerSession } = Promise.withResolvers(); - server.once("session", onServerSession); + const serverSessionOpened = nextEvent(server, "session"); const client = http2.connect(`http://127.0.0.1:${server.address().port}`); + // The non-zero codes below make whichever side receives them destroy itself with + // ERR_HTTP2_SESSION_ERROR, and the destroy() row emits its error on the issuing side. client.on("error", () => {}); try { - await new Promise(resolve => client.once("connect", resolve)); - const serverSession = await serverSessionOpened; - const { promise: codeReceived, resolve: onGoaway } = Promise.withResolvers(); + await nextEvent(client, "connect"); + const [serverSession] = await serverSessionOpened; const [session, peer] = sender === "client" ? [client, serverSession] : [serverSession, client]; - peer.once("goaway", onGoaway); try { issue(session); } catch (e) { return { threw: e.code, destroyed: session.destroyed }; } const destroyed = session.destroyed; - return { code: await codeReceived, destroyed }; + // The frame just written cannot reach the peer before this tick yields, so listening for it + // only now is safe and leaves nothing pending on the `threw` path above. + const [code] = await nextEvent(peer, "goaway"); + return { code, destroyed }; } finally { client.destroy(); } @@ -1980,15 +1990,15 @@ it("http2 client session.goaway() sends opaqueData given as any Buffer, TypedArr try { const received = {}; for (const [name, view] of Object.entries(views)) { - const { promise: goawayReceived, resolve: onGoaway } = Promise.withResolvers(); - server.once("session", session => - session.once("goaway", (code, lastStreamID, opaqueData) => onGoaway(opaqueData)), - ); + const serverSessionOpened = nextEvent(server, "session"); const client = http2.connect(`http://127.0.0.1:${server.address().port}`); try { - await new Promise(resolve => client.once("connect", resolve)); + await nextEvent(client, "connect"); + const [serverSession] = await serverSessionOpened; + const goawayReceived = nextEvent(serverSession, "goaway"); client.goaway(http2.constants.NGHTTP2_NO_ERROR, 0, view); - received[name] = (await goawayReceived).toString(); + const [, , opaqueData] = await goawayReceived; + received[name] = opaqueData.toString(); } finally { client.destroy(); }