diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index 19d51f3db43a..a88c05ee0ff5 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -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; if(requestLineResult.isConnect) { isConnectRequest = true; } @@ -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); + } + /* 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 diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index f915b160a1b2..3b4d97530e36 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -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((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((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(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((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.