Skip to content

Commit 46db406

Browse files
waleedlatif1claude
andauthored
feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers (#4441)
* feat(mcp): OAuth 2.1 support for outbound MCP servers * fix(mcp): tighten OAuth refresh race and session-error detection Re-load the OAuth row inside withMcpOauthRefreshLock so concurrent callers observe predecessor-written tokens instead of a stale snapshot loaded before lock acquisition. Without this, the second caller's provider held a rotated-out refresh token and the SDK tripped invalid_grant, forcing reauthorization. Switch isSessionError to match the SDK's typed StreamableHTTPError (code 404/400) instead of substring-checking arbitrary error messages, removing false positives on URLs that happen to contain those digits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(mcp): tighten OAuth callback contract and registration metadata - Validate callback query params via mcpOauthCallbackContract instead of raw searchParams.get, matching the rest of the MCP route surface. - Drop non-RFC-7591 application_type field from dynamic client registration to avoid rejection by strict authorization servers. - Collapse the pre-lock OAuth row load in createClient — the row is now loaded exclusively inside withMcpOauthRefreshLock, removing a redundant query and a stale-snapshot path. * fix(mcp): narrow workspaceId before async closure in OAuth createClient * fix(mcp): return authType from create-server endpoint The POST /api/mcp/servers handler omitted authType from the success response, so useCreateMcpServer always saw data.data.authType as undefined and never triggered the OAuth popup after creating an OAuth-protected server. Thread authType through performCreateMcpServer into the response so the client can decide whether to auto-start OAuth. * fix(mcp): mirror server null normalization in optimistic oauthClientId update Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(mcp): revert optimistic oauthClientId to undefined to match McpServer type The response contract preprocesses null → undefined, so McpServer.oauthClientId is string | undefined. Using null broke type checking. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(mcp): tighten OAuth probe signal and clear stale popup interval - probe: only classify as OAuth on resource_metadata or scope params. Bare `Bearer error="invalid_token"` is generic and used by API-key servers, so it must not auto-flip the auth type to OAuth. - popup hook: clear any existing close-watcher interval before overwriting when startOauthForServer is invoked twice for the same serverId. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(mcp): normalize empty-string oauthClientId at route boundary Orchestration already converts falsy → null via `|| null` (server-lifecycle.ts), so the DB was never receiving an empty string. Tightening the route layer to match the same convention keeps the boundary contract consistent and avoids relying on downstream normalization. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(canvas): expand MCP tool params into per-row labels on block tile The MCP Tool block on the workflow canvas previously crammed every selected- tool parameter into a stringified blob under the `Tool` row. Now, when a tool is selected, the tile reads the cached `_toolSchema` and emits one labeled SubBlockRow per parameter (matching the Exa block's per-param layout). Labels reuse `formatParameterLabel` for parity with the editor panel; values pass through the existing `getDisplayValue` so booleans/numbers/arrays render identically to other blocks. Deterministic tile height counts expanded rows so the tile sizes correctly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(logs): show MCP icon and strip prefix in trace tool spans Tool spans for MCP calls were rendering the raw id (e.g. `mcp-f908f259-planetscale_list_organizations`) with the default blank- square icon. Now they read just the tool name and render the MCP block's icon and bgColor, matching how workflow-execute tools render. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(logs): lift near-black trace icon backgrounds for dark-mode contrast Block bgColors below a small luminance threshold (e.g. the MCP block's #181C1E) rendered nearly invisible against the dark-mode surface (--bg: #1b1b1b). Adds a tiny adjustBgForContrast helper that floors each RGB channel at 0x33 only when luminance is below 30,000, leaving every branded color above that band untouched. Applied to both the trace tree row and the detail pane. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(logs): fall back to neutral gray for near-black trace icon bgs #333333 was still too close to the dark-mode surface to read. For bgs below the luminance threshold (e.g. the MCP block's #181C1E) we now fall back to DEFAULT_BLOCK_COLOR (#6b7280) — the same neutral the renderer uses for blocks with no distinct identity. Clearly visible in both themes; brighter brand colors still pass through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(db): drop 0209_mcp_oauth migration ahead of staging merge Staging shipped 0209_smiling_fixer; the MCP OAuth migration will be regenerated on top of staging as 0210. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(db): regenerate MCP OAuth migration as 0210 Re-runs drizzle-kit generate on top of staging's 0209_smiling_fixer. Same schema (mcp_server_oauth table + mcp_servers.auth_type / oauth_* columns) as the dropped 0209_mcp_oauth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(audit): bump route baseline 748 → 749 after staging merge The post-merge route count is 749 (this branch's OAuth start/callback plus staging's new route). I had set the baseline to 748 in the merge conflict resolution — bumping to match reality so the strict audit passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: remove source-command skill files committed by accident These were untracked-then-accidentally-staged in 05c4bc1 via a wide `git add -A`. They aren't part of this PR's scope. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d9dd7a3 commit 46db406

45 files changed

Lines changed: 19496 additions & 548 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
import { auth as mcpAuth } from '@modelcontextprotocol/sdk/client/auth.js'
2+
import { db } from '@sim/db'
3+
import { mcpServers } from '@sim/db/schema'
4+
import { createLogger } from '@sim/logger'
5+
import { toError } from '@sim/utils/errors'
6+
import { and, eq, isNull } from 'drizzle-orm'
7+
import type { NextRequest } from 'next/server'
8+
import { NextResponse } from 'next/server'
9+
import { mcpOauthCallbackContract } from '@/lib/api/contracts/mcp'
10+
import { parseRequest } from '@/lib/api/server'
11+
import { getSession } from '@/lib/auth'
12+
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
13+
import {
14+
assertSafeOauthServerUrl,
15+
clearState,
16+
clearVerifier,
17+
loadOauthRowByState,
18+
loadPreregisteredClient,
19+
type McpOauthCallbackReason,
20+
SimMcpOauthProvider,
21+
} from '@/lib/mcp/oauth'
22+
import { mcpService } from '@/lib/mcp/service'
23+
24+
const logger = createLogger('McpOauthCallbackAPI')
25+
26+
export const dynamic = 'force-dynamic'
27+
28+
function escapeHtml(value: string): string {
29+
return value
30+
.replace(/&/g, '&amp;')
31+
.replace(/</g, '&lt;')
32+
.replace(/>/g, '&gt;')
33+
.replace(/"/g, '&quot;')
34+
.replace(/'/g, '&#39;')
35+
}
36+
37+
function jsonLiteral(value: string | undefined): string {
38+
if (value === undefined) return 'undefined'
39+
return JSON.stringify(value).replace(/</g, '\\u003c').replace(/>/g, '\\u003e')
40+
}
41+
42+
function htmlClose(
43+
message: string,
44+
ok: boolean,
45+
reason: McpOauthCallbackReason,
46+
serverId?: string
47+
): NextResponse {
48+
const safeMessage = escapeHtml(message)
49+
const title = ok ? 'Connected' : 'Connection failed'
50+
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>
51+
try { window.opener && window.opener.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${jsonLiteral(serverId)}, reason: ${jsonLiteral(reason)} }, window.location.origin) } catch (e) {}
52+
setTimeout(function () { window.close() }, 800)
53+
</script></body></html>`
54+
return new NextResponse(body, {
55+
headers: { 'Content-Type': 'text/html; charset=utf-8' },
56+
})
57+
}
58+
59+
export const GET = withRouteHandler(async (request: NextRequest) => {
60+
const parsed = await parseRequest(mcpOauthCallbackContract, request, {})
61+
if (!parsed.success) {
62+
return htmlClose('Malformed authorization callback.', false, 'missing_params')
63+
}
64+
const { state, code, error: errorParam } = parsed.data.query
65+
66+
const initialRow = state ? await loadOauthRowByState(state).catch(() => null) : null
67+
const stateRowServerId = initialRow?.mcpServerId
68+
69+
if (errorParam) {
70+
logger.warn(`MCP OAuth callback received error: ${errorParam}`)
71+
if (initialRow) await clearState(initialRow.id).catch(() => {})
72+
return htmlClose(
73+
`Authorization failed: ${errorParam}`,
74+
false,
75+
'provider_error',
76+
stateRowServerId
77+
)
78+
}
79+
if (!state || !code) {
80+
return htmlClose(
81+
'Missing state or code in callback URL.',
82+
false,
83+
'missing_params',
84+
stateRowServerId
85+
)
86+
}
87+
88+
let serverId: string | undefined
89+
try {
90+
const session = await getSession()
91+
if (!session?.user?.id) {
92+
return htmlClose(
93+
'You must be signed in to complete authorization.',
94+
false,
95+
'unauthenticated',
96+
stateRowServerId
97+
)
98+
}
99+
100+
const row = initialRow
101+
if (!row) {
102+
return htmlClose('Invalid or expired authorization state.', false, 'invalid_state')
103+
}
104+
serverId = row.mcpServerId
105+
106+
if (session.user.id !== row.userId) {
107+
return htmlClose(
108+
'You must be signed in as the same user that initiated the flow.',
109+
false,
110+
'user_mismatch',
111+
serverId
112+
)
113+
}
114+
115+
const [server] = await db
116+
.select({ id: mcpServers.id, url: mcpServers.url, workspaceId: mcpServers.workspaceId })
117+
.from(mcpServers)
118+
.where(and(eq(mcpServers.id, row.mcpServerId), isNull(mcpServers.deletedAt)))
119+
.limit(1)
120+
if (!server || !server.url) {
121+
return htmlClose('Server no longer exists.', false, 'server_gone', serverId)
122+
}
123+
if (server.workspaceId !== row.workspaceId) {
124+
return htmlClose(
125+
'Workspace mismatch on authorization callback.',
126+
false,
127+
'invalid_state',
128+
serverId
129+
)
130+
}
131+
try {
132+
assertSafeOauthServerUrl(server.url)
133+
} catch {
134+
return htmlClose(
135+
'MCP OAuth requires https (or http://localhost for development).',
136+
false,
137+
'insecure_url',
138+
serverId
139+
)
140+
}
141+
142+
// Burn state before token exchange so a replayed callback cannot reuse it.
143+
await clearState(row.id)
144+
145+
const preregistered = await loadPreregisteredClient(server.id)
146+
const provider = new SimMcpOauthProvider({ row, preregistered })
147+
let result: Awaited<ReturnType<typeof mcpAuth>>
148+
try {
149+
result = await mcpAuth(provider, {
150+
serverUrl: server.url,
151+
authorizationCode: code,
152+
})
153+
} catch (e) {
154+
logger.error('Token exchange failed during MCP OAuth callback', e)
155+
return htmlClose(
156+
'Token exchange failed. Please try again.',
157+
false,
158+
'token_exchange_failed',
159+
server.id
160+
)
161+
} finally {
162+
await clearVerifier(row.id)
163+
}
164+
165+
if (result !== 'AUTHORIZED') {
166+
return htmlClose('Authorization did not complete.', false, 'token_exchange_failed', server.id)
167+
}
168+
169+
try {
170+
await mcpService.clearCache(server.workspaceId)
171+
await mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId)
172+
} catch (e) {
173+
logger.warn('Post-auth tools refresh failed', toError(e).message)
174+
}
175+
176+
return htmlClose('Connected. You can close this window.', true, 'authorized', server.id)
177+
} catch (error) {
178+
logger.error('MCP OAuth callback failed', error)
179+
return htmlClose('Authorization failed. Please try again.', false, 'unknown', serverId)
180+
}
181+
})
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import {
5+
dbChainMock,
6+
dbChainMockFns,
7+
hybridAuthMock,
8+
hybridAuthMockFns,
9+
McpOauthRedirectRequiredMock,
10+
mcpOauthMock,
11+
mcpOauthMockFns,
12+
permissionsMock,
13+
permissionsMockFns,
14+
resetDbChainMock,
15+
schemaMock,
16+
} from '@sim/testing'
17+
import { NextRequest } from 'next/server'
18+
import { beforeEach, describe, expect, it, vi } from 'vitest'
19+
20+
const { mockMcpAuth } = vi.hoisted(() => ({
21+
mockMcpAuth: vi.fn(),
22+
}))
23+
24+
vi.mock('@sim/db', () => dbChainMock)
25+
vi.mock('@sim/db/schema', () => schemaMock)
26+
vi.mock('drizzle-orm', () => ({
27+
and: vi.fn(),
28+
eq: vi.fn(),
29+
isNull: vi.fn(),
30+
}))
31+
vi.mock('@modelcontextprotocol/sdk/client/auth.js', () => ({
32+
auth: mockMcpAuth,
33+
}))
34+
vi.mock('@/lib/auth/hybrid', () => hybridAuthMock)
35+
vi.mock('@/lib/workspaces/permissions/utils', () => permissionsMock)
36+
vi.mock('@/lib/mcp/oauth', () => mcpOauthMock)
37+
38+
import { GET } from './route'
39+
40+
describe('MCP OAuth start route', () => {
41+
beforeEach(() => {
42+
vi.clearAllMocks()
43+
resetDbChainMock()
44+
hybridAuthMockFns.mockCheckSessionOrInternalAuth.mockResolvedValue({
45+
success: true,
46+
userId: 'user-2',
47+
userName: 'User Two',
48+
userEmail: 'user2@example.com',
49+
authType: 'session',
50+
})
51+
permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
52+
dbChainMockFns.limit.mockResolvedValue([
53+
{
54+
id: 'server-1',
55+
name: 'Exa',
56+
url: 'https://mcp.exa.ai/mcp',
57+
workspaceId: 'workspace-1',
58+
authType: 'oauth',
59+
deletedAt: null,
60+
},
61+
])
62+
mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValue({
63+
id: 'oauth-row-1',
64+
mcpServerId: 'server-1',
65+
userId: 'user-1',
66+
workspaceId: 'workspace-1',
67+
clientInformation: null,
68+
tokens: null,
69+
codeVerifier: null,
70+
state: null,
71+
stateCreatedAt: null,
72+
updatedAt: new Date(),
73+
})
74+
mcpOauthMockFns.mockLoadPreregisteredClient.mockResolvedValue(undefined)
75+
mockMcpAuth.mockRejectedValue(new McpOauthRedirectRequiredMock('https://mcp.exa.ai/authorize'))
76+
})
77+
78+
it('requires workspace write permission via MCP auth middleware', async () => {
79+
const request = new NextRequest(
80+
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
81+
)
82+
83+
await GET(request)
84+
85+
expect(permissionsMockFns.mockGetUserEntityPermissions).toHaveBeenCalledWith(
86+
'user-2',
87+
'workspace',
88+
'workspace-1'
89+
)
90+
})
91+
92+
it('uses a workspace-scoped OAuth row and stamps the latest authorizing user', async () => {
93+
const request = new NextRequest(
94+
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
95+
)
96+
97+
const response = await GET(request)
98+
const body = await response.json()
99+
100+
expect(response.status).toBe(200)
101+
expect(body).toEqual({
102+
status: 'redirect',
103+
authorizationUrl: 'https://mcp.exa.ai/authorize',
104+
})
105+
expect(mcpOauthMockFns.mockGetOrCreateOauthRow).toHaveBeenCalledWith({
106+
mcpServerId: 'server-1',
107+
userId: 'user-2',
108+
workspaceId: 'workspace-1',
109+
})
110+
expect(mcpOauthMockFns.mockSetOauthRowUser).toHaveBeenCalledWith('oauth-row-1', 'user-2')
111+
})
112+
113+
it('rejects a second user starting OAuth while another authorization is active', async () => {
114+
mcpOauthMockFns.mockGetOrCreateOauthRow.mockResolvedValueOnce({
115+
id: 'oauth-row-1',
116+
mcpServerId: 'server-1',
117+
userId: 'user-1',
118+
workspaceId: 'workspace-1',
119+
clientInformation: null,
120+
tokens: null,
121+
codeVerifier: null,
122+
state: 'hashed-active-state',
123+
stateCreatedAt: new Date(),
124+
updatedAt: new Date(),
125+
})
126+
const request = new NextRequest(
127+
'http://localhost:3000/api/mcp/oauth/start?workspaceId=workspace-1&serverId=server-1'
128+
)
129+
130+
const response = await GET(request)
131+
const body = await response.json()
132+
133+
expect(response.status).toBe(409)
134+
expect(body.error).toBe('OAuth authorization already in progress for this server')
135+
expect(mockMcpAuth).not.toHaveBeenCalled()
136+
})
137+
})

0 commit comments

Comments
 (0)