http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect#30785
http_jsc: narrow unsafe surface of WebSocketUpgradeClient::connect#30785robobun wants to merge 1 commit into
Conversation
WalkthroughThis PR refactors ChangesHTTPClient unsafe surface narrowing
Platform detection formatting and misc refinements
WebSocket header handling regression tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 11:21 AM PT - May 22nd, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 30785That installs a local version of the PR into your bun-30785 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.rs`:
- Around line 214-221: In connect, before calling Headers8Bit::init for the
request headers and again for proxy headers, explicitly validate that
header_names.len() == header_values.len() and proxy_header_names.len() ==
proxy_header_values.len(); if either pair mismatches, return an appropriate Err
(or propagate an error) instead of relying on debug_assert_eq! so the safe API
cannot trigger an index-out-of-bounds in Headers8Bit::init; update the checks
adjacent to the current Headers8Bit::init calls referencing the symbols
header_names, header_values, proxy_header_names, proxy_header_values and
Headers8Bit::init to enforce and handle the length invariant in release builds.
- Around line 199-205: The connect function currently accepts and stores a raw
pointer websocket: *mut CppWebSocket while being declared safe; make the API
sound by either changing the connect signature to unsafe fn connect(...) (and
document the required invariant that the caller must ensure the pointee outlives
all callbacks like did_abrupt_close, reject_unauthorized, set_protocol,
did_connect_with_tunnel, did_connect) or, as an alternative minimal fix, change
the parameter type to a NonNull<CppWebSocket> (or a lifetime-bound reference) so
the pointer’s non-nullness/lifetime is encoded in the type; update the connect
declaration and its callers accordingly and ensure any storage of the pointer
uses the stronger typed wrapper rather than a raw *mut CppWebSocket.
🪄 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: 09c71104-09a4-4fd5-b23c-989fb29564a7
📒 Files selected for processing (12)
src/crash_handler/lib.rssrc/errno/lib.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/perf/tracy.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/js/web/websocket/websocket-custom-headers.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // JSON.stringify so the define: value is a proper JSON string. | ||
| // Raw CSS starts with `*{…}`, which the JSON lexer only tokenises | ||
| // as a recoverable SyntaxError in bun >= 1.3.14-canary (commit | ||
| // 314d044c0a); older bootstrap bins reject it before auto-quote | ||
| // can kick in. Quoting here keeps bake-codegen.ts forward- and | ||
| // backward-compatible with the lexer change. | ||
| OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)), |
There was a problem hiding this comment.
🟡 Now that OVERLAY_CSS is wrapped in JSON.stringify(), two existing comments that cite bake-codegen.ts as a live example of passing raw *{…} CSS to define: are stale: src/parsers/json_lexer.rs:1304 ("bake-codegen.ts's OVERLAY_CSS") and test/bundler/bun-build-api.test.ts:76 ("src/codegen/bake-codegen.ts passes verbatim as OVERLAY_CSS"). The auto-quote codepath and its test remain valid for the general case; only the bake-codegen.ts attribution should be reworded (e.g. "previously passed") or dropped.
Extended reasoning...
This PR changes src/codegen/bake-codegen.ts so that OVERLAY_CSS is now passed as JSON.stringify(css(...)) instead of the raw minified CSS string. The new in-file comment is explicit about the intent: "Quoting here keeps bake-codegen.ts forward- and backward-compatible with the lexer change" — i.e., bake-codegen.ts deliberately stops relying on the JSON lexer's auto-quote rescue path.
However, two other places in the tree still describe bake-codegen.ts as a present-tense consumer of that rescue path:
src/parsers/json_lexer.rs:1302-1304— the comment explaining why*/?/(/)are tokenised rather than errored says: "e.g. aBun.builddefine:whose value is a raw minified CSS string starting with*{...}(bake-codegen.ts'sOVERLAY_CSS)".test/bundler/bun-build-api.test.ts:76— the comment above the auto-quote test table says: "a raw minified CSS string starts with*{...}, which src/codegen/bake-codegen.ts passes verbatim asOVERLAY_CSS".
After this PR, both statements are factually wrong: bake-codegen.ts now passes a JSON-quoted string (e.g. "\"*{box-sizing:...}\""), which the lexer parses as a normal TStringLiteral and never reaches the TAsterisk → auto-quote path. A reader following the cross-reference from json_lexer.rs to bake-codegen.ts will find JSON.stringify(...) and conclude either that the lexer comment is wrong or that the auto-quote codepath is now dead — neither of which is true.
Step-by-step:
- Before:
define: { OVERLAY_CSS: "*{box-sizing:border-box}..." }→ JSON lexer sees*at offset 0 → tokenisesTAsterisk(per the json_lexer.rs:1299-1306 arm) →parse_env_jsonfails to parse as expression → auto-quotes the whole value. The json_lexer.rs comment correctly named this as the live example. - After:
define: { OVERLAY_CSS: "\"*{box-sizing:border-box}...\"" }→ JSON lexer sees"at offset 0 → tokenisesTStringLiteral→ parses cleanly. The auto-quote path is never entered for this value, so citing it as "bake-codegen.ts'sOVERLAY_CSS" is now a dangling reference.
Impact: None at runtime. The auto-quote codepath in json_lexer.rs and the .each(["*{box-sizing:...}", ...]) test in bun-build-api.test.ts are still correct and still needed for the general case (any user can pass raw CSS to define:). Only the parenthetical/clause attributing the behavior to bake-codegen.ts has gone stale.
Fix: Reword both comments to drop or past-tense the bake-codegen.ts citation — e.g. in json_lexer.rs change "(bake-codegen.ts's OVERLAY_CSS)" to "(as bake-codegen.ts's OVERLAY_CSS did before it was pre-quoted)" or simply delete the parenthetical; in bun-build-api.test.ts change "passes verbatim" to "used to pass verbatim" or drop the file reference. This is a comment-only doc-sync nit, not a behavioral bug.
The whole 300+ line body was `unsafe fn` despite already having focused
`unsafe {}` blocks around each genuine FFI call. Drop the outer
`unsafe` and lift the raw-pointer header params to `&[BunString]` slices
so the function signature is safe.
- `unsafe fn connect` → `fn connect`.
- `header_names: *const BunString, header_values: *const BunString, header_count: usize`
collapses to `header_names: &[BunString], header_values: &[BunString]`;
same for the proxy header arrays.
- `Headers8Bit::init` takes slices instead of (ptr, len) pairs, dropping
its own `unsafe` requirement.
- The `Bun__WebSocketHTTPS?Client__connect` extern-C shim keeps the
(ptr, len) ABI and does the `bun_core::ffi::slice` conversion in a
focused `unsafe {}` block before handing off to safe `connect`.
- `websocket: *mut CppWebSocket` stays as a raw pointer (no deref in
`connect` — just stored on the client).
[autofix.ci] apply automated fixes
test(#30777): cover zero/many-header slice boundaries in WebSocket connect
Lock in the two ends of the refactored slice parameter: zero user
headers hits the `ffi::slice(ptr, 0)` path in the extern-C shim, and
many user headers drives `Headers8Bit::init`'s `chunks_exact(2)`
name/value interleave. Behavioral equivalence is the guarantee — the
refactor must not lose a header or pair them off by one.
codegen(bake): quote OVERLAY_CSS define value so it parses as JSON
The raw CSS handed to `define: { OVERLAY_CSS: … }` starts with `*{…}`,
which older bootstrap bun builds reject as 'Operators are not allowed
in JSON' before the auto-quote fallback can turn it into a string
literal. The lexer was taught to recover from that in 314d044, but
the release bun driving bake-codegen.ts in `bun bd` doesn't always
have that fix yet — the bootstrap is a regular failure point.
Wrapping the CSS in `JSON.stringify` makes the define value a proper
JSON string, which both old and new lexers accept. Matches how every
other `define:` value in the block is already stringified (`side`,
`IS_ERROR_RUNTIME`, `IS_BUN_DEVELOPMENT`).
http_jsc: harden WebSocketUpgradeClient::connect against bad slice lengths and null pointer
- Headers8Bit::init now iterates via `zip`, so unequal-length parallel
slices truncate to the shorter side in release instead of indexing
past the end. The `debug_assert_eq!` still fires loudly in debug —
mismatched lengths remain a programmer bug the extern-C shim can't
produce, but the safe API no longer panics if they ever leak through.
- connect() now takes `NonNull<CppWebSocket>` instead of `*mut
CppWebSocket`. The C++ caller in WebSocket.cpp always passes `this`
(live object), so the non-null invariant is encoded in the type; the
extern-C shim guards the nullable wire pointer before dispatch.
Storing is still `Option<*mut …>.as_ptr()` — the field type stays
the same to keep this change scoped to the connect signature.
robobun
force-pushed
the
farm/43aedeeb/narrow-unsafe-connect-ws-upgrade
branch
from
3b7b0d0 to
e1a5778
Compare
|
Closing: this is a pure behavior-preserving refactor (narrowing the The two things my diff changes that a test could distinguish are both unreachable from user code:
Every other change is source-level: If a maintainer wants the refactor, the branch is at https://github.com/oven-sh/bun/tree/farm/43aedeeb/narrow-unsafe-connect-ws-upgrade — the code and tests are clean, CI was green on every real lane (only darwin-14-aarch64 expired on infra). The branch also carries an incidental Ref #30777. |
| const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`, opts); | ||
| ws.onopen = () => resolveOpen(); | ||
| ws.onerror = e => rejectOpen((e as any).error ?? new Error((e as any).message ?? "ws error")); | ||
| const [, headers] = await Promise.all([opened, headersSeen]); | ||
| ws.close(); |
There was a problem hiding this comment.
🟡 The new openAndEchoHeaders helper creates a WebSocket without pushing it to the module-level clients[] array, unlike every other test in this file. If Promise.all([opened, headersSeen]) rejects via ws.onerror, ws.close() is skipped and afterEach won't terminate the socket — add clients.push(ws); right after construction to match the file's established cleanup pattern (test/CLAUDE.md: "Track resources (servers, clients) in arrays for cleanup in afterEach()").
Extended reasoning...
What this is
The file declares module-level clients: WebSocket[] / servers: Subprocess[] arrays (lines 7-8) plus beforeEach(cleanUp) / afterEach(cleanUp) hooks (lines 21-22) that iterate clients and call .terminate?.() on each entry. Because these hooks are registered at module scope, they apply to every describe block in the file — including the new "WebSocket connect() header slice boundaries (#30777)" block. All nine pre-existing tests in this file follow the same pattern: construct the WebSocket, then immediately clients.push(ws) (lines 60, 97, 137, 166, 196, 233, 279, 309, 358). The new openAndEchoHeaders helper at line 407 constructs ws but never pushes it.
Code path
openAndEchoHeadersis called.const ws = new WebSocket(...)runs at line 407 —wsis not added toclients[].Promise.all([opened, headersSeen])is awaited at line 410.- If
ws.onerrorfires (e.g. the upgrade fails for any reason),rejectOpen(...)rejectsopened, soPromise.allrejects and theawaitthrows. - Because the throw happens before line 411,
ws.close()is never reached. afterEach(cleanUp)runs, butclientsis empty, so it doesn't terminatews.
Why this is only a nit
The refutation is right that practical leakage is bounded: await using server = Bun.serve(...) guarantees the server is disposed when the helper unwinds (even on rejection), and tearing down the Bun.serve instance closes the peer side of the connection, which in turn tears down the client ws. An onerror'd WebSocket is also already in a failed state with nothing meaningful left to terminate. So this is not a resource leak in practice.
It is, however, an inconsistency with both (a) the explicit pattern every other test in this file follows, and (b) the documented test guideline in test/CLAUDE.md:247: "Track resources (servers, clients) in arrays for cleanup in afterEach()". The refutation argues that the clients[] array exists specifically because the other tests use a subprocess server with no using semantics — that's true for why the array was originally needed, but it doesn't make it wrong to also use it here. The module-scope afterEach already runs for these new tests regardless; pushing to clients[] is a one-line, zero-cost belt-and-suspenders that keeps the file uniform and makes the cleanup path independent of whether server disposal happens to close the peer.
Step-by-step example
- Suppose a future regression in
HTTPClient::connectcauses the upgrade to fail with an error event beforeopenfires. ws.onerror→rejectOpen(err)→await Promise.all(...)throws → helper unwinds.await usingdisposesserver. The clientwsis left to be closed by the server teardown rather than by the test's own cleanup hook.- With
clients.push(ws),afterEachwould also callws.terminate?.()— same outcome, but via the file's documented cleanup contract rather than as a side effect of server disposal.
Fix
const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`, opts);
clients.push(ws);One line, brings the new tests in line with the rest of the file and with test/CLAUDE.md.
1 participant
Fixes #30777.
Problem
HTTPClient::<SSL>::connectinsrc/http_jsc/websocket_client/WebSocketUpgradeClient.rswas markedunsafe fnover its ~300-line body, even though the body already contained discreteunsafe {}blocks around every genuine FFI call (Headers8Bit::init,(hooks.ssl_ctx_cache_get_or_create),(hooks.default_client_ssl_ctx),Self::deref,&mut *clientre-derivations,(*vm_ptr).rare_data(), etc.). Marking the outer functionunsafe fnadded no information and hid what was actually unsafe.Fix
unsafe fn connect→ safefn connect.(*const BunString, *const BunString, usize)parameters (target headers + proxy headers) collapse to&[BunString]slices on the innerconnect.websocketis nowNonNull<CppWebSocket>so non-nullness is encoded in the type.connectnever dereferences it (just stores on the client); the deref sites are the later callbacks (handle_connect_error,did_connect, etc.) which are already markedunsafeat the site.Headers8Bit::inittakes slices instead of(ptr, len)pairs, dropping its ownunsaferequirement. Useszipso a length mismatch truncates to the shorter slice in release instead of OOB-panicking;debug_assert_eq!still catches the bug loudly in debug.Bun__WebSocketHTTPClient__connect/Bun__WebSocketHTTPSClient__connectextern "C"shims keep the(ptr, len)ABI unchanged (C++ callers inWebSocket.cppare unaffected), do thebun_core::ffi::sliceconversion in a focusedunsafe {}block, and turn the wire*mut CppWebSocketinto aNonNull(returning null on the defensive-unreachable branch) before handing off to safeconnect.The remaining
unsafe {}blocks inside the body (VM pointer reads, pre-connect&mut *clientre-derivations,Self::derefon error paths, rawSSL_CTX*creation) cover the genuinely unsafe operations and are unchanged.Also includes a small
src/codegen/bake-codegen.tsfix:OVERLAY_CSSis nowJSON.stringify-wrapped so thedefine:value is a proper JSON string — older bootstrap bun binaries reject raw*{…}CSS as "operators not allowed in JSON" before the auto-quote fallback can rescue it.Verification
cargo check -p bun_http_jscis clean.bun bdbuilds a working debug binary.websocket-client,websocket-custom-headers,websocket-subprotocol-strict,websocket-utf16-headers,websocket-accept-header-validation,websocket-close-connecting,websocket-close-fragmented,websocket-client-short-read,websocket-blob— all green locally.Rebase notes (latest push)
Rebased onto current
mainwith three conflicts, all mechanical:src/http_jsc/websocket_client/WebSocketUpgradeClient.rs— insideHeaders8Bit::init, upstream still had the oldunsafe { ffi::slice(names_ptr, len) }lines above the iteration. My refactor removed the raw-pointer params from that function's signature, so those lines were dead; dropped them. Upstream's unrelated input-validation additions inhandle_data/ request-parse paths (saturating_add/max_http_header_sizechecks from Hardening: input validation and bounds tightening across 28 subsystems (round 2) #31175, Hardening: input validation and bounds tightening across 26 subsystems #31129) are preserved intact.src/runtime/cli/run_command.rs— upstream changedentry_point_load_failed'serrparam from ownedbun_core::Errorto&bun_core::Error. My branch only carried cfg-attr formatting changes (from autofix.ci) that are already on main; took upstream in full.src/runtime/jsc_hooks.rs— same situation: upstream's simpler(*Fs::FileSystem::instance()).tmpdir().ok()?form vs my branch'sunsafe { &mut *... }autofix.ci restyle. Took upstream.