mirror of
https://github.com/oven-sh/bun
synced 2026-02-17 06:12:08 +00:00
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 <claude-bot@bun.sh> Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -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)));
|
||||
|
||||
@@ -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});
|
||||
|
||||
@@ -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<void>();
|
||||
|
||||
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)", () => {
|
||||
|
||||
Reference in New Issue
Block a user