fix(mcp): correlate the OAuth result on the state nonce, not serverId

waleedlatif1 · waleedlatif1 · commit 3553676a7b44 · 2026-07-19T10:24:02.000-07:00
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.
diff --git 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
@@ -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 = `<!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 { var ch = new BroadcastChannel('mcp-oauth'); ch.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${jsonLiteral(serverId)}, reason: ${jsonLiteral(reason)} }); ch.close() } 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, {
@@ -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
@@ -49,11 +49,12 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
   const { mutateAsync: startOauth } = useStartMcpOauth()
 
   const [connectingServers, setConnectingServers] = useState<Set<string>>(() => 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<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())
@@ -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<McpOauthCallbackMessage> | 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
@@ -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
@@ -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
 }
