Bun.serve: reject Transfer-Encoding on an HTTP/1.0 request (RFC 9112 6.1) - #35864
Conversation
WalkthroughChangesHTTP Transfer-Encoding handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Independently reproduced and landed on the same fix location (branch |
|
Updated 5:05 AM PT - Jul 29th, 2026
❌ @robobun, your commit ba20a7e has some failures in 🧪 To try this PR locally: bunx bun-pr 35864That installs a local version of the PR into your bun-35864 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bun-uws/src/HttpParser.h`:
- Around line 964-969: Add a regression test covering pipelined requests on one
connection: send an HTTP/1.0 keep-alive request followed in the same write by a
chunked HTTP/1.1 request, then assert the second request is dispatched and its
chunked body decodes correctly. Place the test alongside the existing HTTP
parser/connection tests and ensure it would fail if isAncientHTTP remains stale
between requests.
In `@test/js/bun/http/request-smuggling.test.ts`:
- Line 190: Strengthen the response assertions in
test/js/bun/http/request-smuggling.test.ts at lines 190, 217, and 249: replace
substring containment checks with assertions that the response starts with the
exact status lines “HTTP/1.1 400 Bad Request\r\n”, “HTTP/1.1 200 OK\r\n”, and
“HTTP/1.1 200 OK\r\n” respectively.
🪄 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: 59a31107-ee7e-4c82-a309-2c96069fc922
📒 Files selected for processing (2)
packages/bun-uws/src/HttpParser.htest/js/bun/http/request-smuggling.test.ts
There was a problem hiding this comment.
Both of my earlier findings (per-request ancientHttp reset, node:http test try/finally) are addressed in 8ad1bfb, and the toStartWith tightening in 1561d2d looks right. I didn't find anything new, but this edits the core uWS HTTP request parser on a smuggling-relevant path, so worth a human look.
What was reviewed:
- The new
!IsNodeHttp && ancientHttp && transferEncoding.hasguard — strictly tightens (only rejects more), placed alongside the sibling TE checks, node:http path unchanged. - The unconditional
isAncientHTTP = requestLineResult.isAncientHTTPwrite — only reached after a successful request-line parse (short-read/error return earlier), and its only other consumer (the Host-required check) becomes correctly stricter for a pipelined 1.1-after-1.0 request. - Tests: 400 + handler-not-called for both HTTP/1.0 variants, HTTP/1.1 control still 200 with decoded body, node:http parity preserved, server cleanup on all paths.
Extended reasoning...
Overview
Two-file change: packages/bun-uws/src/HttpParser.h adds a guard in fenceAndConsumePostPadded that rejects any HTTP/1.0 request carrying a Transfer-Encoding header (Bun.serve only, via !IsNodeHttp), and changes getHeaders to write isAncientHTTP unconditionally instead of only latching it to true. test/js/bun/http/request-smuggling.test.ts adds four tests (two 1.0 rejection variants, an HTTP/1.1 control, and a node:http llhttp-parity check).
Security risks
This is a request-smuggling hardening in the HTTP/1 wire parser. The new guard is monotonically stricter for Bun.serve — it can only cause additional 400s, never accept a request that was previously rejected — and the RFC 9112 6.1 citation is accurate. The node:http path is explicitly excluded so llhttp parity is preserved (verified by the new test). The unconditional isAncientHTTP write fixes a latent staleness I flagged in the first pass; its only pre-existing in-loop consumer was the Host-required check, which now correctly requires Host on a pipelined HTTP/1.1 request following an HTTP/1.0 one (previously it was wrongly exempt). No new attack surface is introduced.
Level of scrutiny
High. packages/bun-uws/src/HttpParser.h is the vendored uWS HTTP/1 request parser that fronts every Bun.serve connection, and the change sits in the Transfer-Encoding / framing validation block that exists specifically to prevent request smuggling. Even though the change is small and strictly-tightening, edits here warrant a maintainer's eyes.
Other factors
All prior review threads are resolved: my two findings were fixed in 8ad1bfb, CodeRabbit's toStartWith request was applied in 1561d2d, and CodeRabbit withdrew its pipelined-regression-test request after the author explained it would conflict with #33878. The gate evidence in the PR body shows the new tests fail on main and pass on the PR under both ASAN-debug and release. I confirmed ancientHttp/isAncientHTTP has no consumers outside HttpParser.h, and the unconditional write is only reached after consumeRequestLine succeeds (error/short-read paths return before it), so it never clobbers state with an indeterminate value.
|
CI build 84928 (rebased head ba20a7e) finished: 194 jobs passed, 2 expired, 0 test failures. Ready for review. |
There was a problem hiding this comment.
Both earlier findings (stale ancientHttp across pipelined iterations, and the node:http test's server leak on the reject path) are addressed in 8ad1bfb, and the toStartWith tightening from CodeRabbit is in. No new issues found — deferring only because this is the HTTP/1 parser's request-smuggling gate and the !IsNodeHttp scoping is a Node-compat policy call worth a maintainer nod.
What was reviewed:
getHeadersnow writesisAncientHTTPunconditionally on the success path; short-read/error paths return before the TE check consumes it, so no stale read remains.- New rejection is scoped to
!IsNodeHttpand gated ontransferEncoding.has(any TE, not just chunked) — matches RFC 9112 6.1's "containing a Transfer-Encoding header field". - Control test confirms HTTP/1.1 chunked still 200s; node:http parity test wrapped in try/finally.
Extended reasoning...
Overview
Two-file change: a 3-line rejection in packages/bun-uws/src/HttpParser.h (fenceAndConsumePostPadded) that 400s any HTTP/1.0 request carrying a Transfer-Encoding header for Bun.serve, plus a supporting fix making getHeaders write isAncientHTTP unconditionally (previously latched true). Four new tests in request-smuggling.test.ts: two rejection variants (default-close and Connection: keep-alive), an HTTP/1.1 positive control, and a node:http llhttp-parity test.
Security risks
This is a security hardening — it narrows accepted input per an RFC 9112 6.1 MUST, closing a proxy/backend framing-disagreement vector. The only regression risk is over-rejection. I checked: the gate is !IsNodeHttp && req->ancientHttp && transferEncoding.has, so (a) node:http keeps llhttp's dispatch-then-close behaviour, (b) only genuine HTTP/1.0 request-lines trigger it, and (c) after 8ad1bfb the per-request ancientHttp is written fresh on every getHeaders success, so a pipelined HTTP/1.1 request behind an HTTP/1.0 one no longer inherits a stale true. On the short-read/error branches getHeaders returns before the caller reaches the TE check, so the unwritten out-param there is harmless.
Level of scrutiny
High — this is the HTTP/1 request parser's smuggling-defence block, and the change alters which requests reach user handlers. That said, the diff is ~10 effective lines, follows the exact pattern of the adjacent TE+CL / chunked-not-last rejections, cites the governing RFC clause, and is covered by tests that fail on main and pass on the branch (per the gate evidence in the PR body).
Other factors
All prior review threads are resolved: my two inline findings from the first pass were fixed in 8ad1bfb, and CodeRabbit's toStartWith tightening landed in 1561d2d (its pipelined-regression-test request was withdrawn with a reasonable justification about #33878). CI didn't run tests due to a fleet-wide build-cpp scheduling stall unrelated to this diff. I'm deferring rather than approving because the !IsNodeHttp scoping (Bun.serve rejects, node:http dispatches-then-closes) is a Node-compat policy decision a maintainer should confirm, and HTTP parser changes in the smuggling path warrant a human sign-off even when they look correct.
…6.1) A POST /x HTTP/1.0 request with Transfer-Encoding: chunked was accepted, the body decoded as chunked, and the handler invoked. RFC 9112 6.1 says 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 it. Bun.serve now rejects such a request with 400 Bad Request before dispatch, consistent with the existing strict rejections for TE+CL conflicts and non-chunked TE values. node:http is unchanged: llhttp dispatches the request and the HTTP/1.0 connection already closes after.
…e:http test server getHeaders previously only set isAncientHTTP to true, never back to false, so a pipelined HTTP/1.1 request in the same recv buffer after an HTTP/1.0 one saw a stale ancientHttp and the new Transfer-Encoding check would wrongly 400 it. getHeaders now writes the flag unconditionally. The node:http parity test now closes its server in a finally so a rejected client promise cannot leak the listening server.
dd9f4e2 to
ba20a7e
Compare
|
Rebased onto main (ba20a7e, was 32 commits behind) and dropped the empty retrigger commit. |
There was a problem hiding this comment.
No issues found — both earlier review points (per-request ancientHttp reset, try/finally around the node:http test server) are addressed and the assertions were tightened to toStartWith. Deferring to a human because this is a change to the HTTP/1.x request parser's framing validation.
What was reviewed:
- The new
!IsNodeHttp && req->ancientHttp && transferEncoding.hasrejection sits alongside the existing TE validity checks and only tightens validation (fail-closed). isAncientHTTPis now written unconditionally afterconsumeRequestLinesucceeds; short-read paths return before the assignment, and eachconsumePostPaddedcall constructs a freshHttpRequest, so no stale state persists across recv boundaries.- Tests:
await using/ try-finally cleanup, error wired to reject, close-buffered response,handlerCallednegative assertion, HTTP/1.1 control case, node:http parity.
Extended reasoning...
Overview
Two-file change: (1) packages/bun-uws/src/HttpParser.h gains a ~3-line check rejecting any HTTP/1.0 request that carries a Transfer-Encoding header (RFC 9112 §6.1) for the Bun.serve personality, plus a change to write isAncientHTTP unconditionally in getHeaders so a pipelined HTTP/1.1 request in the same recv buffer after an HTTP/1.0 one is not mis-classified; (2) four new tests in test/js/bun/http/request-smuggling.test.ts covering the 400 rejection (both default-close and Connection: keep-alive), an HTTP/1.1 control, and node:http parity with llhttp.
Security risks
This is request-smuggling hardening. The change is purely additive on the rejection side — it introduces a new 400 path for a spec-invalid combination and cannot loosen framing. The only theoretical risk was a false-positive rejection of a valid HTTP/1.1 request via a stale ancientHttp flag, which was raised in the first review pass and fixed by making getHeaders write the flag unconditionally. I re-traced the short-read paths: on request-line short-read getHeaders returns before the assignment, and on header short-read the flag is written correctly then re-derived on the retry from a fresh stack-allocated HttpRequest, so no stale state crosses recv calls. The node:http path (IsNodeHttp) is deliberately excluded to preserve llhttp parity, and the HTTP/1.0 request already sets nodeHttpSawConnectionClose / HTTP_CONNECTION_CLOSE so the connection closes afterward.
Level of scrutiny
High — HttpParser.h is the wire-level framing validator for every Bun.serve connection, and this file's history is a sequence of request-smuggling fixes. Even though the diff is small and fail-closed, changes here warrant a maintainer's eyes.
Other factors
Both of my earlier inline findings were addressed in 5befe25 (unconditional isAncientHTTP write; try/finally around the node:http server). CodeRabbit's toStartWith tightening was applied in ba20a7e; its pipelined-regression request was declined with a stated reason (would conflict with #33878 and doesn't fail on main). The mechgate evidence in the PR description shows the two rejection tests fail on main and pass with the fix; the release-without-fix run shows the HTTP/1.1 control and node:http parity tests pass on main, so they are true controls. CI on the previous build was green on every lane that received a binary. No bugs surfaced in this run's bug-hunt.
2 participants
Repro
Before:
HTTP/1.1 200 OK, handler invoked, body"hello"decoded from the chunked framing.RFC 9112 6.1:
An HTTP/1.0 sender by definition does not implement Transfer-Encoding, so a proxy and backend that disagree on whether to honour it on a 1.0 message disagree on where the body ends.
Cause
fenceAndConsumePostPaddedinpackages/bun-uws/src/HttpParser.hvalidatesTransfer-Encoding(TE+CL conflict, chunked-not-last, duplicate chunked) but never consultsreq->ancientHttp, so an HTTP/1.0 request withTransfer-Encoding: chunkedpasses every check and is parsed as chunked.Fix
For
Bun.serve(non-IsNodeHttp), returnHTTP_PARSER_ERROR_INVALID_TRANSFER_ENCODING(400) whenreq->ancientHttp && transferEncoding.has, checked alongside the existing TE validity checks.node:httpkeeps llhttp's behaviour (dispatch and close): Node v26.3.0 dispatches such a request withhttpVersion === "1.0"and the decoded body, and the HTTP/1.0 request already marks the connection for close viaisAncient.Verification
New tests in
test/js/bun/http/request-smuggling.test.ts:rejects Transfer-Encoding on an HTTP/1.0 request(default closeandConnection: keep-alivevariants): 400, handler never called. Both return 200 on main.accepts Transfer-Encoding: chunked on an HTTP/1.1 request: control, still 200 with body"hello".node:http dispatches Transfer-Encoding on an HTTP/1.0 request (llhttp parity): unchanged, matches Node.Related: #33878 (non-persistent pipelined dispatch), #35295 (TE lists), #33633 (DEL in header values). None of those reject the HTTP/1.0 + TE case.
[review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 6
evidence per changed file