Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441

Open
waleedlatif1 wants to merge 40 commits intostagingfrom
waleedlatif1/mcp-oauth
Open

feat(mcp): OAuth 2.1 + PKCE for outbound MCP servers#4441
waleedlatif1 wants to merge 40 commits intostagingfrom
waleedlatif1/mcp-oauth

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Adds spec-compliant OAuth 2.1 + PKCE support for outbound MCP servers via the SDK's OAuthClientProvider
  • Auto-detects OAuth requirement on server create via unauthenticated probe (WWW-Authenticate / oauth-protected-resource)
  • Persists per-user-per-server tokens (encrypted) in new mcp_server_oauth table; SDK refreshes automatically before expiry
  • Popup-based consent flow (/api/mcp/oauth/start/api/mcp/oauth/callback) with state CSRF protection
  • Pre-registered OAuth client support (Client ID + Secret in Advanced settings) for servers without RFC 7591 DCR
  • Surfaces reauth_required from tool execution when refresh token is invalid so the UI can prompt to reconnect

Type of Change

  • New feature

Testing

Tested manually against OAuth-protected MCP servers (Linear). Existing header-auth servers regression-checked.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 7, 2026 3:16am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 5, 2026

PR Summary

High Risk
Adds an OAuth consent flow and encrypted token storage for outbound MCP connections, and changes how tools are executed/discovered across server auth modes; mistakes here can break integrations or expose auth edge cases. Also introduces new DB tables/columns and per-user connection scoping, increasing rollout risk.

Overview
Enables outbound MCP servers to authenticate via OAuth 2.1 + PKCE (in addition to header/none auth), including auto-probing server auth requirements and a popup-based consent flow.

Adds new /api/mcp/oauth/start and /api/mcp/oauth/callback routes plus an SDK OAuthClientProvider implementation that persists per-user OAuth state (client info, tokens, PKCE verifier/state) in a new encrypted mcp_server_oauth table; callback burns state and refreshes discovered tools.

Extends MCP server CRUD to store authType and optional pre-registered oauthClientId/oauthClientSecret (encrypted), hides secrets via hasOauthClientSecret, and clears per-user OAuth rows when URL/credentials change. Updates tool execution/discovery and the persistent connection manager to attach OAuth providers, scope managed connections by (serverId,userId), and return a 401 reauth_required when OAuth is missing/expired so the UI can prompt reconnect.

Reviewed by Cursor Bugbot for commit aa8078d. Configure here.

Comment thread apps/sim/lib/mcp/oauth/provider.ts
Comment thread apps/sim/hooks/queries/mcp.ts
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Greptile Summary

This PR implements full OAuth 2.1 + PKCE support for outbound MCP servers, introducing a mcp_server_oauth DB table for per-user encrypted token storage, a SimMcpOauthProvider that drives the SDK's standard auth flow, and a popup-based consent UI (/api/mcp/oauth/start/api/mcp/oauth/callback). Several security issues found in earlier review passes have all been addressed in subsequent commits.

  • New OAuth infrastructure: mcp_server_oauth table, SimMcpOauthProvider, encrypted storage helpers, and probe-based authType auto-detection are wired together coherently; connection-manager cache keys are correctly scoped per-user for OAuth servers.
  • reauth_required propagation: Tool execution surfaces a structured { code: 'reauth_required', serverId } response for all OAuth error types, and both discoverServerTools and getServerSummaries mark expired-token servers as disconnected rather than error.
  • Remaining minor concerns: The state-lifetime TTL in loadOauthRowByState is driven by updatedAt which any row write can bump, and the form modal's connection-test bypass heuristic is broader than necessary.

Confidence Score: 5/5

Safe 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

Filename Overview
apps/sim/lib/mcp/oauth/storage.ts New file for DB persistence of MCP OAuth state; tokens and client info are encrypted correctly, PKCE verifier is encrypted, state stored as SHA-256 hash. Minor: loadOauthRowByState TTL driven by updatedAt which unrelated row writes can bump.
apps/sim/lib/mcp/oauth/provider.ts New SimMcpOauthProvider correctly implements OAuthClientProvider, delegates to encrypted storage helpers, supports both pre-registered and DCR clients.
apps/sim/app/api/mcp/oauth/callback/route.ts Burns state before token exchange, validates session user matches flow initiator, HTML-escapes all reflected values, uses postMessage with same-origin check.
apps/sim/app/api/mcp/oauth/start/route.ts Correctly uses withMcpAuth middleware, validates server authType, surfaces authorization URL via McpOauthRedirectRequired signal.
apps/sim/app/api/mcp/servers/route.ts POST handler probes new/URL-changed servers and preserves existing authType on same-URL edits; OAuth token cleanup correctly added to upsert transaction.
apps/sim/app/api/mcp/servers/[id]/route.ts PATCH handler wrapped in transaction, clears mcp_server_oauth when URL or OAuth credentials change, strips raw secret from response.
apps/sim/app/api/mcp/tools/execute/route.ts Correctly catches all OAuth error types returning structured reauth_required response with correct serverId; timeout handle properly cleared.
apps/sim/lib/mcp/service.ts Threads userId into createClient for OAuth servers; discoverServerTools and getServerSummaries handle UnauthorizedError and McpOauthAuthorizationRequiredError by marking servers disconnected.
apps/sim/lib/mcp/connection-manager.ts Cache keys correctly scoped per-user for OAuth servers; disconnectServer removes all per-user connections for a given server ID.
apps/sim/hooks/mcp/use-mcp-oauth-popup.ts Per-server connecting state tracked in a Set; setInterval polling cleaned up on unmount; postMessage origin check enforced.
apps/sim/hooks/queries/mcp.ts Query key structure refactored for hierarchical invalidation; useStartMcpOauth validates https scheme with localhost loopback exception before opening popup.
packages/db/schema.ts New mcp_server_oauth table with correct FK cascades, unique index on (mcpServerId, userId), and state lookup index.
apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx OAuth fields added with secretTouched tracking; connection test bypass heuristic is overly broad and could allow saving misconfigured header-auth servers.

Sequence Diagram

sequenceDiagram
    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
Loading

Reviews (24): Last reviewed commit: "fix(mcp): final audit fixes — state TTL,..." | Re-trigger Greptile

Comment thread apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx Outdated
Comment thread apps/sim/lib/mcp/oauth/storage.ts
Comment thread apps/sim/lib/mcp/oauth/storage.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

Greptile summary findings addressed in f587e82:

  • Edit modal drops existing OAuth Client ID: editInitialData now includes oauthClientId; the API already returns it (only the secret is masked) so the field populates and Advanced auto-expands.
  • Shared OAuth mutation disables all buttons: per-server pending tracked in a local Set<string>; the spinner is scoped to the card whose flow is in progress.
  • Plaintext PKCE codeVerifier: now encrypted at rest via encryptSecret to match tokens/clientInformation.

The point about clearing a pre-registered Client ID by emptying the field is a follow-up — oauthClientId || undefined collapses an intentional clear into a no-op. Will tackle when adding TTL cleanup for abandoned OAuth sessions.

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts
Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/[id]/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/servers/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/mcp/tools/execute/route.ts
Comment thread apps/sim/lib/mcp/service.ts Outdated
Comment thread apps/sim/lib/mcp/service.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

Comment thread apps/sim/app/api/mcp/oauth/callback/route.ts
waleedlatif1 and others added 22 commits May 6, 2026 18:36
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.
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant