Skip to content

node:http: match body framing to Transfer-Encoding on HTTP/1.0 responses - #33872

Closed
robobun wants to merge 2 commits into
mainfrom
claude/farm/2664c353/http10-transfer-encoding-framing
Closed

robobun wants to merge 2 commits into
mainfrom
claude/farm/2664c353/http10-transfer-encoding-framing

Conversation

@robobun

@robobun robobun commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes #19789

Fixes node:http server responses to HTTP/1.0 clients where the head advertised Transfer-Encoding: chunked but the body bytes were written as identity, desyncing any client that honours the header.

Reproduction

import http from "node:http";
import net from "node:net";

const raw10 = (port, path) => new Promise((res) => {
  const c = [];
  const s = net.connect(port, "127.0.0.1", () => s.write(`GET ${path} HTTP/1.0\r\nHost: h\r\n\r\n`));
  s.on("data", (d) => c.push(d));
  s.on("close", () => {
    const a = Buffer.concat(c).toString("latin1"), i = a.indexOf("\r\n\r\n");
    res({ head: a.slice(0, i), body: a.slice(i + 4) });
  });
});

const srv = http.createServer((q, r) => {
  if (q.url === "/rmcl") { r.setHeader("Content-Length", 5); r.removeHeader("Content-Length"); r.end("hello"); }
  else { r.writeHead(200, { "Transfer-Encoding": "chunked" }); r.end("hello"); }
});
srv.listen(0, "127.0.0.1", async () => {
  const p = srv.address().port;
  console.log(await raw10(p, "/rmcl"));
  console.log(await raw10(p, "/whte"));
  srv.close();
});

Before this PR, /rmcl carried Transfer-Encoding: chunked in the head (Bun invented it) with identity body bytes "hello", and /whte carried the user's Transfer-Encoding: chunked header with identity body bytes "hello". A real HTTP/1.0 client fails hard:

$ curl --http1.0 http://127.0.0.1:<port>/whte
curl: (56) chunk hex-length char not a hex digit: 0x68

Node.js close-delimits /rmcl (no Transfer-Encoding) and chunk-frames /whte as 5\r\nhello\r\n0\r\n\r\n.

Cause

Two owners decide head and body independently and never look at each other:

  • renderNativeHeaders in src/js/node/_http_server.ts falls through to forceChunked when only Content-Length was removed, without checking useChunkedEncodingByDefault (set to false for HTTP/1.0 in the ServerResponse constructor). Node's _storeHeader takes the !useChunkedEncodingByDefault close-delimited branch before it ever reaches the auto-chunked path.
  • NodeHTTPServer__writeHead forwards a user-set Transfer-Encoding: chunked header to the wire, but the uWS body writer gates every chunk-framing path on !fromAncientRequest, so the body is written as identity while the head says chunked.

Fix

  • renderNativeHeaders: when the user removed Content-Length and useChunkedEncodingByDefault is false, close-delimit the response instead of forcing chunked.
  • NodeHTTP.cpp: when a Transfer-Encoding header whose value contains the chunked token is written, clear fromAncientRequest so the uWS writer chunk-frames the body to match the head (like Node). Other TE values (gzip, identity) leave the gate intact and the body stays raw.

Verification

test/js/node/http/node-http-transfer-encoding.test.ts adds five HTTP/1.0 cases asserting the exact raw head and body bytes against Node: removeHeader('Content-Length'), explicit Transfer-Encoding: chunked via setHeader / writeHead / streaming write, and a Transfer-Encoding: gzip regression guard. Four fail on the released binary and all pass after; the first two vendored test-http-1.0*.js tests and test-http-remove-header-stays-removed.js are unchanged.

This is the HTTP/1.0 counterpart to #33871 (HTTP/1.1 TE value vs body framing); different code path (fromAncientRequest gate vs Content-Length/TE-value conflict).


[review] gate passed · iteration 1 · 3 files touched

fails on main (without fix)
ASAN without fix: 4 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-transfer-encoding.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (677db270c)

