fix(server): bound live sideband websocket frames - #1381
Conversation
📝 WalkthroughWalkthroughLive sideband WebSocket handling now enforces a 50 MiB payload limit and a cumulative pending-buffer byte limit. It tracks pending bytes, rejects frame-count and byte overflows, closes oversized frames with code 1009, resets accounting during lifecycle transitions, and adds boundary and relay tests. ChangesLive sideband limits
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant LiveSideband
participant LiveUpstream
WebSocketClient->>LiveSideband: Send client frame
LiveSideband->>LiveSideband: Check frame and pending-byte limits
LiveSideband->>LiveUpstream: Forward accepted or queued frame
LiveUpstream->>LiveSideband: Send upstream frame
LiveSideband->>LiveSideband: Check upstream frame size
LiveSideband->>WebSocketClient: Forward accepted frame
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
b5000d5 to
5d37fc3
Compare
5d37fc3 to
48a99aa
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: 1
🤖 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 `@tests/server-live.test.ts`:
- Line 573: Remove the duplicate const declarations for byteBounded and
countBounded within the same test callback, retaining one declaration of each
and preserving their existing initial values and usage.
🪄 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 Plus
Run ID: 7eaaace6-4779-4401-bc29-072e926be138
📒 Files selected for processing (3)
src/server/index.tssrc/server/ws-bridge.tstests/server-live.test.ts
|
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a99aaa6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== src/server/index.ts structure ==='
ast-grep outline src/server/index.ts --items all --type function --match 'LiveSideband|liveSideband|enqueue|final' || true
echo '=== Relevant implementation ==='
rg -n -C 8 'LIVE_SIDEBAND|LiveSideband|livePending|livePendingBytes|enqueueLiveSideband|finalize|\.close\s*\(|\.open\s*\(' \
src/server/index.ts src/server/ws-bridge.ts
echo '=== Focused tests ==='
rg -n -C 5 'LiveSideband|livePending|frame.*limit|pending.*limit|1009' tests/server-live.test.ts
echo '=== Current diff ==='
git diff --no-ext-diff --unified=80 HEAD^ HEAD -- \
src/server/index.ts src/server/ws-bridge.ts tests/server-live.test.tsLength of output: 89809
The cleanup paths clear
I did not run tests in the inspection sandbox. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
c76fee0 to
dc4dd45
Compare
|
Superseded by #1398 — merged to Thank you for the work on this. The live sideband relay path needed explicit bounds on both directions: without them, oversized WebSocket frames or unbounded pending buffers while the upstream socket connects could let a client pin memory or pass frames the proxy was never meant to retain. This fix is useful because it:
Closing in favor of #1398. |
Supersedes #1381. Rebased onto latest dev. - Apply 50 MiB WebSocket frame ceiling to both directions - Cap pending client frames at 32 frames and 1 MiB total - Raise macOS CI test timeout for 50 MiB frame relay test
2 participants
Summary
Verification
bun test tests/server-live.test.ts— 29 passed, 0 failed.bun run typecheck— passed.bun run privacy:scan— passed.git diff --check HEAD^ HEAD— passed.bun run testwas attempted once; unrelated current-dev Windows baseline failures appeared in API usage, CLI restore/auth, catalog, and history paths, then Bun 1.3.14 panicked after 1,089 seconds. All changed Live tests passed before the crash.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit