Don't write Connection: close into an in-flight chunked response body - #32036
Don't write Connection: close into an in-flight chunked response body#32036robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the 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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds safety guards to the HTTP response code path to prevent header injection after the response body has begun writing. Changes protect both the C++ ChangesHTTP Response Header Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 7:32 PM PT - Jun 23rd, 2026
❌ @robobun, your commit 5cf190a has 5 failures in
🧪 To try this PR locally: bunx bun-pr 32036That installs a local version of the PR into your bun-32036 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Rebased onto main at be6a664 (sha 5cf190a). Conflict was additive in CI build 64380 finished with 281 green, 5 failed jobs; all in unrelated subsystems:
Both regression tests in this PR and the files touched by this diff are absent from every failure annotation (passed on every lane). The most recent main build (64400) also finished in state failed, so these are ambient rather than introduced here. Diff is ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/bun/http/serve-direct-readable-stream.test.ts (2)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the arbitrary timing wait.
The
sleep(10)here doesn't wait for a condition—it's an arbitrary delay between writes. The test validates thatleakedCtrl.write()throws afterpull()returns; that assertion doesn't depend on a 10ms delay. Per coding guidelines, "do not use setTimeout in tests; instead,awaitthe condition to be met."✂️ Remove the sleep
async pull(ctrl) { await ctrl.write("a"); - await sleep(10); await ctrl.write("b"); ctrl.flush();🤖 Prompt for 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. In `@test/js/bun/http/serve-direct-readable-stream.test.ts` at line 14, Remove the arbitrary await sleep(10) and instead rely on the actual sequence between pull() and leakedCtrl.write(); delete the call to sleep(10) and make the test assert immediately after pull() resolves that leakedCtrl.write() throws. Locate the test lines referencing sleep(10), leakedCtrl.write(), and the pull() implementation and ensure the test awaits the pull completion (or the promise it returns) before asserting the thrown error, rather than inserting a timed delay.Source: Coding guidelines
4-4: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer concurrent tests for independent server-spawn tests.
Both tests spawn independent servers on random ports (
port: 0), so they can run concurrently without interference. As per coding guidelines, prefertest.concurrentfor tests that spawn processes or servers when they don't share state.♻️ Mark tests concurrent
-test("HTTPResponseSink displays correct message", async () => { +test.concurrent("HTTPResponseSink displays correct message", async () => {-test("ending an HTTP/1.0 streaming response does not inject a Content-Length header", async () => { +test.concurrent("ending an HTTP/1.0 streaming response does not inject a Content-Length header", async () => {Also applies to: 36-36
🤖 Prompt for 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. In `@test/js/bun/http/serve-direct-readable-stream.test.ts` at line 4, Change the two independent server-spawning tests to run concurrently by replacing the synchronous Jest/Mocha test declarations with concurrent ones: update the test call for "HTTPResponseSink displays correct message" and the other test titled the same at the second occurrence to use test.concurrent (or the framework's equivalent) instead of test so each server-created test runs in parallel on random ports; ensure the test names and behavior remain unchanged and only the test function wrapper is modified.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/http/serve-direct-readable-stream.test.ts`:
- Line 14: Remove the arbitrary await sleep(10) and instead rely on the actual
sequence between pull() and leakedCtrl.write(); delete the call to sleep(10) and
make the test assert immediately after pull() resolves that leakedCtrl.write()
throws. Locate the test lines referencing sleep(10), leakedCtrl.write(), and the
pull() implementation and ensure the test awaits the pull completion (or the
promise it returns) before asserting the thrown error, rather than inserting a
timed delay.
- Line 4: Change the two independent server-spawning tests to run concurrently
by replacing the synchronous Jest/Mocha test declarations with concurrent ones:
update the test call for "HTTPResponseSink displays correct message" and the
other test titled the same at the second occurrence to use test.concurrent (or
the framework's equivalent) instead of test so each server-created test runs in
parallel on random ports; ensure the test names and behavior remain unchanged
and only the test function wrapper is modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80ee5c29-e3f0-4505-abf2-fc2f79c35e4c
📒 Files selected for processing (1)
test/js/bun/http/serve-direct-readable-stream.test.ts
|
Re the two outside-diff suggestions on
|
88334f7 to
29da399
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the guard mirrors the existing HTTP_WRITE_CALLED check already used for Connection: close in internalEnd — but since this touches wire-level response framing in the core HTTP server path, it's worth a human sign-off.
Extended reasoning...
Overview
This PR adds HTTP_WRITE_CALLED guards at two sites to prevent header bytes from being injected into an in-flight response body:
packages/bun-uws/src/HttpResponse.h(internalEnd): skip theContent-Length: <n>\r\n\r\nwrite oncewrite()has already terminated the header section and put body bytes on the wire (HTTP/1.0 / close-delimited streaming path).src/uws_sys/libuwsockets.cpp(uws_res_end_without_body, SSL + non-SSL): skip theConnection: closeheader and the bare header-terminating\r\nonce the body has started (theres.destroy()path).
Two regression tests cover both sites with raw-socket assertions on the exact bytes received.
Security risks
None identified. The change strictly removes writes in a state where they were corrupting the body; it does not add any new data to responses, change parsing, or touch auth/crypto/permissions.
Level of scrutiny
High. Although the diff is small and mechanically mirrors the identical guard already present a few lines above (HttpResponse.h:125 for Connection: close in internalEnd), this is the core HTTP response framing path that every Bun.serve / node:http response flows through. A subtle regression here — e.g. a caller of uws_res_end_without_body or the non-chunked internalEnd branch that relied on the old behavior in some edge case (sendfile, SSE, WebSocket upgrade, 204/304, content-length-already-set) — would have a wide blast radius. A maintainer familiar with all the call sites should confirm.
Other factors
- The root-cause analysis in the description is thorough and the fix matches Node's verified behavior (send nothing on destroy mid-body).
- The author ran the broader HTTP test suite (
node-http.test.ts,serve.test.ts,hspec.test.ts, etc.) with no new failures, and the flaky upstream test went 0/130 after. - No CODEOWNERS cover these files; no outstanding human review comments.
- The bug hunting system found no issues.
uws_res_end_without_body wrote a Connection: close header and a bare CRLF without checking HTTP_WRITE_CALLED. When a response was destroyed after chunked body data had been written (e.g. res.destroy() from the server captureRejections handler), those bytes were appended to the body stream. Clients parsed them as chunk framing: 'C' became a chunk size of 0xC, 'onnection: close' a chunk extension, and the trailing CRLF two bytes of body data, producing a spurious 'data' event. Guard both writes with HTTP_WRITE_CALLED, matching the guard in uWS::HttpResponse::internalEnd.
Same bug class as the previous commit, different site: internalEnd's content-length branch checked HTTP_END_CALLED but not HTTP_WRITE_CALLED. An HTTP/1.0 (fromAncientRequest) streaming response skips the chunked branch, so when the response sink ended the stream with leftover buffered data after raw body writes had begun, internalEnd injected 'Content-Length: <n>' plus a blank line into the body. Guard the content-length write with HTTP_WRITE_CALLED, like the bare CRLF arm next to it. Fixes #28019
Move it from http-server-chunking.test.ts to serve-direct-readable-stream.test.ts and drive the response sink directly: write one chunk at the highWaterMark so it is flushed to the socket immediately, then end the sink with a small chunk still buffered. That hits the end-with-leftover path on every run instead of depending on event-loop timing, and the previous host file also carried an unrelated test that binds localhost and cannot pass in IPv6-first environments.
29da399 to
5cf190a
Compare
There was a problem hiding this comment.
I didn't find any issues — the guard mirrors the existing HTTP_WRITE_CALLED check at HttpResponse.h:125 and the reasoning is solid — but this sits on the core HTTP response write path where the state-flag interactions are subtle, so worth a human look before merge.
Extended reasoning...
Overview
This PR adds an HTTP_WRITE_CALLED guard at two sites that previously could write header bytes after the response body had already started:
packages/bun-uws/src/HttpResponse.h—internalEnd()'s Content-Length branchsrc/uws_sys/libuwsockets.cpp—uws_res_end_without_body()(both SSL and non-SSL branches), for theConnection: closeheader and the header-terminating\r\n
Two regression tests are added that deterministically reproduce both injection sites and assert no stray bytes appear on the wire.
Security risks
None identified. The change is purely subtractive (suppresses header bytes that were being written at the wrong time); it does not introduce new parsing, allocation, or user-controlled input handling. If anything, it removes a wire-corruption vector that could confuse downstream proxies/clients.
Level of scrutiny
High. HttpResponse::internalEnd and uws_res_end_without_body sit on the hot path for every HTTP response Bun serves. The correctness of the change hinges on the interaction between several state flags (HTTP_WRITE_CALLED, HTTP_END_CALLED, HTTP_WROTE_CONTENT_LENGTH_HEADER, fromAncientRequest, closeDelimited, noBodyStatus). I traced the else-branch entry conditions and the new guard looks correct — once write() has terminated the header section, neither a Content-Length header nor a bare \r\n should be emitted, and the existing else if (!HTTP_WRITE_CALLED) already encodes the same invariant. The pattern also exactly matches the pre-existing guard for Connection: close at HttpResponse.h:125.
Other factors
The PR description is unusually thorough (wire-level capture, root-cause analysis, before/after flake counts, cross-reference to #28019), and the author re-verified the gate proof after rebase. The bug-hunting system found nothing. Still, given this is core HTTP server infrastructure, I'd prefer a maintainer sign-off rather than auto-approving.
|
The same import http from 'node:http';
import net from 'node:net';
const srv = http.createServer((q, r) => { r.writeHead(200); r.write('a'); setImmediate(() => { r.write('b'); r.end('c'); }); });
srv.listen(0, () => {
const s = net.connect(srv.address().port);
s.on('connect', () => s.write('GET / HTTP/1.0\r\nHost:a\r\n\r\n'));
let got = ''; s.on('data', d => got += d); s.on('close', () => console.log(JSON.stringify(got)));
});body comes back as I pushed a node:http regression test for this variant (and an equivalent HttpResponse.h fix that gates the whole header-terminator block on |
2 participants
What
test/js/node/test/parallel/test-http-server-capture-rejections.jsflakes in CI with:The client's
datalistener (common.mustCall, expected once for the single-byte body{) fires a second time with"\r\n".Root cause
Capturing the wire traffic of a failing run shows the server injecting header bytes into the response body:
When the test's async request handler throws after
res.write('{'), the servercaptureRejectionshandler callsres.destroy(), which reachesuws_res_end_without_bodyinsrc/uws_sys/libuwsockets.cpp. That shim wroteConnection: close\r\nplus a bare\r\ngated only onHTTP_CONNECTION_CLOSE/HTTP_END_CALLED, without checkingHTTP_WRITE_CALLED, so the bytes landed in the middle of an in-flight chunked body.The client's chunked decoder then parses
Cas a chunk size (0xC),onnection: closeas a chunk extension, and delivers the trailing\r\nas two bytes of body data: the seconddataevent.The flakiness is only in delivery: the stray bytes are written on every run, but the socket is destroyed immediately afterwards, so whether they get flushed before the connection dies depends on machine timing.
Second site, same class (#28019)
uWS::HttpResponse::internalEnd's content-length branch has the same missing guard: it checksHTTP_END_CALLEDbut notHTTP_WRITE_CALLED. An HTTP/1.0 (fromAncientRequest) streaming response skips the chunked branch, so when the response sink ends with leftover buffered data after raw body writes already started,internalEndinjectsContent-Length: <n>\r\n\r\ninto the body. This is the repro in #28019 (Content-Length: 83appears near the end of the body).Fix
Guard both sites with
HTTP_WRITE_CALLED, the same guardinternalEndalready uses for theConnection: closeheader and for the bare header-terminating CRLF (packages/bun-uws/src/HttpResponse.h:125,180): oncewrite()has been called, the header section is terminated and body bytes are on the wire, so there is nothing valid left to append. Node sends nothing in either situation (verified on v24.3.0).Likely also the mechanism behind proxy reports like #19789 (
ERR_INCOMPLETE_CHUNKED_ENCODINGbehind NGINX): injected header bytes corrupt the chunked framing mid-stream. That issue has no minimal repro, so it is not claimed as fixed here.Verification
Two new tests:
test/js/node/http/node-http-transfer-encoding.test.ts: drives a node:http server with a rawnetsocket, destroys the response after the first chunk is on the wire, and asserts no further bytes arrive. On the unfixed build it fails with:test/js/bun/http/serve-direct-readable-stream.test.ts: uses atype: "direct"stream to drive the response sink deterministically: one chunk at the highWaterMark (flushed to the socket immediately, so the body is started), thenend()with a small chunk still buffered, which is exactly the end-with-leftover path. On the unfixed build the client receivesContent-Length: 11\r\n\r\nspliced into the body; this fails on every run under the ASAN debug build, independent of event-loop timing.test-http-server-capture-rejections.js: 3 failures in 25 runs before, 0 in 130 runs aftercurl --http1.0against the issue's server): injected header before, byte-exact 1.1MB body afternode-http.test.ts,node-http-nested-cork.test.ts,node-http-backpressure.test.ts,bun-serve-file.test.ts,hspec.test.ts,serve.test.ts,http-server-chunking.test.ts: no new failures (remaining failures reproduce on clean main in this environment)Fixes #28019