Add MCP Connect button OAuth flow#251
Merged
Merged
Conversation
Add a generic tools/list probe that classifies MCP servers as connected, needing authentication, or errored, including WWW-Authenticate parsing for the OAuth handoff.
Track user-added MCP server status and tool counts in a local sidecar while keeping the runtime server config clean for agent consumption.
Move MCP server creation to a name-and-URL flow, probe immediately after create, and add endpoints for row refreshes and manual Bearer-token Connect.
Replace token entry during add with a row-level Connect action, state badges, tool-count display, retry controls, and the manual token fallback dialog.
Add chmod-protected MCP secret storage plus OAuth discovery, dynamic client registration, PKCE authorization URL construction, and token exchange utilities.
Add the Connect start endpoint that probes for OAuth metadata, performs dynamic client registration, stores PKCE state, and returns the authorization URL.
Handle authorization callbacks by validating state, exchanging PKCE codes, persisting tokens, probing the server, and notifying the settings popup opener.
Refresh expired OAuth access tokens inline when tools/list returns invalid_token, re-probe with the fresh token, and clear stale refresh tokens when recovery fails.
Fan out one probe per configured MCP server after settings load so connection state and tool counts refresh without manual row interaction.
Emit server-side telemetry for OAuth and token Connect starts, completions, and failure fallbacks so the flow is observable in production.
Add edge coverage for authorization servers without dynamic registration and ensure the internally managed composio MCP server cannot be mutated by user-server helpers.
Clear stale hydration tracking for MCP servers that are no longer rendered so deleting and re-adding the same key still triggers automatic probing.
Delete per-server OAuth sidecar entries when MCP servers are removed and clear callback-only redirect URIs after token exchange so stale auth state cannot linger.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: RFC 8414 metadata URL wrongly appended to issuer path
- Fixed buildMetadataUrls to insert /.well-known/oauth-authorization-server between the origin and path per RFC 8414 §3, instead of appending it to the end of the issuer URL.
- ✅ Fixed: Callback skips validation of
cached.asMetadataUrlbefore discovery- Added !cached.asMetadataUrl to the existing validation guard on line 247, consistent with the probe route's tryRefreshAfterInvalidToken check.
Or push these changes by commenting:
@cursor push 0ccd44c8f5
Preview (0ccd44c8f5)
diff --git a/apps/web/app/api/settings/mcp/connect/callback/route.ts b/apps/web/app/api/settings/mcp/connect/callback/route.ts
--- a/apps/web/app/api/settings/mcp/connect/callback/route.ts
+++ b/apps/web/app/api/settings/mcp/connect/callback/route.ts
@@ -244,7 +244,7 @@
}
const cached = getMcpServerSecret(serverKey);
- if (!cached || !cached.codeVerifier || !cached.redirectUri) {
+ if (!cached || !cached.codeVerifier || !cached.redirectUri || !cached.asMetadataUrl) {
return renderResultPage(
{
kind: "error",
diff --git a/apps/web/lib/mcp-oauth.ts b/apps/web/lib/mcp-oauth.ts
--- a/apps/web/lib/mcp-oauth.ts
+++ b/apps/web/lib/mcp-oauth.ts
@@ -261,10 +261,15 @@
}
function buildMetadataUrls(issuer: string): string[] {
- const trimmed = issuer.endsWith("/") ? issuer.slice(0, -1) : issuer;
+ // RFC 8414 §3: insert /.well-known/oauth-authorization-server between the
+ // origin and the path, e.g. https://example.com/tenant becomes
+ // https://example.com/.well-known/oauth-authorization-server/tenant
+ const url = new URL(issuer);
+ const path = url.pathname.replace(/\/+$/, "");
return [
- `${trimmed}/.well-known/oauth-authorization-server`,
- `${trimmed}/.well-known/openid-configuration`,
+ `${url.origin}/.well-known/oauth-authorization-server${path}`,
+ // OIDC Discovery appends to the issuer per OpenID Connect Discovery §4.
+ `${url.origin}${path}/.well-known/openid-configuration`,
];
}You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 5da08f9. Configure here.
| `${trimmed}/.well-known/oauth-authorization-server`, | ||
| `${trimmed}/.well-known/openid-configuration`, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
RFC 8414 metadata URL wrongly appended to issuer path
Medium Severity
buildMetadataUrls appends /.well-known/oauth-authorization-server to the end of the issuer URL. Per RFC 8414 §3, for path-based issuers like https://auth.example.com/tenant, the well-known URI is https://auth.example.com/.well-known/oauth-authorization-server/tenant, not https://auth.example.com/tenant/.well-known/oauth-authorization-server. This causes OAuth discovery to fail for any authorization server with a non-root issuer path.
Reviewed by Cursor Bugbot for commit 5da08f9. Configure here.
| }, | ||
| targetOrigin, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Callback skips validation of cached.asMetadataUrl before discovery
Low Severity
The guard on line 247 checks cached.codeVerifier and cached.redirectUri but not cached.asMetadataUrl. Since emptySecret() initializes asMetadataUrl to "", a corrupted or partially-written secrets entry could pass validation and then feed an empty string to discoverOAuthMetadata, producing a confusing "network_error" instead of a clear "missing_pkce"-style diagnostic. The probe route's tryRefreshAfterInvalidToken correctly checks !cached.asMetadataUrl for the same field.
Reviewed by Cursor Bugbot for commit 5da08f9. Configure here.
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Or push these changes by commenting: Preview (dcc2952495)diff --git a/apps/web/lib/mcp-secrets.ts b/apps/web/lib/mcp-secrets.ts
--- a/apps/web/lib/mcp-secrets.ts
+++ b/apps/web/lib/mcp-secrets.ts
@@ -184,6 +184,7 @@
...current,
codeVerifier: null,
oauthState: null,
+ redirectUri: null,
};
writeAll(all);
}
diff --git a/apps/web/lib/mcp-servers.ts b/apps/web/lib/mcp-servers.ts
--- a/apps/web/lib/mcp-servers.ts
+++ b/apps/web/lib/mcp-servers.ts
@@ -1,5 +1,6 @@
import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
import { join } from "node:path";
+import { deleteMcpServerSecret } from "@/lib/mcp-secrets";
import { resolveOpenClawStateDir } from "@/lib/workspace";
type UnknownRecord = Record<string, unknown>;
@@ -470,4 +471,6 @@
delete states[normalizedKey];
writeStatesSidecar(states);
}
+
+ deleteMcpServerSecret(normalizedKey);
}You can send follow-ups to the cloud agent here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
pnpm --dir apps/web exec vitest run lib/mcp-probe.test.ts lib/mcp-oauth.test.ts lib/mcp-servers.test.ts app/api/settings/mcp/route.test.ts app/api/settings/mcp/probe/route.test.ts app/api/settings/mcp/connect/token/route.test.ts app/api/settings/mcp/connect/start/route.test.ts app/api/settings/mcp/connect/callback/route.test.tsNote
High Risk
Adds new OAuth/token-based connection endpoints and local secret/state persistence for MCP servers, which touches authentication headers and token handling. Incorrect validation/storage/probing could break connectivity or leak tokens/state despite added tests and escaping.
Overview
Adds a Cursor-style MCP “Add then Connect” flow: server creation no longer accepts an auth token and now immediately probes the server to populate persisted connection state.
Introduces new Connect APIs:
POST /api/settings/mcp/connect/startto probe and (when supported) run OAuth discovery + dynamic client registration + PKCE,GET /api/settings/mcp/connect/callbackto exchange the code, persist refresh tokens, updateAuthorizationheaders, probe, andpostMessageresults back to the opener, andPOST /api/settings/mcp/connect/tokenas a manual token fallback that saves the Bearer header then re-probes.Adds supporting libraries for probing (
tools/listover JSON/SSE with RFC 9728WWW-Authenticateparsing), OAuth client utilities (discovery, DCR, PKCE, code exchange, refresh, expiry computation), and sidecar persistence for secrets (.mcp-secrets.json, 0600) and per-server status (.mcp-states.json), including cleanup on server removal and refresh-on-invalid_tokenduring probes.Updates the settings UI to show per-server status/tool counts, add a Connect button with OAuth popup handling + timeout, fallback token dialog, and automatic hydration probes; expands test coverage across routes and new libs.
Reviewed by Cursor Bugbot for commit 5da08f9. Bugbot is set up for automated code reviews on this repo. Configure here.