Skip to content

Commit da5d059

Browse files
authored
fix(api): answer upstream MCP, OAuth, and id-conflict failures with 4xx; keep PII keep-alive past the load balancer (#9210)
* fix(api): answer upstream MCP, OAuth, and id-conflict failures with 4xx; keep PII keep-alive past the load balancer - MCP tool discovery answers 422 (warn) when the registered server is unreachable or cooling down - MCP OAuth start answers 422 (warn) with the server's message for any authorization-server refusal - POST /api/workflows answers 409 when a caller-supplied id already exists - PII service keeps idle connections open longer than the load balancer's idle timeout - PII mask client replaces NUL with U+FFFD so the route does not reject the whole chunk * fix(mcp): treat tools/list timeouts and HTTP errors as an unavailable server; fix test route contexts - Share the v2 discovery classification (timeout, connection, protocol, HTTP error) as isMcpServerUnavailableError and use it in the internal discover route - Restore the original POST refresh catch, which never sees per-server failures - Pass a route context in the new and existing oauth start and mask client tests; drop the oauth start test-types baseline entry
1 parent 9f7b00d commit da5d059

14 files changed

Lines changed: 380 additions & 75 deletions

File tree

‎apps/sim/app/api/mcp/oauth/start/route.test.ts‎

Lines changed: 103 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
permissionsMockFns,
1010
resetDbChainMock,
1111
} from '@sim/testing'
12+
import { createRouteContext } from '@sim/testing/helpers/http'
1213
import { NextRequest } from 'next/server'
1314
import { beforeEach, describe, expect, it, vi } from 'vitest'
1415

