From 24682d526a0c732aac73a5d39d414f81a2e92f5a Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:31:35 +0000 Subject: [PATCH 1/3] node:http2: check settings before callback in session.settings() Http2Session#settings() validated the callback before the settings and rejected every non-function callback except undefined. Node validates the settings first and validates the callback only when it is truthy, so null, 0, "" and false are accepted and ignored. Http2Server#setTimeout() and Http2SecureServer#setTimeout() had the same hand-written callback check ahead of the timeout assignment. Node assigns the timeout first, then validates a callback that is not undefined. --- src/js/node/http2.ts | 28 +++---- test/js/node/http2/node-http2.test.js | 105 ++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 14 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index abfd52512f03..f870c142e65b 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -4620,15 +4620,15 @@ class ServerHttp2Session extends Http2Session { settings(settings: Settings, callback?) { if (this.destroyed) throw $ERR_HTTP2_INVALID_SESSION(); - if (callback !== undefined && typeof callback !== "function") { - throw $ERR_INVALID_ARG_TYPE("callback", "function", callback); - } // node treats an omitted/undefined settings object as an empty update. if (settings === undefined) settings = {} as Settings; // Validate the caller-supplied object FIRST so null / arrays / primitives // still throw ERR_INVALID_ARG_TYPE — spreading ({ ...null }) would hide // these from the type guard in validateSettings. validateSettings(settings); + // node validates the callback after the settings, and only a truthy one: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1559-L1564 + if (callback) validateFunction(callback, "callback"); // RFC 9113 §6.5.2: a server MUST NOT advertise SETTINGS_ENABLE_PUSH != 0. // Force-override whatever the caller passes so a mid-connection SETTINGS // frame stays compliant (the initial SETTINGS frame already clamps this @@ -5553,12 +5553,12 @@ class ClientHttp2Session extends Http2Session { settings(settings: Settings, callback?) { if (this.destroyed) throw $ERR_HTTP2_INVALID_SESSION(); - if (callback !== undefined && typeof callback !== "function") { - throw $ERR_INVALID_ARG_TYPE("callback", "function", callback); - } // node treats an omitted/undefined settings object as an empty update. if (settings === undefined) settings = {} as Settings; validateSettings(settings); + // node validates the callback after the settings, and only a truthy one: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1559-L1564 + if (callback) validateFunction(callback, "callback"); // node: when more SETTINGS are submitted than maxOutstandingSettings allows un-ACKed, the // session is destroyed with ERR_HTTP2_MAX_PENDING_SETTINGS_ACK (surfaced via 'error'). this.#pendingSettingsAckCount++; @@ -6493,11 +6493,11 @@ class Http2Server extends net.Server { } setTimeout(ms, callback) { - if (callback !== undefined && typeof callback !== "function") { - throw $ERR_INVALID_ARG_TYPE("callback", "function", callback); - } + // node assigns the timeout before it validates the callback: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3502-L3509 this.timeout = ms; - if (typeof callback === "function") { + if (callback !== undefined) { + validateFunction(callback, "callback"); this.on("timeout", callback); } return this; @@ -6627,11 +6627,11 @@ class Http2SecureServer extends tls.Server { return super.emit(event, ...args); } setTimeout(ms, callback) { - if (callback !== undefined && typeof callback !== "function") { - throw $ERR_INVALID_ARG_TYPE("callback", "function", callback); - } + // node assigns the timeout before it validates the callback: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3460-L3467 this.timeout = ms; - if (typeof callback === "function") { + if (callback !== undefined) { + validateFunction(callback, "callback"); this.on("timeout", callback); } return this; diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 7cf5c3e7ab3a..7a417e91bd80 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -1919,6 +1919,111 @@ it("http2 stream.close() validates input types and ranges", async () => { }); }); +// node checks `settings` first and validates only a truthy callback: +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1556-L1564 +it("http2 session.settings() checks settings before callback and ignores a falsy callback", async () => { + function outcome(fn) { + try { + fn(); + return "accepted"; + } catch (err) { + return { name: err.name, code: err.code, message: err.message }; + } + } + + function probe(session) { + const { promise, resolve, reject } = Promise.withResolvers(); + session.on("error", reject); + session.once("close", () => reject(new Error("session closed before the last SETTINGS ACK"))); + const rejected = { + "settings(null, 1)": outcome(() => session.settings(null, 1)), + "settings({ maxFrameSize: 1 }, 1)": outcome(() => session.settings({ maxFrameSize: 1 }, 1)), + "settings({}, 1)": outcome(() => session.settings({}, 1)), + }; + // Each accepted call sends its own headerTableSize, so the log shows which ACK ran which callback. + const log = []; + const callback = (err, settings) => log.push(`callback ${err} ${settings.headerTableSize}`); + const calls = [ + [1000, null], + [2000, 0], + [3000, callback], + [4000, ""], + [5000, false], + [6000, callback], + ]; + const accepted = calls.map(([size, cb]) => outcome(() => session.settings({ headerTableSize: size }, cb))); + session.on("localSettings", settings => { + const size = settings.headerTableSize; + if (!calls.some(call => call[0] === size)) return; // the ACK of the connection preface + log.push(`localSettings ${size}`); + if (size === 6000) resolve({ rejected, accepted, log }); + }); + return promise; + } + + const expected = { + rejected: { + "settings(null, 1)": { + name: "TypeError", + code: "ERR_INVALID_ARG_TYPE", + message: 'The "settings" argument must be of type object. Received null', + }, + "settings({ maxFrameSize: 1 }, 1)": { + name: "RangeError", + code: "ERR_HTTP2_INVALID_SETTING_VALUE", + message: 'Invalid value for setting "maxFrameSize": 1', + }, + "settings({}, 1)": { + name: "TypeError", + code: "ERR_INVALID_ARG_TYPE", + message: 'The "callback" argument must be of type function. Received type number (1)', + }, + }, + accepted: ["accepted", "accepted", "accepted", "accepted", "accepted", "accepted"], + // An ignored callback still takes the ACK of its own frame. + log: [ + "localSettings 1000", + "localSettings 2000", + "callback null 3000", + "localSettings 3000", + "localSettings 4000", + "localSettings 5000", + "callback null 6000", + "localSettings 6000", + ], + }; + + const server = http2.createServer(); + const serverSession = new Promise(resolve => server.once("session", resolve)); + 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, reject) => client.once("connect", resolve).once("error", reject)); + expect({ client: await probe(client), server: await probe(await serverSession) }).toEqual({ + client: expected, + server: expected, + }); + } finally { + client.destroy(); + server.close(); + } +}); + +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3502-L3509 +it("http2 server.setTimeout() assigns the timeout before it validates the callback", () => { + for (const server of [http2.createServer(), http2.createSecureServer({})]) { + expect(() => server.setTimeout(123, 1)).toThrow( + expect.objectContaining({ + name: "TypeError", + code: "ERR_INVALID_ARG_TYPE", + message: 'The "callback" argument must be of type function. Received type number (1)', + }), + ); + expect(server.timeout).toBe(123); + expect(server.listenerCount("timeout")).toBe(0); + } +}); + it("http2 session.goaway() sends custom data", async done => { const { mustCall } = createCallCheckCtx(done); From b1d698a61ba2dd5f11b5bd8d1baf8d3fa43d3061 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 20 Sep 2026 02:13:03 +0000 Subject: [PATCH 2/3] test(http2): give each server kind its own setTimeout() test case --- test/js/node/http2/node-http2.test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/js/node/http2/node-http2.test.js b/test/js/node/http2/node-http2.test.js index 7a417e91bd80..6c11a5fae929 100644 --- a/test/js/node/http2/node-http2.test.js +++ b/test/js/node/http2/node-http2.test.js @@ -2010,8 +2010,10 @@ it("http2 session.settings() checks settings before callback and ignores a falsy }); // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3502-L3509 -it("http2 server.setTimeout() assigns the timeout before it validates the callback", () => { - for (const server of [http2.createServer(), http2.createSecureServer({})]) { +it.each(["createServer", "createSecureServer"])( + "http2 %s().setTimeout() assigns the timeout before it validates the callback", + factory => { + const server = http2[factory]({}); expect(() => server.setTimeout(123, 1)).toThrow( expect.objectContaining({ name: "TypeError", @@ -2021,8 +2023,8 @@ it("http2 server.setTimeout() assigns the timeout before it validates the callba ); expect(server.timeout).toBe(123); expect(server.listenerCount("timeout")).toBe(0); - } -}); + }, +); it("http2 session.goaway() sends custom data", async done => { const { mustCall } = createCallCheckCtx(done); From e6d564e1211966638251693c97f060784c5ba282 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 20 Sep 2026 02:15:07 +0000 Subject: [PATCH 3/3] node:http2: keep one permalink line above each reordered check --- src/js/node/http2.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index f870c142e65b..0c32fdcefc06 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -4626,7 +4626,6 @@ class ServerHttp2Session extends Http2Session { // still throw ERR_INVALID_ARG_TYPE — spreading ({ ...null }) would hide // these from the type guard in validateSettings. validateSettings(settings); - // node validates the callback after the settings, and only a truthy one: // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1559-L1564 if (callback) validateFunction(callback, "callback"); // RFC 9113 §6.5.2: a server MUST NOT advertise SETTINGS_ENABLE_PUSH != 0. @@ -5556,7 +5555,6 @@ class ClientHttp2Session extends Http2Session { // node treats an omitted/undefined settings object as an empty update. if (settings === undefined) settings = {} as Settings; validateSettings(settings); - // node validates the callback after the settings, and only a truthy one: // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1559-L1564 if (callback) validateFunction(callback, "callback"); // node: when more SETTINGS are submitted than maxOutstandingSettings allows un-ACKed, the @@ -6493,7 +6491,6 @@ class Http2Server extends net.Server { } setTimeout(ms, callback) { - // node assigns the timeout before it validates the callback: // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3502-L3509 this.timeout = ms; if (callback !== undefined) { @@ -6627,7 +6624,6 @@ class Http2SecureServer extends tls.Server { return super.emit(event, ...args); } setTimeout(ms, callback) { - // node assigns the timeout before it validates the callback: // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L3460-L3467 this.timeout = ms; if (callback !== undefined) {