ServerWebSocket: convert string arguments before the closed check

[Jarred-Sumner]

What

ServerWebSocket.subscribe/unsubscribe/isSubscribed/send/sendText/ping/pong checked is_closed() and then converted their argument with toString(), which can run user JS that terminates/closes the socket. The uWS call then ran against a closed socket; subscribe left a dangling subscriber in the TopicTree, so the next server.publish() was a heap-use-after-free.

Each of these now converts its argument once, up front (holding the JSString* for the duration of the send), and performs the single closed check after that, immediately before the uWS call.

Repro (before)

const server = Bun.serve({ port: 0,
  fetch(req, srv) { if (srv.upgrade(req)) return; return new Response("no"); },
  websocket: { open(ws) {
    const topic = Object.assign(new String("room"), { [Symbol.toPrimitive]() { ws.terminate(); return "room"; } });
    console.log(ws.subscribe(topic));                                  // true (should be false)
    setTimeout(() => { console.log(server.publish("room", "hello")); process.exit(0); }, 100); // UAF / SEGV
  }, message() {} } });
new WebSocket(`ws://127.0.0.1:${server.port}/`);

Tests

test/js/bun/websocket/websocket-server.test.ts — "ws.subscribe()/send() with an argument whose toPrimitive terminates the socket does not touch the dead socket". Fails on the ASan canary and debug main; passes here.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 12 minutes

Limit details: You’ve used all 1 included review currently available under your plan. You completed 71 included PR reviews in the past 7 days; at that activity level, included reviews refill at 1 review per hour.

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: 9f84dcbb-79a6-4077-adef-67801cbf1b02

📥 Commits

Reviewing files that changed from the base of the PR and between fea1829 and 1f266be.

📒 Files selected for processing (2)

• src/runtime/server/ServerWebSocket.rs
• test/js/bun/websocket/websocket-server.test.ts

---

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

[claude]

LGTM — targeted re-check of is_closed() after each user-JS-observable string conversion, matching the existing pattern in close(). The one inline note is a test-coverage nit; the fix itself is correct at all four sites.

Extended reasoning...

Overview

Adds an is_closed() re-check immediately after each string conversion that can run user JS (to_slice/to_js_string) in topic_dispatch (subscribe/unsubscribe/isSubscribed), send, send_text, and send_ping (ping/pong). Without it, a Symbol.toPrimitive that calls ws.terminate() leaves the code touching a freed uWS socket — most visibly subscribe inserting a dangling subscriber into the TopicTree, so the next server.publish() was a heap-UAF. A subprocess regression test drives all five topic/send variants and then server.publish() to prove the socket is not touched and the process exits cleanly.

Security risks

This closes a user-triggerable heap-use-after-free in the WebSocket server. The fix is purely additive (early-return guards) and returns the documented closed-socket values (false / 0), so no new attack surface is introduced.

Level of scrutiny

Native memory-safety code, so I checked the whole class rather than just the diffed sites. close() already had this exact re-check pattern; publish* route through do_publish() which re-tests !self.is_closed() before dereferencing self.websocket() (falling back to the app-wide path), so they're already safe; send_binary() accepts only ArrayBuffer/Blob and never runs user JS during argument handling. That leaves exactly the four sites this PR patches. I also confirmed JSValue::is_string() accepts StringObject (src/jsc/JSValue.rs:271-273 → is_string_like), so the test's new String(...) wrapper actually reaches the guarded branches in topic_dispatch, send_text, and send_ping.

Other factors

The one finding is a nit: the test omits ping/pong, so the send_ping re-check is unguarded against future regression. That's worth adding but doesn't affect correctness of the fix. The test follows harness conventions (subprocess with piped stdout/stderr drained concurrently, sorted output, exit-code asserted last), and asserting publish=0 after all the evil conversions specifically covers the original UAF path.

[claude]

