From 9d4550f1e622d9c36cb5fbf6322d5720eac09e93 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 23 Oct 2026 12:58:06 -0700 Subject: [PATCH] fix(mcp): classify unresolvable and egress-refused server URLs in discovery An MCP server whose hostname no longer resolves made tool discovery answer 500 and log at error. McpDnsResolutionError is now a server-unavailable condition (discover 422, v2 503), and categorizeError maps it to 502 and egress/domain refusals to 403, matching test-connection and server lifecycle. --- .../app/api/mcp/tools/discover/route.test.ts | 17 +++++++++++++ .../[mcpServerId]/tools/route.test.ts | 14 +++++++++++ apps/sim/lib/mcp/utils.ts | 25 ++++++++++++++----- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/apps/sim/app/api/mcp/tools/discover/route.test.ts b/apps/sim/app/api/mcp/tools/discover/route.test.ts index 7d0eed851e0..eede45ab3f4 100644 --- a/apps/sim/app/api/mcp/tools/discover/route.test.ts +++ b/apps/sim/app/api/mcp/tools/discover/route.test.ts @@ -26,6 +26,11 @@ vi.mock('@/lib/mcp/service', () => mcpServiceMock) import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js' import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js' +import { + McpDnsResolutionError, + McpDomainNotAllowedError, + McpSsrfError, +} from '@/lib/mcp/domain-check' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types' import { GET } from '@/app/api/mcp/tools/discover/route' @@ -56,6 +61,7 @@ describe('GET /api/mcp/tools/discover', () => { ) it.each([ + ['hostname no longer resolves', () => new McpDnsResolutionError('expired.example.com')], ['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')], ['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')], ['times out on tools/list', () => new McpError(ErrorCode.RequestTimeout, 'Request timed out')], @@ -72,6 +78,17 @@ describe('GET /api/mcp/tools/discover', () => { expect((await response.json()).error).toBe('Failed to discover MCP tools') }) + it.each([ + ['resolves to a blocked address', () => new McpSsrfError('blocked address')], + ['is outside ALLOWED_MCP_DOMAINS', () => new McpDomainNotAllowedError('internal.example.com')], + ])('answers 403 when the egress policy refuses a server that %s', async (_name, makeError) => { + mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError()) + + const response = await discover() + + expect(response.status).toBe(403) + }) + it('keeps a 500 for a failure that is not the registered server', async () => { mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(new Error('pool exhausted')) diff --git a/apps/sim/app/api/v2/mcp-servers/[mcpServerId]/tools/route.test.ts b/apps/sim/app/api/v2/mcp-servers/[mcpServerId]/tools/route.test.ts index e07091edd07..6e691a2cb23 100644 --- a/apps/sim/app/api/v2/mcp-servers/[mcpServerId]/tools/route.test.ts +++ b/apps/sim/app/api/v2/mcp-servers/[mcpServerId]/tools/route.test.ts @@ -19,6 +19,7 @@ vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock) import { NoWorkspaceAccessError, WorkspaceApiKeyAuthorizationError } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' +import { McpDnsResolutionError } from '@/lib/mcp/domain-check' import { McpOauthRedirectRequired } from '@/lib/mcp/oauth' import { McpConnectionError, @@ -149,6 +150,19 @@ describe('/api/v2/mcp-servers/[mcpServerId]/tools', () => { expect(JSON.stringify(body)).not.toContain('ECONNREFUSED') }) + it('reports a server whose hostname no longer resolves as a retryable 503', async () => { + mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce( + new McpDnsResolutionError('expired.example.com') + ) + + const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context }) + const body = await response.json() + + expect(response.status).toBe(503) + expect(body.error.code).toBe('SERVICE_UNAVAILABLE') + expect(JSON.stringify(body)).not.toContain('expired.example.com') + }) + it('reports a stale OAuth grant as a 409 a client can branch on, never as a Sim credential failure', async () => { mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce( new McpOauthAuthorizationRequiredError(SERVER_ID, 'Docs server') diff --git a/apps/sim/lib/mcp/utils.ts b/apps/sim/lib/mcp/utils.ts index 72a9ba7c5e7..b8ab227c82b 100644 --- a/apps/sim/lib/mcp/utils.ts +++ b/apps/sim/lib/mcp/utils.ts @@ -4,6 +4,11 @@ import { McpError as McpSdkError } from '@modelcontextprotocol/sdk/types.js' import { generateShortId } from '@sim/utils/id' import { NextResponse } from 'next/server' import { DEFAULT_EXECUTION_TIMEOUT_MS, isTimeoutError } from '@/lib/core/execution-limits' +import { + McpDnsResolutionError, + McpDomainNotAllowedError, + McpSsrfError, +} from '@/lib/mcp/domain-check' import { type McpApiResponse, McpConnectionError, @@ -139,16 +144,18 @@ export function mcpOrchestrationStatus(errorCode: string | undefined): number { } /** - * Reports whether a registered MCP server failed to answer usefully: it could not be - * reached, timed out, is cooling down after a recent failure, or answered `tools/list` - * with a protocol or HTTP error. Typed only, never matched on message text, because - * `McpConnectionError` interpolates the server's display name into its message. Check - * authorization errors first: `UnauthorizedError` is not one of these, but a caller that - * wants to answer reauthorization separately must do so before asking. + * Reports whether a registered MCP server failed to answer usefully: its hostname no + * longer resolves, it could not be reached, timed out, is cooling down after a recent + * failure, or answered `tools/list` with a protocol or HTTP error. Typed only, never + * matched on message text, because `McpConnectionError` interpolates the server's display + * name into its message. Check authorization errors first: `UnauthorizedError` is not one + * of these, but a caller that wants to answer reauthorization separately must do so + * before asking. */ export function isMcpServerUnavailableError(error: unknown): boolean { return ( isTimeoutError(error) || + error instanceof McpDnsResolutionError || error instanceof McpConnectionError || error instanceof McpSdkError || error instanceof StreamableHTTPError @@ -174,6 +181,12 @@ export function categorizeError(error: unknown): { message: string; status: numb if (error instanceof McpConnectionError) { return { message: 'Connection failed', status: 502 } } + if (error instanceof McpDnsResolutionError) { + return { message: 'Server hostname could not be resolved', status: 502 } + } + if (error instanceof McpSsrfError || error instanceof McpDomainNotAllowedError) { + return { message: 'Server URL is not allowed by egress policy', status: 403 } + } // Fall back to substring matching for SDK / third-party errors we don't // own a typed class for.