ws: support upgrade and unexpected-response events#28114
Conversation
|
Updated 7:56 PM PT - Apr 7th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 28114That installs a local version of the PR into your bun-28114 --bun |
|
Found 8 issues this PR may fix:
🤖 Generated with Claude Code |
alii
force-pushed
the
ali/ws-upgrade-events
branch
from
4ea3e91 to
7d24274
Compare
|
@robobun adopt |
|
Build 43967 — fix is green, only darwin infra noise remaining. ASAN (the platform where ws-proxy was SIGILLing): all 20 test shards PASSED. The lazy-register fix in Darwin situation (not my code):
Nothing to push. @alii, the PR is ready for review — the webview darwin failure is infra/flake that predates this PR. |
|
@robobun the gate check had a parsing bug that misread passing tests as "1 FAILED" — fixed. Push an empty commit (or just continue) to re-trigger the check. |
robobun
force-pushed
the
ali/ws-upgrade-events
branch
2 times, most recently
from
10e1c03 to
82ad796
Compare
alii
force-pushed
the
ali/ws-upgrade-events
branch
from
82ad796 to
d3b47ab
Compare
alii
force-pushed
the
ali/ws-upgrade-events
branch
from
cfa87a6 to
0f36bd4
Compare
|
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 native handshake handling and JS plumbing: new C++ span-based Changes
🚥 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: 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 `@test/regression/issue/05951.test.ts`:
- Around line 88-100: The test server currently uses conn.once("data") and
immediately extracts the Sec-WebSocket-Key from the buffer which fails if
headers span multiple packets; change the logic in the createServer callback to
accumulate incoming data on conn.on("data") (using the existing buf variable),
check for the end-of-headers marker "\r\n\r\n" before attempting to parse, only
then run the RegExp to extract the Sec-WebSocket-Key and compute the accept
value, and finally remove the data listener (or switch to once) after handling
the complete request; also defensively check the RegExp result before indexing
[1] to avoid throws.
🪄 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: b90c611c-99a8-4db9-a310-45fa2469599d
📒 Files selected for processing (7)
src/bun.js/bindings/webcore/WebSocket.cppsrc/bun.js/bindings/webcore/WebSocket.hsrc/http/websocket_client/CppWebSocket.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigsrc/js/thirdparty/ws.jstest/regression/issue/05951.test.tstest/regression/issue/24229.test.ts
|
Pushed d479984. The Gate rejection: the farm gate Buildkite CI failures on d479984: The failing test files are almost certainly in Happy to keep iterating if someone can pipe the relevant failing-test names back to me, or if the farm can drop a Buildkite token into the container. |
Addresses review on #28114: - head was a JSString built from UTF-8; switch it to a Node Buffer so the HTTP response head is surfaced as raw bytes, matching body and avoiding a UTF-8 round-trip on headers that are already bytes off the wire. - statusCode/head/body now go through WebCore::builtinNames() instead of Identifier::fromString per call. Added statusCode and head to BunBuiltinNames.h (body was already there). - ws.js shim decodes head as latin1 before parsing (RFC 7230 + node:http convention) and leaves the existing string parser unchanged. - 24229 test asserts head/body are Uint8Array; 05951 upgrade-server reads until the end of headers before extracting Sec-WebSocket-Key so the test doesn't flake on split TCP reads.
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/thirdparty/ws.js`:
- Around line 328-347: Create and store a synthetic http.ClientRequest-like
object on the class (e.g., this._syntheticClientRequest) and ensure
`#ensureHandshakeListener`() always runs so non-101 handshake responses are routed
into `#onHandshake`(); inside `#onHandshake`(statusCode, head, body) build the
handshake response then if statusCode !== 101 set `#unexpectedResponseEmitted` =
true and emit "unexpected-response" with (this._syntheticClientRequest, res)
when listenerCount("unexpected-response") > 0, otherwise emit "error" with the
normalized message; update any places noted (around the handshake listener
registration and lines handling non-101 paths) to reference the synthetic
request and always call `#onHandshake` so behavior matches ws's (request,
response) signature and normalization.
🪄 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: fb339c3c-c4ef-4ad0-abec-5ba8d3e13acf
📒 Files selected for processing (5)
src/bun.js/bindings/webcore/WebSocket.cppsrc/js/builtins/BunBuiltinNames.hsrc/js/thirdparty/ws.jstest/regression/issue/05951.test.tstest/regression/issue/24229.test.ts
Addresses review feedback on #28114: - Collapse the handshake FFI from (head_ptr, head_len, body_ptr, body_len) to (buffer_ptr, buffer_len, head_len). One slice crosses the boundary; C++ splits into head/body spans internally from head_len. - Drop the defensive dupe + re-parse in processWebSocketUpgradeResponse. Instead, move this.body out into a local ArrayList before dispatch — if JS tears us down via ws.close() → clearData() during the sync handshake event, clearData() now finds an empty ArrayList and the backing bytes survive long enough for the post-dispatch processResponse. Single-chunk fast path (body came from uSockets' `data`) is a no-op transfer. - ws.js: synthetic ClientRequest stub for 'unexpected-response' so consumer code that inspects the request (req.method, req.path, req.getHeader) does not crash. Real ws emits (http.ClientRequest, http.IncomingMessage); we bypass node:http so the request is a minimal EventEmitter with no-op header helpers. - 05951 test asserts the stub surface (method='GET', path='/', getHeader() undefined) via the 'unexpected-response' snapshot.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/thirdparty/ws.js (1)
318-323:⚠️ Potential issue | 🟠 MajorKeep non-101 normalization independent of which listeners are attached.
The handshake listener is still only armed from
upgrade/unexpected-response. If a caller only listens toerror, a 503 never reaches#onHandshake()and Bun falls back to the native"Expected 101 status code"error instead of ws'sUnexpected server response: 503. That is still a compatibility gap, and05951.test.tsis snapshotting it now.In the npm `ws` client implementation, when the server returns a non-101 response and there is no 'unexpected-response' listener, what error message is emitted on the WebSocket?Also applies to: 328-405
🤖 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 318 - 323, The code currently arms the native handshake listener only inside ensureHandshakeListener() when listeners for 'upgrade' or 'unexpected-response' are attached, causing non-101 responses to be handled by Bun's native error instead of ws's normalization; modify ensureHandshakeListener() (or call-site logic that registers the native handshake listener) so the handshake listener is registered unconditionally (or at least whenever any user-level listeners like 'error'/'open' may exist) so that non-101 responses always flow into `#onHandshake`() and produce ws's "Unexpected server response: <status>" behavior rather than the native "Expected 101 status code".
🤖 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 87-140: makeHandshakeResponse currently returns a synthetic
Readable that is already ended and incorrectly includes post-header bytes as
HTTP body; change it to construct and return a real http.IncomingMessage (so
callers can consume the live stream) instead of a completed Readable, and for
upgrade responses (statusCode === 101) ensure that any bytes in the head buffer
that follow the header block are not pushed into the HTTP body (they should be
treated as leftover socket data for the upgraded WebSocket), i.e., create an
IncomingMessage tied to a dummy/socket-like object and avoid attaching
post-header bytes to its readable stream when statusCode is 101.
---
Duplicate comments:
In `@src/js/thirdparty/ws.js`:
- Around line 318-323: The code currently arms the native handshake listener
only inside ensureHandshakeListener() when listeners for 'upgrade' or
'unexpected-response' are attached, causing non-101 responses to be handled by
Bun's native error instead of ws's normalization; modify
ensureHandshakeListener() (or call-site logic that registers the native
handshake listener) so the handshake listener is registered unconditionally (or
at least whenever any user-level listeners like 'error'/'open' may exist) so
that non-101 responses always flow into `#onHandshake`() and produce ws's
"Unexpected server response: <status>" behavior rather than the native "Expected
101 status code".
🪄 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: 220cb6c2-bec0-4942-a8c9-ef35710794fa
📒 Files selected for processing (5)
src/bun.js/bindings/webcore/WebSocket.cppsrc/http/websocket_client/CppWebSocket.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigsrc/js/thirdparty/ws.jstest/regression/issue/05951.test.ts
On a 101 Switching Protocols response, any bytes arriving in the same read after the CRLFCRLF are the first WebSocket frame, not HTTP body. Node's http layer delivers those as the `head` buffer on its 'upgrade' event and ws passes them to `setSocket` — never as `res.push()`. Bun's native WebSocket client already forwards buffered post-header bytes to the protocol reader via didConnect, so the correctness of the WebSocket protocol was fine. But makeHandshakeResponse was also pushing those bytes into the synthetic IncomingMessage stream, so reading from `res` on an 'upgrade' listener would surface raw WebSocket frame bytes. Pass null as body to makeHandshakeResponse when statusCode === 101 so the 'upgrade' response stream is cleanly empty.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Addresses review feedback on #28114: - Non-101 responses whose body spans multiple TCP reads were truncated: only the bytes colocated with the headers in the first read made it into the 'unexpected-response' Readable because we dispatched synchronously on the first parse. Now the Zig client defers the handshake dispatch in a new deferred_handshake state until the body is complete (head_len + Content-Length bytes received, or the peer closes when there is no Content-Length). handleEnd flushes any buffered body on connection close. New regression test exercises a three-chunk 7800-byte response body. - #getSyntheticRequest on the ws shim now builds req.path as pathname + search instead of just pathname, matching node's http.ClientRequest.path per RFC 7230 §5.3 — the query string on something like ws://host/path?token=abc no longer disappears from the unexpected-response listener's request object.
Follow-up to the lazy-handshake-listener design: on()/once() went through #onOrOnce which called #ensureHandshakeListener() for 'upgrade' and 'unexpected-response', but the other EventEmitter registration methods fell straight through to the base class and the native 'handshake' event was never armed. Handlers registered via addListener / prependListener / prependOnceListener / addEventListener silently never fired. - addListener aliases on(), routing through #onOrOnce. - prependListener / prependOnceListener call #ensureHandshakeListener() for the two events before delegating to super.prepend*Listener. - addEventListener detects 'upgrade'/'unexpected-response' and registers on the EventEmitter via super.on/once (wrapping the listener in a DOM-style event object) instead of forwarding to the native WebSocket, which never emits those events. Also ref() the HTTPClient around dispatchHandshakeAndProcess so the flush-on-handleEnd path does not UAF when JS calls ws.terminate() inside the handshake listener (handleData / handleDecryptedData already ref, handleEnd did not — moving the guard inside the dispatch helper covers every caller). Regression test covers all four registration APIs plus the bumped timeouts on the existing tests (5s was tight for ASAN debug subprocess spawns).
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/http/websocket_client/WebSocketUpgradeClient.zig:922-941— handleEnd() calls flushDeferredHandshakeAndProcess() without holding an extra ref, so if the user's 'unexpected-response' handler calls ws.close() during the synchronous JS dispatch the refcount can reach zero while still on the call stack, causing a use-after-free when dispatchHandshakeAndProcess reads this.outgoing_websocket after the dispatch returns. Add this.ref() / defer this.deref() at the start of handleEnd's deferred-dispatch branch, matching the guard already present in handleData (line 518) and handleDecryptedData (line 876).Extended reasoning...
handleEnd() in WebSocketUpgradeClient.zig dispatches a synchronous JS event (the "handshake" MessageEvent that triggers "unexpected-response") via flushDeferredHandshakeAndProcess → dispatchHandshakeAndProcess → ws.didReceiveHandshakeResponse(). Both handleData (lines 518-519) and handleDecryptedData (lines 876-877) guard this same call chain with this.ref()/defer this.deref() and handleDecryptedData even carries the explicit comment: "Keep this alive through the synchronous JS dispatch in processWebSocketUpgradeResponse — JS may drop the last ref on us during that call (ws.close())." handleEnd is missing this guard entirely.
The code path that triggers the bug: a server sends a non-101 response with no Content-Length header. The client buffers in the waiting_for_eof state. When the server closes the connection, handleEnd fires and — because deferred_handshake is .waiting_for_eof — calls this.flushDeferredHandshakeAndProcess(this.body.items). That function re-parses the headers and calls dispatchHandshakeAndProcess, which calls ws.didReceiveHandshakeResponse(), the synchronous JS event dispatch.
During that JS dispatch the user's "unexpected-response" handler calls ws.close() (or ws.terminate()), which propagates to cancel(). cancel() does: this.ref() (+1), this.outgoing_websocket = null; this.deref() (-1), then this.tcp.close(.failure). tcp.close may trigger handleClose() synchronously. handleClose() calls this.deref() (-1). cancel's defer this.deref() then runs (-1). If the object started at ref_count 2 (the cpp_websocket ref + the handleEnd frame), the sequence +1 -1 -1 -1 = net -2 reaches zero and frees the object while dispatchHandshakeAndProcess is still on the call stack.
Immediately after ws.didReceiveHandshakeResponse() returns, dispatchHandshakeAndProcess reads: if (this.outgoing_websocket == null) return; — this is a use-after-free because this itself has been freed.
The fix is one line at the top of handleEnd's deferred-dispatch arm: this.ref(); defer this.deref();. This matches the exact pattern used in handleData and handleDecryptedData for the identical reason. The ref keeps this alive across the synchronous JS dispatch and the deref at function exit re-balances when the stack is safe.
Step-by-step proof: (1) Server sends "HTTP/1.1 404 Not Found\r\n\r\nsome body" and then closes TCP. (2) handleData fires, parses headers (no Content-Length), sets deferred_handshake = .waiting_for_eof, appends body bytes to this.body, returns. (3) Server closes connection; handleEnd fires. (4) deferred_handshake is .waiting_for_eof, so handleEnd calls flushDeferredHandshakeAndProcess(this.body.items). (5) dispatchHandshakeAndProcess calls ws.didReceiveHandshakeResponse(404, body, head_len) — synchronous JS dispatch. (6) User's "unexpected-response" handler calls ws.close() → cancel(). cancel: ref(=3), outgoing_websocket=null + deref(=2), tcp.close → handleClose: deref(=1), cancel defer: deref(=0) → object freed. (7) Back in dispatchHandshakeAndProcess: accesses this.outgoing_websocket on freed memory — use-after-free.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js/thirdparty/ws.js:588-607— removeEventListener silently no-ops for 'upgrade'/'unexpected-response' listeners registered via addEventListener: the wrapped arrow function is never stored, and the removal targets the native WebSocket (this.#ws) instead of the JS EventEmitter where those listeners live. Any ws.removeEventListener('upgrade', handler) call after addEventListener('upgrade', handler) leaks the listener permanently.Extended reasoning...
What the bug is
This PR adds a new code path in
addEventListener(ws.js lines 588-607) for 'upgrade' and 'unexpected-response' events. When either event type is detected, the handler wraps the original listener in an anonymous arrow function:const wrapped = (...args) => listener({ type, target: this, data: args }); if (options && options.once) { return super.once(type, wrapped); } return super.on(type, wrapped);
The
wrappedreference is discarded immediately. ThenremoveEventListener(lines 608-610) is unchanged from before the PR:removeEventListener(type, listener) { this.#ws.removeEventListener(type, listener); }
Two compounding failures
-
Wrong target: 'upgrade'/'unexpected-response' listeners are now stored on the JS-side BunWebSocket EventEmitter (
this) viasuper.on/super.once, butremoveEventListenerdelegates tothis.#ws.removeEventListener— the native browser-style WebSocket object. The native WebSocket never had those listeners; the removal silently no-ops. -
Lost wrapper reference: Even if the target were corrected to
this(the EventEmitter), thewrappedarrow function that was actually registered is gone. The EventEmitter internal list holdswrapped, but the caller passes the originallistenertoremoveEventListener. There is no way to match them without a stored map.
Before this PR:
addEventListener('upgrade', cb)forwarded tothis.#ws.addEventListenerandremoveEventListenerforwarded tothis.#ws.removeEventListener— both targeted the same object. The PR broke this symmetry by routing the registration to a different object while leaving removal pointing at the old one.Step-by-step proof
ws.addEventListener('upgrade', handler)is called.#ensureHandshakeListener()arms the native 'handshake' driver.wrapped = (...args) => handler({ type: 'upgrade', target: ws, data: args })is created and registered viasuper.on('upgrade', wrapped)on the BunWebSocket EventEmitter.wrappedgoes out of scope; the only reference to it is inside the EventEmitter internal listener list.ws.removeEventListener('upgrade', handler)is called.- Control reaches
this.#ws.removeEventListener('upgrade', handler)— targeting the native WebSocket. - The native WebSocket has no 'upgrade' listener; removal silently no-ops.
- The EventEmitter still holds
wrapped. Every future 'handshake' event continues to callhandler. The listener is permanently leaked.
Impact
Any code that uses the DOM-style
addEventListener/removeEventListenerpair for 'upgrade' or 'unexpected-response' (e.g., one-time introspection that then cleans up) will accumulate handlers indefinitely. This is a regression introduced by the PR — prior to this change, all event types were symmetrically routed throughthis.#ws.Fix
Store a
WeakMap(keyed by original listener) on the instance. InaddEventListener, recordwrappedMap.set(listener, wrapped)before registering. InremoveEventListener, detect 'upgrade'/'unexpected-response' and callsuper.off(type, wrappedMap.get(listener))then delete the entry, instead of delegating tothis.#ws. -
-
🔴
src/js/thirdparty/ws.js:510-521— prependListener and prependOnceListener call #ensureHandshakeListener() only for 'upgrade'/'unexpected-response', then invoke super.prependListener/super.prependOnceListener directly for all other events, bypassing #onOrOnce(). For standard events ('open', 'close', 'message', 'ping', 'pong', 'error'), the native this.#ws.addEventListener() bridge is set up by #onOrOnce(); without it, any callback registered via prependListener as the sole listener for those events silently never fires. The comment directly above the methods (line 500–505) explicitly states 'Each needs to go through #onOrOnce', and addListener (line 506) was correctly fixed — but prependListener/prependOnceListener were only partially updated.Extended reasoning...
What the bug is
prependListener(event, listener) at lines 510–514 and prependOnceListener at lines 517–521 only call #ensureHandshakeListener() for 'upgrade'/'unexpected-response', then unconditionally call super.prependListener/super.prependOnceListener for all remaining events. For standard ws events ('open', 'close', 'message', 'ping', 'pong', 'error'), the native this.#ws.addEventListener() forwarder that bridges native WebSocket events to the JS EventEmitter is installed by #onOrOnce() — not by the super call. Bypassing #onOrOnce() means the forwarder is never installed for these events when prependListener is the only registration method used.
The specific code path that triggers it
The bit-set guard in #onOrOnce (line ~415) tracks whether a persistent EventEmitter listener has been registered, and it is also where this.#ws.addEventListener('open', ...) etc. are installed. When a caller uses ws.prependListener('open', cb) without any prior ws.on('open', ...) or ws.addListener('open', ...): (1) prependListener calls #ensureHandshakeListener() — no-op since event !== 'upgrade'; (2) super.prependListener pushes cb into EventEmitter's list; (3) #onOrOnce is never reached; (4) this.#ws.addEventListener('open', ...) is never called; (5) when the native WebSocket fires 'open', this.emit('open') is never triggered, so cb silently never fires.
Why existing code doesn't prevent it
The comment block above the three overrides (lines 499–505) explicitly states: "Each needs to go through #onOrOnce so 'upgrade'/'unexpected-response' subscribers lazily arm the native handshake listener". addListener (line 506) was correctly updated to call this.#onOrOnce(event, listener, undefined). But prependListener and prependOnceListener were only half-updated: they handle the handshake events but skip #onOrOnce entirely for every other event, leaving the native bridge uninstalled.
Impact
If a caller registers only via prependListener('open', cb) (or prependOnceListener), the callback silently never executes. In practice most callers use .on() first and then prependListener to insert before existing handlers, so the bridge is usually already installed. But the behavioral inconsistency — addListener('open', cb) works; prependListener('open', cb) alone does not — violates the EventEmitter API contract and will silently break code that relies on insertion ordering for the first handler.
Step-by-step proof
- ws = new WebSocket('ws://...'); ws.prependListener('open', () => console.log('opened'));
- prependListener('open', cb) checks: event === 'upgrade'? No. Calls super.prependListener('open', cb) — cb is in the EventEmitter list.
- #onOrOnce is never called; this.#eventId bit for 'open' remains 0; no this.#ws.addEventListener('open', ...) is installed.
- Native WebSocket fires the open event. Native side calls this.emit('open') only if the forwarder is registered. With no forwarder, this.emit is never called.
- cb never executes. No error or warning is produced.
How to fix it
Route prependListener and prependOnceListener through #onOrOnce for the non-upgrade events, similar to addListener. For example: call this.#onOrOnce(event, () => {}, undefined) to ensure the native bridge is installed, then call super.prependListener(event, listener). Alternatively, extract the bridge-installation logic from #onOrOnce into a helper and call it from prependListener/prependOnceListener before delegating to super.
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/http/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 659-663: The code is forwarding post-header bytes
(body[head_len..]) to handshake listeners on a 101 Switching Protocols response;
change the logic so that when status_code == 101 you do NOT include the bytes
after head_len in the data sent to
didReceiveHandshakeResponse/didReceiveHandshake* handlers — only pass the HTTP
header portion (or an empty body slice) to
dispatchHandshakeAndProcess/didReceiveHandshakeResponse, and keep the
post-header bytes for processResponse() as overflow for the connected WebSocket
client; update both the fast-path at dispatchHandshakeAndProcess and the other
occurrence(s) around the 683-717 range to branch on status_code == 101 and
strip/withhold body[head_len..] when invoking handshake callbacks.
- Around line 625-655: The deferred-handshake buffering needs an upper bound:
introduce a MAX_DEFERRED_HANDSHAKE_BYTES constant and, before any appendSlice or
before accepting a Content-Length target_len, check that the resulting buffered
size (this.body.len + body.len or target_len) does not exceed the limit; if it
would, abort the upgrade/close the connection and do not append (fail fast)
instead of growing unbounded. Apply this guard in both places that call
this.body.appendSlice (the Content-Length branch where you compute target_len
and the EOF branch), and when the limit is exceeded perform the same
abort/cleanup path you use for other fatal errors (do not append and return).
🪄 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: b9b6d89a-d18e-494f-b9dc-bdb96441e7d9
📒 Files selected for processing (8)
src/bun.js/bindings/webcore/EventNames.hsrc/bun.js/bindings/webcore/WebSocket.cppsrc/bun.js/bindings/webcore/WebSocket.hsrc/http/websocket_client/CppWebSocket.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigsrc/js/builtins/BunBuiltinNames.hsrc/js/thirdparty/ws.jstest/regression/issue/24229.test.ts
Addresses three review findings on #28114: 1. Deferred-handshake DoS cap. processWebSocketUpgradeResponse accepts a Content-Length header value directly into the buffer target. A malicious server could send `Content-Length: 18446744073709551615` and make the saturating-add wrap to usize_max, which the length check would never satisfy — every subsequent read would keep appending to this.body until bun.handleOom panicked. Cap at 64 MB. Both the Content-Length branch and the waiting_for_eof branch enforce the limit before every appendSlice via a new appendDeferredHandshakeBody helper that both handleData and handleDecryptedData reuse. Over the cap terminates the upgrade with expected_101_status_code. 2. 101 post-header bytes. On 101 Switching Protocols, bytes after the CRLFCRLF are the first WebSocket frame from the peer — not HTTP body. dispatchHandshakeAndProcess was passing them through didReceiveHandshakeResponse, so listeners on the native 'handshake' event saw protocol bytes as if they were response body. Only processResponse needs them, and only to hand them to the connected client as buffered overflow. Pass an empty body to the handshake dispatch when status_code == 101. 3. Proxy TLS EOF flush. terminate() now flushes a pending deferred_handshake when called with ErrorCode.ended so the wss://-through-HTTP-CONNECT-proxy path (which closes via WebSocketProxyTunnel.onClose → terminate, never through handleEnd) still delivers the 'unexpected-response' event instead of silently dropping the accumulated body. Plus two smaller items: - addListener / prependListener / prependOnceListener / addEventListener route through either #onOrOnce or a new #armNativeBridge helper so the native forwarder is installed for standard events. Previously prependListener/prependOnceListener skipped the bridge setup and the callback silently never fired. - removeEventListener is now symmetric with addEventListener for upgrade/unexpected-response: it calls super.off on the JS EventEmitter instead of forwarding to the native WebSocket (which never had those listeners). The DOM-style wrapper closure is gone so the original listener reference matches on removal. - Drop the unused macro(head) builtin identifier (earlier design iteration that the parsed-rawHeaders rewrite left behind). 05951 grows one 30s-timeout test that exercises all four listener- registration APIs. 24229 still asserts the native handshake payload.
Addresses three more review findings on #28114: - handleClose (the uSockets onClose / TCP RST path) was tearing down via clearData() + dispatchAbruptClose without ever checking deferred_handshake. A server that mid-accumulation RSTs instead of FINs the connection would drop the unexpected-response event entirely. Flush before clearData so the listener sees whatever arrived before the reset. handleEnd and terminate(.ended) already flush; handleClose just needed the same guard. - appendDeferredHandshakeBody.waiting_for_length now caps against target_len (already bounded to head_len + MAX_NON_101_BODY upstream) instead of MAX_NON_101_BODY directly. The accumulated buffer stores both the parsed head and the body, so capping on MAX_NON_101_BODY would terminate head_len bytes short of a response that declared Content-Length: MAX_NON_101_BODY. target_len is the right target. - #armNativeBridge: `1 << undefined` is 1, not 0, so `if (!mask) return` was dead code for unknown event names and would install a spurious noopBridgeListener on bit 0 of #eventId if a caller passed a custom event to prependListener. Guard explicitly on `eventIds[event] === undefined`.
The .waiting_for_length arm of appendDeferredHandshakeBody was terminating with expected_101_status_code whenever a TCP read carried bytes past target_len (TCP coalescing with trailing bytes, pipelined responses, etc.) — suppressing the unexpected-response event entirely. The single-chunk fast path in processWebSocketUpgradeResponse handles the same overflow by truncating to body[0..target_len] and dispatching. The multi-chunk path now does the same: take only target_len - this.body.items.len bytes from the incoming read, append, and flush. Extra bytes are dropped (they belong to the next pipelined response or are garbage). The upstream Content-Length guard already caps target_len at head_len + MAX_NON_101_BODY so unbounded growth is not possible.
3 participants
Adds
'upgrade'and'unexpected-response'events to thewspackage shim. The native WebSocket client now surfaces the handshake response (status, headers, body) to JS instead of discarding it on non-101.This is the load-bearing fix for miniflare/wrangler hanging —
dispatchFetchresolves a promise exclusively from these two events.Fixes #5951
Fixes #24229