fix(mcp): stabilize authenticated tool discovery

Author: j15z

apps/sim/app/api/mcp/oauth/start/route.test.ts

@@ -162,6 +162,23 @@ describe('MCP OAuth start route', () => {
     expect(body.error).not.toContain('ECONNREFUSED')
     expect(body.error).not.toContain('internal-db-host')
   })
+
+  it('returns an actionable 4xx when the server does not support dynamic client registration', async () => {
+    mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(
+      new Error('Incompatible auth server: does not support dynamic client registration')
+    )
+    const request = new NextRequest(
+      'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
+    )
+
+    const response = await GET(request)
+    const body = await response.json()
+
+    expect(response.status).toBe(422)
+    expect(body.error).toBe(
+      "This server doesn't support OAuth client registration. Configure a token instead."
+    )
+  })
 })
 
 describe('surfaceOauthError', () => {

apps/sim/app/api/mcp/oauth/start/route.ts

@@ -25,6 +25,15 @@ import { createMcpErrorResponse } from '@/lib/mcp/utils'
 const logger = createLogger('McpOauthStartAPI')
 const OAUTH_START_TTL_MS = 10 * 60 * 1000
 const MAX_SURFACED_ERROR_LENGTH = 250
+const DCR_UNSUPPORTED_MESSAGE =
+  "This server doesn't support OAuth client registration. Configure a token instead."
+
+function isDynamicClientRegistrationUnsupported(error: unknown): boolean {
+  return (
+    error instanceof Error &&
+    error.message.toLowerCase().includes('does not support dynamic client registration')
+  )
+}
 
 export function surfaceOauthError(error: unknown): string {
   // Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields.
@@ -148,6 +157,9 @@ export const GET = withRouteHandler(
             authorizationUrl: e.authorizationUrl,
           })
         }
+        if (isDynamicClientRegistrationUnsupported(e)) {
+          return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
+        }
         throw e
       }
     } catch (error) {

apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts

@@ -81,6 +81,8 @@ function selectRows(rows: unknown[]) {
 describe('MCP server refresh route', () => {
   beforeEach(() => {
     vi.clearAllMocks()
+    mockSelect.mockReset()
+    mockSelect.mockReturnValue(selectRows([persistedServer]))
     mockSelect.mockReturnValueOnce(selectRows([initialServer]))
     mockUpdateSet.mockReturnValue({
       where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([persistedServer]) }),
@@ -112,11 +114,7 @@ describe('MCP server refresh route', () => {
     mockDiscoverServerTools.mockRejectedValueOnce(
       new Error(`Upstream reflected ${reflectedSecret}`)
     )
-    mockUpdateSet.mockReturnValueOnce({
-      where: vi.fn().mockReturnValue({
-        returning: vi.fn().mockResolvedValue([initialServer]),
-      }),
-    })
+    mockSelect.mockReturnValueOnce(selectRows([initialServer]))
 
     const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
       method: 'POST',
@@ -128,6 +126,7 @@ describe('MCP server refresh route', () => {
       expect.objectContaining({
         status: 'disconnected',
         error: 'Internal server error',
+        toolCount: 0,
         workflowsUpdated: 0,
       })
     )
@@ -142,11 +141,7 @@ describe('MCP server refresh route', () => {
       lastConnected: new Date(Date.now() + 60_000),
       toolCount: 7,
     }
-    mockUpdateSet.mockReturnValueOnce({
-      where: vi.fn().mockReturnValue({
-        returning: vi.fn().mockResolvedValue([newerSuccessfulServer]),
-      }),
-    })
+    mockSelect.mockReturnValueOnce(selectRows([newerSuccessfulServer]))
 
     const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
       method: 'POST',
@@ -162,7 +157,7 @@ describe('MCP server refresh route', () => {
         workflowsUpdated: 0,
       })
     )
-    expect(mockClearCache).toHaveBeenCalledWith('workspace-1')
+    expect(mockClearCache).not.toHaveBeenCalled()
   })
 
   it('does not 500 when workflow sync fails after a successful discovery', async () => {
@@ -178,9 +173,7 @@ describe('MCP server refresh route', () => {
     // The route's server lookup consumes the first select (beforeEach). The sync's
     // workflow select is left unmocked, so it throws — exercising the guard that
     // keeps a secondary sync failure from turning a successful refresh into a 500.
-    mockUpdateSet.mockReturnValueOnce({
-      where: vi.fn().mockReturnValue({ returning: vi.fn().mockResolvedValue([initialServer]) }),
-    })
+    mockSelect.mockReturnValueOnce(selectRows([initialServer]))
 
     const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
       method: 'POST',

apps/sim/app/api/mcp/servers/[id]/refresh/route.ts

@@ -208,14 +208,36 @@ export const POST = withRouteHandler(
           })
         }
 
+        const [refreshedServer] = await db
+          .select({
+            name: mcpServers.name,
+            url: mcpServers.url,
+            connectionStatus: mcpServers.connectionStatus,
+            lastConnected: mcpServers.lastConnected,
+            lastError: mcpServers.lastError,
+            toolCount: mcpServers.toolCount,
+          })
+          .from(mcpServers)
+          .where(
+            and(
+              eq(mcpServers.id, serverId),
+              eq(mcpServers.workspaceId, workspaceId),
+              isNull(mcpServers.deletedAt)
+            )
+          )
+          .limit(1)
+
         if (discoveryError === null) {
           try {
             syncResult = await syncToolSchemasToWorkflows(
               workspaceId,
               serverId,
               discoveredTools,
               requestId,
-              { url: server.url ?? undefined, name: server.name ?? undefined }
+              {
+                url: refreshedServer?.url ?? undefined,
+                name: refreshedServer?.name ?? undefined,
+              }
             )
           } catch (error) {
             // Discovery already persisted status and cached tools; a workflow-sync
@@ -227,31 +249,9 @@ export const POST = withRouteHandler(
           }
         }
 
-        const now = new Date()
-
-        const [refreshedServer] = await db
-          .update(mcpServers)
-          .set({
-            lastToolsRefresh: now,
-            updatedAt: now,
-          })
-          .where(
-            and(
-              eq(mcpServers.id, serverId),
-              eq(mcpServers.workspaceId, workspaceId),
-              isNull(mcpServers.deletedAt)
-            )
-          )
-          .returning({
-            connectionStatus: mcpServers.connectionStatus,
-            lastConnected: mcpServers.lastConnected,
-            lastError: mcpServers.lastError,
-            toolCount: mcpServers.toolCount,
-          })
-
         let connectionStatus = refreshedServer?.connectionStatus ?? 'error'
         let lastError = refreshedServer ? refreshedServer.lastError : discoveryError
-        const toolCount = refreshedServer?.toolCount ?? discoveredTools.length
+        let toolCount = refreshedServer?.toolCount ?? discoveredTools.length
 
         if (discoveryError !== null && connectionStatus === 'connected') {
           const newerSuccessWonRace =
@@ -261,13 +261,10 @@ export const POST = withRouteHandler(
           if (!newerSuccessWonRace) {
             connectionStatus = 'disconnected'
             lastError = discoveryError
+            toolCount = 0
           }
         }
 
-        if (connectionStatus === 'connected') {
-          await mcpService.clearCache(workspaceId)
-        }
-
         return createMcpSuccessResponse({
           status: connectionStatus,
           toolCount,

apps/sim/lib/mcp/client.test.ts

@@ -1,6 +1,7 @@
 /**
  * @vitest-environment node
  */
+import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const { mockLogger, mockSdkConnect, mockSdkListTools } = vi.hoisted(() => ({
@@ -265,6 +266,54 @@ describe('McpClient notification handler', () => {
     expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain(secret)
   })
 
+  it('does not misclassify rejected static headers as an OAuth authorization flow', async () => {
+    mockSdkConnect.mockRejectedValueOnce(new UnauthorizedError('Static token rejected'))
+    const client = new McpClient({
+      config: {
+        ...createConfig(),
+        authType: 'headers',
+        headers: { Authorization: 'Bearer rejected-static-token' },
+      },
+      securityPolicy: { requireConsent: false, auditLevel: 'basic' },
+    })
+
+    await expect(client.connect()).rejects.toThrow('Static token rejected')
+
+    expect(mockLogger.error).toHaveBeenCalledWith(
+      expect.stringContaining('Failed to connect'),
+      expect.objectContaining({ outcome: 'unauthorized' })
+    )
+  })
+
+  it('logs tools/list failures without echoed credentials or session identifiers', async () => {
+    const secret = 'opaque-tools-list-credential'
+    mockSdkListTools.mockRejectedValueOnce(new Error(`Upstream rejected ${secret}`))
+    const client = new McpClient({
+      config: {
+        ...createConfig(),
+        authType: 'headers',
+        headers: { 'X-Custom-Credential': secret },
+      },
+      securityPolicy: { requireConsent: false, auditLevel: 'basic' },
+    })
+
+    await client.connect()
+    await expect(client.listTools()).rejects.toThrow(secret)
+
+    expect(mockLogger.error).toHaveBeenCalledWith(
+      expect.stringContaining('Failed to list tools'),
+      expect.objectContaining({
+        phase: 'tools/list',
+        serverId: 'server-1',
+        sessionIdPresent: true,
+        error: expect.objectContaining({ name: 'Error' }),
+      })
+    )
+    const logged = JSON.stringify(mockLogger.error.mock.calls)
+    expect(logged).not.toContain(secret)
+    expect(logged).not.toContain('test-session')
+  })
+
   it('passes configured headers for OAuth transports as well as header auth transports', () => {
     const authProvider = {} as unknown as NonNullable<McpClientOptions['authProvider']>
     new McpClient({

apps/sim/lib/mcp/client.ts

@@ -43,17 +43,33 @@ type ConnectionOutcome =
   | 'cancelled'
   | 'error'
 
-function classifyConnectionOutcome(error: unknown): ConnectionOutcome {
-  if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) {
+function classifyConnectionOutcome(
+  error: unknown,
+  authType: McpServerConfig['authType']
+): ConnectionOutcome {
+  if (error instanceof McpOauthRedirectRequired) {
     return 'authorization_required'
   }
+  if (error instanceof UnauthorizedError) {
+    return authType === 'oauth' ? 'authorization_required' : 'unauthorized'
+  }
   const message = getErrorMessage(error, '').toLowerCase()
   if (message.includes('connection attempt cancelled')) return 'cancelled'
   if (message.includes('timeout') || message.includes('timed out')) return 'timeout'
   if (message.includes('401') || message.includes('unauthorized')) return 'unauthorized'
   return 'error'
 }
 
+function getSafeErrorDiagnostics(error: unknown) {
+  const describedError = describeError(error)
+  return {
+    name: sanitizeForLogging(describedError.name, 100),
+    code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined,
+    errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined,
+    syscall: describedError.syscall ? sanitizeForLogging(describedError.syscall, 100) : undefined,
+  }
+}
+
 interface McpClientConnectOptions {
   isCancelled?: () => boolean
 }
@@ -109,7 +125,9 @@ export class McpClient {
     this.client.onerror = (error) => {
       logger.warn(`MCP transport error for ${this.config.name}`, {
         serverId: this.config.id,
-        error: sanitizeForLogging(getErrorMessage(error, 'Unknown transport error'), 200),
+        phase: 'transport',
+        sessionIdPresent: Boolean(this.transport.sessionId),
+        error: getSafeErrorDiagnostics(error),
       })
     }
   }
@@ -178,19 +196,11 @@ export class McpClient {
     } catch (error) {
       this.isConnected = false
       const errorMessage = getErrorMessage(error, 'Unknown error')
-      const describedError = describeError(error)
-      const outcome = classifyConnectionOutcome(error)
+      const outcome = classifyConnectionOutcome(error, this.config.authType)
       logger.error(`Failed to connect to MCP server ${this.config.name}`, {
         ...diagnostics,
         durationMs: Date.now() - startedAt,
-        error: {
-          name: sanitizeForLogging(describedError.name, 100),
-          code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined,
-          errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined,
-          syscall: describedError.syscall
-            ? sanitizeForLogging(describedError.syscall, 100)
-            : undefined,
-        },
+        error: getSafeErrorDiagnostics(error),
         outcome,
       })
       if (outcome === 'authorization_required') {
@@ -236,6 +246,7 @@ export class McpClient {
       MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS
     )
     const maxTotalTimeoutMs = MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS
+    const startedAt = Date.now()
 
     try {
       const result: ListToolsResult = await this.client.listTools(undefined, {
@@ -265,7 +276,15 @@ export class McpClient {
         serverName: this.config.name,
       }))
     } catch (error) {
-      logger.error(`Failed to list tools from server ${this.config.name}:`, error)
+      logger.error(`Failed to list tools from server ${this.config.name}`, {
+        serverId: this.config.id,
+        phase: 'tools/list',
+        durationMs: Date.now() - startedAt,
+        idleTimeoutMs,
+        maxTotalTimeoutMs,
+        sessionIdPresent: Boolean(this.transport.sessionId),
+        error: getSafeErrorDiagnostics(error),
+      })
       throw error
     }
   }