🟡 The test covers subscribe/unsubscribe/isSubscribed/send/sendText but omits ping and pong, even though send_ping() received the same is_closed() re-check and the PR description lists them. Consider adding ping/pong cases to the open() dispatch and client loop (bumping pending to 7 and adding ping=0 subscribed=false / pong=0 subscribed=false to the expected output) so the send_ping re-check can't be reverted without a test failing.

Extended reasoning...

What's missing

This PR adds an is_closed() re-check after the string conversion in four places, one of which is send_ping() at ServerWebSocket.rs:1285-1287. send_ping() backs both ws.ping() and ws.pong(), and the PR description explicitly lists ping/pong among the fixed methods. However, the new regression test at websocket-server.test.ts:1524-1539 only exercises subscribe, unsubscribe, isSubscribed, send, and sendText — ping and pong are absent from both the open() dispatch chain and the client-connection loop.

Why the test approach would work for ping/pong

The same evil() helper reaches the fixed code path in send_ping(). Bun's JSValue::is_string() (src/jsc/JSValue.rs:271) delegates to JSType::is_string_like(), which matches String | StringObject | DerivedStringObject. So Object.assign(new String(v), {[Symbol.toPrimitive]: ...}) — a StringObject — passes the value.is_string() guard at line 1279, and to_js_string() on it invokes Symbol.toPrimitive, which calls ws.terminate(). Without the re-check at line 1285, self.websocket().send(...) would run against the freed uWS socket.

Why REVIEW.md flags this

REVIEW.md's test rules require:

• "Cover the variant matrix, not just the repro. Every sibling entry point receiving the same fix."
• "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test."

As written, the send_ping re-check (lines 1285-1287) could be deleted and no test in the suite would fail. The other three re-checks (in topic_dispatch, send, send_text) are each guarded by at least one test case, but this one is not.

Step-by-step proof

1. Take the branch else if value.is_string() in send_ping() (line 1279). The test's evil(ws, "x") is a StringObject, so is_string_like() returns true and this branch is entered.
2. value.to_js_string(global_this) calls ToPrimitive → Symbol.toPrimitive → ws.terminate() → self.flags.closed becomes true and the uWS socket is freed.
3. The re-check at line 1285 returns 0 before self.websocket().send(...) touches the dead socket.
4. Delete lines 1285-1287 and re-run the test suite: nothing calls ws.ping(evil(...)) or ws.pong(evil(...)), so nothing fails. The load-bearing clause is unguarded.

Suggested fix

Add two cases to the open() handler:

else if (which === "ping") rc = ws.ping(evil(ws, "x"));
else if (which === "pong") rc = ws.pong(evil(ws, "x"));

bump pending from 5 to 7, add "ping" and "pong" to the client-connection for loop, and add "ping=0 subscribed=false" / "pong=0 subscribed=false" to the sorted expected output.

Severity

nit — the production fix is correct and present at all seven sites; merging as-is causes no concrete failure. This is a test-coverage completeness gap: two of the seven fixed entry points are unguarded against future regression.

[robobun]

^{Updated 9:43 PM PT - Aug 16th, 2026}

✅ @Jarred-Sumner, your commit 1f266be9e5eeb775cf6dc3855505506495e684ec passed in Build #99848! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39385

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

bun-39385 --bun

[claude]

Beyond the inline nits, I also checked the untouched sibling entry points for the same convert-then-use-freed-socket shape: publish/publishText/publishBinary re-check is_closed() inside do_publish immediately before self.websocket().publish(), close() already re-checks after to_slice_or_null, and sendBinary/cork don't run a user-observable conversion between the check and the uWS call — so the fix set looks complete.

Extended reasoning...

The two findings are both nits (test-coverage gap for ping/pong, and an intentional validate-before-closed-check reorder that shifts the throw/sentinel boundary for invalid args on already-closed sockets). The production fix itself looks correct at all four touched sites. I'm not approving because this is a memory-safety fix with non-trivial control-flow restructuring in send() and send_ping() — the borrowed &[u8] now spans the is_closed() check and is kept live via hoisted array_buffer/string_slice/js_string locals plus trailing ensure_still_alive(), which is the kind of lifetime dance a human should confirm.

