Skip to content

node:http2: send array values in raw-form header lists as one field per element #37796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 13 commits into
base: robobun/12c95c7e/http2-raw-list-kept-array
Choose a base branch
from
Open

node:http2: send array values in raw-form header lists as one field per element #37796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
22b97b8
node:http2: send array values in raw-form header lists as one field p…
robobun Aug 12, 2026
d9d5f35
test: state the element rule the rejection cases exercise
robobun Aug 12, 2026
a6f9214
Trim the comments on the raw header walk and rawHeadersToObject
robobun Aug 12, 2026
917c345
test: run the raw-headers array tests sequentially
robobun Aug 12, 2026
6826664
node:http2: judge the single-value rule on the raw header list slot b…
robobun Aug 12, 2026
fee4e36
Shorten the assertSingleValueHeaders comment
robobun Aug 12, 2026
46fdf86
node:http2: copy the raw header name before coercing its values
robobun Aug 12, 2026
cab32ee
Shorten the owned-name comment
robobun Aug 12, 2026
07414d2
node:http2: apply the raw walk's single-value check once per slot
robobun Aug 12, 2026
73fb09c
node:http2: let request() take an array in the :method slot
robobun Aug 12, 2026
7d565f8
ci: retrigger
robobun Aug 12, 2026
acd595c
Merge branch 'robobun/12c95c7e/http2-raw-list-kept-array' into farm/7…
robobun Oct 8, 2026
4ff37ff
node:http2: send each element of an array in a raw header list as one…
robobun Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 93 additions & 25 deletions src/js/node/http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3564,6 +3564,7 @@ class ServerHttp2Stream extends Http2Stream {
// date header appended when missing. The derived object form (original-case
// keys, array values for duplicates) backs sentHeaders.
let rawHeadersList: any[] | null = null;
let rawHeadersExpanded = false;
let statusCode;
if (headers == undefined) {
headers = {};
Expand Down Expand Up @@ -3595,19 +3596,11 @@ class ServerHttp2Stream extends Http2Stream {
if (!isDateSet && (sendDateOption == null || sendDateOption)) {
headers.push(HTTP2_HEADER_DATE, utcDate());
}
rawHeadersList = headers as any[];
const headersObject = { __proto__: null };
for (let i = 0; i < rawHeadersList.length; i += 2) {
const key = rawHeadersList[i];
let value = rawHeadersList[i + 1];
if (typeof value === "object" && $isArray(value)) value = copyHeaderValueArray(value);
const existing = headersObject[key];
if (existing === undefined) headersObject[key] = value;
else if ($isArray(existing)) existing.push(value);
else headersObject[key] = [existing, value];
}
if (rawHeadersList[sensitiveHeaders] !== undefined) {
headersObject[sensitiveHeaders] = rawHeadersList[sensitiveHeaders];
rawHeadersList = foldRawHeaders(headers as any[], headersObject, session[kStrictSingleValueFields] !== false);
rawHeadersExpanded = rawHeadersList !== headers;
if (sensitiveNamesForCopy !== undefined) {
headersObject[sensitiveHeaders] = sensitiveNamesForCopy;
}
headers = headersObject;
} else if (!$isObject(headers)) {
Expand All @@ -3626,7 +3619,9 @@ class ServerHttp2Stream extends Http2Stream {
const sensitiveNames = buildSensitiveNames(headers, sensitives);
// Pre-validate single-value headers in JS so a throwing respond() leaves no partial state in
// the shared HPACK table (same rule request() applies).
if (session[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers);
if (rawHeadersExpanded) {
assertExpandedRawHeaders(rawHeadersList!, session[kStrictSingleValueFields] !== false);
} else if (session[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers);
Comment on lines +3622 to +3624

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: callers who repeat a single-value header with an undefined value slot after the real value still get ERR_HTTP2_HEADER_SINGLE_VALUE, which node accepts, unless some unrelated slot holds an array. At http2.ts:3624 and :6188 a list with no array value is still judged by assertSingleValueHeaders on the folded object, where ["content-type", "text/html", "content-type", undefined] folds to a length-2 array; the same list with "x-a", ["a"] added passes via assertExpandedRawHeaders. Fix: judge every raw list slot by slot (skip undefined and empty-array slots) regardless of whether foldRawHeaders expanded it, so the outcome does not depend on an unrelated slot. [also at: src/js/node/http2.ts:6188 - pre-existing: callers of request()/respond() still get ERR_HTTP2_HEADER_SINGLE_VALUE for a raw list that repeats a single-value name with an undefined second slot, which node accepts, unless some unrelated slot happens to hold an array. src/js/node/http2.ts:6188 and :3624 keep judging a…]
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

A caller passes request([":path", "/", "content-type", "text/html", "content-type", undefined]) or the same list to respond(). foldRawHeaders (http2.ts:3923) sees no array, so wire === list and rawHeadersExpanded is false; the folded object has content-type = ["text/html", undefined] (http2.ts:3957). http2.ts:3624 / :6188 then call assertSingleValueHeaders(headers), whose check at http2.ts:3871 sees value.length > 1 and throws ERR_HTTP2_HEADER_SINGLE_VALUE. Node's buildNgHeaderString skips an undefined value slot in the array form and sends one content-type field. If the caller adds any array value elsewhere in the list (test row at node-http2-header-list.test.ts:518), the list is expanded and assertExpandedRawHeaders (http2.ts:3967) skips the undefined slot, so the same two content-type slots are accepted. The base branch throws for this input too (pre-existing), but the test comment at :499-501 claims an undefined slot is 'none, in every slot of the list', which the non-expanded path does not honor.

Verification: Pre-existing. A raw list that repeats a single-value name with an undefined value after the real value and has no array-valued slot folds to ["text/html", undefined] at src/js/node/http2.ts:3957; lines 3624 / 6188 hand it to assertSingleValueHeaders, which throws ERR_HTTP2_HEADER_SINGLE_VALUE at 3871. assertExpandedRawHeaders (3966) skips the slot, and the base revision throws by the same route.

// node keeps the never-index list visible on sentHeaders (symbol keys are not iterated by the
// wire-encoding path, so re-attaching is safe).
if (sensitives !== undefined) headers[sensitiveHeaders] = sensitives;
Expand Down Expand Up @@ -3924,6 +3919,79 @@ function copyHeaderValueArray(values: any[]): any[] {
return copy;
}

// Folds a raw [name, value, ...] list into `object`, which backs sentHeaders, and returns the list to encode.
function foldRawHeaders(list: any[], object: Record<string, any>, strictSingleValue: boolean): any[] {
let wire = list;
for (let i = 0; i < list.length; i += 2) {
const key = list[i];
let value = list[i + 1];
if (typeof value === "object" && $isArray(value)) {
if (wire === list) {
wire = [];
for (let j = 0; j < i; j++) $arrayPush(wire, list[j]);
}
value = copyHeaderValueArray(value);
const length = value.length;
if (
!strictSingleValue &&
length > 1 &&
(typeof key === "string" ? key : String(key)).charCodeAt(0) === 0x3a /* ':' */
) {
// node joins the elements of a pseudo-header: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/util.js#L809
$arrayPush(wire, key);
$arrayPush(wire, String(value));
} else {
// One field per element: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/http2/util.js#L819-L826
for (let j = 0; j < length; j++) {
$arrayPush(wire, key);
$arrayPush(wire, String(value[j]));
}
}
} else if (wire !== list) {
$arrayPush(wire, key);
$arrayPush(wire, value);
}
const existing = object[key];
if (existing === undefined) object[key] = value;
else if ($isArray(existing)) existing.push(value);

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Every raw-list respond() or request() that repeats a header name (the set-cookie case this PR targets) routes its sentHeaders fold through user-overridable Array.prototype.push at src/js/node/http2.ts:3956, while the same new helper uses $arrayPush for the wire list four lines above. Page code that patches Array.prototype.push corrupts or throws from sentHeaders for a duplicate name. Fix: use $arrayPush(existing, value) here, and audit the sibling fold sites in this file for the same call so the builtin stays tamper-proof as src/js/CLAUDE.md requires.

Why this was flagged

Trigger: any raw list whose name appears three or more times, e.g. respond([":status", 200, "set-cookie", "a", "set-cookie", "b", "set-cookie", "c"]), or twice when the first slot is an array. foldRawHeaders at src/js/node/http2.ts:3954-3957 does existing.push(value) on the internal array it created, dispatching through Array.prototype.push, which user or polyfill code can replace; a replaced push that throws or drops arguments makes respond() throw before the HEADERS frame is sent or leaves sentHeaders missing values. The wire list in the same function is built with $arrayPush (:3931, :3941, :3947, :3951), so the diff itself shows the correct primitive. The base inline loop had the same call, but this PR rewrote that loop into a new helper and chose the intrinsic for every other push in it, so the remaining .push is the one tamper-reachable call on a path that runs once per duplicate header on every raw-list response. Remedy: $arrayPush at :3956.

Verification: Pre-existing. Triggering condition: user code has replaced Array.prototype.push and a raw list repeats a name. src/js/node/http2.ts:3954-3957 does existing.push(value) on an internal plain array, so the call runs whatever the user installed. The base branch contains the identical call in both inline fold loops, so merging makes nothing worse than the base.

else object[key] = [existing, value];
}
return wire;
}

// For a list that foldRawHeaders() expanded: the single-value rule per pair, then the value bytes the encoder refuses.
function assertExpandedRawHeaders(list: any[], strictSingleValue: boolean) {
if (strictSingleValue) {
let seen: Set<string> | null = null;
for (let i = 0; i < list.length; i += 2) {
if (list[i + 1] === undefined) continue;
const name = list[i];
const lower = StringPrototypeToLowerCase.$call(typeof name === "string" ? name : String(name));
if (!kSingleValueHeaders.has(lower)) continue;
if (seen !== null && seen.has(lower)) {
throw $ERR_HTTP2_HEADER_SINGLE_VALUE(`Header field "${lower}" must only have a single value`);
}
if (seen === null) seen = new SafeSet();
seen.add(lower);
}
}
for (let i = 0; i < list.length; i += 2) {
const value = list[i + 1];
if (typeof value !== "string") continue;
for (let j = 0; j < value.length; j++) {
const c = value.charCodeAt(j);
if (c === 0x00 || c === 0x0a || c === 0x0d) {
const name = list[i];
const lower = StringPrototypeToLowerCase.$call(typeof name === "string" ? name : String(name));
// The encoder's message for this value.
const error = new TypeError(`Invalid value for header "${lower}"`);
error.code = "ERR_HTTP2_INVALID_HEADER_VALUE";
Comment on lines +3987 to +3988

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): Callers of request()/respond() with an expanded raw list get a hand-built TypeError with a hand-assigned .code, bypassing the shared error machinery the rest of this file uses. src/js/node/http2.ts:3987-3988 constructs new TypeError(...) and sets error.code = "ERR_HTTP2_INVALID_HEADER_VALUE" by hand, while the same file throws $ERR_HTTP2_INVALID_HEADER_VALUE(value, name) at http2.ts:618 for the same code. Fix: throw through the $ERR_HTTP2_INVALID_HEADER_VALUE helper (or, if the encoder's shorter message must be preserved, add that variant to ErrorCode.ts) so every ERR_HTTP2_INVALID_HEADER_VALUE thrown from JS carries the same class, code and message shape. [also at: src/js/node/http2.ts:3989 - nit: this hand-builds a new TypeError and assigns error.code instead of going through the $ERR_* ErrorCode machinery that every other throw in this file uses.]

Why this was flagged

A raw header list containing an array value plus a string element with CR, LF or NUL reaches assertExpandedRawHeaders via ClientHttp2Session.request() at src/js/node/http2.ts:6187 or ServerHttp2Stream.respond() at src/js/node/http2.ts:3623. At src/js/node/http2.ts:3987-3989 the code builds a plain TypeError and assigns .code manually instead of using the $ERR_HTTP2_INVALID_HEADER_VALUE helper declared in src/js/builtins.d.ts:461 and already used at src/js/node/http2.ts:618. The helper's message template in src/jsc/bindings/ErrorCode.cpp:1906 is Invalid value "<value>" for header "<name>", so the same error code now has two different message shapes, and the hand-rolled error does not go through the centralized ErrorCode construction. On the base branch this input never threw from JS at all (the native encoder threw mid-encode); the behaviour change itself is intended, only the construction of the error diverges from the file's convention. No safeguard applies; this is a convention/consistency nit, not a runtime failure.

Verification: Any expanded raw header list whose string element contains NUL/CR/LF reaches assertExpandedRawHeaders via respond() (src/js/node/http2.ts:3622-3624) or request() (:6186-6188). src/js/node/http2.ts:3987-3989 throws a plain TypeError with a hand-assigned own .code, while the same file throws $ERR_HTTP2_INVALID_HEADER_VALUE(value, name) at :618. REVIEW.md states: never inline new Error with a hand-assigned .code.

throw error;
}
}
}
}

function toHeaderObject(headers, sensitiveHeadersValue) {
const obj = { __proto__: null, [sensitiveHeaders]: sensitiveHeadersValue };
for (let n = 0; n < headers.length; n += 2) {
Expand Down Expand Up @@ -5997,6 +6065,7 @@ class ClientHttp2Session extends Http2Session {
// given order. The derived object form (original-case keys, array values
// for duplicates) backs sentHeaders.
let rawHeadersList: any[] | null = null;
let rawHeadersExpanded = false;
if (headers == undefined) {
headers = {};
} else if ($isArray(headers)) {
Expand Down Expand Up @@ -6045,17 +6114,14 @@ class ClientHttp2Session extends Http2Session {
if (scheme !== undefined) throw $ERR_HTTP2_CONNECT_SCHEME();
if (path !== undefined) throw $ERR_HTTP2_CONNECT_PATH();
}
rawHeadersList = additionalPseudoHeaders.length ? additionalPseudoHeaders.concat(raw) : raw;
const headersObject = { __proto__: null };
for (let i = 0; i < rawHeadersList.length; i += 2) {
const key = rawHeadersList[i];
let value = rawHeadersList[i + 1];
if (typeof value === "object" && $isArray(value)) value = copyHeaderValueArray(value);
const existing = headersObject[key];
if (existing === undefined) headersObject[key] = value;
else if ($isArray(existing)) existing.push(value);
else headersObject[key] = [existing, value];
let list = raw;
if (additionalPseudoHeaders.length) {
list = additionalPseudoHeaders;
for (let i = 0; i < raw.length; i++) $arrayPush(list, raw[i]);
}
const headersObject = { __proto__: null };
rawHeadersList = foldRawHeaders(list, headersObject, this[kStrictSingleValueFields] !== false);
rawHeadersExpanded = rawHeadersList !== list;
Comment on lines +6117 to +6124

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: A caller that queues request() on a still-connecting session and then mutates its raw header array sees the mutation sent on the wire, unlike node. When the list carries every pseudo-header and no array value, http2.ts:6117 keeps list = raw and foldRawHeaders returns that same caller array, which is stored as wireHeaders at http2.ts:6351 and encoded later at http2.ts:6445. Fix: encode a copy of the caller's list on every request() path (e.g. slice raw as respond() does at http2.ts:3578, or always build a fresh list), while keeping sentHeaders derived from the same copy. [also at: src/js/node/http2.ts:6120 - pre-existing: a request queued on a still-connecting (or stream-limited) session encodes the caller's own list later, so edits the caller makes after request() returns reach the wire; node encodes at the call.; src/js/node/http2.ts:6121 - pre-existing: a caller who passes every pseudo-header in a raw list and mutates that array after request() returns gets the mutated list on the wire when the request was queued.]
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

On a session that has not emitted 'connect', client.request([":method","GET",":scheme","http",":authority",host,":path","/","x-tag","a"]) followed by list.push("x-late","v"). additionalPseudoHeaders is empty at http2.ts:6118, so list is the caller's own array; foldRawHeaders at http2.ts:3922 sees no array value and returns wire === list (http2.ts:3923, 3958). rawHeadersList is the caller's array, pushed into #pendingRequests as wireHeaders at http2.ts:6351 and only handed to parser.request at http2.ts:6445 once the session connects, so the late "x-late" pair goes on the wire while req.sentHeaders (built eagerly at http2.ts:6123) does not show it. Node encodes the block synchronously inside request(), so the late pair is never sent. The base branch has the same : raw branch, so this is pre-existing, but the new test at test/js/node/http2/node-http2-header-list.test.ts:373-386 claims the list is read at the call and only passes because its list contains an array value, which forces the copy. respond() avoids this by headers.slice() at http2.ts:3578.

Verification: pre-existing. request() is queued with a raw list that supplies every pseudo-header and contains no array value, and the caller then mutates that array before the queue flushes. With no array values rawHeadersList === raw, stored as wireHeaders at 6351 and encoded only at 6445/6447. Base line 6038 aliases the caller's array identically, so merging makes nothing worse.

if (raw[sensitiveHeaders] !== undefined) {
headersObject[sensitiveHeaders] = raw[sensitiveHeaders];
}
Expand Down Expand Up @@ -6117,7 +6183,9 @@ class ClientHttp2Session extends Http2Session {
}
// Validate single-value constraints before anything is encoded (a mid-encode throw would
// desync the shared HPACK table from the peer).
if (this[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers);
if (rawHeadersExpanded) {
assertExpandedRawHeaders(rawHeadersList!, this[kStrictSingleValueFields] !== false);

@claude claude Bot Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Callers whose raw list holds an array value now get a synchronous TypeError for a :path containing CR, LF or NUL, where the base (and node) deliver an async stream error. assertExpandedRawHeaders at http2.ts:3978-3991 scans every string value, pseudo-headers included, and runs at http2.ts:6187 before the :path control-character check at http2.ts:6274-6288 that turns such a path into a nextTick NGHTTP2_PROTOCOL_ERROR on the returned stream. Fix: keep one contract for :path regardless of list form, e.g. skip :path in the expanded pre-check (or run the 6274 check first) so request([":path", "/a\n", "x", ["b"]]) still returns a stream that errors asynchronously.

Why this was flagged

A caller passes client.request([":path", "/a\n", "x-multi", ["b", "c"]]). foldRawHeaders at http2.ts:3923 expands the list, so rawHeadersExpanded is true and http2.ts:6187 calls assertExpandedRawHeaders. Its second loop at http2.ts:3978-3991 does not exclude pseudo-headers; it finds 0x0a in the ":path" value and throws TypeError with code ERR_HTTP2_INVALID_HEADER_VALUE and message 'Invalid value for header ":path"' synchronously. On the base branch the list reached the check at http2.ts:6274-6288, which for c <= 0x20 creates the ClientHttp2Stream, sets rstCode = NGHTTP2_PROTOCOL_ERROR, schedules emitStreamErrorNT and returns the stream. After the change the same path value is a sync throw only when another slot is an array, and stays async otherwise, so the error contract now depends on an unrelated slot. The single-value guard does not apply; nothing else intercepts the pre-check before it reaches pseudo-header values.

Verification: foldRawHeaders (src/js/node/http2.ts:3923-3960) returns a new wire list whenever a value slot is an array, so rawHeadersExpanded = rawHeadersList !== list (6124) is true and 6186-6187 calls assertExpandedRawHeaders. Its value loop (3978-3991) has no pseudo-header exclusion, so :path = "/a\n" throws synchronously. After the PR the same call throws before 6274 is reached.

} else if (this[kStrictSingleValueFields] !== false) assertSingleValueHeaders(headers);
// node keeps the never-index list visible on the request's sentHeaders (symbol keys are
// not iterated by the wire-encoding path, so re-attaching is safe).
if (sensitives !== undefined) headers[sensitiveHeaders] = sensitives;
Expand Down Expand Up @@ -6176,7 +6244,7 @@ class ClientHttp2Session extends Http2Session {
}

let rejectContentLengthOnNoPayload = false;
if (NoPayloadMethods.has(method.toUpperCase())) {
if (typeof method === "string" && NoPayloadMethods.has(method.toUpperCase())) {
Comment thread
robobun marked this conversation as resolved.
// Like Node, a payload-meaningless method only defaults endStream to
// true when the caller expressed no preference; an explicit endStream
// (validated above) is honored, so { endStream: false } stays open.
Expand Down
Loading
Loading