fix(server): bound Claude Desktop apply bodies - #1272
Conversation
📝 WalkthroughWalkthroughThe request-body reader now enforces streaming size limits and cancellation. Management routes accept optional JSON bodies, validate Claude Desktop apply modes, and defer state changes until validation succeeds. Tests cover malformed, oversized, compressed, empty, and streamed bodies. ChangesManagement request-body handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ClaudeDesktopApplyRoute
participant BoundedBodyReader
participant Configuration
Client->>ClaudeDesktopApplyRoute: Submit optional JSON body
ClaudeDesktopApplyRoute->>BoundedBodyReader: Read bounded request stream
BoundedBodyReader-->>ClaudeDesktopApplyRoute: Return body or size error
ClaudeDesktopApplyRoute->>Configuration: Validate and apply profile
Configuration-->>Client: Return success or error response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
Port the Claude Desktop apply validation change onto current dev and stop raw request streams at the byte cap before full buffering. Missing or dishonest Content-Length values now cancel at the first overflow without waiting on hostile or teed streams, while preserving abort identity and existing 413 mappings. Tests cover malformed and oversized compressed bodies, invalid profile no-mutation, absent and low Content-Length streams, exact-cap fragmentation, non-settling cancellation, and Claude Desktop config/file no-mutation.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 `@src/server/request-decompress.ts`:
- Around line 217-219: Reorder the condition in the empty-body fallback check so
the `options && "emptyBodyFallback" in options` guard is evaluated before
`text.trim() === ""`. Preserve the existing fallback behavior for opted-in
callers while ensuring callers without options skip the full-body trim.
🪄 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: 34724dbd-3c40-4b65-9d14-2c43c6f6acc0
📒 Files selected for processing (6)
src/server/management/agent-settings-routes.tssrc/server/management/body.tssrc/server/request-decompress.tstests/claude-management-api.test.tstests/native-claude-desktop-toggle.test.tstests/request-decompress.test.ts
Check optional fallback enrollment before scanning the decoded body for whitespace.
… closed (#1322) Two of the four contributor-held PRs resolved. @Wibias met every condition on #1244 -- rebase, Russian locale parity, two completed non-cancelled CI runs at the same SHA -- including resolving the conflict I created by merging #1305. Verified independently: two CI successes at d5e70a2 and a local full suite of 10120 pass / 0 fail on that head. Landed as c75e68e, 58 files. #241 closed with its chain named, since #1244 references #1056 rather than #241. #1301 rebased again after drifting 33 behind, then merged with --admin over a red test 1/4. The failure is a 5000ms timeout in tests/crash-guard.test.ts, which my one-file diff to tests/ci-workflows.test.ts cannot reach, and which passes 14/14 locally three times. Logged as MERGE-DESPITE-CI with the reason rather than as a clean green -- and recorded that this is the same shape of reasoning I criticised as 'rerun until green' earlier. #1272 hit a tenth #1302 occurrence and was rerun but NOT merged: it is still a draft and those boxes are the contributor's attestation. A broken CI is not a reason to tick them -- it is a reason not to, since they assert what CI can no longer confirm. Method note: rerun-failed-jobs overwrites the job log, so I destroyed the #1302 evidence by fetching it after the rerun. Capture first, then rerun.
Stop raw request streams at the byte cap before full buffering and move Desktop mutation after mode/profile validation. Missing or dishonest Content-Length values now cancel at the first overflow instead of waiting on hostile or teed streams, preserving abort identity and 413 mapping. Squashed from PR #1272 (contributor branch carried dev merge commits).
|
Landed on Independent security review passed. Confirmed the cap applies before allocation: The red aggregate CI on your head was an unrelated shard-4 hang in Thanks — cancelling at first overflow rather than draining the stream is the part that actually makes this a bound. |
2 participants
Summary
POST /api/claude-desktop/applythrough the shared 4 MiB management JSON reader instead of unboundedreq.text();Content-Lengthis missing, malformed, or dishonest;Why
The Claude Desktop apply route was the remaining management endpoint that parsed its body with
req.text(). A compressed or chunked request could bypass the management-body contract, and invalid input could set the persistent desired state to ON before the route returned 400.A post-
arrayBuffer()size check was still insufficient for absent or falsely lowContent-Length: the platform could drain and allocate the entire body before returning 413. The shared reader now enforces the raw bound while consuming the stream and rejects before any route mutation.Behavior and bounds
Content-Lengthis rejected and the unlocked body is canceled before reading.Request.clone()tee queues can overlap by a finite constant factor; the security contract is a bounded logical-byte limit, not an exact resident-memory figure.Verification
tests/request-decompress.test.ts31/31 passed.tests/native-claude-desktop-toggle.test.ts9/9 passed.tests/claude-management-api.test.ts25/25 passed.tests/management-integration-routes.test.ts22/22 passed.tests/claude-desktop-cli.test.ts6/6 passed.b22e0e6d0): request decompression + native Claude Desktop 40/40 passed.1272): 40/40 passed.bun x tsc --noEmit: passed.bun scripts/privacy-scan.ts: passed.git diff --check: passed.b9866e0b; thread resolved.--isolaterun hit an EBUSY storage-fixture cleanup, an unrelated management-auth 401 cascade, and then Bun'sInternal assertion failure. This matches the repository's documented dispatch-only Windows/storage-runtime class in Windows test suite remains dispatch-only until full suite is green #1059; no storage, auth-admission, or Worker-lifecycle file is changed here.Checklist
Draft pending upstream CI and the repository-required maintainer security review for management-boundary changes.
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
New Features
Bug Fixes
Tests