fix(node:http2): implement HTTP/1.1 fallback for secure servers - #28787
fix(node:http2): implement HTTP/1.1 fallback for secure servers#28787ElliotNB wants to merge 7 commits into
Conversation
When creating a secure HTTP/2 server with `allowHTTP1: true`, the server previously failed to negotiate HTTP/1.1 connections because it only advertised `h2` via ALPN. This caused clients explicitly requesting HTTP/1.1 to fail with an application protocol negotiation error. This commit resolves the issue by: 1. Conditionally advertising `http/1.1` in the ALPN protocols list when `allowHTTP1` is enabled. 2. Intercepting `http/1.1` connections in `connectionListener` and buffering the raw socket data. 3. Implementing a lightweight, pure-JavaScript parser and a custom `FallbackServerResponse` to manually frame HTTP/1.1 payloads, bypassing the native handle requirement and successfully emitting standard `request` events. Fixes oven-sh#26721
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WalkthroughAdded HTTP/1.1 ALPN fallback to HTTP/2 secure servers: when Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 3983-4035: The fallback HTTP/1.1 response path currently
concatenates statusMessage and header names/values directly into the raw head
(methods setHeader and writeHead), which allows CR/LF injection; add validation
to reject any header names, header values, and statusMessage that contain '\r'
or '\n' (or other control chars) before storing them in this.headers or
assembling the head, throwing an error (or returning) on invalid input; ensure
setHeader(name, value) performs the same validation for name and value
(including array values), and update writeHead to validate statusMessage plus
any headers passed in via the headers argument and those read from this.headers
before building the head buffer so no unsafe characters are written to
socket.write.
- Around line 4071-4141: The current parser only calls req.push(null) on socket
"end", so requests don't finish when their HTTP body is fully received (breaking
Content-Length and chunked handling); update the socket.on("data") handler to
parse and track the current message body length (using headers["content-length"]
and Transfer-Encoding: chunked semantics), consume only the bytes belonging to
the current request body (including zero-length bodies), call
req.push(bodyChunks) as data arrives and call req.push(null) as soon as that
request is complete, then reset parsed/buffer/req to allow parsing subsequent
pipelined requests; ensure socket.on("end") still ends any outstanding req, and
keep code references to req.push(null), socket.on("data"), socket.on("end"),
headers, and the FallbackServerResponse creation when implementing these
changes.
In `@test/js/node/http2/http1-fallback.test.ts`:
- Around line 5-7: Replace manual temp-dir creation and cleanup (uses of tmpdir,
join, mkdtempSync and rmSync) with the shared tempDir helper from harness:
import tempDir from the test harness and call it to create the fixture directory
instead of mkdtempSync(join(tmpdir(), ...)). Remove manual rmSync cleanup and
any manual path logic used to create the temp dir; use the path returned by
tempDir for readFileSync calls. Apply the same change for the other occurrences
mentioned (the blocks around lines 16-18 and 44-48) so all temporary-directory
handling uses harness.tempDir consistently.
🪄 Autofix (Beta)
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: 448de484-aa7f-4374-ab58-b99290c9fdfa
📒 Files selected for processing (2)
src/js/node/http2.tstest/js/node/http2/http1-fallback.test.ts
| setHeader(name, value) { | ||
| this.headers.set(name.toLowerCase(), [name, value]); | ||
| return this; | ||
| } | ||
| getHeader(name) { | ||
| return this.headers.get(name.toLowerCase())?.[1]; | ||
| } | ||
| getHeaders() { | ||
| const out = {}; | ||
| for (const [k, v] of this.headers) out[k] = v[1]; | ||
| return out; | ||
| } | ||
| hasHeader(name) { | ||
| return this.headers.has(name.toLowerCase()); | ||
| } | ||
| removeHeader(name) { | ||
| this.headers.delete(name.toLowerCase()); | ||
| } | ||
| writeHead(statusCode: number, statusMessage?: any, headers?: any) { | ||
| if (this.headersSent) return this; | ||
| this.statusCode = statusCode; | ||
| if (typeof statusMessage === "object") { | ||
| headers = statusMessage; | ||
| statusMessage = "OK"; | ||
| } | ||
| if (statusMessage) this.statusMessage = statusMessage; | ||
| if (headers) { | ||
| for (const k in headers) this.setHeader(k, headers[k]); | ||
| } | ||
| let head = `HTTP/1.1 ${this.statusCode} ${this.statusMessage}\r\n`; | ||
| if (!this.headers.has("date")) this.setHeader("Date", new Date().toUTCString()); | ||
|
|
||
| let hasConnection = this.headers.has("connection"); | ||
| let hasContentLength = this.headers.has("content-length"); | ||
| let hasTransferEncoding = this.headers.has("transfer-encoding"); | ||
|
|
||
| if (!hasConnection) { | ||
| this.setHeader("Connection", "close"); | ||
| } | ||
| if (!hasContentLength && !hasTransferEncoding) { | ||
| this.setHeader("Transfer-Encoding", "chunked"); | ||
| this.chunkedEncoding = true; | ||
| } | ||
|
|
||
| for (const [_, [name, value]] of this.headers) { | ||
| if (Array.isArray(value)) { | ||
| for (const v of value) head += `${name}: ${v}\r\n`; | ||
| } else { | ||
| head += `${name}: ${value}\r\n`; | ||
| } | ||
| } | ||
| head += "\r\n"; | ||
| this.socket.write(head); |
There was a problem hiding this comment.
Validate the fallback status line and headers before writing them to the socket.
FallbackServerResponse concatenates statusMessage, header names, and header values straight into the raw head buffer. On the HTTP/1.1 fallback path that reintroduces response-splitting bugs: any \r/\n in application-supplied data can inject extra headers or body bytes. Please reject invalid chars before building head.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/http2.ts` around lines 3983 - 4035, The fallback HTTP/1.1
response path currently concatenates statusMessage and header names/values
directly into the raw head (methods setHeader and writeHead), which allows CR/LF
injection; add validation to reject any header names, header values, and
statusMessage that contain '\r' or '\n' (or other control chars) before storing
them in this.headers or assembling the head, throwing an error (or returning) on
invalid input; ensure setHeader(name, value) performs the same validation for
name and value (including array values), and update writeHead to validate
statusMessage plus any headers passed in via the headers argument and those read
from this.headers before building the head buffer so no unsafe characters are
written to socket.write.
|
@cirospaciari Wanted to ping you for a review since you're assigned to #15419 and know the HTTP/2 codebase well. This PR implements the |
| return; | ||
| } | ||
| buffer = Buffer.concat([buffer, chunk]); | ||
| const headerEnd = buffer.indexOf("\r\n\r\n"); |
There was a problem hiding this comment.
we should not manually parse http we have multiple parsers we should do what nodejs do we have process.binding('http_parser') we also have picohttp etc
There was a problem hiding this comment.
@cirospaciari Thanks for the feedback, I'll work on correcting this.
There was a problem hiding this comment.
@cirospaciari I looked into replacing the manual parsing with process.binding('http_parser') as suggested. However, doing so immediately triggers an infinite loop in the server (with [dateheadertimer] run spamming the console indefinitely). I alluded to this problem in the original PR description.
Because this connection is intercepted from a raw TLSSocket in the HTTP/2 module, it lacks the underlying Bun.serve kHandle that the native HTTPParser relies on. I believe the native parser gets stuck waiting for a signal that never comes. This pure-JS parser bypasses all of that.
So I suppose the real question is whether you're comfortable with a userland parser for this specific ALPN fallback edgecase (I could harden the parser), or if we need to dive into the core to wire up a native kHandle for these intercepted TLS sockets.
There was a problem hiding this comment.
thank you! I strong believe that this is not a task for TS only, and we already have 3 http parsers to maintain (uWebSocket one, picohttp one and node one)
There was a problem hiding this comment.
Understood! I'll see if I can dive into C++/Zig codebase and write a bridge that attaches a dummy kHandle to the TLS socket.
There was a problem hiding this comment.
@cirospaciari I've updated the PR to address your feedback. The fallback now strips out the manual string parsing and pipes the raw bytes directly into the native HTTPParser via process.binding('http_parser'). Ended up not being necessary to dive into the C++/Zig code.
I did however have to keep the FallbackServerResponse bridge in place. I initially tried fully delegating to node:_http_server's connection listener, but doing so triggered an infinite loop in the server (spamming [dateheadertimer] run in the console).
Is this an acceptable approach? Thanks for reviewing!
|
@robobun look how node.js do that use process.binding('http_parser') make sure all tests pass |
This updates the HTTP/1.1 ALPN fallback in `Http2SecureServer` to use
the native parser `process.binding("http_parser")`.
The custom `FallbackServerResponse` is retained to ensure the parsed
request safely bridges the TLS socket to userland without triggering
the infinite timeout loop.
…into fix-http1-fallback
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Around line 4073-4161: The HTTPParser instance (parser) is never cleaned up,
causing native resource leaks; ensure you call parser.close() (or the
appropriate native cleanup method) when the socket lifecycle ends—add handlers
on socket 'close' and 'error' (and after socket.on('end') completes) to invoke
parser.close() and remove listeners so the native parser is freed; reference the
parser variable and the existing socket.on('data'), socket.on('end') handlers
and ensure you also call parser.close() if parser.execute(chunk) returns an
Error.
- Around line 3964-4070: The FallbackServerResponse class is being redefined on
every HTTP/1.1 fallback because it's declared inside connectionListener; move
the class declaration to module scope (outside and before connectionListener) so
it’s instantiated once, then update any references inside connectionListener to
use the now-module-scoped FallbackServerResponse; ensure imports/exports and
surrounding closures still work and no captured locals are required by the
class.
🪄 Autofix (Beta)
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: c1722934-ee5f-4a80-8b5a-8357dc67906b
📒 Files selected for processing (1)
src/js/node/http2.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/http2.ts`:
- Line 4098: The assignment to req.method can set it to undefined when method is
an out-of-range index from allMethods; update the logic around the existing
req.method assignment (the expression using allMethods and variable method) to
guard against undefined by falling back to the original method value or a safe
default (e.g., String(method) or "UNKNOWN") when allMethods[method] is
undefined; ensure the updated code still handles numeric and string inputs for
method and references req.method, allMethods, and method so tests and callers
continue to receive a valid method string.
- Around line 4074-4148: The parser is missing a handler for
HTTPParser.kOnHeaders so fragmented header chunks get lost; add
parser[HTTPParser.kOnHeaders] to accumulate incoming header fragments into a
temporary array (e.g., headersAcc or pendingHeaders) as they arrive,
initialize/clear that accumulator when the parser is created and on message
complete, and in parser[HTTPParser.kOnHeadersComplete] use the accumulated
fragments (concatenating pendingHeaders with the headers argument) to build
rawHeaders and headersObj exactly as currently done; ensure
parser[HTTPParser.kOnMessageComplete] clears the accumulator to avoid leaks.
🪄 Autofix (Beta)
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: b5a22ca1-82fb-4886-8d0b-71712c09693b
📒 Files selected for processing (1)
src/js/node/http2.ts
| const parser = new HTTPParser(); | ||
| // 0 = REQUEST type, 16384 = max header size, 0 = strict parsing flags | ||
| parser.initialize(0, 16384, 0); | ||
|
|
||
| let req: any = null; | ||
|
|
||
| parser[HTTPParser.kOnHeadersComplete] = ( | ||
| versionMajor, | ||
| versionMinor, | ||
| headers, | ||
| method, | ||
| url, | ||
| statusCode, | ||
| statusMessage, | ||
| upgrade, | ||
| shouldKeepAlive, | ||
| ) => { | ||
| // Initialize the request stream here so it's fresh for every request | ||
| req = new Readable({ read() {} }); | ||
| req.httpVersionMajor = versionMajor; | ||
| req.httpVersionMinor = versionMinor; | ||
| req.httpVersion = `${versionMajor}.${versionMinor}`; | ||
|
|
||
| // Use allMethods from process.binding to get the correct HTTP method | ||
| req.method = typeof method === "number" ? allMethods[method] : method; | ||
| req.url = url; | ||
|
|
||
| // 'headers' comes as a flat array from C++: [key1, val1, key2, val2, ...] | ||
| const headersObj = {}; | ||
| const rawHeaders = []; | ||
| if (headers) { | ||
| for (let i = 0; i < headers.length; i += 2) { | ||
| const key = headers[i]; | ||
| const val = headers[i + 1]; | ||
| rawHeaders.push(key, val); | ||
|
|
||
| const lowerKey = key.toLowerCase(); | ||
| if (headersObj[lowerKey]) { | ||
| headersObj[lowerKey] += `, ${val}`; | ||
| } else { | ||
| headersObj[lowerKey] = val; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| req.headers = headersObj; | ||
| req.rawHeaders = rawHeaders; | ||
| req.socket = socket; | ||
| req.connection = socket; | ||
|
|
||
| const res = new FallbackServerResponse(req); | ||
|
|
||
| if (headersObj.expect === "100-continue") { | ||
| if (this.listenerCount("checkContinue") > 0) { | ||
| this.emit("checkContinue", req, res); | ||
| } else { | ||
| socket.write("HTTP/1.1 100 Continue\r\n\r\n"); | ||
| this.emit("request", req, res); | ||
| } | ||
| } else { | ||
| this.emit("request", req, res); | ||
| } | ||
|
|
||
| return 0; // 0 tells the parser to continue processing | ||
| }; | ||
|
|
||
| parser[HTTPParser.kOnBody] = chunk => { | ||
| if (req) req.push(chunk); | ||
| return 0; | ||
| }; | ||
|
|
||
| parser[HTTPParser.kOnMessageComplete] = () => { | ||
| if (req) req.push(null); | ||
| return 0; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that kOnHeaders is used in the canonical HTTPParser setup
rg -n "kOnHeaders" src/js/node/_http_common.ts -A2 -B2Repository: oven-sh/bun
Length of output: 627
🏁 Script executed:
#!/bin/bash
# Search for parserOnHeaders implementation to understand fragmentation behavior
rg -n "parserOnHeaders" src/js/node/_http_common.ts -A 10Repository: oven-sh/bun
Length of output: 1016
🏁 Script executed:
#!/bin/bash
# Check HTTPParser usage patterns and header fragmentation
rg -n "kOnHeaders" src/js/node/ -A 3 -B 1 | head -100Repository: oven-sh/bun
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Verify the actual http1Fallback code in http2.ts around lines 4074-4148
sed -n '4074,4148p' src/js/node/http2.ts | cat -nRepository: oven-sh/bun
Length of output: 2876
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or comments about header fragmentation in http2.ts
rg -n "maxHeaderPairs\|maxHeaderSize\|fragment" src/js/node/http2.ts | head -20Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check the actual HTTPParser documentation or behavior notes
rg -n "kOnHeaders" src/js/node/ -B 5 | grep -A 5 "fragment\|accumulate\|exceed"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Verify the parser behavior with large headers by checking how HTTPParser.kOnHeaders is used elsewhere
rg -n "this._headers" src/js/node/_http_common.ts -A 2 -B 2Repository: oven-sh/bun
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Check the exact flow: does kOnHeadersComplete receive ALL headers or only the final batch?
# Look at the comment in _http_common.ts about when headers are passed
sed -n '68,90p' src/js/node/_http_common.tsRepository: oven-sh/bun
Length of output: 570
🏁 Script executed:
#!/bin/bash
# Verify that without kOnHeaders, fragmented headers are indeed lost by checking HTTPParser behavior
# Look at lines 85-87 more carefully in context
sed -n '72,95p' src/js/node/_http_common.tsRepository: oven-sh/bun
Length of output: 436
🏁 Script executed:
#!/bin/bash
# Verify the complete flow in _http_common.ts parserOnHeadersComplete to confirm the fragmentation logic
sed -n '72,110p' src/js/node/_http_common.tsRepository: oven-sh/bun
Length of output: 1107
🏁 Script executed:
#!/bin/bash
# Final verification: check if there are any other HTTP/1.1 handlers in http2.ts or elsewhere that DO register kOnHeaders
rg -n "kOnHeaders\|kOnHeadersComplete" src/js/node/http2.tsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Verify the full initialization and callback setup in http1Fallback
sed -n '4060,4080p' src/js/node/http2.ts | cat -nRepository: oven-sh/bun
Length of output: 1397
🏁 Script executed:
#!/bin/bash
# Double-check the exact initialization parameters and compare with _http_server.ts if it exists
rg -n "parser.initialize\|new HTTPParser" src/js/node/_http_server.ts -B 2 -A 8 | head -40Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Final verification: check if http1Fallback is indeed missing kOnHeaders by comparing line counts
sed -n '4074,4150p' src/js/node/http2.ts | grep -n "kOnHeaders"Repository: oven-sh/bun
Length of output: 104
Missing kOnHeaders callback for fragmented header handling.
The HTTPParser setup in _http_common.ts registers kOnHeaders to accumulate headers when they exceed the internal buffer size. This http1Fallback() handler registers only kOnHeadersComplete without kOnHeaders, which means requests with headers larger than 16KB will lose fragmented data. While large HTTP/1.1 headers are uncommon in HTTP/2 contexts, this is a correctness issue that should be addressed.
🔧 Proposed fix
Add the kOnHeaders callback to accumulate fragmented headers:
parser.initialize(0, 16384, 0);
let req: any = null;
+ let accumulatedHeaders: string[] = [];
+
+ parser[HTTPParser.kOnHeaders] = (headers, url) => {
+ if (headers) {
+ accumulatedHeaders.push(...headers);
+ }
+ return 0;
+ };
parser[HTTPParser.kOnHeadersComplete] = (
versionMajor,
@@ ... @@
const headersObj = {};
const rawHeaders = [];
+ // Merge accumulated headers from kOnHeaders
+ const allHeaders = accumulatedHeaders.length > 0
+ ? accumulatedHeaders.concat(headers || [])
+ : (headers || []);
+ accumulatedHeaders = [];
+
- if (headers) {
- for (let i = 0; i < headers.length; i += 2) {
+ for (let i = 0; i < allHeaders.length; i += 2) {
+ const key = allHeaders[i];
+ const val = allHeaders[i + 1];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/http2.ts` around lines 4074 - 4148, The parser is missing a
handler for HTTPParser.kOnHeaders so fragmented header chunks get lost; add
parser[HTTPParser.kOnHeaders] to accumulate incoming header fragments into a
temporary array (e.g., headersAcc or pendingHeaders) as they arrive,
initialize/clear that accumulator when the parser is created and on message
complete, and in parser[HTTPParser.kOnHeadersComplete] use the accumulated
fragments (concatenating pendingHeaders with the headers argument) to build
rawHeaders and headersObj exactly as currently done; ensure
parser[HTTPParser.kOnMessageComplete] clears the accumulator to avoid leaks.
| req.httpVersion = `${versionMajor}.${versionMinor}`; | ||
|
|
||
| // Use allMethods from process.binding to get the correct HTTP method | ||
| req.method = typeof method === "number" ? allMethods[method] : method; |
There was a problem hiding this comment.
Guard against undefined method from allMethods lookup.
If the parser returns a method index that exceeds the allMethods array bounds (e.g., for rare WebDAV methods like SEARCH, BIND, UNBIND), allMethods[method] returns undefined, which would propagate to req.method. Consider adding a fallback.
🛡️ Proposed fix
- req.method = typeof method === "number" ? allMethods[method] : method;
+ req.method = typeof method === "number" ? (allMethods[method] || `UNKNOWN_${method}`) : method;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/http2.ts` at line 4098, The assignment to req.method can set it
to undefined when method is an out-of-range index from allMethods; update the
logic around the existing req.method assignment (the expression using allMethods
and variable method) to guard against undefined by falling back to the original
method value or a safe default (e.g., String(method) or "UNKNOWN") when
allMethods[method] is undefined; ensure the updated code still handles numeric
and string inputs for method and references req.method, allMethods, and method
so tests and callers continue to receive a valid method string.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/http2/http1-fallback.test.ts`:
- Around line 50-103: The test currently can leave the server open when
assertions throw inside async callbacks (handlers for server.on("request") and
the https.request response callbacks), so update the handlers (the
server.on("request" callback, the https.request response callback, and
req.on("error")) to always call server.close() on any failure path before
rejecting or throwing: catch assertion errors in the request and response
handlers and call server.close() then reject(e), ensure the res.on("end")
success path closes the server as it already does, and make the req.on("error")
handler close the server before rejecting; this guarantees server.close() runs
on all error branches.
🪄 Autofix (Beta)
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: 9e99cc53-6db6-4838-85bd-fd6ecacb4a62
📒 Files selected for processing (1)
test/js/node/http2/http1-fallback.test.ts
| await new Promise<void>((resolve, reject) => { | ||
| // 1. Verify the server emits the standard 'request' event | ||
| server.on("request", (req, res) => { | ||
| try { | ||
| expect(req.httpVersionMajor).toBe(1); | ||
| expect(req.httpVersionMinor).toBe(1); | ||
| expect(req.method).toBe("GET"); | ||
| expect(req.url).toBe("/fallback-test"); | ||
|
|
||
| res.writeHead(200, { "X-Custom-Header": "bun-test" }); | ||
| res.end("HTTP/1.1 fallback successful"); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }); | ||
|
|
||
| // 2. Bind to an ephemeral port explicitly on IPv4 | ||
| server.listen(0, "127.0.0.1", () => { | ||
| const port = (server.address() as any).port; | ||
|
|
||
| // 3. Make an HTTPS request forcing HTTP/1.1 via ALPN | ||
| const req = https.request( | ||
| { | ||
| hostname: "127.0.0.1", | ||
| port: port, | ||
| path: "/fallback-test", | ||
| method: "GET", | ||
| rejectUnauthorized: false, // Bypass self-signed cert warning | ||
| ALPNProtocols: ["http/1.1"], // Explicitly demand HTTP/1.1 | ||
| }, | ||
| res => { | ||
| try { | ||
| expect(res.statusCode).toBe(200); | ||
| expect(res.headers["x-custom-header"]).toBe("bun-test"); | ||
|
|
||
| let data = ""; | ||
| res.on("data", chunk => { | ||
| data += chunk; | ||
| }); | ||
|
|
||
| res.on("end", () => { | ||
| expect(data).toBe("HTTP/1.1 fallback successful"); | ||
| server.close(() => resolve()); | ||
| }); | ||
| } catch (e) { | ||
| reject(e); | ||
| } | ||
| }, | ||
| ); | ||
|
|
||
| req.on("error", err => { | ||
| server.close(); | ||
| reject(err); | ||
| }); |
There was a problem hiding this comment.
Ensure server teardown on all failure paths to avoid hanging tests.
If an assertion throws inside async callbacks (notably Line 91), server.close() may never run, leaving an open handle and making this test flaky/hanging under failure.
🛠️ Suggested hardening
- await new Promise<void>((resolve, reject) => {
+ await new Promise<void>((resolve, reject) => {
+ const fail = (err: unknown) => {
+ server.close(() => reject(err));
+ };
+
// 1. Verify the server emits the standard 'request' event
- server.on("request", (req, res) => {
+ server.once("request", (req, res) => {
try {
expect(req.httpVersionMajor).toBe(1);
expect(req.httpVersionMinor).toBe(1);
expect(req.method).toBe("GET");
expect(req.url).toBe("/fallback-test");
res.writeHead(200, { "X-Custom-Header": "bun-test" });
res.end("HTTP/1.1 fallback successful");
} catch (e) {
- reject(e);
+ fail(e);
}
});
// 2. Bind to an ephemeral port explicitly on IPv4
server.listen(0, "127.0.0.1", () => {
const port = (server.address() as any).port;
@@
res => {
try {
expect(res.statusCode).toBe(200);
expect(res.headers["x-custom-header"]).toBe("bun-test");
@@
- res.on("end", () => {
- expect(data).toBe("HTTP/1.1 fallback successful");
- server.close(() => resolve());
- });
+ res.on("end", () => {
+ try {
+ expect(data).toBe("HTTP/1.1 fallback successful");
+ server.close(() => resolve());
+ } catch (e) {
+ fail(e);
+ }
+ });
} catch (e) {
- reject(e);
+ fail(e);
}
},
);
- req.on("error", err => {
- server.close();
- reject(err);
- });
+ req.on("error", fail);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/js/node/http2/http1-fallback.test.ts` around lines 50 - 103, The test
currently can leave the server open when assertions throw inside async callbacks
(handlers for server.on("request") and the https.request response callbacks), so
update the handlers (the server.on("request" callback, the https.request
response callback, and req.on("error")) to always call server.close() on any
failure path before rejecting or throwing: catch assertion errors in the request
and response handlers and call server.close() then reject(e), ensure the
res.on("end") success path closes the server as it already does, and make the
req.on("error") handler close the server before rejecting; this guarantees
server.close() runs on all error branches.
|
@cirospaciari Just a quick follow-up on this! I've pushed the updates to use the native HTTPParser as suggested. Whenever you have a moment to take another look, I'd love to hear your thoughts. Please let me know if there's anything else I can do to help get this unblocked. Thank you! |
|
@cirospaciari Hey, just wanted to follow up on this pull request. The latest changes incorporate your request to use the native HTTPParser. If further changes are needed, I'd appreciate your input on the general path I need to follow -- I'm happy to spend the time necessary to get this HTTP1.1 fallback bug resolved. |
|
@cirospaciari Any update on this? I'm ready to move forward in whichever direction you feel is appropriate. |
|
I think this will also resolve #14825. |
|
Hey @Jarred-Sumner / @paperclover -- I know Ciro is busy, so I wanted to see if either of you might have a few minutes to look this over? I've addressed all the feedback from April and moved the fallback logic to use the native HTTPParser as requested. My team is currently blocked by this HTTP/1.1 fallback issue, so we'd love to get this merged in. Please let me know if any other changes are needed! |
|
Thanks for the PR, and sorry this sat without a follow-up review. The HTTP/1.1 fallback for I ran Since the behavior this PR adds is on main, closing it. It is in the current canary ( |
4 participants
What does this PR do?
Resolves #26721 and #15419
When creating a secure HTTP/2 server with
allowHTTP1: true, the server previously failed to negotiate HTTP/1.1 connections because it only advertisedh2via ALPN. This caused clients explicitly requesting HTTP/1.1 to fail with an application protocol negotiation error.Furthermore, attempting to bridge the raw
TLSSocketdirectly into the standardnode:_http_serverServerResponseresulted in and infinite timeouts, as Bun's native response object strictly expects an underlyingBun.serveC++/Zig handle.This commit resolves the issue by:
http/1.1in the ALPN protocols list whenallowHTTP1is enabled.http/1.1connections inconnectionListenerand delegating the byte-level protocol parsing to the native C++HTTPParser(viaprocess.binding('http_parser')).FallbackServerResponseto manually frame HTTP/1.1 response payloads. This acts as a safe bridge between the native parser callbacks and userland, bypassing the nativekHandlerequirement while emitting standardrequestevents.How did you verify your code works?
I added a new test under
test/js/node/http2/http1-fallback.test.ts. Because BoringSSL strictly validates certificates, I couldn't just use hardcoded dummy strings. Instead, the test shells out to openssl via child_process to generate a real self-signed cert in a temp directory. It then binds the secure server explicitly to 127.0.0.1 (to dodge any IPv6-disabled CI flakiness) and hits it with an https.request that demands http/1.1 via ALPN. The test asserts that the standard request event fires and properly exposes req.httpVersion === '1.1'.I also verified this locally by compiling
bun-debug, running the original reproduction script from the issue, and hitting it withcurl -vk --http1.1 https://localhost:8443/. It successfully negotiates the fallback, streams the 200 OK response, and closes cleanly.