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 into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Introduces encrypted, workspace-scoped OAuth persistence in a new Updates MCP runtime paths ( Reviewed by Cursor Bugbot for commit 08fe845. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds OAuth 2.1 + PKCE support for outbound MCP servers using the SDK's
Confidence Score: 5/5Safe to merge. The OAuth flow is well-structured: state is burned before token exchange, PKCE verifier is cleared in a finally block, cross-workspace revocation is guarded, and the execute path propagates reauth_required correctly. All previously identified issues have been addressed. The security-sensitive paths (callback state validation, workspace isolation on DELETE/PATCH, secret encryption, URL scheme checks before window.open) are all handled correctly. The two remaining observations are a documentation mismatch (workspace-shared vs. per-user credentials) and a UX edge case where tool discovery failure after OAuth doesn't immediately flip connectionStatus to connected — neither affects correctness or security of the authorization flow itself. packages/db/schema.ts and apps/sim/app/api/mcp/oauth/callback/route.ts warrant a second look for the workspace-shared credential design and the post-auth discovery error path. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Browser (mcp.tsx)
participant Start as /api/mcp/oauth/start
participant CB as /api/mcp/oauth/callback
participant AS as Authorization Server
participant DB as mcp_server_oauth (DB)
participant Svc as McpService
UI->>Start: "GET ?serverId=&workspaceId="
Start->>DB: getOrCreateOauthRow (mcpServerId unique)
Start->>AS: mcpAuth(provider, serverUrl) → redirectToAuthorization
Start-->>UI: "{status:redirect, authorizationUrl}"
UI->>UI: window.open(authorizationUrl, popup)
UI->>AS: user consents in popup
AS->>CB: "GET /callback?code=&state="
CB->>DB: loadOauthRowByState(hashState)
CB->>DB: clearState (burn before exchange)
CB->>AS: mcpAuth(provider, authorizationCode) → token exchange
AS-->>CB: access_token + refresh_token
CB->>DB: saveTokens (encrypted)
CB->>DB: clearVerifier
CB->>Svc: discoverServerTools (post-auth refresh)
CB-->>UI: "postMessage {type:mcp-oauth, ok:true, serverId}"
UI->>UI: invalidate queries, toast Authorized
Reviews (32): Last reviewed commit: "fix(mcp): strip secret from create mutat..." | 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 |
|
@greptile |
|
@cursor review |
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 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 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 08fe845. Configure here.
| }, | ||
| }), | ||
| signal: controller.signal, | ||
| }) |
There was a problem hiding this comment.
Probe leaks MCP session by sending initialize without cleanup
Low Severity
detectMcpAuthType sends a real JSON-RPC initialize request to the target MCP server to probe for OAuth. When the server responds with 200 (no auth required), it has allocated a server-side session, but the probe never sends a corresponding close/disconnect. This leaks a session on the remote server for every probe invocation, which happens on every new MCP server creation where authType auto-detection runs.
Reviewed by Cursor Bugbot for commit 08fe845. Configure here.
| } | ||
| next.then(cleanup, cleanup) | ||
| return next | ||
| } |
There was a problem hiding this comment.
Refresh lock silently swallows predecessor errors in chain
Medium Severity
withMcpOauthRefreshLock chains via prev.then(fn, fn), meaning fn is invoked as the rejection handler for the predecessor. If the previous operation failed, fn receives the rejection reason as its first argument (silently ignored since fn takes no args). More critically, if two callers chain and the first rejects, the second caller's fn runs but its completion/failure doesn't propagate back to the first caller — the first caller only gets the original rejection from its own fn. This makes debugging silent failures harder and could mask the root cause of token refresh failures.
Reviewed by Cursor Bugbot for commit 08fe845. Configure here.
| userId, | ||
| workspaceId, | ||
| }) | ||
| const hasActiveFlow = !!row.state && row.updatedAt.getTime() > Date.now() - OAUTH_START_TTL_MS |
There was a problem hiding this comment.
Active flow check uses mutable updatedAt instead of state creation time
Low Severity
The hasActiveFlow TTL check compares row.updatedAt against OAUTH_START_TTL_MS, but updatedAt is modified by any write to the OAuth row (e.g., saveCodeVerifier, saveTokens, clearVerifier). This means the effective TTL window can be silently extended beyond 10 minutes. The same issue affects loadOauthRowByState, which also gates on updatedAt > now - STATE_TTL_MS. A dedicated stateCreatedAt or stable timestamp from when the state was saved would make the TTL accurate.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 08fe845. 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