fix(mcp): prefer static bearer auth in connection tests

j15z · j15z · commit c0bd4e4b142d · 2026-07-14T11:02:52.000-07:00
diff --git a/apps/sim/app/api/mcp/servers/test-connection/route.test.ts b/apps/sim/app/api/mcp/servers/test-connection/route.test.ts
@@ -0,0 +1,228 @@
+/**
+ * @vitest-environment node
+ */
+import { createMockRequest, loggerMock } from '@sim/testing'
+import type { NextRequest } from 'next/server'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const {
+  mockClientOptions,
+  mockConnect,
+  mockDetectMcpAuthType,
+  mockDisconnect,
+  mockListTools,
+  mockResolveMcpConfigEnvVars,
+  mockValidateMcpServerSsrf,
+  MockMcpSsrfError,
+} = vi.hoisted(() => ({
+  mockClientOptions: vi.fn(),
+  mockConnect: vi.fn(),
+  mockDetectMcpAuthType: vi.fn(),
+  mockDisconnect: vi.fn(),
+  mockListTools: vi.fn(),
+  mockResolveMcpConfigEnvVars: vi.fn(),
+  mockValidateMcpServerSsrf: vi.fn(),
+  MockMcpSsrfError: class extends Error {},
+}))
+
+vi.mock('@/lib/core/utils/with-route-handler', () => ({
+  withRouteHandler: (handler: unknown) => handler,
+}))
+
+vi.mock('@/lib/mcp/client', () => ({
+  McpClient: class {
+    constructor(options: unknown) {
+      mockClientOptions(options)
+    }
+
+    static getVersionInfo() {
+      return { preferred: '2025-06-18', supported: ['2025-06-18'] }
+    }
+
+    connect = mockConnect
+    disconnect = mockDisconnect
+    listTools = mockListTools
+
+    getNegotiatedVersion() {
+      return '2025-06-18'
+    }
+  },
+}))
+
+vi.mock('@/lib/mcp/domain-check', () => ({
+  McpDnsResolutionError: class extends Error {},
+  McpDomainNotAllowedError: class extends Error {},
+  McpSsrfError: MockMcpSsrfError,
+  validateMcpDomain: vi.fn(),
+  validateMcpServerSsrf: mockValidateMcpServerSsrf,
+}))
+
+vi.mock('@/lib/mcp/middleware', () => ({
+  mcpBodyReadErrorResponse: vi.fn(() => null),
+  readMcpJsonBodyWithLimit: (request: NextRequest) => request.json(),
+  withMcpAuth:
+    () =>
+    (
+      handler: (
+        request: NextRequest,
+        context: { userId: string; workspaceId: string; requestId: string }
+      ) => Promise<Response>
+    ) =>
+    (request: NextRequest) =>
+      handler(request, {
+        userId: 'user-1',
+        workspaceId: 'workspace-1',
+        requestId: 'request-1',
+      }),
+}))
+
+vi.mock('@/lib/mcp/oauth', () => ({
+  detectMcpAuthType: mockDetectMcpAuthType,
+}))
+
+vi.mock('@/lib/mcp/resolve-config', () => ({
+  resolveMcpConfigEnvVars: mockResolveMcpConfigEnvVars,
+}))
+
+import { POST } from '@/app/api/mcp/servers/test-connection/route'
+
+const mockLogger = vi.mocked(loggerMock.createLogger).mock.results.at(-1)?.value
+
+function createTestRequest(headers: Record<string, string> = {}) {
+  return createMockRequest(
+    'POST',
+    {
+      name: 'Dual Auth Server',
+      transport: 'streamable-http',
+      url: 'https://example.com/mcp',
+      headers,
+      timeout: 10000,
+    },
+    {},
+    'http://localhost/api/mcp/servers/test-connection'
+  )
+}
+
+describe('MCP server test-connection route', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockDetectMcpAuthType.mockResolvedValue('oauth')
+    mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
+    mockResolveMcpConfigEnvVars.mockImplementation(async (config: unknown) => ({
+      config,
+      missingVars: [],
+    }))
+    mockConnect.mockResolvedValue(undefined)
+    mockListTools.mockResolvedValue([])
+    mockDisconnect.mockResolvedValue(undefined)
+  })
+
+  it('tests configured bearer headers before treating OAuth discovery as mandatory', async () => {
+    const response = await POST(createTestRequest({ Authorization: 'Bearer static-api-token' }))
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.data).toEqual(
+      expect.objectContaining({ success: true, authType: 'headers', toolCount: 0 })
+    )
+    expect(mockDetectMcpAuthType).not.toHaveBeenCalled()
+    expect(mockClientOptions).toHaveBeenCalledWith(
+      expect.objectContaining({
+        config: expect.objectContaining({
+          headers: { Authorization: 'Bearer static-api-token' },
+        }),
+      })
+    )
+    expect(mockConnect).toHaveBeenCalledTimes(1)
+  })
+
+  it('returns a header-auth failure when the configured token is rejected', async () => {
+    mockConnect.mockRejectedValueOnce(new Error('HTTP 401: Unauthorized'))
+
+    const response = await POST(createTestRequest({ Authorization: 'Bearer invalid-static-token' }))
+    const body = await response.json()
+
+    expect(response.status).toBe(400)
+    expect(body.data).toEqual(
+      expect.objectContaining({
+        success: false,
+        authType: 'headers',
+        error: 'HTTP 401: Unauthorized',
+      })
+    )
+    expect(mockDetectMcpAuthType).not.toHaveBeenCalled()
+    expect(mockConnect).toHaveBeenCalledTimes(1)
+    expect(mockDisconnect).toHaveBeenCalledTimes(1)
+  })
+
+  it('does not expose configured credentials echoed by an upstream error', async () => {
+    const token = 'opaque-static-token'
+    mockConnect.mockRejectedValueOnce(new Error(`Upstream rejected ${token}`))
+
+    const response = await POST(createTestRequest({ Authorization: `Bearer ${token}` }))
+    const body = await response.json()
+
+    expect(response.status).toBe(400)
+    expect(body.data).toEqual(
+      expect.objectContaining({ success: false, authType: 'headers', error: 'Connection failed' })
+    )
+    expect(mockLogger).toBeDefined()
+    expect(JSON.stringify(mockLogger?.warn.mock.calls)).not.toContain(token)
+  })
+
+  it('preserves OAuth discovery when no static headers are configured', async () => {
+    const response = await POST(createTestRequest())
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.data).toEqual(
+      expect.objectContaining({ success: false, authRequired: true, authType: 'oauth' })
+    )
+    expect(mockDetectMcpAuthType).toHaveBeenCalledWith('https://example.com/mcp', '203.0.113.10')
+    expect(mockClientOptions).not.toHaveBeenCalled()
+  })
+
+  it('preserves OAuth discovery when only supplemental headers are configured', async () => {
+    const response = await POST(createTestRequest({ 'X-Sim-Via': 'workflow' }))
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.data).toEqual(
+      expect.objectContaining({ success: false, authRequired: true, authType: 'oauth' })
+    )
+    expect(mockDetectMcpAuthType).toHaveBeenCalledWith('https://example.com/mcp', '203.0.113.10')
+    expect(mockClientOptions).not.toHaveBeenCalled()
+  })
+
+  it('blocks an env-resolved private URL before forwarding configured credentials', async () => {
+    const token = 'private-static-token'
+    mockResolveMcpConfigEnvVars.mockResolvedValueOnce({
+      config: {
+        id: 'test-request-1',
+        name: 'Dual Auth Server',
+        transport: 'streamable-http',
+        url: 'http://127.0.0.1/mcp',
+        headers: { Authorization: `Bearer ${token}` },
+        timeout: 10000,
+        retries: 1,
+        enabled: true,
+      },
+      missingVars: [],
+    })
+    mockValidateMcpServerSsrf.mockImplementation(async (url: string) => {
+      if (url === 'http://127.0.0.1/mcp') {
+        throw new MockMcpSsrfError('Private network targets are not allowed')
+      }
+      return '203.0.113.10'
+    })
+
+    const response = await POST(createTestRequest({ Authorization: `Bearer ${token}` }))
+    const responseText = await response.text()
+
+    expect(response.status).toBe(403)
+    expect(responseText).not.toContain(token)
+    expect(mockValidateMcpServerSsrf).toHaveBeenNthCalledWith(2, 'http://127.0.0.1/mcp')
+    expect(mockClientOptions).not.toHaveBeenCalled()
+    expect(mockDetectMcpAuthType).not.toHaveBeenCalled()
+  })
+})
diff --git a/apps/sim/app/api/mcp/servers/test-connection/route.ts b/apps/sim/app/api/mcp/servers/test-connection/route.ts
@@ -1,6 +1,5 @@
 import { createLogger } from '@sim/logger'
 import { toError } from '@sim/utils/errors'
