[robobun]

Repro

import net from "node:net";
const CRLF = "\r\n";
const server = Bun.serve({ hostname: "127.0.0.1", port: 0, fetch: req => new Response("ok") });
const play = bytes => new Promise(res => {
  const out = []; const s = net.connect({ port: server.port, host: "127.0.0.1" });
  const fin = () => { s.destroy(); res(Buffer.concat(out).toString("latin1")); };
  s.on("connect", () => s.write(Buffer.from(bytes, "latin1")));
  s.on("data", d => out.push(d)); s.on("close", fin); s.on("error", fin);
});
const st = r => (r.match(/HTTP\/1\.[01] (\d{3})/g) || []).map(x => x.slice(-3)).join(",");
const A = st(await play(CRLF + `GET /a HTTP/1.1${CRLF}Host: x${CRLF}${CRLF}`));
const B = st(await play(`POST /a HTTP/1.1${CRLF}Host: x${CRLF}Content-Length: 2${CRLF}${CRLF}hi` + CRLF +
                        `GET /b HTTP/1.1${CRLF}Host: x${CRLF}Connection: close${CRLF}${CRLF}`));
console.log(JSON.stringify({ A, B })); server.stop(true);

bun (main):  {"A":"400","B":"200,400"}
node 26.3:   {"A":"200","B":"200,200"}

A node:http server on the same build already answers 200/200,200.

Cause

RFC 9112 2.2: "a server that is expecting to receive and parse a request-line SHOULD ignore at least one empty line (CRLF) received prior to the request-line." The RFC's motivating case is exactly case B: some clients emit an extra CRLF after a POST body, which then sits in front of the next request on the same keep-alive connection.

The leading-CR/LF skip in fenceAndConsumePostPadded (packages/bun-uws/src/HttpParser.h) is gated behind if constexpr (IsNodeHttp), so the node:http compat server tolerates a leading empty line but Bun.serve still answers 400 Bad Request + close.

Fix

Remove the IsNodeHttp gate so the existing skip applies to both server stacks. The skip consumes the CR/LF bytes into consumedTotal, so they are never buffered and the MAX_FALLBACK_SIZE accounting is unchanged. node:http behavior is identical (the skip was already running there).

Verification

• test/js/bun/http/bun-serve-leading-crlf.test.ts covers a single request with a leading CRLF / bare LF / multiple CRLFs and the pipelined POST+CRLF+GET case against Bun.serve. All four fail on an unfixed build (400) and pass with the fix.
• test/js/bun/http/http-spec.ts (h1spec against Bun.serve) gains three leading-empty-line cases.
• The existing serve.test.ts cases that asserted Bun.serve rejects a leading CRLF/LF/CR are inverted to assert 200 OK.
• test-http-blank-header.js, test-http-upgrade-server2.js, test-http-server-destroy-socket-on-client-error.js and the rest of the node:http compat suites are unaffected (the skip was already active for that path).

---

no test proof · iteration 6 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/serve.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c7e948b0-1e07-47e7-b96b-1f20897c77ae

📥 Commits

Reviewing files that changed from the base of the PR and between 59242d6 and edbb347.

📒 Files selected for processing (4)

• packages/bun-uws/src/HttpParser.h
• test/js/bun/http/bun-serve-leading-crlf.test.ts
• test/js/bun/http/http-spec.ts
• test/js/bun/http/serve.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 5:30 AM PT - Jul 29th, 2026}

❌ @robobun, your commit edbb347 has 3 failures in Build #84933 (All Failures):

• test/js/web/fetch/fetch-leak.test.ts - code 1 on 🍎 14 x64
• test/js/web/fetch/body-stream.test.ts - timeout on 🍎 14 x64
• step failed outside runner - exit 1 on 🐧 aarch64 - build-bun

---

🧪   To try this PR locally:

bunx bun-pr 33634

That installs a local version of the PR into your bun-33634 executable, so you can run:

bun-33634 --bun

[claude]

I didn't find any issues — the skip is bounded by end, the short-read path preserves fragmented-input buffering, and consumed-byte accounting is unchanged — but since this loosens what the HTTP request-line parser accepts, it's worth a human sanity-check for request-smuggling implications.

