Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions apps/sim/app/api/mcp/oauth/start/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,23 @@ describe('MCP OAuth start route', () => {
expect(body.error).not.toContain('ECONNREFUSED')
expect(body.error).not.toContain('internal-db-host')
})

it('returns an actionable 4xx when the server does not support dynamic client registration', async () => {
mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValueOnce(
new Error('Incompatible auth server: does not support dynamic client registration')
)
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(422)
expect(body.error).toBe(
"This server doesn't support automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or configure a token instead."
)
})
})

describe('surfaceOauthError', () => {
Expand Down
13 changes: 12 additions & 1 deletion apps/sim/app/api/mcp/oauth/start/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { OAuthError, ServerError } from '@modelcontextprotocol/sdk/server/auth/e
import { db } from '@sim/db'
import { mcpServers } from '@sim/db/schema'
import { createLogger } from '@sim/logger'
import { toError } from '@sim/utils/errors'
import { getErrorMessage, toError } from '@sim/utils/errors'
import { and, eq, isNull } from 'drizzle-orm'
import type { NextRequest } from 'next/server'
import { NextResponse } from 'next/server'
Expand All @@ -25,6 +25,14 @@ import { createMcpErrorResponse } from '@/lib/mcp/utils'
const logger = createLogger('McpOauthStartAPI')
const OAUTH_START_TTL_MS = 10 * 60 * 1000
const MAX_SURFACED_ERROR_LENGTH = 250
const DCR_UNSUPPORTED_MESSAGE =
"This server doesn't support automatic OAuth client registration. Add a pre-registered OAuth client ID and secret, or configure a token instead."

function isDynamicClientRegistrationUnsupported(error: unknown): boolean {
return getErrorMessage(error, '')
.toLowerCase()
.includes('does not support dynamic client registration')
}

export function surfaceOauthError(error: unknown): string {
// Spec-compliant OAuth servers throw typed subclasses with clean RFC 6749 fields.
Expand Down Expand Up @@ -148,6 +156,9 @@ export const GET = withRouteHandler(
authorizationUrl: e.authorizationUrl,
})
}
if (isDynamicClientRegistrationUnsupported(e)) {
return createMcpErrorResponse(toError(e), DCR_UNSUPPORTED_MESSAGE, 422)
}
throw e
}
} catch (error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,12 @@ function ServerListItem({
onViewDetails,
}: ServerListItemProps) {
const transportLabel = formatTransportLabel(server.transport || 'http')
const toolsLabel = getServerToolsLabel(tools, server.connectionStatus, server.lastError)
const toolsLabel = getServerToolsLabel(
tools,
server.connectionStatus,
server.lastError,
server.authType
)
const hasConnectionIssue =
server.connectionStatus === 'error' || server.connectionStatus === 'disconnected'

Expand Down Expand Up @@ -380,6 +385,8 @@ export function MCP() {
const refreshAction = getRefreshActionState({
mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle',
connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined,
authType: server.authType,
error: isCurrentRefresh ? refreshServerMutation.data?.error : undefined,
workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined,
})

Expand Down Expand Up @@ -427,7 +434,12 @@ export function MCP() {
<div className='flex flex-col gap-2'>
<span className='text-[var(--text-muted)] text-caption'>Status</span>
<p className='text-[var(--text-error)] text-sm'>
{getServerToolsLabel([], server.connectionStatus, server.lastError)}
{getServerToolsLabel(
[],
server.connectionStatus,
server.lastError,
server.authType
)}
</p>
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,37 @@ describe('getRefreshActionState', () => {
})
})

it('shows OAuth authorization required when an OAuth refresh finishes disconnected', () => {
expect(
getRefreshActionState({
mutationStatus: 'success',
connectionStatus: 'disconnected',
authType: 'oauth',
workflowsUpdated: 0,
})
).toEqual({
text: 'OAuth authorization required',
textTone: 'error',
disabled: false,
})
})

it('keeps Failed when a disconnected OAuth refresh has a concrete error', () => {
expect(
getRefreshActionState({
mutationStatus: 'success',
connectionStatus: 'disconnected',
authType: 'oauth',
error: 'The MCP server took too long to respond and timed out',
workflowsUpdated: 0,
})
).toEqual({
text: 'Failed',
textTone: 'error',
disabled: false,
})
})

it('preserves successful refresh feedback', () => {
expect(
getRefreshActionState({
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import type { MutationStatus } from '@tanstack/react-query'
import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
import type { McpServer, RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'

interface RefreshActionStateInput {
mutationStatus: MutationStatus
connectionStatus?: RefreshMcpServerResult['status']
authType?: McpServer['authType']
error?: RefreshMcpServerResult['error']
workflowsUpdated?: number
}

Expand All @@ -13,12 +15,23 @@ type RefreshActionState = Pick<SettingsAction, 'text' | 'textTone' | 'disabled'>
export function getRefreshActionState({
mutationStatus,
connectionStatus,
authType,
error,
workflowsUpdated,
}: RefreshActionStateInput): RefreshActionState {
if (mutationStatus === 'pending') {
return { text: 'Refreshing...', textTone: undefined, disabled: true }
}

if (
mutationStatus === 'success' &&
connectionStatus === 'disconnected' &&
authType === 'oauth' &&
!error?.trim()
) {
return { text: 'OAuth authorization required', textTone: 'error', disabled: false }
}

if (
mutationStatus === 'error' ||
(mutationStatus === 'success' && connectionStatus !== 'connected')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,20 @@ describe('getServerToolsLabel', () => {
expect(getServerToolsLabel([], 'error', null)).toBe('Unable to connect')
})

it('shows a disconnected state when OAuth was not completed', () => {
expect(getServerToolsLabel([], 'disconnected', null)).toBe('Not Connected')
it('shows that OAuth authorization is required when OAuth was not completed', () => {
expect(getServerToolsLabel([], 'disconnected', null, 'oauth')).toBe(
'OAuth authorization required'
)
})

it('keeps the generic disconnected state for non-OAuth servers', () => {
expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not Connected')
})

it('shows the persisted error for disconnected connections', () => {
expect(getServerToolsLabel([], 'disconnected', 'Request timed out')).toBe('Request timed out')
expect(getServerToolsLabel([], 'disconnected', 'Request timed out', 'oauth')).toBe(
'Request timed out'
)
})

it('continues showing discovered tools for healthy connections', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ interface NamedTool {
export function getServerToolsLabel(
tools: NamedTool[],
connectionStatus?: McpServer['connectionStatus'],
lastError?: McpServer['lastError']
lastError?: McpServer['lastError'],
authType?: McpServer['authType']
): string {
if (connectionStatus === 'error') {
return lastError?.trim() || 'Unable to connect'
}

if (connectionStatus === 'disconnected') {
return lastError?.trim() || 'Not Connected'
return (
lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not Connected')
)
}

const count = tools.length
Expand Down
31 changes: 28 additions & 3 deletions apps/sim/lib/core/security/input-validation.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,9 +433,34 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction {
*
* The `Agent` is captured for the lifetime of the returned function, so repeated
* calls (e.g. a provider tool loop) reuse its keep-alive connections.
*
* `allowH2` opts the pinned Agent into HTTP/2 (ALPN-negotiated, h1.1 fallback).
* It defaults to `false` to leave existing consumers unchanged. Enabling it does
* not weaken pinning: the pinned `connect.lookup` forces every connection on the
* Agent to `resolvedIP` regardless of authority, so h2 connection coalescing can
* never reach an address other than the validated one.
*/
export function createPinnedFetch(
resolvedIP: string,
options?: { allowH2?: boolean }
): typeof fetch {
return createPinnedFetchWithDispatcher(resolvedIP, options).fetch
}

/**
* Same as {@link createPinnedFetch} but also returns the underlying `Agent` so a
* caller with a defined connection lifetime (e.g. a long-lived MCP transport) can
* tear the Agent down on close instead of waiting for its idle timeout. Closing
* the Agent is what releases any pooled keep-alive / HTTP/2 sockets it holds.
*/
export function createPinnedFetch(resolvedIP: string): typeof fetch {
const dispatcher = new Agent({ connect: { lookup: createPinnedLookup(resolvedIP) } })
export function createPinnedFetchWithDispatcher(
resolvedIP: string,
options?: { allowH2?: boolean }
): { fetch: typeof fetch; dispatcher: Agent } {
const dispatcher = new Agent({
allowH2: options?.allowH2 ?? false,
connect: { lookup: createPinnedLookup(resolvedIP) },
})

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

return pinned
return { fetch: pinned, dispatcher }
}

/**
Expand Down
12 changes: 12 additions & 0 deletions apps/sim/lib/core/security/pinned-fetch.server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ describe('createPinnedFetch', () => {
expect(resolved).toEqual({ address: '203.0.113.10', family: 4 })
})

it('defaults allowH2 to false so existing consumers are unchanged', () => {
createPinnedFetch('203.0.113.10')
const opts = capturedAgentOptions[0] as { allowH2?: boolean }
expect(opts.allowH2).toBe(false)
})

it('opts the Agent into HTTP/2 when allowH2 is requested', () => {
createPinnedFetch('203.0.113.10', { allowH2: true })
const opts = capturedAgentOptions[0] as { allowH2?: boolean }
expect(opts.allowH2).toBe(true)
})

it('uses IPv6 family when the validated IP is IPv6', async () => {
createPinnedFetch('2606:4700:4700::1111')
const { connect } = capturedAgentOptions[0] as { connect: { lookup: PinnedLookup } }
Expand Down
Loading
Loading