http: support CONNECT method in node:http client#31574
Conversation
|
Updated 5:30 PM PT - May 29th, 2026
❌ @robobun, your commit dc84886 has some failures in 🧪 To try this PR locally: bunx bun-pr 31574That installs a local version of the PR into your bun-31574 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Thanks — I verified each against this build:
So only #22311 is marked as fixed. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HTTP CONNECT proxy tunneling to the Node.js HTTP client shim: CONNECT requests bypass fetch(), open raw TCP/TLS sockets to proxies, parse proxy responses, construct no-body responses, and emit Node-style connect/close/error events; includes tests for success, non-200, head bytes, malformed status, unreachable proxy, and custom DNS lookup. ChangesHTTP CONNECT Proxy Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/js/node/_http_client.ts`:
- Around line 747-757: The code currently proceeds to emit a successful proxy
'connect' even when CONNECT_STATUS_LINE_REGEX fails to parse headerText, leaving
IncomingMessage (res) without statusCode/statusMessage; update the path in the
CONNECT handling (around where headerText is split, statusLine is matched
against CONNECT_STATUS_LINE_REGEX and IncomingMessage is constructed) to detect
a missing statusMatch and treat it as a failure: create a descriptive Error
(include the raw headerText/statusLine), destroy/cleanup the socket/tunnel, and
emit or callback an error instead of emitting 'connect' so the request fails for
malformed proxy responses; mirror the same check and behavior in the other
occurrence that uses the same logic to set statusCodeSymbol and
statusMessageSymbol.
- Around line 298-305: The early return in the CONNECT handling block causes
CONNECT requests to skip the address-resolution flow (options.lookup /
multi-candidate resolution), so restore that resolution before dispatching the
raw TCP socket: in the CONNECT branch (check of this[kMethod] === "CONNECT" and
the call to startConnect()), invoke or reuse the same lookup/address-resolution
logic used by the normal request path (the options.lookup + multi-candidate
iteration used elsewhere) and only after resolving candidates set fetching =
true and startConnect(); remove the immediate return so CONNECT still emits
'connect' but uses the resolved addresses/candidate fallback sequence.
- Around line 702-722: When the CONNECT socket fails before headers (connected
=== false) the onError/onClose handlers currently clear listeners and emit
"error" but never call maybeEmitClose(), so req.on("close") isn't fired; update
onError (and onClose path) to call maybeEmitClose() after terminal failure
handling: inside onError, after setting fetching = false and emitting the error
(and only when not isAbortError and not connected), invoke maybeEmitClose() (or
this.maybeEmitClose()) so the request emits "close"; ensure onClose which calls
onError(new ConnResetException(...)) also results in maybeEmitClose() being
called (avoid duplicate calls when onError already invoked).
🪄 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: 5719ddf7-9bc0-4810-8a78-cf37d33b924c
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/js/node/http/node-http-connect.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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/node/http/node-http-connect.test.ts (1)
688-696: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRun the Bun compatibility subprocess via
bun bd test.This file is under
test/**/*.test.ts, so invokingbun testhere weakens the required debug-build coverage for the cross-runtime check.♻️ Proposed fix
- cmd: [bunExe(), "test", join(import.meta.dir, "node-http-connect.node.mts")], + cmd: [bunExe(), "bd", "test", join(import.meta.dir, "node-http-connect.node.mts")],As per coding guidelines "Use
bun bd test <...test file>to run tests, notbun test. The debug build compiles your code automatically."🤖 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/node/http/node-http-connect.test.ts` around lines 688 - 696, The test currently spawns Bun with "bun test" which bypasses the required debug-build; update the Bun.spawn invocation in the "tests should run on bun" test to use "bun bd test" instead of "bun test" (keep using bunExe(), bunEnv, and the same args array and process.exited handling) so the subprocess runs the debug-build test runner.src/js/node/_http_client.ts (1)
780-791:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail CONNECT on malformed header lines.
At Line 783, lines without a valid
name:pair are silently skipped, so a response likeHTTP/1.1 200 OK\r\nbad-header\r\n\r\nstill emits'connect'and hands back a tunnel. Invalid proxy headers should take the same HPE parse-error path as the malformed status-line case.🐛 Proposed fix
for (let i = 0; i < lines.length; i++) { const line = lines[i]; const colon = line.indexOf(":"); - if (colon === -1) continue; + if (colon <= 0) { + socket.destroy(); + onError($HPE_INVALID_HEADER_TOKEN("Parse Error: Invalid header token encountered")); + return; + } const key = line.slice(0, colon); + try { + validateHeaderName(key); + } catch { + socket.destroy(); + onError($HPE_INVALID_HEADER_TOKEN("Parse Error: Invalid header token encountered")); + return; + } // Strip OWS = *(SP / HTAB) on both sides of the value, matching llhttp // (RFC 7230 §3.2.4), so padded proxy headers parse like they do in Node. let start = colon + 1;🤖 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 `@src/js/node/_http_client.ts` around lines 780 - 791, The loop that parses proxy header lines (the for loop over lines with variable line and the check if (colon === -1) continue) currently silently skips malformed header lines; change that to treat a header without a colon as a parse error and fail the CONNECT flow the same way the malformed status-line does so we don't emit 'connect' for invalid proxy responses. Locate the header-parsing loop in _http_client.ts (the block that computes colon, key, start/end, val) and replace the skip-on-missing-colon behavior with the same HPE parse-error handling used for the malformed status-line case (trigger the parse-failure/abort path and do not emit the 'connect' event or return a tunnel). Ensure the change references the same error path used elsewhere so logs/metrics remain consistent.
🤖 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 `@src/js/node/_http_client.ts`:
- Around line 780-791: The loop that parses proxy header lines (the for loop
over lines with variable line and the check if (colon === -1) continue)
currently silently skips malformed header lines; change that to treat a header
without a colon as a parse error and fail the CONNECT flow the same way the
malformed status-line does so we don't emit 'connect' for invalid proxy
responses. Locate the header-parsing loop in _http_client.ts (the block that
computes colon, key, start/end, val) and replace the skip-on-missing-colon
behavior with the same HPE parse-error handling used for the malformed
status-line case (trigger the parse-failure/abort path and do not emit the
'connect' event or return a tunnel). Ensure the change references the same error
path used elsewhere so logs/metrics remain consistent.
In `@test/js/node/http/node-http-connect.test.ts`:
- Around line 688-696: The test currently spawns Bun with "bun test" which
bypasses the required debug-build; update the Bun.spawn invocation in the "tests
should run on bun" test to use "bun bd test" instead of "bun test" (keep using
bunExe(), bunEnv, and the same args array and process.exited handling) so the
subprocess runs the debug-build test runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30b3bb73-42d4-4035-953f-86b12bab5b38
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/js/node/http/node-http-connect.test.ts
|
The CONNECT diff is green and non-flaky — the red CI lanes are unrelated flake/infra, not this change:
No annotation touches I have already spent my single CI re-roll on an earlier (also unrelated) flake, so I am not pushing another retrigger. This needs a maintainer to re-run the flaky lanes / merge. |
There was a problem hiding this comment.
Nothing further from automated review — all prior feedback has been addressed. This is a ~280-line hand-rolled CONNECT tunnel path (raw socket + manual HTTP response parsing + abort/close lifecycle wiring) in the core node:http client, with one acknowledged follow-up around socketCloseListener and post-tunnel req.destroy(), so it's worth a human pass before merge.
Extended reasoning...
Overview
This PR adds HTTP CONNECT method support to Bun's node:http client by introducing a new startConnect() code path in src/js/node/_http_client.ts (~280 net new lines) that bypasses the existing fetch-based dispatch entirely. It opens a raw net/tls socket to the proxy, hand-writes the CONNECT host:port HTTP/1.1 request head, and hand-parses the proxy's response (status-line regex, CRLF splitting, OWS trimming, Node's _addHeaderLine set-cookie/singleton folding rules, maxHeaderSize enforcement) before constructing an IncomingMessage and emitting 'connect'. ~400 lines of tests are added covering the happy path, non-2xx, head bytes, malformed status, header overflow, ECONNREFUSED, custom lookup, abort-before-response, and post-header data buffering.
Security risks
The new code parses untrusted proxy response bytes. Mitigations in place: maxHeaderSize bounds the header buffer (both never-terminated and complete-in-one-read cases), the parsed-headers object is null-prototype, the status line is regex-validated and rejected with an HPE_* error otherwise, and header values are OWS-trimmed. The request-side header serialization writes user-supplied header values verbatim into the wire bytes; header-name/value validation is delegated to the existing setHeader path so this is no worse than the normal request path. I don't see an injection or memory-exhaustion vector beyond what the normal HTTP client already exposes, but the hand-rolled parser is the part most deserving of a second pair of eyes.
Level of scrutiny
High. This is a new protocol-handling code path in a core Node-compat module (node:http), with manual wire-format parsing and non-trivial event-lifecycle coordination (the new socket interacts with the pre-existing socketCloseListener/onAbort/AbortController machinery that was designed around FakeSocket, not a real net.Socket). The author explicitly left one edge case — req.destroy() after 'connect' fires causing a double socket 'close' — as a documented follow-up requiring deeper socketCloseListener rework. CONNECT was entirely non-functional before, so the change is purely additive (no regression surface for non-CONNECT requests), but the architectural choice to bypass fetch and hand-roll the tunnel deserves maintainer sign-off.
Other factors
The PR has been through ~10 rounds of automated review (CodeRabbit + claude[bot]) and every comment has been addressed with a follow-up commit and Node-v24 verification. Test coverage is thorough. CI is reported green on the CONNECT suite (unrelated flakes elsewhere). No CODEOWNERS apply to these files. Given the scope, complexity, and the acknowledged follow-up, this should not be auto-approved.
http.request({ method: "CONNECT" }) routed through fetch(), which has
no representation for a tunnel: the request target is a host:port
authority rather than a URL path and the response is a raw socket, not a
message body. The shim prepended a spurious leading slash to the target
and never emitted the 'connect' event, so CONNECT proxy clients hung.
Dispatch CONNECT over a raw TCP (or TLS, for https proxies) socket
instead: write the CONNECT request line + headers, parse the proxy's
status line and headers, and emit 'connect' with (res, socket, head) the
way Node does. Default Host/Connection headers are added to match Node
and satisfy proxies that reject a CONNECT without a Host.
Fixes HTTP CONNECT proxy clients such as @grpc/grpc-js proxy support.
The 'tests should run on node.js/bun' cases spawn a full test-runner subprocess of the sibling .node.mts file, which can take ~5s. Bump their timeout off the 5s default so they don't flake on a loaded CI machine.
…okup) Addresses review feedback on the CONNECT path: - Reject an unparseable proxy status line instead of emitting 'connect' with an undefined statusCode. A response whose first line isn't 'HTTP/x.y NNN' now fails the request with an HPE parse error, matching Node. - Emit 'close' on the ClientRequest after a pre-tunnel failure (ECONNREFUSED, socket hang up, parse error). Node emits 'close' after 'error' on a failed request; previously only 'error' fired. - Forward options.lookup/family/hints/localAddress/localPort to the proxy connect so a custom DNS resolver and address selection work the same as the normal request path. net.connect() already performs the resolution, so no manual loop is needed. Adds tests for the malformed-status-line error, the error→close ordering on an unreachable proxy, and a custom lookup resolving the proxy host. All verified against Node v24.
The CONNECT response header parser stripped only a single leading ASCII space. Per RFC 7230 §3.2.4 the field value may be surrounded by OWS (*(SP / HTAB)), which llhttp trims on both sides. Trim leading/trailing spaces and tabs so padded proxy headers (leading tab, multiple spaces, trailing space) parse identically to Node. Adds a regression test.
Two Node-compat refinements to the CONNECT client: - Write the request headers using their original-case names (getRawHeaderNames) instead of the lowercased getHeaders() keys, so the wire bytes read `Host:`/`Connection:`/the caller's casing like Node rather than `host:`/`connection:`. Header names are case-insensitive, but this matches Node byte-for-byte. - Set res.socket to the tunnel socket and this.res to the response before emitting 'connect', so inside the listener res.socket === socket and req.res === res as in Node (res.req stays undefined for CONNECT, also matching Node). Extends the tunnel test to assert the res.socket/req.res wiring.
…ding)
Three Node-compat refinements to the CONNECT client from review:
- Set res.upgrade = true on the CONNECT response before emitting
'connect', so res.upgrade === true in the listener like Node.
- Propagate the net/tls connection error verbatim instead of replacing
it with a bare Error("ECONNREFUSED"). The net error is already
Node-shaped (code/syscall/address/port and a 'connect ECONNREFUSED
host:port' message), so flattening it dropped those diagnostic fields.
- Fold duplicate proxy response headers with Node's _addHeaderLine
rules: set-cookie is always an array, singleton headers keep the first
value, everything else is comma-joined (previously everything was
comma-joined).
Extends the tests to assert res.upgrade, the preserved error fields, and
set-cookie/singleton/multi header folding. Verified against Node v24.
Keep the synchronous net/tls connect() catch consistent with the async
onError path: emit 'close' after 'error' (and clear fetching) so a
req.on('close') cleanup listener runs on every terminal pre-tunnel
failure, not just the async ones.
A proxy header whose lowercased name collides with Object.prototype
(e.g. "constructor", "__proto__") folded against the inherited member
rather than an absent own property, producing a garbage value or
silently dropping the header. Build the parsed-headers map with
{ __proto__: null } so lookups see only own properties, matching Node.
Extends the folding test with a Constructor header.
The size guard only ran while the \r\n\r\n terminator was still missing, so an oversized header block that arrived complete in one read bypassed it and got parsed. Also reject when the located header block exceeds maxHeaderSize, matching Node's llhttp byte counting (HPE_HEADER_OVERFLOW regardless of where the terminator lands). Adds a regression test.
Follow test/CLAUDE.md's string convention (String.prototype.repeat is slow in debug JSC builds); matches BIG_DATA at the top of the file.
- Re-remove src/runtime/linear_fifo_testing.rs, which got re-staged by accident again; it belongs to unrelated #31563 work and is orphaned. - Don't emit a spurious 'error' after 'close' when a CONNECT request is aborted/destroyed before the proxy responds, and keep a backstop error listener on the raw socket so the AbortController's teardown AbortError is swallowed instead of crashing with ERR_UNHANDLED_ERROR. - Deliver the proxy status reason phrase verbatim ('' when omitted) like llhttp/Node instead of substituting the STATUS_CODES text. Adds a regression test for aborting a CONNECT before the proxy responds.
The pre-tunnel AbortError backstop listener was never removed, so the
tunnel socket reached the user with an internal no-op 'error' listener
(socket.listenerCount('error') >= 1) and post-tunnel socket errors were
silently swallowed instead of surfacing like Node. Name the backstop and
remove it when the tunnel is established; keep it attached on the
pre-tunnel failure/abort path so the teardown AbortError stays swallowed
(removing it there would reintroduce the unhandled-error crash).
The internal 'data' listener put the tunnel socket into flowing mode; removing the listener doesn't clear that, so the socket was handed to the user still flowing and bytes arriving after the headers could be dropped if the 'connect' handler attached its 'data' listener later (e.g. after an await). Reset socket.readableFlowing = null before emitting 'connect', matching Node, so those bytes stay buffered. Adds a regression test.
robobun
force-pushed
the
farm/61bdc34d/http-client-connect
branch
from
86d55d7 to
e3f46c6
Compare
|
CI for build #59100 is green on everything this diff touches — the only red lane ( The ASAN test lane ( A |
Node's socketOnData strips its close listener before emitting 'connect', so
the user's handler sees the tunnel socket with socket.listenerCount('close')
=== 0. Bun attached its maybeEmitClose 'close' listener before the emit, so
the delivered socket had one internal listener, contradicting the handover
comment and differing from Node.
Move the socket.once('close', ...) to after the emit. Socket 'close' is
async, so the listener still fires even if the handler calls socket.destroy()
synchronously. The CONNECT test now asserts listenerCount('close') === 0 on
handover (fails before this change with 1).
There was a problem hiding this comment.
All my earlier feedback is addressed and I have nothing further to flag — deferring only because ~280 lines of new hand-rolled HTTP response parsing and socket-lifecycle logic in the core node:http client path is worth a maintainer's eyes on the overall approach before merge.
Extended reasoning...
Overview
This PR adds CONNECT-method support to node:http's ClientRequest by introducing a new startConnect() path (~280 lines) in src/js/node/_http_client.ts that bypasses the fetch-based dispatch entirely: it opens a raw net/tls socket to the proxy, hand-writes the request head, hand-parses the proxy's status line and headers (status-line regex, OWS trimming, set-cookie/singleton folding, maxHeaderSize enforcement, null-prototype header map), and emits 'connect' with (res, socket, head). It also adds ~430 lines of tests in test/js/node/http/node-http-connect.test.ts covering tunneling, non-200 status, head bytes, header folding/whitespace, malformed status lines, header overflow, abort-before-response, custom lookup, and the non-flowing socket handover.
Security risks
The new code parses untrusted bytes from a (user-chosen) proxy. The obvious risks — unbounded header buffering, prototype pollution via header names, malformed-status-line acceptance — were each raised in earlier review rounds and addressed (maxHeaderSize enforced on both incomplete and complete header blocks; { __proto__: null } for the parsed-headers map; malformed status line now fails with HPE_INVALID_HEADER_TOKEN). The request-side header serialization uses getRawHeaderNames() + getHeader() from the existing OutgoingMessage machinery, so CRLF-injection exposure is no different from the existing fetch path. I don't see a new security risk introduced.
Level of scrutiny
This is a substantial new feature in a core, production-critical Node-compat module — not a config tweak or mechanical change. It hand-implements an HTTP/1.1 response parser and manages raw socket lifecycle (listener attach/detach ordering, flowing-state reset, abort/error/close interleaving) in a file that previously delegated everything to native fetch. That's exactly the kind of change where a maintainer should sanity-check the architectural choice (raw-socket + JS parser vs. a native path) and the known deferred follow-up (post-tunnel req.destroy() double-emitting 'close' on the user's socket via the shared socketCloseListener, which the author explicitly left for later).
Other factors
The PR has been through ~15 rounds of bot review (CodeRabbit + my own inline comments); every thread is resolved, and the current bug-hunting pass found nothing. Test coverage is thorough and each fix has a regression test. The stray linear_fifo_testing.rs file that leaked in twice is confirmed gone (changed-files count is 2). CI failures are unrelated flake per the author's analysis. Given all that, I'm not approving only because the size and subsystem warrant a human sign-off, not because of any open concern.
|
CI build #59103 (head This is a different flaky set than the previous build (#59100, which flaked on A |
|
Heads up: #31798 (fixing #31795, |
|
Heads up: #32171 reports the same bug (CONNECT via node:http, mangled target + no 'connect' event + divergent ECONNREFUSED message), so this PR fixes it too. I implemented the same raw-socket approach independently on
|
|
Closing this as superseded by #31587 (merged), which rewrote the I verified against a debug build of main by running this PR's own "HTTP client CONNECT" suite against it: 10 of the 11 tests pass unchanged (tunnel establishment, non-200 status, OWS trimming, head-byte delivery, post-tunnel buffering with The one test that doesn't pass is Since the merged rewrite covers this feature properly (and the vendored upstream client-proxy suite covers it in CI), there is nothing left to rebase here. Thanks. |
1 participant
What
Implements the HTTP
CONNECTmethod for thenode:httpclient so thathttp.request({ method: "CONNECT", ... })works for proxy tunneling.Fixes #31573.
Repro
Before: the proxy received a bogus
/example.com:443target and theconnectevent never fired — the request hung forever. (On 1.3.11 it threwTypeError: fetch() URL is invalidsynchronously.)After: matches Node — the proxy sees
example.com:443,connectfires withstatusCode === 200, and bytes tunnel through the returned socket.Cause
ClientRequestdispatched every method throughfetch()(nodeHttpClient).fetch()has no representation for a CONNECT tunnel:host:portauthority, not a URL path —getURL()prepended a leading/, so proxies saw/example.com:443;connectevent, so the request just hung.Fix
In
src/js/node/_http_client.ts, detectmethod === "CONNECT"and bypass the fetch path entirely:https:proxy) to the proxy authority vianode:net/node:tls;CONNECT <host:port> HTTP/1.1request line verbatim (no slash) plus the request headers, adding defaultHost/Connectionheaders the way Node does (a CONNECT with noHostis rejected by many proxies, including Bun's own server parser);connectwith(res, socket, head)—resis a minimalIncomingMessagecarryingstatusCode/statusMessage/headers/rawHeaders,socketis the raw tunneled socket, andheadis any bytes received after the response headers;error(e.g.ECONNREFUSED) when the proxy is unreachable, and emitcloseon the request once the tunnel socket closes.Event order on the request now matches Node:
socket,finish,connect,close.This unblocks HTTP CONNECT proxy clients, notably
@grpc/grpc-jsproxy support (grpc_proxy/http_proxy).Tests
test/js/node/http/node-http-connect.test.ts— newHTTP client CONNECTblock:connectfires withstatusCode200 + headers, and data echoes through the tunnel;connectfires even on a non-200 status (403) with the correctstatusMessage/headers;head;errorwithECONNREFUSEDwhen the proxy is unreachable.Verified the tunnel/status/head tests fail on the released binary (they time out —
connectnever fires) and pass with this change. Behavior was compared directly against Node v24 (wire bytes, status/headers,head, and request event order all match).Related issues
Fixes #22311 (the exact-same bug — verified end-to-end with the reporter's repro).
This PR also makes
https-proxy-agentwork through a CONNECT tunnel (verified end-to-end: an HTTPS request tunneled through a proxy returns200), which is the core of #15499 and #31474. I've left those open rather than auto-closing them because each has a part this PR does not cover: #15499 also trackssocks-proxy-agent(SOCKS, unrelated to HTTP CONNECT), and #31474 is about the exact error code on an unreachable proxy, which still differs from Node at thenode:netDNS layer (ESERVFAILvsEAI_AGAIN), not in this CONNECT path. #4474 (undici.ProxyAgent) is a separate undici TLS code path that is still broken independently of this change.