Extended reasoning...

Overview

This PR adds an 8-line prelude to uWS::HttpParser::consumeRequestLine in packages/bun-uws/src/HttpParser.h that skips any run of \r/\n before the method token, returning a short read if the run consumes all available input. It adds three cases to the h1spec harness (test/js/bun/http/http-spec.ts) and a new describe block in test/js/node/http/node-http.test.ts covering leading CRLF/LF, the RFC's pipelined POST+CRLF+GET motivating case, and a CRLF arriving in its own TCP write.

Security risks

The change relaxes what the HTTP/1.1 request parser accepts on the wire. That is the exact surface where request-smuggling bugs live, so even a small, RFC-sanctioned relaxation deserves human eyes. I traced the specific concerns and they look fine to me: the skip loop is bounded by end (no overrun into the post-padding); an all-CRLF packet returns shortRead() and lands in the fallback buffer, which is capped at MAX_FALLBACK_SIZE (so no unbounded buffering); start in getHeaders is the original buffer pointer, so skipped bytes are counted in consumed and still hit the consumed > MAX_FALLBACK_SIZE → 431 guard; and the skip only runs at the start of request-line parsing, not inside the header loop, so it can't be used to smuggle a header terminator. It matches Node/llhttp behavior, which is the compatibility target.

Level of scrutiny

High. HttpParser.h is the shared parser under both Bun.serve and node:http, so every HTTP request Bun serves goes through this function. Changes to acceptance criteria in an HTTP parser are the textbook place where subtle framing bugs turn into security issues, and per the approval guidelines this falls under "touches security-sensitive code" and "critical code paths" — I'm deferring on that basis rather than because I found a defect.

Other factors

No bugs surfaced from the multi-agent review. Test coverage is solid (both server stacks, fragmented input, the pipelining case that motivates the RFC clause). The PR description is thorough and cites RFC 9112 §2.2 correctly. There are no prior human review comments to address.

[robobun]

On the request-smuggling question: the skip only runs where the parser is expecting the start of a request-line, i.e. at connection open or immediately after the previous message's framing (Content-Length bytes or the terminal chunked 0\r\n\r\n) has been fully consumed. It cannot fire inside a header block or a body, so it does not change how message boundaries are determined. An upstream proxy that forwards a stray CRLF between framed messages already sees two messages; this change makes Bun agree with that count instead of erroring on the second one. This is the behavior RFC 9112 2.2 recommends and what Node/llhttp, nginx, and Go net/http do.

The skip is bounded by end and the skipped bytes count toward the existing MAX_FALLBACK_SIZE guard (consumed is measured from the original buffer start in getHeaders), so a CRLF-only stream cannot grow the fallback past the header-size limit.

[claude]

I didn't find any bugs — the skip is bounded, consumed-byte accounting stays correct, and the smuggling analysis holds up — but since this loosens what the HTTP request-line parser accepts, it's worth a human sign-off.

Extended reasoning...

Overview

Eight added lines in uWS::HttpParser::consumeRequestLine (packages/bun-uws/src/HttpParser.h) skip any leading run of \r/\n before the method scan, per RFC 9112 §2.2 and matching Node/llhttp. Three new h1spec cases in test/js/bun/http/http-spec.ts and a new test/js/node/http/node-http-leading-crlf.test.ts covering single/multiple/bare-LF prefixes, the pipelined POST-then-CRLF-then-GET motivating case, and a CRLF arriving in its own TCP write.

Security risks

This is the HTTP request-line parser — the request-smuggling attack surface. The change loosens input acceptance. I traced the concerns:

