From c57af9df38c9bb8fee1145f370fd565db9957ca8 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Fri, 13 Feb 2026 14:49:40 -0800 Subject: [PATCH] Fix websocket proxy ping crash (#26995) ### What does this PR do? The `sendPong` fix alone wasn't sufficient. The bug only manifests with **wss:// through HTTP proxy** (not ws://), because only that path uses `initWithTunnel` with a detached socket. **Two bugs were found and fixed:** 1. **`sendPong`/`sendCloseWithBody` socket checks** (`src/http/websocket_client.zig`): Replaced `socket.isClosed() or socket.isShutdown()` with `!this.hasTCP()` as originally proposed. Also guarded `shutdownRead()` against detached sockets. 2. **Spurious 1006 during clean close** (`src/http/websocket_client.zig` + `WebSocketProxyTunnel.zig`): When `sendCloseWithBody` calls `clearData()`, it shuts down the proxy tunnel. The tunnel's `onClose` callback was calling `ws.fail(ErrorCode.ended)` which dispatched a 1006 abrupt close, overriding the clean 1000 close already in progress. Fixed by adding `tunnel.clearConnectedWebSocket()` before `tunnel.shutdown()` so the callback is a no-op. ### How did you verify your code works? - `USE_SYSTEM_BUN=1`: Fails with `Unexpected close code: 1006` - `bun bd test`: Passes with clean 1000 close - Full proxy test suite: 25 pass, 4 skip, 0 fail - Related fragmented/close tests: all passing --------- Co-authored-by: Claude Bot Co-authored-by: Claude --- src/http/websocket_client.zig | 13 +++- .../websocket_client/WebSocketProxyTunnel.zig | 7 ++ test/js/web/websocket/websocket-proxy.test.ts | 65 +++++++++++++++++++ 3 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/http/websocket_client.zig b/src/http/websocket_client.zig index 0d9187efdf..8be16a5d42 100644 --- a/src/http/websocket_client.zig +++ b/src/http/websocket_client.zig @@ -138,6 +138,10 @@ pub fn NewWebSocketClient(comptime ssl: bool) type { // Set to null FIRST to prevent re-entrancy (shutdown can trigger callbacks) if (this.proxy_tunnel) |tunnel| { this.proxy_tunnel = null; + // Detach the websocket from the tunnel before shutdown so the + // tunnel's onClose callback doesn't dispatch a spurious 1006 + // after we've already handled a clean close. + tunnel.clearConnectedWebSocket(); tunnel.shutdown(); tunnel.deref(); } @@ -910,7 +914,7 @@ pub fn NewWebSocketClient(comptime ssl: bool) type { } fn sendPong(this: *WebSocket, socket: Socket) bool { - if (socket.isClosed() or socket.isShutdown()) { + if (!this.hasTCP()) { this.dispatchAbruptClose(ErrorCode.ended); return false; } @@ -942,14 +946,17 @@ pub fn NewWebSocketClient(comptime ssl: bool) type { body_len: usize, ) void { log("Sending close with code {d}", .{code}); - if (socket.isClosed() or socket.isShutdown()) { + if (!this.hasTCP()) { this.dispatchAbruptClose(ErrorCode.ended); this.clearData(); return; } // we dont wanna shutdownRead when SSL, because SSL handshake can happen when writting + // For tunnel mode, shutdownRead on the detached socket is a no-op; skip it. if (comptime !ssl) { - socket.shutdownRead(); + if (this.proxy_tunnel == null) { + socket.shutdownRead(); + } } var final_body_bytes: [128 + 8]u8 = undefined; var header = @as(WebsocketHeader, @bitCast(@as(u16, 0))); diff --git a/src/http/websocket_client/WebSocketProxyTunnel.zig b/src/http/websocket_client/WebSocketProxyTunnel.zig index 18b6e8c919..be23ce0322 100644 --- a/src/http/websocket_client/WebSocketProxyTunnel.zig +++ b/src/http/websocket_client/WebSocketProxyTunnel.zig @@ -253,6 +253,13 @@ pub fn setConnectedWebSocket(this: *WebSocketProxyTunnel, ws: *WebSocketClient) this.#upgrade_client = .{ .none = {} }; } +/// Clear the connected WebSocket reference. Called before tunnel shutdown during +/// a clean close so the tunnel's onClose callback doesn't dispatch a spurious +/// abrupt close (1006) after the WebSocket has already sent a clean close frame. +pub fn clearConnectedWebSocket(this: *WebSocketProxyTunnel) void { + this.#connected_websocket = null; +} + /// SSLWrapper callback: Called with encrypted data to send to network fn writeEncrypted(this: *WebSocketProxyTunnel, encrypted_data: []const u8) void { log("writeEncrypted: {} bytes", .{encrypted_data.len}); diff --git a/test/js/web/websocket/websocket-proxy.test.ts b/test/js/web/websocket/websocket-proxy.test.ts index c2b5a6e1c3..ae1f5b759d 100644 --- a/test/js/web/websocket/websocket-proxy.test.ts +++ b/test/js/web/websocket/websocket-proxy.test.ts @@ -398,6 +398,71 @@ describe("WebSocket wss:// through HTTP proxy (TLS tunnel)", () => { expect(messages).toContain("hello via tls tunnel"); gc(); }); + + test("server-initiated ping survives through TLS tunnel proxy", async () => { + // Regression test: sendPong checked socket.isClosed() on the detached tcp + // field instead of using hasTCP(). For wss:// through HTTP proxy, the + // WebSocket uses initWithTunnel which sets tcp = detached (all I/O goes + // through proxy_tunnel). Detached sockets return true for isClosed(), so + // sendPong would immediately dispatch a 1006 close instead of sending the + // pong through the tunnel. + using pingServer = Bun.serve({ + port: 0, + tls: { + key: tlsCerts.key, + cert: tlsCerts.cert, + }, + fetch(req, server) { + if (server.upgrade(req)) return; + return new Response("Expected WebSocket", { status: 400 }); + }, + websocket: { + message(ws, message) { + if (String(message) === "ready") { + // Send a ping after the client confirms it's connected. + // On the buggy code path, this triggers sendPong on the detached + // socket → dispatchAbruptClose → 1006. + ws.ping(); + // Follow up with a text message. If the client receives this, + // the connection survived the ping/pong exchange. + ws.send("after-ping"); + } + }, + }, + }); + + const { promise, resolve, reject } = Promise.withResolvers(); + + const ws = new WebSocket(`wss://127.0.0.1:${pingServer.port}`, { + proxy: `http://127.0.0.1:${proxyPort}`, + tls: { rejectUnauthorized: false }, + }); + + ws.onopen = () => { + ws.send("ready"); + }; + + ws.onmessage = event => { + if (String(event.data) === "after-ping") { + ws.close(1000); + } + }; + + ws.onclose = event => { + if (event.code === 1000) { + resolve(); + } else { + reject(new Error(`Unexpected close code: ${event.code}`)); + } + }; + + ws.onerror = event => { + reject(event); + }; + + await promise; + gc(); + }); }); describe("WebSocket through HTTPS proxy (TLS proxy)", () => {