diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 79b7058ddea..9d63191fb32 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -16,55 +16,17 @@ import { loadOauthRowByState, loadPreregisteredClient, type McpOauthCallbackReason, + makeTimedStep, mcpAuthGuarded, SimMcpOauthProvider, } from '@/lib/mcp/oauth' import { mcpService } from '@/lib/mcp/service' const logger = createLogger('McpOauthCallbackAPI') +const timedStep = makeTimedStep(logger) export const dynamic = 'force-dynamic' -class OauthCallbackStepTimeout extends Error { - constructor(step: string, ms: number) { - super(`MCP OAuth callback step "${step}" did not settle within ${ms}ms`) - this.name = 'OauthCallbackStepTimeout' - } -} - -/** - * Times and bounds one awaited step of the callback so a stalled operation - * surfaces as a labeled, logged error instead of hanging the request forever. - * The losing promise is not cancelled (a wedged DB/socket op can't be), so it - * settles in the background with its rejection swallowed; the point is that the - * request stops waiting on it and the logs name the exact step that stalled. - */ -async function timedStep(step: string, ms: number, fn: () => Promise): Promise { - const start = Date.now() - logger.info(`OAuth callback step start: ${step}`) - const work = Promise.resolve(fn()) - work.catch(() => {}) - let timer: ReturnType | undefined - try { - const value = await Promise.race([ - work, - new Promise((_, reject) => { - timer = setTimeout(() => reject(new OauthCallbackStepTimeout(step, ms)), ms) - timer.unref?.() - }), - ]) - logger.info(`OAuth callback step done: ${step} (${Date.now() - start}ms)`) - return value - } catch (error) { - logger.error(`OAuth callback step failed: ${step} (${Date.now() - start}ms)`, { - error: toError(error).message, - }) - throw error - } finally { - clearTimeout(timer) - } -} - function escapeHtml(value: string): string { return value .replace(/&/g, '&') diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index 6b0df17c66e..6806f1ffe91 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -9,6 +9,7 @@ import { McpOauthRedirectRequiredMock, mcpOauthMock, mcpOauthMockFns, + OauthStepTimeoutErrorMock, permissionsMock, permissionsMockFns, resetDbChainMock, @@ -86,6 +87,54 @@ describe('MCP OAuth start route', () => { ) }) + it('returns 504 (not a retry) when the auth step times out', async () => { + // The stall is intentionally NOT auto-retried — a lingering attempt shares the OAuth row and + // could corrupt the retry's PKCE/state. The bounded step fails fast; the user re-clicks. + mcpOauthMockFns.mockMcpAuthGuarded.mockImplementationOnce(() => { + throw new OauthStepTimeoutErrorMock('mcpAuthGuarded', 12_000) + }) + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' + ) + + const response = await GET(request) + + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1) + expect(response.status).toBe(504) + }) + + it('returns 504 (not a generic 500) when a DB step times out', async () => { + // DB-step timeouts are bounded too; their OauthStepTimeoutError must reach the same + // 504 handler, not fall through to the generic 500. + mcpOauthMockFns.mockGetOrCreateOauthRow.mockImplementationOnce(() => { + throw new OauthStepTimeoutErrorMock('getOrCreateOauthRow', 5_000) + }) + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' + ) + + const response = await GET(request) + + expect(response.status).toBe(504) + expect(mcpOauthMockFns.mockMcpAuthGuarded).not.toHaveBeenCalled() + }) + + it('returns the authorize URL without error-logging the success redirect throw', async () => { + mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce( + new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize') + ) + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' + ) + + const response = await GET(request) + const body = await response.json() + + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1) + expect(response.status).toBe(200) + expect(body).toEqual({ status: 'redirect', authorizationUrl: 'https://mcp.exa.ai/authorize' }) + }) + it('requires workspace write permission via MCP auth middleware', async () => { const request = new NextRequest( 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 5bc08af493f..3a936167650 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -16,14 +16,31 @@ import { loadPreregisteredClient, McpOauthInsecureUrlError, McpOauthRedirectRequired, + makeTimedStep, mcpAuthGuarded, + OauthStepTimeoutError, SimMcpOauthProvider, setOauthRowUser, } from '@/lib/mcp/oauth' import { createMcpErrorResponse } from '@/lib/mcp/utils' const logger = createLogger('McpOauthStartAPI') +const timedStep = makeTimedStep(logger) const OAUTH_START_TTL_MS = 10 * 60 * 1000 +/** + * Per-step budgets, kept small so the whole request stays under the client's 30s `/oauth/start` + * deadline even in the worst case: up to four bounded DB steps (loadServer, getOrCreateOauthRow, + * setOauthRowUser, loadPreregisteredClient) + the auth step = 4×4 + 10 = 26s, leaving margin for + * middleware and network. OAuth discovery + DCR occasionally hits the transient + * headers-then-stalled-body class documented for CDN-fronted MCP hosts; the bound turns that into + * a fast, labeled failure so the popup closes with a clear error and the user can retry (a fresh + * click = a fresh connection that dodges the per-connection stall) rather than the popup hanging + * blank. We deliberately do NOT auto-retry here: `timedStep` can't cancel a wedged attempt, and a + * lingering first attempt sharing this server's OAuth row could overwrite the retry's PKCE + * verifier / state and break the callback. + */ +const DB_STEP_MS = 4_000 +const MCP_AUTH_STEP_MS = 10_000 const MAX_SURFACED_ERROR_LENGTH = 250 const DCR_UNSUPPORTED_MESSAGE = "This server doesn't support automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or configure a token instead." @@ -81,18 +98,21 @@ export const GET = withRouteHandler( const parsed = await parseRequest(startMcpOauthContract, request, {}) if (!parsed.success) return parsed.response const { serverId } = parsed.data.query + logger.info(`Starting MCP OAuth flow for server ${serverId}`) - const [server] = await db - .select() - .from(mcpServers) - .where( - and( - eq(mcpServers.id, serverId), - eq(mcpServers.workspaceId, workspaceId), - isNull(mcpServers.deletedAt) + const [server] = await timedStep('loadServer', DB_STEP_MS, () => + db + .select() + .from(mcpServers) + .where( + and( + eq(mcpServers.id, serverId), + eq(mcpServers.workspaceId, workspaceId), + isNull(mcpServers.deletedAt) + ) ) - ) - .limit(1) + .limit(1) + ) if (!server) { return createMcpErrorResponse(new Error('Server not found'), 'Server not found', 404) @@ -107,8 +127,9 @@ export const GET = withRouteHandler( if (!server.url) { return createMcpErrorResponse(new Error('Server has no URL'), 'Missing server URL', 400) } + const serverUrl = server.url try { - assertSafeOauthServerUrl(server.url) + assertSafeOauthServerUrl(serverUrl) } catch (e) { if (e instanceof McpOauthInsecureUrlError) { return createMcpErrorResponse( @@ -120,11 +141,13 @@ export const GET = withRouteHandler( throw e } - const row = await getOrCreateOauthRow({ - mcpServerId: server.id, - userId, - workspaceId, - }) + const row = await timedStep('getOrCreateOauthRow', DB_STEP_MS, () => + getOrCreateOauthRow({ + mcpServerId: server.id, + userId, + workspaceId, + }) + ) const hasActiveFlow = !!row.state && !!row.stateCreatedAt && @@ -137,17 +160,38 @@ export const GET = withRouteHandler( ) } if (row.userId !== userId) { - await setOauthRowUser(row.id, userId) + await timedStep('setOauthRowUser', DB_STEP_MS, () => setOauthRowUser(row.id, userId)) row.userId = userId } - const preregistered = await loadPreregisteredClient(server.id) + const preregistered = await timedStep('loadPreregisteredClient', DB_STEP_MS, () => + loadPreregisteredClient(server.id) + ) const provider = new SimMcpOauthProvider({ row, preregistered }) try { - const result = await mcpAuthGuarded(provider, { - serverUrl: server.url, + // OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails + // fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is + // the SUCCESS signal (a throw carrying the authorize URL), so we catch it INSIDE the + // bounded step and return it as a normal value — otherwise timedStep would error-log + // every successful authorize. Only a real error or a timeout escapes as a throw. + const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => { + try { + return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) } + } catch (e) { + if (e instanceof McpOauthRedirectRequired) { + return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl } + } + throw e + } }) - if (result === 'AUTHORIZED') { + if (authOutcome.kind === 'redirect') { + logger.info(`OAuth redirect for server ${serverId}`) + return NextResponse.json({ + status: 'redirect', + authorizationUrl: authOutcome.authorizationUrl, + }) + } + if (authOutcome.value === 'AUTHORIZED') { return NextResponse.json({ status: 'already_authorized' }) } return createMcpErrorResponse( @@ -156,19 +200,26 @@ export const GET = withRouteHandler( 500 ) } catch (e) { - if (e instanceof McpOauthRedirectRequired) { - logger.info(`OAuth redirect for server ${serverId}`) - return NextResponse.json({ - status: 'redirect', - authorizationUrl: e.authorizationUrl, - }) - } if (isDynamicClientRegistrationUnsupported(e)) { return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422) } throw e } } catch (error) { + // Any bounded step timing out (DB reads or the auth step) is a stall, not a bug — + // surface it as a fast 504 so the popup closes with a clear "try again" rather than a + // generic 500. A fresh retry is a clean flow: the callback correlates on the `state` + // nonce, so even if a lingering timed-out attempt later overwrites the row's state, the + // user's authorize URL (carrying the fresh nonce) simply fails `invalid_state` — a clean + // retry, never silent corruption. + if (error instanceof OauthStepTimeoutError) { + logger.warn('MCP OAuth start stalled') + return createMcpErrorResponse( + error, + 'Authorization is taking too long — please try again.', + 504 + ) + } logger.error('Error starting MCP OAuth flow:', error) // Only surface OAuth-flow errors verbatim; everything else (DB, decryption, // network) gets a generic message to avoid leaking internal details. diff --git a/apps/sim/lib/mcp/oauth/index.ts b/apps/sim/lib/mcp/oauth/index.ts index 5237b22fe16..7e89f72d7c3 100644 --- a/apps/sim/lib/mcp/oauth/index.ts +++ b/apps/sim/lib/mcp/oauth/index.ts @@ -28,4 +28,5 @@ export { setOauthRowUser, withMcpOauthRefreshLock, } from './storage' +export { makeTimedStep, OauthStepTimeoutError } from './timed-step' export { assertSafeOauthServerUrl, McpOauthInsecureUrlError } from './url-validation' diff --git a/apps/sim/lib/mcp/oauth/timed-step.ts b/apps/sim/lib/mcp/oauth/timed-step.ts new file mode 100644 index 00000000000..4429969b798 --- /dev/null +++ b/apps/sim/lib/mcp/oauth/timed-step.ts @@ -0,0 +1,45 @@ +import type { Logger } from '@sim/logger' +import { toError } from '@sim/utils/errors' + +/** Thrown when a `timedStep`-bounded operation doesn't settle within its budget. */ +export class OauthStepTimeoutError extends Error { + constructor(step: string, ms: number) { + super(`MCP OAuth step "${step}" did not settle within ${ms}ms`) + this.name = 'OauthStepTimeoutError' + } +} + +/** + * Times and bounds one awaited step of an OAuth route so a stalled operation surfaces + * as a labeled, logged error instead of hanging the request (and the browser popup + * waiting on it) forever. The losing promise is not cancelled — a wedged DB/socket op + * can't be — so it settles in the background with its rejection swallowed; the point is + * that the request stops waiting on it and the logs name the exact step that stalled. + */ +export function makeTimedStep(logger: Logger) { + return async function timedStep(step: string, ms: number, fn: () => Promise): Promise { + const start = Date.now() + logger.info(`OAuth step start: ${step}`) + const work = Promise.resolve(fn()) + work.catch(() => {}) + let timer: ReturnType | undefined + try { + const value = await Promise.race([ + work, + new Promise((_, reject) => { + timer = setTimeout(() => reject(new OauthStepTimeoutError(step, ms)), ms) + timer.unref?.() + }), + ]) + logger.info(`OAuth step done: ${step} (${Date.now() - start}ms)`) + return value + } catch (error) { + logger.error(`OAuth step failed: ${step} (${Date.now() - start}ms)`, { + error: toError(error).message, + }) + throw error + } finally { + clearTimeout(timer) + } + } +} diff --git a/packages/testing/src/mocks/index.ts b/packages/testing/src/mocks/index.ts index e3f6c2d6f62..296c9796a5f 100644 --- a/packages/testing/src/mocks/index.ts +++ b/packages/testing/src/mocks/index.ts @@ -96,6 +96,7 @@ export { McpOauthRedirectRequiredMock, mcpOauthMock, mcpOauthMockFns, + OauthStepTimeoutErrorMock, } from './mcp-oauth.mock' // Permission mocks export { permissionsMock, permissionsMockFns } from './permissions.mock' diff --git a/packages/testing/src/mocks/mcp-oauth.mock.ts b/packages/testing/src/mocks/mcp-oauth.mock.ts index 81dee8de9c4..e5318930a60 100644 --- a/packages/testing/src/mocks/mcp-oauth.mock.ts +++ b/packages/testing/src/mocks/mcp-oauth.mock.ts @@ -44,6 +44,13 @@ export class McpOauthInsecureUrlErrorMock extends Error { } } +export class OauthStepTimeoutErrorMock extends Error { + constructor(step: string, ms: number) { + super(`MCP OAuth step "${step}" did not settle within ${ms}ms`) + this.name = 'OauthStepTimeoutErrorMock' + } +} + /** * Returns the provider config back as the constructed instance, matching the * original identity passthrough. Declared as a named function (not an arrow) so @@ -82,5 +89,12 @@ export const mcpOauthMock = { withMcpOauthRefreshLock: mcpOauthMockFns.mockWithMcpOauthRefreshLock, McpOauthRedirectRequired: McpOauthRedirectRequiredMock, McpOauthInsecureUrlError: McpOauthInsecureUrlErrorMock, + OauthStepTimeoutError: OauthStepTimeoutErrorMock, SimMcpOauthProvider: vi.fn().mockImplementation(buildSimMcpOauthProvider), + // Pass-through: run the step immediately, no bounding, so route tests exercise real behavior. + // Wrap in Promise.resolve like the real helper so a mock returning a non-promise still chains. + makeTimedStep: + () => + (_step: string, _ms: number, fn: () => Promise): Promise => + Promise.resolve(fn()), }