From 16852a8471aaf099b024d9accc694231b0bc6521 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 09:37:12 -0700 Subject: [PATCH 1/5] fix(mcp): deliver OAuth callback result over BroadcastChannel so COOP can't strand the connect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../app/api/mcp/oauth/callback/route.test.ts | 27 +++++++++++++++++++ apps/sim/app/api/mcp/oauth/callback/route.ts | 23 +++++++++++++--- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 14 +++++++--- apps/sim/hooks/queries/mcp.ts | 2 +- apps/sim/lib/mcp/oauth/callback-reasons.ts | 11 +++++--- 5 files changed, 66 insertions(+), 11 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 1a1f5bcb8e8..61b2ea3a983 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,31 @@ describe('MCP OAuth callback route', () => { }) ) }) + + it('signals success over a same-origin BroadcastChannel scoped to the server and workspace', 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. + expect(body).toContain("new BroadcastChannel('mcp-oauth')") + expect(body).toContain('ok: true') + expect(body).toContain('"server-1"') + expect(body).toContain('"workspace-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() + }) }) diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 5c75416a3bc..63474bb1f17 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -43,12 +43,23 @@ function htmlClose( message: string, ok: boolean, reason: McpOauthCallbackReason, - serverId?: string + serverId?: string, + workspaceId?: 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. const body = `${title}

${safeMessage}

` return new NextResponse(body, { @@ -173,7 +184,13 @@ 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 htmlClose( + 'Connected. You can close this window.', + true, + 'authorized', + server.id, + server.workspaceId + ) } catch (error) { logger.error('MCP OAuth callback failed', error) return htmlClose('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..196982a0333 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -53,10 +53,17 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { }, []) 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 + // Ignore results for a different workspace open in another tab. Early failures + // carry no workspaceId and clear this tab's in-flight popups regardless. + if (data.workspaceId && data.workspaceId !== workspaceId) return if (data.serverId) { const serverId = data.serverId const interval = popupIntervalsRef.current.get(serverId) @@ -95,8 +102,7 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { toast.error(reasonToMessage(data.reason)) } } - window.addEventListener('message', onMessage) - return () => window.removeEventListener('message', onMessage) + return () => channel.close() }, [queryClient, workspaceId]) const startOauthForServer = useCallback( diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 21ce5202682..570c1ccf6f1 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -304,7 +304,7 @@ export function useCreateMcpServer() { }) } -/** On `redirect`, the caller must wait for `popup.closed` or the `mcp-oauth` postMessage. */ +/** On `redirect`, the caller must wait for `popup.closed` or the `mcp-oauth` BroadcastChannel signal. */ export type StartMcpOauthMutationResult = | { status: 'redirect'; popup: Window } | { status: 'already_authorized' } diff --git a/apps/sim/lib/mcp/oauth/callback-reasons.ts b/apps/sim/lib/mcp/oauth/callback-reasons.ts index e0f348adc94..60e2eb4c136 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,7 @@ export interface McpOauthCallbackMessage { type: 'mcp-oauth' ok: boolean serverId?: string + /** Scopes the broadcast so other open workspaces ignore it. Absent on early failures. */ + workspaceId?: string reason?: McpOauthCallbackReason } From 4c57c51058e9dfab74b005e6ec4672da21b8010c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 09:50:43 -0700 Subject: [PATCH 2/5] fix(mcp): react to the OAuth broadcast only in the tab that opened the popup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/app/api/mcp/oauth/callback/route.test.ts | 10 ++++------ apps/sim/app/api/mcp/oauth/callback/route.ts | 15 ++++----------- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 11 ++++++++--- apps/sim/lib/mcp/oauth/callback-reasons.ts | 2 -- 4 files changed, 16 insertions(+), 22 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 61b2ea3a983..d8a0c53e8fb 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.test.ts @@ -73,7 +73,7 @@ describe('MCP OAuth callback route', () => { ) }) - it('signals success over a same-origin BroadcastChannel scoped to the server and workspace', async () => { + it('signals success over a same-origin BroadcastChannel carrying the server id', async () => { const request = new NextRequest( 'http://localhost:3000/api/mcp/oauth/callback?state=state-1&code=auth-code-1' ) @@ -81,18 +81,16 @@ describe('MCP OAuth callback route', () => { 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. + // so a COOP `same-origin` provider that severs the opener can't strand the parent. The + // server id lets the hook react only in the tab that opened this flow's popup. expect(body).toContain("new BroadcastChannel('mcp-oauth')") expect(body).toContain('ok: true') expect(body).toContain('"server-1"') - expect(body).toContain('"workspace-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 request = new NextRequest('http://localhost:3000/api/mcp/oauth/callback?state=state-1') const body = await (await GET(request)).text() diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 63474bb1f17..4ef8f07b336 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -43,8 +43,7 @@ function htmlClose( message: string, ok: boolean, reason: McpOauthCallbackReason, - serverId?: string, - workspaceId?: string + serverId?: string ): NextResponse { if (!ok) { logger.warn( @@ -57,9 +56,9 @@ function htmlClose( // `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. + // unaffected by opener severance; the hook ignores results for popups it did not open. const body = `${title}

