node:http: deliver all request headers on HTTP/1 fallback connections and enforce requireHostHeader

[robobun]

Problem

Two gaps in the JS HTTP/1 path in src/js/internal/http1_server_fallback.ts, which serves an http2.createSecureServer({ allowHTTP1: true }) connection that negotiated http/1.1 and any socket handed to an http.Server through server.emit("connection", socket):

1. An HTTP/1.1 request without a Host header is dispatched to the 'request' handler.

   const http = require("http");
   const { duplexPair } = require("stream");
   const server = http.createServer((req, res) => res.end("served"));
   const [client, serverSide] = duplexPair();
   server.emit("connection", serverSide);
   client.on("data", d => console.log(JSON.stringify(String(d))));
   client.write("GET / HTTP/1.1\r\n\r\n");

   node v26.3.0: "HTTP/1.1 400 Bad Request\r\nConnection: close\r\nDate: ...\r\nTransfer-Encoding: chunked\r\n\r\n0\r\n\r\n"  (then closes the connection)
   bun 1.4.0:    "HTTP/1.1 200 OK\r\nDate: ...\r\nConnection: keep-alive\r\nKeep-Alive: timeout=5\r\nContent-Length: 6\r\n\r\nserved"
   

   Same over allowHTTP1, where Node also answers 400. A request carrying Expect: 100-continue but no Host got a 100 Continue (or a 'checkContinue' event) instead.

