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..4825584d657 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() @@ -162,10 +180,10 @@ 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', - popup: { closed: false }, + authorizationUrl: 'https://as.example/a?state=st-a', state: 'st-a', }) const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) @@ -177,15 +195,52 @@ 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() + }) + + 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..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), @@ -153,27 +153,65 @@ 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?.() + // 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 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. - retireFlows(serverId) if (result.status === 'already_authorized') { + try { + popup.close() + } catch {} invalidateServer(serverId) decConnecting(serverId) // this attempt ends 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. + // This runs after the await, so it can itself be popup-blocked — check it. + 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 + } // 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..2f0f2002080 100644 --- a/apps/sim/hooks/queries/mcp.ts +++ b/apps/sim/hooks/queries/mcp.ts @@ -310,16 +310,35 @@ 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() { 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. + // 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) @@ -332,15 +351,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 } }, } )