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 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Introduces a new Updates the MCP client/service/connection manager to use an SDK Reviewed by Cursor Bugbot for commit c292149. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds spec-compliant OAuth 2.1 + PKCE support for outbound MCP servers, including auto-detection of OAuth requirements via an unauthenticated probe, encrypted per-server token storage, a popup-based consent flow with CSRF state protection, and optional pre-registered client credentials. A large number of previously-flagged issues (workspace-boundary revocation, stale token cleanup on credential change,
Confidence Score: 3/5The core OAuth flow is sound, but a TTL design issue in the state-expiry logic can permanently block workspace members from starting new OAuth flows under realistic concurrent conditions. Both the apps/sim/lib/mcp/oauth/storage.ts — the Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Browser (MCP Settings)
participant Start as /api/mcp/oauth/start
participant AS as Authorization Server
participant Callback as /api/mcp/oauth/callback
participant DB as mcp_server_oauth (DB)
participant SDK as MCP SDK (mcpAuth)
UI->>Start: "GET ?serverId=&workspaceId="
Start->>DB: getOrCreateOauthRow()
Start->>SDK: "mcpAuth(provider, {serverUrl})"
SDK->>DB: saveState(hash) + saveCodeVerifier(enc)
SDK-->>Start: throws McpOauthRedirectRequired(url)
Start-->>UI: "{status:redirect, authorizationUrl}"
UI->>AS: window.open(authorizationUrl)
AS-->>UI: "Redirect to /api/mcp/oauth/callback?code=&state="
UI->>Callback: "GET ?code=&state="
Callback->>DB: loadOauthRowByState(hash(state))
Callback->>DB: clearState()
Callback->>SDK: "mcpAuth(provider, {serverUrl, authorizationCode})"
SDK->>AS: POST /token (code + PKCE verifier)
AS-->>SDK: "{access_token, refresh_token}"
SDK->>DB: saveTokens(encrypted)
Callback->>DB: clearVerifier()
Callback-->>UI: "postMessage({type:mcp-oauth, ok:true})"
UI->>UI: Invalidate server/tool queries
Reviews (33): Last reviewed commit: "fix(mcp): release probe session, clean u..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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 |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@greptile |
|
@cursor review |
|
@cursor review |
|
@greptile |
Look up the OAuth row by state once at the top of the callback so the serverId can be threaded into provider_error, missing_params, and unauthenticated closes. Without it, the popup's postMessage omits serverId and the parent UI can't clear the connecting state until the 500ms popup-closed poll fires. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
revokeMcpOauthTokens bailed when row.clientInformation was null, which is always the case for pre-registered clients (SimMcpOauthProvider.saveClientInformation is a no-op when preregistered is set). Fall back to mcpServers.oauthClientId so revocation proceeds for the pre-registered path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
- Burn the state row on the provider-error path so a replayed callback with the same state cannot complete a token exchange after the user saw the error. - Read the OAuth state row once at the top of the callback and reuse it on the happy path, eliminating a redundant query and a TTL-boundary race where a legitimate flow could be marked invalid_state between two reads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@cursor review |
|
@greptile |
The useUpdateMcpServer optimistic update was spreading the raw updates object into the TanStack Query cache, which briefly placed the plaintext oauthClientSecret in client-side cache. Strip it from the optimistic spread — the cache holds the server-sanitized record after onSettled invalidation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
…mapping - useCreateMcpServer was returning the raw serverData (including plaintext oauthClientSecret) as mutation.data; strip the secret to match the equivalent fix applied to useUpdateMcpServer. - Extract the McpOauthRow mapping/decryption from loadOauthRow and loadOauthRowByState into a shared mapOauthRow helper. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
- detectMcpAuthType now reads the Mcp-Session-Id response header and fires a best-effort DELETE to release the streamable-HTTP session the probe just allocated. Failures are ignored; servers will time the session out anyway. - Rewrite the refresh-lock chain to `prev.catch(() => undefined).then(() => fn())` so the predecessor's rejection reason is explicitly discarded rather than threaded into fn as an ignored argument. Each caller's outcome stays its own. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c292149. Configure here.
| } | ||
|
|
||
| await revokeMcpOauthTokens(serverId) | ||
|
|
There was a problem hiding this comment.
Delete handler doesn't clean up OAuth database rows
Medium Severity
The DELETE handler calls revokeMcpOauthTokens(serverId) before hard-deleting the mcpServers row but never deletes the corresponding mcpServerOauth row. Both the PATCH handler and the POST (update-existing) handler explicitly call tx.delete(mcpServerOauth).where(eq(mcpServerOauth.mcpServerId, serverId)) inside their transactions when OAuth state needs clearing. The DELETE handler is missing this step, leaving orphaned rows containing encrypted tokens and client credentials in the mcp_server_oauth table after a server is removed.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit c292149. Configure here.
|
|
||
| export async function clearVerifier(rowId: string): Promise<void> { | ||
| await db | ||
| .update(mcpServerOauth) |
There was a problem hiding this comment.
updatedAt used for state TTL is refreshed by token saves, enabling permanent 409 loops
hasActiveFlow in start/route.ts checks !!row.state && row.updatedAt.getTime() > Date.now() - OAUTH_START_TTL_MS, and loadOauthRowByState checks gt(mcpServerOauth.updatedAt, …) with the same 10-minute window. Both TTLs are anchored to updatedAt, but saveTokens also sets updatedAt: new Date() on every token refresh.
Concretely: if user A starts an OAuth flow and abandons it (state column stays non-null), while user B is actively executing tools against the server (triggering background token refreshes via the SDK), updatedAt keeps getting pushed forward. User C who tries to start a new flow will see hasActiveFlow = true and get a 409, indefinitely — the 10-minute window resets on every tool execution.
The fix is a dedicated stateCreatedAt column (set only in saveState) so that token refreshes no longer interfere with state expiry, and so that loadOauthRowByState can expire on actual state age rather than row age.
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