-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers #4441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
c9b183f
feat(mcp): OAuth 2.1 support for outbound MCP servers
waleedlatif1 f620a1b
fix(mcp): tighten OAuth refresh race and session-error detection
waleedlatif1 0bd2c7c
refactor(mcp): tighten OAuth callback contract and registration metadata
waleedlatif1 37e57ee
fix(mcp): narrow workspaceId before async closure in OAuth createClient
waleedlatif1 aaadbac
fix(mcp): return authType from create-server endpoint
waleedlatif1 6c75f50
fix(mcp): mirror server null normalization in optimistic oauthClientI…
waleedlatif1 5be7f79
fix(mcp): revert optimistic oauthClientId to undefined to match McpSe…
waleedlatif1 05c4bc1
fix(mcp): tighten OAuth probe signal and clear stale popup interval
waleedlatif1 b7c937d
fix(mcp): normalize empty-string oauthClientId at route boundary
waleedlatif1 15e2b41
feat(canvas): expand MCP tool params into per-row labels on block tile
waleedlatif1 2c6d2d1
feat(logs): show MCP icon and strip prefix in trace tool spans
waleedlatif1 ebeee76
fix(logs): lift near-black trace icon backgrounds for dark-mode contrast
waleedlatif1 243a6e6
fix(logs): fall back to neutral gray for near-black trace icon bgs
waleedlatif1 294136e
chore(db): drop 0209_mcp_oauth migration ahead of staging merge
waleedlatif1 96e6428
Merge remote-tracking branch 'origin/staging' into waleedlatif1/mcp-o…
waleedlatif1 8deaaeb
chore(db): regenerate MCP OAuth migration as 0210
waleedlatif1 b8f8f4a
chore(audit): bump route baseline 748 → 749 after staging merge
waleedlatif1 9fbbbff
chore: remove source-command skill files committed by accident
waleedlatif1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| 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' | ||
| import { toError } from '@sim/utils/errors' | ||
| import { and, eq, isNull } from 'drizzle-orm' | ||
| import type { NextRequest } from 'next/server' | ||
| import { NextResponse } from 'next/server' | ||
| import { mcpOauthCallbackContract } from '@/lib/api/contracts/mcp' | ||
| import { parseRequest } from '@/lib/api/server' | ||
| import { getSession } from '@/lib/auth' | ||
| import { withRouteHandler } from '@/lib/core/utils/with-route-handler' | ||
| import { | ||
| assertSafeOauthServerUrl, | ||
| clearState, | ||
| clearVerifier, | ||
| loadOauthRowByState, | ||
| loadPreregisteredClient, | ||
| type McpOauthCallbackReason, | ||
| SimMcpOauthProvider, | ||
| } from '@/lib/mcp/oauth' | ||
| import { mcpService } from '@/lib/mcp/service' | ||
|
|
||
| const logger = createLogger('McpOauthCallbackAPI') | ||
|
|
||
| export const dynamic = 'force-dynamic' | ||
|
|
||
| function escapeHtml(value: string): string { | ||
| return value | ||
| .replace(/&/g, '&') | ||
| .replace(/</g, '<') | ||
| .replace(/>/g, '>') | ||
| .replace(/"/g, '"') | ||
| .replace(/'/g, ''') | ||
| } | ||
|
|
||
| function jsonLiteral(value: string | undefined): string { | ||
| if (value === undefined) return 'undefined' | ||
| return JSON.stringify(value).replace(/</g, '\\u003c').replace(/>/g, '\\u003e') | ||
| } | ||
|
|
||
| function htmlClose( | ||
| message: string, | ||
| ok: boolean, | ||
| reason: McpOauthCallbackReason, | ||
| serverId?: string | ||
| ): NextResponse { | ||
| const safeMessage = escapeHtml(message) | ||
| const title = ok ? 'Connected' : 'Connection failed' | ||
| const body = `<!doctype html><html><head><meta charset="utf-8"><title>${title}</title></head><body style="font-family: system-ui; padding: 24px"><p>${safeMessage}</p><script> | ||
| try { window.opener && window.opener.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${jsonLiteral(serverId)}, reason: ${jsonLiteral(reason)} }, window.location.origin) } catch (e) {} | ||
| setTimeout(function () { window.close() }, 800) | ||
| </script></body></html>` | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
| return new NextResponse(body, { | ||
| headers: { 'Content-Type': 'text/html; charset=utf-8' }, | ||
| }) | ||
| } | ||
|
|
||
| export const GET = withRouteHandler(async (request: NextRequest) => { | ||
| const parsed = await parseRequest(mcpOauthCallbackContract, request, {}) | ||
| if (!parsed.success) { | ||
| return htmlClose('Malformed authorization callback.', false, 'missing_params') | ||
| } | ||
| const { state, code, error: errorParam } = parsed.data.query | ||
|
|
||
| const initialRow = state ? await loadOauthRowByState(state).catch(() => null) : null | ||
| const stateRowServerId = initialRow?.mcpServerId | ||
|
|
||
| if (errorParam) { | ||
| logger.warn(`MCP OAuth callback received error: ${errorParam}`) | ||
| if (initialRow) await clearState(initialRow.id).catch(() => {}) | ||
| return htmlClose( | ||
| `Authorization failed: ${errorParam}`, | ||
| false, | ||
| 'provider_error', | ||
| stateRowServerId | ||
| ) | ||
| } | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
| if (!state || !code) { | ||
| return htmlClose( | ||
| 'Missing state or code in callback URL.', | ||
| false, | ||
| 'missing_params', | ||
| stateRowServerId | ||
| ) | ||
| } | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
|
|
||
| let serverId: string | undefined | ||
| try { | ||
| const session = await getSession() | ||
| if (!session?.user?.id) { | ||
| return htmlClose( | ||
| 'You must be signed in to complete authorization.', | ||
| false, | ||
| 'unauthenticated', | ||
| stateRowServerId | ||
| ) | ||
| } | ||
|
|
||
| const row = initialRow | ||
| if (!row) { | ||
| return htmlClose('Invalid or expired authorization state.', false, 'invalid_state') | ||
| } | ||
| serverId = row.mcpServerId | ||
|
|
||
| if (session.user.id !== row.userId) { | ||
| return htmlClose( | ||
| 'You must be signed in as the same user that initiated the flow.', | ||
| false, | ||
| 'user_mismatch', | ||
| serverId | ||
| ) | ||
| } | ||
|
|
||
| const [server] = await db | ||
| .select({ id: mcpServers.id, url: mcpServers.url, workspaceId: mcpServers.workspaceId }) | ||
| .from(mcpServers) | ||
| .where(and(eq(mcpServers.id, row.mcpServerId), isNull(mcpServers.deletedAt))) | ||
| .limit(1) | ||
| if (!server || !server.url) { | ||
| return htmlClose('Server no longer exists.', false, 'server_gone', serverId) | ||
| } | ||
| if (server.workspaceId !== row.workspaceId) { | ||
| return htmlClose( | ||
| 'Workspace mismatch on authorization callback.', | ||
| false, | ||
| 'invalid_state', | ||
| serverId | ||
| ) | ||
| } | ||
| try { | ||
| assertSafeOauthServerUrl(server.url) | ||
| } catch { | ||
| return htmlClose( | ||
| 'MCP OAuth requires https (or http://localhost for development).', | ||
| false, | ||
| 'insecure_url', | ||
| serverId | ||
| ) | ||
| } | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
|
|
||
| // Burn state before token exchange so a replayed callback cannot reuse it. | ||
| await clearState(row.id) | ||
|
|
||
| const preregistered = await loadPreregisteredClient(server.id) | ||
| const provider = new SimMcpOauthProvider({ row, preregistered }) | ||
| let result: Awaited<ReturnType<typeof mcpAuth>> | ||
| try { | ||
| result = await mcpAuth(provider, { | ||
| serverUrl: server.url, | ||
| authorizationCode: code, | ||
| }) | ||
| } catch (e) { | ||
| logger.error('Token exchange failed during MCP OAuth callback', e) | ||
| return htmlClose( | ||
| 'Token exchange failed. Please try again.', | ||
| false, | ||
| 'token_exchange_failed', | ||
| server.id | ||
| ) | ||
| } finally { | ||
| await clearVerifier(row.id) | ||
| } | ||
|
|
||
| if (result !== 'AUTHORIZED') { | ||
| return htmlClose('Authorization did not complete.', false, 'token_exchange_failed', server.id) | ||
| } | ||
|
|
||
| try { | ||
| await mcpService.clearCache(server.workspaceId) | ||
| await mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId) | ||
| } catch (e) { | ||
| logger.warn('Post-auth tools refresh failed', toError(e).message) | ||
| } | ||
|
|
||
| return htmlClose('Connected. You can close this window.', true, 'authorized', server.id) | ||
| } catch (error) { | ||
| logger.error('MCP OAuth callback failed', error) | ||
| return htmlClose('Authorization failed. Please try again.', false, 'unknown', serverId) | ||
| } | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| /** | ||
| * @vitest-environment node | ||
| */ | ||
| import { | ||
| dbChainMock, | ||
| dbChainMockFns, | ||
| hybridAuthMock, | ||
| hybridAuthMockFns, | ||
| McpOauthRedirectRequiredMock, | ||
| mcpOauthMock, | ||
| mcpOauthMockFns, | ||
| permissionsMock, | ||
| permissionsMockFns, | ||
| resetDbChainMock, | ||
| schemaMock, | ||
| } from '@sim/testing' | ||
| import { NextRequest } from 'next/server' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const { mockMcpAuth } = vi.hoisted(() => ({ | ||
| mockMcpAuth: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock('@sim/db', () => dbChainMock) | ||
| vi.mock('@sim/db/schema', () => schemaMock) | ||
| vi.mock('drizzle-orm', () => ({ | ||
| and: vi.fn(), | ||
| eq: vi.fn(), | ||
| isNull: vi.fn(), | ||
| })) | ||
| vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({ | ||
| auth: mockMcpAuth, | ||
| })) | ||
| vi.mock('@/lib/auth/hybrid', () => hybridAuthMock) | ||
| vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock) | ||
| vi.mock('@/lib/mcp/oauth', () => mcpOauthMock) | ||
|
|
||
| import { GET } from './route' | ||
|
|
||
| describe('MCP OAuth start route', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| resetDbChainMock() | ||
| hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({ | ||
| success: true, | ||
| userId: 'user-2', | ||
| userName: 'User Two', | ||
| userEmail: 'user2@example.com', | ||
| authType: 'session', | ||
| }) | ||
| permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') | ||
| dbChainMockFns.limit.mockResolvedValue([ | ||
| { | ||
| id: 'server-1', | ||
| name: 'Exa', | ||
| url: 'https://mcp.exa.ai/mcp', | ||
| workspaceId: 'workspace-1', | ||
| authType: 'oauth', | ||
| deletedAt: null, | ||
| }, | ||
| ]) | ||
| mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValue({ | ||
| id: 'oauth-row-1', | ||
| mcpServerId: 'server-1', | ||
| userId: 'user-1', | ||
| workspaceId: 'workspace-1', | ||
| clientInformation: null, | ||
| tokens: null, | ||
| codeVerifier: null, | ||
| state: null, | ||
| stateCreatedAt: null, | ||
| updatedAt: new Date(), | ||
| }) | ||
| mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined) | ||
| mockMcpAuth.mockRejectedValue(new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize')) | ||
| }) | ||
|
|
||
| it('requires workspace write permission via MCP auth middleware', async () => { | ||
| const request = new NextRequest( | ||
| 'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1' | ||
| ) | ||
|
|
||
| await GET(request) | ||
|
|
||
| expect(permissionsMockFns.mockGetUserEntityPermissions).toHaveBeenCalledWith( | ||
| 'user-2', | ||
| 'workspace', | ||
| 'workspace-1' | ||
| ) | ||
| }) | ||
|
|
||
| it('uses a workspace-scoped OAuth row and stamps the latest authorizing user', async () => { | ||
| 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(200) | ||
| expect(body).toEqual({ | ||
| status: 'redirect', | ||
| authorizationUrl: 'https://mcp.exa.ai/authorize', | ||
| }) | ||
| expect(mcpOauthMockFns.mockGetOrCreateOauthRow).toHaveBeenCalledWith({ | ||
| mcpServerId: 'server-1', | ||
| userId: 'user-2', | ||
| workspaceId: 'workspace-1', | ||
| }) | ||
| expect(mcpOauthMockFns.mockSetOauthRowUser).toHaveBeenCalledWith('oauth-row-1', 'user-2') | ||
| }) | ||
|
|
||
| it('rejects a second user starting OAuth while another authorization is active', async () => { | ||
| mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValueOnce({ | ||
| id: 'oauth-row-1', | ||
| mcpServerId: 'server-1', | ||
| userId: 'user-1', | ||
| workspaceId: 'workspace-1', | ||
| clientInformation: null, | ||
| tokens: null, | ||
| codeVerifier: null, | ||
| state: 'hashed-active-state', | ||
| stateCreatedAt: new Date(), | ||
| updatedAt: new Date(), | ||
| }) | ||
| 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(409) | ||
| expect(body.error).toBe('OAuth authorization already in progress for this server') | ||
| expect(mockMcpAuth).not.toHaveBeenCalled() | ||
| }) | ||
|
waleedlatif1 marked this conversation as resolved.
|
||
| }) | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.