node:http: apply joinDuplicateHeaders on HTTP/1 fallback connections

[robobun]

Problem

• A request served by the JS HTTP/1 path in src/js/internal/http1_server_fallback.ts ignores the server's joinDuplicateHeaders option: with http.createServer({ joinDuplicateHeaders: true }) and a request carrying Authorization: one and Authorization: two, a handler reached through server.emit("connection", socket) sees req.headers.authorization === "one". The same server's listening (native) path, and node for both paths, give "one, two".
• http2.createSecureServer({ allowHTTP1: true, joinDuplicateHeaders: true }) (or http1Options: { joinDuplicateHeaders: true }) has the same problem for connections that negotiated http/1.1, and non-boolean values are accepted where node throws ERR_INVALID_ARG_TYPE.
• Cause, in two parts:
  • connectionListenerHTTP1's kOnHeadersComplete builds the IncomingMessage itself and never sets req.joinDuplicateHeaders, so _addHeaderLine (src/js/node/_http_incoming.ts) takes its drop-duplicates branch for every single-valued header. Node's parserOnHeadersComplete copies the flag from the server; so does bun's native dispatcher (_http_server.ts, server.joinDuplicateHeaders -> http_req.joinDuplicateHeaders), which is why only the fallback differs.
  • Http2SecureServer copies the other HTTP/1 options (keepAliveTimeout, maxHeaderSize, ...) onto itself in its allowHTTP1 block in src/js/node/http2.ts but not this one, so even with the first part fixed there would be nothing for the fallback to read. http.Server already stores it (storeHTTPOptions), so the emit("connection") path only needs the first part.

Fix

