fix(node:http): emit 'upgrade' event on ClientRequest for 101 responses#27859
fix(node:http): emit 'upgrade' event on ClientRequest for 101 responses#27859MarcinDudekDev wants to merge 10 commits into
Conversation
|
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 Upgrade handling to the Node HTTP client by opening raw TCP/TLS sockets for Upgrade requests (performing an HTTP/1.1 handshake and emitting an Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 348-366: The manual upgrade parser in onData keeps concatenating
chunks into buf without enforcing header limits, which allows unbounded memory
growth; modify onData (and the upgrade handling) to consult the same limits used
by the normal parser (maxHeaderSize and maxHeadersCount) — before
concatenating/appending a chunk, check (buf.length + chunk.length) against
maxHeaderSize and, after finding headerEnd, split headerLines and ensure
headerLines.length <= maxHeadersCount and headerPart.length <= maxHeaderSize; if
any limit is exceeded, stop parsing by removing the data listener and
closing/destroying upgradeSocket (and emit/propagate an HTTP parser error) so
large or oversized upgrade headers are rejected. Ensure you reference and use
the existing config/option variables (maxHeaderSize, maxHeadersCount) or fall
back to sensible defaults used elsewhere in the module.
- Around line 389-405: The upgrade socket handoff must complete before emitting
"upgrade": detach internal handlers (remove onData and the request-scoped error
listener attached to upgradeSocket), assign ownership of the socket to the
response (set res.socket = upgradeSocket and mark any internal upgraded flag
like this[kUpgradeOrConnect] = true), clear the request timeout via
this[kClearTimeout](), and only then call this.emit("upgrade", res,
upgradeSocket, head); also ensure you no longer forward later upgradeSocket
errors to the ClientRequest (remove or replace the upgradeSocket.on("error",
...) handler so user code receives errors instead of the request error path).
- Around line 319-342: The Host/SNI are being derived from parsedUrl.hostname
which may already be a resolved IP; change onSocketConnect and TLS opts so Host
header and servername come from this.getHeader("host") ?? this[kHost] (falling
back to parsedUrl.hostname only if neither is set), while using the resolved
address (parsedUrl.hostname when it is an IP candidate) only for the connect
call (tls.connect/net.connect). Update the Host construction in onSocketConnect
(currently building raw with hostname and this[kUseDefaultPort]/this[kPort]) to
use the header-derived host, and build tlsOpts in the tls.connect branch using
servername from this.getHeader("host") ?? this[kHost] rather than the resolved
address; keep the resolved address solely as the host/port passed to the connect
function.
- Around line 373-383: The current code constructs a Fetch Response (new
Response(...)) for status 101 which throws for 1xx and flattens duplicate
headers; instead, when building the IncomingMessage for upgrades in the block
around IncomingMessage construction, stop using new Response and pass a plain
object containing status: statusCode, statusText: statusMatch?.[3] || "Switching
Protocols", headers: responseHeaders, and include rawHeaders as-is so duplicate
headers are preserved; keep the surrounding calls to
getIsNextIncomingMessageHTTPS(), setIsNextIncomingMessageHTTPS(protocol ===
"https:"), and the metadata properties ([typeSymbol]:
NodeHTTPIncomingRequestType.FetchResponse, [reqSymbol]: this) unchanged when
creating the IncomingMessage to ensure the upgrade event can be emitted without
RangeError.
- Around line 392-395: The code currently treats non-101 upgrade responses as
fatal by destroying upgradeSocket and emitting an error; instead create an
IncomingMessage (using the existing socket and parsed headers/statusCode) and
emit a "response" event so consumers can read the status and body; specifically,
in the branch handling non-101 statuses (referencing upgradeSocket and
statusCode), do not call upgradeSocket.destroy() or emit("error"); construct an
IncomingMessage (or reuse the client-side response wrapper used elsewhere in
this file), attach the socket as its connection/stream, set
headers/statusCode/statusMessage, and call this.emit("response",
incomingMessage) so normal HTTP response handling occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e7c6d5b-cb5d-423f-bb12-1e65c830028e
📒 Files selected for processing (1)
src/js/node/_http_client.ts
MarcinDudekDev
force-pushed
the
http-upgrade-fix
branch
from
5628f08 to
21e1d00
Compare
|
Thanks @coderabbitai — all 5 issues addressed in the force-pushed update:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the current state of the implementation to verify all 5 fixes. 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 7654 🏁 Script executed: Length of output: 1372
One small residual concern worth noting: the non-101 branch (Line 431) still uses const safeStatus = statusCode >= 200 ? statusCode : 200;
const res = new IncomingMessage(new Response(null, {
status: safeStatus,
statusText: statusMatch?.[3] || "",
}), { ... });
res.statusCode = statusCode; // restore real code after constructionOr alternatively, apply the same 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 409-424: The handmade IncomingMessage lacks stream/internal flags;
after creating res = Object.create(IncomingMessage.prototype) (in the 101
handling block) set minimal stream-safe defaults such as res._consuming = false,
res._dumped = false, res.readable = true, and set the internal aborted
symbol/flag to false (the same symbol/name used elsewhere in this file or
project), so the object behaves like a real IncomingMessage for libraries that
call pipe/on('data'); add these assignments before assigning socket/req and
before emitting "upgrade" (i.e., near where res.statusCode/statusMessage/headers
are set) and keep names consistent with existing symbols like kUpgradeOrConnect
and kClearTimeout usage.
- Around line 425-437: The Response constructor can throw for 1xx status codes
other than 101; update the block that builds the IncomingMessage (around
IncomingMessage, Response, getIsNextIncomingMessageHTTPS,
setIsNextIncomingMessageHTTPS, typeSymbol, reqSymbol) to guard against that:
check if statusCode is in the 100–199 range and not 101 (or wrap new
Response(...) in try/catch), and if so avoid calling new Response with the raw
1xx code — instead fall back to a safe Response (e.g., use 200 or another
acceptable status) or create the IncomingMessage via a safe alternative and
preserve the original status in a header like X-Original-Status so you don’t
throw; ensure you still call setIsNextIncomingMessageHTTPS(protocol ===
"https:") and attach [typeSymbol] and [reqSymbol] as before.
- Around line 358-364: The onError handler in _http_client.ts must destroy the
socket to avoid half-open connections and you must add handlers for socket
'close' and 'end' that abort the request if the server closes before headers
complete; specifically, update the onError function to call
upgradeSocket.destroy() (or upgradeSocket.destroy(err)) after clearing the
timeout and before emitting the error, and add onClose/onEnd listeners that call
this[kClearTimeout](), mark the request as errored/destroyed and emit an
appropriate error to prevent hanging; finally, ensure these new onClose/onEnd
listeners are removed in the same detach block where other listeners are removed
(the block that currently removes onData/onError) so they don’t leak.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17abfd2f-37d0-43b7-af43-68aab9ea106a
📥 Commits
Reviewing files that changed from the base of the PR and between 5628f08 and 21e1d00c6b578e1229f03881f307376d750570eb.
📒 Files selected for processing (1)
src/js/node/_http_client.ts
When a ClientRequest has an Upgrade header and the caller listens for the 'upgrade' event, use a raw TCP socket instead of the internal fetch-based nodeHttpClient. This is necessary because fetch doesn't support HTTP protocol upgrades (101 Switching Protocols). The fix: - Detects upgrade requests (has Upgrade header + 'upgrade' listeners) - Opens a raw net.Socket (or tls.connect for HTTPS) to the target - Sends the HTTP request manually - Parses the 101 response headers - Emits 'upgrade' with (res, socket, head) matching Node.js behavior - Falls back to existing fetch path for all non-upgrade requests Details: - Uses original host (not resolved IP) for Host header and TLS SNI - Enforces maxHeaderSize limit on the manual header parser - Builds IncomingMessage directly (Fetch Response rejects 1xx status) - Properly detaches internal handlers before socket handoff - Non-101 responses emit 'response' (not error) per Node.js semantics Fixes oven-sh#9911 Fixes oven-sh#9357 Fixes oven-sh#17320 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MarcinDudekDev
force-pushed
the
http-upgrade-fix
branch
from
34a7da6 to
7a7dcf4
Compare
|
Updated — addressed the remaining 3 issues:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/js/node/_http_client.ts (1)
424-442: 🧹 Nitpick | 🔵 TrivialIncomingMessage for 101 lacks full stream initialization.
The
Object.create(IncomingMessage.prototype)approach correctly avoids the FetchResponseconstructor's rejection of 1xx status codes. However, the manually created object lacks initialization of internal symbols and Readable stream internals:
- Missing symbol initialization:
abortedSymbolandeofInProgress(used in_destroyand_readmethods per relevant code snippet)- Missing
Readable.$call(res): Without this,_readableStateis undefined and stream methods (pipe(),on('data')) will failFor the primary WebSocket/CDP use cases (Playwright, ws), this is acceptable since consumers use the socket directly rather than reading from the response. However, if any library attempts to treat this as a Readable stream, it will throw.
🛡️ Optional: Add minimal stream-safe initialization
const res = Object.create(IncomingMessage.prototype); +// Initialize internal symbols expected by _destroy/_read +res[require("internal/http").abortedSymbol] = false; +res[require("internal/http").eofInProgress] = false; res.statusCode = statusCode; res.statusMessage = statusMatch?.[3] || "Switching Protocols";Note: Full stream compatibility would require calling
Readable.$call(res), but this may have unintended side effects for upgrade responses where the socket is the primary data channel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 424 - 442, The created IncomingMessage for 101 lacks internal stream setup; initialize the internal symbols (e.g., abortedSymbol and eofInProgress) on the fabricated res and invoke the Readable internal initializer (Readable.$call(res)) so res._readableState and stream internals exist before emitting "upgrade"; keep the rest of the fields (statusCode, headers, socket, req) as-is and perform these initializations just after Object.create(IncomingMessage.prototype) and before emitting the "upgrade" event (referencing IncomingMessage, abortedSymbol, eofInProgress, Readable.$call, kUpgradeOrConnect, kClearTimeout).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 424-442: The created IncomingMessage for 101 lacks internal stream
setup; initialize the internal symbols (e.g., abortedSymbol and eofInProgress)
on the fabricated res and invoke the Readable internal initializer
(Readable.$call(res)) so res._readableState and stream internals exist before
emitting "upgrade"; keep the rest of the fields (statusCode, headers, socket,
req) as-is and perform these initializations just after
Object.create(IncomingMessage.prototype) and before emitting the "upgrade" event
(referencing IncomingMessage, abortedSymbol, eofInProgress, Readable.$call,
kUpgradeOrConnect, kClearTimeout).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf456484-c780-494b-8da7-46b60487221c
📥 Commits
Reviewing files that changed from the base of the PR and between 21e1d00c6b578e1229f03881f307376d750570eb and 7a7dcf4.
📒 Files selected for processing (1)
src/js/node/_http_client.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 334-338: The loop and subsequent parsing use unguarded globals
(Object.entries, Array.isArray, statusLine.match and direct string methods);
replace them with the file's captured primordials/privates (use the existing
$-prefixed intrinsics already captured in this module) when iterating `headers`
and when matching/parsing `statusLine` and manipulating `raw` so all calls use
the captured variants (e.g., use the module's captured
entries/isArray/match/string prototype methods via their $ names or
.$call()/$apply() forms) to ensure the parser/serializer uses primordials
throughout.
- Around line 326-342: The onSocketConnect handler currently writes only the
request line and headers to upgradeSocket and never forwards buffered or
subsequent request body bytes, causing payloads in this[kBodyChunks] to be
dropped once fetching becomes true; modify onSocketConnect (and related upgrade
flow) to flush all existing buffered body chunks from this[kBodyChunks] to
upgradeSocket immediately after writing the headers, and also attach forwarding
handlers so future req.write/req.end data is piped to upgradeSocket (e.g.,
register a data listener and end handling or pipe the request stream) before
toggling fetching, ensuring upgradeSocket receives both buffered and subsequent
body bytes.
- Around line 316-370: The raw-upgrade branch currently doesn't return a
thenable, so callers of go() (used by options.lookup() iterator) get undefined
and cannot handle fallback; modify the upgrade path around
onSocketConnect/upgradeSocket/onError to return a Promise from the raw upgrade
branch (the same contract as other code paths of go()) that resolves when the
upgrade succeeds (socket established and write completes) and rejects on fatal
errors, but onError must respect softFail semantics (i.e., reject only when not
softFail so the iterator can fall back) and clean up via
detachListeners/upgradeSocket.destroy and this[kClearTimeout] before
resolving/rejecting; locate this logic in the function handling upgrade requests
(symbols: go, onSocketConnect, upgradeSocket, onError, detachListeners,
this[kClearTimeout]) and return that Promise from the branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d75db27b-5c84-4229-86fd-09d58408564b
📒 Files selected for processing (1)
src/js/node/_http_client.ts
|
Added a second commit: fix(ws): implement 'upgrade' event on ws.WebSocket This addresses a related gap — Bun's The fix emits a synthetic Also removes the misleading Together with the first commit (HTTP client upgrade support), this gives full WebSocket upgrade compatibility for tools like Playwright's |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/thirdparty/ws.js`:
- Around line 279-306: The synthetic "upgrade" can be emitted after "open"
depending on listener registration order; ensure "upgrade" always fires before
"open" by invoking the upgrade emission synchronously before any "open"
listeners run: change the logic around this.#ws.addEventListener("open", ...) /
onceObject so that when the native "open" is handled you first call the same
upgrade-emission code (this.emit("upgrade", {...})) unconditionally and
synchronously (or implement a guard like this._emittedUpgrade and in the
wrapper's emit method: if event === "open" and !_emittedUpgrade call the upgrade
emitter then proceed to emit "open"). Reference the existing symbols
this.#ws.addEventListener, emit("upgrade"), emit("open"), and onceObject when
making the change.
- Around line 298-303: The emitted "upgrade" object from the call
this.emit("upgrade", {...}) is a minimal synthetic IncomingMessage; update its
construction and documentation so downstream libs know the shape and get better
compatibility: in the block that emits "upgrade" (the object passed to
this.emit("upgrade")), add common IncomingMessage-like properties (e.g.,
socket/null, httpVersion/"1.1", complete=false, rawTrailers=[], trailers={},
method=undefined) or explicitly document which fields are intentionally omitted,
and update the surrounding comment to list included and excluded fields so
consumers (and tests) can rely on the exact synthetic response shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 380552de-b2d4-4712-8e3c-513f10baf614
📥 Commits
Reviewing files that changed from the base of the PR and between 4d64425 and 13285999555347e4c19ad977009c386462959264.
📒 Files selected for processing (1)
src/js/thirdparty/ws.js
Bun's HTTP client uses fetch() internally, which doesn't support HTTP protocol upgrades (101 Switching Protocols). This means the 'upgrade' event is never emitted on ClientRequest, breaking libraries like ws and Playwright that rely on it for WebSocket connections. This adds a raw TCP/TLS socket path for requests with an Upgrade header and active 'upgrade' listeners. The implementation: - Returns a Promise from the upgrade branch so the happy-eyeballs DNS iterator can .catch() on connection failures and try the next address (softFail semantics match the fetch path) - Uses original hostname (not resolved IP) for Host header and TLS SNI - Enforces configurable header size limit (default 16 KiB) - Parses the HTTP response and emits the appropriate event: - 101: emits 'upgrade' with a properly constructed IncomingMessage - Other 1xx: emits 'error' (unexpected during upgrade handshake) - 2xx-5xx: emits 'response' with body piping from the raw socket - Handles premature close, socket errors, and timeout cleanup Closes oven-sh#9911 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MarcinDudekDev
force-pushed
the
http-upgrade-fix
branch
from
1328599 to
8a9340c
Compare
Round 3 fixes (Gemini code review)Addressed issues found by thorough code review: Critical fixes:
Moderate fixes:
|
|
All issues from the latest CodeRabbit reviews (09:20, 09:34, 10:01 UTC) have been addressed in the force-pushed commits:
@coderabbitai review |
🧩 Analysis chain
🏁 Script executed: Length of output: 2438 🏁 Script executed: Length of output: 11794
One very minor residual edge case: if the server sends a completely malformed status line (no match for Triggering the full review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/node/_http_client.ts (1)
337-343: 🧹 Nitpick | 🔵 TrivialUse primordials for
Object.entriesto prevent user tampering.The upgrade path parses untrusted response data. Using
Object.entries(headers)directly allows user code to tamper withObject.entriesbehavior. Consider using the captured primordial or iterating withfor...in.♻️ Proposed fix using for...in loop
- for (const [key, val] of Object.entries(headers)) { - if ($isJSArray(val)) { - for (const v of val) raw += `${key}: ${v}\r\n`; - } else if (val != null) { - raw += `${key}: ${val}\r\n`; - } - } + for (const key in headers) { + const val = headers[key]; + if ($isJSArray(val)) { + for (const v of val) raw += `${key}: ${v}\r\n`; + } else if (val != null) { + raw += `${key}: ${val}\r\n`; + } + }As per coding guidelines, "Use private globals and methods with
$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 337 - 343, The loop building raw headers uses Object.entries(headers) which can be tampered with; replace it with a primordial-safe iteration (use the captured $Object or a for...in loop) when iterating the headers variable in _http_client.ts so you don't call the global Object.entries; specifically update the block that iterates headers (the for (const [key, val] of Object.entries(headers)) {...} section) to iterate own enumerable keys via a secure method (e.g., for (const key in headers) with Object.prototype.hasOwnProperty.call or use $Object.entries if a primordial is available) and keep the inner logic for handling $isJSArray(val) and null checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 473-479: The Response constructor can throw if statusCode is out
of 200–599 range (malformed status line); update the block that constructs the
synthetic Response used to create IncomingMessage by validating/clamping
statusCode before calling new Response (e.g., ensure statusCode is an integer in
200..599, fallback to 500 if not) and/or wrap the Response creation in a
try/catch to substitute a safe status value, then continue to construct
IncomingMessage with the validated status; make changes around the symbols
IncomingMessage, Response, statusCode, statusMatch,
NodeHTTPIncomingRequestType.FetchResponse, typeSymbol, reqSymbol and this.
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 337-343: The loop building raw headers uses
Object.entries(headers) which can be tampered with; replace it with a
primordial-safe iteration (use the captured $Object or a for...in loop) when
iterating the headers variable in _http_client.ts so you don't call the global
Object.entries; specifically update the block that iterates headers (the for
(const [key, val] of Object.entries(headers)) {...} section) to iterate own
enumerable keys via a secure method (e.g., for (const key in headers) with
Object.prototype.hasOwnProperty.call or use $Object.entries if a primordial is
available) and keep the inner logic for handling $isJSArray(val) and null checks
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40df939b-cd2c-4ff1-9b29-e731ba37e066
📥 Commits
Reviewing files that changed from the base of the PR and between 13285999555347e4c19ad977009c386462959264 and 8a9340cc6dfdef7f8471539023e950980433e1ea.
📒 Files selected for processing (2)
src/js/node/_http_client.tssrc/js/thirdparty/ws.js
Bun's ws shim wraps the native WebSocket, which handles HTTP upgrades
internally without exposing response details. Libraries like Playwright
listen for the 'upgrade' event to capture response headers after the
WebSocket handshake.
This emits a synthetic 'upgrade' event just before 'open' with a
minimal http.IncomingMessage-like response containing statusCode,
statusMessage, headers, and rawHeaders built from ws.protocol and
ws.extensions.
Key implementation details:
- Both 'upgrade' and 'open' share a single native 'open' listener
to guarantee correct event ordering regardless of registration order
- The persistent eventId bit is set AFTER the native listener guard
to prevent on("open") from blocking its own listener registration
- Removes misleading warnings for 'upgrade' and 'unexpected-response'
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MarcinDudekDev
force-pushed
the
http-upgrade-fix
branch
from
0a6b28e to
225ca16
Compare
|
Fixed the minor finding — malformed status line guard ( If the server sends a garbage status line that doesn't match Now guarded: if |
- Replace Object.entries(headers) with for...in loop to prevent user tampering with Object.entries (per Bun coding guidelines) - Flush kBodyChunks to upgradeSocket after writing headers, so payloads queued via req.write()/req.end() are not silently dropped Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/js/node/_http_client.ts (3)
358-389:⚠️ Potential issue | 🟠 MajorAbort/timeout does not tear down the handshake socket.
This branch never wires
upgradeSocketinto the existing abort path before headers complete.onAbort()/destroy()operate on the request's synthetic socket, so a timeout or explicit abort can leave the real TCP/TLS connection open until the peer closes it.Possible fix
+ const onAbortHandshake = () => { + detachListeners(); + upgradeSocket.destroy(new ConnResetException("aborted")); + this[kClearTimeout](); + resolve(); + }; + const detachListeners = () => { upgradeSocket.removeListener("data", onData); upgradeSocket.removeListener("error", onError); upgradeSocket.removeListener("close", onClose); + this[kAbortController]?.signal.removeEventListener("abort", onAbortHandshake); }; + + this[kAbortController]?.signal.addEventListener("abort", onAbortHandshake, { once: true });Also applies to: 529-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 358 - 389, The TLS/TCP "upgradeSocket" created in the handshake path isn't wired into the request abort/timeout logic, leaving real sockets open on abort/timeout; update the abort/destroy handlers (the onAbort/destroy path that currently calls this[kClearTimeout] and operates on the request's synthetic socket) to also reference and clean up upgradeSocket: ensure upgradeSocket is assigned to the request's socket reference or otherwise captured, call upgradeSocket.destroy() and detachListeners() in the onAbort/onTimeout/destroy handlers, and make onError/onClose also clear the same timeout via this[kClearTimeout]; apply the same fix to the equivalent code around the referenced second location (lines ~529-531) so timeouts/aborts always tear down the real TCP/TLS connection.
425-456: 🛠️ Refactor suggestion | 🟠 MajorUse captured intrinsics in the new header parser.
The manual parser is still calling user-tamperable prototype methods like
match,split,indexOf,substring,trim, andtoLowerCaseon wire data. Please switch these to the captured/$-prefixed intrinsics used elsewhere insrc/js/node.As per coding guidelines, "Use
.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation" and "Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 425 - 456, The header parser is calling user-tamperable prototype methods (match, split, indexOf, substring, trim, toLowerCase) on wire data; replace these with the captured intrinsics used elsewhere in this module (e.g., $StringPrototypeMatch.$call(statusLine,...), $StringPrototypeSplit.$call(headerPart,"\r\n"), $StringPrototypeIndexOf.$call(line, ":"), $StringPrototypeSubstring.$call(...), $StringPrototypeTrim.$call(...), $StringPrototypeToLowerCase.$call(lowerKey) or their $-prefixed equivalents) when computing statusMatch/statusCode, splitting headerPart, and extracting key/value in the loop (affecting headerPart, head, statusLine, statusMatch, statusCode, responseHeaders, rawHeaders); also use .$call() or .$apply() for any function invocation to avoid prototype tampering and keep all references to upgradeSocket, this[kClearTimeout], softFail, reject, resolve and emit unchanged.
348-355:⚠️ Potential issue | 🟠 MajorLater writes still have no consumer here.
This only flushes the chunks buffered before connect. After that,
write_()keeps appending tothis[kBodyChunks], but the raw-upgrade path never drains that queue again, soreq.write()/req.end(chunk)afterflushHeaders()are still silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 348 - 355, The current flush only writes the pre-connect buffer (this[kBodyChunks]) to upgradeSocket but leaves write_() appending future chunks to that buffer, so later req.write()/req.end() are never forwarded; after draining this[kBodyChunks] in the upgrade path (the code shown), mark the request as raw-upgraded and either replace/patch the request's write_() and end() implementations to directly write to upgradeSocket (and forward any remaining buffered chunks first) or add a connected flag that write_() checks to write directly to upgradeSocket when true; ensure you clear the buffer and forward any pending chunks before swapping behavior so no data is lost (references: this[kBodyChunks], write_(), flushHeaders(), upgradeSocket).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 309-369: The raw-upgrade branch in go() dials parsedUrl.hostname
directly and therefore ignores the proxy argument and socketPath; update the
upgrade branch (the code that builds upgradeSocket and calls
tls.connect/net.createConnection) to honor proxy and socketPath: if proxy is set
use the proxy/tunnel address and perform CONNECT through the proxy (or otherwise
proxy the TCP connect) so SNI and Host headers still use originalHost, and if
socketPath (this[kSocketPath] or similar) is set use that Unix domain socket
instead of connecting to parsedUrl.hostname; alternatively, if supporting
proxies/sockets in the upgrade flow is impractical, detect proxy or socketPath
at the start of the upgrade branch and throw a clear error from go() indicating
upgrade with proxy/socketPath is unsupported (referencing upgradeHeader,
parsedUrl, originalHost, protocol, upgradeSocket).
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 358-389: The TLS/TCP "upgradeSocket" created in the handshake path
isn't wired into the request abort/timeout logic, leaving real sockets open on
abort/timeout; update the abort/destroy handlers (the onAbort/destroy path that
currently calls this[kClearTimeout] and operates on the request's synthetic
socket) to also reference and clean up upgradeSocket: ensure upgradeSocket is
assigned to the request's socket reference or otherwise captured, call
upgradeSocket.destroy() and detachListeners() in the onAbort/onTimeout/destroy
handlers, and make onError/onClose also clear the same timeout via
this[kClearTimeout]; apply the same fix to the equivalent code around the
referenced second location (lines ~529-531) so timeouts/aborts always tear down
the real TCP/TLS connection.
- Around line 425-456: The header parser is calling user-tamperable prototype
methods (match, split, indexOf, substring, trim, toLowerCase) on wire data;
replace these with the captured intrinsics used elsewhere in this module (e.g.,
$StringPrototypeMatch.$call(statusLine,...),
$StringPrototypeSplit.$call(headerPart,"\r\n"),
$StringPrototypeIndexOf.$call(line, ":"), $StringPrototypeSubstring.$call(...),
$StringPrototypeTrim.$call(...), $StringPrototypeToLowerCase.$call(lowerKey) or
their $-prefixed equivalents) when computing statusMatch/statusCode, splitting
headerPart, and extracting key/value in the loop (affecting headerPart, head,
statusLine, statusMatch, statusCode, responseHeaders, rawHeaders); also use
.$call() or .$apply() for any function invocation to avoid prototype tampering
and keep all references to upgradeSocket, this[kClearTimeout], softFail, reject,
resolve and emit unchanged.
- Around line 348-355: The current flush only writes the pre-connect buffer
(this[kBodyChunks]) to upgradeSocket but leaves write_() appending future chunks
to that buffer, so later req.write()/req.end() are never forwarded; after
draining this[kBodyChunks] in the upgrade path (the code shown), mark the
request as raw-upgraded and either replace/patch the request's write_() and
end() implementations to directly write to upgradeSocket (and forward any
remaining buffered chunks first) or add a connected flag that write_() checks to
write directly to upgradeSocket when true; ensure you clear the buffer and
forward any pending chunks before swapping behavior so no data is lost
(references: this[kBodyChunks], write_(), flushHeaders(), upgradeSocket).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a1958cb-666f-4afe-a608-f2257faa38c4
📒 Files selected for processing (2)
src/js/node/_http_client.tssrc/js/thirdparty/ws.js
- Throw clear errors when upgrade is attempted through a proxy or Unix socket path (not supported in raw TCP upgrade flow) - Wire upgradeSocket into AbortController so req.destroy()/req.abort() tears down the real TCP/TLS connection during handshake - Clean up abort listener on all exit paths via detachListeners() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 436-442: In the header size check inside _http_client.ts (the
block that currently calls detachListeners(), upgradeSocket.destroy(),
this.emit("error", new Error(...)), resolve()), make it consistent with other
error paths by first calling this[kClearTimeout](), then detaching listeners and
destroying upgradeSocket, and before emitting an error ensure !this.destroyed;
also respect the softFail flag: if softFail is true call reject(new Error(...))
to signal failure for happy-eyeballs fallback, otherwise emit the error via
this.emit("error", ...); keep resolve/reject behavior consistent with the
surrounding Promise handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50489321-578d-461e-8bb1-4e3a51d156df
📒 Files selected for processing (2)
src/js/node/_http_client.tssrc/js/thirdparty/ws.js
Add missing kClearTimeout, softFail/reject branch, and !destroyed guard to the header size limit check, matching onError/onClose/ statusCode patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/thirdparty/ws.js (1)
301-306: 🧹 Nitpick | 🔵 TrivialConsider adding
httpVersionfor broader compatibility.The synthetic upgrade response object is minimal. While sufficient for primary use cases like Playwright/CDP, adding
httpVersionwould improve compatibility with code that checks the HTTP version:💡 Optional: Add httpVersion
this.emit("upgrade", { statusCode: 101, statusMessage: "Switching Protocols", headers, rawHeaders, + httpVersion: "1.1", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/thirdparty/ws.js` around lines 301 - 306, The synthetic upgrade response emitted via this.emit("upgrade", { statusCode: 101, statusMessage: "Switching Protocols", headers, rawHeaders }) should include an httpVersion property for broader compatibility; update the object passed to this.emit("upgrade") to include httpVersion (e.g. "1.1" or derived from the incoming socket/request if available) so consumers that check response.httpVersion see a valid value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 550-552: The added listeners on upgradeSocket
(upgradeSocket.on("data", ...), .on("end", ...), .on("error", ...)) are never
removed if the response stream res is destroyed early; add a res.once("close",
...) handler that removes those three listeners from upgradeSocket (use
removeListener/removeAllListeners as appropriate) to avoid retaining the socket
reference, and ensure the handler is cleaned up after it runs; reference the
existing upgradeSocket and res variables and the data/end/error listener
functions when removing them.
---
Duplicate comments:
In `@src/js/thirdparty/ws.js`:
- Around line 301-306: The synthetic upgrade response emitted via
this.emit("upgrade", { statusCode: 101, statusMessage: "Switching Protocols",
headers, rawHeaders }) should include an httpVersion property for broader
compatibility; update the object passed to this.emit("upgrade") to include
httpVersion (e.g. "1.1" or derived from the incoming socket/request if
available) so consumers that check response.httpVersion see a valid value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdf592f1-081d-440d-a062-7130cc531196
📒 Files selected for processing (2)
src/js/node/_http_client.tssrc/js/thirdparty/ws.js
| upgradeSocket.on("data", (chunk) => res.push(chunk)); | ||
| upgradeSocket.on("end", () => res.push(null)); | ||
| upgradeSocket.on("error", (err) => res.destroy(err)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Non-101 response socket listeners lack cleanup on stream destruction.
When piping the response body, the data/end/error listeners added to upgradeSocket are never removed if res is destroyed early (e.g., user calls res.destroy() before stream ends). This could cause the socket to remain referenced.
Consider removing these listeners in a res.once('close', ...) handler to ensure cleanup.
🔧 Optional cleanup handler
if (head.length > 0) res.push(head);
- upgradeSocket.on("data", (chunk) => res.push(chunk));
- upgradeSocket.on("end", () => res.push(null));
- upgradeSocket.on("error", (err) => res.destroy(err));
+ const onBodyData = (chunk) => res.push(chunk);
+ const onBodyEnd = () => res.push(null);
+ const onBodyError = (err) => res.destroy(err);
+ upgradeSocket.on("data", onBodyData);
+ upgradeSocket.on("end", onBodyEnd);
+ upgradeSocket.on("error", onBodyError);
+ res.once("close", () => {
+ upgradeSocket.removeListener("data", onBodyData);
+ upgradeSocket.removeListener("end", onBodyEnd);
+ upgradeSocket.removeListener("error", onBodyError);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/_http_client.ts` around lines 550 - 552, The added listeners on
upgradeSocket (upgradeSocket.on("data", ...), .on("end", ...), .on("error",
...)) are never removed if the response stream res is destroyed early; add a
res.once("close", ...) handler that removes those three listeners from
upgradeSocket (use removeListener/removeAllListeners as appropriate) to avoid
retaining the socket reference, and ensure the handler is cleaned up after it
runs; reference the existing upgradeSocket and res variables and the
data/end/error listener functions when removing them.
|
Hey @cirospaciari - I saw you've been working on This fixes the missing The approach uses a raw TCP/TLS socket for upgrade requests instead of going through fetch (which can't handle 101 responses). I tried to keep it consistent with the existing patterns in the file. No rush, just figured you'd be the right person to review this given your recent work in the same area. |
|
Hope this get's merged soon, would love to use Bun with Playwright WS specifically. |
|
@cirospaciari 🙏 🙏 🙏 |
2 participants
Problem
`http.request()` with `Connection: Upgrade` headers never emits the `upgrade` event when the server responds with 101 Switching Protocols. This breaks the `ws` library (and everything depending on it — Playwright, Puppeteer CDP mode, etc.) because they listen for `upgrade` to complete WebSocket handshakes.
Open since April 2024. Fixes #9911, fixes #9357, fixes #17320.
Root cause
`ClientRequest` uses `nodeHttpClient` (fetch-based) internally. The response handler in `_http_client.ts` always emits `response` for every status code, including 101. Fetch doesn't support protocol upgrades, so there's no raw socket to hand off.
The `kUpgradeOrConnect` symbol is imported and initialized to `false` but never set to `true` or used in any logic.
Fix
When a `ClientRequest` has an `Upgrade` header AND `upgrade` event listeners, bypass fetch and use a raw `net.Socket` (or `tls.connect` for HTTPS):
For all other requests (99% of traffic), the existing fast fetch path is unchanged.
Key design decisions
What this unblocks
```javascript
// This now works (was hanging forever):
import { chromium } from 'playwright';
const browser = await chromium.connectOverCDP('http://localhost:9222');
// ws library upgrade event:
const req = http.request({ headers: { Upgrade: 'websocket', Connection: 'Upgrade' } });
req.on('upgrade', (res, socket, head) => { /* now fires */ });
```
Test suggestion
```javascript
import http from 'node:http';
import { WebSocketServer } from 'ws';
const wss = new WebSocketServer({ port: 0 });
wss.on('listening', () => {
const port = wss.address().port;
const req = http.request({
hostname: '127.0.0.1', port,
headers: {
Connection: 'Upgrade', Upgrade: 'websocket',
'Sec-WebSocket-Version': '13',
'Sec-WebSocket-Key': Buffer.from('test-key-12345678').toString('base64'),
}
});
req.on('upgrade', (res, socket, head) => {
console.log('upgrade event received!', res.statusCode); // Should print: 101
socket.end();
wss.close();
});
req.end();
});
```