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
 }