diff --git a/apps/sim/app/api/mcp/oauth/callback/route.test.ts b/apps/sim/app/api/mcp/oauth/callback/route.test.ts index 1a1f5bcb8e8..96a7de727fe 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.test.ts @@ -72,4 +72,45 @@ describe('MCP OAuth callback route', () => { }) ) }) + + it('signals success over a same-origin BroadcastChannel carrying the state nonce', async () => { + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/callback?state=state-1&code=auth-code-1' + ) + + const body = await (await GET(request)).text() + + // The completion is delivered over a BroadcastChannel (not window.opener.postMessage) + // so a COOP `same-origin` provider that severs the opener can't strand the parent. The + // `state` nonce lets the hook react only in the tab that started this exact flow. + expect(body).toContain("new BroadcastChannel('mcp-oauth')") + expect(body).toContain('ok: true') + expect(body).toContain('"server-1"') + expect(body).toContain('"state-1"') + }) + + it('reports an early failure over the channel without attempting token exchange', async () => { + // Missing `code` fails at the param gate, before any network work. + const request = new NextRequest('http://localhost:3000/api/mcp/oauth/callback?state=state-1') + + const body = await (await GET(request)).text() + + expect(body).toContain('ok: false') + expect(mcpOauthMockFns.mockMcpAuthGuarded).not.toHaveBeenCalled() + }) + + it('echoes the state on a serverless invalid_state failure so the initiating tab can react', async () => { + // No row loads for the state -> failure with no serverId. The state must still be echoed, + // or the initiating tab would sit on "Connecting…" until its safety timeout. + mcpOauthMockFns.mockLoadOauthRowByState.mockResolvedValueOnce(null) + const request = new NextRequest( + 'http://localhost:3000/api/mcp/oauth/callback?state=state-1&code=auth-code-1' + ) + + const body = await (await GET(request)).text() + + expect(body).toContain('ok: false') + expect(body).toContain('"state-1"') + expect(body).toContain('serverId: undefined') + }) }) diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 5c75416a3bc..f0b99eb522b 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -43,12 +43,24 @@ function htmlClose( message: string, ok: boolean, reason: McpOauthCallbackReason, - serverId?: string + serverId?: string, + state?: string ): NextResponse { + if (!ok) { + logger.warn( + `MCP OAuth callback did not complete: ${reason}${serverId ? ` (server ${serverId})` : ''}` + ) + } const safeMessage = escapeHtml(message) const title = ok ? 'Connected' : 'Connection failed' + // Signal the opener over a same-origin BroadcastChannel rather than + // `window.opener.postMessage`: a provider whose authorize page sets COOP + // `same-origin` severs `window.opener`, which would silently drop the result and + // leave the parent stuck on "Connecting…". A BroadcastChannel is origin-scoped and + // unaffected by opener severance; the hook correlates on `state` and ignores flows it + // did not start. const body = `${title}

${safeMessage}

` return new NextResponse(body, { @@ -63,21 +75,26 @@ export const GET = withRouteHandler(async (request: NextRequest) => { } const { state, code, error: errorParam } = parsed.data.query + // Echo the flow's `state` on every result so the opener can correlate a broadcast back to + // the exact flow it started — including failures (e.g. `invalid_state`) that never resolve + // a serverId. Without it those results would strand the initiating tab on "Connecting…". + const respond = ( + message: string, + ok: boolean, + reason: McpOauthCallbackReason, + serverId?: string + ) => htmlClose(message, ok, reason, serverId, state) + const initialRow = state ? await loadOauthRowByState(state).catch(() => null) : null const stateRowServerId = initialRow?.mcpServerId if (errorParam) { logger.warn(`MCP OAuth callback received error: ${errorParam}`) if (initialRow) await clearState(initialRow.id).catch(() => {}) - return htmlClose( - `Authorization failed: ${errorParam}`, - false, - 'provider_error', - stateRowServerId - ) + return respond(`Authorization failed: ${errorParam}`, false, 'provider_error', stateRowServerId) } if (!state || !code) { - return htmlClose( + return respond( 'Missing state or code in callback URL.', false, 'missing_params', @@ -89,7 +106,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { try { const session = await getSession() if (!session?.user?.id) { - return htmlClose( + return respond( 'You must be signed in to complete authorization.', false, 'unauthenticated', @@ -99,12 +116,12 @@ export const GET = withRouteHandler(async (request: NextRequest) => { const row = initialRow if (!row) { - return htmlClose('Invalid or expired authorization state.', false, 'invalid_state') + return respond('Invalid or expired authorization state.', false, 'invalid_state') } serverId = row.mcpServerId if (session.user.id !== row.userId) { - return htmlClose( + return respond( 'You must be signed in as the same user that initiated the flow.', false, 'user_mismatch', @@ -118,10 +135,10 @@ export const GET = withRouteHandler(async (request: NextRequest) => { .where(and(eq(mcpServers.id, row.mcpServerId), isNull(mcpServers.deletedAt))) .limit(1) if (!server || !server.url) { - return htmlClose('Server no longer exists.', false, 'server_gone', serverId) + return respond('Server no longer exists.', false, 'server_gone', serverId) } if (server.workspaceId !== row.workspaceId) { - return htmlClose( + return respond( 'Workspace mismatch on authorization callback.', false, 'invalid_state', @@ -131,7 +148,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { try { assertSafeOauthServerUrl(server.url) } catch { - return htmlClose( + return respond( 'MCP OAuth requires https (or http://localhost for development).', false, 'insecure_url', @@ -152,7 +169,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { }) } catch (e) { logger.error('Token exchange failed during MCP OAuth callback', e) - return htmlClose( + return respond( 'Token exchange failed. Please try again.', false, 'token_exchange_failed', @@ -163,7 +180,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => { } if (result !== 'AUTHORIZED') { - return htmlClose('Authorization did not complete.', false, 'token_exchange_failed', server.id) + return respond('Authorization did not complete.', false, 'token_exchange_failed', server.id) } try { @@ -173,9 +190,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => { logger.warn('Post-auth tools refresh failed', toError(e).message) } - return htmlClose('Connected. You can close this window.', true, 'authorized', server.id) + return respond('Connected. You can close this window.', true, 'authorized', server.id) } catch (error) { logger.error('MCP OAuth callback failed', error) - return htmlClose('Authorization failed. Please try again.', false, 'unknown', serverId) + return respond('Authorization failed. Please try again.', false, 'unknown', serverId) } }) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index b3fd9a6c58a..a7588ba7bac 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -37,103 +37,146 @@ interface UseMcpOauthPopupProps { workspaceId: string } +/** + * Bounds how long a row shows "Connecting…" without a result. Matches the server-side OAuth + * start TTL: once it lapses the authorization state has expired and the flow can no longer + * complete, so a still-pending flow is safe to drop. + */ +const OAUTH_FLOW_TIMEOUT_MS = 10 * 60 * 1000 + export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { const queryClient = useQueryClient() const { mutateAsync: startOauth } = useStartMcpOauth() const [connectingServers, setConnectingServers] = useState>(() => new Set()) - const popupIntervalsRef = useRef>(new Map()) + // OAuth `state` nonce -> { serverId, safety timeout }. The state keys the BroadcastChannel + // correlation: the callback echoes it on every result (even failures that can't resolve a + // serverId), so the tab that started this exact flow matches it while other same-origin tabs + // ignore it. Cleared only when the flow completes or times out, never by popup.closed polling + // — COOP can make popup.closed misreport, and clearing early would drop a genuine completion. + const pendingFlowsRef = useRef>(new Map()) + // serverId -> popup.closed poll. Best-effort fast "Connecting…" clear when the user + // abandons the popup; never used to correlate a result. + const popupPollsRef = useRef>(new Map()) + + const stopConnecting = useCallback((serverId: string) => { + setConnectingServers((prev) => { + if (!prev.has(serverId)) return prev + const next = new Set(prev) + next.delete(serverId) + return next + }) + }, []) + + const stopPopupPoll = useCallback((serverId: string) => { + const poll = popupPollsRef.current.get(serverId) + if (poll !== undefined) { + window.clearInterval(poll) + popupPollsRef.current.delete(serverId) + } + }, []) + + /** End a flow entirely (by its state nonce): stop the spinner, safety timeout, and popup poll. */ + const settleFlow = useCallback( + (state: string) => { + const flow = pendingFlowsRef.current.get(state) + if (!flow) return + window.clearTimeout(flow.timeout) + pendingFlowsRef.current.delete(state) + stopPopupPoll(flow.serverId) + stopConnecting(flow.serverId) + }, + [stopConnecting, stopPopupPoll] + ) useEffect(() => { - const intervals = popupIntervalsRef.current + const pending = pendingFlowsRef.current + const polls = popupPollsRef.current return () => { - for (const id of intervals.values()) window.clearInterval(id) - intervals.clear() + for (const { timeout } of pending.values()) window.clearTimeout(timeout) + for (const p of polls.values()) window.clearInterval(p) + pending.clear() + polls.clear() } }, []) useEffect(() => { - function onMessage(event: MessageEvent) { - if (event.origin !== window.location.origin) return + // The callback signals over a same-origin BroadcastChannel (see the OAuth callback + // route): a provider whose authorize page sets COOP `same-origin` severs + // `window.opener`, so a popup `postMessage` can be lost and leave the row stuck on + // "Connecting…". A BroadcastChannel is origin-scoped, so it needs no origin check. + const channel = new BroadcastChannel('mcp-oauth') + channel.onmessage = (event) => { const data = event.data as Partial | null if (data?.type !== 'mcp-oauth') return - if (data.serverId) { - const serverId = data.serverId - const interval = popupIntervalsRef.current.get(serverId) - if (interval !== undefined) { - window.clearInterval(interval) - popupIntervalsRef.current.delete(serverId) - } - setConnectingServers((prev) => { - if (!prev.has(serverId)) return prev - const next = new Set(prev) - next.delete(serverId) - return next - }) - } else if (!data.ok) { - // Early callback failures (missing params, invalid state) post back - // without a serverId, so we can't target a specific row — clear all - // in-flight popups instead of leaving the UI stuck on "Connecting…". - for (const id of popupIntervalsRef.current.values()) window.clearInterval(id) - popupIntervalsRef.current.clear() - setConnectingServers((prev) => (prev.size === 0 ? prev : new Set())) - } + // A BroadcastChannel reaches every same-origin tab, so react only to a result for a flow + // THIS tab started, matched on the OAuth `state` nonce. Every result (success or failure) + // carries it, so unrelated tabs — and unrelated flows in this tab — ignore the broadcast. + if (!data.state) return + const flow = pendingFlowsRef.current.get(data.state) + if (!flow) return + const { serverId } = flow + settleFlow(data.state) if (data.ok) { queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) }) - if (data.serverId) { - queryClient.invalidateQueries({ - queryKey: mcpKeys.serverToolsList(workspaceId, data.serverId), - }) - } else { - queryClient.invalidateQueries({ - queryKey: mcpKeys.serverToolsWorkspace(workspaceId), - }) - } + queryClient.invalidateQueries({ + queryKey: mcpKeys.serverToolsList(workspaceId, serverId), + }) queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) }) toast.success('Server authorized') } else { toast.error(reasonToMessage(data.reason)) } } - window.addEventListener('message', onMessage) - return () => window.removeEventListener('message', onMessage) - }, [queryClient, workspaceId]) + return () => channel.close() + }, [queryClient, workspaceId, settleFlow]) const startOauthForServer = useCallback( async (serverId: string) => { setConnectingServers((prev) => new Set(prev).add(serverId)) - const clear = () => { - const existing = popupIntervalsRef.current.get(serverId) - if (existing !== undefined) { - window.clearInterval(existing) - popupIntervalsRef.current.delete(serverId) - } - setConnectingServers((prev) => { - const next = new Set(prev) - next.delete(serverId) - return next - }) - } try { const result = await startOauth({ serverId, workspaceId }) if (result.status === 'already_authorized') { - clear() + stopConnecting(serverId) return } - const { popup } = result - const existing = popupIntervalsRef.current.get(serverId) - if (existing !== undefined) window.clearInterval(existing) - const interval = window.setInterval(() => { - if (popup.closed) clear() - }, 500) - popupIntervalsRef.current.set(serverId, interval) + const { popup, state } = result + // Drop any prior in-flight flow for this server (e.g. an abandoned attempt now being + // retried) so its stale safety timeout can't later clear this new flow's state. + for (const [prevState, flow] of pendingFlowsRef.current) { + if (flow.serverId === serverId) { + window.clearTimeout(flow.timeout) + pendingFlowsRef.current.delete(prevState) + } + } + // Track this in-flight flow keyed by its `state` nonce for the BroadcastChannel gate, + // bounded by a safety timeout in case no result ever arrives (popup abandoned, or a + // callback failure the client can't otherwise clear). + pendingFlowsRef.current.set(state, { + serverId, + timeout: window.setTimeout(() => settleFlow(state), OAUTH_FLOW_TIMEOUT_MS), + }) + // Best-effort: clear "Connecting…" quickly when the user closes the popup without + // finishing. popup.closed can misreport under COOP, so this only stops the spinner — + // it never touches `pendingFlowsRef`, so it can't drop a real result. + stopPopupPoll(serverId) + popupPollsRef.current.set( + serverId, + window.setInterval(() => { + if (popup.closed) { + stopPopupPoll(serverId) + stopConnecting(serverId) + } + }, 500) + ) } catch (e) { - clear() + stopPopupPoll(serverId) + stopConnecting(serverId) logger.error('Failed to start MCP OAuth', e) toast.error(toError(e).message || 'Failed to start authorization') } }, - [startOauth, workspaceId] + [startOauth, workspaceId, settleFlow, stopConnecting, stopPopupPoll] ) return { connectingServers, startOauthForServer } diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 21ce5202682..36e47c2d382 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -304,9 +304,13 @@ export function useCreateMcpServer() { }) } -/** On `redirect`, the caller must wait for `popup.closed` or the `mcp-oauth` postMessage. */ +/** + * On `redirect`, the caller waits for the `mcp-oauth` BroadcastChannel signal (matched on + * `state`) or `popup.closed`. `state` is the per-flow OAuth nonce the callback echoes, used to + * correlate the eventual result back to this exact flow. + */ export type StartMcpOauthMutationResult = - | { status: 'redirect'; popup: Window } + | { status: 'redirect'; popup: Window; state: string } | { status: 'already_authorized' } export function useStartMcpOauth() { @@ -324,6 +328,10 @@ export function useStartMcpOauth() { if (parsedUrl.protocol !== 'https:' && !isLoopbackHttp) { throw new Error('Authorization URL must use HTTPS') } + const state = parsedUrl.searchParams.get('state') + if (!state) { + throw new Error('Authorization URL is missing the OAuth state parameter') + } const popup = window.open( result.authorizationUrl, `mcp-oauth-${serverId}`, @@ -332,7 +340,7 @@ export function useStartMcpOauth() { if (!popup) { throw new Error('Popup blocked. Please allow popups for this site and retry.') } - return { status: 'redirect', popup } + return { status: 'redirect', popup, state } }, } ) diff --git a/apps/sim/lib/mcp/oauth/callback-reasons.ts b/apps/sim/lib/mcp/oauth/callback-reasons.ts index e0f348adc94..b214c255d9d 100644 --- a/apps/sim/lib/mcp/oauth/callback-reasons.ts +++ b/apps/sim/lib/mcp/oauth/callback-reasons.ts @@ -1,7 +1,10 @@ /** - * Reasons surfaced from the OAuth callback popup back to the parent window via - * `window.opener.postMessage`. Consumed by the popup hook to render user-facing - * status messages and by the callback route to discriminate failure modes. + * Reasons surfaced from the OAuth callback popup back to the parent window over a + * same-origin `BroadcastChannel`. A provider whose authorization page sets + * `Cross-Origin-Opener-Policy: same-origin` severs `window.opener`, so a targeted + * `postMessage` from the popup can be lost; a BroadcastChannel is origin-scoped and + * unaffected. Consumed by the popup hook to render user-facing status messages and + * by the callback route to discriminate failure modes. */ export type McpOauthCallbackReason = | 'authorized' @@ -19,5 +22,12 @@ export interface McpOauthCallbackMessage { type: 'mcp-oauth' ok: boolean serverId?: string + /** + * The OAuth `state` nonce, echoed on every result — including failures that can't resolve + * a serverId. The opener correlates a broadcast to the exact flow it started by matching + * this, so other same-origin tabs ignore it. Absent only on a malformed callback with no + * parseable state. + */ + state?: string reason?: McpOauthCallbackReason }