test/js/node/http/node-http-transfer-encoding.test.ts:
(pass) should not duplicate transfer-encoding header in request [742.17ms]
(pass) should not duplicate transfer-encoding header in response when explicitly set [421.95ms]
125 |     const { port } = server.address() as AddressInfo;
126 | 
127 |     const { head, body } = await rawHTTP10(port, "/");
128 |     // Node.js never auto-adds Transfer-Encoding here (useChunkedEncodingByDefault
129 |     // is false for HTTP/1.0); the response is close-delimited instead.
130 |     expect(head.toLowerCase()).not.toContain("transfer-encoding");
                                         ^
error: expect(received).not.toContain(expected)

Expected to not contain:
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (81fa5debf)

test/js/node/http/node-http-transfer-encoding.test.ts:
(pass) should not duplicate transfer-encoding header in request [13.33ms]
(pass) should not duplicate transfer-encoding header in response when explicitly set [8.80ms]
(pass) HTTP/1.0 response framing matches the advertised headers > removing Content-Length does not invent Transfer-Encoding: chunked [15.37ms]
(pass) HTTP/1.0 response framing matches the advertised headers > an explicit Transfer-Encoding: chunked via writeHead chunk-frames the one-shot body [15.93ms]
(pass) HTTP/1.0 response framing matches the advertised headers > an explicit Transfer-Encoding: chunked via setHeader chunk-frames the one-shot body [16.05ms]
(pass) HTTP/1.0 response framing matches the advertised headers > an explicit Transfer-Encoding: chunked chunk-frames streaming writes [17.74ms]
(pass) HTTP/1.0 response framing matches the advertised headers > a Transfer-Encoding value without 'chunked' does not chunk-frame the body [17.13ms]

 7 pass
 0 fail
 15 expect() calls
Ran 7 tests across 1 file. [260.00ms]
__F:0:S:0
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/http/node-http-transfer-encoding.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (677db270c)

test/js/node/http/node-http-transfer-encoding.test.ts:
(pass) should not duplicate transfer-encoding header in request [739.73ms]
(pass) should not duplicate transfer-encoding header in response when explicitly set [431.13ms]
(pass) HTTP/1.0 response framing matches the advertised headers > removing Content-Length does not invent Transfer-Encoding: chunked [126.01ms]
(pass) HTTP/1.0 response framing matches the advertised headers > an explicit Transfer-Encoding: chunked via writeHead chunk-frames the one-shot body [109.50ms]
(pass) HTTP/1.0 response framing matches the advertised headers > an explicit Transfer-Encoding: chunked via setHeader chunk-frames the one-shot body [69.51ms]
(pass) HTTP/1.0 res
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
[configured] bun-profile → bun (stripped) in 718ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/21] gen cpp.rs (cppbind)
[2/21] gen JS modules (bundle-modules)
Preprocess modules (6263ms)
Bundle modules (42ms)
Postprocesss modules (21ms)
Bundle Functions (569ms)
Generate Code (72ms)

[6.98s] Bundled "src/js" for production
  1889 kb
  161 internal modules
  12 native modules
  90 internal functions across 19 files
[2/8] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: component rust-std is up to date

  nightly-2026-05-06-x86_64-unknown-linux-gnu unchanged - rustc 1.97.0-nightly (e95e73209 2026-05-05)

info: checking for s
... (truncated)
diff hotspot
src/js/node/_http_server.ts                        | 10 ++-
 src/jsc/bindings/NodeHTTP.cpp                      | 26 +++++++
 .../node/http/node-http-transfer-encoding.test.ts  | 87 +++++++++++++++++++++-
 3 files changed, 121 insertions(+), 2 deletions(-)

gate history · 2 passed · 0 rejected · iteration 1

evidence per changed file
file                                                   reads  edits  tests
src/js/node/_http_server.ts                                3      1      0
src/jsc/bindings/NodeHTTP.cpp                              3      3      0
test/js/node/http/node-http-transfer-encoding.test.ts      1      4      0

The ServerResponse header renderer pushed Transfer-Encoding: chunked for
HTTP/1.0 requests while the uWS body writer never chunk-frames an HTTP/1.0
(fromAncientRequest) response, so the head and body disagreed. curl --http1.0
died with 'curl: (56) chunk hex-length char not a hex digit'.

Two cases:

- removeHeader('Content-Length') fell through to forceChunked without
  consulting useChunkedEncodingByDefault (false on HTTP/1.0). Node close-
  delimits here; now we do too.

- An explicit Transfer-Encoding: chunked header was written to the wire but
  the uWS writer's fromAncientRequest gate suppressed chunk framing. Clear
  that gate when the user's TE value names 'chunked' so the body matches
  the head (like Node). Other TE values (gzip) stay raw.
@github-actions github-actions Bot added the claude label Jul 9, 2026
@robobun

robobun commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:52 PM PT - Jul 9th, 2026

❌ @robobun, your commit 677db27 has 3 failures in Build #71206 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 33872

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

bun-33872 --bun

@coderabbitai

coderabbitai Bot commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

HTTP/1.0 framing

Layer / File(s) Summary
Default response framing
src/js/node/_http_server.ts
Responses without Content-Length use close-delimited framing when chunking is disabled, otherwise forcing chunked encoding.
Native chunked-header detection
src/jsc/bindings/NodeHTTP.cpp
Detects standalone chunked tokens and updates both native response-header paths to clear ancient-request framing when explicitly requested.
Raw HTTP/1.0 framing validation
test/js/node/http/node-http-transfer-encoding.test.ts
Adds raw HTTP/1.0 response parsing and tests close-delimited, chunked, streaming, and non-chunked transfer encoding cases.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main fix: HTTP/1.0 body framing now matches Transfer-Encoding.
Description check ✅ Passed The description covers the PR purpose, cause, fix, reproduction, and verification, which is mostly complete for the template.

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

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. ERR_INCOMPLETE_CHUNKED_ENCODING with Next.js app using Bun in Docker behind NGINX proxy #19789 - NGINX defaults to HTTP/1.0 for proxied requests, triggering the exact forceChunked = true bug that causes ERR_INCOMPLETE_CHUNKED_ENCODING and duplicate Transfer-Encoding: chunked headers

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #19789

🤖 Generated with Claude Code

@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Linked #19789: NGINX's proxy_pass defaults to HTTP/1.0, and the proxy_http_version 1.1; workaround reported there confirms it is this code path. The ERR_INCOMPLETE_CHUNKED_ENCODING the browser sees is NGINX forwarding a Transfer-Encoding: chunked head with identity body bytes from the upstream Bun server.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found — the fix looks correct and fromAncientRequest is only a framing gate (connection-close is set independently), but this is wire-level HTTP framing across the JS layer and C++ uWS bindings, so worth a human sanity check.

Extended reasoning...

Overview

This PR fixes HTTP/1.0 response framing in node:http where the advertised Transfer-Encoding header and the emitted body bytes disagreed. It touches three files: a ~10-line branch in renderNativeHeaders (src/js/node/_http_server.ts) to close-delimit instead of force-chunk when useChunkedEncodingByDefault === false; a new transferEncodingHasChunked helper in src/jsc/bindings/NodeHTTP.cpp (mirroring the existing connectionValueHasClose right above it) that clears httpResponseData->fromAncientRequest when the user explicitly writes a Transfer-Encoding: chunked header; and five new tests asserting exact raw head/body bytes over a raw HTTP/1.0 socket.

Security risks

HTTP response framing is response-smuggling territory. I verified in packages/bun-uws/src/HttpContext.h that HTTP_CONNECTION_CLOSE is set from httpRequest->isAncient() before and independently of fromAncientRequest, and grepped all uses of fromAncientRequest in HttpResponse.h — it gates only chunk-framing paths. So clearing it does not affect connection reuse (the socket still closes after the response), and the change strictly makes the body match a header the user already committed to the wire. I don't see a security regression, but wire-framing changes deserve a second pair of eyes.

Level of scrutiny

Medium-high. The diff is small (~30 production lines), follows existing local patterns exactly, and has strong byte-exact test coverage including a negative guard (Transfer-Encoding: gzip stays identity). But it modifies protocol-level body framing in both the built-in JS module and the C++ uWS binding, which is a critical code path for every node:http server response.

Other factors

