Skip to content

Commit 32de415

Browse files
authored
fix(mcp): auth-type handling, OAuth-pending UX, safe diagnostics + HTTP/2 (#5757)
* improvement(mcp): negotiate HTTP/2 for MCP transport MCP servers are commonly behind HTTP/2 fronts (CDNs, cloud LBs), but the SSRF-pinned undici Agent is HTTP/1.1-only unless it opts into h2 via ALPN. Add an allowH2 option to createPinnedFetch (default false, so LLM-provider callers are unchanged) and centralize the MCP h2 decision in one createPinnedMcpFetch routed through the transport, OAuth probe, and SSRF-guarded fetch. Pinning is unaffected: the pinned lookup forces every connection to the resolved IP. * 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. * fix(mcp): reset connection on any auth-type flip, scope h2 to live transport * fix(mcp): close pinned h2 Agent on disconnect and revoke OAuth tokens on auth-type change * fix(mcp): release pinned Agent on failed connect and point DCR error at client-id/secret setup * fix(mcp): make pinned Agent teardown idempotent to avoid double-destroy on cancel/failed connect
1 parent 369eef7 commit 32de415

17 files changed

Lines changed: 548 additions & 41 deletions

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 automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or 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 automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or 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/core/security/input-validation.server.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -433,9 +433,34 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction {
433433
*
434434
* The `Agent` is captured for the lifetime of the returned function, so repeated
435435
* calls (e.g. a provider tool loop) reuse its keep-alive connections.
436+
*
437+
* `allowH2` opts the pinned Agent into HTTP/2 (ALPN-negotiated, h1.1 fallback).
438+
* It defaults to `false` to leave existing consumers unchanged. Enabling it does
439+
* not weaken pinning: the pinned `connect.lookup` forces every connection on the
440+
* Agent to `resolvedIP` regardless of authority, so h2 connection coalescing can
441+
* never reach an address other than the validated one.
442+
*/
443+
export function createPinnedFetch(
444+
resolvedIP: string,
445+
options?: { allowH2?: boolean }
446+
): typeof fetch {
447+
return createPinnedFetchWithDispatcher(resolvedIP, options).fetch
448+
}
449+
450+
/**
451+
* Same as {@link createPinnedFetch} but also returns the underlying `Agent` so a
452+
* caller with a defined connection lifetime (e.g. a long-lived MCP transport) can
453+
* tear the Agent down on close instead of waiting for its idle timeout. Closing
454+
* the Agent is what releases any pooled keep-alive / HTTP/2 sockets it holds.
436455
*/
437-
export function createPinnedFetch(resolvedIP: string): typeof fetch {
438-
const dispatcher = new Agent({ connect: { lookup: createPinnedLookup(resolvedIP) } })
456+
export function createPinnedFetchWithDispatcher(
457+
resolvedIP: string,
458+
options?: { allowH2?: boolean }
459+
): { fetch: typeof fetch; dispatcher: Agent } {
460+
const dispatcher = new Agent({
461+
allowH2: options?.allowH2 ?? false,
462+
connect: { lookup: createPinnedLookup(resolvedIP) },
463+
})
439464

440465
const pinned = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => {
441466
// double-cast-allowed: DOM RequestInfo/URL and undici fetch input types differ but are structurally compatible at runtime (Node's global fetch IS undici)
@@ -447,7 +472,7 @@ export function createPinnedFetch(resolvedIP: string): typeof fetch {
447472
return response as unknown as Response
448473
}
449474

450-
return pinned
475+
return { fetch: pinned, dispatcher }
451476
}
452477

453478
/**

apps/sim/lib/core/security/pinned-fetch.server.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,18 @@ describe('createPinnedFetch', () => {
5555
expect(resolved).toEqual({ address: '203.0.113.10', family: 4 })
5656
})
5757

58+
it('defaults allowH2 to false so existing consumers are unchanged', () => {
59+
createPinnedFetch('203.0.113.10')
60+
const opts = capturedAgentOptions[0] as { allowH2?: boolean }
61+
expect(opts.allowH2).toBe(false)
62+
})
63+
64+
it('opts the Agent into HTTP/2 when allowH2 is requested', () => {
65+
createPinnedFetch('203.0.113.10', { allowH2: true })
66+
const opts = capturedAgentOptions[0] as { allowH2?: boolean }
67+
expect(opts.allowH2).toBe(true)
68+
})
69+
5870
it('uses IPv6 family when the validated IP is IPv6', async () => {
5971
createPinnedFetch('2606:4700:4700::1111')
6072
const { connect } = capturedAgentOptions[0] as { connect: { lookup: PinnedLookup } }

0 commit comments

Comments
 (0)