Skip to content

Bun.serve: validate websocket integer options before narrowing - #36998

Open
robobun wants to merge 3 commits into
mainfrom
farm/d0a6ba4c/ws-int-option-wrap
Open

robobun wants to merge 3 commits into
mainfrom
farm/d0a6ba4c/ws-int-option-wrap

Conversation

@robobun

@robobun robobun commented Aug 5, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The Bun.serve({ websocket }) integer options were narrowed with as u32 / as u16 before any range check in src/runtime/server/WebSocketServerContext.rs, so out-of-range values silently wrapped:

  • maxPayloadLength: 2**32 + 16 behaved as 16: every message over 16 bytes closed the connection with code 1006
  • idleTimeout: 65566 was accepted and behaved as 30 seconds, while idleTimeout: 961 threw websocket expects idleTimeout to be 960 or less (and idleTimeout: 65536 wrapped to 0, silently disabling the timeout)
  • backpressureLimit: 2**32 + 1 behaved as 1 byte, so queued sends were dropped (ws.send() returned 0)
for (const ws of [{ idleTimeout: 65566 }, { idleTimeout: 961 }]) {
  try { Bun.serve({ port: 0, fetch() {}, websocket: { message() {}, ...ws } }).stop(true); console.log(ws, "accepted"); }
  catch (e) { console.log(ws, "threw:", e.message); }
}
// before: {idleTimeout:65566} accepted, {idleTimeout:961} threw
// after:  both throw

Fix

  • idleTimeout is range-checked on the wide i64 value before narrowing, matching how the top-level Bun.serve({ idleTimeout }) is handled in ServerConfig.rs, so any value above 960 now throws.
  • maxPayloadLength and backpressureLimit now use the saturating JSValue::to_u32() helper, so oversized values clamp to u32::MAX instead of wrapping. These two have no semantic cap (the u32 is a storage limit), and clamping keeps configs like maxPayloadLength: Number.MAX_SAFE_INTEGER meaning "as large as supported" rather than turning them into startup errors. idleTimeout's 960 cap is a real uWebSockets limit, so exceeding it rejects.

Negative values clamp to 0 as before.

Verification

Three tests added to test/js/bun/http/bun-serve-args.test.ts under Bun.serve websocket options: all three fail on bun 1.4.0 (65566 accepted, 100-byte message closed with 1006, queued sends dropped) and pass with this change. The full file passes in 2.7s under a debug ASAN build.


no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-serve-args.test.ts

maxPayloadLength, idleTimeout and backpressureLimit were cast with
as u32 / as u16 before any range check, so out-of-range values wrapped:
maxPayloadLength: 2**32 + 16 behaved as 16 (every message over 16 bytes
closed the connection with 1006), idleTimeout: 65566 was accepted and
behaved as 30 seconds while idleTimeout: 961 threw, and
backpressureLimit: 2**32 + 1 behaved as 1 byte.

idleTimeout is now range-checked on the wide value before narrowing,
matching the top-level Bun.serve idleTimeout in ServerConfig.rs, so any
value above 960 throws. maxPayloadLength and backpressureLimit use the
saturating JSValue::to_u32() helper, so oversized values clamp to
u32::MAX instead of wrapping.
@coderabbitai

coderabbitai Bot commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

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

Next review available in: 17 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: 1adb6eb2-f138-4bf2-975f-1851eae439a3

📥 Commits

Reviewing files that changed from the base of the PR and between 4aeaaee and 0088337.

📒 Files selected for processing (2)
  • src/runtime/server/WebSocketServerContext.rs
  • test/js/bun/http/bun-serve-args.test.ts

Walkthrough

Changes

WebSocket option validation

