diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 9c7747d20737..f053942817f2 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -1960,14 +1960,6 @@ function createPendingStreamCancelError(cause?: any) { cancelError.code = "ERR_HTTP2_STREAM_CANCEL"; return cancelError; } -// The native settings object for a server session: session options + the user's settings, with -// enablePush forced off only when the caller explicitly provided it (see the RFC 9113 §6.5.2 note -// at the construction site). Only explicitly-present settings are serialized by the native layer. -function serverNativeSettings(options) { - const merged = { ...options, ...options?.settings }; - if (merged.enablePush !== undefined) merged.enablePush = false; - return merged; -} function sessionErrorFromCode(code: number) { if (code === 0xe) { @@ -4469,7 +4461,7 @@ class ServerHttp2Session extends Http2Session { if (typeof options?.maxOutstandingSettings === "number" && options.maxOutstandingSettings >= 1) { this.#maxOutstandingSettings = options.maxOutstandingSettings; } - const advertisedMaxConcurrentStreams = options?.settings?.maxConcurrentStreams ?? options?.maxConcurrentStreams; + const advertisedMaxConcurrentStreams = options?.settings?.maxConcurrentStreams; if (typeof advertisedMaxConcurrentStreams === "number") { this.#advertisedMaxConcurrentStreams = advertisedMaxConcurrentStreams; } @@ -4477,18 +4469,11 @@ class ServerHttp2Session extends Http2Session { if (options?.settings !== undefined) { validateSettings(options.settings); } - const nativeSettings = serverNativeSettings(options); - this.#localSettings = initialLocalSettings(nativeSettings); + this.#localSettings = initialLocalSettings(options?.settings); this.#parser = new H2FrameParser({ native: nativeSocket, context: this, - // RFC 9113 §6.5.2: a server MUST NOT send SETTINGS_ENABLE_PUSH with a - // value other than 0 — any non-zero value is treated by a client as a - // PROTOCOL_ERROR (nghttp2 reports this as callback failure). When the - // caller asked for enablePush it is forced to false; when it is absent - // the setting is simply never serialized (node's initial SETTINGS frame - // is empty for default options). - settings: nativeSettings, + options, type: 0, // server type handlers: ServerHttp2Session.#Handlers, }); @@ -5714,12 +5699,11 @@ class ClientHttp2Session extends Http2Session { if (options?.settings !== undefined) { validateSettings(options.settings); } - const nativeSettings = { ...options, ...options?.settings }; - this.#localSettings = initialLocalSettings(nativeSettings); + this.#localSettings = initialLocalSettings(options?.settings); // #onConnect attaches the native socket; frames written before that (the preface) queue. this.#parser = new H2FrameParser({ context: this, - settings: nativeSettings, + options, handlers: ClientHttp2Session.#Handlers, }); socket.on("data", this.#onRead.bind(this)); diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index 0047486e23b6..04e6405ad4e7 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -4281,7 +4281,8 @@ impl H2FrameParser { if let Some(v) = options.get(global_object, $key)? { if v.is_number() { let value = v.as_number(); - if value < ($min as f64) || value > $max { + // `contains`, not `<`/`>`: NaN compares false with both bounds. + if !(($min as f64)..=$max).contains(&value) { return global_object .err_http2_invalid_setting_value_range_error($err) .throw(); @@ -4324,7 +4325,7 @@ impl H2FrameParser { if let Some(v) = options.get(global_object, "initialWindowSize")? { if v.is_number() { let value = v.as_number(); - if value < 0.0 || value > MAX_WINDOW_SIZE_F64 { + if !(0.0..=MAX_WINDOW_SIZE_F64).contains(&value) { return global_object .err_http2_invalid_setting_value_range_error( "Expected initialWindowSize to be a number between 0 and 2^32-1", @@ -4446,7 +4447,7 @@ impl H2FrameParser { // Validate setting value is in range [0, 2^32-1] if setting_value.is_number() { let value = setting_value.as_number(); - if value < 0.0 || value > MAX_HEADER_TABLE_SIZE_F64 { + if !(0.0..=MAX_HEADER_TABLE_SIZE_F64).contains(&value) { return global_object .err_http2_invalid_setting_value_range_error( "Invalid custom setting value", @@ -4465,32 +4466,6 @@ impl H2FrameParser { } } - // remoteCustomSettings (session option, not a SETTINGS parameter): non-standard setting - // ids whose received values should be exposed on remoteSettings.customSettings. Staged - // before any state is committed so a throwing getter / iterator (Proxy/getter on the - // user array) does not leave the four cells above already installed. - let mut staged_remote_filter: Vec = Vec::new(); - if let Some(remote_custom) = options.get(global_object, "remoteCustomSettings")? { - if remote_custom.is_array() { - let mut value_iter = remote_custom.array_iterator(global_object)?; - while let Some(item) = value_iter.next()? { - if !item.is_number() { - continue; - } - let id = item.as_number(); - if !(0.0..=65535.0).contains(&id) { - continue; - } - let id = id as u16; - if !staged_remote_filter.contains(&id) - && staged_remote_filter.len() < MAX_CUSTOM_SETTINGS - { - staged_remote_filter.push(id); - } - } - } - } - self.local_settings.set(local_settings); self.explicit_settings.set(explicit_settings); self.custom_settings.with_mut(|cs| { @@ -4503,13 +4478,6 @@ impl H2FrameParser { } }); self.wire_custom_settings.with_mut(|cs| *cs = staged_custom); - self.remote_custom_settings_filter.with_mut(|f| { - for id in staged_remote_filter { - if !f.contains(&id) && f.len() < MAX_CUSTOM_SETTINGS { - f.push(id); - } - } - }); Ok(()) } @@ -7689,25 +7657,48 @@ impl H2FrameParser { let _ = this_ref.flush(); } } - if let Some(settings_js) = options.get(global_object, "settings")? { - if !settings_js.is_empty_or_undefined_or_null() { - bun_output::scoped_log!(H2FrameParser, "settings received in the constructor"); - this_ref.load_settings_from_js_value(global_object, settings_js)?; - // The constructor settings ride on the connection preface, so received header - // blocks are checked against them right away; later settings() submissions only - // take effect for enforcement once the peer ACKs them. - this_ref - .enforced_max_header_list_size - .set(this_ref.local_settings.get().max_header_list_size); + let mut is_server = false; + if let Some(type_js) = options.get(global_object, "type")? { + is_server = type_js.is_number() && type_js.to_u32() == 0; + } - if let Some(max_pings) = settings_js.get(global_object, "maxOutstandingPings")? { + // Node reads SETTINGS from `options.settings` and the session limits from the top level. + if let Some(session_options) = options.get(global_object, "options")? { + if session_options.is_object() { + if let Some(settings_js) = session_options.get(global_object, "settings")? { + if !settings_js.is_empty_or_undefined_or_null() { + bun_output::scoped_log!( + H2FrameParser, + "settings received in the constructor" + ); + this_ref.load_settings_from_js_value(global_object, settings_js)?; + // RFC 9113 §6.5.2: a server MUST NOT advertise ENABLE_PUSH other than 0. + if is_server + && this_ref.explicit_settings.get() & SETTING_BIT_ENABLE_PUSH != 0 + { + let mut local_settings = this_ref.local_settings.get(); + local_settings.enable_push = 0; + this_ref.local_settings.set(local_settings); + } + // The constructor settings ride on the connection preface, so received + // header blocks are checked against them right away; later settings() + // submissions only take effect for enforcement once the peer ACKs them. + this_ref + .enforced_max_header_list_size + .set(this_ref.local_settings.get().max_header_list_size); + } + } + + if let Some(max_pings) = + session_options.get(global_object, "maxOutstandingPings")? + { if max_pings.is_number() { this_ref .max_outstanding_pings .set(max_pings.to_uint64_no_truncate()); } } - if let Some(max_memory) = settings_js.get(global_object, "maxSessionMemory")? { + if let Some(max_memory) = session_options.get(global_object, "maxSessionMemory")? { if max_memory.is_number() { this_ref .max_session_memory @@ -7715,7 +7706,7 @@ impl H2FrameParser { } } if let Some(max_header_list_pairs) = - settings_js.get(global_object, "maxHeaderListPairs")? + session_options.get(global_object, "maxHeaderListPairs")? { if max_header_list_pairs.is_number() { this_ref @@ -7723,7 +7714,7 @@ impl H2FrameParser { .set(Self::session_option_u32(max_header_list_pairs).max(4)); } } - if let Some(max_settings) = settings_js.get(global_object, "maxSettings")? { + if let Some(max_settings) = session_options.get(global_object, "maxSettings")? { if max_settings.is_number() { this_ref .max_settings @@ -7731,7 +7722,7 @@ impl H2FrameParser { } } if let Some(max_rejected_streams) = - settings_js.get(global_object, "maxSessionRejectedStreams")? + session_options.get(global_object, "maxSessionRejectedStreams")? { if max_rejected_streams.is_number() { this_ref @@ -7740,7 +7731,7 @@ impl H2FrameParser { } } if let Some(max_session_invalid_frames) = - settings_js.get(global_object, "maxSessionInvalidFrames")? + session_options.get(global_object, "maxSessionInvalidFrames")? { if max_session_invalid_frames.is_number() { this_ref @@ -7749,7 +7740,7 @@ impl H2FrameParser { } } if let Some(max_outstanding_settings) = - settings_js.get(global_object, "maxOutstandingSettings")? + session_options.get(global_object, "maxOutstandingSettings")? { if max_outstanding_settings.is_number() { this_ref @@ -7758,7 +7749,7 @@ impl H2FrameParser { } } if let Some(max_send_header_block_length) = - settings_js.get(global_object, "maxSendHeaderBlockLength")? + session_options.get(global_object, "maxSendHeaderBlockLength")? { if max_send_header_block_length.is_number() { this_ref @@ -7767,7 +7758,7 @@ impl H2FrameParser { } } if let Some(strict_single_value) = - settings_js.get(global_object, "strictSingleValueFields")? + session_options.get(global_object, "strictSingleValueFields")? { if strict_single_value.is_boolean() { this_ref @@ -7775,7 +7766,9 @@ impl H2FrameParser { .set(strict_single_value.to_boolean()); } } - if let Some(padding_strategy) = settings_js.get(global_object, "paddingStrategy")? { + if let Some(padding_strategy) = + session_options.get(global_object, "paddingStrategy")? + { if padding_strategy.is_number() { this_ref .padding_strategy @@ -7786,12 +7779,30 @@ impl H2FrameParser { }); } } + if let Some(remote_custom) = + session_options.get(global_object, "remoteCustomSettings")? + { + if remote_custom.is_array() { + let mut filter: Vec = Vec::new(); + let mut value_iter = remote_custom.array_iterator(global_object)?; + while let Some(item) = value_iter.next()? { + if !item.is_number() { + continue; + } + let id = item.as_number(); + if !(0.0..=65535.0).contains(&id) { + continue; + } + let id = id as u16; + if !filter.contains(&id) && filter.len() < MAX_CUSTOM_SETTINGS { + filter.push(id); + } + } + this_ref.remote_custom_settings_filter.set(filter); + } + } } } - let mut is_server = false; - if let Some(type_js) = options.get(global_object, "type")? { - is_server = type_js.is_number() && type_js.to_u32() == 0; - } this_ref.is_server.set(is_server); JSH2FrameParser::Gc::context.set(this_value, global_object, context_obj); diff --git a/test/js/node/http2/node-http2-continuation.test.ts b/test/js/node/http2/node-http2-continuation.test.ts index 3c106f8410ca..b42f94f9de07 100644 --- a/test/js/node/http2/node-http2-continuation.test.ts +++ b/test/js/node/http2/node-http2-continuation.test.ts @@ -32,13 +32,10 @@ const TLS_OPTIONS = { ca: CA_CERT }; const H2_CLIENT_OPTIONS = { ...TLS_OPTIONS, rejectUnauthorized: false, - // Node.js uses top-level maxHeaderListPairs maxHeaderListPairs: 2000, settings: { // Allow receiving up to 256KB of header data maxHeaderListSize: 256 * 1024, - // Bun reads maxHeaderListPairs from settings - maxHeaderListPairs: 2000, }, }; diff --git a/test/js/node/http2/node-http2-session-options.test.ts b/test/js/node/http2/node-http2-session-options.test.ts new file mode 100644 index 000000000000..40f009b7584b --- /dev/null +++ b/test/js/node/http2/node-http2-session-options.test.ts @@ -0,0 +1,246 @@ +/** + * node:http2 session options and SETTINGS parameters. + * + * node reads the SETTINGS parameters from `options.settings`, and the session limits from the top + * level of the options. A key in the other place is ignored: it is not validated, and a SETTINGS + * key does not go on the wire. + * + * Works with both: + * - bun bd test test/js/node/http2/node-http2-session-options.test.ts + * - node --experimental-strip-types --test test/js/node/http2/node-http2-session-options.test.ts + */ +import assert from "node:assert/strict"; +import { once } from "node:events"; +import http2 from "node:http2"; +import net from "node:net"; +import { after, before, describe, test } from "node:test"; + +const isBun = typeof Bun !== "undefined"; + +const PREFACE = Buffer.from("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n", "latin1"); +const EMPTY_SETTINGS_FRAME = Buffer.from([0, 0, 0, 0x4, 0, 0, 0, 0, 0]); + +let server: http2.Http2Server; +let port: number; + +before(async () => { + server = http2.createServer(); + server.on("stream", stream => { + stream.respond({ ":status": 200 }); + stream.end("ok"); + }); + server.listen(0); + await once(server, "listening"); + port = (server.address() as net.AddressInfo).port; +}); + +after(() => { + server?.close(); +}); + +/** The id to value entries of the first frame a server created with `options` sends. */ +async function initialSettings(options: object): Promise> { + const server = http2.createServer(options); + server.listen(0); + await once(server, "listening"); + const socket = net.connect((server.address() as net.AddressInfo).port, "127.0.0.1"); + try { + await once(socket, "connect"); + socket.write(PREFACE); + socket.write(EMPTY_SETTINGS_FRAME); + let received = Buffer.alloc(0); + for await (const chunk of socket) { + received = Buffer.concat([received, chunk]); + if (received.length >= 9 && received.length >= 9 + received.readUIntBE(0, 3)) break; + } + const length = received.readUIntBE(0, 3); + assert.equal(received.readUInt8(3), 0x4, "the first frame is a SETTINGS frame"); + assert.equal(received.readUInt8(4) & 0x1, 0, "the first frame is not an ACK"); + const settings: Record = {}; + for (let i = 9; i < 9 + length; i += 6) { + settings[received.readUInt16BE(i)] = received.readUInt32BE(i + 2); + } + return settings; + } finally { + socket.destroy(); + server.close(); + } +} + +describe("session options and SETTINGS parameters", () => { + for (const [label, value] of [ + ["Infinity", Infinity], + ["-1", -1], + ["NaN", NaN], + ["1.5", 1.5], + ["2**53", 2 ** 53], + ['"10"', "10"], + ] as const) { + test(`a top-level maxHeaderListSize of ${label} is ignored by createServer() and connect()`, async () => { + const server = http2.createServer({ maxHeaderListSize: value } as any); + server.on("stream", stream => { + stream.respond({ ":status": 204 }); + stream.end(); + }); + server.listen(0); + await once(server, "listening"); + let client: http2.ClientHttp2Session | undefined; + try { + client = http2.connect(`http://127.0.0.1:${(server.address() as net.AddressInfo).port}`, { + maxHeaderListSize: value, + } as any); + const req = client.request({ ":path": "/" }); + req.end(); + const [headers] = await once(req, "response"); + assert.equal(headers[":status"], 204); + } finally { + client?.destroy(); + server.close(); + } + }); + } + + test("the initial SETTINGS frame carries only the keys under options.settings", async () => { + assert.deepEqual( + await initialSettings({ + headerTableSize: 100, + enablePush: true, + maxConcurrentStreams: 7, + initialWindowSize: 100000, + maxFrameSize: 20000, + maxHeaderListSize: 1000, + maxHeaderSize: 1000, + enableConnectProtocol: true, + customSettings: { 1000: 5 }, + }), + {}, + ); + assert.deepEqual( + await initialSettings({ maxHeaderListSize: -1, settings: { maxHeaderListSize: 1000, maxConcurrentStreams: 7 } }), + { 3: 7, 6: 1000 }, + ); + }); + + test("a session limit applies at the top level of the options, not under options.settings", async () => { + // 4 pseudo-headers and 10 regular headers: over a maxHeaderListPairs of 4. + const headers: Record = { ":path": "/" }; + for (let i = 0; i < 10; i++) headers[`x-header-${i}`] = "1"; + + async function request(options: object) { + const server = http2.createServer(options); + server.on("stream", stream => { + stream.respond({ ":status": 204 }); + stream.end(); + }); + server.listen(0); + await once(server, "listening"); + const client = http2.connect(`http://127.0.0.1:${(server.address() as net.AddressInfo).port}`); + client.on("error", () => {}); + try { + const req = client.request(headers); + req.end(); + const [response] = await once(req, "response"); + return response[":status"]; + } catch (err: any) { + return err.code; + } finally { + client.destroy(); + server.close(); + } + } + + assert.equal(await request({ settings: { maxHeaderListPairs: 4 } }), 204); + assert.equal(await request({ maxHeaderListPairs: 4 }), "ERR_HTTP2_STREAM_ERROR"); + }); + + for (const key of ["maxFrameSize", "initialWindowSize"]) { + test(`connect(url, { ${key}: NaN }) serves a request`, async () => { + const client = http2.connect(`http://127.0.0.1:${port}`, { [key]: NaN } as any); + try { + // The ACK comes first: with a frame size of 0 on the wire, request() never returns. + await once(client, "localSettings"); + const req = client.request({ ":path": "/" }); + req.end(); + const [headers] = await once(req, "response"); + assert.equal(headers[":status"], 200); + req.setEncoding("utf8"); + let body = ""; + for await (const chunk of req) body += chunk; + assert.equal(body, "ok"); + } finally { + client.destroy(); + } + }); + } + + // Each getter gives validateSettings() a valid number and the native read NaN. + // Bun-only: node v26.3.0 aborts on this input (an assertion in Http2Settings::Send()). + function settingNanAfterValidation(key: string) { + let armed = false; + return { + get [key]() { + return armed ? NaN : 65535; + }, + // validateSettings() reads customSettings after every other key. + get customSettings() { + armed = true; + return undefined; + }, + }; + } + function customSettingNanAfterValidation() { + let reads = 0; + return { + customSettings: { + get 1000() { + return reads++ === 0 ? 5 : NaN; + }, + }, + }; + } + for (const [label, nanAfterValidation] of [ + ["headerTableSize", () => settingNanAfterValidation("headerTableSize")], + ["initialWindowSize", () => settingNanAfterValidation("initialWindowSize")], + ["maxFrameSize", () => settingNanAfterValidation("maxFrameSize")], + ["maxConcurrentStreams", () => settingNanAfterValidation("maxConcurrentStreams")], + ["maxHeaderListSize", () => settingNanAfterValidation("maxHeaderListSize")], + ["maxHeaderSize", () => settingNanAfterValidation("maxHeaderSize")], + ["customSettings value", customSettingNanAfterValidation], + ] as const) { + test(`the native layer rejects a ${label} that becomes NaN after the JS validation`, { skip: !isBun }, async () => { + function thrownCode(fn: () => void) { + try { + fn(); + } catch (err: any) { + return err.code; + } + } + + const url = `http://127.0.0.1:${port}`; + const client = http2.connect(url); + client.on("error", () => {}); + // connect() throws from the session constructor, so the test owns the socket it would leave. + const socket = net.connect(port, "127.0.0.1"); + socket.on("error", () => {}); + let second: http2.ClientHttp2Session | undefined; + try { + await Promise.all([once(client, "connect"), once(socket, "connect")]); + assert.equal( + thrownCode(() => client.settings(nanAfterValidation())), + "ERR_HTTP2_INVALID_SETTING_VALUE", + ); + assert.equal( + thrownCode(() => { + second = http2.connect(url, { createConnection: () => socket, settings: nanAfterValidation() }); + second.on("error", () => {}); + }), + "ERR_HTTP2_INVALID_SETTING_VALUE", + ); + } finally { + second?.destroy(); + socket.destroy(); + client.destroy(); + } + }); + } +});