From f09fe988c1672229956c8e483c133692126f8589 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:26:35 -0700 Subject: [PATCH 1/8] fix(mcp): surface authorization-server error instead of generic toast --- apps/sim/app/api/mcp/oauth/start/route.ts | 29 ++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 55a7d41956f..0d4851fbce4 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -23,6 +23,33 @@ import { createMcpErrorResponse } from '@/lib/mcp/utils' const logger = createLogger('McpOauthStartAPI') const OAUTH_START_TTL_MS = 10 * 60 * 1000 +const MAX_SURFACED_ERROR_LENGTH = 250 + +function surfaceOauthError(error: unknown): string { + const raw = error instanceof Error ? error.message : String(error) + const rawBodyMatch = raw.match(/Raw body:\s*(\{[\s\S]*\})\s*$/) + if (rawBodyMatch) { + try { + const body = JSON.parse(rawBodyMatch[1]) as Record + const vendorMessage = + typeof body.error_description === 'string' + ? body.error_description + : typeof body.message === 'string' + ? body.message + : typeof body.error === 'string' + ? body.error + : null + if (vendorMessage) return truncate(`Authorization server: ${vendorMessage}`) + } catch {} + } + return truncate(raw.split('\n')[0] || 'Failed to start OAuth flow') +} + +function truncate(message: string): string { + return message.length > MAX_SURFACED_ERROR_LENGTH + ? `${message.slice(0, MAX_SURFACED_ERROR_LENGTH)}…` + : message +} export const dynamic = 'force-dynamic' @@ -116,7 +143,7 @@ export const GET = withRouteHandler( } } catch (error) { logger.error('Error starting MCP OAuth flow:', error) - return createMcpErrorResponse(toError(error), 'Failed to start OAuth flow', 500) + return createMcpErrorResponse(toError(error), surfaceOauthError(error), 500) } }) ) From 3d56049f3dc7b4176ca959a08e3c7f03d6a5084a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:26:35 -0700 Subject: [PATCH 2/8] perf(mcp): cache tool discovery per-server with test coverage --- apps/sim/lib/mcp/service.test.ts | 243 +++++++++++++++++++++++++++++++ apps/sim/lib/mcp/service.ts | 212 ++++++++++++++++----------- 2 files changed, 369 insertions(+), 86 deletions(-) create mode 100644 apps/sim/lib/mcp/service.test.ts diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts new file mode 100644 index 00000000000..23e57ed9d14 --- /dev/null +++ b/apps/sim/lib/mcp/service.test.ts @@ -0,0 +1,243 @@ +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { + MockMcpClient, + mockListTools, + mockConnect, + mockDisconnect, + mockGetWorkspaceServersRows, + mockResolveEnvVars, + mockValidateDomain, + mockValidateSsrf, + mockIsDomainAllowed, +} = vi.hoisted(() => { + const mockListTools = vi.fn() + const mockConnect = vi.fn() + const mockDisconnect = vi.fn() + return { + MockMcpClient: vi.fn().mockImplementation(() => ({ + connect: mockConnect, + disconnect: mockDisconnect, + listTools: mockListTools, + hasListChangedCapability: vi.fn(() => false), + onClose: vi.fn(), + getNegotiatedVersion: vi.fn(() => '2025-06-18'), + })), + mockListTools, + mockConnect, + mockDisconnect, + mockGetWorkspaceServersRows: vi.fn(), + mockResolveEnvVars: vi.fn(), + mockValidateDomain: vi.fn(), + mockValidateSsrf: vi.fn(), + mockIsDomainAllowed: vi.fn(() => true), + } +}) + +vi.mock('@sim/db', () => { + const where = vi.fn() + const setter = vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }) + return { + db: { + select: vi.fn().mockReturnValue({ + from: vi.fn().mockReturnValue({ + where: (...args: unknown[]) => mockGetWorkspaceServersRows(...args), + }), + }), + update: vi.fn().mockReturnValue({ set: setter }), + insert: vi.fn(), + delete: vi.fn(), + }, + __where: where, + } +}) + +vi.mock('@/lib/mcp/client', () => ({ + McpClient: MockMcpClient, +})) + +vi.mock('@/lib/mcp/connection-manager', () => ({ + mcpConnectionManager: null, +})) + +vi.mock('@/lib/mcp/domain-check', () => ({ + isMcpDomainAllowed: (...args: unknown[]) => mockIsDomainAllowed(...args), + validateMcpDomain: (...args: unknown[]) => mockValidateDomain(...args), + validateMcpServerSsrf: (...args: unknown[]) => mockValidateSsrf(...args), +})) + +vi.mock('@/lib/mcp/oauth', () => ({ + getOrCreateOauthRow: vi.fn(), + loadPreregisteredClient: vi.fn(), + SimMcpOauthProvider: vi.fn(), + withMcpOauthRefreshLock: vi.fn(), +})) + +vi.mock('@/lib/mcp/resolve-config', () => ({ + resolveMcpConfigEnvVars: (...args: unknown[]) => mockResolveEnvVars(...args), +})) + +import { mcpService } from '@/lib/mcp/service' +import { McpOauthAuthorizationRequiredError } from '@/lib/mcp/types' + +const WORKSPACE_ID = 'workspace-test' +const USER_ID = 'user-test' + +function dbRow(id: string, name: string, overrides: Record = {}) { + return { + id, + name, + description: null, + transport: 'streamable-http', + url: `https://${id}.example.com/mcp`, + authType: 'headers', + workspaceId: WORKSPACE_ID, + headers: {}, + timeout: 30000, + retries: 3, + enabled: true, + deletedAt: null, + createdAt: new Date('2026-01-01T00:00:00Z'), + updatedAt: new Date('2026-01-01T00:00:00Z'), + ...overrides, + } +} + +function tool(name: string, serverId: string) { + return { + name, + description: name, + inputSchema: { type: 'object' }, + serverId, + serverName: serverId, + } +} + +describe('McpService.discoverTools per-server caching', () => { + beforeEach(async () => { + vi.clearAllMocks() + mockIsDomainAllowed.mockReturnValue(true) + mockValidateSsrf.mockResolvedValue('1.2.3.4') + mockValidateDomain.mockImplementation(() => undefined) + mockResolveEnvVars.mockImplementation((config: { url: string }) => + Promise.resolve({ config: { ...config, url: config.url }, missingVars: [] }) + ) + mockConnect.mockResolvedValue(undefined) + mockDisconnect.mockResolvedValue(undefined) + // The McpService singleton holds cache state across imports. + await mcpService.clearCache() + }) + + it('caches each server independently after first discovery', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A'), dbRow('mcp-b', 'B')]) + mockListTools + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + .mockResolvedValueOnce([tool('b1', 'mcp-b')]) + + const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(first.map((t) => t.name).sort()).toEqual(['a1', 'b1']) + expect(mockListTools).toHaveBeenCalledTimes(2) + + mockListTools.mockClear() + const second = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(second.map((t) => t.name).sort()).toEqual(['a1', 'b1']) + expect(mockListTools).not.toHaveBeenCalled() + }) + + it("one server failing does not poison another server's cache", async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A'), dbRow('mcp-b', 'B')]) + mockListTools + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + .mockRejectedValueOnce(new Error('Request timed out')) + + const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(first.map((t) => t.name)).toEqual(['a1']) + + mockListTools.mockClear() + mockListTools.mockResolvedValueOnce([tool('b1', 'mcp-b')]) + + const second = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(second.map((t) => t.name).sort()).toEqual(['a1', 'b1']) + expect(mockListTools).toHaveBeenCalledTimes(1) + }) + + it("forceRefresh bypasses every server's cache", async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A'), dbRow('mcp-b', 'B')]) + mockListTools + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + .mockResolvedValueOnce([tool('b1', 'mcp-b')]) + + await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(mockListTools).toHaveBeenCalledTimes(2) + + mockListTools.mockClear() + mockListTools + .mockResolvedValueOnce([tool('a2', 'mcp-a')]) + .mockResolvedValueOnce([tool('b2', 'mcp-b')]) + + const refreshed = await mcpService.discoverTools(USER_ID, WORKSPACE_ID, true) + expect(refreshed.map((t) => t.name).sort()).toEqual(['a2', 'b2']) + expect(mockListTools).toHaveBeenCalledTimes(2) + }) + + it('OAuth-pending is treated as a soft skip without poisoning cache', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A'), dbRow('mcp-b', 'B')]) + mockListTools + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + .mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-b', 'B')) + + const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(first.map((t) => t.name)).toEqual(['a1']) + + mockListTools.mockClear() + mockListTools.mockRejectedValueOnce(new McpOauthAuthorizationRequiredError('mcp-b', 'B')) + + await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(mockListTools).toHaveBeenCalledTimes(1) + }) + + it('returns empty array immediately when workspace has no servers', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([]) + + const result = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(result).toEqual([]) + expect(mockListTools).not.toHaveBeenCalled() + expect(MockMcpClient).not.toHaveBeenCalled() + }) + + it('clearCache(workspaceId) drops cached tools so next call re-fetches', async () => { + mockGetWorkspaceServersRows.mockResolvedValue([dbRow('mcp-a', 'A')]) + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + + await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(mockListTools).toHaveBeenCalledTimes(1) + + await mcpService.clearCache(WORKSPACE_ID) + + mockListTools.mockClear() + mockListTools.mockResolvedValueOnce([tool('a1', 'mcp-a')]) + await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + expect(mockListTools).toHaveBeenCalledTimes(1) + }) + + it('isolates caches across workspaces', async () => { + const otherWorkspaceId = 'workspace-other' + mockGetWorkspaceServersRows + .mockResolvedValueOnce([dbRow('mcp-a', 'A')]) + .mockResolvedValueOnce([dbRow('mcp-a', 'A', { workspaceId: otherWorkspaceId })]) + + mockListTools + .mockResolvedValueOnce([tool('a1', 'mcp-a')]) + .mockResolvedValueOnce([tool('a-other', 'mcp-a')]) + + const first = await mcpService.discoverTools(USER_ID, WORKSPACE_ID) + const second = await mcpService.discoverTools(USER_ID, otherWorkspaceId) + + expect(first.map((t) => t.name)).toEqual(['a1']) + expect(second.map((t) => t.name)).toEqual(['a-other']) + expect(mockListTools).toHaveBeenCalledTimes(2) + }) +}) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 1c3c2fd291a..01b12123635 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -45,6 +45,22 @@ import { MCP_CONSTANTS } from '@/lib/mcp/utils' const logger = createLogger('McpService') +// Per-server keys so one slow server can't invalidate another's cached tools. +function serverCacheKey(workspaceId: string, serverId: string): string { + return `workspace:${workspaceId}:server:${serverId}` +} + +type DiscoveryOutcome = + | { kind: 'cached'; tools: McpTool[] } + | { + kind: 'fetched' + tools: McpTool[] + resolvedConfig: McpServerConfig + resolvedIP: string | null + } + | { kind: 'oauth-pending' } + | { kind: 'error'; message: string } + class McpService { private cacheAdapter: McpCacheStorageAdapter private readonly cacheTimeout = MCP_CONSTANTS.CACHE_TIMEOUT @@ -56,7 +72,11 @@ class McpService { if (mcpConnectionManager) { this.unsubscribeConnectionManager = mcpConnectionManager.subscribe((event) => { - this.clearCache(event.workspaceId) + this.cacheAdapter + .delete(serverCacheKey(event.workspaceId, event.serverId)) + .catch((err) => + logger.warn(`Failed to invalidate cache for ${event.serverName} on listChanged:`, err) + ) }) } } @@ -379,20 +399,7 @@ class McpService { ): Promise { const requestId = generateRequestId() - const cacheKey = `workspace:${workspaceId}` - try { - if (!forceRefresh) { - try { - const cached = await this.cacheAdapter.get(cacheKey) - if (cached) { - return cached.tools - } - } catch (error) { - logger.warn(`[${requestId}] Cache read failed, proceeding with discovery:`, error) - } - } - logger.info(`[${requestId}] Discovering MCP tools for workspace ${workspaceId}`) const servers = await this.getWorkspaceServers(workspaceId) @@ -402,51 +409,91 @@ class McpService { return [] } - const allTools: McpTool[] = [] - const results = await Promise.allSettled( - servers.map(async (config) => { - const { config: resolvedConfig, resolvedIP } = await this.resolveConfigEnvVars( - config, - userId, - workspaceId - ) - const client = await this.createClient(resolvedConfig, resolvedIP, userId) + const outcomes = await Promise.all( + servers.map(async (config): Promise => { + const cacheKey = serverCacheKey(workspaceId, config.id) + + if (!forceRefresh) { + try { + const cached = await this.cacheAdapter.get(cacheKey) + if (cached) return { kind: 'cached', tools: cached.tools } + } catch (error) { + logger.warn( + `[${requestId}] Cache read failed for ${config.name}, proceeding with discovery:`, + error + ) + } + } + try { - const tools = await client.listTools() - logger.debug( - `[${requestId}] Discovered ${tools.length} tools from server ${config.name}` + const { config: resolvedConfig, resolvedIP } = await this.resolveConfigEnvVars( + config, + userId, + workspaceId ) - return { serverId: config.id, tools, resolvedConfig, resolvedIP } - } finally { - await client.disconnect() + const client = await this.createClient(resolvedConfig, resolvedIP, userId) + try { + const tools = await client.listTools() + logger.debug( + `[${requestId}] Discovered ${tools.length} tools from server ${config.name}` + ) + return { kind: 'fetched', tools, resolvedConfig, resolvedIP } + } finally { + await client.disconnect() + } + } catch (error) { + if ( + error instanceof McpOauthAuthorizationRequiredError || + error instanceof UnauthorizedError + ) { + return { kind: 'oauth-pending' } + } + return { kind: 'error', message: getErrorMessage(error, 'Unknown error') } } }) ) + const allTools: McpTool[] = [] + const cacheWrites: Promise[] = [] + const deferredSideEffects: Promise[] = [] + const liveConnections: Array<{ + resolvedConfig: McpServerConfig + resolvedIP: string | null + }> = [] + let cachedCount = 0 + let fetchedCount = 0 let failedCount = 0 - const statusUpdates: Promise[] = [] - results.forEach((result, index) => { + outcomes.forEach((outcome, index) => { const server = servers[index] - if (result.status === 'fulfilled') { - allTools.push(...result.value.tools) - statusUpdates.push( - this.updateServerStatus( - server.id!, - workspaceId, - true, - undefined, - result.value.tools.length - ) + if (outcome.kind === 'cached') { + cachedCount++ + allTools.push(...outcome.tools) + return + } + if (outcome.kind === 'fetched') { + fetchedCount++ + allTools.push(...outcome.tools) + deferredSideEffects.push( + this.updateServerStatus(server.id, workspaceId, true, undefined, outcome.tools.length) + ) + cacheWrites.push( + this.cacheAdapter + .set(serverCacheKey(workspaceId, server.id), outcome.tools, this.cacheTimeout) + .catch((err) => + logger.warn(`[${requestId}] Cache write failed for ${server.name}:`, err) + ) ) - } else if ( - result.reason instanceof McpOauthAuthorizationRequiredError || - result.reason instanceof UnauthorizedError - ) { - // Force 'disconnected' so the settings UI surfaces the re-auth button - // instead of a stale 'connected' state when refresh has expired. + liveConnections.push({ + resolvedConfig: outcome.resolvedConfig, + resolvedIP: outcome.resolvedIP, + }) + return + } + if (outcome.kind === 'oauth-pending') { + // Mark disconnected so the UI surfaces the re-auth button. logger.info(`[${requestId}] Skipping server ${server.name}: OAuth authorization pending`) - statusUpdates.push( + deferredSideEffects.push( db .update(mcpServers) .set({ @@ -454,55 +501,44 @@ class McpService { lastError: null, updatedAt: new Date(), }) - .where(eq(mcpServers.id, server.id!)) + .where(eq(mcpServers.id, server.id)) .then(() => undefined) .catch((err) => { logger.warn(`[${requestId}] Failed to mark server ${server.id} disconnected:`, err) }) ) - } else { - failedCount++ - const errorMessage = getErrorMessage(result.reason, 'Unknown error') - logger.warn(`[${requestId}] Failed to discover tools from server ${server.name}:`) - statusUpdates.push(this.updateServerStatus(server.id!, workspaceId, false, errorMessage)) + return } + failedCount++ + logger.warn( + `[${requestId}] Failed to discover tools from server ${server.name}: ${outcome.message}` + ) + deferredSideEffects.push( + this.updateServerStatus(server.id, workspaceId, false, outcome.message) + ) }) - Promise.allSettled(statusUpdates).catch((err) => { - logger.error(`[${requestId}] Error updating server statuses:`, err) + // Await cache writes so a follow-up discoverTools sees consistent state. + await Promise.allSettled(cacheWrites) + Promise.allSettled(deferredSideEffects).catch((err) => { + logger.error(`[${requestId}] Error in deferred discovery work:`, err) }) - // Fire-and-forget persistent connections for servers that support listChanged if (mcpConnectionManager) { - for (const [index, result] of results.entries()) { - if (result.status === 'fulfilled') { - const { resolvedConfig, resolvedIP } = result.value - mcpConnectionManager - .connect(resolvedConfig, userId, workspaceId, resolvedIP) - .catch((err) => { - logger.warn( - `[${requestId}] Persistent connection failed for ${servers[index].name}:`, - err - ) - }) - } + for (const conn of liveConnections) { + mcpConnectionManager + .connect(conn.resolvedConfig, userId, workspaceId, conn.resolvedIP) + .catch((err) => { + logger.warn( + `[${requestId}] Persistent connection failed for ${conn.resolvedConfig.name}:`, + err + ) + }) } } - if (failedCount === 0) { - try { - await this.cacheAdapter.set(cacheKey, allTools, this.cacheTimeout) - } catch (error) { - logger.warn(`[${requestId}] Cache write failed:`, error) - } - } else { - logger.warn( - `[${requestId}] Skipping cache due to ${failedCount} failed server(s) - will retry on next request` - ) - } - logger.info( - `[${requestId}] Discovered ${allTools.length} tools from ${servers.length - failedCount}/${servers.length} servers` + `[${requestId}] Discovered ${allTools.length} tools from ${servers.length} servers (cached=${cachedCount} fetched=${fetchedCount} failed=${failedCount})` ) return allTools } catch (error) { @@ -635,14 +671,18 @@ class McpService { } /** - * Clear tool cache for a workspace or all workspaces + * Clear tool cache for a workspace or all workspaces. */ async clearCache(workspaceId?: string): Promise { try { if (workspaceId) { - const workspaceCacheKey = `workspace:${workspaceId}` - await this.cacheAdapter.delete(workspaceCacheKey) - logger.debug(`Cleared MCP tool cache for workspace ${workspaceId}`) + const servers = await this.getWorkspaceServers(workspaceId) + await Promise.allSettled( + servers.map((s) => this.cacheAdapter.delete(serverCacheKey(workspaceId, s.id))) + ) + logger.debug( + `Cleared MCP tool cache for workspace ${workspaceId} (${servers.length} servers)` + ) } else { await this.cacheAdapter.clear() logger.debug('Cleared all MCP tool cache') From ccfa60f27ef947b441cc93100384eaeb7d927bd2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:36:45 -0700 Subject: [PATCH 3/8] refactor(mcp): use SDK's typed OAuthError subclasses for error surfacing --- apps/sim/app/api/mcp/oauth/start/route.ts | 39 +++++++++++++---------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 0d4851fbce4..9f0abe8ffba 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -1,4 +1,5 @@ import { auth as mcpAuth } from '@modelcontextprotocol/sdk/client/auth.js' +import { OAuthError, ServerError } from '@modelcontextprotocol/sdk/server/auth/errors.js' import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' @@ -26,23 +27,29 @@ const OAUTH_START_TTL_MS = 10 * 60 * 1000 const MAX_SURFACED_ERROR_LENGTH = 250 function surfaceOauthError(error: unknown): string { - const raw = error instanceof Error ? error.message : String(error) - const rawBodyMatch = raw.match(/Raw body:\s*(\{[\s\S]*\})\s*$/) - if (rawBodyMatch) { - try { - const body = JSON.parse(rawBodyMatch[1]) as Record - const vendorMessage = - typeof body.error_description === 'string' - ? body.error_description - : typeof body.message === 'string' - ? body.message - : typeof body.error === 'string' - ? body.error - : null - if (vendorMessage) return truncate(`Authorization server: ${vendorMessage}`) - } catch {} + // Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields. + if (error instanceof OAuthError && !(error instanceof ServerError)) { + return truncate(`${error.errorCode}: ${error.message}`) + } + + // ServerError wraps non-spec response bodies as "HTTP N: Invalid OAuth error + // response: ... Raw body: {...}". Dig the vendor message out of the JSON tail. + if (error instanceof Error) { + const rawBodyMatch = error.message.match(/Raw body:\s*(\{[\s\S]*\})\s*$/) + if (rawBodyMatch) { + try { + const body = JSON.parse(rawBodyMatch[1]) as Record + const vendorMessage = + (typeof body.error_description === 'string' && body.error_description) || + (typeof body.message === 'string' && body.message) || + (typeof body.error === 'string' && body.error) || + null + if (vendorMessage) return truncate(`Authorization server: ${vendorMessage}`) + } catch {} + } + return truncate(error.message.split('\n')[0] || 'Failed to start OAuth flow') } - return truncate(raw.split('\n')[0] || 'Failed to start OAuth flow') + return 'Failed to start OAuth flow' } function truncate(message: string): string { From f26895adfd6db180191603b0d677f2a1ade4b19d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:40:20 -0700 Subject: [PATCH 4/8] test(mcp): unit-test surfaceOauthError typed and fallback paths --- .../sim/app/api/mcp/oauth/start/route.test.ts | 46 ++++++++++++++++++- apps/sim/app/api/mcp/oauth/start/route.ts | 2 +- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index 7c81138ca42..c25e366c4a6 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -35,7 +35,7 @@ vi.mock('@/lib/auth/hybrid', () => hybridAuthMock) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) vi.mock('@/lib/mcp/oauth', () => mcpOauthMock) -import { GET } from './route' +import { GET, surfaceOauthError } from './route' describe('MCP OAuth start route', () => { beforeEach(() => { @@ -135,3 +135,47 @@ describe('MCP OAuth start route', () => { expect(mockMcpAuth).not.toHaveBeenCalled() }) }) + +describe('surfaceOauthError', () => { + it('uses typed OAuthError errorCode and message for spec-compliant errors', async () => { + const { InvalidGrantError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js') + const err = new InvalidGrantError('Refresh token expired') + expect(surfaceOauthError(err)).toBe('invalid_grant: Refresh token expired') + }) + + it('parses Raw body envelope for ServerError fallbacks (non-spec vendors)', async () => { + const { ServerError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js') + const err = new ServerError( + 'HTTP 400: Invalid OAuth error response: zod error. Raw body: {"code":400,"message":"redirect URI https://example.com/cb is not allowed","retryable":false}' + ) + expect(surfaceOauthError(err)).toBe( + 'Authorization server: redirect URI https://example.com/cb is not allowed' + ) + }) + + it('prefers error_description over message over error in fallback envelope', async () => { + const { ServerError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js') + const err = new ServerError( + 'HTTP 400: Invalid OAuth error response: zod. Raw body: {"error":"invalid_grant","error_description":"the description","message":"the message"}' + ) + expect(surfaceOauthError(err)).toBe('Authorization server: the description') + }) + + it('returns first line of generic errors', () => { + const err = new Error('Network blip\n at fetch (...)') + expect(surfaceOauthError(err)).toBe('Network blip') + }) + + it('truncates messages longer than 250 chars with ellipsis', async () => { + const { InvalidGrantError } = await import('@modelcontextprotocol/sdk/server/auth/errors.js') + const longMessage = 'x'.repeat(300) + const result = surfaceOauthError(new InvalidGrantError(longMessage)) + expect(result.endsWith('…')).toBe(true) + expect(result.length).toBe(251) // 250 chars + ellipsis + }) + + it('returns generic fallback for non-Error values', () => { + expect(surfaceOauthError(null)).toBe('Failed to start OAuth flow') + expect(surfaceOauthError(undefined)).toBe('Failed to start OAuth flow') + }) +}) diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 9f0abe8ffba..986e3e1b802 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -26,7 +26,7 @@ const logger = createLogger('McpOauthStartAPI') const OAUTH_START_TTL_MS = 10 * 60 * 1000 const MAX_SURFACED_ERROR_LENGTH = 250 -function surfaceOauthError(error: unknown): string { +export function surfaceOauthError(error: unknown): string { // Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields. if (error instanceof OAuthError && !(error instanceof ServerError)) { return truncate(`${error.errorCode}: ${error.message}`) From 4ffc7ee136f0aca60c6e912e08fcd86422a1a9b2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:41:52 -0700 Subject: [PATCH 5/8] chore(mcp): remove unused __where mock export in service.test.ts --- apps/sim/lib/mcp/service.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/sim/lib/mcp/service.test.ts b/apps/sim/lib/mcp/service.test.ts index 23e57ed9d14..539ee384579 100644 --- a/apps/sim/lib/mcp/service.test.ts +++ b/apps/sim/lib/mcp/service.test.ts @@ -38,7 +38,6 @@ const { }) vi.mock('@sim/db', () => { - const where = vi.fn() const setter = vi.fn().mockReturnValue({ where: vi.fn().mockResolvedValue(undefined) }) return { db: { @@ -51,7 +50,6 @@ vi.mock('@sim/db', () => { insert: vi.fn(), delete: vi.fn(), }, - __where: where, } }) From f155e3cc5260c1ba666fe8c416b4c6eb5cc2cca7 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:45:07 -0700 Subject: [PATCH 6/8] chore(mcp): drop redundant clearCache TSDoc and stray trailing comment --- apps/sim/app/api/mcp/oauth/start/route.test.ts | 2 +- apps/sim/lib/mcp/service.ts | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index c25e366c4a6..c2d988cf21e 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -171,7 +171,7 @@ describe('surfaceOauthError', () => { const longMessage = 'x'.repeat(300) const result = surfaceOauthError(new InvalidGrantError(longMessage)) expect(result.endsWith('…')).toBe(true) - expect(result.length).toBe(251) // 250 chars + ellipsis + expect(result.length).toBe(251) }) it('returns generic fallback for non-Error values', () => { diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 01b12123635..52336dc3cd5 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -670,9 +670,6 @@ class McpService { } } - /** - * Clear tool cache for a workspace or all workspaces. - */ async clearCache(workspaceId?: string): Promise { try { if (workspaceId) { From af2c93c8f9b277630415c708a32d49cfbd724efa Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:56:38 -0700 Subject: [PATCH 7/8] fix(mcp): don't leak non-OAuth errors; clearCache covers disabled servers --- apps/sim/app/api/mcp/oauth/start/route.test.ts | 17 +++++++++++++++++ apps/sim/app/api/mcp/oauth/start/route.ts | 6 +++++- apps/sim/lib/mcp/service.ts | 14 +++++++++----- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/apps/sim/app/api/mcp/oauth/start/route.test.ts b/apps/sim/app/api/mcp/oauth/start/route.test.ts index c2d988cf21e..fa8ebf26dc7 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -134,6 +134,23 @@ describe('MCP OAuth start route', () => { expect(body.error).toBe('OAuth authorization already in progress for this server') expect(mockMcpAuth).not.toHaveBeenCalled() }) + + it('does not leak non-OAuth internal error details to the client', async () => { + mcpOauthMockFns.mockGetOrCreateOauthRow.mockRejectedValueOnce( + new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal-db-host)') + ) + 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(500) + expect(body.error).toBe('Failed to start OAuth flow') + expect(body.error).not.toContain('ECONNREFUSED') + expect(body.error).not.toContain('internal-db-host') + }) }) describe('surfaceOauthError', () => { diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 986e3e1b802..c7619b9d555 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -150,7 +150,11 @@ export const GET = withRouteHandler( } } catch (error) { logger.error('Error starting MCP OAuth flow:', error) - return createMcpErrorResponse(toError(error), surfaceOauthError(error), 500) + // 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) } }) ) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 52336dc3cd5..052790cdeb5 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -673,13 +673,17 @@ class McpService { async clearCache(workspaceId?: string): Promise { try { if (workspaceId) { - const servers = await this.getWorkspaceServers(workspaceId) + // Enumerate without enabled/deletedAt filters so disabled or soft-deleted + // servers still get their cache keys dropped (hard-deleted rows are gone + // from the table and their keys expire naturally via TTL). + const rows = await db + .select({ id: mcpServers.id }) + .from(mcpServers) + .where(eq(mcpServers.workspaceId, workspaceId)) await Promise.allSettled( - servers.map((s) => this.cacheAdapter.delete(serverCacheKey(workspaceId, s.id))) - ) - logger.debug( - `Cleared MCP tool cache for workspace ${workspaceId} (${servers.length} servers)` + rows.map((r) => this.cacheAdapter.delete(serverCacheKey(workspaceId, r.id))) ) + logger.debug(`Cleared MCP tool cache for workspace ${workspaceId} (${rows.length} servers)`) } else { await this.cacheAdapter.clear() logger.debug('Cleared all MCP tool cache') From e7eb5f27be4bdfea051b4b0e0deb6283fdf2bae2 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 20 May 2026 20:57:29 -0700 Subject: [PATCH 8/8] chore(mcp): tighten clearCache comment to one block --- apps/sim/lib/mcp/service.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/sim/lib/mcp/service.ts b/apps/sim/lib/mcp/service.ts index 052790cdeb5..f3ca6683ae1 100644 --- a/apps/sim/lib/mcp/service.ts +++ b/apps/sim/lib/mcp/service.ts @@ -673,9 +673,9 @@ class McpService { async clearCache(workspaceId?: string): Promise { try { if (workspaceId) { - // Enumerate without enabled/deletedAt filters so disabled or soft-deleted - // servers still get their cache keys dropped (hard-deleted rows are gone - // from the table and their keys expire naturally via TTL). + // No enabled/deletedAt filter so disabled and soft-deleted rows are + // cleared too. Hard-deleted rows are gone from the table; their keys + // expire via TTL. const rows = await db .select({ id: mcpServers.id }) .from(mcpServers)