From 4fa5fde24a4ae4bfe1223298e67b70afb2a710e8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 13:52:15 -0700 Subject: [PATCH 1/5] fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/lib/mcp/pinned-fetch.test.ts | 59 +++++++++++++++++++++++++-- apps/sim/lib/mcp/pinned-fetch.ts | 45 ++++++++++++++++---- 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 64354ee708b..7c6e46bda17 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -32,9 +32,62 @@ 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('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..d9b18f8ba67 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,20 @@ 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 + /** * 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 +57,35 @@ 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. + * + * Note: the deadline 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 by up to the OS DNS timeout. Acceptable here because + * all consumers are best-effort authorization/revocation flows. * + * @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) + const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal + try { + return await pinnedFetch(url, { ...init, signal }) + } catch (error) { + if (timeoutSignal.aborted && !init?.signal?.aborted) { + 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 } From aa42acb9499eaedab51bbb60c456ab761281b079 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 14:02:04 -0700 Subject: [PATCH 2/5] fix(mcp): bound SSRF/DNS validation by the deadline too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/sim/lib/mcp/pinned-fetch.test.ts | 11 ++++++++ apps/sim/lib/mcp/pinned-fetch.ts | 39 ++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 7c6e46bda17..2991b66f80d 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -68,6 +68,17 @@ describe('createSsrfGuardedMcpFetch', () => { ) }) + 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('propagates a caller-initiated abort unchanged (composed with the deadline)', async () => { mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10') sentinelFetch.mockImplementation( diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index d9b18f8ba67..5f8ef7a8919 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -46,6 +46,30 @@ export function createPinnedMcpFetch(resolvedIP: string): PinnedMcpFetch { */ 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 can't accept a signal) by the + * request deadline. Removes the abort listener once `promise` settles so a later + * timeout can't surface as an unhandled rejection. + */ +function withDeadline(promise: Promise, signal: AbortSignal): Promise { + if (signal.aborted) 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 @@ -62,10 +86,11 @@ const OAUTH_FETCH_TIMEOUT_MS = 30_000 * own deadline is relabeled to an `McpError`; a caller abort or any other failure * propagates unchanged. * - * Note: the deadline 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 by up to the OS DNS timeout. Acceptable here because - * all consumers are best-effort authorization/revocation flows. + * The deadline covers the whole guarded call — both SSRF validation (whose + * `dns.lookup` can't take a signal, so it's raced against the deadline) and the + * HTTP request — so a caller awaiting this never waits longer than `timeoutMs`. + * 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 @@ -74,11 +99,11 @@ const OAUTH_FETCH_TIMEOUT_MS = 30_000 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 const timeoutSignal = AbortSignal.timeout(timeoutMs) - const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal try { + const resolvedIP = await withDeadline(validateMcpServerSsrf(target), timeoutSignal) + const pinnedFetch: FetchLike = resolvedIP ? createPinnedFetch(resolvedIP) : globalThis.fetch + const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal return await pinnedFetch(url, { ...init, signal }) } catch (error) { if (timeoutSignal.aborted && !init?.signal?.aborted) { From ea33016280ce41efe781ae69bc6ec334674d6b35 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 14:11:51 -0700 Subject: [PATCH 3/5] 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. --- apps/sim/lib/mcp/pinned-fetch.test.ts | 12 ++++++++++++ apps/sim/lib/mcp/pinned-fetch.ts | 28 ++++++++++++++++----------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 2991b66f80d..ea90724b94c 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -79,6 +79,18 @@ describe('createSsrfGuardedMcpFetch', () => { expect(sentinelFetch).not.toHaveBeenCalled() }) + 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( diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index 5f8ef7a8919..ec93c35c7d2 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -48,11 +48,11 @@ 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 can't accept a signal) by the - * request deadline. Removes the abort listener once `promise` settles so a later - * timeout can't surface as an unhandled rejection. + * 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 withDeadline(promise: Promise, signal: AbortSignal): Promise { +function raceWithSignal(promise: Promise, signal: AbortSignal): Promise { if (signal.aborted) return Promise.reject(signal.reason) return new Promise((resolve, reject) => { const onAbort = () => reject(signal.reason) @@ -86,10 +86,11 @@ function withDeadline(promise: Promise, signal: AbortSignal): Promise { * own deadline is relabeled to an `McpError`; a caller abort or any other failure * propagates unchanged. * - * The deadline covers the whole guarded call — both SSRF validation (whose - * `dns.lookup` can't take a signal, so it's raced against the deadline) and the - * HTTP request — so a caller awaiting this never waits longer than `timeoutMs`. - * A stalled DNS resolution still runs to completion in the background, but its + * 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). @@ -100,13 +101,18 @@ export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOU return (async (url, init) => { const target = typeof url === 'string' ? url : url.href 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 withDeadline(validateMcpServerSsrf(target), timeoutSignal) + const resolvedIP = await raceWithSignal(validateMcpServerSsrf(target), signal) const pinnedFetch: FetchLike = resolvedIP ? createPinnedFetch(resolvedIP) : globalThis.fetch - const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal return await pinnedFetch(url, { ...init, signal }) } catch (error) { - if (timeoutSignal.aborted && !init?.signal?.aborted) { + // 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`) } From 071d0cc7dad3dbd1bf601bcd09fe09f83757c01a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 14:19:24 -0700 Subject: [PATCH 4/5] 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. --- apps/sim/lib/mcp/pinned-fetch.test.ts | 16 ++++++++++++++++ apps/sim/lib/mcp/pinned-fetch.ts | 7 ++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index ea90724b94c..8079cd2474b 100644 --- a/apps/sim/lib/mcp/pinned-fetch.test.ts +++ b/apps/sim/lib/mcp/pinned-fetch.test.ts @@ -79,6 +79,22 @@ describe('createSsrfGuardedMcpFetch', () => { 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 new Promise((resolve) => setTimeout(resolve, 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(() => {})) diff --git a/apps/sim/lib/mcp/pinned-fetch.ts b/apps/sim/lib/mcp/pinned-fetch.ts index ec93c35c7d2..0d12fcf0192 100644 --- a/apps/sim/lib/mcp/pinned-fetch.ts +++ b/apps/sim/lib/mcp/pinned-fetch.ts @@ -53,7 +53,12 @@ const OAUTH_FETCH_TIMEOUT_MS = 30_000 * settles so a late abort can't surface as an unhandled rejection. */ function raceWithSignal(promise: Promise, signal: AbortSignal): Promise { - if (signal.aborted) return Promise.reject(signal.reason) + 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 }) From 1a8be6f57fada2448258970c5901e98ce7f06808 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 20 Jul 2026 14:26:49 -0700 Subject: [PATCH 5/5] 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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/mcp/pinned-fetch.test.ts b/apps/sim/lib/mcp/pinned-fetch.test.ts index 8079cd2474b..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(() => ({ @@ -92,7 +93,7 @@ describe('createSsrfGuardedMcpFetch', () => { ).rejects.toThrow('pre-aborted') expect(mockCreatePinnedFetch).not.toHaveBeenCalled() // Let the swallowed validation rejection settle so a leak would surface here. - await new Promise((resolve) => setTimeout(resolve, 0)) + await sleep(0) }) it('cancels a stalled validation when the caller aborts (not just the deadline)', async () => {