fix(mcp): always guard the MCP transport and validate on every managed connect

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({

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(

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(

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: {

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)
             }

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
   /**