[claude]

🟡 Moving the is_closed() check to after argument validation changes observable behavior on already-closed sockets: e.g. ws.subscribe(123) / ws.send(null) / ws.ping(Buffer.alloc(200)) now throw where they previously returned false/0. This looks intentional per 7e83cbc ("check closed once") and is arguably more consistent — the arg-count guard already threw regardless of state — but if you want to preserve the old sentinel-return for invalid args, keeping the early is_closed() return in addition to the post-ToString one (as 978171a did) would do so without weakening the UAF fix.

Extended reasoning...

What changed

In topic_dispatch, send, send_text, and send_ping, the is_closed() short-circuit moved from before argument validation to after it. On a socket that is already closed, argument validation now runs first, so invalid arguments that previously fell through to the sentinel return now throw:

Call on closed socket	Before	After
ws.subscribe(123)	false	throws "topic must be a string"
ws.subscribe("")	false	throws "requires a non-empty topic name"
ws.send(null)	0	throws "send requires a non-empty message"
ws.send("x", "notbool")	0	throws "send expects compress to be a boolean"
ws.sendText(123)	0	throws "sendText expects a string"
ws.ping(123)	0	throws "requires a string, Blob, or BufferSource"
ws.ping(Buffer.alloc(200))	0	throws RangeError (>125 bytes)

Step-by-step proof

Take topic_dispatch at ServerWebSocket.rs:335-359:

Before (removed lines):

1. arguments_count() < 1 → throw
2. is_closed() → return false ← short-circuit
3. !args[0].is_string() → throw
4. empty topic → throw

After:

1. arguments_count() < 1 → throw
2. !args[0].is_string() → throw ← now reached on closed socket
3. to_js_string() (may run user JS)
4. empty topic → throw ← now reached on closed socket
5. is_closed() → return false

So closedWs.subscribe(123) used to hit step 2 and return false; now it hits the type guard and throws. The same reordering applies to send (null check + parse_compress_arg moved before the closed check), send_text (is_string() check moved before), and send_ping (type check + MAX_CONTROL_FRAME_PAYLOAD check moved before).

Why existing code doesn't prevent it

Nothing does — the reordering is deliberate. Commit 978171a first added a second is_closed() check after the ToString while keeping the early one; commit 7e83cbc ("convert the argument first, then check closed once") then removed the early check in favor of a single check after conversion. So the author explicitly chose validate-then-check-closed over check-closed-then-validate.

Impact

Only affects callers passing programmatically-invalid arguments (wrong type, null, oversized control frame) to a socket that has already closed. Correct callers — those passing valid strings/buffers — are unaffected: they still get false/0 on a closed socket, as the existing "subscribe/unsubscribe return false on a closed socket" test asserts. And the pre-PR behavior was already partly state-independent: ws.subscribe() (no args) threw on closed sockets before this change, since the arguments_count() < 1 guard preceded is_closed(). The new behavior extends that consistency to the remaining validation.

REVIEW.md's "treat every refactor as guilty until proven behavior-preserving" is what surfaces this: it is a user-visible behavior change not mentioned in the PR description, but one that only bites code that was already passing invalid inputs.

How to fix (if desired)

If preserving the old sentinel-on-closed behavior matters, restore the early check in addition to the post-conversion one — i.e. revert to the 978171a shape:

if self.is_closed() { return Ok(JSValue::FALSE); }   // early: preserves old fast-path
// ... validate + to_js_string() ...
if self.is_closed() { return Ok(JSValue::FALSE); }   // post-ToString: the UAF fix

The second check is the load-bearing one for the UAF; the first only exists to keep invalid-arg-on-closed-socket returning the sentinel. If the single-check design is intended (which the commit title suggests), no code change is needed — just worth being aware that the throw/sentinel boundary moved.