• The skip is bounded by end, so no OOB read; the post-padding sentinel is never touched by this loop.
• start in getHeaders is captured before consumeRequestLine, so skipped bytes are counted as consumed and the fallback/MAX_FALLBACK_SIZE accounting is unchanged; a CRLF-only flood still hits the 431 cap.
• The skip only executes where the parser is already expecting a new request-line (connection open or immediately after the prior message's framing is fully consumed), so it does not shift message boundaries relative to an upstream proxy — it aligns Bun with llhttp/nginx/Go, which already do this.

I did not find a smuggling or memory-safety issue, but loosening a parser on the smuggling boundary is exactly the class of change a maintainer should confirm.

Level of scrutiny

High. Vendored uWS HTTP parser, hot path for every Bun.serve and node:http request, and directly security-relevant. Not a mechanical or config change.

Other factors

Test coverage is good (both server stacks, fragmented input, the RFC's motivating pipelined case). No prior human review on the PR. The author's follow-up comment proactively addresses the smuggling question, which is reassuring but also flags that the author recognizes this is security-adjacent.

[claude]

Thanks for addressing the test-comment nit. I didn't find any bugs in this revision, but since this loosens what the HTTP request-line parser accepts (and adds a second behavior change in the case -1 short-target path), it's worth a human sign-off given the request-smuggling surface.

Extended reasoning...

Overview

Two changes to uWS::HttpParser::consumeRequestLine in packages/bun-uws/src/HttpParser.h:

1. A bounded skip of leading \r/\n bytes before the method scan, returning shortRead() if the skip consumes all input. This implements RFC 9112 §2.2 and matches Node/llhttp.
2. A refinement of the isHTTPorHTTPSPrefixForProxies case -1 branch: instead of unconditionally short-reading when fewer than 8 target bytes are available, it now checks whether the bytes seen so far are still a valid prefix of http:// or https:// and errors immediately if not. This was added so trailing garbage like "\r\nhello world" after a request errors instead of buffering until timeout.

Tests: three new h1spec cases in test/js/bun/http/http-spec.ts and a new test/js/node/http/node-http-leading-crlf.test.ts covering single/multiple CRLF, bare LF, the pipelined POST+CRLF+GET case, invalid-after-CRLF, and split writes.

Security risks

HTTP request-line parsing is the canonical request-smuggling attack surface. The PR's analysis is sound as far as I can trace: the skip fires only where the parser is already expecting a request-line (connection start or after prior message framing is fully consumed), it is bounded by end, skipped bytes count toward MAX_FALLBACK_SIZE via getHeaders' start-relative accounting, and start for the method view is captured after the skip. The second change tightens rather than loosens behavior. I did not find a smuggling or DoS vector, but any relaxation of what an HTTP parser accepts warrants a maintainer's eyes.

Level of scrutiny

High. This is the production HTTP/1.1 parser shared by Bun.serve and node:http; every request flows through consumeRequestLine. The change is small and RFC-aligned, but the file is exactly the kind of code where subtle interactions with the fallback buffer, post-padding sentinels, and pipelining matter.

Other factors

• My earlier nit (test comment overstating packet fragmentation) was addressed in c2341fc — the comment is softened and a setImmediate yield is inserted; that thread is resolved.
• The bug-hunting pass on this revision found nothing.
• The case -1 prefix-check is a second behavior change riding on this PR (commit c2341fc) that isn't in the title; it looks correct (only tightens rejection of inputs that could never become valid) but is one more reason for a human to review the whole diff.

[robobun]

CI status for c2341fc: the diff is green on every lane that ran tests (all Linux ×3 distros ×3 archs including x64-asan, Windows 2019 x64/x64-baseline, Windows 11 aarch64). The three red jobs are unrelated:

• 2× darwin 26 aarch64 - test-bun: buildkite-agent artifact download timed out after 120s before any test ran. Same infra failure on builds 69793, 69801 and 69834, so the agent is unhealthy independent of this branch.
• 1× windows 11 aarch64 - test-bun shard: segfault in test/js/node/test/sequential/test-net-listen-shared-ports.js, a node:cluster + net.createServer test that does not touch the HTTP parser. The other shards of the same lane passed.

node-http-leading-crlf.test.ts, hspec.test.ts, and the upstream test-http-blank-header.js all pass on the lanes that ran. Ready for review.

[claude]

No issues found, but this reverses a previously-deliberate Bun.serve default (the removed comment explicitly said "Node-compat only: Bun.serve keeps rejecting…") in the HTTP request-line parser, so a maintainer should confirm the direction.

What was reviewed:

• The parser change is a pure de-gate of the existing IsNodeHttp skip; the loop, consumedTotal accounting, and length == 0 → break short-read path are byte-identical to what already runs for node:http.
• Request-smuggling: the skip only fires at the top of the for(;length;) loop when the parser is expecting a request-line (connection open or after the prior message's Content-Length/chunked framing is fully consumed) — it cannot fire mid-headers or mid-body, so message-boundary determination is unchanged.
• The inverted serve.test.ts cases now send Connection: close so the socket closes deterministically; the new test file wires error/close to resolve and asserts exact {events, statuses}.

Extended reasoning...

Overview

The one-line functional change removes the if constexpr (IsNodeHttp) gate around the leading-CR/LF skip in fenceAndConsumePostPadded (packages/bun-uws/src/HttpParser.h), so Bun.serve now ignores empty lines before the request-line the same way the node:http compat server already did. Test changes: a new bun-serve-leading-crlf.test.ts (single-request CRLF/LF/multi-CRLF prefixes + the pipelined POST-body-trailing-CRLF case), three new http-spec.ts h1spec entries, and the three serve.test.ts cases that previously asserted 400 inverted to assert 200 OK.

Security risks

HTTP request-line parsing is request-smuggling-adjacent. I verified the skip runs only where the parser is between messages (top of the for(;length;) loop, before getHeaders), so it cannot alter framing inside a header block or body. The skipped bytes are counted into consumedTotal, so a CRLF-only stream is consumed rather than buffered and cannot grow fallback past maxFallbackSize. This matches RFC 9112 §2.2's SHOULD and what Node/llhttp, nginx, and Go net/http do. I did not find a smuggling or DoS vector, but HTTP parser leniency changes are the canonical place where subtle desync bugs hide, and the removed comment shows the prior strictness was a deliberate Bun-native choice — a maintainer should sign off on relaxing it.

Level of scrutiny

High. Per REVIEW.md, changes to Bun-native defaults and security-sensitive parsing paths get maintainer sign-off even when the diff is mechanically small. The code path itself is not new (it already ships for node:http), which reduces risk, but the policy change — Bun.serve accepting input it previously rejected — is a product decision.

Other factors

My earlier nit on the split-write test comment was addressed (thread resolved). CI is green on every lane that executed tests per the author's summary; the three red jobs are infra timeouts and an unrelated node:cluster segfault. Test coverage is thorough: single-request prefixes, the RFC's motivating keep-alive case, h1spec entries, and the inverted regression tests. The new tests use port: 0, wire error/close to the awaited promise, and assert exact event sequences and status arrays.

[robobun]

On the policy direction: the prior IsNodeHttp-only gating was revisited at @Jarred-Sumner's request after re-testing the RFC 9112 2.2 repro against Bun.serve on current main. The motivating case (a stray CRLF after a POST body killing the next keep-alive request with a 400) is an interop break for Bun.serve specifically, not just a Node-compat concern, so the skip is un-gated here. The code path itself is unchanged from what already ships for node:http.

[robobun]

CI status for edbb347 (build 84933, complete): green on every lane that ran tests (alpine aarch64/x64, debian aarch64/x64/x64-asan, ubuntu aarch64/x64, darwin 26 aarch64, windows 2019 x64, windows 11 aarch64; 192 jobs passed). Transient build-bun timeouts on linux-aarch64/linux-x64/windows-x64 were auto-retried and passed.

The darwin 14 queue is unhealthy independent of this branch: most shards expired without an agent, one shard failed at git clone (exit 128, dirty checkout dir on the agent), and one shard hit the step timeout with test/js/web/fetch/fetch-leak.test.ts (URLSearchParams leak timeout + fetch({compress}) 33 MB vs a 32 MB threshold) and test/js/web/fetch/body-stream.test.ts red. Both are client-side fetch() leak/stream tests; this change only touches the server-side request-line CR/LF skip and does not affect fetch(), compression, or memory allocation. The same tests pass on x64-asan and every other platform.

Ready for review.