fix(node:http): hand off upgrade socket to userland for bidirectional communication#28158
fix(node:http): hand off upgrade socket to userland for bidirectional communication#28158robobun wants to merge 2 commits into
Conversation
… communication After the "upgrade" event was emitted on a node:http server, the TCP connection was not properly handed off to userland. Two issues: 1. socket.write() silently dropped data because kEnableStreaming(true) was never called for upgrade connections (only for CONNECT). 2. The HTTP parser (uWebSockets) continued parsing incoming data as HTTP requests, producing 400 Bad Request for post-upgrade data. Fix by mirroring the CONNECT handler: enable streaming, mark the connection as raw mode (setting isConnectRequest on HttpResponseData to stop HTTP parsing), and return a promise resolved on socket close instead of falling through to the HTTP response lifecycle. Closes #28157 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Updated 9:09 AM PT - Mar 16th, 2026
❌ @autofix-ci[bot], your commit 0f2be2c has 3 failures in
🧪 To try this PR locally: bunx bun-pr 28158That installs a local version of the PR into your bun-28158 --bun |
WalkthroughAdds Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_server.ts`:
- Around line 617-618: The upgrade branch currently emits server.emit("upgrade",
http_req, socket, kEmptyBuffer) which drops any buffered post-header bytes;
change that emit to forward the actual buffered head (use connectHead ??
kEmptyBuffer) so buffered bytes reached userland (match the CONNECT path
behavior) and ensure HttpRequest.headData/headLength are not cleared after
onNodeHTTPRequest/upgrade handling so callers can access the captured bytes;
update the code around server.emit("upgrade", http_req, socket, ...) and any
post-handler clearing logic to preserve headData/headLength.
In `@test/regression/issue/28157.test.ts`:
- Around line 61-64: Remove the in-child 5s watchdog (the setTimeout block that
calls console.error("TIMEOUT") and process.exit(1)) and instead enforce time
limits from the parent test harness: delete the
setTimeout/...console.error/...process.exit(1) block in issue/28157.test.ts and
implement the timeout in the parent test (e.g., use the test framework's timeout
API or the parent spawn/childProcess timeout options and give a larger overall
timeout) so the parent kills the child on timeout rather than the child
self-aborting.
- Around line 33-40: The test currently sends the protocol payload only after
parsing the 101 response, so it never exercises the buffered-head code path;
update the net.connect/client.write call in the test to append the first
protocol payload (e.g. "hello from client") directly to the initial HTTP request
bytes so the server receives headers and protocol data in the same read, and
make the same change for the other occurrence noted (lines 49-52). Also ensure
the server-side behavior preserves HttpRequest.headData and
HttpRequest.headLength after requestHandler returns (do not clear them) so
assertions about buffered head data remain valid; reference the
net.connect/client.write call in the test and the
HttpRequest.headData/headLength and requestHandler symbols when making these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 208a76b8-1591-4b40-a3a0-2476da2d4193
📒 Files selected for processing (6)
packages/bun-uws/src/HttpResponse.hsrc/bun.js/bindings/node/JSNodeHTTPServerSocketPrototype.cppsrc/deps/libuwsockets.cppsrc/deps/uws/Response.zigsrc/js/node/_http_server.tstest/regression/issue/28157.test.ts
| server.emit("upgrade", http_req, socket, kEmptyBuffer); | ||
| if (!socket._httpMessage) { | ||
| if (canUseInternalAssignSocket) { | ||
| // ~10% performance improvement in JavaScriptCore due to avoiding .once("close", ...) and removing a listener | ||
| assignSocketInternal(http_res, socket); | ||
| } else { | ||
| http_res.assignSocket(socket); | ||
| } | ||
| } | ||
| return upgradePromise; |
There was a problem hiding this comment.
Don't drop buffered post-header bytes in the upgrade path.
connectHead is already threaded into onNodeHTTPRequest, but this branch always emits kEmptyBuffer. If the parser has already captured bytes beyond \r\n\r\n, they never reach userland here. Forward connectHead ?? kEmptyBuffer, like the CONNECT path does.
Suggested change
- server.emit("upgrade", http_req, socket, kEmptyBuffer);
+ const head = connectHead ?? kEmptyBuffer;
+ server.emit("upgrade", http_req, socket, head);
return upgradePromise;Based on learnings, headData and headLength on HttpRequest are intentionally left populated for all request types and should not be cleared after the requestHandler call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/node/_http_server.ts` around lines 617 - 618, The upgrade branch
currently emits server.emit("upgrade", http_req, socket, kEmptyBuffer) which
drops any buffered post-header bytes; change that emit to forward the actual
buffered head (use connectHead ?? kEmptyBuffer) so buffered bytes reached
userland (match the CONNECT path behavior) and ensure
HttpRequest.headData/headLength are not cleared after onNodeHTTPRequest/upgrade
handling so callers can access the captured bytes; update the code around
server.emit("upgrade", http_req, socket, ...) and any post-handler clearing
logic to preserve headData/headLength.
| const client = net.connect(port, "127.0.0.1", () => { | ||
| client.write( | ||
| "GET / HTTP/1.1\\r\\n" + | ||
| "Host: 127.0.0.1\\r\\n" + | ||
| "Upgrade: custom-protocol\\r\\n" + | ||
| "Connection: Upgrade\\r\\n" + | ||
| "\\r\\n" | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cover the buffered-head path in this regression.
The client only sends hello from client after it has parsed the 101 response, so this test never exercises bytes that arrive in the same read as the upgrade request. Sending the first protocol payload together with the HTTP headers would catch a dropped head handoff too.
Based on learnings, headData and headLength on HttpRequest are intentionally left populated for all request types and should not be cleared after the requestHandler call.
Also applies to: 49-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/28157.test.ts` around lines 33 - 40, The test currently
sends the protocol payload only after parsing the 101 response, so it never
exercises the buffered-head code path; update the net.connect/client.write call
in the test to append the first protocol payload (e.g. "hello from client")
directly to the initial HTTP request bytes so the server receives headers and
protocol data in the same read, and make the same change for the other
occurrence noted (lines 49-52). Also ensure the server-side behavior preserves
HttpRequest.headData and HttpRequest.headLength after requestHandler returns (do
not clear them) so assertions about buffered head data remain valid; reference
the net.connect/client.write call in the test and the
HttpRequest.headData/headLength and requestHandler symbols when making these
changes.
| setTimeout(() => { | ||
| console.error("TIMEOUT"); | ||
| process.exit(1); | ||
| }, 5000); |
There was a problem hiding this comment.
Move the watchdog out of the child process.
This fixed 5s setTimeout makes the regression time-based and can race the parent test timeout on slow CI. Prefer enforcing the timeout from the parent side and giving the overall test more slack than the child watchdog.
Based on learnings, spawned parent/child tests need explicit timeouts because the default 5000ms is often insufficient; as per coding guidelines, "Do not use setTimeout in tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/28157.test.ts` around lines 61 - 64, Remove the
in-child 5s watchdog (the setTimeout block that calls console.error("TIMEOUT")
and process.exit(1)) and instead enforce time limits from the parent test
harness: delete the setTimeout/...console.error/...process.exit(1) block in
issue/28157.test.ts and implement the timeout in the parent test (e.g., use the
test framework's timeout API or the parent spawn/childProcess timeout options
and give a larger overall timeout) so the parent kills the child on timeout
rather than the child self-aborting.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } else if (is_upgrade) { | ||
| // Hand off the connection to userland, mirroring CONNECT handler. | ||
| // Enable streaming so socket.write() and socket.on("data") work. | ||
| socket[kEnableStreaming](true); | ||
| // Tell uWebSockets to stop parsing HTTP on this connection, | ||
| // switching to raw pass-through mode for bidirectional data. | ||
| socketHandle.markAsRawMode(); | ||
| const { promise: upgradePromise, resolve: upgradeResolve } = $newPromiseCapability(Promise); | ||
| socket.once("close", upgradeResolve); | ||
| server.emit("upgrade", http_req, socket, kEmptyBuffer); | ||
| if (!socket._httpMessage) { | ||
| if (canUseInternalAssignSocket) { | ||
| // ~10% performance improvement in JavaScriptCore due to avoiding .once("close", ...) and removing a listener | ||
| assignSocketInternal(http_res, socket); | ||
| } else { | ||
| http_res.assignSocket(socket); | ||
| } | ||
| } | ||
| return upgradePromise; |
There was a problem hiding this comment.
🔴 The upgrade handler (line 608) unconditionally calls socket[kEnableStreaming](true), socketHandle.markAsRawMode(), and returns a promise without first checking server.listenerCount("upgrade") > 0. If a client sends a request with an Upgrade header but the server has no "upgrade" listeners, the socket is permanently put in raw mode (HTTP parser disabled) with no handler to read or write it, leaving it hanging until timeout. The fix should mirror the CONNECT handler at line 538: check server.listenerCount("upgrade") > 0 before entering raw mode, and destroy the socket if no listeners exist.
Extended reasoning...
What the bug is
The new upgrade handler at lines 608-618 unconditionally enters raw mode and returns a promise, without checking whether any "upgrade" event listeners are registered on the server. This is in contrast to the CONNECT handler at line 538, which correctly guards with server.listenerCount("connect") > 0 before doing the same operations.
How it manifests
When a client sends an HTTP request with an Upgrade header (e.g., Upgrade: custom-protocol or Upgrade: websocket) to a server that has NO "upgrade" event listener, the following sequence occurs:
is_upgradeis truthy at line 588 (it checkshttp_req.headers.upgrade, which is the header value)socket[kEnableStreaming](true)is called at line 611socketHandle.markAsRawMode()is called at line 614, which setsisConnectRequest = trueon the uWebSocketsHttpResponseData, permanently disabling the HTTP parser for this connection- A promise is created that only resolves when the socket closes (line 615-616)
server.emit("upgrade", ...)fires at line 617, but since there are no listeners, this is a no-op- The function returns
upgradePromiseat line 618, which never resolves because nothing closes the socket
Why existing code doesn't prevent it
The is_upgrade check at line 588 only tests whether the Upgrade header is present in the request — it does NOT check whether the server has any "upgrade" event listeners. The CONNECT handler correctly separates these concerns: it checks the method (method === "CONNECT") and then separately checks server.listenerCount("connect") > 0. The upgrade handler skips the listener count check entirely.
Impact
The socket is left in an irrecoverable state: the HTTP parser is permanently disabled via markAsRawMode(), so even if the connection could somehow continue, it cannot parse any further HTTP. The socket will hang until the idle timeout fires (or indefinitely if timeout is set to 0, which is the default for this server). This is also a potential resource exhaustion vector — an attacker could send many requests with Upgrade headers to a server without upgrade listeners, tying up connections.
Step-by-step proof
- Create an HTTP server with
http.createServer()— no "upgrade" listener attached - A client sends:
GET / HTTP/1.1\r\nHost: localhost\r\nUpgrade: custom\r\nConnection: Upgrade\r\n\r\n is_upgradeevaluates to"custom"(truthy) at line 588- Code enters the
else if (is_upgrade)branch at line 608 markAsRawMode()permanently disables the HTTP parserserver.emit("upgrade", ...)is a no-op — nobody is listening- The returned promise never resolves because nothing writes to or closes the socket
- The socket hangs in raw mode, consuming a connection slot
How to fix
Mirror the CONNECT handler pattern. Before entering raw mode, check server.listenerCount("upgrade") > 0. If no listeners exist, destroy the socket (matching Node.js behavior) or close it via socketHandle.close().
| server.emit("upgrade", http_req, socket, kEmptyBuffer); | ||
| if (!socket._httpMessage) { | ||
| if (canUseInternalAssignSocket) { | ||
| // ~10% performance improvement in JavaScriptCore due to avoiding .once("close", ...) and removing a listener | ||
| assignSocketInternal(http_res, socket); | ||
| } else { | ||
| http_res.assignSocket(socket); | ||
| } | ||
| } | ||
| return upgradePromise; |
There was a problem hiding this comment.
🔴 The upgrade handler at line 617 always passes kEmptyBuffer as the head argument to server.emit("upgrade", ...), discarding any buffered post-header bytes (connectHead) that arrived in the same TCP segment as the upgrade request. The CONNECT handler at line 546 correctly forwards connectHead ?? kEmptyBuffer. The fix is to change line 617 to: server.emit("upgrade", http_req, socket, connectHead ?? kEmptyBuffer).
Extended reasoning...
What the bug is
The onNodeHTTPRequest callback receives connectHead as its last parameter (line 524), which contains any bytes the HTTP parser found beyond the \r\n\r\n header terminator in the same TCP read. Per the Node.js "upgrade" event contract, the third argument (head) should be a Buffer containing the first packet of the upgraded protocol stream.
The inconsistency
The CONNECT handler at line 546 correctly forwards this buffer:
const head = connectHead ? connectHead : kEmptyBuffer;
server.emit("connect", http_req, socket, head);But the upgrade handler at line 617 ignores connectHead entirely:
server.emit("upgrade", http_req, socket, kEmptyBuffer);Why the data is permanently lost
The C++ layer (NodeHTTP.cpp lines 477-484) populates request->head for ALL request types (not just CONNECT) with any data found after the headers in the same read buffer. The comment in the C++ code explicitly says: "Pass pipelined data (head buffer) for Node.js compat (connect/upgrade events)". Once markAsRawMode() is called (line 614), the HTTP parser stops owning the connection. The pipelined bytes have already been consumed from the TCP buffer by the parser, so they will never be re-delivered through the raw socket data path.
Step-by-step proof
- A client sends a single TCP segment containing:
GET / HTTP/1.1\r\nUpgrade: custom\r\nConnection: Upgrade\r\n\r\nHELLO - The HTTP parser finds headers up to
\r\n\r\nand storesHELLOinrequest->head onNodeHTTPRequestis called withconnectHead=Buffer("HELLO")- The code enters the
else if (is_upgrade)branch at line 608 markAsRawMode()disables further HTTP parsing on this connection- Line 617 emits
server.emit("upgrade", http_req, socket, kEmptyBuffer)— theHELLObytes are dropped - The upgrade event listener receives an empty buffer as
headinstead ofBuffer("HELLO") - Since the parser already consumed those bytes and raw mode is now active,
HELLOis permanently lost
Impact
Any protocol that pipelines initial data with the HTTP upgrade request (which is common in WebSocket and other upgrade protocols) will silently lose those bytes. The existing regression test doesn't catch this because it sends the protocol payload only after receiving the 101 response.
Fix
Change line 617 from:
server.emit("upgrade", http_req, socket, kEmptyBuffer);to:
server.emit("upgrade", http_req, socket, connectHead ?? kEmptyBuffer);| } else if (is_upgrade) { | ||
| // Hand off the connection to userland, mirroring CONNECT handler. | ||
| // Enable streaming so socket.write() and socket.on("data") work. | ||
| socket[kEnableStreaming](true); | ||
| // Tell uWebSockets to stop parsing HTTP on this connection, | ||
| // switching to raw pass-through mode for bidirectional data. | ||
| socketHandle.markAsRawMode(); |
There was a problem hiding this comment.
🟡 When an upgrade request has a body (hasBody=true), handle.pause() at line 566 pauses the OS socket before markAsRawMode() at line 614 sets isConnectRequest=true. Later, resumeSocket() in Zig checks isConnectRequest() which is now true and returns early without calling us_socket_resume — the OS-level pause is never cleared, making the connection permanently unreadable. The CONNECT handler avoids this by returning at line 548 before handle.pause(). Fix: move the upgrade check before handle.pause() (like CONNECT) or explicitly resume after markAsRawMode().
Extended reasoning...
What the bug is
The new upgrade handler at lines 608-618 runs after the if (hasBody) { handle.pause(); } check at line 565-566. When an upgrade request includes a body (Content-Length > 0 or Transfer-Encoding present), the OS-level socket is paused via handle.pause() → doPause() → pauseSocket() (NodeHTTPResponse.zig:124). At this point, isConnectRequest() is still false, so the guard at line 126 passes and the OS socket is paused.
Then at line 614, socketHandle.markAsRawMode() sets isConnectRequest = true on the uWebSockets HttpResponseData.
Why the socket can never be unpaused
When userland calls socket.resume(), it flows through #resumeSocket() → response.resume() → doResume() → resumeSocket() (NodeHTTPResponse.zig:133). At line 135, the guard checks this.raw_response.?.isConnectRequest() — which is now true — and returns early WITHOUT calling this.raw_response.?.resume(). The OS-level socket pause is never cleared. No data can be received on the connection.
Why the CONNECT handler is unaffected
The CONNECT handler correctly avoids this issue because it returns at line 548, before handle.pause() at line 565. The upgrade handler at line 608 runs after the pause, creating a window where the socket is OS-level paused and then markAsRawMode() prevents it from ever being unpaused.
Step-by-step proof with a concrete example
- Client sends:
POST / HTTP/1.1\r\nHost: localhost\r\nUpgrade: custom-protocol\r\nConnection: Upgrade\r\nContent-Length: 5\r\n\r\nhello hasBodyistrue(Content-Length > 0), so line 566 callshandle.pause()→pauseSocket()at Zig:124 →isConnectRequest()isfalse→ OS socket is paused viaus_socket_pause.is_upgradeis truthy ("custom-protocol"), entering theelse if (is_upgrade)branch at line 608.- Line 614:
socketHandle.markAsRawMode()setsisConnectRequest = true. - Server handler calls
socket.resume()→#resumeSocket()→resumeSocket()at Zig:133 → guard at line 135 checksisConnectRequest()→true→ returns early. OS socket stays paused. - The connection is permanently unreadable.
Impact
This is a nit/edge case because standard WebSocket upgrades use GET without a body (hasBody=false), so handle.pause() is never called and the socket works fine. Only custom protocol upgrades using POST/PUT with bodies would trigger this. However, for those cases, the connection becomes completely non-functional after the upgrade.
Suggested fix
Either (a) move the upgrade check before handle.pause() at line 565 — mirroring how the CONNECT handler returns at line 548 before the pause — or (b) explicitly resume the OS socket in the upgrade handler after markAsRawMode(), e.g. by calling handle.resume() before markAsRawMode() or directly after it.
|
Superseded by #30664 — same CONNECT-mirroring approach but keeps the diff to |
1 participant
Summary
node:httpserver upgrade socket not being handed off to userland after the"upgrade"event, causingsocket.write()to silently drop data and post-upgrade client data to get 400 Bad RequestRoot Cause
Two issues in
src/js/node/_http_server.ts:Write side:
socket[kEnableStreaming](false)was called for all non-CONNECT requests (line 555), but the upgrade handler never re-enabled it. This lefthandle.ondrainas undefined, causing_write()to silently skiphandle.write().Read side: The upgrade handler didn't return early — it fell through to
socket.cork()and the HTTP response promise. More critically, uWebSockets' HTTP parser continued owning the connection, parsing post-upgrade data as new HTTP requests (→ 400 Bad Request).Fix
Mirror the existing CONNECT handler pattern:
socket[kEnableStreaming](true)before emitting the upgrade eventsocketHandle.markAsRawMode()to setisConnectRequeston the uWebSocketsHttpResponseData, switching the HTTP parser to raw pass-through modeThe
markAsRawMode()method is new — exposed through the full stack:HttpResponse.h→libuwsockets.cpp→Response.zig→JSNodeHTTPServerSocketprototype.Test plan
test/regression/issue/28157.test.tsverifies bidirectional communication after upgradetest/js/web/fetch/fetch.upgrade.test.tspassestest/js/node/http/node-http-with-ws.test.tspasses (WebSocket over node:http)test/js/node/http/node-http-connect.test.tsCONNECT raw socket test passesCloses #28157
🤖 Generated with Claude Code