From acb2b2074ca5453c2f7bdb56f574a709ec1f033d Mon Sep 17 00:00:00 2001 From: waleed Date: Sat, 4 Jul 2026 15:23:32 -0700 Subject: [PATCH] fix(mcp): make SSRF-guarded fetch structurally mandatory in OAuth auth() calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #5399 fixed the callback route forgetting to pass fetchFn into the SDK's auth(), but every call site (start + callback routes) still imported the raw SDK auth() directly and had to remember to pass fetchFn: createSsrfGuardedMcpFetch() by hand — the same omission was possible again at any future call site. Add mcpAuthGuarded() in lib/mcp/oauth/auth.ts, a thin wrapper around the SDK's auth() that always defaults fetchFn to the SSRF-guarded fetch (still overridable for tests). Both routes now import mcpAuthGuarded from @/lib/mcp/oauth instead of the raw SDK auth, so omitting the guard is no longer possible by omission. --- .../app/api/mcp/oauth/callback/route.test.ts | 28 ++++------ apps/sim/app/api/mcp/oauth/callback/route.ts | 8 ++- .../sim/app/api/mcp/oauth/start/route.test.ts | 29 ++++------ apps/sim/app/api/mcp/oauth/start/route.ts | 6 +-- apps/sim/lib/mcp/oauth/auth.test.ts | 54 +++++++++++++++++++ apps/sim/lib/mcp/oauth/auth.ts | 19 +++++++ apps/sim/lib/mcp/oauth/index.ts | 1 + packages/testing/src/mocks/mcp-oauth.mock.ts | 2 + 8 files changed, 100 insertions(+), 47 deletions(-) create mode 100644 apps/sim/lib/mcp/oauth/auth.test.ts create mode 100644 apps/sim/lib/mcp/oauth/auth.ts diff --git a/apps/sim/app/api/mcp/oauth/callback/route.test.ts b/apps/sim/app/api/mcp/oauth/callback/route.test.ts index d79c43e4a76..1a1f5bcb8e8 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.test.ts @@ -13,13 +13,9 @@ import { import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockMcpAuth, mockCreateSsrfGuardedMcpFetch, mockGuardedFetch, mockDiscoverServerTools } = - vi.hoisted(() => ({ - mockMcpAuth: vi.fn(), - mockCreateSsrfGuardedMcpFetch: vi.fn(), - mockGuardedFetch: vi.fn(), - mockDiscoverServerTools: vi.fn(), - })) +const { mockDiscoverServerTools } = vi.hoisted(() => ({ + mockDiscoverServerTools: vi.fn(), +})) vi.mock('@sim/db', () => dbChainMock) vi.mock('@sim/db/schema', () => schemaMock) @@ -28,13 +24,7 @@ vi.mock('drizzle-orm', () => ({ eq: vi.fn(), isNull: vi.fn(), })) -vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ - auth: mockMcpAuth, -})) vi.mock('@/lib/mcp/oauth', () => mcpOauthMock) -vi.mock('@/lib/mcp/pinned-fetch', () => ({ - createSsrfGuardedMcpFetch: mockCreateSsrfGuardedMcpFetch, -})) vi.mock('@/lib/mcp/service', () => ({ mcpService: { discoverServerTools: mockDiscoverServerTools }, })) @@ -45,7 +35,6 @@ describe('MCP OAuth callback route', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() - mockCreateSsrfGuardedMcpFetch.mockReturnValue(mockGuardedFetch) authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-1' } }) mcpOauthMockFns.mockLoadOauthRowByState.mockResolvedValue({ id: 'oauth-row-1', @@ -61,24 +50,25 @@ describe('MCP OAuth callback route', () => { }, ]) mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined) - mockMcpAuth.mockResolvedValue('AUTHORIZED') + mcpOauthMockFns.mockMcpAuthGuarded.mockResolvedValue('AUTHORIZED') mockDiscoverServerTools.mockResolvedValue(undefined) }) - it('performs the token exchange through the SSRF-guarded fetch', async () => { + it('performs the token exchange through the SSRF-guarded mcpAuthGuarded wrapper', async () => { const request = new NextRequest( 'http://localhost:3000/api/mcp/oauth/callback?state=state-1&code=auth-code-1' ) await GET(request) - expect(mockCreateSsrfGuardedMcpFetch).toHaveBeenCalledTimes(1) - expect(mockMcpAuth).toHaveBeenCalledWith( + // The route must call the guarded wrapper (which defaults fetchFn to the + // SSRF-guarded fetch internally) rather than the raw SDK `auth()` — see + // apps/sim/lib/mcp/oauth/auth.test.ts for the wrapper's own fetchFn coverage. + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ serverUrl: 'https://mcp.example.com/mcp', authorizationCode: 'auth-code-1', - fetchFn: mockGuardedFetch, }) ) }) diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts index 0acfff85778..5c75416a3bc 100644 --- a/apps/sim/app/api/mcp/oauth/callback/route.ts +++ b/apps/sim/app/api/mcp/oauth/callback/route.ts @@ -1,4 +1,3 @@ -import { auth as mcpAuth } from '@modelcontextprotocol/sdk/client/auth.js' import { db } from '@sim/db' import { mcpServers } from '@sim/db/schema' import { createLogger } from '@sim/logger' @@ -17,9 +16,9 @@ import { loadOauthRowByState, loadPreregisteredClient, type McpOauthCallbackReason, + mcpAuthGuarded, SimMcpOauthProvider, } from '@/lib/mcp/oauth' -import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' import { mcpService } from '@/lib/mcp/service' const logger = createLogger('McpOauthCallbackAPI') @@ -145,12 +144,11 @@ export const GET = withRouteHandler(async (request: NextRequest) => { const preregistered = await loadPreregisteredClient(server.id) const provider = new SimMcpOauthProvider({ row, preregistered }) - let result: Awaited> + let result: Awaited> try { - result = await mcpAuth(provider, { + result = await mcpAuthGuarded(provider, { serverUrl: server.url, authorizationCode: code, - fetchFn: createSsrfGuardedMcpFetch(), }) } catch (e) { logger.error('Token exchange failed during MCP OAuth callback', e) 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 e4a5132a4fa..68dd96b2bf3 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.test.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.test.ts @@ -17,12 +17,6 @@ import { import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockMcpAuth, mockCreateSsrfGuardedMcpFetch, mockGuardedFetch } = vi.hoisted(() => ({ - mockMcpAuth: vi.fn(), - mockCreateSsrfGuardedMcpFetch: vi.fn(), - mockGuardedFetch: vi.fn(), -})) - vi.mock('@sim/db', () => dbChainMock) vi.mock('@sim/db/schema', () => schemaMock) vi.mock('drizzle-orm', () => ({ @@ -30,12 +24,6 @@ vi.mock('drizzle-orm', () => ({ eq: vi.fn(), isNull: vi.fn(), })) -vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ - auth: mockMcpAuth, -})) -vi.mock('@/lib/mcp/pinned-fetch', () => ({ - createSsrfGuardedMcpFetch: mockCreateSsrfGuardedMcpFetch, -})) vi.mock('@/lib/auth/hybrid', () => hybridAuthMock) vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) vi.mock('@/lib/mcp/oauth', () => mcpOauthMock) @@ -77,21 +65,24 @@ describe('MCP OAuth start route', () => { updatedAt: new Date(), }) mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined) - mockMcpAuth.mockRejectedValue(new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize')) - mockCreateSsrfGuardedMcpFetch.mockReturnValue(mockGuardedFetch) + mcpOauthMockFns.mockMcpAuthGuarded.mockRejectedValue( + new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize') + ) }) - it('routes OAuth discovery through the SSRF-guarded fetch', async () => { + it('routes OAuth discovery through the SSRF-guarded mcpAuthGuarded wrapper', async () => { const request = new NextRequest( 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' ) await GET(request) - expect(mockCreateSsrfGuardedMcpFetch).toHaveBeenCalledTimes(1) - expect(mockMcpAuth).toHaveBeenCalledWith( + // The route must call the guarded wrapper (which defaults fetchFn to the + // SSRF-guarded fetch internally) rather than the raw SDK `auth()` — see + // apps/sim/lib/mcp/oauth/auth.test.ts for the wrapper's own fetchFn coverage. + expect(mcpOauthMockFns.mockMcpAuthGuarded).toHaveBeenCalledWith( expect.anything(), - expect.objectContaining({ serverUrl: 'https://mcp.exa.ai/mcp', fetchFn: mockGuardedFetch }) + expect.objectContaining({ serverUrl: 'https://mcp.exa.ai/mcp' }) ) }) @@ -152,7 +143,7 @@ describe('MCP OAuth start route', () => { expect(response.status).toBe(409) expect(body.error).toBe('OAuth authorization already in progress for this server') - expect(mockMcpAuth).not.toHaveBeenCalled() + expect(mcpOauthMockFns.mockMcpAuthGuarded).not.toHaveBeenCalled() }) it('does not leak non-OAuth internal error details to the client', async () => { diff --git a/apps/sim/app/api/mcp/oauth/start/route.ts b/apps/sim/app/api/mcp/oauth/start/route.ts index 3b228bd95c0..edb17a0f1c9 100644 --- a/apps/sim/app/api/mcp/oauth/start/route.ts +++ b/apps/sim/app/api/mcp/oauth/start/route.ts @@ -1,4 +1,3 @@ -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' @@ -17,10 +16,10 @@ import { loadPreregisteredClient, McpOauthInsecureUrlError, McpOauthRedirectRequired, + mcpAuthGuarded, SimMcpOauthProvider, setOauthRowUser, } from '@/lib/mcp/oauth' -import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' import { createMcpErrorResponse } from '@/lib/mcp/utils' const logger = createLogger('McpOauthStartAPI') @@ -130,9 +129,8 @@ export const GET = withRouteHandler( const provider = new SimMcpOauthProvider({ row, preregistered }) try { - const result = await mcpAuth(provider, { + const result = await mcpAuthGuarded(provider, { serverUrl: server.url, - fetchFn: createSsrfGuardedMcpFetch(), }) if (result === 'AUTHORIZED') { return NextResponse.json({ status: 'already_authorized' }) diff --git a/apps/sim/lib/mcp/oauth/auth.test.ts b/apps/sim/lib/mcp/oauth/auth.test.ts new file mode 100644 index 00000000000..8b8df25dfdd --- /dev/null +++ b/apps/sim/lib/mcp/oauth/auth.test.ts @@ -0,0 +1,54 @@ +/** + * @vitest-environment node + */ +import type { OAuthClientProvider } from '@modelcontextprotocol/sdk/client/auth.js' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockAuth, mockCreateSsrfGuardedMcpFetch, mockGuardedFetch } = vi.hoisted(() => ({ + mockAuth: vi.fn(), + mockCreateSsrfGuardedMcpFetch: vi.fn(), + mockGuardedFetch: vi.fn(), +})) + +vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ + auth: mockAuth, +})) +vi.mock('@/lib/mcp/pinned-fetch', () => ({ + createSsrfGuardedMcpFetch: mockCreateSsrfGuardedMcpFetch, +})) + +import { mcpAuthGuarded } from '@/lib/mcp/oauth/auth' + +describe('mcpAuthGuarded', () => { + const provider = {} as OAuthClientProvider + + beforeEach(() => { + vi.clearAllMocks() + mockCreateSsrfGuardedMcpFetch.mockReturnValue(mockGuardedFetch) + mockAuth.mockResolvedValue('AUTHORIZED') + }) + + it('defaults fetchFn to the SSRF-guarded fetch', async () => { + await mcpAuthGuarded(provider, { serverUrl: 'https://mcp.example.com/mcp' }) + + expect(mockCreateSsrfGuardedMcpFetch).toHaveBeenCalledTimes(1) + expect(mockAuth).toHaveBeenCalledWith(provider, { + serverUrl: 'https://mcp.example.com/mcp', + fetchFn: mockGuardedFetch, + }) + }) + + it('lets a caller-supplied fetchFn override the default', async () => { + const overrideFetch = vi.fn() + + await mcpAuthGuarded(provider, { + serverUrl: 'https://mcp.example.com/mcp', + fetchFn: overrideFetch, + }) + + expect(mockAuth).toHaveBeenCalledWith(provider, { + serverUrl: 'https://mcp.example.com/mcp', + fetchFn: overrideFetch, + }) + }) +}) diff --git a/apps/sim/lib/mcp/oauth/auth.ts b/apps/sim/lib/mcp/oauth/auth.ts new file mode 100644 index 00000000000..68d9ff7f1fb --- /dev/null +++ b/apps/sim/lib/mcp/oauth/auth.ts @@ -0,0 +1,19 @@ +import { auth, type OAuthClientProvider } from '@modelcontextprotocol/sdk/client/auth.js' +import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch' + +type McpAuthOptions = Parameters[1] + +/** + * Wraps the MCP SDK's `auth()` and defaults `fetchFn` to the SSRF-guarded + * fetch. Every URL touched during an MCP OAuth exchange — discovery, + * authorization, token, and revocation endpoints — can come from + * attacker-controllable authorization-server metadata, so callers must not + * be able to omit the guard by forgetting to pass `fetchFn` explicitly. + * Pass `fetchFn` in `options` to override (e.g. in tests). + */ +export function mcpAuthGuarded( + provider: OAuthClientProvider, + options: McpAuthOptions +): ReturnType { + return auth(provider, { fetchFn: createSsrfGuardedMcpFetch(), ...options }) +} diff --git a/apps/sim/lib/mcp/oauth/index.ts b/apps/sim/lib/mcp/oauth/index.ts index 41dd96111cc..5237b22fe16 100644 --- a/apps/sim/lib/mcp/oauth/index.ts +++ b/apps/sim/lib/mcp/oauth/index.ts @@ -1,3 +1,4 @@ +export { mcpAuthGuarded } from './auth' export type { McpOauthCallbackMessage, McpOauthCallbackReason, diff --git a/packages/testing/src/mocks/mcp-oauth.mock.ts b/packages/testing/src/mocks/mcp-oauth.mock.ts index 86d6f5508d0..81dee8de9c4 100644 --- a/packages/testing/src/mocks/mcp-oauth.mock.ts +++ b/packages/testing/src/mocks/mcp-oauth.mock.ts @@ -12,6 +12,7 @@ import { vi } from 'vitest' */ export const mcpOauthMockFns = { mockAssertSafeOauthServerUrl: vi.fn(), + mockMcpAuthGuarded: vi.fn(), mockGetOrCreateOauthRow: vi.fn(), mockLoadOauthRow: vi.fn(), mockLoadOauthRowByState: vi.fn(), @@ -63,6 +64,7 @@ function buildSimMcpOauthProvider(value: object) { */ export const mcpOauthMock = { assertSafeOauthServerUrl: mcpOauthMockFns.mockAssertSafeOauthServerUrl, + mcpAuthGuarded: mcpOauthMockFns.mockMcpAuthGuarded, getOrCreateOauthRow: mcpOauthMockFns.mockGetOrCreateOauthRow, loadOauthRow: mcpOauthMockFns.mockLoadOauthRow, loadOauthRowByState: mcpOauthMockFns.mockLoadOauthRowByState,