Skip to content

Commit 897a0e0

Browse files
committed
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
1 parent 9cdc1bd commit 897a0e0

11 files changed

Lines changed: 337 additions & 56 deletions

File tree

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,102 @@ describe('MCP OAuth start route', () => {
148148
})
149149
})
150150

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

2727
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
28+
import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types'
2829
import { GET } from '@/app/api/mcp/tools/discover/route'
2930

3031
describe('GET /api/mcp/tools/discover', () => {
@@ -43,4 +44,32 @@ describe('GET /api/mcp/tools/discover', () => {
4344
expect(response.status).toBe(401)
4445
expect(JSON.stringify(await response.json())).not.toContain('secret-state')
4546
})
47+
48+
const discover = () =>
49+
GET(
50+
new NextRequest(
51+
'http://localhost:3000/api/mcp/tools/discover?workspaceId=workspace-1&serverId=mcp-1'
52+
),
53+
createRouteContext({})
54+
)
55+
56+
it.each([
57+
['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')],
58+
['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')],
59+
])('answers 422 when the registered server %s', async (_name, makeError) => {
60+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError())
61+
62+
const response = await discover()
63+
64+
expect(response.status).toBe(422)
65+
expect((await response.json()).error).toBe('Failed to discover MCP tools')
66+
})
67+
68+
it('keeps a 500 for a failure that is not the registered server', async () => {
69+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(new Error('pool exhausted'))
70+
71+
const response = await discover()
72+
73+
expect(response.status).toBe(500)
74+
})
4675
})

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

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,44 @@ import {
1212
} from '@/lib/mcp/middleware'
1313
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
1414
import { mcpService } from '@/lib/mcp/service'
15-
import { McpOauthAuthorizationRequiredError, type McpToolDiscoveryResponse } from '@/lib/mcp/types'
15+
import {
16+
McpConnectionError,
17+
McpOauthAuthorizationRequiredError,
18+
type McpToolDiscoveryResponse,
19+
} from '@/lib/mcp/types'
1620
import { categorizeError, createMcpErrorResponse, createMcpSuccessResponse } from '@/lib/mcp/utils'
1721

1822
const logger = createLogger('McpToolDiscoveryAPI')
1923
const MCP_REFRESH_DISCOVERY_CONCURRENCY = 5
2024

2125
export const dynamic = 'force-dynamic'
2226

27+
/**
28+
* Renders a discovery failure that is not an authorization problem.
29+
*
30+
* Discovery talks to a server the workspace registered, so a server that cannot be
31+
* reached, times out, or is cooling down after a recent failure is a fault of that
32+
* server rather than of Sim: it answers 422 (the request was valid; the dependency it
33+
* names cannot serve it), the status this API family already uses for an upstream that
34+
* cannot complete the operation, and logs at warn. Anything else keeps
35+
* {@link categorizeError}'s status and logs at error only when that status is a 5xx.
36+
*/
37+
function discoveryFailureResponse(requestId: string, error: unknown, defaultMessage: string) {
38+
if (error instanceof McpConnectionError) {
39+
logger.warn(`[${requestId}] MCP server unavailable for tool discovery`, {
40+
error: getErrorMessage(error),
41+
})
42+
return createMcpErrorResponse(error, defaultMessage, 422)
43+
}
44+
const { message, status } = categorizeError(error)
45+
if (status >= 500) {
46+
logger.error(`[${requestId}] ${defaultMessage}:`, error)
47+
} else {
48+
logger.warn(`[${requestId}] ${defaultMessage}`, { error: getErrorMessage(error), status })
49+
}
50+
return createMcpErrorResponse(new Error(message), defaultMessage, status)
51+
}
52+
2353
async function settleWithConcurrency<T, R>(
2454
items: T[],
2555
concurrency: number,
@@ -104,9 +134,7 @@ export const GET = withRouteHandler(
104134
) {
105135
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
106136
}
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)
137+
return discoveryFailureResponse(requestId, error, 'Failed to discover MCP tools')
110138
}
111139
})
112140
)
@@ -171,9 +199,7 @@ export const POST = withRouteHandler(
171199
) {
172200
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
173201
}
174-
logger.error(`[${requestId}] Error refreshing tool discovery:`, error)
175-
const { message, status } = categorizeError(error)
176-
return createMcpErrorResponse(new Error(message), 'Failed to refresh tool discovery', status)
202+
return discoveryFailureResponse(requestId, error, 'Failed to refresh tool discovery')
177203
}
178204
})
179205
)

‎apps/sim/lib/guardrails/mask-client.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { resetUrlsMock, urlsMockFns } from '@sim/testing'
22
import { authInternalMock, authInternalMockFns } from '@sim/testing/mocks/auth-internal.mock'
3+
import { hybridAuthMockFns } from '@sim/testing/mocks/hybrid-auth.mock'
34
import { utilsHelpersMock, utilsHelpersMockFns } from '@sim/testing/mocks/utils-helpers.mock'
5+
import { NextRequest } from 'next/server'
46
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
57

68
afterAll(resetUrlsMock)
@@ -13,6 +15,7 @@ vi.mock('@/lib/auth/internal', () => authInternalMock)
1315
vi.mock('@sim/utils/helpers', () => utilsHelpersMock)
1416

1517
import { maskPIIBatchViaHttp } from '@/lib/guardrails/mask-client'
18+
import { POST as maskBatchRoute } from '@/app/api/guardrails/mask-batch/route'
1619

1720
const mockSleep = utilsHelpersMockFns.mockSleep
1821

@@ -119,4 +122,49 @@ describe('maskPIIBatchViaHttp', () => {
119122
expect(fetchMock).toHaveBeenCalledTimes(1)
120123
expect(mockSleep).not.toHaveBeenCalled()
121124
})
125+
126+
describe('through the real mask-batch route', () => {
127+
let presidioTexts: string[][]
128+
129+
beforeEach(() => {
130+
presidioTexts = []
131+
hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({ success: true })
132+
fetchMock.mockImplementation(
133+
async (url: string, init: { body: string; headers: Record<string, string> }) => {
134+
if (url.startsWith('http://app.internal:3000')) {
135+
return maskBatchRoute(
136+
new NextRequest(url, { method: 'POST', body: init.body, headers: init.headers })
137+
)
138+
}
139+
const { texts } = JSON.parse(init.body) as { texts: string[] }
140+
presidioTexts.push(texts)
141+
return Response.json({
142+
texts: texts.map((t) => t.replace('ada@example.com', '<EMAIL>')),
143+
})
144+
}
145+
)
146+
})
147+
148+
it('masks a chunk whose strings carry a NUL instead of failing the whole chunk', async () => {
149+
const { masked, scrubbedCount } = await maskPIIBatchViaHttp(
150+
['mail ada@example.com\u0000end', 'plain text'],
151+
['EMAIL_ADDRESS'],
152+
undefined,
153+
undefined,
154+
{ failedChunkPlaceholder: '[FAILED]' }
155+
)
156+
157+
expect(scrubbedCount).toBe(0)
158+
expect(masked).toEqual(['mail <EMAIL>\uFFFDend', 'plain text'])
159+
})
160+
161+
it('sends text without a NUL to Presidio unchanged', async () => {
162+
const text = 'mail ada@example.com \u{1F600} done'
163+
164+
const { masked } = await maskPIIBatchViaHttp([text], ['EMAIL_ADDRESS'])
165+
166+
expect(presidioTexts).toEqual([[text]])
167+
expect(masked).toEqual(['mail <EMAIL> \u{1F600} done'])
168+
})
169+
})
122170
})

0 commit comments

Comments
 (0)