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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds configurable MCP client access control. Client declarations are extracted from ChangesClient Access Guard
Estimated code review effort: 3 (Moderate) | ~20 minutes 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
Possibly related PRs
Suggested reviewers: 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
![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
🧹 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
|
isn't there is way to disable MCP in codex? i.e you can disable the |
|
@Waishnav Thanks. Codex does expose a disable control, but DevSpace is inherited as a shared ChatGPT app rather than configured as a local [mcp_servers.devspace] entry. I tested disabling DevSpace from Codex’s Apps/Plugins UI. That change is synchronized to the shared app state and also removes/disables DevSpace in ChatGPT. OpenAI’s documentation also notes that app settings are shared and disabling an app can affect other ChatGPT experiences using the same app. The desired setup is: keep DevSpace connected and available in ChatGPT; Since there is currently no Codex-local entry for this shared app in config.toml, the server-side client policy is intended to provide that separation. |
5a28200 to
7c5c766
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in MCP client access policy to DevSpace to prevent accidental connections from disallowed clients (e.g., Codex) by inspecting initialize.params.clientInfo early in the /mcp request flow and returning a consistent access-denied JSON-RPC error when enforcement is enabled.
Changes:
- Introduces
clientAccessconfiguration (mode + denylist) loaded from env and user config. - Enforces the denylist during MCP
initialize(HTTP 403, JSON-RPC error-32003) and emits structured observation/denial logs. - Adds unit + HTTP regression tests and wires them into
npm test.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/user-config.ts | Extends the user config shape to include optional clientAccess settings. |
| src/server.ts | Enforces client access policy during initialize, returns 403 JSON-RPC error when denied, and logs observed/denied clients. |
| src/config.ts | Parses DEVSPACE_CLIENT_ACCESS_MODE / DEVSPACE_DENIED_CLIENTS and threads clientAccess into ServerConfig. |
| src/config.test.ts | Adds config parsing regression coverage for default/valid/invalid client access settings and file config loading. |
| src/client-access.ts | Implements client extraction, identity normalization, and denylist evaluation logic. |
| src/client-access.test.ts | Unit tests for client extraction and denylist decisions (including “no clientInfo” allowance behavior). |
| src/client-access-http.test.ts | HTTP-level regression test validating 403 + JSON-RPC error payload and that allowed requests continue. |
| package.json | Adds the new test files to the npm test script. |
| .env.example | Documents the new opt-in environment variables for enforcing the denylist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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