Layer / File(s) Summary
Numeric option conversion
src/runtime/server/WebSocketServerContext.rs
maxPayloadLength and backpressureLimit use to_u32(). idleTimeout rejects values above 960 before narrowing to u16.
Overflow regression tests
test/js/bun/http/bun-serve-args.test.ts
Tests cover the idleTimeout boundary and oversized payload and backpressure limits through WebSocket behavior.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes validation before narrowing of Bun.serve websocket integer options.
Description check ✅ Passed The description explains the problem, fix, affected options, and verification results, covering the template requirements despite different headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/runtime/server/WebSocketServerContext.rs`:
- Around line 327-330: Update the idle_timeout validation in the
WebSocketServerContext error path to include the rejected idleTimeout value
while preserving the existing “websocket expects idleTimeout to be 960 or less”
prefix. Keep the current invalid-argument behavior unchanged and append the
supplied value plus a concrete remedy.

In `@test/js/bun/http/bun-serve-args.test.ts`:
- Around line 266-270: Update the WebSocket promise handling in
test/js/bun/http/bun-serve-args.test.ts lines 266-270 by adding an error handler
that rejects the echo promise. In lines 283-307, include reject in
Promise.withResolvers, reject on error and unexpected close events, and clear
handlers before intentional cleanup.
🪄 Autofix

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: 16c551d4-f380-42c7-962e-c279ede96777

📥 Commits

Reviewing files that changed from the base of the PR and between 4fb02aa and 4aeaaee.

📒 Files selected for processing (2)
  • src/runtime/server/WebSocketServerContext.rs
  • test/js/bun/http/bun-serve-args.test.ts

Comment thread src/runtime/server/WebSocketServerContext.rs
Comment thread test/js/bun/http/bun-serve-args.test.ts
Comment thread src/runtime/server/WebSocketServerContext.rs Outdated
@robobun

robobun commented Aug 5, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:10 PM PT - Aug 5th, 2026

❌ @robobun, your commit 0088337 has 1 failures in Build #89357 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 36998

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

bun-36998 --bun

Comment thread test/js/bun/http/bun-serve-args.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — the earlier nit about wiring reject in the backpressure test is addressed; both new WebSocket tests now reject on error/close and clear handlers before the intentional close.

Extended reasoning...

Overview

This PR fixes integer-narrowing bugs in Bun.serve({ websocket }) option validation in src/runtime/server/WebSocketServerContext.rs. Three options were being narrowed with as u32/as u16 before range checks, so out-of-range values silently wrapped: idleTimeout: 65566 was accepted (wrapped to 30) while 961 threw; maxPayloadLength: 2**32 + 16 behaved as a 16-byte cap; backpressureLimit: 2**32 + 1 behaved as 1 byte. The fix range-checks idleTimeout on the wide i64 before narrowing, and switches maxPayloadLength/backpressureLimit to the saturating JSValue::to_u32() helper. Three regression tests are added to test/js/bun/http/bun-serve-args.test.ts.

Security risks

None. This is input validation on server configuration options passed by the application author, not on network data. The change tightens validation (values that previously wrapped now clamp or throw), so the direction is strictly safer.

Level of scrutiny

Low-to-moderate. The Rust change is ~10 lines and mechanical: it applies REVIEW.md's "compare in the wide type, cast last" rule. I verified JSValue::to_u32() at src/jsc/JSValue.rs:783 is self.to_int64().clamp(0, u32::MAX as i64) as u32 — saturating as the PR description claims, so negatives → 0 and oversize → u32::MAX. The idleTimeout path now matches the pattern the PR cites in ServerConfig.rs. The is_any_int() guard remains in front of all three, so non-integral inputs still throw the existing "expects … to be an integer" error.

Other factors

All prior review threads are resolved. My earlier nit (backpressure test lacked reject wiring) is addressed in the current diff: both async tests destructure reject, wire onerror and onclose to it, and null the handlers in finally before the intentional ws.close()/server.stop(true). The comment-cop flag on a redundant comment was addressed in 0088337. The one unapplied CodeRabbit suggestion (echo the rejected idleTimeout value in the error message) is cosmetic and the thread is marked resolved; keeping the existing message text unchanged is a reasonable scope choice for this bugfix. Tests cover the boundary (960 accepted, 961 throws), the wrap points (65536, 65566, 2^32+30), and exercise the clamp behavior end-to-end over a real loopback WebSocket.

@robobun

robobun commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: 195 of 196 jobs passed. The one failed job is the Debian 13 x64-asan test lane, failing on test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts with an ASan leak in RSA key generation (BoringSSL EVP_PKEY_keygen via node:crypto). That failure is pre-existing on main and unrelated to this change; it has been reported separately. The diff itself is green on every lane, including the new tests in test/js/bun/http/bun-serve-args.test.ts.

@robobun

robobun commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

Cross-reference: #40097 (opened 2026-08-22) changes the same idleTimeout block in WebSocketServerContext::on_create. It rejects values outside 0..=960 and also rejects negative values. The two PRs will conflict on that hunk. Whichever lands second needs a rebase.

The maxPayloadLength and backpressureLimit changes exist only in this PR. On current main (abe2ad4) both options still wrap at 2^32: maxPayloadLength: 2 ** 32 + 16 closes a 100-byte message with 1006 "Received too big message".

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants