Skip to content
Merged
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
20 changes: 17 additions & 3 deletions packages/bun-uws/src/HttpParser.h
Original file line number Diff line number Diff line change
Expand Up @@ -961,9 +961,12 @@ struct HttpResponseData;
}
postPaddedBuffer = requestLineResult.position;

if(requestLineResult.isAncientHTTP) {
isAncientHTTP = true;
}
/* Written unconditionally (not just on true): ancientHttp is per-request and
* the caller re-enters this function for each pipelined request in the same
* recv buffer without clearing it, so a stale true from a prior HTTP/1.0
* request would mis-classify a following HTTP/1.1 request. isConnectRequest
* below is deliberately latched (tunnel mode persists across the loop). */
isAncientHTTP = requestLineResult.isAncientHTTP;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if(requestLineResult.isConnect) {
isConnectRequest = true;
}
Expand Down Expand Up @@ -1201,6 +1204,17 @@ struct HttpResponseData;
/* Check Transfer-Encoding header validity and conflicts */
HttpRequest::TransferEncoding transferEncoding = req->getTransferEncoding();

/* RFC 9112 6.1: Transfer-Encoding was introduced in HTTP/1.1. A server that
* receives an HTTP/1.0 message containing a Transfer-Encoding header field
* MUST treat the message as if the framing is faulty and close the connection
* after processing the message. Bun.serve rejects such a request outright,
* consistent with the TE+CL and non-chunked TE rejections below. node:http
* follows llhttp, which dispatches the request (the HTTP/1.0 request already
* marks the connection for close via isAncient). */
if (!IsNodeHttp && req->ancientHttp && transferEncoding.has) [[unlikely]] {
return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_TRANSFER_ENCODING);
}
Comment thread
robobun marked this conversation as resolved.

/* node:http compat: a Transfer-Encoding that names no chunked coding (e.g.
* "chunkedchunked") and no Content-Length is rejected by llhttp only after
* the request head completes - Node dispatches the 'request' first and the
Expand Down
102 changes: 102 additions & 0 deletions test/js/bun/http/request-smuggling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,108 @@ test("rejects Transfer-Encoding + Content-Length", async () => {
});
});

test.each([
["default close", ""],
["Connection: keep-alive", "Connection: keep-alive\r\n"],
])("rejects Transfer-Encoding on an HTTP/1.0 request (%s)", async (_label, connectionHeader) => {
// RFC 9112 6.1: a server that receives an HTTP/1.0 message containing a
// Transfer-Encoding header field MUST treat the message as if the framing is
// faulty and close the connection after processing the message. Bun.serve
// rejects such a request outright (400) so a proxy/backend split on whether
// HTTP/1.0 honours Transfer-Encoding cannot desync on the body boundary.
let handlerCalled = false;
await using server = Bun.serve({
port: 0,
fetch() {
handlerCalled = true;
return new Response("OK");
},
});

const client = net.connect(server.port, "127.0.0.1");

const maliciousRequest =
"POST / HTTP/1.0\r\n" +
"Host: localhost\r\n" +
connectionHeader +
"Transfer-Encoding: chunked\r\n" +
"\r\n" +
"5\r\nhello\r\n0\r\n\r\n";

const response = await new Promise<string>((resolve, reject) => {
let buf = "";
client.on("error", reject);
client.on("data", data => (buf += data.toString("latin1")));
client.on("close", () => resolve(buf));
client.write(maliciousRequest);
});

expect(response).toStartWith("HTTP/1.1 400 Bad Request\r\n");
expect(handlerCalled).toBe(false);
});

test("accepts Transfer-Encoding: chunked on an HTTP/1.1 request", async () => {
// Control for the HTTP/1.0 rejection above: the same chunked body is valid on HTTP/1.1.
let received = "";
await using server = Bun.serve({
port: 0,
async fetch(req) {
received = await req.text();
return new Response("OK");
},
});

const client = net.connect(server.port, "127.0.0.1");
const request =
"POST / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n0\r\n\r\n";

const response = await new Promise<string>((resolve, reject) => {
let buf = "";
client.on("error", reject);
client.on("data", data => (buf += data.toString("latin1")));
client.on("close", () => resolve(buf));
client.write(request);
});

expect(response).toStartWith("HTTP/1.1 200 OK\r\n");
expect(received).toBe("hello");
});

test("node:http dispatches Transfer-Encoding on an HTTP/1.0 request (llhttp parity)", async () => {
// node:http follows llhttp here: the HTTP/1.0 + Transfer-Encoding: chunked request
// is dispatched with the chunked body decoded, and the connection closes after (an
// HTTP/1.0 request already marks the connection for close).
const hits: { url: string; body: string; httpVersion: string }[] = [];
const server = createServer((req, res) => {
let body = "";
req.on("data", d => (body += d));
req.on("end", () => {
hits.push({ url: req.url!, body, httpVersion: req.httpVersion });
res.end("ok");
});
});
await new Promise<void>(r => server.listen(0, r));
try {
const port = (server.address() as net.AddressInfo).port;

const client = net.connect(port, "127.0.0.1");
const request = "POST /a HTTP/1.0\r\nHost: x\r\nTransfer-Encoding: chunked\r\n\r\n5\r\nhello\r\n0\r\n\r\n";

const response = await new Promise<string>((resolve, reject) => {
let buf = "";
client.on("error", reject);
client.on("data", d => (buf += d.toString("latin1")));
client.on("close", () => resolve(buf));
client.write(request);
});

expect(response).toStartWith("HTTP/1.1 200 OK\r\n");
expect(hits).toEqual([{ url: "/a", body: "hello", httpVersion: "1.0" }]);
} finally {
server.close();
}
});

test("rejects conflicting duplicate Content-Length headers", async () => {
// RFC 9112 6.3: multiple Content-Length headers with differing values must be rejected
// to prevent request smuggling.
Expand Down
Loading