No CODEOWNERS cover these files. No prior review comments. The PR description is thorough with a repro, root-cause analysis referencing Node's _storeHeader, and verification against the released binary. The bug-hunting pass found nothing. I'm deferring only because protocol-framing changes across native and JS layers are the kind of thing a maintainer familiar with the uWS integration should confirm.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any bugs, but this touches HTTP wire-framing across two layers (the JS renderNativeHeaders path and the C++ writeHead paths that mutate uWS's fromAncientRequest gate, which is also reached from Bun.serve via writeFetchHeadersToUWSResponse), so it's worth a human look.

Extended reasoning...

Overview

Fixes HTTP/1.0 response framing in node:http so the body encoding matches the advertised Transfer-Encoding header. Two changes:

  • src/js/node/_http_server.ts (renderNativeHeaders): when the user removed Content-Length and useChunkedEncodingByDefault === false (HTTP/1.0), close-delimit instead of forcing chunked.
  • src/jsc/bindings/NodeHTTP.cpp: adds transferEncodingHasChunked() (mirroring the existing connectionValueHasClose pattern) and, in both header-write paths, clears httpResponseData->fromAncientRequest when a Transfer-Encoding header containing the chunked token is written, so the uWS body writer chunk-frames to match the head.
  • Five new HTTP/1.0 raw-socket tests asserting exact head/body bytes.

I traced fromAncientRequest through packages/bun-uws/src/HttpResponse.h — it gates only body-framing decisions (lines ~143/513/562/635/677); connection-close semantics are carried by the separate HTTP_CONNECTION_CLOSE state flag, which is set from httpRequest->isAncient() in HttpContext.h and is untouched here. So clearing the flag should not leak keep-alive behavior onto HTTP/1.0 connections.

Security risks

HTTP body-framing mismatches are the classic response-splitting/desync vector. This PR reduces that risk (before, the head said chunked while the body was identity), but it does so by mutating a shared uWS state flag that gates several code paths. The transferEncodingHasChunked token match is intentionally loose (mirrors Node's /(?:^|\W)chunked(?:$|\W)/i), which is correct for compat but worth a second pair of eyes.

Level of scrutiny

Medium-high. This is production HTTP wire-protocol code spanning a built-in JS module and C++ JSC bindings, and writeFetchHeadersToUWSResponse (one of the two patched sites) is also invoked from the Bun.serve path via WebCore__FetchHeaders__toUWSResponse, so the blast radius isn't strictly limited to node:http. Not a mechanical/config change I'd rubber-stamp.

Other factors

  • Well-tested: five new tests, verified failing on release/ASAN without the fix and passing with it.
  • No CODEOWNERS coverage for these files.
  • No prior human review yet; CI build was just triggered.
  • Companion to #33871 (HTTP/1.1 TE-value framing), so a maintainer may want to look at both together.

@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Re blast radius into Bun.serve: writeFetchHeadersToUWSResponse is reachable from there, but RequestContext::do_write_headers strips Transfer-Encoding (and Content-Length) before calling it (RequestContext.rs:3799-3800), so the new fromAncientRequest clear is unreachable on that path. Verified empirically: a Bun.serve handler returning new Response("hello", { headers: { "Transfer-Encoding": "chunked" } }) to an HTTP/1.0 client emits identical bytes before and after this change (Content-Length: 5, no TE header).

@robobun

robobun commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

The diff is green: test/js/node/http/node-http-transfer-encoding.test.ts passes on every lane, and the gate confirmed fail-before/pass-after on ASAN and release.

Remaining CI red is unrelated Windows flake on lanes this change does not touch:

  • build 71194: proxy-stress-concurrent.test.ts (1/1200 stress flake on macOS, uses Bun.serve/fetch not node:http), plus postgres/mysql container startup failures
  • build 71206: test/napi/napi.test.ts on Windows x64-baseline, plus Windows-only flakes in hot.test.ts, compile-windows-metadata.test.ts, deinitialization.test.ts, node-tls-connect.test.ts (all passed on retry)

None of these exercise node:http server responses, HTTP/1.0 requests, or NodeHTTPServer__writeHead. Ready for review.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-09, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#19789) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

@robobun robobun closed this Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ERR_INCOMPLETE_CHUNKED_ENCODING with Next.js app using Bun in Docker behind NGINX proxy

1 participant