Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
244 changes: 239 additions & 5 deletions src/js/node/http2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const net = require("node:net");
const fs = require("node:fs");
const { $data } = require("node:fs/promises");
const FileHandle = $data.FileHandle;
const { HTTPParser, allMethods } = process.binding("http_parser");
const bunTLSConnectOptions = Symbol.for("::buntlsconnectoptions::");
const bunSocketServerOptions = Symbol.for("::bunnetserveroptions::");
const kInfoHeaders = Symbol("sent-info-headers");
Expand Down Expand Up @@ -3948,13 +3949,247 @@ function closeAllSessions(server: Http2Server | Http2SecureServer) {
}
}

class FallbackServerResponse extends EventEmitter {
req: any;
socket: Socket;
statusCode: number;
statusMessage: string;
headers: Map<string, [string, any]>;
headersSent: boolean;
finished: boolean;
chunkedEncoding: boolean;
constructor(req) {
super();
this.req = req;
this.socket = req.socket;
this.statusCode = 200;
this.statusMessage = "OK";
this.headers = new Map();
this.headersSent = false;
this.finished = false;
this.chunkedEncoding = false;
}
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);
this.headersSent = true;
return this;
}
write(chunk: any, encoding?: any, cb?: any) {
if (!this.headersSent) this.writeHead(this.statusCode);
if (this.chunkedEncoding) {
const len = Buffer.byteLength(chunk, encoding);
this.socket.write(`${len.toString(16)}\r\n`);
this.socket.write(chunk, encoding);
return this.socket.write("\r\n", cb);
}
return this.socket.write(chunk, encoding, cb);
}
end(chunk?: any, encoding?: any, cb?: any) {
if (typeof chunk === "function") {
cb = chunk;
chunk = null;
encoding = null;
} else if (typeof encoding === "function") {
cb = encoding;
encoding = null;
}
if (chunk) this.write(chunk, encoding);
if (!this.headersSent) this.writeHead(this.statusCode);
if (this.chunkedEncoding) {
this.socket.write("0\r\n\r\n");
}
this.finished = true;
this.socket.end();
if (cb) cb();
this.emit("finish");
return this;
}
}

function connectionListener(socket: Socket) {
const options = this[bunSocketServerOptions] || {};
if (socket.alpnProtocol === false || socket.alpnProtocol === "http/1.1") {
// TODO: Fallback to HTTP/1.1
// if (options.allowHTTP1 === true) {
// Handle HTTP/1.1 fallback via ALPN when `allowHTTP1` is enabled.
// Note: We cannot use the standard `node:_http_server` ServerResponse here.
// Bun's native ServerResponse is heavily optimized and tightly coupled to the
// internal `Bun.serve` native handle (`kHandle`). Because this connection is
// intercepted from a raw TLSSocket, that native HTTP handle does not exist.
// To bridge this gap without triggering infinite timeouts in the native state
// machine, we use the built-in `http_parser` binding to safely parse the
// request, and a custom `FallbackServerResponse` to manually frame the
// outbound payload, fulfilling the standard `node:http` API contract.
if (options.allowHTTP1 === true) {
// Initialize native HTTP parser
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;

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.

⚠️ Potential issue | 🟡 Minor

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.

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;
};
Comment on lines +4074 to +4148

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.

⚠️ Potential issue | 🟠 Major

🧩 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 -B2

Repository: 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 10

Repository: 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 -100

Repository: 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 -n

Repository: 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 -20

Repository: 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 2

Repository: 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.ts

Repository: 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.ts

Repository: 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.ts

Repository: 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.ts

Repository: 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 -n

Repository: 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 -40

Repository: 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.


let isParserClosed = false;

const cleanupParser = () => {
if (isParserClosed) return;
isParserClosed = true;

// Safely free the native C++ memory
if (typeof parser.close === "function") {
parser.close();
} else if (typeof parser.free === "function") {
parser.free();
}

// Prevent memory leaks by detaching closures from the socket
socket.removeListener("data", onData);
socket.removeListener("end", onEnd);
socket.removeListener("close", cleanupParser);
socket.removeListener("error", cleanupParser);
};

const onData = chunk => {
const ret = parser.execute(chunk);
if (ret instanceof Error) {
cleanupParser();
socket.destroy(ret); // Destroy socket on parse error (e.g. invalid headers)
}
};

const onEnd = () => {
if (req && !req.readableEnded) req.push(null);
cleanupParser();
};

// Attach the named listeners to the socket
socket.on("data", onData);
socket.on("end", onEnd);
socket.on("close", cleanupParser);
socket.on("error", cleanupParser);

socket.resume();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
return;
}

// }
// Let event handler deal with the socket

if (!this.emit("unknownProtocol", socket)) {
Expand Down Expand Up @@ -4126,10 +4361,9 @@ class Http2SecureServer extends tls.Server {
timeout = 0;
[kSessions] = new SafeSet();
constructor(options, onRequestHandler) {
//TODO: add 'http/1.1' on ALPNProtocols list after allowHTTP1 support
if (typeof options !== "undefined") {
if (options && typeof options === "object") {
options = { ...options, ALPNProtocols: ["h2"] };
options = { ...options, ALPNProtocols: options.allowHTTP1 ? ["h2", "http/1.1"] : ["h2"] };
} else {
throw $ERR_INVALID_ARG_TYPE("options", "object", options);
}
Expand Down
109 changes: 109 additions & 0 deletions test/js/node/http2/http1-fallback.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { describe, test, expect, beforeAll, afterAll } from "bun:test";
import http2 from "node:http2";
import https from "node:https";
import { spawnSync } from "node:child_process";
import { join, basename } from "node:path";
import { readFileSync } from "node:fs";
import { tempDir } from "harness";

let KEY: Buffer;
let CERT: Buffer;

beforeAll(() => {
const tmp = tempDir(basename(import.meta.path), {});

const keyPath = join(tmp, "key.pem");
const certPath = join(tmp, "cert.pem");

const result = spawnSync("openssl", [
"req",
"-x509",
"-newkey",
"rsa:2048",
"-nodes",
"-keyout",
keyPath,
"-out",
certPath,
"-days",
"1",
"-subj",
"/CN=localhost",
]);

if (result.status !== 0) {
throw new Error(`Failed to generate test certificates: ${result.stderr.toString()}`);
}

KEY = readFileSync(keyPath);
CERT = readFileSync(certPath);
});

describe("http2.createSecureServer", () => {
test("allowHTTP1: true falls back to HTTP/1.1 correctly", async () => {
const server = http2.createSecureServer({
allowHTTP1: true,
key: KEY,
cert: CERT,
});

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);
});
Comment on lines +50 to +103

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.

⚠️ Potential issue | 🟠 Major

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.


req.end();
});
});
});
});