Skip to content

Commit 7ab2eef

Browse files
authored
fix(api): map MCP reauth redirect, deleted-account visit, and task service outage to non-500 statuses (#9051)
* fix(api): map MCP reauth redirect, deleted-account visit, and task service outage to non-500 statuses * fix(api): type the discover route test call and keep POST refresh per-server
1 parent 9f9befd commit 7ab2eef

12 files changed

Lines changed: 259 additions & 36 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import { createRouteContext } from '@sim/testing/helpers/http'
2+
import { mcpServiceMock, mcpServiceMockFns } from '@sim/testing/mocks/mcp-service.mock'
3+
import { NextRequest } from 'next/server'
4+
import { describe, expect, it, vi } from 'vitest'
5+
6+
vi.mock('@/lib/core/utils/with-route-handler', () => ({
7+
withRouteHandler: (handler: unknown) => handler,
8+
}))
9+
10+
vi.mock('@/lib/mcp/middleware', () => ({
11+
mcpBodyReadErrorResponse: vi.fn(() => null),
12+
readMcpJsonBodyWithLimit: (request: NextRequest) => request.json(),
13+
withMcpAuth:
14+
() =>
15+
(
16+
handler: (
17+
request: NextRequest,
18+
context: { userId: string; workspaceId: string; requestId: string }
19+
) => Promise<Response>
20+
) =>
21+
(request: NextRequest) =>
22+
handler(request, { userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' }),
23+
}))
24+
25+
vi.mock('@/lib/mcp/service', () => mcpServiceMock)
26+
27+
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
28+
import { GET } from '@/app/api/mcp/tools/discover/route'
29+
30+
describe('GET /api/mcp/tools/discover', () => {
31+
it('asks for reauthorization when the stored grant needs a fresh redirect', async () => {
32+
mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(
33+
new McpOauthRedirectRequired('https://auth.example.com/authorize?state=secret-state')
34+
)
35+
36+
const response = await GET(
37+
new NextRequest(
38+
'http://localhost:3000/api/mcp/tools/discover?workspaceId=workspace-1&serverId=mcp-1'
39+
),
40+
createRouteContext({})
41+
)
42+
43+
expect(response.status).toBe(401)
44+
expect(JSON.stringify(await response.json())).not.toContain('secret-state')
45+
})
46+
})

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
readMcpJsonBodyWithLimit,
1111
withMcpAuth,
1212
} from '@/lib/mcp/middleware'
13+
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
1314
import { mcpService } from '@/lib/mcp/service'
1415
import { McpOauthAuthorizationRequiredError, type McpToolDiscoveryResponse } from '@/lib/mcp/types'
1516
import { categorizeError, createMcpErrorResponse, createMcpSuccessResponse } from '@/lib/mcp/utils'
@@ -98,6 +99,7 @@ export const GET = withRouteHandler(
9899
} catch (error) {
99100
if (
100101
error instanceof McpOauthAuthorizationRequiredError ||
102+
error instanceof McpOauthRedirectRequired ||
101103
error instanceof UnauthorizedError
102104
) {
103105
return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)

‎apps/sim/app/api/mothership/tasks/[taskId]/route.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
import { getMothershipTaskStatusContract } from '@/lib/api/contracts/mothership-tasks'
22
import {
33
defineInternalJsonRoute,
4+
extendInternalErrorPolicy,
5+
internalErrorResponse,
46
internalOrchestrationErrorPolicy,
57
internalRateLimits,
68
internalSessionAuth,
79
} from '@/lib/api/server/routes'
810
import { taskOperations } from '@/lib/mothership/tasks/application/operations'
9-
import { readTaskStatus } from '@/lib/mothership/tasks/application/read-status'
11+
import {
12+
readTaskStatus,
13+
TaskServiceUnavailableError,
14+
} from '@/lib/mothership/tasks/application/read-status'
1015

1116
export const GET = defineInternalJsonRoute({
1217
contract: getMothershipTaskStatusContract,
@@ -17,7 +22,11 @@ export const GET = defineInternalJsonRoute({
1722
// Twenty active watches, each refreshing every ten seconds.
1823
config: { maxTokens: 120, refillRate: 120, refillIntervalMs: 60_000 },
1924
}),
20-
errorPolicy: internalOrchestrationErrorPolicy,
25+
errorPolicy: extendInternalErrorPolicy(internalOrchestrationErrorPolicy, (error) =>
26+
error instanceof TaskServiceUnavailableError
27+
? internalErrorResponse(error.timedOut ? 504 : 503, { error: error.message })
28+
: null
29+
),
2130
mapInput: ({ params }) => params,
2231
useCase: readTaskStatus,
2332
})

‎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 { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
2223
import {
2324
McpConnectionError,
2425
McpOauthAuthorizationRequiredError,
@@ -161,6 +162,19 @@ describe('/api/v2/mcp-servers/[mcpServerId]/tools', () => {
161162
expect(body.error.details).toEqual({ code: 'MCP_SERVER_REAUTHORIZATION_REQUIRED' })
162163
})
163164

165+
it('reports a grant that needs a fresh authorization redirect as the same 409', async () => {
166+
mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce(
167+
new McpOauthRedirectRequired('https://auth.example.com/authorize?state=secret-state')
168+
)
169+
170+
const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context })
171+
const body = await response.json()
172+
173+
expect(response.status).toBe(409)
174+
expect(body.error.details).toEqual({ code: 'MCP_SERVER_REAUTHORIZATION_REQUIRED' })
175+
expect(JSON.stringify(body)).not.toContain('secret-state')
176+
})
177+
164178
it('does not blame the caller for an upstream protocol fault', async () => {
165179
mockDiscoverMcpServerToolsUseCase.mockRejectedValueOnce(
166180
new Error('MCP error -32602: Invalid params')

‎apps/sim/app/api/v2/mcp-servers/utils.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { McpError as McpSdkError } from '@modelcontextprotocol/sdk/types.js'
44
import { type V2McpServer, v2McpServerSchema } from '@/lib/api/contracts/v2/mcp-servers'
55
import { createV2ResourceConcealmentPolicy, type V2ErrorPolicy } from '@/lib/api/server/routes'
66
import { isTimeoutError } from '@/lib/core/execution-limits'
7+
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
78
import { projectMcpHeaders } from '@/lib/mcp/projection'
89
import type { McpServerRow } from '@/lib/mcp/queries'
910
import {
@@ -74,9 +75,11 @@ function unreachableServerMessage(error: unknown): string {
7475
* cooling-down server is a retryable 503 (`v2Error` stamps it with
7576
* `Retry-After`), and a server whose stored OAuth grant no longer works is a 409
7677
* — the registration exists but its grant no longer does, which is a state
77-
* conflict a human resolves by reauthorizing. Answering all of those with a bare
78-
* 500 would make the endpoint that completes MCP onboarding indistinguishable
79-
* from a Sim outage.
78+
* conflict a human resolves by reauthorizing. A grant whose refresh failed
79+
* surfaces as `McpOauthRedirectRequired`: the SDK fell back to the browser
80+
* authorization step, which no API caller can complete. Answering all of those
81+
* with a bare 500 would make the endpoint that completes MCP onboarding
82+
* indistinguishable from a Sim outage.
8083
*
8184
* The reauthorization case deliberately does **not** reuse 401. On this surface
8285
* 401 means exactly one thing — the Sim API key is missing or invalid — and the
@@ -101,7 +104,11 @@ export const v2McpToolDiscoveryErrorPolicy = {
101104
const orchestrated = mcpServerResourceErrorPolicy.render(error)
102105
if (orchestrated) return orchestrated
103106

104-
if (error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError) {
107+
if (
108+
error instanceof McpOauthAuthorizationRequiredError ||
109+
error instanceof McpOauthRedirectRequired ||
110+
error instanceof UnauthorizedError
111+
) {
105112
return v2Error(
106113
'CONFLICT',
107114
'The MCP server must be reauthorized in Sim before its tools can be listed',
Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createMockRequest } from '@sim/testing'
1+
import { createMockRequest, dbChainMockFns, resetDbChainMock } from '@sim/testing'
22
import { createRouteContext } from '@sim/testing/helpers/http'
33
import { authMockFns } from '@sim/testing/mocks/auth.mock'
44
import { workspaceAuthzMock, workspaceAuthzMockFns } from '@sim/testing/mocks/workspace-authz.mock'
@@ -8,19 +8,13 @@ import {
88
} from '@sim/testing/mocks/workspace-context.mock'
99
import { beforeEach, describe, expect, it, vi } from 'vitest'
1010

11-
const hoisted = vi.hoisted(() => ({
12-
record: vi.fn(),
13-
}))
14-
1511
vi.mock('@sim/platform-authz/workspace', () => workspaceAuthzMock)
1612
vi.mock('@/lib/workspaces/application/context', () => workspaceContextMock)
17-
vi.mock('@/lib/workspaces/visits', () => ({ recordWorkspaceVisitRecord: hoisted.record }))
1813

1914
import { OrchestrationError } from '@/lib/core/orchestration/types'
2015
import { POST } from '@/app/api/workspaces/[id]/visit/route'
2116

2217
const mocks = {
23-
...hoisted,
2418
getSession: authMockFns.mockGetSession,
2519
role: workspaceAuthzMockFns.mockResolveEffectiveWorkspacePermission,
2620
context: workspaceContextMockFns.mockResolveActiveWorkspaceApplicationContext,
@@ -30,14 +24,14 @@ const routeContext = createRouteContext({ id: 'ws-1' })
3024

3125
describe('POST /api/workspaces/[id]/visit', () => {
3226
beforeEach(() => {
27+
resetDbChainMock()
3328
mocks.getSession.mockResolvedValue({ user: { id: 'user-1' }, session: { id: 'session-1' } })
3429
mocks.context.mockImplementation(async (workspaceId: string) => ({
3530
workspaceId,
3631
workspaceOrganizationId: null,
3732
allowPersonalApiKeys: true,
3833
}))
3934
mocks.role.mockResolvedValue('read')
40-
mocks.record.mockResolvedValue(undefined)
4135
})
4236

4337
it('answers 404 for a workspace outside the caller reach, same as a missing one', async () => {
@@ -50,6 +44,38 @@ describe('POST /api/workspaces/[id]/visit', () => {
5044
expect(denied.status).toBe(404)
5145
expect(missing.status).toBe(404)
5246
expect(await denied.json()).toEqual(await missing.json())
53-
expect(mocks.record).not.toHaveBeenCalled()
47+
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
48+
})
49+
50+
/**
51+
* Account deletion removes the user row while a page load still carries its
52+
* session, so the visit insert loses the `user_id` foreign key. The caller is
53+
* no longer signed in as anyone, which is a 401, not a server fault.
54+
*/
55+
it('answers 401 when the signed-in account is deleted before the visit lands', async () => {
56+
dbChainMockFns.onConflictDoUpdate.mockRejectedValueOnce(
57+
Object.assign(new Error('insert or update on table "workspace_visit" violates foreign key'), {
58+
code: '23503',
59+
constraint_name: 'workspace_visit_user_id_user_id_fk',
60+
})
61+
)
62+
63+
const response = await POST(createMockRequest('POST'), routeContext)
64+
65+
expect(response.status).toBe(401)
66+
expect(await response.json()).toEqual({ error: 'Unauthorized' })
67+
})
68+
69+
it('keeps any other insert failure a server fault', async () => {
70+
dbChainMockFns.onConflictDoUpdate.mockRejectedValueOnce(
71+
Object.assign(new Error('insert or update on table "workspace_visit" violates foreign key'), {
72+
code: '23503',
73+
constraint_name: 'workspace_visit_workspace_id_workspace_id_fk',
74+
})
75+
)
76+
77+
const response = await POST(createMockRequest('POST'), routeContext)
78+
79+
expect(response.status).toBe(500)
5480
})
5581
})

‎apps/sim/app/api/workspaces/[id]/visit/route.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { recordWorkspaceVisitContract } from '@/lib/api/contracts/workspaces'
22
import {
33
defineInternalJsonRoute,
4+
extendInternalErrorPolicy,
5+
internalErrorResponse,
46
internalRateLimits,
57
internalSessionAuth,
68
} from '@/lib/api/server/routes'
@@ -9,6 +11,7 @@ import {
911
recordWorkspaceVisit,
1012
workspaceVisitOperations,
1113
} from '@/lib/workspaces/application/record-workspace-visit'
14+
import { WorkspaceOwnerMissingError } from '@/lib/workspaces/policy'
1215

1316
export const POST = defineInternalJsonRoute({
1417
contract: recordWorkspaceVisitContract,
@@ -17,7 +20,14 @@ export const POST = defineInternalJsonRoute({
1720
rateLimit: internalRateLimits.none({
1821
reason: 'One idempotent upsert per workspace page load, replacing the settings write it made.',
1922
}),
20-
errorPolicy: internalWorkspaceErrorPolicies.concealWorkspaceAuthorization,
23+
errorPolicy: extendInternalErrorPolicy(
24+
internalWorkspaceErrorPolicies.concealWorkspaceAuthorization,
25+
// A session that outlived its deleted account answers as signed out.
26+
(error) =>
27+
error instanceof WorkspaceOwnerMissingError
28+
? internalErrorResponse(401, { error: 'Unauthorized' })
29+
: null
30+
),
2131
mapInput: ({ params }) => ({ workspaceId: params.id }),
2232
useCase: recordWorkspaceVisit,
2333
present: () => ({ success: true as const }),

‎apps/sim/lib/mcp/service.test.ts‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
22
import { dbChainMockFns, loggerMock, resetDbChainMock } from '@sim/testing'
3-
import { mcpOauthMock } from '@sim/testing/mocks/mcp-oauth.mock'
3+
import { McpOauthRedirectRequiredMock, mcpOauthMock } from '@sim/testing/mocks/mcp-oauth.mock'
44
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
55

66
const {
@@ -353,6 +353,31 @@ describe('McpService.discoverTools per-server caching', () => {
353353
expect(mockResolveEnvVars).toHaveBeenCalledTimes(1)
354354
})
355355

356+
/**
357+
* A refresh that fails sends the SDK to the browser authorization step, which
358+
* Sim's provider refuses by throwing. That is the same stale grant as above, so
359+
* it must not negative-cache the server: the cooldown would hide the reauth prompt.
360+
*/
361+
it('keeps a grant that needs a fresh authorization redirect soft-pending', async () => {
362+
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A', { authType: 'oauth' })])
363+
mockResolveEnvVars.mockRejectedValue(
364+
new McpOauthRedirectRequiredMock('https://auth.example.com/authorize')
365+
)
366+
367+
expect(await mcpService.discoverTools(USER_ID, WORKSPACE_ID)).toEqual([])
368+
369+
await vi.waitFor(() => {
370+
expect(dbChainMockFns.set).toHaveBeenCalledWith(
371+
expect.objectContaining({ connectionStatus: 'disconnected', lastError: null })
372+
)
373+
})
374+
expect(mockCacheAdapter.set).not.toHaveBeenCalledWith(
375+
`workspace:${WORKSPACE_ID}:server:mcp-a:failure`,
376+
[],
377+
expect.any(Number)
378+
)
379+
})
380+
356381
it('retries a transient tools/list timeout and succeeds on the second attempt', async () => {
357382
mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')])
358383
mockListTools

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,12 @@ import {
2424
validateMcpDomain,
2525
validateMcpServerSsrf,
2626
} from '@/lib/mcp/domain-check'
27-
import { getOrCreateOauthRow, loadPreregisteredClient, SimMcpOauthProvider } from '@/lib/mcp/oauth'
27+
import {
28+
getOrCreateOauthRow,
29+
loadPreregisteredClient,
30+
McpOauthRedirectRequired,
31+
SimMcpOauthProvider,
32+
} from '@/lib/mcp/oauth'
2833
import type { McpOauthCredentials } from '@/lib/mcp/oauth/coordinated-fetch'
2934
import { resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config'
3035
import {
@@ -164,6 +169,7 @@ type ServerStatusUpdate = { discoveryStartedAt?: Date } & (
164169
function isOauthAuthorizationError(error: unknown, authType: McpServerConfig['authType']): boolean {
165170
return (
166171
error instanceof McpOauthAuthorizationRequiredError ||
172+
error instanceof McpOauthRedirectRequired ||
167173
(authType === 'oauth' && error instanceof UnauthorizedError)
168174
)
169175
}

‎apps/sim/lib/mothership/tasks/application/read-status.ts‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { resolvePrincipalSubjectUserId } from '@sim/auth/principal'
2+
import { isTimeoutError } from '@/lib/core/execution-limits'
23
import { OrchestrationError } from '@/lib/core/orchestration/types'
34
import { defineAuthorizedChatUseCase } from '@/lib/mothership/chat/application/authorized-chat-use-case'
45
import { resolveOwnedChatContext } from '@/lib/mothership/chat/application/context'
@@ -11,6 +12,33 @@ import {
1112
taskOperations,
1213
} from '@/lib/mothership/tasks/application/operations'
1314

15+
/** The task service failed to answer a status read; `timedOut` separates a slow service from a down one. */
16+
export class TaskServiceUnavailableError extends Error {
17+
constructor(readonly timedOut: boolean) {
18+
super(timedOut ? 'Task service timed out' : 'Task service is unavailable')
19+
this.name = 'TaskServiceUnavailableError'
20+
}
21+
}
22+
23+
async function fetchTaskStatus(base: string, input: { taskId: string }) {
24+
try {
25+
const response = await fetchGo(`${base}/api/tasks/status`, {
26+
method: 'POST',
27+
headers: mothershipRequestHeaders(),
28+
body: JSON.stringify(input),
29+
signal: AbortSignal.timeout(10_000),
30+
redirect: 'error',
31+
spanName: 'copilot.tasks.status',
32+
})
33+
if (response.status === 404) throw new OrchestrationError('not_found', 'Task not found')
34+
if (!response.ok) throw new TaskServiceUnavailableError(false)
35+
return InternalTaskStatus.parse(await response.json())
36+
} catch (error) {
37+
if (isTimeoutError(error)) throw new TaskServiceUnavailableError(true)
38+
throw error
39+
}
40+
}
41+
1442
export const readTaskStatus = defineAuthorizedChatUseCase({
1543
operation: taskOperations.readStatus,
1644
organizationOperation: organizationTaskOperations.readStatus,
@@ -24,17 +52,7 @@ export const readTaskStatus = defineAuthorizedChatUseCase({
2452
const userId = resolvePrincipalSubjectUserId(principal)
2553
if (!userId) throw new OrchestrationError('forbidden', 'A user is required')
2654
const base = await getMothershipBaseURL({ userId })
27-
const response = await fetchGo(`${base}/api/tasks/status`, {
28-
method: 'POST',
29-
headers: mothershipRequestHeaders(),
30-
body: JSON.stringify(input),
31-
signal: AbortSignal.timeout(10_000),
32-
redirect: 'error',
33-
spanName: 'copilot.tasks.status',
34-
})
35-
if (response.status === 404) throw new OrchestrationError('not_found', 'Task not found')
36-
if (!response.ok) throw new Error('Task service is unavailable')
37-
const task = InternalTaskStatus.parse(await response.json())
55+
const task = await fetchTaskStatus(base, input)
3856
if (task.taskId !== input.taskId) throw new Error('Task service returned an unrelated task')
3957
return { ...(await resolveOwnedChatContext(principal, task.chatId)), task }
4058
},

0 commit comments

Comments
 (0)