fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout

apps/sim/lib/mcp/pinned-fetch.test.ts

@@ -1,6 +1,7 @@
 /**
  * @vitest-environment node
  */
+import { sleep } from '@sim/utils/helpers'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const { mockCreatePinnedFetch, mockValidateMcpServerSsrf, sentinelFetch } = vi.hoisted(() => ({
@@ -32,9 +33,101 @@ describe('createSsrfGuardedMcpFetch', () => {
 
     expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/revoke')
     expect(mockCreatePinnedFetch).toHaveBeenCalledWith('203.0.113.10')
-    expect(sentinelFetch).toHaveBeenCalledWith('https://attacker.example/revoke', {
-      method: 'POST',
-    })
+    expect(sentinelFetch).toHaveBeenCalledWith(
+      'https://attacker.example/revoke',
+      expect.objectContaining({ method: 'POST', signal: expect.any(AbortSignal) })
+    )
+  })
+
+  it('attaches an abort signal to every guarded request even without a caller signal', async () => {
+    mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
+    const fetchLike = createSsrfGuardedMcpFetch()
+    await fetchLike('https://attacker.example/discover')
+
+    const [, init] = sentinelFetch.mock.calls[0]
+    expect(init.signal).toBeInstanceOf(AbortSignal)
+  })
+
+  it('surfaces an McpError when a request exceeds the deadline', async () => {
+    mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
+    // Hang until the guard's own deadline aborts the request.
+    sentinelFetch.mockImplementation(
+      (_url: string, init: RequestInit) =>
+        new Promise((_resolve, reject) => {
+          const signal = init.signal
+          if (signal?.aborted) {
+            reject(signal.reason)
+            return
+          }
+          signal?.addEventListener('abort', () => reject(signal.reason), { once: true })
+        })
+    )
+    const fetchLike = createSsrfGuardedMcpFetch(5)
+
+    await expect(fetchLike('https://slow.example/token', { method: 'POST' })).rejects.toThrow(
+      /timed out after 5ms/
+    )
+  })
+
+  it('bounds a stalled SSRF/DNS validation by the deadline', async () => {
+    // Validation never resolves (mimics a hanging dns.lookup, which takes no signal).
+    mockValidateMcpServerSsrf.mockReturnValue(new Promise(() => {}))
+    const fetchLike = createSsrfGuardedMcpFetch(5)
+
+    await expect(fetchLike('https://slow-dns.example/token')).rejects.toThrow(/timed out after 5ms/)
+    // Never got past validation, so no request was issued.
+    expect(mockCreatePinnedFetch).not.toHaveBeenCalled()
+    expect(sentinelFetch).not.toHaveBeenCalled()
+  })
+
+  it('does not orphan the validation promise when the signal is already aborted', async () => {
+    // Caller aborts before the guard runs, then validation rejects. Without adopting
+    // the in-flight validation, its rejection would surface as an unhandled rejection.
+    mockValidateMcpServerSsrf.mockRejectedValue(new Error('blocked late'))
+    const controller = new AbortController()
+    controller.abort(new Error('pre-aborted'))
+    const fetchLike = createSsrfGuardedMcpFetch(60_000)
+
+    await expect(
+      fetchLike('https://slow.example/token', { signal: controller.signal })
+    ).rejects.toThrow('pre-aborted')
+    expect(mockCreatePinnedFetch).not.toHaveBeenCalled()
+    // Let the swallowed validation rejection settle so a leak would surface here.
+    await sleep(0)
+  })
+
+  it('cancels a stalled validation when the caller aborts (not just the deadline)', async () => {
+    // Validation hangs; the caller's abort — well before the 60s deadline — must settle it.
+    mockValidateMcpServerSsrf.mockReturnValue(new Promise(() => {}))
+    const controller = new AbortController()
+    const fetchLike = createSsrfGuardedMcpFetch(60_000)
+    const pending = fetchLike('https://slow-dns.example/token', { signal: controller.signal })
+    controller.abort(new Error('caller cancelled'))
+
+    await expect(pending).rejects.toThrow('caller cancelled')
+    expect(mockCreatePinnedFetch).not.toHaveBeenCalled()
+  })
+
+  it('propagates a caller-initiated abort unchanged (composed with the deadline)', async () => {
+    mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
+    sentinelFetch.mockImplementation(
+      (_url: string, init: RequestInit) =>
+        new Promise((_resolve, reject) => {
+          const signal = init.signal
+          if (signal?.aborted) {
+            reject(signal.reason)
+            return
+          }
+          signal?.addEventListener('abort', () => reject(signal.reason), { once: true })
+        })
+    )
+    const controller = new AbortController()
+    // Long deadline so the caller's abort — not the timeout — is what settles the request.
+    const fetchLike = createSsrfGuardedMcpFetch(60_000)
+    const pending = fetchLike('https://slow.example/token', { signal: controller.signal })
+    controller.abort(new Error('caller cancelled'))
+
+    await expect(pending).rejects.toThrow('caller cancelled')
   })
 
   it('rejects URLs that resolve to blocked IPs without issuing the request', async () => {

apps/sim/lib/mcp/pinned-fetch.ts

@@ -4,6 +4,7 @@ import {
   createPinnedFetchWithDispatcher,
 } from '@/lib/core/security/input-validation.server'
 import { validateMcpServerSsrf } from '@/lib/mcp/domain-check'
+import { McpError } from '@/lib/mcp/types'
 
 /** Pinned fetch for the live MCP transport, plus a handle to release its sockets. */
 export interface PinnedMcpFetch {
@@ -31,6 +32,49 @@ export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch {
   return { fetch: pinnedFetch, close: () => dispatcher.destroy() }
 }
 
+/**
+ * Per-request deadline for guarded MCP OAuth / RFC 7009 revocation HTTP calls.
+ *
+ * The MCP SDK issues OAuth discovery, dynamic client registration, and token
+ * exchange with a bare `fetch` and no `AbortSignal` — only the JSON-RPC message
+ * layer gets the SDK's request timeout. Combined with undici's 5-minute default
+ * headers/body timeouts, a slow or unresponsive authorization server leaves the
+ * request (and the browser the user is waiting on during `/oauth/start`) pending
+ * for minutes. Bounding each leg turns that into a fast, actionable failure. 30s
+ * mirrors `MCP_CLIENT_CONSTANTS.DEFAULT_CONNECTION_TIMEOUT` and leaves wide
+ * headroom over a healthy server, which completes each leg in well under a second.
+ */
+const OAUTH_FETCH_TIMEOUT_MS = 30_000
+
+/**
+ * Awaits `promise` but rejects with the signal's reason if `signal` aborts first.
+ * Bounds `dns.lookup`-based SSRF validation (which accepts no signal) by the
+ * composed deadline + caller signal. Removes the abort listener once `promise`
+ * settles so a late abort can't surface as an unhandled rejection.
+ */
+function raceWithSignal<T>(promise: Promise<T>, signal: AbortSignal): Promise<T> {
+  if (signal.aborted) {
+    // The promise is already in flight; adopt its settlement so a later rejection
+    // (SSRF/DNS failure) can't surface as an unhandled rejection once we've aborted.
+    promise.catch(() => {})
+    return Promise.reject(signal.reason)
+  }
+  return new Promise<T>((resolve, reject) => {
+    const onAbort = () => reject(signal.reason)
+    signal.addEventListener('abort', onAbort, { once: true })
+    promise.then(
+      (value) => {
+        signal.removeEventListener('abort', onAbort)
+        resolve(value)
+      },
+      (error) => {
+        signal.removeEventListener('abort', onAbort)
+        reject(error)
+      }
+    )
+  })
+}
+
 /**
  * Builds a `FetchLike` that validates every outbound request URL against the
  * MCP SSRF policy before issuing it, then pins the connection to the resolved
@@ -42,19 +86,42 @@ export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch {
  * per request and rejects private/reserved/loopback targets (honoring
  * `ALLOWED_MCP_DOMAINS` and self-hosted localhost rules).
  *
- * Note: a caller-provided `AbortSignal` in `init` only bounds the HTTP request,
- * not the validation DNS lookup — Node's `dns.lookup` does not accept a signal,
- * so a hanging resolution can extend the overall call past the caller's timeout
- * by up to the OS DNS timeout. Acceptable here because all consumers are
- * best-effort, non-blocking flows (OAuth discovery and RFC 7009 revocation).
+ * Each request is bounded by a `timeoutMs` deadline via `AbortSignal.timeout`,
+ * composed with any caller-provided signal so cancellation still works. Only our
+ * own deadline is relabeled to an `McpError`; a caller abort or any other failure
+ * propagates unchanged.
+ *
+ * Both the deadline and any caller-provided signal cover the whole guarded call —
+ * SSRF validation (whose `dns.lookup` takes no signal, so it's raced against the
+ * composed signal) and the HTTP request — so a caller awaiting this never waits
+ * past `timeoutMs` and can cancel at any point, including mid-validation. A
+ * stalled DNS resolution still runs to completion in the background, but its
+ * result is discarded.
  *
+ * @param timeoutMs Per-request deadline in ms (defaults to 30s; override for tests).
  * @throws McpSsrfError if a request URL resolves to a blocked IP address
+ * @throws McpError if a request exceeds `timeoutMs`
  */
-export function createSsrfGuardedMcpFetch(): FetchLike {
+export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOUT_MS): FetchLike {
   return (async (url, init) => {
     const target = typeof url === 'string' ? url : url.href
-    const resolvedIP = await validateMcpServerSsrf(target)
-    const pinnedFetch: FetchLike = resolvedIP ? createPinnedFetch(resolvedIP) : globalThis.fetch
-    return pinnedFetch(url, init)
+    const timeoutSignal = AbortSignal.timeout(timeoutMs)
+    // Compose deadline + caller signal up front so both phases — SSRF validation
+    // and the HTTP request — are bounded by the deadline and caller cancellation.
+    const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal
+    try {
+      const resolvedIP = await raceWithSignal(validateMcpServerSsrf(target), signal)
+      const pinnedFetch: FetchLike = resolvedIP ? createPinnedFetch(resolvedIP) : globalThis.fetch
+      return await pinnedFetch(url, { ...init, signal })
+    } catch (error) {
+      // Relabel only when our own deadline is what fired — identified by the
+      // rejection reason's identity, not init.signal's state (which may abort
+      // independently just after the deadline).
+      if (timeoutSignal.aborted && error === timeoutSignal.reason) {
+        const host = URL.canParse(target) ? new URL(target).host : target
+        throw new McpError(`MCP authorization request to ${host} timed out after ${timeoutMs}ms`)
+      }
+      throw error
+    }
   }) satisfies FetchLike
 }