2. A request with 32 or more header fields reaches the handler with only its last few headers. With the request above plus Host and 31 X-* headers, the handler sees req.headers.host === undefined and one header; Node delivers all 32. Chunked request trailers are dropped as well, and server.maxHeadersCount has no effect on this path. (This is the bug node:http2: deliver all headers in the allowHTTP1 fallback past 31 headers #33540 was opened for, against the previous location of this code.) Enforcing Host on top of this would have turned those requests into 400s, so both are fixed here.

Cause

• http.Server stores requireHostHeader (default true) and the native listener enforces it (HTTP_PARSER_ERROR_MISSING_HOST_HEADER -> replyMissingHostHeader in _http_server.ts), but the fallback's kOnHeadersComplete never looked at it, and Http2SecureServer did not store the option at all.
• The native HTTPParser (src/jsc/bindings/node/http/NodeHTTPParser.cpp, a port of Node's) buffers 32 header fields; each time the buffer fills, and again for a chunked body's trailers, it hands the block to the parser's kOnHeaders callback and starts over, and once it has done that it passes undefined headers/url to kOnHeadersComplete. _http_common's pooled parsers install parserOnHeaders for this; the fallback creates a bare HTTPParser with no kOnHeaders, so flush() returns without delivering anything and the buffered fields are discarded. It also set parser.maxHeaderPairs but never read it.

Fix

• http1_server_fallback.ts, header assembly: install _http_common's parserOnHeaders as kOnHeaders (now exported, with MAX_HEADER_PAIRS) on the same _headers / _url / maxHeaderPairs fields its pooled parsers use; kOnHeadersComplete takes the collected headers and url when its own arguments are undefined and applies maxHeaderPairs the way parserOnHeadersComplete does; kOnMessageComplete files whatever was collected after the header block as the request's trailers (_addHeaderLines on a completed request) and clears it so nothing carries into the next request on the connection, as parserOnMessageComplete does. This is the same logic node:http's own connection listener runs in Node, so the observable results (headers, rawHeaders, trailers, maxHeadersCount truncation, including the case where truncation removes Host) match Node; the tests below were checked against node v26.3.0 on the same inputs.
• http1_server_fallback.ts, Host check: port the check from Node's parserOnIncoming. For an HTTP/1.1 request (HTTP/1.0 may omit Host) on a server with requireHostHeader set and req.headers.host === undefined, reply through the response object with writeHead(400, { Connection: "close" }); end() and return without dispatching. It sits where Node has it: after the upgrade/CONNECT hand-off (exempt in Node and in the native path) and before the Expect routing. The reply is the connection's last response, so the response handle is created with keep-alive off and ends the socket once the reply is written, which is what Node's _last handling does for the Connection: close it sends. No 'clientError' is emitted, also like Node. The reply is byte for byte the one Node writes; honoring a response-level Connection: close in general on fallback connections is part of node:http: queue pipelined responses on fallback connections instead of throwing ERR_HTTP_SOCKET_ASSIGNED #36991.
• http2.ts: Http2SecureServer with allowHTTP1: true stores requireHostHeader from options / options.http1Options (default true) next to the other HTTP/1 options it already stores there, and type-checks it, so requireHostHeader: false, http1Options: { requireHostHeader: false } and the ERR_INVALID_ARG_TYPE for non-booleans all behave as they do through Node's storeHTTPOptions. Without allowHTTP1 the option is neither validated nor stored, also as in Node.

Why this is correct to have: RFC 9112 section 3.2 requires a server to answer an HTTP/1.1 request without Host with 400, and both Node and Bun's native http.Server path already do; the fallback was the only server path that did not. The check is only safe once the path sees every header, which is what the assembly change provides, and that change is itself the behavior Node's connection listener has (and what plain node:http servers in Bun already have).

Tests

test/js/node/http/node-http.test.ts (server.emit("connection") over a duplexPair) and test/js/node/http2/node-http2.test.js (allowHTTP1 over a TLS connection negotiating http/1.1):

• the 400 reply compared against Node's exact bytes (modulo the Date value), the connection being closed, and the handler not running; Expect: 100-continue without Host getting the 400 rather than 100 Continue; a Host-less Upgrade or CONNECT with a listener still being handed off, and a Host-less Upgrade without a listener getting the 400 (the ordering); HTTP/1.0 without Host still dispatched; requireHostHeader: false on http.createServer, and both option spellings on http2.createSecureServer, disabling the check; server.requireHostHeader stored on the http2 server; non-boolean values rejected with Node's exact error and ignored without allowHTTP1.
• Host plus 31 or 63 other headers dispatching with every header delivered (both entry points); 40-, 2- and 70-header requests on one kept-alive connection each arriving complete (after the first block-delivered request, the parser delivers every later request on that connection the same way); a chunked request's trailers landing on that request only; maxHeadersCount truncating on both the single-block and the assembled path, and a Host beyond the cut counting as missing, as in Node.
• The raw-request helpers reject on connection errors and on the connection going away, so a transport failure fails a test immediately.

Without the src/js changes, 9 of the 13 connectionListener tests and all 5 new http2 tests fail (200 / 100 Continue where 400 is expected, host undefined with 1 of 32 headers delivered, no trailers, no requireHostHeader on the http2 server); with them everything passes, as do the full node-http.test.ts / node-http2.test.js files and the upstream test-http2-allow-http1, test-http2-https-fallback*, test-http2-createsecureserver-options, test-http-generic-streams, test-http-*-per-stream, test-http-max-headers-count and test-http-request-host-header tests.

[robobun]

Status: ready for a maintainer.

Reproduced on bun 1.4.0 and current main with both entry points (server.emit("connection", duplexPair half) writing GET / HTTP/1.1\r\n\r\n, and http2.createSecureServer({ allowHTTP1: true }) with a TLS client negotiating http/1.1); both answered 200 OK, Node v26.3.0 answers 400 Bad Request + Connection: close and closes. Self-review of the first version found that the same path also drops the first 31 headers of any request with 32 or more header fields (pre-existing; the bug #33540 was opened for), which the Host check alone would have turned into 400s for valid requests, so this PR fixes the header assembly as well; see the PR body. Without the src/js changes 9 of the 13 connectionListener tests and the 5 new http2 tests fail; with them both full files pass.

CI (build 93901, the current head, finished): 180 of 181 jobs passed, including the new tests on every test lane that ran. The one failed job is the Windows 2019 x64 test lane, which failed before running anything ("artifact download timed out after 120s for step windows-x64-build-bun"); the annotations on the passing jobs are retried-and-passed flakes on files this PR does not touch (the macOS one is the pre-existing h2c maxSessionMemory test, tracked separately). Earlier builds lost build lanes to vendored-dependency download failures, also unrelated. The one retrigger has been used, so this will stay as is unless something real shows up.

Pre-existing fallback gaps noticed along the way and tracked separately, not part of this PR: maxRequestsPerSocket and joinDuplicateHeaders are ignored on this path, and the h2c maxSessionMemory test flakes on the macOS lane.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 18 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: 81ca9b2b-289d-488c-a5aa-4f3ca179af70

📥 Commits

Reviewing files that changed from the base of the PR and between cad189d and 290161c.

📒 Files selected for processing (1)

• src/js/internal/http1_server_fallback.ts

Walkthrough

The HTTP/1 fallback now assembles fragmented headers and trailers, enforces the HTTP/1.1 Host requirement, applies header-count limits, and supports configurable enforcement in secure HTTP/2 fallback servers. Tests cover parser delivery, connection reuse, dispatch, and option validation.

Changes

HTTP/1 Fallback Compatibility

Layer / File(s)	Summary
Fragmented header and trailer assembly src/js/internal/http1_server_fallback.ts, src/js/node/_http_common.ts	The fallback collects deferred headers and URL fragments, limits parsed header pairs, and attaches deferred headers as request trailers.
Fallback parser enforcement src/js/internal/http1_server_fallback.ts, src/js/node/http2.ts	The fallback rejects HTTP/1.1 requests without Host with 400 Bad Request and Connection: close. The secure fallback validates requireHostHeader and defaults it to true.
Host and parser behavior validation test/js/node/http/node-http.test.ts, test/js/node/http2/node-http2.test.js	Tests cover fragmented headers, trailers, header limits, connection reuse, HTTP version behavior, dispatch, disabled enforcement, and invalid option values.

Suggested reviewers: cirospaciari, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the two main changes: complete HTTP/1 fallback headers and enforce requireHostHeader.
Description check	✅ Passed	The description explains the problem, implementation, behavior, option validation, and extensive test verification, despite different section headings from the template.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js/node/http2.ts`:
- Line 6807: Validate requireHostHeader before assigning the effective option in
Http2SecureServer, including both the top-level value and nested
http1Options.requireHostHeader when allowHTTP1 is enabled. Reject invalid 0 and
empty-string values instead of treating them as false, while preserving the
existing default behavior for omitted values. Add createSecureServer coverage
for these invalid configurations.

In `@test/js/node/http/node-http.test.ts`:
- Around line 4150-4160: Wire the client-side error event to reject the ended
promise in test/js/node/http/node-http.test.ts:4150-4160, while preserving the
existing end-resolution behavior; apply the same change to the TLS socket’s
ended promise in test/js/node/http2/node-http2.test.js:4311-4323 so every socket
failure rejects instead of leaving the test pending.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 87f56127-4494-4d36-a492-b16f96757ebf

📥 Commits

Reviewing files that changed from the base of the PR and between f59f705 and a93bdcd.

📒 Files selected for processing (4)

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

[claude]

LGTM — small, well-scoped Node compat fix that brings the JS HTTP/1 fallback path in line with Node's parserOnIncoming and Bun's own native listener. The inline nit about wiring reject to the ended promise in the test helpers is worth addressing but doesn't block.

Extended reasoning...

Overview

Two source changes: http1_server_fallback.ts gains a ~10-line missingHostHeader check in kOnHeadersComplete (HTTP/1.1 + server.requireHostHeader + no req.headers.host → writeHead(400, {Connection: 'close'}); end(), with shouldKeepAlive forced off so onfinished ends the socket), and http2.ts adds one line storing requireHostHeader on Http2SecureServer alongside the other http1Options it already stores. Tests cover both entry points (server.emit('connection', duplexPair) and allowHTTP1 over TLS ALPN) across the 400 case, Expect precedence, HTTP/1.0 exemption, Host-present dispatch, and requireHostHeader: false in both option spellings.

Security risks

None introduced. This tightens behavior — a Host-less HTTP/1.1 request that previously reached the user's handler now gets the RFC 9112 §3.2-mandated 400, matching Node and Bun's native http.Server path. No new parsing of untrusted input; the check reads an already-parsed header.

Level of scrutiny

Medium. This is Node compat code in a request-routing path, but the change is a direct port of Node's parserOnIncoming check placed at the same point (after upgrade/CONNECT, before Expect), using the same reply shape the native dispatcher already uses at _http_server.ts:1074. The http2.ts line follows the exact pattern of the six neighboring http1Options assignments and reads from the same merged { ...options, ...options.http1Options } bag, so both option spellings work as claimed.

Other factors

The one finding is a test-helper nit: ended doesn't get a reject wired to the socket 'error' event, so a TLS/socket failure would hang the test to timeout rather than surface the real error. It only degrades the failure mode on a loopback connection to a just-listened server; happy-path correctness and the assertions themselves are unaffected. The PR description confirms the new tests fail without the source change and pass with it, and that the full node-http.test.ts/node-http2.test.js files plus the relevant upstream Node tests still pass.

[robobun]

^{Updated 12:35 AM PT - Aug 13th, 2026}

❌ @robobun, your commit 290161c has 1 failures in Build #93901 (All Failures):

• step failed outside runner - exit 1 on 🪟 2019 x64 - test-bun

---

🧪   To try this PR locally:

bunx bun-pr 37735

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

bun-37735 --bun

[claude]

Thanks — b9e798d addresses the ended-rejection feedback: both helpers now reject head and ended from a shared fail on error/close, with .catch(() => {}) on the un-awaited one. I didn't find any issues this run. The comment-cop flag on http1_server_fallback.ts:323 is still open, and since this is RFC 9112 Host-header enforcement on the request-dispatch path, a human look would still be worthwhile.

Checked: the 400 reply reuses the existing handle/onfinished → socket.end() path (keep-alive disabled before handle creation so the close actually happens); placement after the upgrade/CONNECT hand-off and before Expect routing matches Node's parserOnIncoming and the native _http_server.ts path; validateBoolean is already imported in http2.ts. Also looked at the versionMinor === 1 vs the native path's >= 1 — llhttp only produces 1.0/1.1 and the sibling Expect check two lines below uses the same === 1 form, so it's consistent.

Extended reasoning...

Overview

Ports Node's parserOnIncoming requireHostHeader check into the JS HTTP/1 fallback listener (src/js/internal/http1_server_fallback.ts), so server.emit('connection', socket) and http2.createSecureServer({ allowHTTP1: true }) connections that negotiate http/1.1 answer a Host-less HTTP/1.1 request with 400 + Connection: close instead of dispatching it. Http2SecureServer now stores and validates requireHostHeader alongside its other HTTP/1 options. Nine new tests across node-http.test.ts and node-http2.test.js cover the 400 reply (byte-matched against Node), Expect-before-Host ordering, HTTP/1.0 exemption, requireHostHeader: false (both spellings on http2), and option validation.

Changes since my last review

Commit b9e798d addressed the inline finding I left on the previous revision: both rawRequestOverEmittedConnection and rawHttp1RequestOverAlpn now destructure rejectEnded, wire a shared fail to error and close, and pre-attach .catch(() => {}) to whichever promise a test doesn't await so its rejection doesn't surface as unhandled. It also added validateBoolean on the http2 option (matching _http_server.ts's storeHTTPOptions) plus a test for it.

Security risks

Host-header enforcement is part of RFC 9112 §3.2's request-smuggling / virtual-host-confusion defenses. This change tightens behavior to match both Node and Bun's own native http.Server path — the fallback was the only server path that didn't enforce it. The default is enforcement; opting out requires an explicit requireHostHeader: false, now type-checked. I don't see a way this loosens anything.

Level of scrutiny

Medium-high: it's a small, well-scoped port of an existing Node check into the one path that was missing it, but it sits on the request-dispatch decision for HTTP/1 fallback connections and changes what reaches the user's 'request' handler. The native sibling at _http_server.ts:1065-1075 already does the same thing with the same reply shape.

Other factors

• The versionMinor === 1 vs native >= 1 question was examined and ruled out: llhttp emits only 1.0/1.1 for major 1, and the Expect check immediately below in the same function uses the identical versionMajor === 1 && versionMinor === 1 guard, so the file is internally consistent.
• The github-actions comment-cop flag on the 3-line comment at line ~323 is still open. The comment explains why shouldKeepAlive must be cleared before the handle is created (the handle's onfinished is what ends the socket) — that's non-obvious ordering, and the surrounding file has comments of the same length, but it's the author's call whether to trim it.
• Given the RFC-compliance / dispatch-path nature and the open bot flag, deferring rather than approving.

[claude]

LGTM — the follow-up commits addressed the earlier review feedback (reject wiring in b9e798d, comment trim in 679ca21).

What was reviewed:

• The fallback's missing-Host check mirrors the native path at _http_server.ts:1065-1075 and Node's parserOnIncoming: placed after upgrade hand-off, before Expect routing, keep-alive off so onfinished ends the socket.
• Http2SecureServer stores/validates requireHostHeader alongside the other http1Options it already merges, matching _http_server.ts:3941-3947.
• Test helpers now reject both head and ended on error/close and swallow the un-awaited one; the 'close' fail is a no-op once 'end' has resolved.
• Checked that return 0 after the 400 skips body/Expect handling and that shouldKeepAlive = false is set before the handle is created so onfinished closes the connection.

Extended reasoning...

Overview

~13 lines of production code across two built-in JS modules plus ~230 lines of tests. src/js/internal/http1_server_fallback.ts gains a missing-Host check in kOnHeadersComplete that answers 400 + Connection: close and ends the connection; src/js/node/http2.ts's Http2SecureServer stores and validates requireHostHeader when allowHTTP1: true. New test suites in node-http.test.ts (via server.emit('connection', duplexPair)) and node-http2.test.js (via TLS ALPN http/1.1) cover the 400 reply byte-for-byte against Node, the Expect ordering, HTTP/1.0 exemption, requireHostHeader: false (both option spellings for http2), and ERR_INVALID_ARG_TYPE validation.

Security risks

None introduced; this tightens request validation to match RFC 9112 §3.2 and Node. The check fails closed (reject with 400) and reads req.headers.host — a parser-populated own property, not a user-prototype-pollutable lookup. requireHostHeader is read as a plain truthy check on a server-stored boolean that was validated with validateBoolean at construction.

Level of scrutiny

Moderate — Node compat behavior in the HTTP/1 fallback path. The production change is small and is a direct port of logic that already exists in two places: Node's parserOnIncoming (cited in the PR) and Bun's own native dispatcher at _http_server.ts:1065-1075, which uses the same writeHead(400, { Connection: 'close' }); end() sequence. The http2 option storage follows the exact pattern of the six neighboring http1Options fields and the validation matches _http_server.ts:3941-3947.

Other factors

All prior review feedback has been addressed: my earlier comment about the ended promise not rejecting on socket error was fixed in b9e798d (both helpers now fail() both promises from 'error' and 'close', with .catch(() => {}) on the un-awaited one); the comment-cop flags were resolved in a6365e9/679ca21 by trimming to a one-line pointer. The bug-hunting system found nothing on the current revision. Tests cover the variant matrix (HTTP/1.0 vs 1.1, Expect, both option spellings, validation errors, both entry points) and assert exact Node bytes. I traced the 'close'-fires-after-'end' ordering in both helpers to confirm the late fail() is a no-op on the already-settled promises.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js/internal/http1_server_fallback.ts`:
- Around line 396-402: In the trailer handling block, store trailers.length in a
local variable once, then use that variable for both conditional checks while
preserving the existing reset and _addHeaderLines behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: db5d6551-7fdf-4f32-9f0a-afff84c96980

📥 Commits

Reviewing files that changed from the base of the PR and between a93bdcd and cad189d.

📒 Files selected for processing (5)

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

[claude]

I reviewed this PR and the bug-hunting pass found no issues; all earlier inline feedback (test failure-event wiring, comment length, the oxlint trailers.length hit) has been addressed. Because this changes HTTP/1 request dispatch — which requests reach the handler vs. get a 400 — and adds requireHostHeader to http2.createSecureServer's option surface, a human look would still be worthwhile.

What was reviewed:

• Header-block assembly in kOnHeadersComplete/kOnMessageComplete against _http_common's parserOnHeadersComplete/parserOnMessageComplete — same undefined-guard, maxHeaderPairs clamp, and trailer reset; state cleared per request so nothing leaks across a kept-alive connection.
• Placement of the missingHostHeader check relative to Upgrade/CONNECT hand-off and Expect routing, and that shouldKeepAlive = false closes the connection through the existing onfinished path — matches Node's parserOnIncoming order.
• Http2SecureServer option handling: validateBoolean gated on allowHTTP1, both options.requireHostHeader and http1Options.requireHostHeader spellings, default true.
• Test helpers now reject on error/end/close for every awaited promise (both files), and serve() races against connection.failed.

Extended reasoning...

Overview

The PR fixes two gaps in src/js/internal/http1_server_fallback.ts, the JS HTTP/1 path used by http2.createSecureServer({ allowHTTP1: true }) connections that negotiated http/1.1 and by server.emit('connection', socket):

1. Requests with ≥32 header fields lost all but the last block because the fallback's bare HTTPParser had no kOnHeaders callback. Fixed by installing _http_common's parserOnHeaders (now exported alongside MAX_HEADER_PAIRS) on the same _headers/_url/maxHeaderPairs fields, and having kOnHeadersComplete / kOnMessageComplete read/clear them the way parserOnHeadersComplete / parserOnMessageComplete do. Trailers now land on the request; server.maxHeadersCount now applies.
2. HTTP/1.1 requests without Host were dispatched instead of answered with 400. Fixed by porting Node's parserOnIncoming check: for HTTP/1.1 on a server with requireHostHeader and no req.headers.host, force keep-alive off, writeHead(400, {Connection: 'close'}), end(), return without dispatching. Placed after the Upgrade/CONNECT hand-off and before Expect routing, matching Node.

http2.ts gains 3 lines to store and validateBoolean-check requireHostHeader from options/options.http1Options (default true) when allowHTTP1 is set. _http_common.ts gains 2 export lines. ~430 lines of tests across node-http.test.ts and node-http2.test.js.

Security risks

The Host-header check is itself an RFC 9112 §3.2 security requirement; enforcing it is a hardening. The direction of risk is that the header-assembly change must be correct for the check to be safe — if assembly regressed, valid ≥32-header requests with Host would 400. The tests cover exactly this (32/40/64/70-header requests with Host first, over both entry points and over a kept-alive sequence). No new parsing of untrusted input — parserOnHeaders is the existing shared implementation. No auth/crypto touched. The req.headers.host === undefined check is what Node uses (not truthiness), so an empty Host: value passes — that is Node's behavior.

Level of scrutiny

Medium-high. This is production HTTP request-routing code that decides whether user handlers see a request. It is not a config tweak or mechanical change: it is ~50 lines of new dispatch logic plus new user-facing option surface (requireHostHeader on Http2SecureServer). The logic is a close port of Node's and of Bun's own _http_common.ts, and the PR body traces every choice to its Node source, but HTTP dispatch ordering is exactly the sort of thing REVIEW.md calls out for careful review ("Node/Web compat", request-smuggling adjacency).

Other factors

• All prior review threads are resolved: the ended/head reject wiring (b9e798d), the keepAliveConnection/serve() failure race (cad189d), the paragraph-comment cop (679ca21, b82c350), and the no-duplicate-conditional-property-access lint on trailers.length (0ec3e65).
• Test coverage is unusually thorough for a compat fix: exact-byte comparison to Node's 400 reply, HTTP/1.0 exemption, both requireHostHeader: false spellings, Upgrade/CONNECT ordering (with and without listener), Expect: 100-continue interaction, maxHeadersCount on both single-block and assembled paths, trailers on a kept-alive connection, and ERR_INVALID_ARG_TYPE validation gated on allowHTTP1.
• CI on the head commit (build #93607) shows two build-lane failures (macOS x64, aarch64-android) that per the earlier robobun note are vendored-dependency download failures, not related to these JS-only changes.

Given the security-adjacent nature of HTTP dispatch changes and the new API surface, I'm deferring to a human reviewer rather than approving unilaterally.

[claude]

I reviewed this PR and the automated bug-hunt found nothing on this revision; the three earlier inline findings (test helpers not rejecting on error/close, and the no-duplicate-conditional-property-access lint) are all addressed in b9e798d, cad189d, and 0ec3e65. Because it changes HTTP/1 server request routing on both the allowHTTP1 and emit('connection') paths — a Host-less HTTP/1.1 request that used to dispatch now gets a 400 and a closed connection — a maintainer look is still worthwhile.

What was reviewed:

• Header-block assembly and trailer handling reuse _http_common's parserOnHeaders and mirror parserOnHeadersComplete/parserOnMessageComplete, including maxHeaderPairs truncation and per-request reset of _headers/_url so nothing carries across a kept-alive connection.
• The requireHostHeader 400 sits after the Upgrade/CONNECT hand-off and before the Expect routing, with keep-alive forced off before the response handle is created so onfinished ends the socket — matches Node's parserOnIncoming order and the byte-for-byte reply the tests assert.
• Http2SecureServer stores and validates requireHostHeader only under allowHTTP1, using the same http1Options merge as the neighboring options; the _http_common change only exports two existing symbols.

Extended reasoning...

Overview

The PR closes two Node-compat gaps in src/js/internal/http1_server_fallback.ts, the JS HTTP/1 path used by http2.createSecureServer({ allowHTTP1: true }) and http.Server#emit('connection', socket): (1) requests with ≥32 header fields lost all but the last block because the fallback's bare HTTPParser had no kOnHeaders, and trailers were dropped for the same reason; (2) an HTTP/1.1 request without Host was dispatched instead of answered with 400. It also stores/validates requireHostHeader on Http2SecureServer when allowHTTP1 is set. _http_common.ts gains two exports (parserOnHeaders, MAX_HEADER_PAIRS); http2.ts gains three lines. ~280 lines of new tests in node-http.test.ts and node-http2.test.js cover the 400 reply bytes, HTTP/1.0 vs 1.1, Expect ordering, Upgrade/CONNECT hand-off ordering, 32/40/64/70-field requests over a kept-alive connection, chunked-body trailers, maxHeadersCount truncation on both paths, both requireHostHeader option spellings on http2, and ERR_INVALID_ARG_TYPE for non-booleans.

Security risks

This touches HTTP/1 server request routing. The change is a net tightening — it enforces RFC 9112 §3.2's mandatory 400 for a Host-less HTTP/1.1 request where the fallback previously dispatched, and it now applies maxHeaderPairs (previously set but never read). Header assembly is delegated to the existing parserOnHeaders from _http_common — the same code node:http's own pooled parsers use — rather than new hand-rolled buffering. I did not find a way for the assembly change to loosen validation; the Host check is placed after the Upgrade/CONNECT hand-off exactly as in Node, so CONNECT and listened-for Upgrades remain exempt (and are tested). The one behavioral hazard is that requests previously accepted are now rejected, which is intentional and matches both Node and Bun's native http.Server path.

Level of scrutiny

Higher than a mechanical fix: this alters what the HTTP/1 fallback dispatches vs. rejects, and interacts with the response-handle keep-alive lifecycle (shouldKeepAlive = false before createHttp1FallbackResponseHandle so onfinished ends the socket after the 400). The src changes are small (~40 lines) and each carries a one-line pointer to the _http_common/Node function it mirrors, and the tests assert Node's exact reply bytes and event ordering — but a maintainer should confirm the placement and the keep-alive/close semantics are what they want on this path.

Other factors

All prior review threads are resolved: my three earlier findings (the two ended-not-rejecting-on-error test helpers and the oxlint no-duplicate-conditional-property-access failure) were fixed in b9e798d, cad189d and 0ec3e65; the comment-cop's paragraph-comment complaints were trimmed to one-line pointers; CodeRabbit's items are marked addressed. This run's bug-hunt found nothing. The test helpers now wire 'error'/'close'/'end' to rejection on both promises with the received bytes in the message, and the keep-alive helper races the response-close against a failed promise, so a regression on the header-assembly path fails immediately with a diagnostic instead of timing out. Given the scope (server request routing on two entry points) I'm deferring rather than approving.