• http1_server_fallback.ts: set req.joinDuplicateHeaders = true when the server has the option, before _addHeaderLines, in the same form as the native dispatcher.
• http2.ts: in the allowHTTP1 block, read joinDuplicateHeaders from { ...options, ...options.http1Options } like the options next to it, type-check it, and store it on the server (next to maxRequestsPerSocket, the other option the fallback reads per request; the comment on the three parser-init options below it now names node's storeHTTPOptions, the function it was already describing).
• Why this is right: it matches node. Node v26.3.0 runs storeHTTPOptions.call(this, { ...options, ...options.http1Options }) in Http2SecureServer when allowHTTP1 is set (so http1Options wins, non-booleans throw ERR_INVALID_ARG_TYPE, and nothing is validated or stored without allowHTTP1), and its parserOnHeadersComplete copies socket.server.joinDuplicateHeaders onto every request. The header combining itself is unchanged: _addHeaderLine already implements the joined form for the native path, it just never saw the flag on fallback requests. Both src changes were checked against node v26.3.0 on the scripts in the details below.
• Tests:
  • test/js/node/http/node-http.test.ts: the same raw request (doubled Authorization, Content-Type, Cookie and X-Custom) through server.emit("connection") over a duplexPair and through a listening server built from the same options, for joinDuplicateHeaders true / false / unset; both paths must produce the same complete req.headers, which is node's (single-valued headers joined only with the option, Cookie / unknown headers joined regardless).
  • test/js/node/http2/node-http2.test.js: allowHTTP1 over a TLS connection negotiating http/1.1 for the option given at top level, in http1Options, in both (http1Options wins), false, and unset, including the value stored on the server; and the ERR_INVALID_ARG_TYPE for both spellings, with nothing stored or validated without allowHTTP1.
  • test/js/node/http/node-http-proxy.js (run by node-http.test.ts, unrelated to the fix): the proxy server now binds 127.0.0.1 and the client dials the bound address. Listening on and dialing "localhost" fails with ECONNREFUSED on hosts where the server side resolves it to ::1 and the client side to 127.0.0.1 (node does the same with that setup), which made the full file fail in such environments. Same hunk as in node:http: enforce headersTimeout and requestTimeout on HTTP/1 fallback connections #37739.
  • Without the src changes, the emit test and both http2 tests fail ("one" where "one, two" is expected; no throw); with them, all three pass, as do the full node-http.test.ts and node-http2.test.js files and the upstream test-http-request-join-authorization-headers, test-http-generic-streams, test-http2-allow-http1, test-http2-https-fallback*, test-http2-createsecureserver-options and test-http-server-options-* tests.
• Related open PRs touch the same areas but not this: node:http: deliver all request headers on HTTP/1 fallback connections and enforce requireHostHeader #37735 (header assembly and requireHostHeader; it adds requireHostHeader at the same spot in http2.ts in the same style) and node:http: enforce headersTimeout and requestTimeout on HTTP/1 fallback connections #37739 (switches that block to storeHTTPOptions, which would store this option too, but nothing on the fallback side reads it). Whichever lands second has a small, mechanical conflict.
• Deliberately not included: the same function also diverges from the native path for requests with 32 or more header fields and ignores maxHeadersCount; both are what node:http: deliver all request headers on HTTP/1 fallback connections and enforce requireHostHeader #37735 fixes, so the new parity test uses a small request and leaves those cases to that PR's tests.

Background

• joinDuplicateHeaders (node 18.14+): a server (and client) option. IncomingMessage.headers is built by _addHeaderLine, which has fixed rules per header name: most are joined with ", ", Cookie with "; ", Set-Cookie becomes an array, and a list of single-valued headers (Authorization, Content-Type, Host, User-Agent, ...) keeps the first value. The option makes that last group join with ", " as well (RFC 9110 section 5.3); _addHeaderLine reads it as this.joinDuplicateHeaders on the request, so whoever constructs the request has to copy it from the server.
• The HTTP/1 fallback: bun's http.Server normally serves connections natively. Two cases are served by JS instead, driving the llhttp-based HTTPParser binding over a plain Duplex: an http2.createSecureServer({ allowHTTP1: true }) connection whose ALPN negotiation picked http/1.1 (or nothing), and any socket an application hands to an http.Server with server.emit("connection", socket). That code builds its own IncomingMessage / ServerResponse, so server options only apply there if it copies them over itself.
• http1Options: node lets createSecureServer take the HTTP/1 server options either at top level or grouped under options.http1Options, the latter taking precedence.

Repro scripts and output (node v26.3.0 vs bun 1.4.0 vs this PR)

server.emit("connection"):

const http = require("http");
const { duplexPair } = require("stream");
const server = http.createServer({ joinDuplicateHeaders: true }, (req, res) => {
  console.log(req.headers.authorization);
  res.end();
});
const [c, s] = duplexPair();
server.emit("connection", s);
c.resume();
c.write("GET / HTTP/1.1\r\nHost: a\r\nAuthorization: one\r\nAuthorization: two\r\nConnection: close\r\n\r\n");

node v26.3.0: one, two
bun 1.4.0:    one        (server.listen() path on the same bun: one, two)
this PR:      one, two

allowHTTP1 (TLS client with ALPNProtocols: ["http/1.1"], same two Authorization lines), per server options, req.headers.authorization / server.joinDuplicateHeaders:

options	node v26.3.0	bun 1.4.0	this PR
{ joinDuplicateHeaders: true }	one, two / true	one / undefined	one, two / true
{ http1Options: { joinDuplicateHeaders: true } }	one, two / true	one / undefined	one, two / true
{ joinDuplicateHeaders: true, http1Options: { joinDuplicateHeaders: false } }	one / false	one / undefined	one / false
{ joinDuplicateHeaders: false }	one / false	one / undefined	one / false
{}	one / undefined	one / undefined	one / undefined
{ joinDuplicateHeaders: "yes" }	throws ERR_INVALID_ARG_TYPE	constructs	throws ERR_INVALID_ARG_TYPE
{ http1Options: { joinDuplicateHeaders: 1 } }	throws ERR_INVALID_ARG_TYPE	constructs	throws ERR_INVALID_ARG_TYPE
{ joinDuplicateHeaders: "yes" } without allowHTTP1	constructs, nothing stored	same	same

The error message is node's verbatim: The "options.joinDuplicateHeaders" property must be of type boolean. Received type string ('yes').

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e54f6e75-2171-4fb4-88b8-6c0c8fbf99b0

📥 Commits

Reviewing files that changed from the base of the PR and between bdb7382 and 0c21d69.

📒 Files selected for processing (5)

• src/js/internal/http1_server_fallback.ts
• src/js/node/http2.ts
• test/js/node/http/node-http-proxy.js
• test/js/node/http/node-http.test.ts
• test/js/node/http2/node-http2.test.js

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 5:05 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 0c21d69 has some failures in Build #94595 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38036

That installs a local version of the PR into your bun-38036 executable, so you can run:

bun-38036 --bun

[robobun]

Status: fix is up in this PR (current head 0c21d69).

Reproduced with the scripts in the PR description (server.emit("connection") over a duplexPair, and http2.createSecureServer({ allowHTTP1: true }) with a TLS client negotiating http/1.1): node v26.3.0 reports one, two for a doubled Authorization header on both paths with joinDuplicateHeaders: true, bun 1.4.0 and main report one. With this branch both paths report one, two; the new tests in test/js/node/http/node-http.test.ts (emitted connection compared against the same server listening) and test/js/node/http2/node-http2.test.js fail on main and pass here.

[claude]

LGTM — small, focused Node compat fix that mirrors the existing native-path pattern.

What was reviewed:

• Verified the fallback's if (server.joinDuplicateHeaders) { req.joinDuplicateHeaders = true; } is byte-identical to the native dispatcher in _http_server.ts:791-793, and that _addHeaderLine in _http_incoming.ts reads it as this.joinDuplicateHeaders before the flag is consulted.
• Verified the http2.ts hunk sits inside { ...options, ...options.http1Options } so http1Options wins, validateBoolean is already imported, and the validate-then-unconditionally-assign shape matches storeHTTPOptions in _http_server.ts:3956-3960.
• Checked the Object.hasOwn(..., "joinDuplicateHeaders") assertion holds because the whole block is gated on allowHTTP1 === true, and the { server: undefined } case is consistent with the unconditional assignment.
• Tests wire error/close to reject, clean up in finally, and cover the true/false/unset × top-level/http1Options/precedence matrix plus the type-check error message.

Extended reasoning...

Overview

Two small src changes (~8 lines total) plus three new tests. http1_server_fallback.ts copies server.joinDuplicateHeaders onto the IncomingMessage before _addHeaderLines, exactly as the native dispatcher already does. http2.ts reads, type-checks, and stores joinDuplicateHeaders in the existing allowHTTP1 block alongside maxHeaderSize/insecureHTTPParser/etc., using the same shape as storeHTTPOptions in _http_server.ts.

Security risks

None. This only affects how duplicate request header values are combined into req.headers (join with ", " vs. keep-first) when the user explicitly opts in — no new parsing, no auth/permission surface, and the behavior already exists on the native path.

Level of scrutiny

Low. Both changes are copy-the-existing-pattern edits: the fallback line matches _http_server.ts:791-793 verbatim, and the http2 block matches storeHTTPOptions at _http_server.ts:3956-3960. validateBoolean is already imported in http2.ts. The http1Options local is { ...options, ...options.http1Options }, so precedence matches Node and the tests. The header-combining logic in _http_incoming.ts (_addHeaderLine reading this.joinDuplicateHeaders) is unchanged and already correct for the native path.

Other factors

Tests are well-formed per the repo's review guidance: failure events (error, close) are wired to reject the awaited promise, cleanup happens in finally, assertions are exact (toEqual on full objects, expect.objectContaining with code and full message for the throw), and the variant matrix is covered (true/false/unset for emit; top-level/http1Options/both-with-precedence/false/unset for allowHTTP1; type error for both spellings; no validation without allowHTTP1). The PR description confirms fail-before/pass-after and cross-checks against Node v26.3.0. The noted overlap with #37735/#37739 is a mechanical merge conflict, not a correctness concern.

[claude]

LGTM — small, well-scoped Node compat fix that mirrors the existing native-path pattern exactly.

What was reviewed:

• http1_server_fallback.ts change is byte-for-byte the same shape as the native dispatcher at _http_server.ts:791-793; placed before _addHeaderLines so _addHeaderLine sees the flag.
• http2.ts change matches storeHTTPOptions in _http_server.ts:3956-3960 and _http_client.ts:291-296 (validate-if-defined, then store); validateBoolean already imported.
• Tests cover both entry points (emit-connection and allowHTTP1), true/false/unset, http1Options precedence, and the ERR_INVALID_ARG_TYPE gate; error/close events are wired to reject and resources released in finally.

The comment-cop bot flag on the two-line comment at http1_server_fallback.ts:294-295 looks like a false positive — it's shorter than most neighbouring comments in that file and isn't justifying a workaround.

Extended reasoning...

Overview

Two source changes plus tests:

• src/js/internal/http1_server_fallback.ts: copy server.joinDuplicateHeaders onto the freshly-constructed IncomingMessage before _addHeaderLines, so _addHeaderLine in _http_incoming.ts takes its join branch for single-valued headers. This is the same conditional the native dispatcher already applies at _http_server.ts:791-793.
• src/js/node/http2.ts: in the allowHTTP1 block of Http2SecureServer, read joinDuplicateHeaders from the merged { ...options, ...options.http1Options } bag, validateBoolean when defined, and store on this — the same shape as storeHTTPOptions in _http_server.ts:3956-3960 and the client at _http_client.ts:291-296. Also fixes a stale storeHTTP1Options → storeHTTPOptions reference in the adjacent comment.
• New tests in node-http.test.ts (emit-connection over duplexPair compared against the same server listening natively, for true/false/unset) and node-http2.test.js (allowHTTP1 over TLS negotiating http/1.1, top-level vs http1Options vs both, plus type-check behaviour with and without allowHTTP1).

Security risks

None. joinDuplicateHeaders is an opt-in Node-compat option that changes how duplicate single-valued request headers are folded into req.headers (join with ", " instead of first-wins). It doesn't relax parsing, doesn't touch auth/TLS, and only fires when the application explicitly sets the option. The type check added in http2.ts is a strictness increase, matching Node.

Level of scrutiny

Low-to-medium. The runtime change is ~5 lines that copy an established pattern verbatim from two sibling call sites; the header-combining logic in _addHeaderLine is unchanged. The PR description includes a Node v26.3.0 comparison table for every option shape, and the tests assert against the listening (native) path so a divergence would surface. Node/Web compat area, but this is plumbing an existing flag through a path that forgot to read it, not new behaviour.

Other factors

• All required imports (validateBoolean, duplexPair, once, connect, AddressInfo) are already present in the touched files.
• Tests follow harness conventions: port: 0, error/close wired to reject the awaited promise, cleanup in finally, exact-value assertions on the whole headers object, and the http test compares the fallback path against the native path so it can't vacuously pass.
• The github-actions comment-cop flag on the new two-line comment appears to be a false positive: it's a reference to Node's parserOnHeadersComplete (the pattern REVIEW.md asks for on ported code), not a workaround justification, and is shorter than the surrounding comments in the same function.
• The PR notes small mechanical conflicts with #37735/#37739; whichever lands second resolves trivially and doesn't affect this change's correctness.

[robobun]

Self-review follow-ups, pushed as aafb657 and d6960b1 (no change to the fix itself):

• The node-http.test.ts test now sends the same raw request through a listening server built from the same options as well, and expects both paths to produce the same complete req.headers for joinDuplicateHeaders true / false / unset. On main the failure output shows the emitted connection diverging while the listening path already matches. The same function's two other known divergences from the native path (requests with 32 or more header fields, maxHeadersCount) are what node:http: deliver all request headers on HTTP/1 fallback connections and enforce requireHostHeader #37735 fixes, so they are left to that PR and the parity request here stays small; the description says so now.
• In http2.ts the new option moved up next to maxRequestsPerSocket (the other option the fallback reads per request) so the comment on the three parser-init options below it stays accurate; that comment referred to a storeHTTP1Options, which node does not have, and now names storeHTTPOptions.
• The comment in http1_server_fallback.ts is one line.

All three new tests still fail on bun 1.4.0 and pass on the debug build, as do the upstream test-http2-allow-http1, test-http2-https-fallback*, test-http2-createsecureserver-options, test-http-generic-streams and test-http-request-join-authorization-headers tests.

[claude]

LGTM — small Node.js compat fix that mirrors the native dispatcher and storeHTTPOptions exactly.

What was reviewed:

• http1_server_fallback.ts: the added req.joinDuplicateHeaders assignment matches the native path at _http_server.ts:791-793 verbatim and runs before _addHeaderLines.
• http2.ts: validateBoolean is already imported; the merged http1Options bag at line 6801 gives http1Options precedence as Node does; the validate-then-store shape matches storeHTTPOptions in _http_server.ts:3956-3960.
• Tests cover both entry paths (emit-connection and allowHTTP1), true/false/unset, http1Options precedence, and the type-check gated on allowHTTP1.
• The comment-cop bot note appears stale/misfired — the added comment is already one line as of d6960b1.

Extended reasoning...

Overview

This PR wires the joinDuplicateHeaders server option through to requests served by the JS HTTP/1 fallback (server.emit('connection', ...) and http2.createSecureServer({ allowHTTP1: true })). Two source changes: a 4-line addition in src/js/internal/http1_server_fallback.ts that copies server.joinDuplicateHeaders onto the IncomingMessage before _addHeaderLines, and a 3-line addition in src/js/node/http2.ts that reads, type-checks, and stores the option in the existing allowHTTP1 block. Three new tests across node-http.test.ts and node-http2.test.js.

Security risks

None. The header-combining logic in _addHeaderLine is unchanged; this only propagates an existing boolean flag so the fallback path behaves the same as the native path and Node. No new parsing, no new trust boundaries.

Level of scrutiny

Low-to-medium. The source changes are tiny and each is a byte-for-byte copy of an existing pattern: the fallback assignment mirrors _http_server.ts:791-793, and the http2 storage mirrors storeHTTPOptions at _http_server.ts:3956-3960 (same validateBoolean guard, same undefined-passes-through, same unconditional store). The http1Options precedence comes from the pre-existing { ...options, ...options.http1Options } merge two lines above. Also confirmed validateBoolean is already imported in http2.ts.

Other factors

• Tests are thorough: they compare the emitted-connection path against the listening (native) path with the same request bytes, cover true/false/unset, single-valued vs always-joined headers, http1Options precedence over top-level, and that validation only fires with allowHTTP1. Failure paths are wired to reject the awaited promise.
• The PR description tabulates node v26.3.0 vs bun 1.4.0 vs this PR for every option combination, and lists the upstream Node tests that pass.
• The github-actions comment-cop note on http1_server_fallback.ts was posted after d6960b1 already shortened the comment to one line; the added comment in the diff is a single line, so this looks like a bot misfire rather than an outstanding request.
• The description flags known small mechanical conflicts with #37735 / #37739 — that's a merge-order concern, not a correctness one.

[claude]

LGTM — small Node-compat fix that mirrors the native path exactly.

What was reviewed:

• Confirmed the fallback's if (server.joinDuplicateHeaders) block matches the native dispatcher in _http_server.ts line-for-line, and _addHeaderLine reads it as this.joinDuplicateHeaders.
• Confirmed the http2.ts addition matches storeHTTPOptions (validate-if-not-undefined, then store), validateBoolean is already imported, and the http1Options local is { ...options, ...options.http1Options } so precedence is Node's.
• Tests cover true/false/unset, top-level vs http1Options vs both, the type error, and parity between the emitted-connection and listening paths; error events are wired to reject and cleanup is in finally.
• The node-http-proxy.js change just binds and dials 127.0.0.1 — a hermetic-test fix, no behavior change.

Extended reasoning...

Overview

Two source changes totalling ~7 lines: src/js/internal/http1_server_fallback.ts copies server.joinDuplicateHeaders onto the IncomingMessage before _addHeaderLines (so _addHeaderLine's join branch fires on the JS fallback path the way it already does on the native path), and src/js/node/http2.ts reads/validates/stores joinDuplicateHeaders in the allowHTTP1 block so the fallback has something to read. Three new tests plus a drive-by hermetic fix to node-http-proxy.js (bind/dial 127.0.0.1 instead of localhost to avoid IPv6/IPv4 mismatch).

Security risks

None. The header-combining logic in _addHeaderLine is unchanged; this only propagates an existing, already-validated server option to a code path that was missing it. The new validateBoolean call adds strictness rather than removing any.

Level of scrutiny

Low-to-medium. Pure JS Node-compat layer, no native/memory concerns. Both source edits are line-for-line copies of established patterns: the native dispatcher at _http_server.ts:791-793 and storeHTTPOptions at _http_server.ts:3956-3960. I verified validateBoolean is imported, the http1Options local already spreads { ...options, ...options.http1Options } so http1Options wins as in Node, and IncomingMessage defaults joinDuplicateHeaders to false so the truthy-only assignment matches the native path's semantics.

Other factors

Test coverage is thorough for the size of the fix — the parity test compares the emitted-connection path against a listening server built from the same options across true/false/unset, and the http2 tests cover both option spellings, precedence, the stored value, and the ERR_INVALID_ARG_TYPE (including that it does not throw without allowHTTP1). Error events are wired to reject and resources are released in finally. The comment-cop bot's note about a long comment was addressed (now one line, thread resolved). The PR description is upfront about the two known adjacent divergences it deliberately leaves to #37735, which is the right scoping call.

[robobun]

Ready for review at 0c21d69. Verification summary for the current head:

• The three new tests fail on bun 1.4.0 ("one" where "one, two" is expected; createSecureServer accepting a non-boolean) and pass on a debug build of this branch.
• node-http.test.ts and node-http2.test.js pass in full on a release build of this branch (511 tests, 0 failures) and on a debug build (504 pass, 7 skipped). The only failures I have seen in these files on this branch are 5 s timeouts of pre-existing subprocess-spawning tests when running the debug/ASAN build with the default per-test timeout on a heavily loaded machine (require("http") alone takes about 2 s in that configuration there); which tests hit it varies from run to run, none of them involve the changed code, and CI runs these files with its longer ASAN timeouts.
• CI for this head: https://buildkite.com/bun/bun/builds/94595.