feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441
feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441waleedlatif1 wants to merge 13 commits intostagingfrom
Conversation
Adds spec-compliant OAuth support for MCP servers that require it (Linear, Slack, Notion, Atlassian, etc.) using the SDK's OAuthClientProvider. Tokens are persisted per-user-per-server and refreshed automatically. Also supports pre-registered OAuth clients for servers that don't expose Dynamic Client Registration.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Extends MCP server CRUD and types/contracts to support Updates MCP execution and discovery paths to attach an SDK Reviewed by Cursor Bugbot for commit e37f10a. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile Summary
Confidence Score: 3/5Not safe to merge as-is: one XSS security issue and one stale-token logic bug, both on the new OAuth paths. Two P1 findings: a javascript: XSS vector in the popup flow and missing mcp_server_oauth cleanup in the POST upsert path. Both are on net-new OAuth code paths introduced by this PR. Score is capped at 4 by P1 ceiling; two P1s bring it to 3. apps/sim/hooks/queries/mcp.ts (authorizationUrl scheme validation) and apps/sim/app/api/mcp/servers/route.ts (POST upsert OAuth token cleanup)
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/mcp/oauth/callback/route.ts | New OAuth callback handler; CSRF guard (state burn before exchange), XSS escaping, and user-identity verification are all present. State stored as SHA-256 hash in DB. |
| apps/sim/app/api/mcp/oauth/start/route.ts | New OAuth start handler; workspace membership enforced via withMcpAuth, authType guard present, correct error handling. |
| apps/sim/app/api/mcp/servers/route.ts | POST upsert path updates OAuth credentials but never deletes stale mcp_server_oauth rows; PATCH handler has the fix but this branch is missing it (P1). |
| apps/sim/app/api/mcp/servers/[id]/route.ts | PATCH handler correctly encrypts oauthClientSecret, clears mcp_server_oauth on URL/credential change, and strips the secret from API responses. |
| apps/sim/lib/mcp/oauth/storage.ts | Tokens and clientInformation are encrypted at rest; codeVerifier is now also encrypted; state stored as SHA-256 hash. |
| apps/sim/lib/mcp/oauth/provider.ts | SimMcpOauthProvider correctly implements the OAuthClientProvider interface; redirect captured via McpOauthRedirectRequired exception; preregistered client support included. |
| apps/sim/lib/mcp/oauth/probe.ts | Lightweight unauthenticated probe with 5 s timeout; SSRF validation runs before the probe in the POST handler; correct WWW-Authenticate header inspection. |
| apps/sim/hooks/queries/mcp.ts | useStartMcpOauth mutation opens authorizationUrl in a popup without validating the URL scheme, allowing javascript: XSS (P1). Query key refactor from flat keys to hierarchical keys looks correct. |
| apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx | OAuth connect button correctly scoped per-server via connectingOauthServers Set; postMessage listener validates origin and mcp-oauth type; auto-OAuth after server creation now passes workspaceId. |
| apps/sim/lib/mcp/service.ts | discoverServerTools and getServerSummaries both handle UnauthorizedError and McpOauthAuthorizationRequiredError, pushing disconnected status rather than error. |
Sequence Diagram
sequenceDiagram
participant User
participant UI as mcp.tsx
participant Start as /api/mcp/oauth/start
participant Callback as /api/mcp/oauth/callback
participant DB as mcp_server_oauth (DB)
participant AS as Authorization Server
User->>UI: Click "Connect with OAuth"
UI->>Start: GET /api/mcp/oauth/start?serverId&workspaceId
Start->>DB: getOrCreateOauthRow()
Start->>AS: mcpAuth(provider, {serverUrl}) → fetches OAuth metadata
AS-->>Start: throws McpOauthRedirectRequired(authorizationUrl)
Start-->>UI: {status: 'redirect', authorizationUrl}
UI->>User: window.open(authorizationUrl) → popup
User->>AS: Authorize in popup
AS->>Callback: GET /api/mcp/oauth/callback?state=&code=
Callback->>DB: loadOauthRowByState(SHA256(state))
Callback->>DB: clearState(rowId)
Callback->>AS: mcpAuth(provider, {serverUrl, authorizationCode}) → token exchange
AS-->>Callback: AUTHORIZED
Callback->>DB: saveTokens(encrypted), clearVerifier()
Callback-->>User: htmlClose(Connected) + postMessage({type:'mcp-oauth', ok:true})
UI->>UI: invalidate query cache → refresh server list
Reviews (12): Last reviewed commit: "fix(mcp): allow OAuth flow for DCR-only ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- provider: clear `state` from DB in `invalidateCredentials` to prevent stale state values from matching during CSRF check - storage: encrypt PKCE `codeVerifier` at rest to match `tokens` / `clientInformation` security posture - queries: `useForceRefreshMcpTools` now writes the fetched payload directly into the query cache instead of invalidating, eliminating the duplicate network round-trip - mcp settings: track per-server OAuth pending state so a "Connecting…" spinner only disables the card whose flow is in progress - mcp settings: surface existing `oauthClientId` in edit modal so the Advanced section auto-expands and displays the saved value
|
Greptile summary findings addressed in f587e82:
The point about clearing a pre-registered Client ID by emptying the field is a follow-up — |
|
@greptile |
|
@cursor review |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
|
@cursor review |
… edit Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
- Use word-boundary regex for 401 match in form auth heuristic - SHA-256 hash OAuth state in DB; lookup by hash to prevent replay if DB read leaks Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
…dits - Drop oauthClientId requirement on auth-failed bypass so DCR-capable servers can submit and trigger auto-OAuth - Use oauthClientSecretTouched in hasChanges so clearing the stored secret enables submit Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
| @@ -140,7 +170,10 @@ export const POST = withRouteHandler( | |||
| `[${requestId}] Successfully updated MCP server: ${body.name} (ID: ${serverId})` | |||
| ) | |||
|
|
|||
| return createMcpSuccessResponse({ serverId, updated: true }, 200) | |||
| return createMcpSuccessResponse( | |||
| { serverId, updated: true, authType: resolvedAuthType }, | |||
| 200 | |||
There was a problem hiding this comment.
When a server with the same URL already exists (existingServer is truthy), the code updates oauthClientId and oauthClientSecret directly but never deletes rows from mcp_server_oauth. The PATCH handler was fixed to run db.delete(mcpServerOauth).where(...) when urlChanged || oauthCredsChanged, but the POST upsert branch lacks the same step. Any user whose tokens were stored under the old client registration will continue to have those stale tokens served to them on the next connection attempt, which will fail at the authorization server (wrong client_id), silently leaving the server in a broken state until they manually reconnect.
| if (result.status === 'redirect') { | ||
| const popup = window.open( | ||
| result.authorizationUrl, | ||
| 'mcp-oauth', | ||
| 'width=560,height=720,resizable=yes,scrollbars=yes' | ||
| ) |
There was a problem hiding this comment.
authorizationUrl passed to window.open() without https: scheme validation
startMcpOauthResultSchema validates authorizationUrl with z.string().url(), which uses new URL(val) internally. In both Node.js 18+ and modern browsers, new URL('javascript:alert(1)') succeeds, meaning a javascript: URI passes schema validation. A malicious MCP server that returns javascript:... as its authorization URL would cause the mutation to call window.open('javascript:...', 'mcp-oauth', ...), executing arbitrary JavaScript in the opener's origin context. Add an explicit https: scheme check before opening the popup.
| if (result.status === 'redirect') { | |
| const popup = window.open( | |
| result.authorizationUrl, | |
| 'mcp-oauth', | |
| 'width=560,height=720,resizable=yes,scrollbars=yes' | |
| ) | |
| if (result.status === 'redirect') { | |
| const parsedUrl = new URL(result.authorizationUrl) | |
| if (parsedUrl.protocol !== 'https:') { | |
| throw new Error('Authorization URL must use HTTPS') | |
| } | |
| const popup = window.open( | |
| result.authorizationUrl, | |
| 'mcp-oauth', | |
| 'width=560,height=720,resizable=yes,scrollbars=yes' | |
| ) |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e37f10a. Configure here.
| get rowId(): string { | ||
| return this.row.id | ||
| } | ||
| } |
There was a problem hiding this comment.
SimMcpOauthProvider lacks explicit interface conformance check
Medium Severity
SimMcpOauthProvider does not declare implements OAuthClientProvider from the SDK. The SDK was upgraded from 1.25.3 to 1.29.0 in this PR, and the interface has evolved (e.g., optional methods like saveDiscoveryState, discoveryState, resourceUrl, saveResourceUrl). Without implements, if a future SDK update adds a required method or changes a signature, this will fail silently at runtime rather than at compile time. This is especially risky because the class is used as an authProvider by StreamableHTTPClientTransport, which expects full interface conformance.
Reviewed by Cursor Bugbot for commit e37f10a. Configure here.
| url: formData.url!, | ||
| headers, | ||
| timeout: formData.timeout || 30000, | ||
| oauthClientId: mode === 'edit' ? (oauthClientId ?? '') : oauthClientId || undefined, |
There was a problem hiding this comment.
Edit form always sends oauthClientId causing unnecessary processing
Low Severity
In edit mode, oauthClientId is unconditionally included in the PATCH payload (oauthClientId ?? ''), even when the Advanced section was never opened. Unlike oauthClientSecret which uses a touched guard to avoid unnecessary writes, oauthClientId has no such guard. While server-side normalization ('' || null) prevents false change detection today, the asymmetric approach between the two fields is inconsistent and could lead to subtle issues if normalization logic ever changes.
Reviewed by Cursor Bugbot for commit e37f10a. Configure here.
1 participant
Summary
OAuthClientProviderWWW-Authenticate/oauth-protected-resource)mcp_server_oauthtable; SDK refreshes automatically before expiry/api/mcp/oauth/start→/api/mcp/oauth/callback) withstateCSRF protectionreauth_requiredfrom tool execution when refresh token is invalid so the UI can prompt to reconnectType of Change
Testing
Tested manually against OAuth-protected MCP servers (Linear). Existing header-auth servers regression-checked.
Checklist