From 922515ffb031e65004520ba42b0180ad62b31b59 Mon Sep 17 00:00:00 2001 From: Waleed Date: Mon, 20 Jul 2026 14:51:42 -0700 Subject: [PATCH] fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout (#5776) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout The MCP SDK issues OAuth discovery, dynamic client registration, and token exchange with a bare fetch and no AbortSignal (only the JSON-RPC layer gets the SDK's request timeout), and undici's default headers/body timeouts are 5 min. A slow or unresponsive authorization server therefore left /oauth/start pending for minutes — the browser stuck on "Connecting…" forever. Bound each guarded OAuth/revocation leg with a 30s AbortSignal.timeout, composed with any caller signal so cancellation still works. Only our own deadline is relabeled to an McpError; caller aborts and other failures propagate unchanged. Scoped to createSsrfGuardedMcpFetch (OAuth/revoke/probe) — the live transport's timeouts are untouched. * fix(mcp): bound SSRF/DNS validation by the deadline too Move the timeout signal ahead of validateMcpServerSsrf and race the validation (whose dns.lookup takes no signal) against it, so the deadline covers the whole guarded call — a stalled DNS resolution now rejects at timeoutMs instead of the OS DNS timeout. Listener is cleaned up on settle so a late timeout can't surface as an unhandled rejection. * fix(mcp): compose caller signal before validation + attribute timeout by reason Compose the caller's AbortSignal with the deadline up front and use the combined signal for both SSRF validation and the HTTP request, so caller cancellation now covers the whole guarded call (previously a caller abort during a stalled DNS validation waited for the full deadline). Attribute the timeout relabel by the rejection reason's identity rather than init.signal's state, so a caller signal that aborts just after the deadline can't misattribute a genuine timeout. * fix(mcp): adopt in-flight validation on early abort to avoid unhandled rejection When raceWithSignal is entered with an already-aborted signal it returned without attaching to the in-flight validateMcpServerSsrf promise, so a later SSRF/DNS rejection could surface as an unhandled rejection. Swallow the orphaned promise's settlement in the early-abort branch. * test(mcp): use sleep() instead of raw setTimeout in pinned-fetch test check:utils bans new Promise(resolve => setTimeout(...)) in favor of sleep() from @sim/utils/helpers. --- apps/sim/lib/mcp/pinned-fetch.test.ts | 99 ++++++++++++++++++++++++++- apps/sim/lib/mcp/pinned-fetch.ts | 85 ++++++++++++++++++++--- 2 files changed, 172 insertions(+), 12 deletions(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 64354ee708..f4fd72e674 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/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 () => { diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index f830014952..0d12fcf019 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/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(promise: Promise, signal: AbortSignal): Promise { + 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((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 }