opensource/sim
3
0
Fork 0
mirror of https://github.com/simstudioai/sim.git synced 2026-09-24 15:45:35 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): open the OAuth popup synchronously and bound the start request (#5824)

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

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

* fix(mcp): close the blank popup when post-start navigation fails

* chore(mcp): correct connecting-count ordering comment
This commit is contained in:
Waleed
2026-07-21 17:15:11 -07:00
committed by GitHub
parent 3458220ec9
commit e51a867de3
3 changed files with 136 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx
+63 -8
View File
@@ -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<typeof vi.fn>).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<typeof vi.fn>).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()
})
})
apps/sim/hooks/mcp/use-mcp-oauth-popup.ts
+46 -8
View File
@@ -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<Map<string, number>>(() => 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')
apps/sim/hooks/queries/mcp.ts
+27 -13
View File
@@ -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<StartMcpOauthMutationResult, Error, { serverId: string; workspaceId: string }>(
{
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<typeof setTimeout> | 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<ReturnType<typeof requestJson<typeof startMcpOauthContract>>>
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 }
},
}
)