Skip to content

Commit eea4aa1

Browse files
committed
fix(mcp): correct auth-type handling, OAuth-pending UX, and safe error logging
- Classify a non-OAuth UnauthorizedError as an auth failure instead of an OAuth redirect, so static-bearer servers that merely advertise OAuth stop being diverted into the OAuth flow (fixes the class of server that couldn't connect). - Reset a server to disconnected when it is switched to OAuth, so it can't falsely read as connected before completing its auth flow. - Surface OAuth-pending state as 'OAuth authorization required' in the server list and refresh action instead of a generic 'Not Connected'. - Return an actionable 422 for OAuth servers that don't support dynamic client registration. - Redact error messages/cause/session-ids from MCP transport/connect logs via a shared getMcpSafeErrorDiagnostics helper.
1 parent 313015e commit eea4aa1

13 files changed

Lines changed: 354 additions & 28 deletions

File tree

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,23 @@ describe('MCP OAuth start route', () => {
162162
expect(body.error).not.toContain('ECONNREFUSED')
163163
expect(body.error).not.toContain('internal-db-host')
164164
})
165+
166+
it('returns an actionable 4xx when the server does not support dynamic client registration', async () => {
167+
mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(
168+
new Error('Incompatible auth server: does not support dynamic client registration')
169+
)
170+
const request = new NextRequest(
171+
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
172+
)
173+
174+
const response = await GET(request)
175+
const body = await response.json()
176+
177+
expect(response.status).toBe(422)
178+
expect(body.error).toBe(
179+
"This server doesn't support OAuth client registration. Configure a token instead."
180+
)
181+
})
165182
})
166183

167184
describe('surfaceOauthError', () => {

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { OAuthError, ServerError } from '@modelcontextprotocol/sdk/server/auth/e
22
import { db } from '@sim/db'
33
import { mcpServers } from '@sim/db/schema'
44
import { createLogger } from '@sim/logger'
5-
import { toError } from '@sim/utils/errors'
5+
import { getErrorMessage, toError } from '@sim/utils/errors'
66
import { and, eq, isNull } from 'drizzle-orm'
77
import type { NextRequest } from 'next/server'
88
import { NextResponse } from 'next/server'
@@ -25,6 +25,14 @@ import { createMcpErrorResponse } from '@/lib/mcp/utils'
2525
const logger = createLogger('McpOauthStartAPI')
2626
const OAUTH_START_TTL_MS = 10 * 60 * 1000
2727
const MAX_SURFACED_ERROR_LENGTH = 250
28+
const DCR_UNSUPPORTED_MESSAGE =
29+
"This server doesn't support OAuth client registration. Configure a token instead."
30+
31+
function isDynamicClientRegistrationUnsupported(error: unknown): boolean {
32+
return getErrorMessage(error, '')
33+
.toLowerCase()
34+
.includes('does not support dynamic client registration')
35+
}
2836

2937
export function surfaceOauthError(error: unknown): string {
3038
// Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields.
@@ -148,6 +156,9 @@ export const GET = withRouteHandler(
148156
authorizationUrl: e.authorizationUrl,
149157
})
150158
}
159+
if (isDynamicClientRegistrationUnsupported(e)) {
160+
return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
161+
}
151162
throw e
152163
}
153164
} catch (error) {

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,12 @@ function ServerListItem({
8585
onViewDetails,
8686
}: ServerListItemProps) {
8787
const transportLabel = formatTransportLabel(server.transport || 'http')
88-
const toolsLabel = getServerToolsLabel(tools, server.connectionStatus, server.lastError)
88+
const toolsLabel = getServerToolsLabel(
89+
tools,
90+
server.connectionStatus,
91+
server.lastError,
92+
server.authType
93+
)
8994
const hasConnectionIssue =
9095
server.connectionStatus === 'error' || server.connectionStatus === 'disconnected'
9196

@@ -380,6 +385,8 @@ export function MCP() {
380385
const refreshAction = getRefreshActionState({
381386
mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle',
382387
connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined,
388+
authType: server.authType,
389+
error: isCurrentRefresh ? refreshServerMutation.data?.error : undefined,
383390
workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined,
384391
})
385392

@@ -427,7 +434,12 @@ export function MCP() {
427434
<div className='flex flex-col gap-2'>
428435
<span className='text-[var(--text-muted)] text-caption'>Status</span>
429436
<p className='text-[var(--text-error)] text-sm'>
430-
{getServerToolsLabel([], server.connectionStatus, server.lastError)}
437+
{getServerToolsLabel(
438+
[],
439+
server.connectionStatus,
440+
server.lastError,
441+
server.authType
442+
)}
431443
</p>
432444
</div>
433445
)}

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,37 @@ describe('getRefreshActionState', () => {
3131
})
3232
})
3333

