From 18b67f15e4efe7e6ab40bb40d7e89efb5d340a78 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:13:45 +0000 Subject: [PATCH 1/4] http: report each bad Content-Length with node's code, accept leading zeros The HTTP parser had one error value for every Content-Length failure, and node:http mapped it to HPE_UNEXPECTED_CONTENT_LENGTH. Node uses that code only for a second Content-Length field. An empty, non-numeric, or oversized value is HPE_INVALID_CONTENT_LENGTH. The parser now has one error value for each failure. It parses each Content-Length value in the header scan, in wire order, like llhttp. So the first bad field decides the error, and a bad value is reported before a Transfer-Encoding conflict or a missing Host. node:http sets err.reason and the message from the llhttp reason strings, for example "Parse Error: Empty Content-Length". The value is parsed by its digits. So a value with leading zeros that is longer than 18 bytes is now accepted by Bun.serve and node:http. Node accepts it too. The upper limit stays at STATE_SIZE_MASK. A second field with the same value is still accepted. --- packages/bun-uws/src/HttpParser.h | 78 ++++++++------ src/js/node/_http_server.ts | 28 ++++- test/js/bun/http/request-smuggling.test.ts | 33 ++++++ ...entError-when-Content-Length-is-invalid.ts | 4 +- .../http/node-http-transfer-encoding.test.ts | 101 +++++++++++++++++- 5 files changed, 207 insertions(+), 37 deletions(-) diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index 7925b43cdf92..d79a90b722f2 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -73,6 +73,8 @@ struct HttpResponseData; enum HttpParserError: uint8_t { HTTP_PARSER_ERROR_NONE = 0, HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING = 1, + /* A Content-Length value has a byte that is not a digit (llhttp's + * HPE_INVALID_CONTENT_LENGTH "Invalid character in Content-Length"). */ HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH = 2, HTTP_PARSER_ERROR_INVALID_TRANSFER_ENCODING = 3, HTTP_PARSER_ERROR_MISSING_HOST_HEADER = 4, @@ -105,6 +107,16 @@ struct HttpResponseData; * body. llhttp reports HPE_INVALID_CONTENT_LENGTH ("Content-Length can't * be present with Transfer-Encoding"). node:http compat only. */ HTTP_PARSER_ERROR_TRAILER_CONTENT_LENGTH = 17, + /* A Content-Length field has an empty value (llhttp's + * HPE_INVALID_CONTENT_LENGTH "Empty Content-Length"). */ + HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH = 18, + /* A Content-Length value is too large to frame a body (llhttp's + * HPE_INVALID_CONTENT_LENGTH "Content-Length overflow"). */ + HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW = 19, + /* A second Content-Length field has a different value. llhttp reports + * every second field as HPE_UNEXPECTED_CONTENT_LENGTH "Duplicate + * Content-Length". This parser accepts a second field with the same value. */ + HTTP_PARSER_ERROR_DUPLICATE_CONTENT_LENGTH = 20, }; @@ -619,22 +631,26 @@ struct HttpResponseData; * personality so a client cannot stream unbounded extension bytes. */ static const uint64_t MAX_CHUNK_EXTENSION_SIZE = 16 * 1024; - /* Returns UINT64_MAX on error. Maximum 999999999 is allowed. */ - static uint64_t toUnsignedInteger(std::string_view str) { - /* We assume at least 64-bit integer giving us safely 999999999999999999 (18 number of 9s) */ - if (str.length() > 18) { - return UINT64_MAX; - } - - uint64_t unsignedIntegerValue = 0; + /* Parses a non-empty Content-Length value (RFC 9110 8.6: 1*DIGIT) into value. + * Like llhttp, the first byte that is not a digit is an invalid character and + * a value that grows past the limit is an overflow. The limit is STATE_SIZE_MASK, + * not UINT64_MAX: remainingStreamingBytes holds either this byte count or the + * ChunkedEncoding state word, and isParsingChunkedEncoding() tells them apart + * by the flag bits above STATE_SIZE_MASK. */ + static HttpParserError parseContentLength(std::string_view str, uint64_t &value) { + uint64_t result = 0; for (char c : str) { - /* As long as the letter is 0-9 we cannot overflow. */ if (c < '0' || c > '9') { - return UINT64_MAX; + return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; + } + /* result <= STATE_SIZE_MASK (2^59 - 1) here, so result * 10 + 9 stays below 2^63. */ + result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); + if (result > STATE_SIZE_MASK) { + return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; } - unsignedIntegerValue = unsignedIntegerValue * 10ull + ((unsigned int) c - (unsigned int) '0'); } - return unsignedIntegerValue; + value = result; + return HTTP_PARSER_ERROR_NONE; } static inline uint64_t hasLess(uint64_t x, uint64_t n) { @@ -1186,23 +1202,32 @@ struct HttpResponseData; * the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt * to perform request smuggling (Section 11.2) or response splitting (Section 11.1) and * ought to be handled as an error. */ - /* RFC 9110 8.6 + RFC 9112 6.3: locate the Content-Length header and, in the - * same pass, verify every Content-Length header carries the same non-empty - * value. A single empty value or multiple differing values are ambiguous and - * must be rejected to prevent request smuggling. The bloom filter short-circuits - * the common "no Content-Length" case. */ + /* RFC 9110 8.6 + RFC 9112 6.3: locate the Content-Length header and parse its + * value. An empty, non-numeric or oversized value, or a second field with a + * different value, is ambiguous and must be rejected to prevent request + * smuggling. A second field with the same value is accepted (RFC 9110 8.6 + * allows that, llhttp rejects it). The fields are checked in wire order, so the + * first bad field decides the error, as in llhttp. A bad value is reported + * before the Transfer-Encoding and Host checks below. llhttp does the same when + * Content-Length comes first. When Transfer-Encoding comes first, llhttp rejects + * the Content-Length field by its name. The bloom filter short-circuits the + * common "no Content-Length" case. */ std::string_view contentLengthString; + uint64_t contentLength = 0; if (req->bf.mightHave("content-length")) { for (HttpRequest::Header *h = req->headers; (++h)->key.length(); ) { if (h->key.length() == 14 && !strncasecmp(h->key.data(), "content-length", 14)) { + if (h->value.length() == 0) { + return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH); + } if (contentLengthString.data() == nullptr) { - if (h->value.length() == 0) { - return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH); + if (HttpParserError contentLengthError = parseContentLength(h->value, contentLength)) [[unlikely]] { + return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, contentLengthError); } contentLengthString = h->value; } else if (h->value.length() != contentLengthString.length() || strncmp(h->value.data(), contentLengthString.data(), contentLengthString.length())) { - return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH); + return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_DUPLICATE_CONTENT_LENGTH); } } } @@ -1279,17 +1304,10 @@ struct HttpResponseData; const char *querySeparatorPtr = (const char *) memchr(req->headers->value.data(), '?', req->headers->value.length()); req->querySeparator = (unsigned int) ((querySeparatorPtr ? querySeparatorPtr : req->headers->value.data() + req->headers->value.length()) - req->headers->value.data()); - // lets check if content len is valid before calling requestHandler + /* Set before calling requestHandler. parseContentLength() already kept the + * value at or below STATE_SIZE_MASK, so it cannot reach a chunked flag bit. */ if(contentLengthStringLen) { - remainingStreamingBytes = toUnsignedInteger(contentLengthString); - /* remainingStreamingBytes is overloaded: for Content-Length it holds the raw byte - * count, for Transfer-Encoding: chunked it holds the ChunkedEncoding state word. - * isParsingChunkedEncoding() distinguishes the two by testing the flag bits, so a - * Content-Length value must never reach a flag bit. UINT64_MAX (parse error) is - * also caught by this since UINT64_MAX > STATE_SIZE_MASK. */ - if (remainingStreamingBytes > STATE_SIZE_MASK) [[unlikely]] { - return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH); - } + remainingStreamingBytes = contentLength; } /* If returned socket is not what we put in we need diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index 45fb072718c6..9097c70a6e92 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -1209,6 +1209,9 @@ enum HttpParserError { HTTP_PARSER_ERROR_TRAILER_FIELDS_TOO_LARGE = 15, HTTP_PARSER_ERROR_CHUNK_TERMINATOR_EXPECTED = 16, HTTP_PARSER_ERROR_TRAILER_CONTENT_LENGTH = 17, + HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH = 18, + HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW = 19, + HTTP_PARSER_ERROR_DUPLICATE_CONTENT_LENGTH = 20, } // Native callback fired when the HTTP parser rejects incoming bytes. Builds // the same error object Node's parser produces and routes it through @@ -1234,19 +1237,37 @@ function onServerClientError(ssl: boolean, socket: unknown, errorCode: number, r return; } - let err; + // Node sets err.reason to the llhttp reason string and the message to + // "Parse Error: ". The Content-Length arms set reason. Each of their + // parser errors stands for exactly one llhttp code and reason, so the reason + // is right for every input. The other arms leave reason unset. + let err, reason; switch (errorCode) { case HttpParserError.HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING: err = $HPE_INVALID_CHUNK_SIZE("Parse Error: Invalid character in chunk size"); break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH: - err = $HPE_UNEXPECTED_CONTENT_LENGTH("Parse Error"); + reason = "Invalid character in Content-Length"; + err = $HPE_INVALID_CONTENT_LENGTH(`Parse Error: ${reason}`); + break; + case HttpParserError.HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH: + reason = "Empty Content-Length"; + err = $HPE_INVALID_CONTENT_LENGTH(`Parse Error: ${reason}`); + break; + case HttpParserError.HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW: + reason = "Content-Length overflow"; + err = $HPE_INVALID_CONTENT_LENGTH(`Parse Error: ${reason}`); + break; + case HttpParserError.HTTP_PARSER_ERROR_DUPLICATE_CONTENT_LENGTH: + reason = "Duplicate Content-Length"; + err = $HPE_UNEXPECTED_CONTENT_LENGTH(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_TRANSFER_ENCODING: err = $HPE_INVALID_TRANSFER_ENCODING("Parse Error: Request has invalid `Transfer-Encoding`"); break; case HttpParserError.HTTP_PARSER_ERROR_TRAILER_CONTENT_LENGTH: - err = $HPE_INVALID_CONTENT_LENGTH("Parse Error: Content-Length can't be present with Transfer-Encoding"); + reason = "Content-Length can't be present with Transfer-Encoding"; + err = $HPE_INVALID_CONTENT_LENGTH(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_REQUEST: err = $HPE_INVALID_CONSTANT("Parse Error: Expected HTTP/"); @@ -1289,6 +1310,7 @@ function onServerClientError(ssl: boolean, socket: unknown, errorCode: number, r err = $HPE_INTERNAL("Parse Error"); break; } + if (reason !== undefined) err.reason = reason; err.rawPacket = Buffer.from(rawPacket); socketOnError.$call(nodeSocket, err); } diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index eb846d393c34..6e8d64f78586 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -353,6 +353,39 @@ test("rejects empty-valued Content-Length followed by smuggled Content-Length", expect(seen).not.toContain("GET /admin"); }); +test("frames a Content-Length with leading zeros by its digits", async () => { + // RFC 9110 8.6: Content-Length = 1*DIGIT, so leading zeros are valid. This + // 19-byte value is 5: the body is "hello" and the pipelined GET is served. + const seen: string[] = []; + await using server = Bun.serve({ + port: 0, + async fetch(req) { + seen.push(`${req.method} ${new URL(req.url).pathname} body=${await req.text()}`); + return new Response("OK"); + }, + }); + + const { promise, resolve, reject } = Promise.withResolvers(); + const client = net.connect(server.port, "127.0.0.1", () => { + client.write( + "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello" + + "GET /b HTTP/1.1\r\nHost: x\r\n\r\n", + ); + }); + let raw = ""; + client.on("data", data => { + raw += data; + if (raw.match(/HTTP\/1\.1 200/g)?.length === 2) resolve(raw); + }); + client.on("error", reject); + client.on("close", () => resolve(raw)); + const response = await promise; + client.destroy(); + + expect(seen.sort()).toEqual(["GET /b body=", "POST /a body=hello"]); + expect(response.match(/HTTP\/1\.1 200/g)).toHaveLength(2); +}); + test("accepts valid Transfer-Encoding: chunked", async () => { let receivedBody = ""; diff --git a/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts b/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts index b2779f3e6dc0..96c3a483964e 100644 --- a/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts +++ b/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts @@ -25,4 +25,6 @@ const client = connect(server.address().port, () => { }); const err = (await promise) as Error; -expect(err.code).toBe("HPE_UNEXPECTED_CONTENT_LENGTH"); +// Node reports a non-numeric value as HPE_INVALID_CONTENT_LENGTH. Only a second +// Content-Length field is HPE_UNEXPECTED_CONTENT_LENGTH. +expect(err.code).toBe("HPE_INVALID_CONTENT_LENGTH"); diff --git a/test/js/node/http/node-http-transfer-encoding.test.ts b/test/js/node/http/node-http-transfer-encoding.test.ts index a1f1a3adb8d7..997fd79b66da 100644 --- a/test/js/node/http/node-http-transfer-encoding.test.ts +++ b/test/js/node/http/node-http-transfer-encoding.test.ts @@ -387,12 +387,106 @@ test("insecureHTTPParser accepts a CTL byte in a trailer value like node", async expect(result).toEqual({ trailers: { "x-t": "a\bb" }, raw: ["X-T", "a\bb"] }); }); +// Sends a POST with the given header lines and resolves with the clientError it +// causes. Rejects if the request is dispatched or the socket closes first. +async function contentLengthClientError(headers: string) { + const { promise, resolve, reject } = Promise.withResolvers(); + await using server = createServer((req, res) => { + reject(new Error(`request ${req.url} was dispatched`)); + res.end(); + }); + server.on("clientError", (err: any, socket) => { + socket.destroy(); + resolve({ code: err.code, reason: err.reason, message: err.message }); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as AddressInfo; + + const socket = connect(port, "127.0.0.1", () => { + socket.write(`POST /a HTTP/1.1\r\n${headers}\r\nhello`); + }); + socket.resume(); + socket.on("error", () => {}); + socket.on("close", () => reject(new Error("socket closed without clientError"))); + try { + return await promise; + } finally { + socket.destroy(); + } +} + +// llhttp gives each Content-Length failure its own reason. Only a second field +// is HPE_UNEXPECTED_CONTENT_LENGTH. Bun accepts a second field with the same +// value, so the rows below repeat a different one. llhttp checks the fields in +// wire order while it reads the head, so the first bad field decides, and a bad +// value that comes first is reported before a Transfer-Encoding conflict or a +// missing Host. +describe("bad Content-Length fires clientError with node's code and reason", () => { + const invalid = "HPE_INVALID_CONTENT_LENGTH"; + const duplicate = "HPE_UNEXPECTED_CONTENT_LENGTH"; + const empty = "Empty Content-Length"; + const badChar = "Invalid character in Content-Length"; + const overflow = "Content-Length overflow"; + const repeated = "Duplicate Content-Length"; + test.each([ + ["empty value", "Host: x\r\nContent-Length:\r\n", invalid, empty], + ["whitespace-only value", "Host: x\r\nContent-Length: \r\n", invalid, empty], + ["non-digit value", "Host: x\r\nContent-Length: abc\r\n", invalid, badChar], + ["signed value", "Host: x\r\nContent-Length: +5\r\n", invalid, badChar], + ["two numbers", "Host: x\r\nContent-Length: 5 5\r\n", invalid, badChar], + ["23-digit value", "Host: x\r\nContent-Length: 99999999999999999999999\r\n", invalid, overflow], + ["second field, other value", "Host: x\r\nContent-Length: 5\r\nContent-Length: 6\r\n", duplicate, repeated], + ["second field, non-digit", "Host: x\r\nContent-Length: 5\r\nContent-Length: x\r\n", duplicate, repeated], + ["second field, empty", "Host: x\r\nContent-Length: 5\r\nContent-Length:\r\n", invalid, empty], + ["first field non-digit", "Host: x\r\nContent-Length: x\r\nContent-Length: 5\r\n", invalid, badChar], + ["non-digit, then chunked", "Host: x\r\nContent-Length: x\r\nTransfer-Encoding: chunked\r\n", invalid, badChar], + ["non-digit, no Host", "Content-Length: x\r\n", invalid, badChar], + ])("%s", async (_, headers, code, reason) => { + expect(await contentLengthClientError(headers)).toEqual({ code, reason, message: `Parse Error: ${reason}` }); + }); +}); + +// Content-Length is 1*DIGIT, so leading zeros are valid and node reads this +// 19-byte value as 5. Bun used to reject every value longer than 18 bytes. +test("Content-Length with leading zeros frames the body like node", async () => { + const events: string[] = []; + await using server = createServer((req, res) => { + let body = ""; + req.on("data", d => (body += d)); + req.on("end", () => { + events.push(`request ${req.url} body=${body}`); + res.end("ok"); + }); + }); + server.on("clientError", (err: any, socket) => { + events.push(`clientError ${err.code}`); + socket.destroy(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as AddressInfo; + + const { promise, resolve } = Promise.withResolvers(); + const socket = connect(port, "127.0.0.1", () => { + socket.write( + "POST /p HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\nConnection: close\r\n\r\nhello", + ); + }); + let raw = ""; + socket.on("data", chunk => (raw += chunk.toString())); + socket.on("error", () => {}); + socket.on("close", () => resolve(raw)); + const response = await promise; + expect(events).toEqual(["request /p body=hello"]); + expect(response).toStartWith("HTTP/1.1 200"); +}); + // RFC 9110 6.5.1: framing fields (Content-Length, Transfer-Encoding) are forbidden // in trailers. llhttp runs trailers through the same header state machine and the // already-set F_CHUNKED collides, so node rejects both before the body completes. -for (const { field, value, code } of [ - { field: "Content-Length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH" }, - { field: "content-length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH" }, +const trailerContentLengthReason = "Content-Length can't be present with Transfer-Encoding"; +for (const { field, value, code, reason } of [ + { field: "Content-Length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH", reason: trailerContentLengthReason }, + { field: "content-length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH", reason: trailerContentLengthReason }, { field: "Transfer-Encoding", value: "chunked", code: "HPE_INVALID_TRANSFER_ENCODING" }, { field: "Transfer-Encoding", value: "gzip", code: "HPE_INVALID_TRANSFER_ENCODING" }, { field: "transfer-encoding", value: "chunked", code: "HPE_INVALID_TRANSFER_ENCODING" }, @@ -423,6 +517,7 @@ for (const { field, value, code } of [ const result = await promise; socket.destroy(); expect(result.err?.code).toBe(code); + if (reason !== undefined) expect(result.err?.reason).toBe(reason); expect(result.trailers).toBeUndefined(); }); } From b7e629d692b9dfde0f2f4af5c99c2040c0bea4b7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 2 Oct 2026 12:40:34 +0000 Subject: [PATCH 2/4] node:http: keep Bun.serve's Content-Length cap, set err.reason where it is exact parseContentLength() takes acceptLong. node:http passes true and reads a value longer than 18 bytes by its digits, like llhttp. Bun.serve passes false and rejects such a value, as before this branch. A value of up to 18 bytes takes one loop and one limit check, the same work as before. onServerClientError() sets err.reason on every arm whose parser error maps to exactly one llhttp reason: the Content-Length arms, and the arms for HPE_LF_EXPECTED, HPE_CLOSED_CONNECTION, HPE_PAUSED_H2_UPGRADE, HPE_INVALID_EOF_STATE, HPE_HEADER_OVERFLOW and HPE_CHUNK_EXTENSIONS_OVERFLOW. The other arms stand for several llhttp reasons and stay without one. --- packages/bun-uws/src/HttpParser.h | 68 ++++----- src/js/node/_http_server.ts | 23 ++-- test/js/bun/http/request-smuggling.test.ts | 53 ++++--- ...entError-when-Content-Length-is-invalid.ts | 2 - .../http/node-http-transfer-encoding.test.ts | 129 +++++++++++------- 5 files changed, 161 insertions(+), 114 deletions(-) diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index b74f034870af..4d25e81aba39 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -74,8 +74,7 @@ struct HttpResponseData; enum HttpParserError: uint8_t { HTTP_PARSER_ERROR_NONE = 0, HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING = 1, - /* A Content-Length value has a byte that is not a digit (llhttp's - * HPE_INVALID_CONTENT_LENGTH "Invalid character in Content-Length"). */ + /* A non-digit byte in a Content-Length value (llhttp "Invalid character in Content-Length"). */ HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH = 2, HTTP_PARSER_ERROR_INVALID_TRANSFER_ENCODING = 3, HTTP_PARSER_ERROR_MISSING_HOST_HEADER = 4, @@ -108,15 +107,9 @@ struct HttpResponseData; * body. llhttp reports HPE_INVALID_CONTENT_LENGTH ("Content-Length can't * be present with Transfer-Encoding"). node:http compat only. */ HTTP_PARSER_ERROR_TRAILER_CONTENT_LENGTH = 17, - /* A Content-Length field has an empty value (llhttp's - * HPE_INVALID_CONTENT_LENGTH "Empty Content-Length"). */ HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH = 18, - /* A Content-Length value is too large to frame a body (llhttp's - * HPE_INVALID_CONTENT_LENGTH "Content-Length overflow"). */ HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW = 19, - /* A second Content-Length field has a different value. llhttp reports - * every second field as HPE_UNEXPECTED_CONTENT_LENGTH "Duplicate - * Content-Length". This parser accepts a second field with the same value. */ + /* A second Content-Length with a different value. The same value is accepted; llhttp rejects both. */ HTTP_PARSER_ERROR_DUPLICATE_CONTENT_LENGTH = 20, }; @@ -747,20 +740,31 @@ struct HttpResponseData; * personality so a client cannot stream unbounded extension bytes. */ static const uint64_t MAX_CHUNK_EXTENSION_SIZE = 16 * 1024; - /* Parses a non-empty Content-Length value (RFC 9110 8.6: 1*DIGIT) into value. - * Like llhttp, the first byte that is not a digit is an invalid character and - * a value that grows past the limit is an overflow. The limit is STATE_SIZE_MASK, - * not UINT64_MAX: remainingStreamingBytes holds either this byte count or the - * ChunkedEncoding state word, and isParsingChunkedEncoding() tells them apart - * by the flag bits above STATE_SIZE_MASK. */ - static HttpParserError parseContentLength(std::string_view str, uint64_t &value) { + /* The limit is STATE_SIZE_MASK, not UINT64_MAX: remainingStreamingBytes shares its top bits with the chunked state flags. */ + static HttpParserError parseContentLength(std::string_view str, uint64_t &value, bool acceptLong) { uint64_t result = 0; - for (char c : str) { - if (c < '0' || c > '9') { - return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; + if (str.length() > 18) [[unlikely]] { + /* Only leading zeros keep a longer value at or below the limit. llhttp accepts them. */ + if (!acceptLong) { + return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; + } + for (char c : str) { + if (c < '0' || c > '9') { + return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; + } + result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); + if (result > STATE_SIZE_MASK) { + return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; + } + } + } else { + /* 18 digits stay below 2^63, so one check after the loop is enough. */ + for (char c : str) { + if (c < '0' || c > '9') { + return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; + } + result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); } - /* result <= STATE_SIZE_MASK (2^59 - 1) here, so result * 10 + 9 stays below 2^63. */ - result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); if (result > STATE_SIZE_MASK) { return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; } @@ -1307,16 +1311,13 @@ struct HttpResponseData; * the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt * to perform request smuggling (Section 11.2) or response splitting (Section 11.1) and * ought to be handled as an error. */ - /* RFC 9110 8.6 + RFC 9112 6.3: locate the Content-Length header and parse its - * value. An empty, non-numeric or oversized value, or a second field with a - * different value, is ambiguous and must be rejected to prevent request - * smuggling. A second field with the same value is accepted (RFC 9110 8.6 - * allows that, llhttp rejects it). The fields are checked in wire order, so the - * first bad field decides the error, as in llhttp. A bad value is reported - * before the Transfer-Encoding and Host checks below. llhttp does the same when - * Content-Length comes first. When Transfer-Encoding comes first, llhttp rejects - * the Content-Length field by its name. The bloom filter short-circuits the - * common "no Content-Length" case. */ + /* RFC 9110 8.6 + RFC 9112 6.3: locate the Content-Length header and, in the + * same pass, verify every Content-Length header carries the same non-empty + * value. A single empty value or multiple differing values are ambiguous and + * must be rejected to prevent request smuggling. The value is parsed here, in + * wire order and before the Transfer-Encoding and Host checks, like llhttp when + * Content-Length comes first. The bloom filter short-circuits the common "no + * Content-Length" case. */ std::string_view contentLengthString; uint64_t contentLength = 0; if (req->bf.mightHave("content-length")) { @@ -1326,7 +1327,7 @@ struct HttpResponseData; return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH); } if (contentLengthString.data() == nullptr) { - if (HttpParserError contentLengthError = parseContentLength(h->value, contentLength)) [[unlikely]] { + if (HttpParserError contentLengthError = parseContentLength(h->value, contentLength, IsNodeHttp)) [[unlikely]] { return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, contentLengthError); } contentLengthString = h->value; @@ -1437,8 +1438,7 @@ struct HttpResponseData; const char *querySeparatorPtr = (const char *) memchr(req->headers->value.data(), '?', req->headers->value.length()); req->querySeparator = (unsigned int) ((querySeparatorPtr ? querySeparatorPtr : req->headers->value.data() + req->headers->value.length()) - req->headers->value.data()); - /* Set before calling requestHandler. parseContentLength() already kept the - * value at or below STATE_SIZE_MASK, so it cannot reach a chunked flag bit. */ + /* parseContentLength() kept this at or below STATE_SIZE_MASK, so it cannot reach a chunked flag bit. */ if(contentLengthStringLen) { remainingStreamingBytes = contentLength; } diff --git a/src/js/node/_http_server.ts b/src/js/node/_http_server.ts index f83f758c1c7b..ba3ae9422c8f 100644 --- a/src/js/node/_http_server.ts +++ b/src/js/node/_http_server.ts @@ -1351,10 +1351,7 @@ function onServerClientError( return; } - // Node sets err.reason to the llhttp reason string and the message to - // "Parse Error: ". The Content-Length arms set reason. Each of their - // parser errors stands for exactly one llhttp code and reason, so the reason - // is right for every input. The other arms leave reason unset. + // err.reason is set where the parser error maps to exactly one llhttp reason. let err, reason; switch (errorCode) { case HttpParserError.HTTP_PARSER_ERROR_INVALID_CHUNKED_ENCODING: @@ -1387,7 +1384,8 @@ function onServerClientError( err = $HPE_INVALID_CONSTANT("Parse Error: Expected HTTP/"); break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_EOF: - err = $HPE_INVALID_EOF_STATE("Parse Error: Invalid EOF state"); + reason = "Invalid EOF state"; + err = $HPE_INVALID_EOF_STATE(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_METHOD: err = $HPE_INVALID_METHOD("Parse Error: Invalid method encountered"); @@ -1398,24 +1396,29 @@ function onServerClientError( break; case HttpParserError.HTTP_PARSER_ERROR_REQUEST_HEADER_FIELDS_TOO_LARGE: case HttpParserError.HTTP_PARSER_ERROR_TRAILER_FIELDS_TOO_LARGE: - err = $HPE_HEADER_OVERFLOW("Parse Error: Header overflow"); + reason = "Header overflow"; + err = $HPE_HEADER_OVERFLOW(`Parse Error: ${reason}`); err.bytesParsed = rawPacket.byteLength; break; case HttpParserError.HTTP_PARSER_ERROR_INVALID_HTTP_VERSION: err = $HPE_INVALID_VERSION("Parse Error: Invalid HTTP version"); break; case HttpParserError.HTTP_PARSER_ERROR_LF_EXPECTED: - err = $HPE_LF_EXPECTED("Parse Error: Missing expected LF after header value"); + reason = "Missing expected LF after header value"; + err = $HPE_LF_EXPECTED(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_CHUNK_EXTENSIONS_OVERFLOW: - err = $HPE_CHUNK_EXTENSIONS_OVERFLOW("Parse Error: Chunk extensions overflow"); + reason = "Chunk extensions overflow"; + err = $HPE_CHUNK_EXTENSIONS_OVERFLOW(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_PAUSED_H2_UPGRADE: - err = $HPE_PAUSED_H2_UPGRADE("Parse Error: Pause on PRI/Upgrade"); + reason = "Pause on PRI/Upgrade"; + err = $HPE_PAUSED_H2_UPGRADE(`Parse Error: ${reason}`); err.bytesParsed = 24; break; case HttpParserError.HTTP_PARSER_ERROR_CLOSED_CONNECTION: - err = $HPE_CLOSED_CONNECTION("Parse Error: Data after `Connection: close`"); + reason = "Data after `Connection: close`"; + err = $HPE_CLOSED_CONNECTION(`Parse Error: ${reason}`); break; case HttpParserError.HTTP_PARSER_ERROR_CHUNK_TERMINATOR_EXPECTED: err = $HPE_STRICT("Parse Error: Expected LF after chunk data"); diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index 05b1190ad095..8fd32e1f44ff 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -353,36 +353,57 @@ test("rejects empty-valued Content-Length followed by smuggled Content-Length", expect(seen).not.toContain("GET /admin"); }); -test("frames a Content-Length with leading zeros by its digits", async () => { - // RFC 9110 8.6: Content-Length = 1*DIGIT, so leading zeros are valid. This - // 19-byte value is 5: the body is "hello" and the pipelined GET is served. +// RFC 9110 8.6: Content-Length = 1*DIGIT. This 19-byte value is 5. +const zeroPaddedPost = "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello"; + +test("Bun.serve rejects a Content-Length longer than 18 bytes", async () => { const seen: string[] = []; await using server = Bun.serve({ port: 0, - async fetch(req) { - seen.push(`${req.method} ${new URL(req.url).pathname} body=${await req.text()}`); + fetch(req) { + seen.push(new URL(req.url).pathname); return new Response("OK"); }, }); const { promise, resolve, reject } = Promise.withResolvers(); - const client = net.connect(server.port, "127.0.0.1", () => { - client.write( - "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello" + - "GET /b HTTP/1.1\r\nHost: x\r\n\r\n", - ); - }); + const client = net.connect(server.port, "127.0.0.1", () => client.write(zeroPaddedPost)); let raw = ""; - client.on("data", data => { - raw += data; - if (raw.match(/HTTP\/1\.1 200/g)?.length === 2) resolve(raw); + client.on("data", data => (raw += data)); + client.on("error", reject); + client.on("close", () => resolve(raw)); + + expect(await promise).toStartWith("HTTP/1.1 400"); + expect(seen).toEqual([]); +}); + +test("node:http frames a zero-padded Content-Length by its digits (llhttp parity)", async () => { + const hits: { url: string; body: string }[] = []; + await using server = createServer((req, res) => { + let body = ""; + req.on("data", d => (body += d)); + req.on("end", () => { + hits.push({ url: req.url!, body }); + res.end("ok"); + }); }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const port = (server.address() as net.AddressInfo).port; + + const { promise, resolve, reject } = Promise.withResolvers(); + const client = net.connect(port, "127.0.0.1", () => { + client.write(zeroPaddedPost + "GET /b HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n"); + }); + let raw = ""; + client.on("data", data => (raw += data)); client.on("error", reject); client.on("close", () => resolve(raw)); const response = await promise; - client.destroy(); - expect(seen.sort()).toEqual(["GET /b body=", "POST /a body=hello"]); + expect(hits.sort((a, b) => a.url.localeCompare(b.url))).toEqual([ + { url: "/a", body: "hello" }, + { url: "/b", body: "" }, + ]); expect(response.match(/HTTP\/1\.1 200/g)).toHaveLength(2); }); diff --git a/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts b/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts index 96c3a483964e..8d93426b8c0e 100644 --- a/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts +++ b/test/js/bun/test/parallel/test-http-should-emit-clientError-when-Content-Length-is-invalid.ts @@ -25,6 +25,4 @@ const client = connect(server.address().port, () => { }); const err = (await promise) as Error; -// Node reports a non-numeric value as HPE_INVALID_CONTENT_LENGTH. Only a second -// Content-Length field is HPE_UNEXPECTED_CONTENT_LENGTH. expect(err.code).toBe("HPE_INVALID_CONTENT_LENGTH"); diff --git a/test/js/node/http/node-http-transfer-encoding.test.ts b/test/js/node/http/node-http-transfer-encoding.test.ts index eb437ee7822f..7a2e6296a992 100644 --- a/test/js/node/http/node-http-transfer-encoding.test.ts +++ b/test/js/node/http/node-http-transfer-encoding.test.ts @@ -806,27 +806,28 @@ test("insecureHTTPParser accepts a CTL byte in a trailer value like node", async expect(result).toEqual({ trailers: { "x-t": "a\bb" }, raw: ["X-T", "a\bb"] }); }); -// Sends a POST with the given header lines and resolves with the clientError it -// causes. Rejects if the request is dispatched or the socket closes first. -async function contentLengthClientError(headers: string) { +async function clientErrorFor(wire: string, { end = false } = {}) { + const dispatched: string[] = []; const { promise, resolve, reject } = Promise.withResolvers(); - await using server = createServer((req, res) => { - reject(new Error(`request ${req.url} was dispatched`)); - res.end(); + // No response: HPE_CLOSED_CONNECTION needs the first response to be pending. + await using server = createServer(req => { + dispatched.push(req.url!); + req.resume(); }); server.on("clientError", (err: any, socket) => { socket.destroy(); - resolve({ code: err.code, reason: err.reason, message: err.message }); + resolve({ dispatched, code: err.code, reason: err.reason, message: err.message }); }); await once(server.listen(0, "127.0.0.1"), "listening"); const { port } = server.address() as AddressInfo; const socket = connect(port, "127.0.0.1", () => { - socket.write(`POST /a HTTP/1.1\r\n${headers}\r\nhello`); + socket.write(wire, "latin1"); + if (end) socket.end(); }); socket.resume(); socket.on("error", () => {}); - socket.on("close", () => reject(new Error("socket closed without clientError"))); + socket.on("close", () => reject(new Error(`socket closed without clientError, dispatched: ${dispatched}`))); try { return await promise; } finally { @@ -834,12 +835,7 @@ async function contentLengthClientError(headers: string) { } } -// llhttp gives each Content-Length failure its own reason. Only a second field -// is HPE_UNEXPECTED_CONTENT_LENGTH. Bun accepts a second field with the same -// value, so the rows below repeat a different one. llhttp checks the fields in -// wire order while it reads the head, so the first bad field decides, and a bad -// value that comes first is reported before a Transfer-Encoding conflict or a -// missing Host. +// Bun accepts a second Content-Length with the same value (node rejects it), so the duplicate rows use a different one. describe("bad Content-Length fires clientError with node's code and reason", () => { const invalid = "HPE_INVALID_CONTENT_LENGTH"; const duplicate = "HPE_UNEXPECTED_CONTENT_LENGTH"; @@ -861,51 +857,81 @@ describe("bad Content-Length fires clientError with node's code and reason", () ["non-digit, then chunked", "Host: x\r\nContent-Length: x\r\nTransfer-Encoding: chunked\r\n", invalid, badChar], ["non-digit, no Host", "Content-Length: x\r\n", invalid, badChar], ])("%s", async (_, headers, code, reason) => { - expect(await contentLengthClientError(headers)).toEqual({ code, reason, message: `Parse Error: ${reason}` }); + expect(await clientErrorFor(`POST /a HTTP/1.1\r\n${headers}\r\nhello`)).toEqual({ + dispatched: [], + code, + reason, + message: `Parse Error: ${reason}`, + }); }); }); -// Content-Length is 1*DIGIT, so leading zeros are valid and node reads this -// 19-byte value as 5. Bun used to reject every value longer than 18 bytes. -test("Content-Length with leading zeros frames the body like node", async () => { - const events: string[] = []; - await using server = createServer((req, res) => { - let body = ""; - req.on("data", d => (body += d)); - req.on("end", () => { - events.push(`request ${req.url} body=${body}`); - res.end("ok"); +describe("clientError carries llhttp's reason where the parser error maps to one", () => { + const chunked = "POST /a HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunked\r\n\r\n"; + const fill = (n: number) => Buffer.alloc(n, "a").toString(); + for (const { name, wire, end, code, reason } of [ + { + name: "bare CR in a header value", + wire: "GET /a HTTP/1.1\r\nHost: x\r\nX-Foo: a\rb\r\n\r\n", + code: "HPE_LF_EXPECTED", + reason: "Missing expected LF after header value", + }, + { + name: "head over maxHeaderSize", + wire: `GET /a HTTP/1.1\r\nHost: x\r\nX-Foo: ${fill(20000)}\r\n\r\n`, + code: "HPE_HEADER_OVERFLOW", + reason: "Header overflow", + }, + { + name: "trailers over maxHeaderSize", + wire: `${chunked}0\r\nX-T: ${fill(70000)}\r\n\r\n`, + code: "HPE_HEADER_OVERFLOW", + reason: "Header overflow", + }, + { + name: "Content-Length in trailers", + wire: `${chunked}0\r\nContent-Length: 5\r\n\r\n`, + code: "HPE_INVALID_CONTENT_LENGTH", + reason: "Content-Length can't be present with Transfer-Encoding", + }, + { + name: "chunk extension over 16 KiB", + wire: `${chunked}5;${fill(17000)}\r\n`, + code: "HPE_CHUNK_EXTENSIONS_OVERFLOW", + reason: "Chunk extensions overflow", + }, + { + name: "HTTP/2 preface", + wire: "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n", + code: "HPE_PAUSED_H2_UPGRADE", + reason: "Pause on PRI/Upgrade", + }, + { + name: "bytes after Connection: close", + wire: "GET /a HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\nGET /b HTTP/1.1\r\nHost: x\r\n\r\n", + code: "HPE_CLOSED_CONNECTION", + reason: "Data after `Connection: close`", + }, + { + name: "close inside the head", + wire: "GET /a HTTP/1.1\r\nHost: x\r\n", + end: true, + code: "HPE_INVALID_EOF_STATE", + reason: "Invalid EOF state", + }, + ]) { + test(name, async () => { + expect(await clientErrorFor(wire, { end })).toMatchObject({ code, reason }); }); - }); - server.on("clientError", (err: any, socket) => { - events.push(`clientError ${err.code}`); - socket.destroy(); - }); - await once(server.listen(0, "127.0.0.1"), "listening"); - const { port } = server.address() as AddressInfo; - - const { promise, resolve } = Promise.withResolvers(); - const socket = connect(port, "127.0.0.1", () => { - socket.write( - "POST /p HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\nConnection: close\r\n\r\nhello", - ); - }); - let raw = ""; - socket.on("data", chunk => (raw += chunk.toString())); - socket.on("error", () => {}); - socket.on("close", () => resolve(raw)); - const response = await promise; - expect(events).toEqual(["request /p body=hello"]); - expect(response).toStartWith("HTTP/1.1 200"); + } }); // RFC 9110 6.5.1: framing fields (Content-Length, Transfer-Encoding) are forbidden // in trailers. llhttp runs trailers through the same header state machine and the // already-set F_CHUNKED collides, so node rejects both before the body completes. -const trailerContentLengthReason = "Content-Length can't be present with Transfer-Encoding"; -for (const { field, value, code, reason } of [ - { field: "Content-Length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH", reason: trailerContentLengthReason }, - { field: "content-length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH", reason: trailerContentLengthReason }, +for (const { field, value, code } of [ + { field: "Content-Length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH" }, + { field: "content-length", value: "5", code: "HPE_INVALID_CONTENT_LENGTH" }, { field: "Transfer-Encoding", value: "chunked", code: "HPE_INVALID_TRANSFER_ENCODING" }, { field: "Transfer-Encoding", value: "gzip", code: "HPE_INVALID_TRANSFER_ENCODING" }, { field: "transfer-encoding", value: "chunked", code: "HPE_INVALID_TRANSFER_ENCODING" }, @@ -936,7 +962,6 @@ for (const { field, value, code, reason } of [ const result = await promise; socket.destroy(); expect(result.err?.code).toBe(code); - if (reason !== undefined) expect(result.err?.reason).toBe(reason); expect(result.trailers).toBeUndefined(); }); } From 14ceac0fc14fdaaeb22eb4bd5f2da982e14f716f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:02:32 +0000 Subject: [PATCH 3/4] test: cover the Content-Length limits on both sides node:http: 2^59 - 1 is dispatched and 2^59 overflows, as an 18-byte value and as a zero-padded 19-byte value. Bun.serve: an 18-byte zero-padded value is accepted next to the 19-byte rejection. --- test/js/bun/http/request-smuggling.test.ts | 33 ++++++++++++------- .../http/node-http-transfer-encoding.test.ts | 31 +++++++++++++++++ 2 files changed, 53 insertions(+), 11 deletions(-) diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index 8fd32e1f44ff..bea3148368d9 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -353,28 +353,36 @@ test("rejects empty-valued Content-Length followed by smuggled Content-Length", expect(seen).not.toContain("GET /admin"); }); -// RFC 9110 8.6: Content-Length = 1*DIGIT. This 19-byte value is 5. -const zeroPaddedPost = "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello"; - -test("Bun.serve rejects a Content-Length longer than 18 bytes", async () => { +// RFC 9110 8.6: Content-Length = 1*DIGIT, so each zero-padded value below is 5. +test.each([ + [18, "HTTP/1.1 200", "000000000000000005", ["hello"]], + [19, "HTTP/1.1 400", "0000000000000000005", []], +])("Bun.serve answers a %d-byte zero-padded Content-Length with %s", async (_, status, value, bodies) => { const seen: string[] = []; await using server = Bun.serve({ port: 0, - fetch(req) { - seen.push(new URL(req.url).pathname); + async fetch(req) { + seen.push(await req.text()); return new Response("OK"); }, }); const { promise, resolve, reject } = Promise.withResolvers(); - const client = net.connect(server.port, "127.0.0.1", () => client.write(zeroPaddedPost)); + const client = net.connect(server.port, "127.0.0.1", () => { + client.write(`POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: ${value}\r\n\r\nhello`); + }); let raw = ""; - client.on("data", data => (raw += data)); + client.on("data", data => { + raw += data; + if (raw.includes("\r\n\r\n")) resolve(raw); + }); client.on("error", reject); client.on("close", () => resolve(raw)); + const response = await promise; + client.destroy(); - expect(await promise).toStartWith("HTTP/1.1 400"); - expect(seen).toEqual([]); + expect(response).toStartWith(status); + expect(seen).toEqual(bodies); }); test("node:http frames a zero-padded Content-Length by its digits (llhttp parity)", async () => { @@ -392,7 +400,10 @@ test("node:http frames a zero-padded Content-Length by its digits (llhttp parity const { promise, resolve, reject } = Promise.withResolvers(); const client = net.connect(port, "127.0.0.1", () => { - client.write(zeroPaddedPost + "GET /b HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n"); + client.write( + "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello" + + "GET /b HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n", + ); }); let raw = ""; client.on("data", data => (raw += data)); diff --git a/test/js/node/http/node-http-transfer-encoding.test.ts b/test/js/node/http/node-http-transfer-encoding.test.ts index 7a2e6296a992..afbf1ac2b0dc 100644 --- a/test/js/node/http/node-http-transfer-encoding.test.ts +++ b/test/js/node/http/node-http-transfer-encoding.test.ts @@ -866,6 +866,37 @@ describe("bad Content-Length fires clientError with node's code and reason", () }); }); +// Bun's limit is 2^59 - 1 (node's is 2^64 - 1). The second row reaches it with 19 bytes. +test.each([ + ["576460752303423487", "576460752303423488"], + ["0576460752303423487", "0576460752303423488"], +])("Content-Length %s is dispatched and %s overflows", async (atLimit, pastLimit) => { + const { promise, resolve, reject } = Promise.withResolvers(); + await using server = createServer(req => resolve(req.headers["content-length"])); + server.on("clientError", (err: any, socket) => { + socket.destroy(); + reject(new Error(`clientError ${err.code}: ${err.reason}`)); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as AddressInfo; + + const socket = connect(port, "127.0.0.1", () => { + socket.write(`POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: ${atLimit}\r\n\r\n`); + }); + socket.on("error", () => {}); + try { + expect(await promise).toBe(atLimit); + } finally { + socket.destroy(); + } + + expect(await clientErrorFor(`POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: ${pastLimit}\r\n\r\n`)).toMatchObject({ + dispatched: [], + code: "HPE_INVALID_CONTENT_LENGTH", + reason: "Content-Length overflow", + }); +}); + describe("clientError carries llhttp's reason where the parser error maps to one", () => { const chunked = "POST /a HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunked\r\n\r\n"; const fill = (n: number) => Buffer.alloc(n, "a").toString(); From 95eda28e5957c8f8d296ddec4812ca5a99da1228 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 3 Oct 2026 10:23:01 +0000 Subject: [PATCH 4/4] node:http: keep the 18-byte Content-Length cap for node:http too parseContentLength() rejects every value longer than 18 bytes again, for Bun.serve and for node:http, as on main. So this branch changes which error a rejected request reports, and not which requests are accepted. A long value is an invalid character if a non-digit comes before the overflow, like llhttp, and an overflow otherwise. --- packages/bun-uws/src/HttpParser.h | 31 +++++++------- test/js/bun/http/request-smuggling.test.ts | 40 +++++++------------ .../http/node-http-transfer-encoding.test.ts | 21 +++++----- 3 files changed, 39 insertions(+), 53 deletions(-) diff --git a/packages/bun-uws/src/HttpParser.h b/packages/bun-uws/src/HttpParser.h index 4d25e81aba39..f9ca41198060 100644 --- a/packages/bun-uws/src/HttpParser.h +++ b/packages/bun-uws/src/HttpParser.h @@ -741,33 +741,30 @@ struct HttpResponseData; static const uint64_t MAX_CHUNK_EXTENSION_SIZE = 16 * 1024; /* The limit is STATE_SIZE_MASK, not UINT64_MAX: remainingStreamingBytes shares its top bits with the chunked state flags. */ - static HttpParserError parseContentLength(std::string_view str, uint64_t &value, bool acceptLong) { + static HttpParserError parseContentLength(std::string_view str, uint64_t &value) { uint64_t result = 0; if (str.length() > 18) [[unlikely]] { - /* Only leading zeros keep a longer value at or below the limit. llhttp accepts them. */ - if (!acceptLong) { - return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; - } + /* Never accepted. Like llhttp, a non-digit that comes before the overflow is an invalid character. */ for (char c : str) { if (c < '0' || c > '9') { return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; } result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); if (result > STATE_SIZE_MASK) { - return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; - } - } - } else { - /* 18 digits stay below 2^63, so one check after the loop is enough. */ - for (char c : str) { - if (c < '0' || c > '9') { - return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; + break; } - result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); } - if (result > STATE_SIZE_MASK) { - return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; + return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; + } + /* 18 digits stay below 2^63, so one check after the loop is enough. */ + for (char c : str) { + if (c < '0' || c > '9') { + return HTTP_PARSER_ERROR_INVALID_CONTENT_LENGTH; } + result = result * 10ull + ((unsigned int) c - (unsigned int) '0'); + } + if (result > STATE_SIZE_MASK) { + return HTTP_PARSER_ERROR_CONTENT_LENGTH_OVERFLOW; } value = result; return HTTP_PARSER_ERROR_NONE; @@ -1327,7 +1324,7 @@ struct HttpResponseData; return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, HTTP_PARSER_ERROR_EMPTY_CONTENT_LENGTH); } if (contentLengthString.data() == nullptr) { - if (HttpParserError contentLengthError = parseContentLength(h->value, contentLength, IsNodeHttp)) [[unlikely]] { + if (HttpParserError contentLengthError = parseContentLength(h->value, contentLength)) [[unlikely]] { return HttpParserResult::error(HTTP_ERROR_400_BAD_REQUEST, contentLengthError); } contentLengthString = h->value; diff --git a/test/js/bun/http/request-smuggling.test.ts b/test/js/bun/http/request-smuggling.test.ts index bea3148368d9..08b864383bf3 100644 --- a/test/js/bun/http/request-smuggling.test.ts +++ b/test/js/bun/http/request-smuggling.test.ts @@ -353,7 +353,7 @@ test("rejects empty-valued Content-Length followed by smuggled Content-Length", expect(seen).not.toContain("GET /admin"); }); -// RFC 9110 8.6: Content-Length = 1*DIGIT, so each zero-padded value below is 5. +// An 18-byte limit and 2^59 - 1 cap both servers (node has no length limit). Each zero-padded value below is 5. test.each([ [18, "HTTP/1.1 200", "000000000000000005", ["hello"]], [19, "HTTP/1.1 400", "0000000000000000005", []], @@ -385,37 +385,25 @@ test.each([ expect(seen).toEqual(bodies); }); -test("node:http frames a zero-padded Content-Length by its digits (llhttp parity)", async () => { - const hits: { url: string; body: string }[] = []; - await using server = createServer((req, res) => { - let body = ""; - req.on("data", d => (body += d)); - req.on("end", () => { - hits.push({ url: req.url!, body }); - res.end("ok"); - }); +test("node:http reports a 19-byte zero-padded Content-Length as an overflow", async () => { + const { promise, resolve, reject } = Promise.withResolvers(); + await using server = createServer(req => reject(new Error(`request ${req.url} was dispatched`))); + server.on("clientError", (err: any, socket) => { + socket.destroy(); + resolve({ code: err.code, reason: err.reason }); }); await once(server.listen(0, "127.0.0.1"), "listening"); const port = (server.address() as net.AddressInfo).port; - const { promise, resolve, reject } = Promise.withResolvers(); const client = net.connect(port, "127.0.0.1", () => { - client.write( - "POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello" + - "GET /b HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n", - ); + client.write("POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: 0000000000000000005\r\n\r\nhello"); }); - let raw = ""; - client.on("data", data => (raw += data)); - client.on("error", reject); - client.on("close", () => resolve(raw)); - const response = await promise; - - expect(hits.sort((a, b) => a.url.localeCompare(b.url))).toEqual([ - { url: "/a", body: "hello" }, - { url: "/b", body: "" }, - ]); - expect(response.match(/HTTP\/1\.1 200/g)).toHaveLength(2); + client.on("error", () => {}); + try { + expect(await promise).toEqual({ code: "HPE_INVALID_CONTENT_LENGTH", reason: "Content-Length overflow" }); + } finally { + client.destroy(); + } }); test("accepts valid Transfer-Encoding: chunked", async () => { diff --git a/test/js/node/http/node-http-transfer-encoding.test.ts b/test/js/node/http/node-http-transfer-encoding.test.ts index afbf1ac2b0dc..fb7ea371b01b 100644 --- a/test/js/node/http/node-http-transfer-encoding.test.ts +++ b/test/js/node/http/node-http-transfer-encoding.test.ts @@ -850,6 +850,7 @@ describe("bad Content-Length fires clientError with node's code and reason", () ["signed value", "Host: x\r\nContent-Length: +5\r\n", invalid, badChar], ["two numbers", "Host: x\r\nContent-Length: 5 5\r\n", invalid, badChar], ["23-digit value", "Host: x\r\nContent-Length: 99999999999999999999999\r\n", invalid, overflow], + ["20 bytes with a non-digit", "Host: x\r\nContent-Length: 0000000000000000005x\r\n", invalid, badChar], ["second field, other value", "Host: x\r\nContent-Length: 5\r\nContent-Length: 6\r\n", duplicate, repeated], ["second field, non-digit", "Host: x\r\nContent-Length: 5\r\nContent-Length: x\r\n", duplicate, repeated], ["second field, empty", "Host: x\r\nContent-Length: 5\r\nContent-Length:\r\n", invalid, empty], @@ -866,11 +867,9 @@ describe("bad Content-Length fires clientError with node's code and reason", () }); }); -// Bun's limit is 2^59 - 1 (node's is 2^64 - 1). The second row reaches it with 19 bytes. -test.each([ - ["576460752303423487", "576460752303423488"], - ["0576460752303423487", "0576460752303423488"], -])("Content-Length %s is dispatched and %s overflows", async (atLimit, pastLimit) => { +// Bun's limits are 2^59 - 1 and 18 bytes. Node's limit is 2^64 - 1, with any number of leading zeros. +test("Content-Length 576460752303423487 is dispatched, and a larger or longer value overflows", async () => { + const atLimit = "576460752303423487"; const { promise, resolve, reject } = Promise.withResolvers(); await using server = createServer(req => resolve(req.headers["content-length"])); server.on("clientError", (err: any, socket) => { @@ -890,11 +889,13 @@ test.each([ socket.destroy(); } - expect(await clientErrorFor(`POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: ${pastLimit}\r\n\r\n`)).toMatchObject({ - dispatched: [], - code: "HPE_INVALID_CONTENT_LENGTH", - reason: "Content-Length overflow", - }); + for (const value of ["576460752303423488", "0576460752303423487"]) { + expect(await clientErrorFor(`POST /a HTTP/1.1\r\nHost: x\r\nContent-Length: ${value}\r\n\r\n`)).toMatchObject({ + dispatched: [], + code: "HPE_INVALID_CONTENT_LENGTH", + reason: "Content-Length overflow", + }); + } }); describe("clientError carries llhttp's reason where the parser error maps to one", () => {