fix(mcp): deliver OAuth callback result over BroadcastChannel so COOP can't strand the connect (#5767)

* 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.

Author: waleedlatif1

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')
+  })
 })

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 = `<!doctype html><html><head><meta charset="utf-8"><title>${title}</title></head><body style="font-family: system-ui; padding: 24px"><p>${safeMessage}</p><script>
-    try { window.opener && window.opener.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${jsonLiteral(serverId)}, reason: ${jsonLiteral(reason)} }, window.location.origin) } catch (e) {}
+    try { var ch = new BroadcastChannel('mcp-oauth'); ch.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${jsonLiteral(serverId)}, state: ${jsonLiteral(state)}, reason: ${jsonLiteral(reason)} }); ch.close() } catch (e) {}
     setTimeout(function () { window.close() }, 800)
   </script></body></html>`
   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)
   }
 })

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<Set<string>>(() => new Set())
-  const popupIntervalsRef = useRef<Map<string, number>>(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<Map<string, { serverId: string; timeout: number }>>(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<Map<string, number>>(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<McpOauthCallbackMessage> | 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 }