websocket: release the proxy tunnel when its TLS handshake fails inside start()

[robobun]

Symptom

Every wss:// connection attempt through an HTTP(S) proxy whose 200 Connection Established reply carries bytes that fail the tunnel's TLS handshake immediately leaks the WebSocketProxyTunnel, its SSLWrapper and the SSL_CTX it had just built (about 27 KB of RSS per attempt on a release build, 500 attempts = +13 MB). A proxy that appends a TLS alert is the minimal trigger; a captive portal or misbehaving proxy that tacks an HTML body onto its 200 leaks the same way. A client that retries against such a proxy grows without bound. The close event the user sees is the expected one (1015 TLS handshake failed), so nothing hints at the leak.

import { sslCtxLiveCount } from "bun:internal-for-testing";
import net from "node:net";

// CONNECT proxy that answers 200 and a fatal handshake_failure alert in one write
const proxy = net.createServer(s => s.once("data", () =>
  s.end(Buffer.concat([Buffer.from("HTTP/1.1 200 Connection Established\r\n\r\n"),
                       Buffer.from([0x15, 0x03, 0x03, 0x00, 0x02, 0x02, 0x28])]))));
await new Promise(r => proxy.listen(0, "127.0.0.1", r));

const attempt = () => new Promise(resolve => {
  const ws = new WebSocket("wss://tunnel-target.invalid/", { proxy: `http://127.0.0.1:${proxy.address().port}` });
  ws.onclose = e => resolve(e.code); // 1015 every time
});

const before = sslCtxLiveCount();
for (let i = 0; i < 10; i++) await attempt();
console.log(sslCtxLiveCount() - before); // before: 10, after: 0

If the proxy sends the same alert in a later packet instead, nothing leaks; only the synchronous case is affected.

Cause

start_proxy_tls_handshake() in src/http_jsc/websocket_client/WebSocketUpgradeClient.rs allocates the tunnel with WebSocketProxyTunnel::init() (refcount 1, owned by nobody yet), calls WebSocketProxyTunnel::start(tunnel, ..., initial_data) and only afterwards stores it with p.set_tunnel(Some(tunnel)), at which point WebSocketProxy's Drop becomes responsible for releasing it.

