From 8d8cfcb872897bf83bda3a46c0aeef24cac32080 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 15:59:09 -0700 Subject: [PATCH 1/4] fix(mcp): open the OAuth popup synchronously and bound the start request Two bugs behind 'pressed Connect/Reopen and nothing happened': - window.open ran AFTER awaiting /oauth/start, outside the browser's user activation, so the popup was silently blocked (worst on the add-server flow). Popup-first now: open about:blank synchronously in the click (named window, so a re-click focuses/reuses an existing authorization window), navigate it once the start returns; close it on failure/already_authorized; clear toast when genuinely blocked (and skip the request entirely). - /oauth/start had no client timeout, so a stalled start held the re-entrancy guard and the connecting label indefinitely. Bounded at 30s; on timeout the popup closes, the label resets, and retry is immediately available. --- .../hooks/mcp/use-mcp-oauth-popup.test.tsx | 60 ++++++++++++++++++- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 36 +++++++++-- apps/sim/hooks/queries/mcp.ts | 18 +++--- 3 files changed, 97 insertions(+), 17 deletions(-) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx index 234a71b057d..276eb3a4c6d 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx @@ -79,6 +79,16 @@ async function flush() { describe('useMcpOauthPopup', () => { beforeEach(() => { vi.clearAllMocks() + // Popup-first: startOauthForServer opens a blank window synchronously. + window.open = vi.fn( + () => + ({ + close: vi.fn(), + focus: vi.fn(), + location: { replace: vi.fn() }, + closed: false, + }) as unknown as Window + ) // jsdom has no BroadcastChannel; the hook opens one on mount. class FakeBroadcastChannel { onmessage: ((event: MessageEvent) => void) | null = null @@ -115,7 +125,11 @@ describe('useMcpOauthPopup', () => { // Settle the first flow so the guard clears. await act(async () => { - resolveStart({ status: 'redirect', popup: { closed: false }, state: 'state-1' }) + resolveStart({ + status: 'redirect', + authorizationUrl: 'https://as.example/a?state=state-1', + state: 'state-1', + }) }) await flush() @@ -123,7 +137,11 @@ describe('useMcpOauthPopup', () => { }) it('allows a fresh start after the previous one settles (reopen after abandon)', async () => { - mockStartOauth.mockResolvedValue({ status: 'redirect', popup: { closed: false }, state: 'st' }) + mockStartOauth.mockResolvedValue({ + status: 'redirect', + authorizationUrl: 'https://as.example/a?state=st', + state: 'st', + }) const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) await flush() @@ -165,7 +183,7 @@ describe('useMcpOauthPopup', () => { it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => { mockStartOauth.mockResolvedValueOnce({ status: 'redirect', - popup: { closed: false }, + authorizationUrl: 'https://as.example/a?state=st-a', state: 'st-a', }) const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) @@ -188,4 +206,40 @@ describe('useMcpOauthPopup', () => { hook.unmount() }) + + it('does not issue the start request when the popup is blocked', async () => { + ;(window.open as ReturnType).mockReturnValue(null) + const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) + await flush() + + await act(async () => { + await hook.result().startOauthForServer('s1') + }) + await flush() + + expect(mockStartOauth).not.toHaveBeenCalled() + expect(hook.result().connectingServers.has('s1')).toBe(false) + hook.unmount() + }) + + it('opens the popup before the start request resolves (popup-first)', async () => { + const order: string[] = [] + ;(window.open as ReturnType).mockImplementation(() => { + order.push('open') + return { close: vi.fn(), focus: vi.fn(), location: { replace: vi.fn() } } as unknown as Window + }) + mockStartOauth.mockImplementation(async () => { + order.push('start') + return { status: 'redirect', authorizationUrl: 'https://as.example/a?state=st', state: 'st' } + }) + const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) + await flush() + + await act(async () => { + await hook.result().startOauthForServer('s1') + }) + + expect(order).toEqual(['open', 'start']) + hook.unmount() + }) }) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 678d9ae7c1b..596333a76d4 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -153,27 +153,55 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { const starting = (startingRef.current ??= new Set()) if (starting.has(serverId)) return starting.add(serverId) + // Popup-first: open (or, via the shared window name, reuse+focus) the popup + // SYNCHRONOUSLY inside the user's click so the browser's activation gate sees + // it. Opening after the /oauth/start await loses the activation and gets + // silently popup-blocked — the "pressed Connect and nothing happened" bug. + const popup = window.open( + 'about:blank', + `mcp-oauth-${serverId}`, + 'width=560,height=720,resizable=yes,scrollbars=yes' + ) + if (!popup) { + starting.delete(serverId) + toast.error('Popup blocked. Please allow popups for this site and retry.') + return + } + popup.focus?.() incConnecting(serverId) // this attempt begins try { const result = await startOauth({ serverId, workspaceId }) - // The replacement start succeeded (already-authorized, or a fresh popup opened), so retire - // any prior attempt for this server now — its result is moot and the server-side `state` - // it depended on has been overwritten. A *failed* start (below) leaves prior flows intact. + // The replacement start succeeded (already-authorized, or a fresh authorization is + // underway), so retire any prior attempt for this server now — its result is moot and + // the server-side `state` it depended on has been overwritten. A *failed* start + // (below) leaves prior flows intact. retireFlows(serverId) if (result.status === 'already_authorized') { + try { + popup.close() + } catch {} invalidateServer(serverId) decConnecting(serverId) // this attempt ends return } + const { authorizationUrl, state } = result + try { + popup.location.replace(authorizationUrl) + } catch { + // A COOP-severed reused window can refuse scripted navigation; reopen by name. + window.open(authorizationUrl, `mcp-oauth-${serverId}`) + } // Track the in-flight flow by its `state` nonce for the BroadcastChannel gate, bounded by // a safety timeout in case no result ever arrives (popup abandoned, or a callback the // client can't otherwise observe under COOP). - const { state } = result pendingFlowsRef.current.set(state, { serverId, timeout: window.setTimeout(() => settleFlow(state), OAUTH_FLOW_TIMEOUT_MS), }) } catch (e) { + try { + popup.close() + } catch {} decConnecting(serverId) // this attempt ends; any prior flow keeps its own count 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 36e47c2d382..d7f06f578d3 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -310,7 +310,7 @@ export function useCreateMcpServer() { * correlate the eventual result back to this exact flow. */ export type StartMcpOauthMutationResult = - | { status: 'redirect'; popup: Window; state: string } + | { status: 'redirect'; authorizationUrl: string; state: string } | { status: 'already_authorized' } export function useStartMcpOauth() { @@ -319,6 +319,9 @@ export function useStartMcpOauth() { mutationFn: async ({ serverId, workspaceId }) => { const result = await requestJson(startMcpOauthContract, { query: { serverId, workspaceId }, + // A stalled /oauth/start must settle so the caller can reset the connecting + // state and close its pre-opened popup instead of appearing bricked. + signal: AbortSignal.timeout(30_000), }) if (result.status === 'already_authorized') return { status: 'already_authorized' } @@ -332,15 +335,10 @@ export function useStartMcpOauth() { if (!state) { throw new Error('Authorization URL is missing the OAuth state parameter') } - const popup = window.open( - result.authorizationUrl, - `mcp-oauth-${serverId}`, - 'width=560,height=720,resizable=yes,scrollbars=yes' - ) - if (!popup) { - throw new Error('Popup blocked. Please allow popups for this site and retry.') - } - return { status: 'redirect', popup, state } + // The popup itself is opened SYNCHRONOUSLY by the caller inside the user's + // click (popup-first) — opening it here, after the network await, loses the + // user activation and gets silently popup-blocked. + return { status: 'redirect', authorizationUrl: result.authorizationUrl, state } }, } ) From 0a5e5ddb101690a32a4d7b9b4907f32da1e02bba Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:18:50 -0700 Subject: [PATCH 2/4] fix(mcp): harden popup-first reopen edges - Retire prior flows as soon as the named popup opens (the open already blanked any prior auth window; a failed start must not leave a windowless flow 'connecting' for the 10-minute safety timeout). - Check the COOP-fallback window.open result; when blocked, clear the state and toast instead of registering a windowless pending flow. - Feature-detect AbortSignal.timeout (Safari <16) with an AbortController fallback for the bounded /oauth/start. --- .../hooks/mcp/use-mcp-oauth-popup.test.tsx | 11 ++++---- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 19 +++++++++---- apps/sim/hooks/queries/mcp.ts | 28 +++++++++++++++---- 3 files changed, 41 insertions(+), 17 deletions(-) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx index 276eb3a4c6d..4825584d657 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx @@ -180,7 +180,7 @@ describe('useMcpOauthPopup', () => { hook.unmount() }) - it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => { + it('clears the row when a reopen fails (the reopen blanked the prior window)', async () => { mockStartOauth.mockResolvedValueOnce({ status: 'redirect', authorizationUrl: 'https://as.example/a?state=st-a', @@ -195,14 +195,15 @@ describe('useMcpOauthPopup', () => { await flush() expect(hook.result().connectingServers.has('s1')).toBe(true) - // Reopen fails (e.g. popup blocked). The still-live prior flow must keep the row connecting - // rather than the failed attempt clearing it (deterministic reference counting). - mockStartOauth.mockRejectedValueOnce(new Error('Popup blocked')) + // Reopen fails after the named-window open already navigated the prior auth window to + // about:blank — the prior flow is moot (windowless), so BOTH it and the failed attempt + // clear, leaving the row idle with 'Connect with OAuth' immediately available. + mockStartOauth.mockRejectedValueOnce(new Error('start failed')) await act(async () => { await hook.result().startOauthForServer('s1') }) await flush() - expect(hook.result().connectingServers.has('s1')).toBe(true) + expect(hook.result().connectingServers.has('s1')).toBe(false) hook.unmount() }) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 596333a76d4..8e9afe78c84 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -168,14 +168,14 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { return } popup.focus?.() + // Opening the shared named window just navigated ANY prior authorization window to + // about:blank, so prior flows for this server are moot regardless of how this attempt + // ends — retire them now (a failed start must not leave a windowless flow "connecting" + // for the 10-minute safety timeout). + retireFlows(serverId) incConnecting(serverId) // this attempt begins try { const result = await startOauth({ serverId, workspaceId }) - // The replacement start succeeded (already-authorized, or a fresh authorization is - // underway), so retire any prior attempt for this server now — its result is moot and - // the server-side `state` it depended on has been overwritten. A *failed* start - // (below) leaves prior flows intact. - retireFlows(serverId) if (result.status === 'already_authorized') { try { popup.close() @@ -185,11 +185,18 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { return } const { authorizationUrl, state } = result + let navigated = true try { popup.location.replace(authorizationUrl) } catch { // A COOP-severed reused window can refuse scripted navigation; reopen by name. - window.open(authorizationUrl, `mcp-oauth-${serverId}`) + // This runs after the await, so it can itself be popup-blocked — check it. + navigated = window.open(authorizationUrl, `mcp-oauth-${serverId}`) !== null + } + if (!navigated) { + decConnecting(serverId) + toast.error('Popup blocked. Please allow popups for this site and retry.') + return } // Track the in-flight flow by its `state` nonce for the BroadcastChannel gate, bounded by // a safety timeout in case no result ever arrives (popup abandoned, or a callback the diff --git a/apps/sim/hooks/queries/mcp.ts b/apps/sim/hooks/queries/mcp.ts index d7f06f578d3..2f0f2002080 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -317,12 +317,28 @@ export function useStartMcpOauth() { return useMutation( { mutationFn: async ({ serverId, workspaceId }) => { - const result = await requestJson(startMcpOauthContract, { - query: { serverId, workspaceId }, - // A stalled /oauth/start must settle so the caller can reset the connecting - // state and close its pre-opened popup instead of appearing bricked. - signal: AbortSignal.timeout(30_000), - }) + // A stalled /oauth/start must settle so the caller can reset the connecting + // state and close its pre-opened popup instead of appearing bricked. + // Feature-detect AbortSignal.timeout (Safari <16 lacks it) with a plain + // controller fallback. + let timeoutSignal: AbortSignal | undefined + let timeoutId: ReturnType | undefined + if (typeof AbortSignal.timeout === 'function') { + timeoutSignal = AbortSignal.timeout(30_000) + } else { + const controller = new AbortController() + timeoutId = setTimeout(() => controller.abort(new Error('Request timed out')), 30_000) + timeoutSignal = controller.signal + } + let result: Awaited>> + try { + result = await requestJson(startMcpOauthContract, { + query: { serverId, workspaceId }, + signal: timeoutSignal, + }) + } finally { + if (timeoutId !== undefined) clearTimeout(timeoutId) + } if (result.status === 'already_authorized') return { status: 'already_authorized' } const parsedUrl = new URL(result.authorizationUrl) From d3b754f2b64e3da828941b5d2d65392cb9696430 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:27:18 -0700 Subject: [PATCH 3/4] fix(mcp): close the blank popup when post-start navigation fails --- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 8e9afe78c84..002ffd27c6e 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -194,6 +194,9 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { navigated = window.open(authorizationUrl, `mcp-oauth-${serverId}`) !== null } if (!navigated) { + try { + popup.close() + } catch {} decConnecting(serverId) toast.error('Popup blocked. Please allow popups for this site and retry.') return From bc6de15fc9809e5c3e6fb7c0d68d109511488802 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 16:52:18 -0700 Subject: [PATCH 4/4] chore(mcp): correct connecting-count ordering comment --- apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index 002ffd27c6e..f9768a39bd5 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -50,9 +50,9 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { // Per-server count of live authorization attempts; a row shows "Connecting…" / "Reopen // authorization" while its count > 0. Reference counting (not a boolean set) keeps the label - // deterministic across concurrent attempts: a reopen increments before the superseded flow - // decrements, so the count never dips to 0 mid-reopen (no flicker), and every attempt clears - // exactly once (never stuck). + // deterministic across concurrent attempts: a reopen retires the superseded flow and starts + // its own count within one batched update (no flicker), and every attempt clears exactly + // once (never stuck). const [connectingCounts, setConnectingCounts] = useState>(() => new Map()) // OAuth `state` nonce -> { serverId, safety timeout }. `state` keys the BroadcastChannel // correlation: the callback echoes it on every result (even failures that resolve no serverId),