-import { truncate } from '@sim/utils/string'
 import type { NextRequest } from 'next/server'
 import { mcpServerTestBodySchema } from '@/lib/api/contracts/mcp'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -50,17 +49,35 @@ interface TestConnectionResult {
 }
 
 /**
- * Extracts a user-friendly error message from connection errors.
- * Keeps diagnostic info (timeout, DNS, HTTP status) but strips
- * verbose internals (Zod details, full response bodies, stack traces).
+ * Maps connection failures to allowlisted messages. Upstream response bodies
+ * may echo configured credentials, so arbitrary error text must not reach API
+ * responses or logs.
  */
 function sanitizeConnectionError(error: unknown): string {
   if (!(error instanceof Error)) {
     return 'Unknown connection error'
   }
 
-  const firstLine = error.message.split('\n')[0]
-  return truncate(firstLine, 200)
+  const message = error.message.toLowerCase()
+  if (message.includes('timeout') || message.includes('timed out')) {
+    return 'Connection timed out'
+  }
+  if (message.includes('401') || message.includes('unauthorized')) {
+    return 'HTTP 401: Unauthorized'
+  }
+  if (message.includes('403') || message.includes('forbidden')) {
+    return 'HTTP 403: Forbidden'
+  }
+  if (message.includes('enotfound') || message.includes('could not resolve')) {
+    return 'MCP server hostname could not be resolved'
+  }
+  if (message.includes('econnrefused') || message.includes('connection refused')) {
+    return 'Connection refused'
+  }
+  if (message.includes('certificate') || message.includes('tls') || message.includes('ssl')) {
+    return 'TLS connection failed'
+  }
+  return 'Connection failed'
 }
 
 /**
@@ -172,8 +189,14 @@ export const POST = withRouteHandler(
 
       const result: TestConnectionResult = { success: false }
 
-      // Skip unauth connect when the server returns an RFC 9728 OAuth challenge.
-      if (testConfig.url) {
+      /** An explicit static Bearer token takes precedence over optional OAuth discovery. */
+      const hasStaticBearerToken = Object.entries(testConfig.headers ?? {}).some(
+        ([name, value]) =>
+          name.toLowerCase() === 'authorization' && /^Bearer\s+\S+/i.test(value.trim())
+      )
+      if (hasStaticBearerToken) {
+        result.authType = 'headers'
+      } else if (testConfig.url) {
         const detectedAuthType = await detectMcpAuthType(testConfig.url, resolvedIP)
         if (detectedAuthType === 'oauth') {
           result.authRequired = true
@@ -199,8 +222,8 @@ export const POST = withRouteHandler(
           const tools = await client.listTools()
           result.toolCount = tools.length
           result.success = true
-        } catch (toolError) {
-          logger.warn(`[${requestId}] Connection established but could not list tools:`, toolError)
+        } catch {
+          logger.warn(`[${requestId}] Connection established but could not list tools`)
           result.success = false
           result.error = 'Connection established but could not list tools'
           result.warnings = result.warnings || []
@@ -224,16 +247,15 @@ export const POST = withRouteHandler(
           capabilities: result.supportedCapabilities,
         })
       } catch (error) {
-        logger.warn(`[${requestId}] MCP server test failed:`, error)
-
         result.success = false
         result.error = sanitizeConnectionError(error)
+        logger.warn(`[${requestId}] MCP server test failed`, { error: result.error })
       } finally {
         if (client) {
           try {
             await client.disconnect()
-          } catch (disconnectError) {
-            logger.debug(`[${requestId}] Test client disconnect error (expected):`, disconnectError)
+          } catch {
+            logger.debug(`[${requestId}] Test client disconnect error (expected)`)
           }
         }
       }
