From fb439c99ca0a7d129e87f8e6cc7811f31413e4af Mon Sep 17 00:00:00 2001 From: Waleed Date: Sun, 19 Jul 2026 11:05:16 -0700 Subject: [PATCH] fix(mcp): deliver OAuth callback result over BroadcastChannel so COOP can't strand the connect (#5767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): deliver OAuth callback result over BroadcastChannel so COOP can't strand the connect An MCP provider whose authorization page sets `Cross-Origin-Opener-Policy: same-origin` (e.g. Gauge) severs `window.opener` when the popup navigates through it, so the callback's `window.opener.postMessage` was silently lost and the row hung on "Connecting…" forever — even when the callback succeeded. - Signal completion over a same-origin `BroadcastChannel` instead, which is origin-scoped and immune to opener severance (the MDN/Chrome-recommended COOP workaround). Scope the message by workspaceId so other open workspaces ignore it. - Log every OAuth callback failure with its reason + serverId. The early-return gates previously returned a silent `ok:false` popup close, so a failed authorization was undiagnosable from the server logs. * fix(mcp): react to the OAuth broadcast only in the tab that opened the popup A BroadcastChannel reaches every same-origin tab, so the previous workspace-id scoping (which the success path carried but failure paths omitted) still let unrelated tabs clear state, refetch, and show spurious toasts. Gate every reaction on whether this tab actually has an in-flight popup for the result's server (`popupIntervalsRef`) — strictly more precise than workspace scoping and correct for both cross-workspace and same-workspace-second-tab cases. Removes the now-redundant workspaceId from the callback message. * fix(mcp): decouple OAuth result correlation from popup.closed Cursor flagged two defects in the previous gate, both rooted in keying the BroadcastChannel filter on `popupIntervalsRef` (the popup.closed poll map): - A genuine completion was dropped whenever the poll had already removed the server's entry — and COOP can make `popup.closed` misreport, which is exactly the case this PR targets, so the fix could silently fail to apply. - A result without a serverId fell back to "any in-flight popup", waking unrelated same-origin tabs with spurious toasts/refetches. Introduce `pendingFlowsRef` (serverId -> safety timeout) as the correlation source of truth: cleared only on completion or a 10-min timeout (matching the server OAuth start TTL), never by popup.closed. The popup.closed poll now only clears the spinner (best-effort abandon UX) and never touches correlation, so a real completion is always processed. Results without a serverId are ignored; the initiating tab's safety timeout clears its own "Connecting…". * fix(mcp): correlate the OAuth result on the state nonce, not serverId Cursor flagged that a failure which can't resolve a serverId (notably invalid_state) broadcasts ok:false with no serverId, so the serverId-keyed gate ignored it and the initiating tab sat on "Connecting…" until the safety timeout with no error feedback. Correlate on the OAuth `state` instead — the per-flow nonce the callback echoes on every result, success or failure. The client parses it from the authorization URL and keys the in-flight map by it; the callback includes it on every response via a `respond` helper. This is the canonical popup-OAuth correlation: it reaches the initiating tab even when no serverId exists, and — because each flow has a unique state — it also fixes the same-user-same-server-in-two-tabs edge a serverId key left open. Results with no parseable state (a malformed callback) are still ignored; that flow clears via its timeout. * fix(mcp): drop a server's prior in-flight OAuth flow when it is retried Each start mints a new `state`, so an abandoned attempt's safety timeout was keyed under a different state than its retry and never cleared. In the contrived case where both stayed pending ~10 min, the stale timer would clear the newer flow's spinner. Sweep any prior in-flight flow for the same serverId on a new start (replacing the can't-happen same-state check). Result delivery was already correct; this makes the state machine airtight. --- .../app/api/mcp/oauth/callback/route.test.ts | 41 +++++ apps/sim/app/api/mcp/oauth/callback/route.ts | 55 ++++-- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 165 +++++++++++------- apps/sim/hooks/queries/mcp.ts | 14 +- apps/sim/lib/mcp/oauth/callback-reasons.ts | 16 +- 5 files changed, 205 insertions(+), 86 deletions(-) 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 1a1f5bcb8e..96a7de727f 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 5c75416a3b..f0b99eb522 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 b3fd9a6c58..a7588ba7ba 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 21ce520268..36e47c2d38 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 e0f348adc9..b214c255d9 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 }