34+
it('shows OAuth authorization required when an OAuth refresh finishes disconnected', () => {
35+
expect(
36+
getRefreshActionState({
37+
mutationStatus: 'success',
38+
connectionStatus: 'disconnected',
39+
authType: 'oauth',
40+
workflowsUpdated: 0,
41+
})
42+
).toEqual({
43+
text: 'OAuth authorization required',
44+
textTone: 'error',
45+
disabled: false,
46+
})
47+
})
48+
49+
it('keeps Failed when a disconnected OAuth refresh has a concrete error', () => {
50+
expect(
51+
getRefreshActionState({
52+
mutationStatus: 'success',
53+
connectionStatus: 'disconnected',
54+
authType: 'oauth',
55+
error: 'The MCP server took too long to respond and timed out',
56+
workflowsUpdated: 0,
57+
})
58+
).toEqual({
59+
text: 'Failed',
60+
textTone: 'error',
61+
disabled: false,
62+
})
63+
})
64+
3465
it('preserves successful refresh feedback', () => {
3566
expect(
3667
getRefreshActionState({

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
import type { MutationStatus } from '@tanstack/react-query'
2-
import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
2+
import type { McpServer, RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
33
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
44

55
interface RefreshActionStateInput {
66
mutationStatus: MutationStatus
77
connectionStatus?: RefreshMcpServerResult['status']
8+
authType?: McpServer['authType']
9+
error?: RefreshMcpServerResult['error']
810
workflowsUpdated?: number
911
}
1012

@@ -13,12 +15,23 @@ type RefreshActionState = Pick<SettingsAction, 'text' | 'textTone' | 'disabled'>
1315
export function getRefreshActionState({
1416
mutationStatus,
1517
connectionStatus,
18+
authType,
19+
error,
1620
workflowsUpdated,
1721
}: RefreshActionStateInput): RefreshActionState {
1822
if (mutationStatus === 'pending') {
1923
return { text: 'Refreshing...', textTone: undefined, disabled: true }
2024
}
2125

26+
if (
27+
mutationStatus === 'success' &&
28+
connectionStatus === 'disconnected' &&
29+
authType === 'oauth' &&
30+
!error?.trim()
31+
) {
32+
return { text: 'OAuth authorization required', textTone: 'error', disabled: false }
33+
}
34+
2235
if (
2336
mutationStatus === 'error' ||
2437
(mutationStatus === 'success' && connectionStatus !== 'connected')

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,20 @@ describe('getServerToolsLabel', () => {
1212
expect(getServerToolsLabel([], 'error', null)).toBe('Unable to connect')
1313
})
1414

15-
it('shows a disconnected state when OAuth was not completed', () => {
16-
expect(getServerToolsLabel([], 'disconnected', null)).toBe('Not Connected')
15+
it('shows that OAuth authorization is required when OAuth was not completed', () => {
16+
expect(getServerToolsLabel([], 'disconnected', null, 'oauth')).toBe(
17+
'OAuth authorization required'
18+
)
19+
})
20+
21+
it('keeps the generic disconnected state for non-OAuth servers', () => {
22+
expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not Connected')
1723
})
1824

1925
it('shows the persisted error for disconnected connections', () => {
20-
expect(getServerToolsLabel([], 'disconnected', 'Request timed out')).toBe('Request timed out')
26+
expect(getServerToolsLabel([], 'disconnected', 'Request timed out', 'oauth')).toBe(
27+
'Request timed out'
28+
)
2129
})
2230

2331
it('continues showing discovered tools for healthy connections', () => {

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,17 @@ interface NamedTool {
77
export function getServerToolsLabel(
88
tools: NamedTool[],
99
connectionStatus?: McpServer['connectionStatus'],
10-
lastError?: McpServer['lastError']
10+
lastError?: McpServer['lastError'],
11+
authType?: McpServer['authType']
1112
): string {
1213
if (connectionStatus === 'error') {
1314
return lastError?.trim() || 'Unable to connect'
1415
}
1516

1617
if (connectionStatus === 'disconnected') {
17-
return lastError?.trim() || 'Not Connected'
18+
return (
19+
lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not Connected')
20+
)
1821
}
1922

2023
const count = tools.length

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'
45
import { beforeEach, describe, expect, it, vi } from 'vitest'
56

67
const { mockLogger, mockSdkConnect, mockSdkListTools } = vi.hoisted(() => ({
@@ -265,6 +266,55 @@ describe('McpClient notification handler', () => {
265266
expect(JSON.stringify(mockLogger.error.mock.calls)).not.toContain(secret)
266267
})
267268

269+
it('does not misclassify rejected static headers as an OAuth authorization flow', async () => {
270+
mockSdkConnect.mockRejectedValueOnce(new UnauthorizedError('Static token rejected'))
271+
const client = new McpClient({
272+
config: {
273+
...createConfig(),
274+
authType: 'headers',
275+
headers: { Authorization: 'Bearer rejected-static-token' },
276+
},
277+
securityPolicy: { requireConsent: false, auditLevel: 'basic' },
278+
})
279+
280+
await expect(client.connect()).rejects.toBeInstanceOf(UnauthorizedError)
281+
282+
expect(mockLogger.error).toHaveBeenCalledWith(
283+
expect.stringContaining('Failed to connect'),
284+
expect.objectContaining({ outcome: 'unauthorized' })
285+
)
286+
expect(client.getStatus().lastError).toBe('Authentication failed')
287+
})
288+
289+
it('logs tools/list failures without echoed credentials or session identifiers', async () => {
290+
const secret = 'opaque-tools-list-credential'
291+
mockSdkListTools.mockRejectedValueOnce(new Error(`Upstream rejected ${secret}`))
292+
const client = new McpClient({
293+
config: {
294+
...createConfig(),
295+
authType: 'headers',
296+
headers: { 'X-Custom-Credential': secret },
297+
},
298+
securityPolicy: { requireConsent: false, auditLevel: 'basic' },
299+
})
300+
301+
await client.connect()
302+
await expect(client.listTools()).rejects.toThrow(secret)
303+
304+
expect(mockLogger.error).toHaveBeenCalledWith(
305+
expect.stringContaining('Failed to list tools'),
306+
expect.objectContaining({
307+
phase: 'tools/list',
308+
serverId: 'server-1',
309+
sessionIdPresent: true,
310+
error: expect.objectContaining({ name: 'Error' }),
311+
})
312+
)
313+
const logged = JSON.stringify(mockLogger.error.mock.calls)
314+
expect(logged).not.toContain(secret)
315+
expect(logged).not.toContain('test-session')
316+
})
317+
268318
it('passes configured headers for OAuth transports as well as header auth transports', () => {
269319
const authProvider = {} as unknown as NonNullable<McpClientOptions['authProvider']>
270320
new McpClient({

apps/sim/lib/mcp/client.ts

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ import {
99
ToolListChangedNotificationSchema,
1010
} from '@modelcontextprotocol/sdk/types.js'
1111
import { createLogger } from '@sim/logger'
12-
import { describeError, getErrorMessage } from '@sim/utils/errors'
12+
import { getErrorMessage } from '@sim/utils/errors'
1313
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
14-
import { createPinnedFetch } from '@/lib/core/security/input-validation.server'
15-
import { sanitizeForLogging } from '@/lib/core/security/redaction'
14+
import { getMcpSafeErrorDiagnostics } from '@/lib/mcp/error-diagnostics'
1615
import { McpOauthRedirectRequired } from '@/lib/mcp/oauth'
16+
import { createPinnedMcpFetch } from '@/lib/mcp/pinned-fetch'
1717
import {
1818
type McpClientOptions,
1919
McpConnectionError,
@@ -43,10 +43,16 @@ type ConnectionOutcome =
4343
| 'cancelled'
4444
| 'error'
4545

46-
function classifyConnectionOutcome(error: unknown): ConnectionOutcome {
47-
if (error instanceof McpOauthRedirectRequired || error instanceof UnauthorizedError) {
46+
function classifyConnectionOutcome(
47+
error: unknown,
48+
authType: McpServerConfig['authType']
49+
): ConnectionOutcome {
50+
if (error instanceof McpOauthRedirectRequired) {
4851
return 'authorization_required'
4952
}
53+
if (error instanceof UnauthorizedError) {
54+
return authType === 'oauth' ? 'authorization_required' : 'unauthorized'
55+
}
5056
const message = getErrorMessage(error, '').toLowerCase()
5157
if (message.includes('connection attempt cancelled')) return 'cancelled'
5258
if (message.includes('timeout') || message.includes('timed out')) return 'timeout'
@@ -92,7 +98,7 @@ export class McpClient {
9298
this.transport = new StreamableHTTPClientTransport(new URL(this.config.url), {
9399
authProvider: useOauth ? this.authProvider : undefined,
94100
requestInit: { headers: this.config.headers },
95-
...(resolvedIP ? { fetch: createPinnedFetch(resolvedIP) } : {}),
101+
...(resolvedIP ? { fetch: createPinnedMcpFetch(resolvedIP) } : {}),
96102
})
97103

98104
this.client = new Client(
@@ -109,7 +115,9 @@ export class McpClient {
109115
this.client.onerror = (error) => {
110116
logger.warn(`MCP transport error for ${this.config.name}`, {
111117
serverId: this.config.id,
112-
error: sanitizeForLogging(getErrorMessage(error, 'Unknown transport error'), 200),
118+
phase: 'transport',
119+
sessionIdPresent: Boolean(this.transport.sessionId),
120+
error: getMcpSafeErrorDiagnostics(error),
113121
})
114122
}
115123
}
@@ -178,25 +186,21 @@ export class McpClient {
178186
} catch (error) {
179187
this.isConnected = false
180188
const errorMessage = getErrorMessage(error, 'Unknown error')
181-
const describedError = describeError(error)
182-
const outcome = classifyConnectionOutcome(error)
189+
const outcome = classifyConnectionOutcome(error, this.config.authType)
183190
logger.error(`Failed to connect to MCP server ${this.config.name}`, {
184191
...diagnostics,
185192
durationMs: Date.now() - startedAt,
186-
error: {
187-
name: sanitizeForLogging(describedError.name, 100),
188-
code: describedError.code ? sanitizeForLogging(describedError.code, 100) : undefined,
189-
errno: describedError.errno ? sanitizeForLogging(describedError.errno, 100) : undefined,
190-
syscall: describedError.syscall
191-
? sanitizeForLogging(describedError.syscall, 100)
192-
: undefined,
193-
},
193+
error: getMcpSafeErrorDiagnostics(error),
194194
outcome,
195195
})
196196
if (outcome === 'authorization_required') {
197197
this.connectionStatus.lastError = undefined
198198
throw error
199199
}
200+
if (error instanceof UnauthorizedError) {
201+
this.connectionStatus.lastError = 'Authentication failed'
202+
throw error
203+
}
200204
this.connectionStatus.lastError = errorMessage
201205
throw new McpConnectionError(errorMessage, this.config.name)
202206
}
@@ -236,6 +240,7 @@ export class McpClient {
236240
MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS
237241
)
238242
const maxTotalTimeoutMs = MCP_CLIENT_CONSTANTS.LIST_TOOLS_MAX_TOTAL_TIMEOUT_MS
243+
const startedAt = Date.now()
239244

240245
try {
241246
const result: ListToolsResult = await this.client.listTools(undefined, {
@@ -265,7 +270,15 @@ export class McpClient {
265270
serverName: this.config.name,
266271
}))
267272
} catch (error) {
268-
logger.error(`Failed to list tools from server ${this.config.name}:`, error)
273+
logger.error(`Failed to list tools from server ${this.config.name}`, {
274+
serverId: this.config.id,
275+
phase: 'tools/list',
276+
durationMs: Date.now() - startedAt,
277+
idleTimeoutMs,
278+
maxTotalTimeoutMs,
279+
sessionIdPresent: Boolean(this.transport.sessionId),
280+
error: getMcpSafeErrorDiagnostics(error),
281+
})
269282
throw error
270283
}
271284
}

0 commit comments

Comments
 (0)