diff --git a/src/js/node/http2.ts b/src/js/node/http2.ts index 1ec56d7470bc..5696cde1c54c 100644 --- a/src/js/node/http2.ts +++ b/src/js/node/http2.ts @@ -112,9 +112,14 @@ const kDefaultSettings = { // the SETTINGS frame buffer, so only customSettings is visible pre-ACK. function initialLocalSettings(submitted: any) { const settings: any = { ...kDefaultSettings }; - const custom = submitted?.customSettings; - if (custom != null && typeof custom === "object") { - settings.customSettings = { ...custom }; + // `submitted` is built with toNativeSettings(), so customSettings holds [id, value] pairs. + const pairs = submitted?.customSettings; + if ($isArray(pairs) && pairs.length > 0) { + const byId: Record = {}; + for (let i = 0; i < pairs.length; i += 2) { + byId[pairs[i]] = pairs[i + 1]; + } + settings.customSettings = byId; } return settings; } @@ -198,23 +203,64 @@ function validateSettings(settings: any) { if (typeof cs !== "object" || cs === null) { throwSettingRangeError("customSettings", cs); } + // node's first pass over customSettings: a range check on Number() of each own key and + // value. NaN passes this check. customSettingsPairs() is the second pass. + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/core.js#L1012-L1021 const keys = ObjectKeys(cs); if (keys.length > MAX_ADDITIONAL_SETTINGS) { - const err = new Error("Number of custom settings exceeds MAX_ADDITIONAL_SETTINGS"); - (err as any).code = "ERR_HTTP2_TOO_MANY_CUSTOM_SETTINGS"; - throw err; + throw $ERR_HTTP2_TOO_MANY_CUSTOM_SETTINGS(); } for (const key of keys) { const id = Number(key); - if (!Number.isInteger(id) || id < 0 || id > 0xffff) { - throwSettingRangeError(key, cs[key]); + if (id < 0 || id > 0xffff) { + throwSettingRangeError("customSettings:id", id); } - const val = cs[key]; - if (typeof val !== "number" || val < 0 || val > kMaxInt || !Number.isFinite(val)) { - throwSettingRangeError(key, val); + const value = Number(cs[key]); + if (value < 0 || value > kMaxInt) { + throwSettingRangeError("customSettings:value", value); + } + } + } +} + +// node's second pass over customSettings. It reads each enumerable key that has a number value, +// and the setting id is Number(key). This is the only place that turns a key into an id. +// It returns the [id, value] pairs for the wire, as one flat array. +// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/util.js#L402-L478 +function customSettingsPairs(customSettings: any): number[] | undefined { + if (typeof customSettings !== "object" || customSettings === null) return undefined; + const pairs: number[] = []; + for (const key in customSettings) { + const value = customSettings[key]; + if (typeof value !== "number") continue; + const id = Number(key); + // node also rejects id 0 and value 0 here. Bun accepts both. + if (!(id >= 0 && id <= 0xffff)) { + throwSettingRangeError("Range Error", id); + } + if (!(value >= 0 && value <= kMaxInt)) { + throwSettingRangeError("Range Error", value); + } + // node stores each pair in a Uint32Array, which truncates it. node looks for an earlier + // entry with the id before truncation, so "10" and "10.5" stay two entries. + let i = 0; + while (i < pairs.length && pairs[i] !== id) i += 2; + if (i < pairs.length) { + pairs[i + 1] = value >>> 0; + } else { + if (pairs.length === MAX_ADDITIONAL_SETTINGS * 2) { + throw $ERR_HTTP2_TOO_MANY_CUSTOM_SETTINGS(); } + pairs.push(id >>> 0, value >>> 0); } } + return pairs; +} + +// The object that H2FrameParser reads: the caller's settings, with customSettings replaced by +// the pairs from customSettingsPairs(). The native side never reads the caller's object. +function toNativeSettings(settings: any) { + return { ...settings, customSettings: customSettingsPairs(settings?.customSettings) }; } function assertSettings(settings: any) { @@ -250,13 +296,11 @@ function getPackedSettings(settings?: any): Buffer { if (settings.enableConnectProtocol !== undefined) { entries.push([0x8, settings.enableConnectProtocol ? 1 : 0]); } - if (settings.customSettings) { - const cs = settings.customSettings; - const keys = ObjectKeys(cs); - // Sort custom settings by ID for consistent output - keys.sort((a, b) => Number(a) - Number(b)); - for (const key of keys) { - entries.push([Number(key), cs[key]]); + // The same pairs, in the same order, that a session sends. + const pairs = customSettingsPairs(settings.customSettings); + if (pairs !== undefined) { + for (let i = 0; i < pairs.length; i += 2) { + entries.push([pairs[i], pairs[i + 1]]); } } @@ -1963,8 +2007,9 @@ function createPendingStreamCancelError(cause?: any) { // 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. +// customSettings comes from options.settings only, as in node. function serverNativeSettings(options) { - const merged = { ...options, ...options?.settings }; + const merged = { ...options, ...toNativeSettings(options?.settings) }; if (merged.enablePush !== undefined) merged.enablePush = false; return merged; } @@ -4646,7 +4691,7 @@ class ServerHttp2Session extends Http2Session { // frame stays compliant (the initial SETTINGS frame already clamps this // in ServerHttp2Session's constructor). Clients still accept `enablePush` // via their own `settings()` method. - settings = { ...settings, enablePush: false }; + settings = { ...toNativeSettings(settings), enablePush: false }; if (typeof settings.maxConcurrentStreams === "number") { this.#advertisedMaxConcurrentStreams = settings.maxConcurrentStreams; } @@ -5571,6 +5616,7 @@ class ClientHttp2Session extends Http2Session { // node treats an omitted/undefined settings object as an empty update. if (settings === undefined) settings = {} as Settings; validateSettings(settings); + const nativeSettings = toNativeSettings(settings); // 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++; @@ -5579,7 +5625,7 @@ class ClientHttp2Session extends Http2Session { return; } this.#pendingSettingsAck = true; - this.#parser?.settings(settings); + this.#parser?.settings(nativeSettings); // The frame is queued on the native session; flush it now (as close() does for its // GOAWAY) instead of waiting for the next unrelated write. Node schedules a session // write for every settings() call, so its SETTINGS goes out with the current batch - @@ -5710,7 +5756,7 @@ class ClientHttp2Session extends Http2Session { if (options?.settings !== undefined) { validateSettings(options.settings); } - const nativeSettings = { ...options, ...options?.settings }; + const nativeSettings = { ...options, ...toNativeSettings(options?.settings) }; this.#localSettings = initialLocalSettings(nativeSettings); this.#parser = new H2FrameParser({ native: nativeSocket, diff --git a/src/runtime/api/bun/h2_frame_parser.rs b/src/runtime/api/bun/h2_frame_parser.rs index 06c2b66811a3..69c9820839d8 100644 --- a/src/runtime/api/bun/h2_frame_parser.rs +++ b/src/runtime/api/bun/h2_frame_parser.rs @@ -4373,75 +4373,43 @@ impl H2FrameParser { // Stage customSettings before committing anything — a later validation throw must not // leave partial state installed for the next submission. let mut staged_custom: Vec<(u16, u32)> = Vec::new(); - // Validate customSettings and remember them so they go on the wire with our SETTINGS. + // customSettings is the flat [id, value, ...] array from customSettingsPairs() in + // src/js/node/http2.ts. That function reads the user's object and turns each key into an + // id, so this side reads numbers only. if let Some(custom_settings) = options.get(global_object, "customSettings")? { if !custom_settings.is_undefined() { - let Some(custom_settings_obj) = custom_settings.get_object() else { + if !custom_settings.is_array() { return global_object - .err_http2_invalid_setting_value("Expected customSettings to be an object") + .err_http2_invalid_setting_value("Expected customSettings to be an array") .throw(); - }; - - let mut count: usize = 0; - let iter = bun_jsc::JSPropertyIterator::init( - global_object, - custom_settings_obj, - bun_jsc::JSPropertyIteratorOptions { - skip_empty_name: false, - include_value: true, - ..Default::default() - }, - )?; - - while let Some((prop_name, setting_value)) = iter.next()? { - count += 1; - if count > MAX_CUSTOM_SETTINGS { - return global_object - .err_http2_too_many_custom_settings( - "Number of custom settings exceeds MAX_ADDITIONAL_SETTINGS", - ) - .throw(); - } - - // Validate setting ID (key) is in range [0, 0xFFFF] - let setting_id_str = prop_name.to_utf8(); - // Parse bytes directly (ASCII decimal); do not insert - // UTF-8 validation on external data. - let Some(setting_id) = - bun_core::parse_int::(setting_id_str.slice(), 10).ok() - else { - return global_object - .err_http2_invalid_setting_value_range_error( - "Invalid custom setting identifier", - ) - .throw(); - }; - if setting_id > 0xFFFF { + } + let mut pairs = custom_settings.array_iterator(global_object)?; + if pairs.len as usize > MAX_CUSTOM_SETTINGS * 2 { + return global_object + .err_http2_too_many_custom_settings( + "Number of custom settings exceeds MAX_ADDITIONAL_SETTINGS", + ) + .throw(); + } + while let Some(id) = pairs.next()? { + let value = pairs.next()?.unwrap_or(JSValue::UNDEFINED); + if !id.is_number() || !(0.0..=65535.0).contains(&id.as_number()) { return global_object .err_http2_invalid_setting_value_range_error( "Invalid custom setting identifier", ) .throw(); } - - // 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 { - return global_object - .err_http2_invalid_setting_value_range_error( - "Invalid custom setting value", - ) - .throw(); - } - staged_custom.push((setting_id as u16, value as u32)); - } else { + if !value.is_number() + || !(0.0..=MAX_HEADER_TABLE_SIZE_F64).contains(&value.as_number()) + { return global_object .err_http2_invalid_setting_value_range_error( - "Expected custom setting value to be a number", + "Invalid custom setting value", ) .throw(); } + staged_custom.push((id.as_number() as u16, value.as_number() as u32)); } } } diff --git a/test/js/node/http2/h2-conformance.test.ts b/test/js/node/http2/h2-conformance.test.ts index 414bf6c3b68b..43c1ab85ced4 100644 --- a/test/js/node/http2/h2-conformance.test.ts +++ b/test/js/node/http2/h2-conformance.test.ts @@ -929,6 +929,153 @@ describe("SETTINGS ack ordering (RFC 9113 §6.5.3)", () => { }); }); +describe("customSettings ids", () => { + const isSettings = (f: Frame) => f.type === FrameType.SETTINGS && (f.flags & 0x1) === 0; + function settingsEntries(payload: Buffer): number[][] { + const entries: number[][] = []; + for (let i = 0; i < payload.length; i += 6) { + entries.push([payload.readUInt16BE(i), payload.readUInt32BE(i + 2)]); + } + return entries; + } + + /** A server with these options, and a raw client that has sent its preface and SETTINGS. */ + async function connectRaw(options?: http2.ServerOptions) { + const server = http2.createServer(options); + const session = Promise.withResolvers(); + server.on("session", session.resolve); + server.listen(0); + await once(server, "listening"); + const c = await RawH2.connect((server.address() as net.AddressInfo).port); + c.sendPreface(); + c.sendEmptySettings(); + return { server, c, session: session.promise }; + } + + // Recorded from node v26.3.0. node reads a key as Number(key), and only when its value is a number: + // https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/util.js#L402-L478 + // A row has the customSettings, the entries that getPackedSettings() returns and that the first + // SETTINGS frame carries, and localSettings.customSettings. + const rows: [string, any, number[][], Record][] = [ + ["a decimal key", { "16": 5 }, [[16, 5]], { "16": 5 }], + ["a hex key", { "0x10": 5 }, [[16, 5]], { "16": 5 }], + [ + "an exponent and whitespace", + { "1e3": 6, " 12": 7, "\n13\t": 8 }, + [ + [1000, 6], + [12, 7], + [13, 8], + ], + { "12": 7, "13": 8, "1000": 6 }, + ], + ["a fractional id", { "10.5": 8 }, [[10, 8]], { "10": 8 }], + ["two keys for one id", { "16": 7, "0x10": 5 }, [[16, 5]], { "16": 5 }], + [ + "an id and its fraction", + { "10": 1, "10.5": 2 }, + [ + [10, 1], + [10, 2], + ], + { "10": 2 }, + ], + ["values that are not numbers", { "16": "5", "17": undefined, abc: null, "18": 9 }, [[18, 9]], { "18": 9 }], + ["a symbol key", { [Symbol("x")]: 5, "20": 9 }, [[20, 9]], { "20": 9 }], + ["the top of the range", { "65535": 1, "0xffff": 2 }, [[65535, 2]], { "65535": 2 }], + ["a fractional value", { "16": 1.5 }, [[16, 1]], { "16": 1 }], + ]; + + test.each(rows)("%s: getPackedSettings() and a server agree with node", async (_, customSettings, entries, byId) => { + expect(settingsEntries(http2.getPackedSettings({ customSettings }))).toEqual(entries); + + const { server, c, session } = await connectRaw({ settings: { customSettings } }); + try { + const settings = await c.waitFor(isSettings); + expect(settingsEntries(settings.payload)).toEqual(entries); + + const s = await session; + expect(s.localSettings.customSettings).toEqual(byId); + const acked = once(s, "localSettings"); + c.sendSettingsAck(); + const [local] = await acked; + expect(local.customSettings).toEqual(byId); + } finally { + c.destroy(); + server.close(); + } + }); + + // "0x11" and "1.7e1" both name id 17. node sends one entry, with the last value. + const update = { "0x11": 9, "1.7e1": 10, "10.5": 8 }; + const updateEntries = [ + [17, 10], + [10, 8], + ]; + + test("a server sends the ids from settings()", async () => { + const { server, c, session } = await connectRaw(); + try { + const first = await c.waitFor(isSettings); + const s = await session; + s.settings({ customSettings: update }); + const second = await c.waitFor(f => isSettings(f) && f !== first); + // A server's settings() also sends ENABLE_PUSH (id 2) with the value 0. + expect(settingsEntries(second.payload).filter(([id]) => id !== 2)).toEqual(updateEntries); + } finally { + c.destroy(); + server.close(); + } + }); + + test("a client sends the ids from connect() and settings()", async () => { + const raw = await RawH2Server.listen(); + const customSettings = { "0x10": 5, "1e3": 6 }; + const client = http2.connect(`http://127.0.0.1:${raw.port}`, { settings: { customSettings } }); + client.on("error", () => {}); + const connected = once(client, "connect"); + try { + const first = await raw.waitFor(isSettings); + expect(settingsEntries(first.payload)).toEqual([ + [16, 5], + [1000, 6], + ]); + await connected; + expect(client.localSettings.customSettings).toEqual({ "16": 5, "1000": 6 }); + + client.settings({ customSettings: update }); + const second = await raw.waitFor(f => isSettings(f) && f !== first); + expect(settingsEntries(second.payload)).toEqual(updateEntries); + } finally { + client.destroy(); + raw.close(); + } + }); + + // Recorded from node v26.3.0. The "customSettings:id" and "customSettings:value" errors come from + // node's first pass. The "Range Error" errors come from its second pass. + test.each([ + ["an id that is not a number", { abc: 5 }, 'Invalid value for setting "Range Error": NaN'], + ["an id above 0xffff", { "65536": 5 }, 'Invalid value for setting "customSettings:id": 65536'], + ["a negative id", { "-1": 5 }, 'Invalid value for setting "customSettings:id": -1'], + ["a negative value", { "16": -1 }, 'Invalid value for setting "customSettings:value": -1'], + ["a string value below 0", { "16": "-1" }, 'Invalid value for setting "customSettings:value": -1'], + ["a NaN value", { "16": NaN }, 'Invalid value for setting "Range Error": NaN'], + ])("getPackedSettings() throws the error node throws for %s", (_, customSettings: any, message) => { + let error: any; + try { + http2.getPackedSettings({ customSettings }); + } catch (e) { + error = e; + } + expect({ name: error?.name, code: error?.code, message: error?.message }).toEqual({ + name: "RangeError", + code: "ERR_HTTP2_INVALID_SETTING_VALUE", + message, + }); + }); +}); + function requestHeaderBlock(method: "GET" | "POST", extra: Buffer = Buffer.alloc(0)): Buffer { return Buffer.concat([ Buffer.from([method === "POST" ? 0x83 : 0x82, 0x86, 0x84, 0x01]),