start() feeds initial_data (whatever followed the proxy's response head in the same read) to the SSLWrapper before returning. A fatal alert or non-TLS bytes fail the handshake right there: on_handshake(false) -> terminate(TlsHandshakeFailed) -> fail() -> tcp.close() -> handle_close() -> clear_data() takes proxy. Back in start_proxy_tls_handshake() the proxy re-check sees None and returns, but unlike the start() error branch next to it, it never released the ref init() handed out. The upgrade client itself survives (handle_data holds a ref guard), so this is a leak rather than a use after free. The fetch proxy tunnel (src/http/ProxyTunnel.rs) stores its tunnel before driving the wrapper and was checked to not leak in the same scenario.

Fix

The ref returned by init() is now held in a bun_ptr::ScopedRef::adopt guard for the rest of the function and forget()-ed at the one point where ownership moves to the proxy (set_tunnel). Both early returns (the start() error and the proxy re-check) release it on the way out, and the manual deref in the error branch goes away.

Driving start() under the caller's own ref rather than storing the tunnel in the proxy first is deliberate: the callbacks above drop proxy while SSLWrapper::receive_data for that very tunnel is still on the stack, so a proxy-owned ref would be the last one and clear_data() would free the wrapper out from under itself. The asynchronous path (WebSocketProxyTunnel::receive) guards against exactly this with its own ScopedRef; keeping the ref in the caller gives start() the same guarantee. No other behaviour changes: the tunnel is freed only after start() has returned, terminate() on the already failed client is a no-op (its WebSocket was detached and the socket closed by the callback), and on success the proxy owns the tunnel exactly as before.

Verification

test/js/web/websocket/websocket-proxy.test.ts gains a test.each over {http proxy, https proxy} x {fatal alert, non-TLS bytes}. The http variants use no TLS options (the tunnel builds its defaults), the https variants pass tls: { rejectUnauthorized: false } (the tunnel clones the user's config), so both upgrade client instantiations and both option branches are covered. Each case asserts that the attempts actually went through the proxy (CONNECT tunnel-target.invalid:443 HTTP/1.1 per attempt), that every attempt closes with 1015 TLS handshake failed, and that sslCtxLiveCount() did not grow across 5 attempts after a warm-up one.

• unfixed (release binary and a debug build with src/ stashed): all four cases fail with Expected: <= 0, Received: 5
• fixed debug build: all four pass (110 to 360 ms each under ASAN); the full file passes with --rerun-each 5
• websocket-proxy-tunnel-{upgrade,client}-leak, websocket-proxy-close-reentrancy, test-ws-bidir-proxy, first_party/ws/ws-proxy and websocket.test.js / websocket-client.test.ts pass on the fixed build (the only failures in this sandbox are the tests that need the public network or an environment without an ambient NO_PROXY, and they fail identically on the unmodified binary)

Found while working on #37487, which fixes a different teardown bug in the same area and does not touch this function.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: de281dbc-1475-46f0-b759-46b8a70ef883

📥 Commits

Reviewing files that changed from the base of the PR and between da3851e and be36173.

📒 Files selected for processing (2)

• src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
• test/js/web/websocket/websocket-proxy.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 6:06 AM PT - Aug 11th, 2026}

✅ @robobun, your commit be36173a4bb9bf7396aab95fcf7a2d2ef4cafec2 passed in Build #92257! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37504

That installs a local version of the PR into your bun-37504 executable, so you can run:

bun-37504 --bun

[robobun]

Status: fix and regression test pushed, CI green (build 92257), ready for review.

Reproduced on the released binary (1.4.0-canary.1+9008ae7ab) and on a debug build with the fix stashed: a CONNECT proxy that appends a fatal TLS alert (or an HTML body) to its 200 makes every wss:// attempt through it leak one SSL_CTX (sslCtxLiveCount() grows by exactly one per attempt, roughly 27 KB RSS each). The new cases in test/js/web/websocket/websocket-proxy.test.ts fail unfixed with Expected: <= 0, Received: 5 for http and https proxies alike, and pass with this diff.

[claude]

I reviewed this PR and didn't find any bugs. The ScopedRef::adopt/forget guard correctly covers all three exit paths and the test proves the leak on the unfixed build. Because this is unsafe refcount lifetime code around the TLS tunnel wrapper, a human look would still be worthwhile.

What was reviewed:

• ScopedRef::adopt does not bump the count and forget defuses via ManuallyDrop, so the success path still hands exactly one ref to set_tunnel and WebSocketProxy::drop releases it as before.
• The start().is_err() branch now derefs after terminate instead of before; the tunnel is not stored anywhere terminate reaches at that point, so the reorder is safe.
• On the leaking path start() has already returned before the guard drops, so the SslWrapper is no longer on the stack when the tunnel is freed; the callbacks' own ScopedRef::new guards keep the count ≥ 2 while they run.

Extended reasoning...

Overview

The PR wraps the raw WebSocketProxyTunnel pointer returned by init() in a bun_ptr::ScopedRef::adopt guard inside HTTPClient::<SSL>::start_proxy_tls_handshake, removes the manual deref in the start().is_err() branch, and forget()s the guard immediately before p.set_tunnel(Some(tunnel)). Net effect: the previously-unreleased ref on the "proxy was taken during start()" early return is now released by the guard's Drop. A four-case test.each in websocket-proxy.test.ts exercises {http, https} proxy × {fatal alert, non-TLS bytes} appended to the CONNECT 200, asserting the exact close event and that sslCtxLiveCount() does not grow across five attempts after a warm-up.

Security risks

The change sits in the TLS proxy tunnel setup path but does not alter validation, certificate handling, or handshake logic — it only changes when the tunnel's single intrusive ref is released on failure. I verified ScopedRef::adopt (src/ptr/ref_count.rs:992) takes over the existing ref without bumping and forget (src/ptr/ref_count.rs:1000) wraps in ManuallyDrop, so the success path's refcount is unchanged (1 handed to WebSocketProxy, released in its Drop at src/http_jsc/websocket_client/WebSocketProxy.rs:65). No new UAF surface: by the time the guard drops on the leaking path, start() has returned and the SslWrapper borrow inside it is gone; the callbacks (on_open, on_data, on_handshake) each hold their own ScopedRef::new so the count cannot hit zero mid-callback. The reorder in the is_err() branch (deref now runs after terminate via drop-at-return) is safe because the tunnel has not been stored in proxy yet, so terminate → clear_data cannot reach it.

Level of scrutiny

High. This is unsafe intrusive-refcount code on a networking/TLS path — exactly the "reference counts provably balanced on every terminal path" category REVIEW.md flags as most-blocked. The fix itself is the textbook RAII pattern the review guide prescribes (arm a Drop guard before the fallible call, disarm at the ownership-transfer point), and the diff is small and local, but a maintainer familiar with the WebSocketProxyTunnel/SSLWrapper re-entrancy model should confirm the analysis.

Other factors

The test is well-constructed: it asserts the CONNECT request line to prove the proxy path was actually taken, asserts the exact {code: 1015, reason: "TLS handshake failed", wasClean: false} on every attempt, uses a warm-up attempt to absorb once-per-process allocations, and cleans up via try/finally. The <= 0 bound (rather than === 0) is explained inline and is well below the unfixed value of 5. The PR description documents that all four cases fail on the unfixed build with Received: 5 and pass on the fix, and that the neighboring proxy-tunnel leak/reentrancy tests still pass. sslCtxLiveCount is an existing bun:internal-for-testing export already used by other leak tests in the suite.