feat: add MCP client access policy#84
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds configurable MCP client access control. Client declarations are extracted from ChangesClient Access Guard
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPServer
participant ClientAccessPolicy
MCPClient->>MCPServer: Send MCP initialize request
MCPServer->>ClientAccessPolicy: Extract declared client
ClientAccessPolicy-->>MCPServer: Return normalized client
MCPServer->>ClientAccessPolicy: Evaluate configured access policy
ClientAccessPolicy-->>MCPServer: Return allow or deny decision
MCPServer-->>MCPClient: Continue or return JSON-RPC 403
Poem
🚥 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 |
8d925bc to
e2e032a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
package.json (1)
31-31: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an HTTP-level regression test for initialize denial.
The new command runs policy-unit tests only. Add a server test proving a denied initialize request returns HTTP 403/code
-32003, while missingclientInfoproceeds, before any transport or session is created.🤖 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 `@package.json` at line 31, Add an HTTP-level regression test to the server test suite, alongside the existing initialize/policy tests, covering both cases: a denied initialize request returns HTTP 403 with code -32003, while an initialize request without clientInfo proceeds before creating any transport or session. Update the package.json test script’s test command to execute the new server test.
🤖 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/config.ts`:
- Around line 129-150: Update parseClientAccessMode to distinguish undefined
from an empty string: default to "off" only when value is undefined, while
treating "" as invalid and throwing the existing configuration error. Preserve
the "off" and "enforce" handling for their explicit values and keep
parseClientAccessConfig’s environment-variable precedence unchanged.
In `@src/server.ts`:
- Around line 1762-1765: Update the denial branch around sendJsonRpcError in the
initialize request handling to pass the validated initialize request’s JSON-RPC
ID, including 0, through the error-response helper. Extend or reuse
sendJsonRpcError so it emits that ID instead of always using null, while
retaining null only when the request ID cannot be established.
---
Nitpick comments:
In `@package.json`:
- Line 31: Add an HTTP-level regression test to the server test suite, alongside
the existing initialize/policy tests, covering both cases: a denied initialize
request returns HTTP 403 with code -32003, while an initialize request without
clientInfo proceeds before creating any transport or session. Update the
package.json test script’s test command to execute the new server test.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8da7000a-369f-4b92-a7f1-724333f28395
📒 Files selected for processing (8)
.env.examplepackage.jsonsrc/client-access.test.tssrc/client-access.tssrc/config.test.tssrc/config.tssrc/server.tssrc/user-config.ts
e2e032a to
5a28200
Compare
Summary
offandenforcemodesinitialize.params.clientInfobefore creating the MCP transport-32003clientInfoMotivation
ChatGPT and Codex currently share connected apps. DevSpace is intended to let ChatGPT control the local PC remotely, while Codex already runs locally and should use its native filesystem, shell, Git, and patch tools.
Without a server-side guard, Codex may accidentally invoke DevSpace instead of its native tools.
Behavior
Enable the guard with:
Clients matching the denylist are rejected before an MCP transport or workspace session is created. Clients that omit
clientInfoare allowed to preserve current ChatGPT compatibility.Practical deployment testing confirmed:
open_workspacesucceededopen_workspacereturned HTTP 403, JSON-RPC-32003, andMCP client not allowedclientInfois self-declared, so this is a guard against accidental client use rather than cryptographic isolation.Verification
npm testnpm run typechecknpm run buildgit diff upstream/main...HEAD --checkSummary by CodeRabbit
initializerequests.