diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 64354ee708b..f4fd72e6741 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 f8300149529..0d12fcf0192 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 }