Conversation
|
Updated 11:05 PM PT - Jun 2nd, 2026
❌ @robobun, your commit 8a2ff86 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30778That installs a local version of the PR into your bun-30778 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR bounds close-frame reason encoding to RFC limits, removes an unsafe buffer cast, adds a regression test exercising long Unicode close reasons, and JSON-stringifies the build-time OVERLAY_CSS value. ChangesWebSocket close frame payload safety
Build-time CSS serialization
Suggested reviewers:
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
67ad0ca to
4cb9101
Compare
![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 `@test/js/web/websocket/websocket-close-fragmented.test.ts`:
- Around line 148-160: The test currently discards child stderr ("stderr:
\"ignore\"") which drops crash diagnostics; change Bun.spawn so proc is created
with stderr: "pipe" (alongside stdout: "pipe"), await proc.stderr.text()
together with proc.stdout.text() and proc.exited, and update the assertion to
include stderr in the failure output; when asserting stderr is empty, filter out
the known ASAN startup warning coming from bunEnv before comparing so benign
ASAN messages don't fail the test.
- Around line 216-219: Replace the use of the string .repeat call in the
ws.close call: locate the ws.close(1000, "\\u0800".repeat(42)) invocation and
replace the repetitive-string construction with the repository pattern using
Buffer.alloc(...).toString() (i.e., build the repeated "\u0800" payload via
Buffer.alloc(count, fill).toString() rather than String.prototype.repeat) so the
test uses Buffer.alloc for creating the repetitive payload.
🪄 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: a9778c52-1a99-4cf3-a408-be0e12c1b2b4
📒 Files selected for processing (1)
test/js/web/websocket/websocket-close-fragmented.test.ts
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "-e", CLOSE_LONG_REASON_FIXTURE], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "ignore", | ||
| }); | ||
| const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); | ||
| // With the fix: the fixture reaches the close listener, prints | ||
| // `close:1000`, and exits cleanly. Without the fix: the child aborts | ||
| // inside `WebSocket::close` before the listener fires — stdout is | ||
| // empty and exitCode is non-zero (SIGILL from the Rust panic). | ||
| expect({ stdout: stdout.trim(), exitCode }).toEqual({ | ||
| stdout: "close:1000", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Pipe the child stderr instead of discarding it.
This test is guarding a crash path; with stderr: "ignore", a regression only gives you stdout/exitCode and drops the most useful crash diagnostics.
♻️ Suggested change
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", CLOSE_LONG_REASON_FIXTURE],
env: bunEnv,
stdout: "pipe",
- stderr: "ignore",
+ stderr: "pipe",
});
- const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+ const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ const cleanStderr = stderr
+ .split("\n")
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"))
+ .join("\n")
+ .trim();
// With the fix: the fixture reaches the close listener, prints
// `close:1000`, and exits cleanly. Without the fix: the child aborts
// inside `WebSocket::close` before the listener fires — stdout is
// empty and exitCode is non-zero (SIGILL from the Rust panic).
- expect({ stdout: stdout.trim(), exitCode }).toEqual({
+ expect({ stdout: stdout.trim(), stderr: cleanStderr, exitCode }).toMatchObject({
stdout: "close:1000",
+ stderr: "",
exitCode: 0,
});Based on learnings, crash-prone spawned-process tests should keep subprocess output in the failure diff, and bunEnv-based tests should filter the known ASAN startup warning before asserting stderr is empty.
🤖 Prompt for 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.
In `@test/js/web/websocket/websocket-close-fragmented.test.ts` around lines 148 -
160, The test currently discards child stderr ("stderr: \"ignore\"") which drops
crash diagnostics; change Bun.spawn so proc is created with stderr: "pipe"
(alongside stdout: "pipe"), await proc.stderr.text() together with
proc.stdout.text() and proc.exited, and update the assertion to include stderr
in the failure output; when asserting stderr is empty, filter out the known ASAN
startup warning coming from bunEnv before comparing so benign ASAN messages
don't fail the test.
| // 42 code units × 3 UTF-8 bytes = 126 bytes — one byte past the | ||
| // 125-byte close-frame payload cap. C++ spec check bounds on UTF-16 | ||
| // code-unit count (42 < 123), so this reaches the native close path. | ||
| ws.close(1000, "\\u0800".repeat(42)); |
There was a problem hiding this comment.
Replace .repeat(42) with the repo’s Buffer.alloc(...).toString() pattern.
This fixture is building a repetitive string in a test, and this repo explicitly avoids .repeat() for that case.
♻️ Suggested change
- // 42 code units × 3 UTF-8 bytes = 126 bytes — one byte past the
+ // 126 UTF-8 bytes total — one byte past the
// 125-byte close-frame payload cap. C++ spec check bounds on UTF-16
// code-unit count (42 < 123), so this reaches the native close path.
- ws.close(1000, "\\u0800".repeat(42));
+ ws.close(1000, Buffer.alloc(126, "\\u0800").toString());As per coding guidelines, "To create repetitive strings in tests, use Buffer.alloc(count, fill).toString() instead of "string".repeat(count) for better performance in debug builds."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 42 code units × 3 UTF-8 bytes = 126 bytes — one byte past the | |
| // 125-byte close-frame payload cap. C++ spec check bounds on UTF-16 | |
| // code-unit count (42 < 123), so this reaches the native close path. | |
| ws.close(1000, "\\u0800".repeat(42)); | |
| // 126 UTF-8 bytes total — one byte past the | |
| // 125-byte close-frame payload cap. C++ spec check bounds on UTF-16 | |
| // code-unit count (42 < 123), so this reaches the native close path. | |
| ws.close(1000, Buffer.alloc(126, "\\u0800").toString()); |
🤖 Prompt for 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.
In `@test/js/web/websocket/websocket-close-fragmented.test.ts` around lines 216 -
219, Replace the use of the string .repeat call in the ws.close call: locate the
ws.close(1000, "\\u0800".repeat(42)) invocation and replace the
repetitive-string construction with the repository pattern using
Buffer.alloc(...).toString() (i.e., build the repeated "\u0800" payload via
Buffer.alloc(count, fill).toString() rather than String.prototype.repeat) so the
test uses Buffer.alloc for creating the repetitive payload.
There was a problem hiding this comment.
Thanks — 5a4e9b4 resolves the prettier-reflow nit; all my earlier comments are now addressed. Deferring to a human since this also bundles an unrelated bake-codegen.ts OVERLAY_CSS behavioral change and overlaps with #30722.
Extended reasoning...
Overview
The headline change is a targeted fix in src/http_jsc/websocket_client.rs: the close-reason transcode buffer is now [u8; 125] (matching send_close_with_body's Option<&mut [u8; 125]> signature), the cursor is built over a 123-byte view (MAX_REASON_BYTES) so overflow bails to the no-reason path at the RFC 6455 boundary, and the unsafe cast::<[u8; 125]>() is gone. A regression test in websocket-close-fragmented.test.ts spawns a child that calls ws.close(1000, '\\u0800'.repeat(42)) and asserts clean exit. The remaining 11 files are autofix.ci/rustfmt reflows of multi-target #[cfg(...)] / #[cfg_attr(...)] attributes (whitespace-only), a jsc_hooks.rs line-wrap, and one substantive one-liner in src/codegen/bake-codegen.ts wrapping OVERLAY_CSS in JSON.stringify(...).
Security risks
The fix removes UB (out-of-bounds slice via a narrowed array reference) and a process-aborting panic across extern "C" reachable from JS WebSocket.prototype.close. The 123-byte cap prevents emitting close frames with header-length 126/127 (the §5.2 extended-length sentinels). I don't see new attack surface introduced; the test fixture binds to 127.0.0.1:0. No auth/crypto/permissions code is touched.
Level of scrutiny
Moderate. The core change is small and well-reasoned, and three rounds of review feedback (off-by-2 on the 125 vs 123 cap, test-convention nits, prettier comment reflow) have all been incorporated. However:
- It rewrites RFC 6455 close-frame payload bounds in a hand-rolled protocol implementation — an area where off-by-one errors have wire-visible consequences.
- It bundles an unrelated build-system behavioral change (
bake-codegen.tsnowJSON.stringifys the CSS define) that isn't covered by the PR title/description. - robobun reports CI failures on Build #54713 (at f6cd1cc); 5a4e9b4 was pushed after that comment and CI status for it isn't reflected yet.
- github-actions flagged #30722 as fixing the same WebSocket overflow via a different approach (runtime clamp vs buffer shrink) — a human should pick one.
Other factors
All three of my prior inline threads are resolved by the current diff (the last via 5a4e9b4). The rustfmt attribute reflows are mechanical and zero-risk. The new test isolates the crash in a subprocess so a regression won't take down the runner, and asserts on stdout + exitCode per repo conventions. Given the bundled unrelated change, the open CI status, and the duplicate-PR coordination needed, this isn't a "merge without a human glance" PR.
|
CI red, but the failures are on |
4db0921 to
8041ffc
Compare
|
Rebased onto main. Conflict in
Release bun on main now includes #30679's JSON lexer fix, so the |
|
CI red again on same pattern as before: Looking at recent builds, ASAN shard 17 has failed with exit -1 on multiple unrelated builds (54920, 55200, 55217) — it's a known flaky infra lane, not specific to this PR. Different agents each time ( Diff is green on every lane that runs to completion, including the ASAN job on shards 0-16, 18, 19. Re-roll already used. Needs a maintainer to re-run the red shard or merge. |
8041ffc to
bbf49b2
Compare
|
Rebased onto main. Conflict in Kept this PR's version: Rust tests still pass: |
|
Closing: #30771 is already fixed on main via #30722 (runtime This PR's contribution is now purely stylistic (buffer sized to match the Happy to leave open if the cleanup is wanted; otherwise please close. |
…cast WebSocket<SSL>::close allocated a 128-byte `close_reason_buf` on the stack, transcoded the reason into it (UTF-16→UTF-8 via `to_owned_slice` or Latin-1→UTF-8 via `copy_latin1_into_utf8`), then pointer-cast the buffer to `&mut [u8; 125]` and passed it to `send_close_with_body` with `body_len = cursor.position()`. The cursor position could reach 128, and the downstream `&data[..body_len]` slice overruns the 125-byte array reference. A 42-code-unit UTF-16 reason of U+0800 passes the C++ spec check (`reason.length() > 123`) but transcodes to 126 UTF-8 bytes, panicking the runtime with `range end index 126 out of range for slice of length 125`. The panic crosses the `extern "C" fn close` boundary and aborts the process. Shrink the transcode buffer to 125 bytes so: - `cursor.write_all` fails with WriteZero when the transcoded UTF-8 exceeds 125 bytes (same `break 'inner` path the Latin-1 arm already takes on overflow), falling through to the no-reason `close` frame. - The fixed-size array is passed directly to `send_close_with_body`, eliminating the unsafe pointer cast. Regression test: send `close(1000, '\u0800'.repeat(42))` to a raw handshake server and wait for the close event without the runtime panicking. Fixes #30771
The shipped `build/release/bun` binary in the tree pre-dates #30679's JSON lexer fix, so when it runs `bake-codegen.ts` during `bun bd` or `bun run build:release` the old lexer aborts on the leading `*` of the minified CSS `define` value: *{box-sizing:border-box;...} ^ error: Unsupported syntax: Operators are not allowed in JSON Main is otherwise unbuildable with the shipped release bun. Wrap OVERLAY_CSS in `JSON.stringify` — matches the canonical `Bun.build` `define` pattern and parses cleanly under every JSON lexer generation.
… frame Follow-up to the buffer-overrun fix: the 125-byte transcode buffer let a reason of 124 or 125 UTF-8 bytes through, and `send_close_with_body` then wrote header-length 126/127 — the RFC 6455 §5.2 extended-length sentinels. Control frames have no extended length encoding, so the receiver misparses the mask key as length and rejects the frame. Keep the array at 125 bytes (to match the `Option<&mut [u8; 125]>` signature on the other caller) but build the cursor over a 123-byte view so `WriteZero` / `NoSpaceLeft` bail at the correct boundary. Also address review nits on the regression test: drop the `panicked` stderr-substring assertion (the stdout + exitCode checks already cover the regression in two independent ways; the substring check is exactly the pattern CLAUDE.md warns against) and drop the per-test timeout (covered by the runner default).
autofix.ci kept collapsing three `//` comments wedged between the test arrow's `}` and the `30_000` timeout onto the `},` line in reverse reading order, and after three repeated bot commits the Format job gave up with 'refusing to patch repeated bot commits'. Move the comment to the line above `test(...)` where prettier won't touch it, and let prettier collapse the call back to its preferred single-line form.
bbf49b2 to
8a2ff86
Compare
|
Rebased onto main again. Two things resolved in
Note: as called out earlier, the underlying panic is already fixed on main via #30722's runtime guard; this PR's remaining value is removing the unsafe |
|
Final status — requesting a maintainer close this PR (or merge as cleanup, their call). The bug from #30771 (panic + malformed close frame on an over-long close reason) is fully fixed on main via #30722's runtime This PR's only remaining delta is stylistic: a No further automated iteration is productive here. |
| IS_ERROR_RUNTIME: String(file === "error"), | ||
| IS_BUN_DEVELOPMENT: String(!!debug), | ||
| OVERLAY_CSS: css("../runtime/bake/client/overlay.css", !!debug), | ||
| OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)), |
There was a problem hiding this comment.
🟡 nit: now that OVERLAY_CSS is wrapped in JSON.stringify(), the define value is a quoted string literal — it no longer reaches the JSON lexer's auto-quote fallback. But src/parsers/json_lexer.rs:1292 still cites "(bake-codegen.ts's OVERLAY_CSS)" as the concrete in-tree example of a raw *{...} define that motivates that fallback; a reader following that pointer will now find JSON.stringify and be confused. Since the wrap is being kept, suggest dropping the parenthetical from the json_lexer comment — the generic "raw minified CSS string starting with *{...}" description before it stays accurate.
Extended reasoning...
What the issue is
This PR changes src/codegen/bake-codegen.ts:56 from passing raw minified CSS to Bun.build's define:
OVERLAY_CSS: css("../runtime/bake/client/overlay.css", !!debug),to passing a JSON-quoted string literal:
OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)),Meanwhile src/parsers/json_lexer.rs:1286–1294 (untouched by this PR) explains why the JSON lexer tolerates ?/*/(/) instead of erroring, and gives one concrete in-tree example:
…e.g. a
Bun.builddefine:whose value is a raw minified CSS string starting with*{...}(bake-codegen.ts'sOVERLAY_CSS). Erroring here abortsLexer::initbeforeparse_env_jsongets a chance to auto-quote.
After this PR, OVERLAY_CSS is a quoted string literal starting with ", not raw CSS starting with *, so it parses as an ordinary JSON string and never reaches the auto-quote fallback the comment is describing. The parenthetical is now a dangling cross-reference.
Step-by-step proof
- Before:
css(...)returns minified CSS, e.g.*{box-sizing:border-box}…. Passed verbatim as adefinevalue, the JSON lexer sees*at position 0. Per json_lexer.rs:1286–1294 it tokenizesTAsteriskwithout error soparse_env_json's auto-quote retry can wrap it.OVERLAY_CSSis the named exemplar of this path. - After:
JSON.stringify(css(...))returns"*{box-sizing:border-box}…"— a valid JSON string. The lexer sees"at position 0, parses a string literal, and the*/?/(/)tolerance arm is never entered. - A future reader auditing why json_lexer.rs special-cases
*/?/(/)follows the comment's pointer tobake-codegen.ts'sOVERLAY_CSS, finds it wrapped inJSON.stringify, and cannot reproduce the described behavior — at best confusing, at worst leading them to conclude the tolerance arm is dead code (it isn't; third-partyBun.buildusers can still pass unquoted defines).
Why this is in scope
The PR author's own rebase note (2026-05-16) says the JSON.stringify wrap "is no longer strictly needed but is the canonical Bun.build define shape — left in", i.e. the change is intentional and permanent. The json_lexer comment's stale example is a direct consequence of a change this PR is making and keeping. This PR's review history also shows comment-accuracy nits are accepted (the "line 1006" rot at websocket_client.rs:1635 and the stale MAX_REASON_BYTES rationale were both flagged and fixed).
Addressing the "too trivial / scope creep" objection
One verifier noted (a) the comment says "e.g." so OVERLAY_CSS is illustrative not load-bearing, (b) it documents the historical motivating case which remains true, and (c) editing json_lexer.rs would further expand an already out-of-scope change. On (a)/(b): the parenthetical is written in the present tense as a concrete file+identifier pointer ("bake-codegen.ts's OVERLAY_CSS"), not as "originally motivated by" — and it is the only concrete example given, so a reader who checks it and finds it doesn't match has nothing else to fall back on except the generic description. On (c): agreed that the cleanest resolution would be to drop the bake-codegen change from this WebSocket PR entirely (CodeRabbit's pre-merge check says the same); but if the author is keeping it — as stated — then the cross-reference it invalidates is worth a one-word mention. Either resolution is fine; flagging so it isn't silently lost.
Impact
Zero runtime impact — pure comment accuracy. The lexer's */?/(/) tolerance remains correct and useful for third-party Bun.build defines; only the cited in-tree example is now wrong. Hence nit.
Fix
Drop the parenthetical from src/parsers/json_lexer.rs:1292:
- // unquoted value that starts with one — e.g. a `Bun.build`
- // `define:` whose value is a raw minified CSS string starting
- // with `*{...}` (`bake-codegen.ts`'s `OVERLAY_CSS`). Erroring
+ // unquoted value that starts with one — e.g. a `Bun.build`
+ // `define:` whose value is a raw minified CSS string starting
+ // with `*{...}`. Erroring(Or, alternatively, drop the bake-codegen.ts change from this PR as CodeRabbit's out-of-scope check suggests, in which case the comment stays accurate as-is.)
|
Closing as fully superseded by main. The bug from #30771 was first fixed on main via #30722's runtime
There is no unsafe cast left to remove and no observable behavior this PR changes, so it can't carry a meaningful regression test (the gate correctly rejects it). Nothing left to contribute. Thanks to everyone who reviewed. |
1 participant
Summary
Fixes #30771 —
WebSocket<SSL>::closeinsrc/http_jsc/websocket_client.rstranscodes the close reason into a 128-byte stack buffer and then pointer-casts the buffer to&mut [u8; 125]before callingsend_close_with_body(.., body_len = cursor.position()). When the transcoded UTF-8 lands in(125, 128], the downstream&data[..body_len]read on the 125-byte array reference panics at runtime — and the panic crosses theextern "C"boundary, aborting the process.Repro
A UTF-16 reason of 42 ×
U+0800(126 UTF-8 bytes) passes the C++reason.length() > 123spec check becauselength()is code units, not UTF-8 bytes:Runtime panic:
Same failure mode via the Latin-1 arm (
copy_latin1_into_utf8can transcode 64 × 0x80 → 128 UTF-8 bytes) and the UTF-8 arm (any 126-byte pre-tagged UTF-8 slice).Cause
Three transcoding arms in
close()each write intoclose_reason_buf: [u8; 128]:cursor.write_all(&to_owned_slice())— succeeds up to 128 bytes before returningWriteZero.cursor.write_all(str.slice())— same.copy_latin1_into_utf8(dst, str.slice())— returnsNoSpaceLeftonly when the buffer ran out before the source, so up to 128 bytes of output slips through on a 128-byte output.Then
close_reason_buf.as_mut_ptr().cast::<[u8; 125]>()is dereferenced as&mut [u8; 125]. The reference only carries 125 bytes of provenance;send_close_with_body's&data[..body_len]withbody_len > 125panics on the bounds check (and is UB under Stacked/Tree Borrows even without the panic).Fix
Shrink
close_reason_bufto[u8; 125]. The existing overflow handling in all three arms (break 'inner→ fall through to the no-reasonsend_close_with_body(code, None, 0)) now triggers exactly when the transcoded UTF-8 exceeds the RFC 6455 125-byte payload cap, matching the Zig original'swrote.ptr[0..125]intent without the unsafe cast. The array reference we hand tosend_close_with_bodynow covers the full 125-byte provenance, and the cast is gone.A
debug_assert!(wrote_len <= close_reason_buf.len())catches any regression that reintroduces the bug.Test
test/js/web/websocket/websocket-close-fragmented.test.tsgains'close() with reason that transcodes beyond 125 UTF-8 bytes does not crash': a raw TCP server completes the WebSocket handshake, the client callsclose(1000, '\u0800'.repeat(42)), and the test waits for'close'on the client.Before:
bun testaborts withrange end index 126 out of range for slice of length 125.After:
2 pass / 0 fail.