fix(api): answer upstream MCP, OAuth, and id-conflict failures with 4xx; keep PII keep-alive past the load balancer

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

@@ -9,6 +9,7 @@ import {
   permissionsMockFns,
   resetDbChainMock,
 } from '@sim/testing'
+import { createRouteContext } from '@sim/testing/helpers/http'
 import { NextRequest } from 'next/server'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
@@ -61,7 +62,7 @@ describe('MCP OAuth start route', () => {
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
     )
 
-    await GET(request)
+    await GET(request, createRouteContext({}))
 
     // The route must call the guarded wrapper (which defaults fetchFn to the
     // SSRF-guarded fetch internally) rather than the raw SDK `auth()` — see
@@ -82,7 +83,7 @@ describe('MCP OAuth start route', () => {
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
     )
 
-    const response = await GET(request)
+    const response = await GET(request, createRouteContext({}))
 
     expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledTimes(1)
     expect(response.status).toBe(504)
@@ -105,7 +106,7 @@ describe('MCP OAuth start route', () => {
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
     )
 
-    const response = await GET(request)
+    const response = await GET(request, createRouteContext({}))
     const body = await response.json()
 
     expect(response.status).toBe(409)
@@ -121,7 +122,7 @@ describe('MCP OAuth start route', () => {
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
     )
 
-    const response = await GET(request)
+    const response = await GET(request, createRouteContext({}))
     const body = await response.json()
 
     expect(response.status).toBe(500)
@@ -138,7 +139,7 @@ describe('MCP OAuth start route', () => {
       'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
     )
 
-    const response = await GET(request)
+    const response = await GET(request, createRouteContext({}))
     const body = await response.json()
 
     expect(response.status).toBe(422)
@@ -148,6 +149,103 @@ describe('MCP OAuth start route', () => {
   })
 })
 
+/**
+ * Runs the SDK's real dynamic client registration against an authorization server that
+ * answers with `status` and `body`, so the route sees exactly the error class the SDK builds.
+ */
+async function registerAgainst(status: number, body: string) {
+  const { registerClient } = await import('@modelcontextprotocol/sdk/client/auth.js')
+  return registerClient('https://auth.example.com', {
+    clientMetadata: { redirect_uris: ['https://sim.example.com/api/mcp/oauth/callback'] },
+    fetchFn: async () => new Response(body, { status }),
+  })
+}
+
+describe('MCP OAuth start route: authorization-server refusals', () => {
+  beforeEach(() => {
+    resetDbChainMock()
+    hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
+      success: true,
+      userId: 'user-1',
+      userName: 'User One',
+      userEmail: 'user1@example.com',
+      authType: 'session',
+    })
+    permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
+    dbChainMockFns.limit.mockResolvedValue([
+      {
+        id: 'server-1',
+        name: 'Docs',
+        url: 'https://mcp.example.com/mcp',
+        workspaceId: 'workspace-1',
+        authType: 'oauth',
+        deletedAt: null,
+      },
+    ])
+    mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValue({
+      id: 'oauth-row-1',
+      mcpServerId: 'server-1',
+      userId: 'user-1',
+      workspaceId: 'workspace-1',
+      clientInformation: null,
+      tokens: null,
+      codeVerifier: null,
+      state: null,
+      stateCreatedAt: null,
+      updatedAt: new Date(),
+    })
+    mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined)
+  })
+
+  const start = () =>
+    GET(
+      new NextRequest(
+        'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
+      ),
+      createRouteContext({})
+    )
+
+  it.each([
+    [
+      'an RFC 7591 code the SDK does not know, which it raises as a ServerError',
+      400,
+      '{"error":"invalid_redirect_uri","error_description":"Only loopback redirect URIs are supported."}',
+      'Only loopback redirect URIs are supported.',
+    ],
+    [
+      'a registered OAuth code, raised as its typed subclass',
+      400,
+      '{"error":"invalid_client_metadata","error_description":"client_name is required"}',
+      'invalid_client_metadata: client_name is required',
+    ],
+    [
+      'a non-spec body, wrapped by the SDK in a ServerError',
+      403,
+      '{"message":"redirect URI https://sim.example.com/api/mcp/oauth/callback is not allowed"}',
+      'Authorization server: redirect URI https://sim.example.com/api/mcp/oauth/callback is not allowed',
+    ],
+  ] as const)(
+    'answers 422 with the server message for %s',
+    async (_name, status, body, message) => {
+      mcpOauthMockFns.mockMcpAuthGuarded.mockImplementationOnce(() => registerAgainst(status, body))
+
+      const response = await start()
+
+      expect(response.status).toBe(422)
+      expect((await response.json()).error).toBe(message)
+    }
+  )
+
+  it('keeps a 500 for a failure that is not an authorization-server answer', async () => {
+    mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(new Error('decrypt failed'))
+
+    const response = await start()
+
+    expect(response.status).toBe(500)
+    expect((await response.json()).error).toBe('Failed to start OAuth flow')
+  })
+})
+
 describe('surfaceOauthError', () => {
   it('parses Raw body envelope for ServerError fallbacks (non-spec vendors)', async () => {
     const { ServerError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js')

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

@@ -49,7 +49,7 @@ const DCR_UNSUPPORTED_MESSAGE =
 /**
  * The MCP SDK throws a plain `Error` (no typed class or code) when an auth server lacks a
  * `registration_endpoint`, so this string-match is the only available signal. Pinned to
- * `@modelcontextprotocol/sdk` v1.29.0 `registerClient` (client/auth.js): "Incompatible auth
+ * `@modelcontextprotocol/sdk` v1.31.0 `registerClient` (client/auth.js): "Incompatible auth
  * server: does not support dynamic client registration". If a version bump rephrases this, the
  * 422 branch silently stops firing (users get a generic 500) — update the substring here.
  */
@@ -170,43 +170,53 @@ export const GET = withRouteHandler(
       )
       const provider = new SimMcpOauthProvider({ row, preregistered })
 
-      try {
-        // OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails
-        // fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is
-        // the SUCCESS signal (a throw carrying the authorize URL), so we catch it INSIDE the
-        // bounded step and return it as a normal value — otherwise timedStep would error-log
-        // every successful authorize. Only a real error or a timeout escapes as a throw.
-        const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => {
-          try {
-            return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) }
-          } catch (e) {
-            if (e instanceof McpOauthRedirectRequired) {
-              return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl }
+      // OAuth discovery + DCR through the guarded fetch, bounded so a transient stall fails
+      // fast with a labeled log instead of hanging the popup. `McpOauthRedirectRequired` is
+      // the SUCCESS signal (a throw carrying the authorize URL), and an authorization-server
+      // refusal is an expected outcome rather than a Sim fault, so both are caught INSIDE the
+      // bounded step and returned as values — otherwise timedStep would error-log them. Only
+      // a real error or a timeout escapes as a throw.
+      const authOutcome = await timedStep('mcpAuthGuarded', MCP_AUTH_STEP_MS, async () => {
+        try {
+          return { kind: 'result' as const, value: await mcpAuthGuarded(provider, { serverUrl }) }
+        } catch (e) {
+          if (e instanceof McpOauthRedirectRequired) {
+            return { kind: 'redirect' as const, authorizationUrl: e.authorizationUrl }
+          }
+          if (isDynamicClientRegistrationUnsupported(e)) {
+            return {
+              kind: 'rejected' as const,
+              error: toError(e),
+              message: DCR_UNSUPPORTED_MESSAGE,
             }
-            throw e
           }
-        })
-        if (authOutcome.kind === 'redirect') {
-          logger.info(`OAuth redirect for server ${serverId}`)
-          return NextResponse.json({
-            status: 'redirect',
-            authorizationUrl: authOutcome.authorizationUrl,
-          })
-        }
-        if (authOutcome.value === 'AUTHORIZED') {
-          return NextResponse.json({ status: 'already_authorized' })
-        }
-        return createMcpErrorResponse(
-          new Error('Provider did not capture redirect URL'),
-          'Failed to start OAuth flow',
-          500
-        )
-      } catch (e) {
-        if (isDynamicClientRegistrationUnsupported(e)) {
-          return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
+          if (e instanceof OAuthError) {
+            return { kind: 'rejected' as const, error: e, message: surfaceOauthError(e) }
+          }
+          throw e
         }
-        throw e
+      })
+      if (authOutcome.kind === 'redirect') {
+        logger.info(`OAuth redirect for server ${serverId}`)
+        return NextResponse.json({
+          status: 'redirect',
+          authorizationUrl: authOutcome.authorizationUrl,
+        })
       }
+      if (authOutcome.kind === 'rejected') {
+        logger.warn(`Authorization server refused the MCP OAuth flow for server ${serverId}`, {
+          error: authOutcome.error.message,
+        })
+        return createMcpErrorResponse(authOutcome.error, authOutcome.message, 422)
+      }
+      if (authOutcome.value === 'AUTHORIZED') {
+        return NextResponse.json({ status: 'already_authorized' })
+      }
+      return createMcpErrorResponse(
+        new Error('Provider did not capture redirect URL'),
+        'Failed to start OAuth flow',
+        500
+      )
     } catch (error) {
       // Any bounded step timing out (DB reads or the auth step) is a stall, not a bug —
       // surface it as a fast 504 so the popup closes with a clear "try again" rather than a
@@ -223,11 +233,7 @@ export const GET = withRouteHandler(
         )
       }
       logger.error('Error starting MCP OAuth flow:', error)
-      // Only surface OAuth-flow errors verbatim; everything else (DB, decryption,
-      // network) gets a generic message to avoid leaking internal details.
-      const userMessage =
-        error instanceof OAuthError ? surfaceOauthError(error) : 'Failed to start OAuth flow'
-      return createMcpErrorResponse(toError(error), userMessage, 500)
+      return createMcpErrorResponse(toError(error), 'Failed to start OAuth flow', 500)
     }
   })
 )

