Skip to content

fix(mcp): probe-based OAuth detection in test-connection#4689

Merged
waleedlatif1 merged 2 commits into
stagingfrom
waleedlatif1/mcp-oauth
May 21, 2026
Merged

fix(mcp): probe-based OAuth detection in test-connection#4689
waleedlatif1 merged 2 commits into
stagingfrom
waleedlatif1/mcp-oauth

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • test-connection runs detectMcpAuthType first; on 401 + resource_metadata (RFC 9728) it returns { success: false, authRequired: true, authType: 'oauth' } via the typed contract
  • Both modal submit paths (regular form + Edit JSON) consume the structured authRequired flag — removes the prior regex heuristic on error message strings
  • authType is threaded from test-connection through to the create call so performCreateMcpServer skips its own probe (one probe per add instead of two)
  • Unblocks spec-compliant OAuth MCP servers like Semrush, Linear, Notion, Atlassian, Asana via the "Edit JSON" path

Type of Change

  • Bug fix

Testing

Tested manually against https://mcp.semrush.com/v1/mcp end-to-end:

  • Add via Edit JSON → probe detects OAuth → modal proceeds → OAuth popup opens → consent → tokens stored
  • Add via regular form → same flow
  • Non-OAuth servers (Exa, Firecrawl) still surface the original error on real failures, no silent suppression

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 21, 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 21, 2026 2:29am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 21, 2026

PR Summary

Medium Risk
Changes the MCP server add/test flow to short-circuit on detected OAuth challenges and propagates authType through UI/API contracts; mistakes could misclassify servers and alter onboarding behavior.

Overview
test-connection now probes the target URL with detectMcpAuthType and, when an RFC 9728 OAuth challenge is detected, returns a structured success payload with authRequired/authType instead of attempting an unauthenticated MCP connect.

The MCP server modal (form + JSON import) consumes the new authRequired flag (removing string/regex heuristics) and threads authType through submit, with the API contract updated to type these new fields.

Reviewed by Cursor Bugbot for commit 54b6a50. Configure here.

@waleedlatif1 waleedlatif1 force-pushed the waleedlatif1/mcp-oauth branch from de3780c to 27a0cfd Compare May 21, 2026 02:18
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR replaces a fragile regex heuristic for OAuth detection (scanning error message strings for "401", "oauth", etc.) with a spec-compliant probe (detectMcpAuthType) that performs a real RFC 9728 WWW-Authenticate inspection on the test-connection route. The structured authRequired / authType fields are now part of the typed API contract and threaded from the test-connection response through both form-submit paths into the create call, reducing the total number of probes from two to one.

  • test-connection/route.ts: Calls detectMcpAuthType before attempting a full MCP client connection; short-circuits with { success: false, authRequired: true, authType: 'oauth' } (HTTP 200) when RFC 9728 OAuth is detected, otherwise stores the detected authType on the result for non-OAuth servers.
  • mcp-server-form-modal.tsx: Both submit paths (regular form + Edit JSON) now gate on connectionResult.authRequired instead of the old error-string regex; authType is forwarded into the onSubmit payload so the create call can skip its own probe.
  • contracts/mcp.ts + hooks/queries/mcp.ts: mcpServerTestResultSchema and McpServerInput / McpServerFormConfig gain authRequired/authType fields, and literal union type repetitions are replaced with the canonical McpAuthType import.

Confidence Score: 5/5

Safe to merge — the change is well-scoped, the probe is inserted after all existing SSRF/domain guards, and the structured contract replaces a fragile string-matching heuristic without altering any other behavior.

The probe correctly short-circuits only on an explicit RFC 9728 OAuth challenge (resource_metadata or scope in WWW-Authenticate), leaving all non-OAuth error paths untouched. The authType field flows cleanly from the probe through the contract and into both submit paths. No existing guards are bypassed and the error handling for real connection failures is preserved.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/api/mcp/servers/test-connection/route.ts Probe is inserted correctly after SSRF validation and env-var resolution; early-return on OAuth detection is clean and the authType is propagated on the happy path.
apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx Both form-submit and Edit JSON paths correctly gate on the structured authRequired flag; authType is forwarded into onSubmit on both paths.
apps/sim/hooks/queries/mcp.ts McpServerInput gains authType; error-path reconstruction in testMcpServerConnection omits authType/authRequired from the parsed 400 body (pre-existing, already flagged separately).
apps/sim/lib/api/contracts/mcp.ts mcpServerTestResultSchema cleanly extended with authRequired and authType; schema uses the shared mcpAuthTypeSchema enum, no issues.

Reviews (2): Last reviewed commit: "fix(mcp): guard undefined url in probe a..." | Re-trigger Greptile

Comment thread apps/sim/hooks/queries/mcp.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile @cursor review

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

@waleedlatif1 waleedlatif1 merged commit e27afaa into staging May 21, 2026
14 checks passed
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 54b6a50. Configure here.

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