feat(mcp): add ALLOWED_MCP_DOMAINS env var for domain allowlist#3240
feat(mcp): add ALLOWED_MCP_DOMAINS env var for domain allowlist#3240waleedlatif1 merged 3 commits intostagingfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryAdds an
Confidence Score: 3/5
Important Files Changed
Flowchartflowchart TD
A[Client: MCP Settings UI] -->|fetch /api/settings/allowed-mcp-domains| B[GET allowed-mcp-domains route]
B -->|reads env| C[getAllowedMcpDomainsFromEnv]
C -->|returns domain list or null| B
B -->|+ platform hostname| A
A -->|user enters URL| D{isDomainAllowed?}
D -->|No| E[Show inline error, disable buttons]
D -->|Yes| F[Submit to API]
F -->|POST /api/mcp/servers| G[POST route]
F -->|PATCH /api/mcp/servers/:id| H[PATCH route]
F -->|POST /api/mcp/servers/test-connection| I[Test Connection route]
G --> J{validateMcpDomain}
H --> J
I --> J
J -->|Domain blocked| K[403 McpDomainNotAllowedError]
J -->|Domain allowed| L[Continue normal flow]
M[McpService.getServerConfig] --> N{isMcpDomainAllowed}
N -->|No| O[Return null / filter out]
N -->|Yes| P[Return server config]
Q[McpService.getWorkspaceServers] --> R{isMcpDomainAllowed filter}
R -->|Blocked servers removed| S[Return filtered list]
J --> T[checkMcpDomain]
N --> T
T --> U[getAllowedMcpDomainsFromEnv]
T --> V[getPlatformHostname - cached]
Last reviewed commit: 9eda826 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
...orkspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/mcp/mcp.tsx
Show resolved
Hide resolved
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return createMcpErrorResponse(e, e.message, 403) | ||
| } | ||
| throw e | ||
| } |
There was a problem hiding this comment.
Missing URL validation before domain check
Medium Severity
The POST endpoint validates domain before checking if URL exists. When an allowlist is configured and URL is missing, users get "Domain (empty) is not allowed" instead of "URL is required", creating confusing error messages. The test-connection endpoint correctly checks URL presence first (lines 67-73), but the POST endpoint doesn't follow this pattern.
| createdAt: server.createdAt.toISOString(), | ||
| updatedAt: server.updatedAt.toISOString(), | ||
| })) | ||
| .filter((config) => isMcpDomainAllowed(config.url)) |
There was a problem hiding this comment.
Cached tools bypass new domain filtering
Medium Severity
Tool discovery returns cached results without applying the new domain filtering. When ALLOWED_MCP_DOMAINS changes (requiring restart), persistent caches like Redis still contain tools from now-disallowed servers, which get returned directly. While tool execution would fail (due to getServerConfig filtering), the tools appear in discovery results, causing confusing UX where users see tools they can't use.
| } | ||
| throw e | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty URL bypasses domain validation in PATCH
Medium Severity
The PATCH endpoint uses a truthy check if (updateData.url) to guard domain validation, allowing empty strings to bypass validation. When an allowlist is configured and a client sends url: "" in the update, validation is skipped (empty string is falsy), and the database is updated with an unusable empty URL. The validateMcpDomain function correctly rejects empty strings when allowlist is configured, but the guard prevents it from being called. This creates broken server state.
1 participant
Summary
ALLOWED_MCP_DOMAINSenv var to restrict which external MCP server domains users can connect togetBaseUrl()) is always implicitly allowedType of Change
Testing
Tested manually
Checklist