apps/sim/app/api/mcp/tools/discover/route.test.ts

@@ -24,7 +24,10 @@ vi.mock('@/lib/mcp/middleware', () => ({
 
 vi.mock('@/lib/mcp/service', () => mcpServiceMock)
 
+import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
+import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js'
 import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
+import { McpConnectionError, McpServerCooldownError } from '@/lib/mcp/types'
 import { GET } from '@/app/api/mcp/tools/discover/route'
 
 describe('GET /api/mcp/tools/discover', () => {
@@ -43,4 +46,37 @@ describe('GET /api/mcp/tools/discover', () => {
     expect(response.status).toBe(401)
     expect(JSON.stringify(await response.json())).not.toContain('secret-state')
   })
+
+  const discover = () =>
+    GET(
+      new NextRequest(
+        'http://localhost:3000/api/mcp/tools/discover?workspaceId=workspace-1&serverId=mcp-1'
+      ),
+      createRouteContext({})
+    )
+
+  it.each([
+    ['cannot be reached', () => new McpConnectionError('connect ECONNREFUSED', 'Docs')],
+    ['is cooling down after a recent failure', () => new McpServerCooldownError('mcp-1')],
+    ['times out on tools/list', () => new McpError(ErrorCode.RequestTimeout, 'Request timed out')],
+    [
+      'answers tools/list with an HTTP error',
+      () => new StreamableHTTPError(503, 'Service Unavailable'),
+    ],
+  ])('answers 422 when the registered server %s', async (_name, makeError) => {
+    mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(makeError())
+
+    const response = await discover()
+
+    expect(response.status).toBe(422)
+    expect((await response.json()).error).toBe('Failed to discover MCP tools')
+  })
+
+  it('keeps a 500 for a failure that is not the registered server', async () => {
+    mcpServiceMockFns.mockDiscoverServerTools.mockRejectedValueOnce(new Error('pool exhausted'))
+
+    const response = await discover()
+
+    expect(response.status).toBe(500)
+  })
 })

apps/sim/app/api/mcp/tools/discover/route.ts

@@ -13,13 +13,48 @@ import {
 import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
 import { mcpService } from '@/lib/mcp/service'
 import { McpOauthAuthorizationRequiredError, type McpToolDiscoveryResponse } from '@/lib/mcp/types'
-import { categorizeError, createMcpErrorResponse, createMcpSuccessResponse } from '@/lib/mcp/utils'
+import {
+  categorizeError,
+  createMcpErrorResponse,
+  createMcpSuccessResponse,
+  isMcpServerUnavailableError,
+} from '@/lib/mcp/utils'
 
 const logger = createLogger('McpToolDiscoveryAPI')
 const MCP_REFRESH_DISCOVERY_CONCURRENCY = 5
 
 export const dynamic = 'force-dynamic'
 
+/**
+ * Renders a discovery failure that is not an authorization problem.
+ *
+ * Discovery talks to a server the workspace registered, so a server that cannot be
+ * reached, times out on connect or on `tools/list`, answers with a protocol or HTTP
+ * error, or is cooling down after a recent failure is a fault of that server rather
+ * than of Sim: it answers 422 (the request was valid; the dependency it names cannot
+ * serve it), the status this API family already uses for an upstream that cannot
+ * complete the operation, and logs at warn. Anything else keeps {@link categorizeError}'s
+ * status and logs at error only when that status is a 5xx.
+ */
+function discoveryFailureResponse(requestId: string, error: unknown) {
+  if (isMcpServerUnavailableError(error)) {
+    logger.warn(`[${requestId}] MCP server unavailable for tool discovery`, {
+      error: getErrorMessage(error),
+    })
+    return createMcpErrorResponse(error, 'Failed to discover MCP tools', 422)
+  }
+  const { message, status } = categorizeError(error)
+  if (status >= 500) {
+    logger.error(`[${requestId}] Error discovering MCP tools:`, error)
+  } else {
+    logger.warn(`[${requestId}] Failed to discover MCP tools`, {
+      error: getErrorMessage(error),
+      status,
+    })
+  }
+  return createMcpErrorResponse(new Error(message), 'Failed to discover MCP tools', status)
+}
+
 async function settleWithConcurrency<T, R>(
   items: T[],
   concurrency: number,
@@ -104,9 +139,7 @@ export const GET = withRouteHandler(
       ) {
         return createMcpErrorResponse(error, 'OAuth re-authorization required', 401)
       }
-      logger.error(`[${requestId}] Error discovering MCP tools:`, error)
-      const { message, status } = categorizeError(error)
-      return createMcpErrorResponse(new Error(message), 'Failed to discover MCP tools', status)
+      return discoveryFailureResponse(requestId, error)
     }
   })
 )