@@ -61,7 +62,7 @@ describe('MCP OAuth start route', () => {
6162
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
6263
)
6364

64-
await GET(request)
65+
await GET(request, createRouteContext({}))
6566

6667
// The route must call the guarded wrapper (which defaults fetchFn to the
6768
// SSRF-guarded fetch internally) rather than the raw SDK `auth()` — see
@@ -82,7 +83,7 @@ describe('MCP OAuth start route', () => {
8283
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
8384
)
8485

85-
const response = await GET(request)
86+
const response = await GET(request, createRouteContext({}))
8687

8788
expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1)
8889
expect(response.status).toBe(504)
@@ -105,7 +106,7 @@ describe('MCP OAuth start route', () => {
105106
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
106107
)
107108

108-
const response = await GET(request)
109+
const response = await GET(request, createRouteContext({}))
109110
const body = await response.json()
110111

111112
expect(response.status).toBe(409)
@@ -121,7 +122,7 @@ describe('MCP OAuth start route', () => {
121122
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
122123
)
123124

124-
const response = await GET(request)
125+
const response = await GET(request, createRouteContext({}))
125126
const body = await response.json()
126127

127128
expect(response.status).toBe(500)
@@ -138,7 +139,7 @@ describe('MCP OAuth start route', () => {
138139
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
139140
)
140141

141-
const response = await GET(request)
142+
const response = await GET(request, createRouteContext({}))
142143
const body = await response.json()
143144

144145
expect(response.status).toBe(422)
@@ -148,6 +149,103 @@ describe('MCP OAuth start route', () => {
148149
})
149150
})
150151

152+
/**
153+
* Runs the SDK's real dynamic client registration against an authorization server that
154+
* answers with `status` and `body`, so the route sees exactly the error class the SDK builds.
155+
*/
156+
async function registerAgainst(status: number, body: string) {
157+
const { registerClient } = await import('@modelcontextprotocol/sdk/client/auth.js')
158+
return registerClient('https://auth.example.com', {
159+
clientMetadata: { redirect_uris: ['https://sim.example.com/api/mcp/oauth/callback'] },
160+
fetchFn: async () => new Response(body, { status }),
161+
})
162+
}
163+
164+
describe('MCP OAuth start route: authorization-server refusals', () => {
165+
beforeEach(() => {
166+
resetDbChainMock()
167+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
168+
success: true,
169+
userId: 'user-1',
170+
userName: 'User One',
171+
userEmail: 'user1@example.com',
172+
authType: 'session',
173+
})
174+
permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
175+
dbChainMockFns.limit.mockResolvedValue([
176+
{
177+
id: 'server-1',
178+
name: 'Docs',
179+
url: 'https://mcp.example.com/mcp',
180+
workspaceId: 'workspace-1',
181+
authType: 'oauth',
182+
deletedAt: null,
183+
},
184+
])
185+
mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValue({
186+
id: 'oauth-row-1',
187+
mcpServerId: 'server-1',
188+
userId: 'user-1',
189+
workspaceId: 'workspace-1',
190+
clientInformation: null,
191+
tokens: null,
192+
codeVerifier: null,
193+
state: null,
194+
stateCreatedAt: null,
195+
updatedAt: new Date(),
196+
})
197+
mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined)
198+
})
199+
200+
const start = () =>
201+
GET(
202+
new NextRequest(
203+
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
204+
),
205+
createRouteContext({})
206+
)
207+
208+
it.each([
209+
[
210+
'an RFC 7591 code the SDK does not know, which it raises as a ServerError',
211+
400,
212+
'{"error":"invalid_redirect_uri","error_description":"Only loopback redirect URIs are supported."}',
213+
'Only loopback redirect URIs are supported.',
214+
],
215+
[
216+
'a registered OAuth code, raised as its typed subclass',
217+
400,
218+
'{"error":"invalid_client_metadata","error_description":"client_name is required"}',
219+
'invalid_client_metadata: client_name is required',
220+
],
221+
[
222+
'a non-spec body, wrapped by the SDK in a ServerError',
223+
403,
224+
'{"message":"redirect URI https://sim.example.com/api/mcp/oauth/callback is not allowed"}',
225+
'Authorization server: redirect URI https://sim.example.com/api/mcp/oauth/callback is not allowed',
226+
],
227+
] as const)(
228+
'answers 422 with the server message for %s',
229+
async (_name, status, body, message) => {
230+
mcpOauthMockFns.mockMcpAuthGuarded.mockImplementationOnce(() => registerAgainst(status, body))
231+
232+
const response = await start()
233+
234+
expect(response.status).toBe(422)
235+
expect((await response.json()).error).toBe(message)
236+
}
237+
)
238+
239+
it('keeps a 500 for a failure that is not an authorization-server answer', async () => {
240+
mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(new Error('decrypt failed'))
241+
242+
const response = await start()
243+
244+
expect(response.status).toBe(500)
245+
expect((await response.json()).error).toBe('Failed to start OAuth flow')
246+
})
247+
})
248+
151249
describe('surfaceOauthError', () => {
152250
it('parses Raw body envelope for ServerError fallbacks (non-spec vendors)', async () => {
153251
const { ServerError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js')

‎apps/sim/app/api/mcp/oauth/start/route.ts‎

Lines changed: 45 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ const DCR_UNSUPPORTED_MESSAGE =
4949
/**
5050
* The MCP SDK throws a plain `Error` (no typed class or code) when an auth server lacks a
5151
* `registration_endpoint`, so this string-match is the only available signal. Pinned to
52-
* `@modelcontextprotocol/sdk` v1.29.0 `registerClient` (client/auth.js): "Incompatible auth
52+
* `@modelcontextprotocol/sdk` v1.31.0 `registerClient` (client/auth.js): "Incompatible auth
5353
* server: does not support dynamic client registration". If a version bump rephrases this, the
5454
* 422 branch silently stops firing (users get a generic 500) — update the substring here.
5555
*/
@@ -170,43 +170,53 @@ export const GET = withRouteHandler(
170170
)
171171
const provider = new SimMcpOauthProvider({ row, preregistered })
172172

173-
try {
174-
// OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails
175-
// fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is
176-
// the SUCCESS signal (a throw carrying the authorize URL), so we catch it INSIDE the
177-
// bounded step and return it as a normal value — otherwise timedStep would error-log
178-
// every successful authorize. Only a real error or a timeout escapes as a throw.
179-
const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => {
180-
try {
181-
return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) }
182-
} catch (e) {
183-
if (e instanceof McpOauthRedirectRequired) {
184-
return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl }
173+
// OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails
174+
// fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is
175+
// the SUCCESS signal (a throw carrying the authorize URL), and an authorization-server
176+
// refusal is an expected outcome rather than a Sim fault, so both are caught INSIDE the
177+
// bounded step and returned as values — otherwise timedStep would error-log them. Only
178+
// a real error or a timeout escapes as a throw.
179+
const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => {
180+
try {
181+
return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) }
182+
} catch (e) {
183+
if (e instanceof McpOauthRedirectRequired) {
184+
return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl }
185+
}
186+
if (isDynamicClientRegistrationUnsupported(e)) {
187+
return {
188+
kind: 'rejected' as const,
189+
error: toError(e),
190+
message: DCR_UNSUPPORTED_MESSAGE,
185191
}
186-
throw e
187192
}
188-
})
189-
if (authOutcome.kind === 'redirect') {
190-
logger.info(`OAuth redirect for server ${serverId}`)
191-
return NextResponse.json({
192-
status: 'redirect',
193-
authorizationUrl: authOutcome.authorizationUrl,
194-
})
195-
}
196-
if (authOutcome.value === 'AUTHORIZED') {
197-
return NextResponse.json({ status: 'already_authorized' })
198-
}
199-
return createMcpErrorResponse(
200-
new Error('Provider did not capture redirect URL'),
201-
'Failed to start OAuth flow',
202-
500
203-
)
204-
} catch (e) {
205-
if (isDynamicClientRegistrationUnsupported(e)) {
206-
return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
193+
if (e instanceof OAuthError) {
194+
return { kind: 'rejected' as const, error: e, message: surfaceOauthError(e) }
195+
}
196+
throw e
207197
}
208-
throw e
198+
})
199+
if (authOutcome.kind === 'redirect') {
200+
logger.info(`OAuth redirect for server ${serverId}`)
201+
return NextResponse.json({
202+
status: 'redirect',
203+
authorizationUrl: authOutcome.authorizationUrl,
204+
})
209205
}
206+
if (authOutcome.kind === 'rejected') {
207+
logger.warn(`Authorization server refused the MCP OAuth flow for server ${serverId}`, {
208+
error: authOutcome.error.message,
209+
})
210+
return createMcpErrorResponse(authOutcome.error, authOutcome.message, 422)
211+
}
212+
if (authOutcome.value === 'AUTHORIZED') {
213+
return NextResponse.json({ status: 'already_authorized' })
214+
}
215+
return createMcpErrorResponse(
216+
new Error('Provider did not capture redirect URL'),
217+
'Failed to start OAuth flow',
218+
500
219+
)
210220
} catch (error) {
211221
// Any bounded step timing out (DB reads or the auth step) is a stall, not a bug —
212222
// surface it as a fast 504 so the popup closes with a clear "try again" rather than a
@@ -223,11 +233,7 @@ export const GET = withRouteHandler(
223233
)
224234
}
225235
logger.error('Error starting MCP OAuth flow:', error)
226-
// Only surface OAuth-flow errors verbatim; everything else (DB, decryption,
227-
// network) gets a generic message to avoid leaking internal details.
228-
const userMessage =
229-
error instanceof OAuthError ? surfaceOauthError(error) : 'Failed to start OAuth flow'
230-
return createMcpErrorResponse(toError(error), userMessage, 500)
236+
return createMcpErrorResponse(toError(error), 'Failed to start OAuth flow', 500)
231237
}
232238
})
233239
)

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ vi.mock('@/lib/mcp/middleware', () => ({
2424

2525
vi.mock('@/lib/mcp/service', () => mcpServiceMock)
2626

27+
import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
28+
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'
2729
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
30+
import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types'
2831
import { GET } from '@/app/api/mcp/tools/discover/route'
2932

3033
describe('GET /api/mcp/tools/discover', () => {
@@ -43,4 +46,37 @@ describe('GET /api/mcp/tools/discover', () => {
4346
expect(response.status).toBe(401)
4447
expect(JSON.stringify(await response.json())).not.toContain('secret-state')
4548
})
49+
50+
const discover = () =>
51+
GET(
52+
new NextRequest(
53+
'http://localhost:3000/api/mcp/tools/discover?workspaceId=workspace-1&serverId=mcp-1'
54+
),
55+
createRouteContext({})
56+
)
57+
58+
it.each([
59+
['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')],
60+
['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')],
61+
['times out on tools/list', () => new McpError(ErrorCode.RequestTimeout, 'Request timed out')],
62+
[
63+
'answers tools/list with an HTTP error',
64+
() => new StreamableHTTPError(503, 'Service Unavailable'),
65+
],
66+
])('answers 422 when the registered server %s', async (_name, makeError) => {
67+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError())
68+
69+
const response = await discover()
70+
71+
expect(response.status).toBe(422)
72+
expect((await response.json()).error).toBe('Failed to discover MCP tools')
73+
})
74+
75+
it('keeps a 500 for a failure that is not the registered server', async () => {
76+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(new Error('pool exhausted'))
77+
78+
const response = await discover()
79+
80+
expect(response.status).toBe(500)
81+
})
4682
})

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

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,48 @@ import {
1313
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
1414
import { mcpService } from '@/lib/mcp/service'
1515
import { McpOauthAuthorizationRequiredError, type McpToolDiscoveryResponse } from '@/lib/mcp/types'
16-
import { categorizeError, createMcpErrorResponse, createMcpSuccessResponse } from '@/lib/mcp/utils'
16+
import {
17+
categorizeError,
18+
createMcpErrorResponse,
19+
createMcpSuccessResponse,
20+
isMcpServerUnavailableError,
21+
} from '@/lib/mcp/utils'
1722

1823
const logger = createLogger('McpToolDiscoveryAPI')
1924
const MCP_REFRESH_DISCOVERY_CONCURRENCY = 5
2025

2126
export const dynamic = 'force-dynamic'
2227

28+
/**
29+
* Renders a discovery failure that is not an authorization problem.
30+
*
31+
* Discovery talks to a server the workspace registered, so a server that cannot be
32+
* reached, times out on connect or on `tools/list`, answers with a protocol or HTTP
33+
* error, or is cooling down after a recent failure is a fault of that server rather
34+
* than of Sim: it answers 422 (the request was valid; the dependency it names cannot
35+
* serve it), the status this API family already uses for an upstream that cannot
36+
* complete the operation, and logs at warn. Anything else keeps {@link categorizeError}'s
37+
* status and logs at error only when that status is a 5xx.
38+
*/
39+
function discoveryFailureResponse(requestId: string, error: unknown) {
40+
if (isMcpServerUnavailableError(error)) {
41+
logger.warn(`[${requestId}] MCP server unavailable for tool discovery`, {
42+
error: getErrorMessage(error),
43+
})
44+
return createMcpErrorResponse(error, 'Failed to discover MCP tools', 422)
45+
}
46+
const { message, status } = categorizeError(error)
47+
if (status >= 500) {
48+
logger.error(`[${requestId}] Error discovering MCP tools:`, error)
49+
} else {
50+
logger.warn(`[${requestId}] Failed to discover MCP tools`, {
51+
error: getErrorMessage(error),
52+
status,
53+
})
54+
}
55+
return createMcpErrorResponse(new Error(message), 'Failed to discover MCP tools', status)
56+
}
57+
2358
async function settleWithConcurrency<T, R>(
2459
items: T[],
2560
concurrency: number,
@@ -104,9 +139,7 @@ export const GET = withRouteHandler(
104139
) {
105140
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
106141
}
107-
logger.error(`[${requestId}] Error discovering MCP tools:`, error)
108-
const { message, status } = categorizeError(error)
109-
return createMcpErrorResponse(new Error(message), 'Failed to discover MCP tools', status)
142+
return discoveryFailureResponse(requestId, error)
110143
}
111144
})
112145
)

0 commit comments

Comments
 (0)