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 39 commits intostagingfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryHigh Risk Overview Adds new Extends MCP server CRUD to store Reviewed by Cursor Bugbot for commit aa8078d. Configure here. |
Greptile SummaryThis PR implements full OAuth 2.1 + PKCE support for outbound MCP servers, introducing a
Confidence Score: 5/5Safe to merge; OAuth plumbing is well-structured, all previously identified security gaps are closed, and remaining findings are minor hardening suggestions that do not affect correctness or security of the happy path. All significant issues from prior review rounds are confirmed addressed. The two new findings are narrow: state TTL via updatedAt is low-risk because states are single-use, and the form modal auth-bypass heuristic affects only the UX guard, not the server-side security boundary. apps/sim/lib/mcp/oauth/storage.ts (state TTL via updatedAt) and the mcp-server-form-modal.tsx (connection-test bypass heuristic) would benefit from a follow-up hardening pass. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Browser/UI
participant Start as /api/mcp/oauth/start
participant CB as /api/mcp/oauth/callback
participant DB as mcp_server_oauth
participant AS as Auth Server MCP
UI->>+Start: GET with serverId and workspaceId
Start->>DB: getOrCreateOauthRow
DB-->>Start: oauth row
Start->>AS: SDK mcpAuth metadata discovery plus DCR
AS-->>Start: McpOauthRedirectRequired
Start-->>-UI: status redirect with authorizationUrl
UI->>UI: window.open authorizationUrl
AS-->>UI: redirect to callback with auth code and state
UI->>+CB: GET callback
CB->>DB: loadOauthRowByState hash lookup with TTL
DB-->>CB: oauth row
CB->>DB: clearState burn before exchange
CB->>AS: SDK mcpAuth token exchange
AS-->>CB: access and refresh tokens
CB->>DB: saveTokens encrypted
CB->>DB: clearVerifier
CB-->>-UI: postMessage ok true
Reviews (24): Last reviewed commit: "fix(mcp): final audit fixes — state TTL,..." | Re-trigger Greptile |
|
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 |
…entProvider conformance - POST upsert now clears mcp_server_oauth rows when URL or client credentials change - Validate https: scheme on authorizationUrl before window.open to prevent javascript: URI execution - SimMcpOauthProvider now declares 'implements OAuthClientProvider' so SDK upgrades surface as compile errors - Edit form only sends oauthClientId when changed, mirroring oauthClientSecret behavior Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…k error Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move popup-opening into the mutation result so the caller can track its lifecycle. The 'Connecting…' spinner now stays until the user dismisses or completes the OAuth popup, preventing accidental double-clicks that would re-navigate the in-flight popup and invalidate state. Auto-OAuth after server creation now uses the same shared helper for consistent visual feedback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… on unmount - POST upsert: when reviving a soft-deleted server, drop any prior mcpServerOauth rows so stale tokens never silently carry over. - mcp.tsx: track the popup-closed setInterval per server in a ref and clear it on component unmount to avoid leaked timers. - client.ts: don't log OAuth-redirect/Unauthorized as connect errors; these are expected control flow during the auth bootstrap. - Use toError() for error message extraction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The reference is only consumed by inline arrow handlers and is not observed by any memoized child or effect dep array, so useCallback adds overhead with no benefit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- useForceRefreshMcpTools: onSuccess → onSettled so cache reconciles on error - useMcpServerTest: replace `instanceof Error` ternaries with `toError().message` - mcp.tsx: use `--text-error` token (not the unused `--error`) and drop redundant dark variant Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- mcp.tsx: drop 8 useCallback wrappers (no React.memo'd children, no effect/memo deps observe them)
- mcp.tsx: drop filteredServers useMemo (cheap O(n) filter, no memoized consumers)
- mcp.tsx: serverToDelete {id, name} → serverToDeleteId; derive name from servers cache
- mcp-server-form-modal.tsx: drop 8 useCallback wrappers (same rationale)
- mcp-server-form-modal.tsx: drop hasChanges useMemo — deps change every keystroke so memo never caches
- mcp-server-form-modal.tsx: hover: → hover-hover: for codebase pointer:fine consistency
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…oken clear - client.ts: pass requestInit.headers for OAuth servers too. Previously OAuth authType set requestInit to undefined, dropping all custom headers including SIM_VIA_HEADER for cross-call loop prevention. The SDK's authProvider adds Authorization on top, so user/system headers must still flow through. - servers/[id]/route.ts: wrap server UPDATE and stale OAuth-token DELETE in a single transaction. Previously the update committed before the token clear, so a token-clear failure would leave new credentials with stale tokens. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…in edit modal Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Id on tool reauth errors Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- use mcp-oauth-${serverId} window target so concurrent OAuth flows on
different servers don't reuse and clobber the same popup
- drop redundant setQueryData before invalidate in useForceRefreshMcpTools
- replace hardcoded text-red-500 with text-[var(--text-error)] token
- normalize Plus icon to default h-[14px] w-[14px]
- drop useMemo on cheap toolsByServer/selectedServer derivations
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- hoist serverId from try-block const into outer scope so the catch's htmlClose carries it through to postMessage. Without it, parent's onMessage couldn't clear connectingOauthServers and the UI button stayed stuck on "Connecting…" until popup close. - relax https-only authorization URL check to permit http://localhost, http://127.0.0.1, and http://[::1] per OAuth 2.1 loopback exemption, unblocking local OAuth-protected MCP server development. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the old 0202_unknown_newton_destine migration which collided with staging's 0202/0203/0204. Bumps API validation route baseline to 735 to account for the two new MCP OAuth endpoints. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…er bug - Prevent stored Authorization header from overwriting OAuth Bearer in McpClient - Per-user connection cache keying in connection-manager (token-leak prevention) - Tighten types in use-mcp-tools and tools/execute (drop `any`) - Replace raw <button> with emcn Button in mcp settings + form modal - Modal Cancel: variant='ghost' → 'default' to match design system - Derive editInitialData and showDeleteDialog from existing state - Replace refreshingServers Record + chained timer with mutation state - Trigger MCP OAuth start on create when authType==='oauth' from tool-input - Invalidate servers/storedTools alongside tools on force-refresh Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ments Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…, callback escape - Extract useMcpOauthPopup hook so the tool-input "add server" flow gets the same postMessage/popup-poll lifecycle as the settings page. - PATCH /mcp/servers/:id now returns hasOauthClientSecret to mirror GET. - Escape '<' / '>' inside the JSON-stringified serverId emitted in the callback's <script> tag for defense-in-depth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tate field - tool-input now passes result.serverId (the contract-defined property) to startOauthForServer instead of the duplicated result.id alias. - Drop the unused state field from McpOauthRow. The DB column stores a hash; the in-memory copy was only ever assigned (never read for logic), and provider.state() was setting it to plaintext, creating an inconsistent hash-vs-plaintext type. Removing it eliminates the foot-gun. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… upsert - useMcpOauthPopup: depend on the stable mutateAsync reference instead of the whole mutation object (which changes identity every render and defeats the useCallback memoization). - POST /api/mcp/servers upsert path: only force connectionStatus to 'disconnected' / clear lastConnected for OAuth servers when we actually cleared the OAuth row (URL/credential change or revival). When the upsert is a no-op for OAuth state, the existing tokens stay valid and the server should remain connected — we now leave both columns alone in that case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fallback - loadOauthRowByState: enforce 10min TTL on state row to prevent replay of stale unconsumed states. - McpClient: throw on authType=oauth without an authProvider instead of silently falling back to header auth. - loadPreregisteredClient: tolerate decrypt failure by treating as no preregistered client rather than crashing the OAuth flow.
88e7f4f to
be03225
Compare
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