Skip to content

Commit 4d26987

Browse files
authored
fix(mcp): classify unresolvable and egress-refused server URLs in discovery (#9341)
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.
1 parent 0f755fe commit 4d26987

3 files changed

Lines changed: 50 additions & 6 deletions

File tree

‎apps/sim/app/api/mcp/tools/discover/route.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ vi.mock('@/lib/mcp/service', () => mcpServiceMock)
2626

2727
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
2828
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'
29+
import {
30+
McpDnsResolutionError,
31+
McpDomainNotAllowedError,
32+
McpSsrfError,
33+
} from '@/lib/mcp/domain-check'
2934
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
3035
import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types'
3136
import { GET } from '@/app/api/mcp/tools/discover/route'
@@ -56,6 +61,7 @@ describe('GET /api/mcp/tools/discover', () => {
5661
)
5762

5863
it.each([
64+
['hostname no longer resolves', () => new McpDnsResolutionError('expired.example.com')],
5965
['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')],
6066
['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')],
6167
['times out on tools/list', () => new McpError(ErrorCode.RequestTimeout, 'Request timed out')],
@@ -72,6 +78,17 @@ describe('GET /api/mcp/tools/discover', () => {
7278
expect((await response.json()).error).toBe('Failed to discover MCP tools')
7379
})
7480

81+
it.each([
82+
['resolves to a blocked address', () => new McpSsrfError('blocked address')],
83+
['is outside ALLOWED_MCP_DOMAINS', () => new McpDomainNotAllowedError('internal.example.com')],
84+
])('answers 403 when the egress policy refuses a server that %s', async (_name, makeError) => {
85+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError())
86+
87+
const response = await discover()
88+
89+
expect(response.status).toBe(403)
90+
})
91+
7592
it('keeps a 500 for a failure that is not the registered server', async () => {
7693
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(new Error('pool exhausted'))
7794

‎apps/sim/app/api/v2/mcp-servers/[mcpServerId]/tools/route.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ vi.mock('@/lib/mcp/application/use-cases', () => mcpUseCasesMock)
1919

2020
import { NoWorkspaceAccessError, WorkspaceApiKeyAuthorizationError } from '@/lib/core/application'
2121
import { OrchestrationError } from '@/lib/core/orchestration/types'
22+
import { McpDnsResolutionError } from '@/lib/mcp/domain-check'
2223
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
2324
import {
2425
McpConnectionError,
@@ -149,6 +150,19 @@ describe('/api/v2/mcp-servers/[mcpServerId]/tools', () => {
149150
expect(JSON.stringify(body)).not.toContain('ECONNREFUSED')
150151
})
151152

153+
it('reports a server whose hostname no longer resolves as a retryable 503', async () => {
154+
mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce(
155+
new McpDnsResolutionError('expired.example.com')
156+
)
157+
158+
const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context })
159+
const body = await response.json()
160+
161+
expect(response.status).toBe(503)
162+
expect(body.error.code).toBe('SERVICE_UNAVAILABLE')
163+
expect(JSON.stringify(body)).not.toContain('expired.example.com')
164+
})
165+
152166
it('reports a stale OAuth grant as a 409 a client can branch on, never as a Sim credential failure', async () => {
153167
mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce(
154168
new McpOauthAuthorizationRequiredError(SERVER_ID, 'Docs server')

‎apps/sim/lib/mcp/utils.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { McpError as McpSdkError } from '@modelcontextprotocol/sdk/types.js'
44
import { generateShortId } from '@sim/utils/id'
55
import { NextResponse } from 'next/server'
66
import { DEFAULT_EXECUTION_TIMEOUT_MS, isTimeoutError } from '@/lib/core/execution-limits'
7+
import {
8+
McpDnsResolutionError,
9+
McpDomainNotAllowedError,
10+
McpSsrfError,
11+
} from '@/lib/mcp/domain-check'
712
import {
813
type McpApiResponse,
914
McpConnectionError,
@@ -139,16 +144,18 @@ export function mcpOrchestrationStatus(errorCode: string | undefined): number {
139144
}
140145

141146
/**
142-
* Reports whether a registered MCP server failed to answer usefully: it could not be
143-
* reached, timed out, is cooling down after a recent failure, or answered `tools/list`
144-
* with a protocol or HTTP error. Typed only, never matched on message text, because
145-
* `McpConnectionError` interpolates the server's display name into its message. Check
146-
* authorization errors first: `UnauthorizedError` is not one of these, but a caller that
147-
* wants to answer reauthorization separately must do so before asking.
147+
* Reports whether a registered MCP server failed to answer usefully: its hostname no
148+
* longer resolves, it could not be reached, timed out, is cooling down after a recent
149+
* failure, or answered `tools/list` with a protocol or HTTP error. Typed only, never
150+
* matched on message text, because `McpConnectionError` interpolates the server's display
151+
* name into its message. Check authorization errors first: `UnauthorizedError` is not one
152+
* of these, but a caller that wants to answer reauthorization separately must do so
153+
* before asking.
148154
*/
149155
export function isMcpServerUnavailableError(error: unknown): boolean {
150156
return (
151157
isTimeoutError(error) ||
158+
error instanceof McpDnsResolutionError ||
152159
error instanceof McpConnectionError ||
153160
error instanceof McpSdkError ||
154161
error instanceof StreamableHTTPError
@@ -174,6 +181,12 @@ export function categorizeError(error: unknown): { message: string; status: numb
174181
if (error instanceof McpConnectionError) {
175182
return { message: 'Connection failed', status: 502 }
176183
}
184+
if (error instanceof McpDnsResolutionError) {
185+
return { message: 'Server hostname could not be resolved', status: 502 }
186+
}
187+
if (error instanceof McpSsrfError || error instanceof McpDomainNotAllowedError) {
188+
return { message: 'Server URL is not allowed by egress policy', status: 403 }
189+
}
177190

178191
// Fall back to substring matching for SDK / third-party errors we don't
179192
// own a typed class for.

0 commit comments

Comments
 (0)