diff --git a/apps/sim/lib/mcp/client.test.ts b/apps/sim/lib/mcp/client.test.ts index 1d5ff39964d..bb7784bdbed 100644 --- a/apps/sim/lib/mcp/client.test.ts +++ b/apps/sim/lib/mcp/client.test.ts @@ -22,6 +22,7 @@ vi.mock('@sim/logger', () => ({ vi.mock('@/lib/mcp/pinned-fetch', () => ({ createGuardedMcpFetch: vi.fn(() => ({ fetch: vi.fn(), close: mockPinnedClose })), + createPinnedPrivateMcpFetch: vi.fn(() => ({ fetch: vi.fn(), close: mockPinnedClose })), })) /** @@ -72,6 +73,7 @@ vi.mock('@/lib/core/execution-limits', () => ({ import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' import { McpClient } from '@/lib/mcp/client' +import { createGuardedMcpFetch, createPinnedPrivateMcpFetch } from '@/lib/mcp/pinned-fetch' import { type McpClientOptions, McpOauthAuthorizationRequiredError, @@ -372,6 +374,38 @@ describe('McpClient notification handler', () => { expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain(secret) }) + it('keeps the transport on the SSRF guard when no validated address is supplied', () => { + new McpClient({ + config: createConfig(), + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + }) + + const guarded = vi.mocked(createGuardedMcpFetch).mock.results.at(-1)?.value + expect(createGuardedMcpFetch).toHaveBeenCalledWith('https://test.example.com/mcp') + expect(createPinnedPrivateMcpFetch).not.toHaveBeenCalled() + expect(vi.mocked(StreamableHTTPClientTransport).mock.calls.at(-1)?.[1]?.fetch).toBe( + guarded.fetch + ) + }) + + it('pins the transport to a validated private address', () => { + new McpClient({ + config: createConfig(), + securityPolicy: { requireConsent: false, auditLevel: 'basic' }, + resolvedIP: '10.0.0.5', + }) + + const pinned = vi.mocked(createPinnedPrivateMcpFetch).mock.results.at(-1)?.value + expect(createPinnedPrivateMcpFetch).toHaveBeenCalledWith( + '10.0.0.5', + 'https://test.example.com/mcp' + ) + expect(createGuardedMcpFetch).not.toHaveBeenCalled() + expect(vi.mocked(StreamableHTTPClientTransport).mock.calls.at(-1)?.[1]?.fetch).toBe( + pinned.fetch + ) + }) + it('closes the pinned transport Agent when connect fails', async () => { mockSdkConnect.mockRejectedValueOnce(new Error('connect boom')) const client = new McpClient({ diff --git a/apps/sim/lib/mcp/client.ts b/apps/sim/lib/mcp/client.ts index 296478e16cb..bcfe4a5072d 100644 --- a/apps/sim/lib/mcp/client.ts +++ b/apps/sim/lib/mcp/client.ts @@ -119,19 +119,18 @@ export class McpClient { throw new McpError('OAuth MCP server must use one authentication strategy') } const useOauth = this.config.authType === 'oauth' - // `resolvedIP` is null only when the hostname still carries an unresolved env-var - // reference, which is checked again once it resolves. Otherwise the guard validates - // addresses per-connect. A private/loopback resolvedIP only reaches here on a - // self-hosted deployment whose policy permits it, and that case pins to the address - // that was validated rather than to whatever the name resolves to next. - const guarded = resolvedIP - ? isPrivateIp(resolvedIP) + // The transport never runs on the global fetch: the guard validates addresses + // per-connect and redirects per-hop whether or not a caller validated the URL + // first. A private/loopback resolvedIP only reaches here on a self-hosted + // deployment whose policy permits it, and that case pins to the address that + // was validated rather than to whatever the name resolves to next. + const guarded = + resolvedIP && isPrivateIp(resolvedIP) ? createPinnedPrivateMcpFetch(resolvedIP, this.config.url) : createGuardedMcpFetch(this.config.url) - : undefined - this.closeGuardedTransport = guarded?.close + this.closeGuardedTransport = guarded.close const oauthFetch = useOauth - ? createMcpEndpointFetch(guarded?.fetch ?? fetch, { + ? createMcpEndpointFetch(guarded.fetch, { serverUrl: this.config.url, headers: this.config.headers, }) @@ -142,11 +141,11 @@ export class McpClient { serverUrl: this.config.url, fetch: oauthFetch, }) - : (oauthFetch ?? guarded?.fetch) + : (oauthFetch ?? guarded.fetch) this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), { authProvider: useOauth ? this.authProvider : undefined, ...(useOauth ? {} : { requestInit: { headers: this.config.headers } }), - ...(transportFetch ? { fetch: transportFetch } : {}), + fetch: transportFetch, }) this.client = new Client( diff --git a/apps/sim/lib/mcp/connection-manager.test.ts b/apps/sim/lib/mcp/connection-manager.test.ts index 6d66ca07340..37eea450396 100644 --- a/apps/sim/lib/mcp/connection-manager.test.ts +++ b/apps/sim/lib/mcp/connection-manager.test.ts @@ -34,8 +34,12 @@ const { mockOnToolsChanged, mockPublishToolsChanged, mockGetOrCreateOauthRow, + mockValidateMcpDomain, + mockValidateMcpServerSsrf, } = vi.hoisted(() => ({ MockMcpClientConstructor: vi.fn(), + mockValidateMcpDomain: vi.fn(), + mockValidateMcpServerSsrf: vi.fn(), mockOnToolsChanged: vi.fn(() => vi.fn()), mockPublishToolsChanged: vi.fn(), mockGetOrCreateOauthRow: vi.fn(), @@ -50,6 +54,10 @@ vi.mock('@/lib/mcp/pubsub', () => ({ vi.mock('@/lib/mcp/client', () => ({ McpClient: MockMcpClientConstructor, })) +vi.mock('@/lib/mcp/domain-check', () => ({ + validateMcpDomain: mockValidateMcpDomain, + validateMcpServerSsrf: mockValidateMcpServerSsrf, +})) vi.mock('@/lib/mcp/oauth', () => ({ getOrCreateOauthRow: mockGetOrCreateOauthRow, loadPreregisteredClient: vi.fn(), @@ -76,6 +84,7 @@ describe('McpConnectionManager', () => { beforeEach(() => { vi.clearAllMocks() + mockValidateMcpServerSsrf.mockResolvedValue('93.184.216.34') mockGetOrCreateOauthRow.mockResolvedValue({ id: 'oauth-row-1', mcpServerId: 'server-oauth', @@ -277,6 +286,82 @@ describe('McpConnectionManager', () => { }) }) + describe('destination validation', () => { + function mockClients(closeHandlers: Array<() => void> = []): MockMcpClient[] { + const instances: MockMcpClient[] = [] + MockMcpClientConstructor.mockImplementation( + class { + constructor() { + const instance: MockMcpClient = { + connect: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + hasListChangedCapability: vi.fn().mockReturnValue(true), + onClose: vi.fn().mockImplementation((handler: () => void) => { + closeHandlers.push(handler) + }), + } + instances.push(instance) + Object.assign(this, instance) + } + } + ) + return instances + } + + it('validates the destination and hands the resolved address to the client', async () => { + mockClients() + const mgr = createFreshManager() + const config = serverConfig('server-validate') + + await mgr.connect(config, 'user-1', 'ws-1') + + expect(mockValidateMcpDomain).toHaveBeenCalledWith(config.url) + expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(config.url) + const options: McpClientOptions = MockMcpClientConstructor.mock.calls[0][0] + expect(options.resolvedIP).toBe('93.184.216.34') + }) + + it('never constructs a client for a refused destination and releases the connecting slot', async () => { + const instances = mockClients() + mockValidateMcpServerSsrf.mockRejectedValueOnce(new Error('refused by egress policy')) + const mgr = createFreshManager() + const config = serverConfig('server-refused') + + await expect(mgr.connect(config, 'user-1', 'ws-1')).rejects.toThrow( + 'refused by egress policy' + ) + expect(instances).toHaveLength(0) + + const retry = await mgr.connect(config, 'user-1', 'ws-1') + expect(retry.supportsListChanged).toBe(true) + expect(instances).toHaveLength(1) + }) + + it('re-validates the destination before every reconnect', async () => { + vi.useFakeTimers() + const closeHandlers: Array<() => void> = [] + const instances = mockClients(closeHandlers) + const mgr = createFreshManager() + const config = serverConfig('server-reconnect') + + await mgr.connect(config, 'user-1', 'ws-1') + mockValidateMcpServerSsrf.mockRejectedValueOnce(new Error('refused by egress policy')) + mockValidateMcpServerSsrf.mockResolvedValueOnce('93.184.216.35') + + closeHandlers[0]() + await vi.advanceTimersByTimeAsync(2_000) + expect(mockValidateMcpServerSsrf).toHaveBeenCalledTimes(2) + expect(instances).toHaveLength(1) + + await vi.advanceTimersByTimeAsync(5_000) + expect(mockValidateMcpServerSsrf).toHaveBeenCalledTimes(3) + expect(instances).toHaveLength(2) + const options: McpClientOptions = MockMcpClientConstructor.mock.calls[1][0] + expect(options.resolvedIP).toBe('93.184.216.35') + expect(mgr.hasConnection('server-reconnect')).toBe(true) + }) + }) + describe('dispose', () => { it('rejects new connections after dispose', async () => { MockMcpClientConstructor.mockImplementation( diff --git a/apps/sim/lib/mcp/connection-manager.ts b/apps/sim/lib/mcp/connection-manager.ts index a0e6d448bd2..ebaa67f96d0 100644 --- a/apps/sim/lib/mcp/connection-manager.ts +++ b/apps/sim/lib/mcp/connection-manager.ts @@ -14,6 +14,7 @@ import { createLogger } from '@sim/logger' import { backoffWithJitter } from '@sim/utils/retry' import { isTest } from '@/lib/core/config/env-flags' import { McpClient } from '@/lib/mcp/client' +import { validateMcpDomain, validateMcpServerSsrf } from '@/lib/mcp/domain-check' import { getOrCreateOauthRow, loadPreregisteredClient, SimMcpOauthProvider } from '@/lib/mcp/oauth' import { mcpPubSub } from '@/lib/mcp/pubsub' import { @@ -106,12 +107,19 @@ export class McpConnectionManager { * * If the server does NOT support `listChanged`, the client is disconnected * immediately — there's nothing to listen for. + * + * `config` must already have its env-var references resolved. The destination + * is validated here on every dial, including reconnects, so the address is + * never carried over from an earlier resolution. + * + * @throws McpDomainNotAllowedError when the domain is not allowlisted + * @throws McpSsrfError when the egress policy refuses the destination + * @throws McpDnsResolutionError when the hostname cannot be resolved */ async connect( config: McpServerConfig, userId: string, - workspaceId: string, - resolvedIP?: string | null + workspaceId: string ): Promise<{ supportsListChanged: boolean }> { if (this.disposed) { logger.warn('Connection manager is disposed, ignoring connect request') @@ -172,6 +180,9 @@ export class McpConnectionManager { } } + validateMcpDomain(config.url) + const resolvedIP = await validateMcpServerSsrf(config.url) + const client = new McpClient({ config, securityPolicy: { diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 74ce71f6d66..7d93b58112b 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -1095,12 +1095,12 @@ class McpService { // survives a transport loss and would block that fresh reconnect. void (async () => { try { - const { config: resolvedConfig, resolvedIP } = await this.resolveConfigEnvVars( + const { config: resolvedConfig } = await resolveMcpConfigEnvVars( config, userId, workspaceId ) - await manager.connect(resolvedConfig, userId, workspaceId, resolvedIP) + await manager.connect(resolvedConfig, userId, workspaceId) } catch (err) { logger.warn(`[${requestId}] Persistent connection failed for ${config.name}:`, err) } diff --git a/apps/sim/lib/mcp/types.ts b/apps/sim/lib/mcp/types.ts index 91dbfadde3c..a4007638a03 100644 --- a/apps/sim/lib/mcp/types.ts +++ b/apps/sim/lib/mcp/types.ts @@ -197,11 +197,11 @@ export interface McpClientOptions { securityPolicy?: McpSecurityPolicy onToolsChanged?: McpToolsChangedCallback /** - * Pre-resolved IP address to pin all transport HTTP connections to. When - * set, the SDK transport uses a custom fetch backed by an undici Agent with - * a fixed DNS lookup, preventing DNS-rebinding (TOCTOU) attacks between - * URL validation and connection. Should be supplied by callers that have - * just validated the URL via `validateMcpServerSsrf`. + * Address returned by `validateMcpServerSsrf` for this URL. A private/loopback + * address (only permitted on a self-hosted deployment whose policy allows it) + * pins every transport connection to it, so the name cannot rebind elsewhere + * after validation. A public address or none leaves the transport on the SSRF + * guard, which validates every connect and redirect hop itself. */ resolvedIP?: string /**