Skip to content

fix(mcp): recover cleanly from OAuth failures#5595

Open
j15z wants to merge 5 commits into
stagingfrom
fix/mcp-page
Open

fix(mcp): recover cleanly from OAuth failures#5595
j15z wants to merge 5 commits into
stagingfrom
fix/mcp-page

Conversation

@j15z

@j15z j15z commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR makes MCP authentication and refresh failures recover cleanly instead of leaving Workspace Settings in repeated discovery loops or generic error states. Abandoned OAuth servers now wait for an explicit reconnect, failed refreshes expose a retryable Failed state, and persisted connection errors reach the list and detail views.

Fixes #5665.

Dual-auth servers can now be added with a configured static Authorization: Bearer ... header: the connection test tries that credential before optional OAuth discovery. Supplemental non-auth headers still follow OAuth discovery, preserving OAuth servers that need routing or metadata headers.

Connection attempts honor the configured timeout, and diagnostics record timing, phase, outcome, auth type, and header names without header values. Connection-test and refresh failures now use safe categorized messages so an upstream server cannot reflect a configured credential into API responses, persisted status, or logs. OAuth-pending servers remain disconnected without negative caching; rejected static credentials increment the normal failure state and negative cache instead of being mislabeled as OAuth-pending. Failed refreshes cannot report a stale connected row unless a newer successful discovery actually won the race.

Merge order

Merge this PR before #5596. Once it lands in staging, rebase #5596 onto the updated base and resolve its verification path against these authentication, status-persistence, and negative-cache semantics.

Related: #5596

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Other: ___________

Testing

  • The original PR validation passed 25 focused MCP/API test files (375 tests) and the React Query pattern audit.
  • The final post-merge validation passed 13 focused MCP/API/UI/security test files (220 tests), including valid and rejected Bearer credentials, headerless OAuth, OAuth with supplemental headers, post-resolution SSRF blocking, reflected-secret handling, static-401 failure persistence, refresh race handling, and settings refresh states.
  • API validation audit passed with all 949 routes Zod-backed.
  • Biome passed for the changed and conflict-resolved files, and the full TypeScript check passed.

Review focus: static Bearer precedence, preservation of OAuth-plus-supplemental-header behavior, safe failure-state persistence, and the credential-reflection boundary.

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)

Post-Deploy Monitoring & Validation

  • Search application logs for McpClient, McpService, authorization_required, timeout, and OAuth authorization pending during the first 24 hours.
  • Healthy signals: abandoned OAuth servers stay disconnected without repeated discovery failures; static Bearer dual-auth servers connect; successful reconnects clear failure counters; no header values appear in MCP diagnostics.
  • Failure signals: renewed discovery loops, static Bearer servers being redirected into OAuth, OAuth servers with supplemental headers failing connection tests, or credential fragments appearing in responses or logs.
  • Roll back if MCP list availability regresses or refresh failures stop exposing a safe actionable error. Owner: MCP/settings team during the first business day after deployment.

Screenshots/Videos

Not applicable; this changes connection behavior and failure handling without altering layout.


Compound Engineering
Codex

@vercel

vercel Bot commented Jul 11, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview, Comment Jul 15, 2026 4:41pm

Request Review

@cursor

cursor Bot commented Jul 11, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches MCP auth, discovery, refresh races, and credential handling in APIs and logs; behavior is heavily tested but wrong OAuth/Bearer precedence or status races could affect production connections.

Overview
MCP connection handling now fails more safely and surfaces clearer states in Workspace Settings instead of looping discovery or showing generic errors.

Discovery and refresh no longer overwrite connection status in the refresh API; status comes from the service layer with race handling when a newer success lands during a failed refresh. OAuth-pending servers stay disconnected without negative caching; header-auth 401 maps to a generic authentication failure; repeated failures can promote to error.

Test connection tries a configured static Authorization: Bearer token before OAuth discovery; supplemental headers still use OAuth when there is no Bearer. Connection errors returned to clients and logs use an allowlisted message set so upstream bodies cannot echo secrets.

Client honors configured initialize timeouts and logs structured diagnostics (header names only, not values). React Query skips auto-discovery for disconnected OAuth servers and refreshes the server list when connected OAuth discovery fails.

Settings UI shows persisted errors and disconnected vs error states, a retryable red Failed refresh action, and shared helpers for tool labels and refresh button state.

Reviewed by Cursor Bugbot for commit 7b2378c. Bugbot is set up for automated code reviews on this repo. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves MCP OAuth recovery and refresh feedback. The main changes are:

  • Skips automatic tool discovery for OAuth servers waiting on reconnect.
  • Persists MCP discovery failures and refresh results for the settings UI.
  • Adds clearer refresh button states and server error labels.
  • Adds connection diagnostics with timing, phase, outcome, auth type, and header names.

Confidence Score: 4/5

The changed MCP failure paths need fixes before merging.

  • Non-OAuth 401 responses can be persisted as OAuth-pending with no actionable error.
  • New diagnostic logging can expose secret prefixes from truncated external error text.
  • The UI-only refresh state changes look contained.

apps/sim/lib/mcp/service.ts and apps/sim/lib/mcp/client.ts

Security Review

The new MCP diagnostics can log truncated external error text before redaction has safely removed secret fragments.

Important Files Changed

Filename Overview
apps/sim/lib/mcp/service.ts Adds MCP status persistence and OAuth-pending handling, but the shared auth-error helper can hide non-OAuth 401 credential failures.
apps/sim/lib/mcp/client.ts Adds connect timeout handling and structured diagnostics, but the new error logging can expose truncated secret fragments.
apps/sim/hooks/queries/mcp.ts Stops auto-discovery for disconnected OAuth servers while keeping non-OAuth discovery behavior.
apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx Uses helper functions for MCP refresh action state and server tool labels.
apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx Adds optional error-tone rendering for settings header actions.

Reviews (1): Last reviewed commit: "fix(mcp): improve OAuth failure recovery" | Re-trigger Greptile

Comment thread apps/sim/lib/mcp/service.ts Outdated
Comment thread apps/sim/lib/mcp/client.ts Outdated
Comment thread apps/sim/hooks/queries/mcp.ts Outdated
Comment thread apps/sim/app/api/mcp/servers/[id]/refresh/route.ts
j15z added 2 commits July 14, 2026 11:17
Treat static-header 401s as credential failures, keep OAuth failures pending, and prevent failed refreshes or reflected upstream errors from masquerading as connected state.
# Conflicts:
#	apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx
#	apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7b2378c. Configure here.

Comment thread apps/sim/lib/mcp/service.ts
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