-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(mcp): add ALLOWED_MCP_DOMAINS env var for domain allowlist #3240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { mcpServers } from '@sim/db/schema' | |
| import { createLogger } from '@sim/logger' | ||
| import { and, eq, isNull } from 'drizzle-orm' | ||
| import type { NextRequest } from 'next/server' | ||
| import { McpDomainNotAllowedError, validateMcpDomain } from '@/lib/mcp/domain-check' | ||
| import { getParsedBody, withMcpAuth } from '@/lib/mcp/middleware' | ||
| import { mcpService } from '@/lib/mcp/service' | ||
| import { | ||
|
|
@@ -72,6 +73,15 @@ export const POST = withMcpAuth('write')( | |
| ) | ||
| } | ||
|
|
||
| try { | ||
| validateMcpDomain(body.url) | ||
| } catch (e) { | ||
| if (e instanceof McpDomainNotAllowedError) { | ||
| return createMcpErrorResponse(e, e.message, 403) | ||
| } | ||
| throw e | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing URL validation before domain checkMedium 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. |
||
|
|
||
| const serverId = body.url ? generateMcpServerId(workspaceId, body.url) : crypto.randomUUID() | ||
|
|
||
| const [existingServer] = await db | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { NextResponse } from 'next/server' | ||
| import { getSession } from '@/lib/auth' | ||
| import { getAllowedMcpDomainsFromEnv } from '@/lib/core/config/feature-flags' | ||
| import { getBaseUrl } from '@/lib/core/utils/urls' | ||
|
|
||
| export async function GET() { | ||
waleedlatif1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const session = await getSession() | ||
| if (!session?.user?.id) { | ||
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | ||
| } | ||
|
|
||
| const configuredDomains = getAllowedMcpDomainsFromEnv() | ||
| if (configuredDomains === null) { | ||
| return NextResponse.json({ allowedMcpDomains: null }) | ||
| } | ||
|
|
||
| try { | ||
| const platformHostname = new URL(getBaseUrl()).hostname.toLowerCase() | ||
| if (!configuredDomains.includes(platformHostname)) { | ||
| return NextResponse.json({ | ||
| allowedMcpDomains: [...configuredDomains, platformHostname], | ||
| }) | ||
| } | ||
| } catch {} | ||
|
|
||
| return NextResponse.json({ allowedMcpDomains: configuredDomains }) | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sendsurl: ""in the update, validation is skipped (empty string is falsy), and the database is updated with an unusable empty URL. ThevalidateMcpDomainfunction correctly rejects empty strings when allowlist is configured, but the guard prevents it from being called. This creates broken server state.