${safeMessage}

` return new NextResponse(body, { @@ -184,13 +183,7 @@ 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, - server.workspaceId - ) + return htmlClose('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) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 196982a0333..96f5a3c4cfe 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -61,9 +61,14 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { channel.onmessage = (event) => { const data = event.data as Partial | null if (data?.type !== 'mcp-oauth') return - // Ignore results for a different workspace open in another tab. Early failures - // carry no workspaceId and clear this tab's in-flight popups regardless. - if (data.workspaceId && data.workspaceId !== workspaceId) return + // A BroadcastChannel reaches every same-origin tab, so only the tab that actually + // opened this flow's popup should react — otherwise an unrelated tab (any workspace) + // would clear state, refetch, and show a spurious toast. `popupIntervalsRef` holds + // this tab's in-flight popups; a result for a server it never opened is not ours. + const initiatedHere = data.serverId + ? popupIntervalsRef.current.has(data.serverId) + : popupIntervalsRef.current.size > 0 + if (!initiatedHere) return if (data.serverId) { const serverId = data.serverId const interval = popupIntervalsRef.current.get(serverId) diff --git a/apps/sim/lib/mcp/oauth/callback-reasons.ts b/apps/sim/lib/mcp/oauth/callback-reasons.ts index 60e2eb4c136..235546600a2 100644 --- a/apps/sim/lib/mcp/oauth/callback-reasons.ts +++ b/apps/sim/lib/mcp/oauth/callback-reasons.ts @@ -22,7 +22,5 @@ export interface McpOauthCallbackMessage { type: 'mcp-oauth' ok: boolean serverId?: string - /** Scopes the broadcast so other open workspaces ignore it. Absent on early failures. */ - workspaceId?: string reason?: McpOauthCallbackReason } From 1d84f3c687e5641c8123cd4319f183f650046904 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 10:05:40 -0700 Subject: [PATCH 3/5] fix(mcp): decouple OAuth result correlation from popup.closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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…". --- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 148 ++++++++++++---------- 1 file changed, 84 insertions(+), 64 deletions(-) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 96f5a3c4cfe..32510a41d6f 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -37,18 +37,64 @@ 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()) + // serverId -> safety timeout. This is the correlation source of truth for the + // BroadcastChannel gate: it is cleared only when the flow completes or times out, never by + // popup.closed polling — COOP can make popup.closed misreport, and clearing it early would + // drop the genuine completion this fix exists to deliver. + 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: stop the spinner, its safety timeout, and its popup poll. */ + const settleFlow = useCallback( + (serverId: string) => { + const timeout = pendingFlowsRef.current.get(serverId) + if (timeout !== undefined) window.clearTimeout(timeout) + pendingFlowsRef.current.delete(serverId) + stopPopupPoll(serverId) + stopConnecting(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 t of pending.values()) window.clearTimeout(t) + for (const p of polls.values()) window.clearInterval(p) + pending.clear() + polls.clear() } }, []) @@ -61,46 +107,16 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { channel.onmessage = (event) => { const data = event.data as Partial | null if (data?.type !== 'mcp-oauth') return - // A BroadcastChannel reaches every same-origin tab, so only the tab that actually - // opened this flow's popup should react — otherwise an unrelated tab (any workspace) - // would clear state, refetch, and show a spurious toast. `popupIntervalsRef` holds - // this tab's in-flight popups; a result for a server it never opened is not ours. - const initiatedHere = data.serverId - ? popupIntervalsRef.current.has(data.serverId) - : popupIntervalsRef.current.size > 0 - if (!initiatedHere) 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. A result without a serverId can't be correlated to a flow, so + // it's ignored here — the initiating tab's safety timeout clears its own "Connecting…". + if (!data.serverId || !pendingFlowsRef.current.has(data.serverId)) return + settleFlow(data.serverId) 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, data.serverId), + }) queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) }) toast.success('Server authorized') } else { @@ -108,43 +124,47 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { } } return () => channel.close() - }, [queryClient, workspaceId]) + }, [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() + settleFlow(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) + // Track this in-flight flow for the BroadcastChannel gate, bounded by a safety + // timeout in case no result ever arrives (popup abandoned, or a callback failure + // that couldn't carry a serverId). + const existingTimeout = pendingFlowsRef.current.get(serverId) + if (existingTimeout !== undefined) window.clearTimeout(existingTimeout) + pendingFlowsRef.current.set( + serverId, + window.setTimeout(() => settleFlow(serverId), 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() + settleFlow(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 } From 3553676a7b444ccb80e2206acf9b1a6579b98f9e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 10:24:02 -0700 Subject: [PATCH 4/5] fix(mcp): correlate the OAuth result on the state nonce, not serverId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../app/api/mcp/oauth/callback/route.test.ts | 20 +++++- apps/sim/app/api/mcp/oauth/callback/route.ts | 47 +++++++------ apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 66 ++++++++++--------- apps/sim/hooks/queries/mcp.ts | 14 +++- apps/sim/lib/mcp/oauth/callback-reasons.ts | 7 ++ 5 files changed, 99 insertions(+), 55 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 d8a0c53e8fb..96a7de727fe 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.test.ts @@ -73,7 +73,7 @@ describe('MCP OAuth callback route', () => { ) }) - it('signals success over a same-origin BroadcastChannel carrying the server id', async () => { + 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' ) @@ -82,10 +82,11 @@ describe('MCP OAuth callback route', () => { // 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 - // server id lets the hook react only in the tab that opened this flow's popup. + // `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 () => { @@ -97,4 +98,19 @@ describe('MCP OAuth callback route', () => { 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 4ef8f07b336..f0b99eb522b 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -43,7 +43,8 @@ function htmlClose( message: string, ok: boolean, reason: McpOauthCallbackReason, - serverId?: string + serverId?: string, + state?: string ): NextResponse { if (!ok) { logger.warn( @@ -56,9 +57,10 @@ function htmlClose( // `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 ignores results for popups it did not open. + // unaffected by opener severance; the hook correlates on `state` and ignores flows it + // did not start. const body = `${title}

${safeMessage}

` return new NextResponse(body, { @@ -73,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', @@ -99,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', @@ -109,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', @@ -128,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', @@ -141,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', @@ -162,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', @@ -173,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 { @@ -183,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 32510a41d6f..fbcf141568e 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -49,11 +49,12 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { const { mutateAsync: startOauth } = useStartMcpOauth() const [connectingServers, setConnectingServers] = useState>(() => new Set()) - // serverId -> safety timeout. This is the correlation source of truth for the - // BroadcastChannel gate: it is cleared only when the flow completes or times out, never by - // popup.closed polling — COOP can make popup.closed misreport, and clearing it early would - // drop the genuine completion this fix exists to deliver. - const pendingFlowsRef = 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()) @@ -75,14 +76,15 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { } }, []) - /** End a flow entirely: stop the spinner, its safety timeout, and its popup poll. */ + /** End a flow entirely (by its state nonce): stop the spinner, safety timeout, and popup poll. */ const settleFlow = useCallback( - (serverId: string) => { - const timeout = pendingFlowsRef.current.get(serverId) - if (timeout !== undefined) window.clearTimeout(timeout) - pendingFlowsRef.current.delete(serverId) - stopPopupPoll(serverId) - stopConnecting(serverId) + (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] ) @@ -91,7 +93,7 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { const pending = pendingFlowsRef.current const polls = popupPollsRef.current return () => { - for (const t of pending.values()) window.clearTimeout(t) + for (const { timeout } of pending.values()) window.clearTimeout(timeout) for (const p of polls.values()) window.clearInterval(p) pending.clear() polls.clear() @@ -107,15 +109,18 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { channel.onmessage = (event) => { const data = event.data as Partial | null if (data?.type !== 'mcp-oauth') return - // A BroadcastChannel reaches every same-origin tab, so react only to a result for a - // flow THIS tab started. A result without a serverId can't be correlated to a flow, so - // it's ignored here — the initiating tab's safety timeout clears its own "Connecting…". - if (!data.serverId || !pendingFlowsRef.current.has(data.serverId)) return - settleFlow(data.serverId) + // 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) }) queryClient.invalidateQueries({ - queryKey: mcpKeys.serverToolsList(workspaceId, data.serverId), + queryKey: mcpKeys.serverToolsList(workspaceId, serverId), }) queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) }) toast.success('Server authorized') @@ -132,19 +137,19 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { try { const result = await startOauth({ serverId, workspaceId }) if (result.status === 'already_authorized') { - settleFlow(serverId) + stopConnecting(serverId) return } - const { popup } = result - // Track this in-flight flow for the BroadcastChannel gate, bounded by a safety - // timeout in case no result ever arrives (popup abandoned, or a callback failure - // that couldn't carry a serverId). - const existingTimeout = pendingFlowsRef.current.get(serverId) - if (existingTimeout !== undefined) window.clearTimeout(existingTimeout) - pendingFlowsRef.current.set( + const { popup, state } = result + // 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). + const existing = pendingFlowsRef.current.get(state) + if (existing !== undefined) window.clearTimeout(existing.timeout) + pendingFlowsRef.current.set(state, { serverId, - window.setTimeout(() => settleFlow(serverId), OAUTH_FLOW_TIMEOUT_MS) - ) + 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. @@ -159,7 +164,8 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { }, 500) ) } catch (e) { - settleFlow(serverId) + stopPopupPoll(serverId) + stopConnecting(serverId) logger.error('Failed to start MCP OAuth', e) toast.error(toError(e).message || 'Failed to start authorization') } diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index 570c1ccf6f1..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` BroadcastChannel signal. */ +/** + * 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 235546600a2..b214c255d9d 100644 --- a/apps/sim/lib/mcp/oauth/callback-reasons.ts +++ b/apps/sim/lib/mcp/oauth/callback-reasons.ts @@ -22,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 } From eb69205a273a767ee3375141929076c9a6e5aff3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 19 Jul 2026 10:59:12 -0700 Subject: [PATCH 5/5] 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. --- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index fbcf141568e..a7588ba7bac 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -141,11 +141,17 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { return } 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). - const existing = pendingFlowsRef.current.get(state) - if (existing !== undefined) window.clearTimeout(existing.timeout) pendingFlowsRef.current.set(state, { serverId, timeout: window.setTimeout(() => settleFlow(state), OAUTH_FLOW_TIMEOUT_MS),