Skip to content

Commit 0a5e5dd

Browse files
committed
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.
1 parent 8d8cfcb commit 0a5e5dd

3 files changed

Lines changed: 41 additions & 17 deletions

File tree

apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ describe('useMcpOauthPopup', () => {
180180
hook.unmount()
181181
})
182182

183-
it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => {
183+
it('clears the row when a reopen fails (the reopen blanked the prior window)', async () => {
184184
mockStartOauth.mockResolvedValueOnce({
185185
status: 'redirect',
186186
authorizationUrl: 'https://as.example/a?state=st-a',
@@ -195,14 +195,15 @@ describe('useMcpOauthPopup', () => {
195195
await flush()
196196
expect(hook.result().connectingServers.has('s1')).toBe(true)
197197

198-
// Reopen fails (e.g. popup blocked). The still-live prior flow must keep the row connecting
199-
// rather than the failed attempt clearing it (deterministic reference counting).
200-
mockStartOauth.mockRejectedValueOnce(new Error('Popup blocked'))
198+
// Reopen fails after the named-window open already navigated the prior auth window to
199+
// about:blank — the prior flow is moot (windowless), so BOTH it and the failed attempt
200+
// clear, leaving the row idle with 'Connect with OAuth' immediately available.
201+
mockStartOauth.mockRejectedValueOnce(new Error('start failed'))
201202
await act(async () => {
202203
await hook.result().startOauthForServer('s1')
203204
})
204205
await flush()
205-
expect(hook.result().connectingServers.has('s1')).toBe(true)
206+
expect(hook.result().connectingServers.has('s1')).toBe(false)
206207

207208
hook.unmount()
208209
})

apps/sim/hooks/mcp/use-mcp-oauth-popup.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -168,14 +168,14 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
168168
return
169169
}
170170
popup.focus?.()
171+
// Opening the shared named window just navigated ANY prior authorization window to
172+
// about:blank, so prior flows for this server are moot regardless of how this attempt
173+
// ends — retire them now (a failed start must not leave a windowless flow "connecting"
174+
// for the 10-minute safety timeout).
175+
retireFlows(serverId)
171176
incConnecting(serverId) // this attempt begins
172177
try {
173178
const result = await startOauth({ serverId, workspaceId })
174-
// The replacement start succeeded (already-authorized, or a fresh authorization is
175-
// underway), so retire any prior attempt for this server now — its result is moot and
176-
// the server-side `state` it depended on has been overwritten. A *failed* start
177-
// (below) leaves prior flows intact.
178-
retireFlows(serverId)
179179
if (result.status === 'already_authorized') {
180180
try {
181181
popup.close()
@@ -185,11 +185,18 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
185185
return
186186
}
187187
const { authorizationUrl, state } = result
188+
let navigated = true
188189
try {
189190
popup.location.replace(authorizationUrl)
190191
} catch {
191192
// A COOP-severed reused window can refuse scripted navigation; reopen by name.
192-
window.open(authorizationUrl, `mcp-oauth-${serverId}`)
193+
// This runs after the await, so it can itself be popup-blocked — check it.
194+
navigated = window.open(authorizationUrl, `mcp-oauth-${serverId}`) !== null
195+
}
196+
if (!navigated) {
197+
decConnecting(serverId)
198+
toast.error('Popup blocked. Please allow popups for this site and retry.')
199+
return
193200
}
194201
// Track the in-flight flow by its `state` nonce for the BroadcastChannel gate, bounded by
195202
// a safety timeout in case no result ever arrives (popup abandoned, or a callback the

apps/sim/hooks/queries/mcp.ts

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -317,12 +317,28 @@ export function useStartMcpOauth() {
317317
return useMutation<StartMcpOauthMutationResult, Error, { serverId: string; workspaceId: string }>(
318318
{
319319
mutationFn: async ({ serverId, workspaceId }) => {
320-
const result = await requestJson(startMcpOauthContract, {
321-
query: { serverId, workspaceId },
322-
// A stalled /oauth/start must settle so the caller can reset the connecting
323-
// state and close its pre-opened popup instead of appearing bricked.
324-
signal: AbortSignal.timeout(30_000),
325-
})
320+
// A stalled /oauth/start must settle so the caller can reset the connecting
321+
// state and close its pre-opened popup instead of appearing bricked.
322+
// Feature-detect AbortSignal.timeout (Safari <16 lacks it) with a plain
323+
// controller fallback.
324+
let timeoutSignal: AbortSignal | undefined
325+
let timeoutId: ReturnType<typeof setTimeout> | undefined
326+
if (typeof AbortSignal.timeout === 'function') {
327+
timeoutSignal = AbortSignal.timeout(30_000)
328+
} else {
329+
const controller = new AbortController()
330+
timeoutId = setTimeout(() => controller.abort(new Error('Request timed out')), 30_000)
331+
timeoutSignal = controller.signal
332+
}
333+
let result: Awaited<ReturnType<typeof requestJson<typeof startMcpOauthContract>>>
334+
try {
335+
result = await requestJson(startMcpOauthContract, {
336+
query: { serverId, workspaceId },
337+
signal: timeoutSignal,
338+
})
339+
} finally {
340+
if (timeoutId !== undefined) clearTimeout(timeoutId)
341+
}
326342
if (result.status === 'already_authorized') return { status: 'already_authorized' }
327343

328344
const parsedUrl = new URL(result.authorizationUrl)

0 commit comments

Comments
 (0)