fix(mcp-oauth): let external OAuth clients use aggregate/virtual MCP endpoints#4263
fix(mcp-oauth): let external OAuth clients use aggregate/virtual MCP endpoints#4263AriOliv wants to merge 5 commits into
Conversation
…endpoints
External MCP clients (Claude Desktop/Code, any RFC 9728 client) could not
connect to an org's aggregate (`/api/:org/mcp`) or virtual-MCP endpoints:
- The aggregate exposed no oauth-protected-resource metadata (404), and virtual
MCPs tried to *proxy* a `virtual://` downstream authorization server and 502'd
("protocol must be http/https/s3"). Neither advertised Studio's own Better
Auth MCP authorization server (which supports Dynamic Client Registration), so
external clients had no auth server that would accept their own redirect_uri.
The connection `oauth-proxy` only accepts Studio's own origin, so it can't
serve external clients.
- `WWW-Authenticate` advertised an `http://` resource_metadata URL behind a
TLS-terminating reverse proxy; https-only clients (e.g. Claude) reject it.
- MCP OAuth sessions resolved the member role from `x-org-*` headers or the
user's single membership. External clients send neither and the org is in the
URL path, so multi-org members resolved to no role, lost the owner/admin
bypass, and every connection tool call 403'd `Access denied to: <tool>`.
Fixes:
- api/app.ts (mcpAuth): honor `X-Forwarded-Proto` when building the
resource_metadata origin so it advertises https behind a proxy.
- api/routes/org-scoped.ts: serve Better Auth protected-resource metadata for
the aggregate `/api/:org/mcp/.well-known/oauth-protected-resource`.
- api/routes/oauth-proxy.ts: for `virtual://` connections, return Better Auth
metadata instead of proxying a nonexistent downstream AS.
- core/context-factory.ts: derive an org-slug hint from the request path
(`/api/:org/...`) for MCP OAuth membership/role resolution.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The oauth-proxy hardcoded the RFC 8707 `resource` parameter to `connection.connection_url` when forwarding the authorize/token legs to a downstream MCP's authorization server. This is correct for servers that validate the resource equals their exact endpoint (e.g. Supabase), but breaks servers that only accept the origin: Pipedream (`https://mcp.pipedream.net/v2`) rejects the path-bearing resource with `invalid_request: resource: Invalid or unauthorized resource parameter`, and gates its protected-resource metadata so RFC 9728 discovery can't resolve the canonical value either. Forward `resource = connection.metadata.oauthResource ?? connection.connection_url`, computed once and reused on both the authorize redirect and the token form-body rewrite. Endpoint-strict servers keep the default; origin-only servers set `metadata.oauthResource` (e.g. `https://mcp.pipedream.net`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-user auth Two fixes that together unblock per-user OAuth connections whose downstream is a strict OAuth server (e.g. Pipedrive's official MCP, mcp.pipedrive.ai): - web/connection connect: stop hardcoding scope "offline_access" in the DCR/authorize call. Many MCP providers don't advertise it, and passing it into Dynamic Client Registration makes strict servers reject /register with HTTP 400 (Pipedrive does). Use the connection's configured scopes when set, else omit scope (refresh is already requested via grant_types). - lazy-client: do not count PerUserAuthorizationRequiredError as a circuit- breaker failure. A per_user connection without a token for the caller throwing "needs authorization" is an expected state, not a downstream outage. Counting it opened the breaker, which 503'd the connection and hid the "Connect your account" UI — blocking the very OAuth the error asks for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/mcp-clients/lazy-client.ts">
<violation number="1" location="apps/mesh/src/mcp-clients/lazy-client.ts:128">
P1: The added suppression prevents per-user auth errors from tripping the circuit breaker, which is good, but it doesn't help when the circuit is already open. Because `assertCircuitClosed(connection.id)` runs before `clientFromConnection`, any caller whose connection already has an open circuit gets a 503 and never reaches the per-user auth prompt—even though their caller merely needs to authorize. Since the breaker is keyed by `connection.id` and shared across all users, a prior unrelated downstream failure can hide the "Connect your account" flow from every subsequent caller. Consider elevating the per-user-auth bypass so it also skips an already-open circuit, or segment the breaker state so expected auth prompts aren't blocked by unrelated outages.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // NOT a downstream outage. Counting it as a failure trips the circuit | ||
| // breaker, which then 503s the connection — hiding the "Connect your | ||
| // account" UI and blocking the very OAuth the error is asking for. | ||
| if (!isPerUserAuthorizationRequiredError(err)) { |
There was a problem hiding this comment.
P1: The added suppression prevents per-user auth errors from tripping the circuit breaker, which is good, but it doesn't help when the circuit is already open. Because assertCircuitClosed(connection.id) runs before clientFromConnection, any caller whose connection already has an open circuit gets a 503 and never reaches the per-user auth prompt—even though their caller merely needs to authorize. Since the breaker is keyed by connection.id and shared across all users, a prior unrelated downstream failure can hide the "Connect your account" flow from every subsequent caller. Consider elevating the per-user-auth bypass so it also skips an already-open circuit, or segment the breaker state so expected auth prompts aren't blocked by unrelated outages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/mcp-clients/lazy-client.ts, line 128:
<comment>The added suppression prevents per-user auth errors from tripping the circuit breaker, which is good, but it doesn't help when the circuit is already open. Because `assertCircuitClosed(connection.id)` runs before `clientFromConnection`, any caller whose connection already has an open circuit gets a 503 and never reaches the per-user auth prompt—even though their caller merely needs to authorize. Since the breaker is keyed by `connection.id` and shared across all users, a prior unrelated downstream failure can hide the "Connect your account" flow from every subsequent caller. Consider elevating the per-user-auth bypass so it also skips an already-open circuit, or segment the breaker state so expected auth prompts aren't blocked by unrelated outages.</comment>
<file context>
@@ -119,7 +120,14 @@ export function createLazyClient(
+ // NOT a downstream outage. Counting it as a failure trips the circuit
+ // breaker, which then 503s the connection — hiding the "Connect your
+ // account" UI and blocking the very OAuth the error is asking for.
+ if (!isPerUserAuthorizationRequiredError(err)) {
+ recordFailure(connection.id);
+ }
</file context>
…ired The MCP proxy route's inner catch ran recordFailure + auto-disable before the error reached the outer handleError (which already renders per-user auth as a 401). For an `auth_mode: "per_user"` connection whose caller has no token yet, the handshake throws PerUserAuthorizationRequiredError — an expected state — so the inner catch was opening the breaker and 503'ing the connection (and, in an aggregate, crashing the whole agent's tool calls). Return renderPerUserAuthorizationRequired(error) at the top of the inner catch, before recordFailure, so the caller gets the OAuth challenge without tripping the breaker or disabling the connection. Complements the lazy-client guard (client- creation path); this covers the proxy handshake/call path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/api/routes/proxy.ts">
<violation number="1" location="apps/mesh/src/api/routes/proxy.ts:421">
P1: The `isPerUserAuthorizationRequiredError(error)` early return gates the circuit-breaker failure accounting and auto-disable logic. If that cross-file classifier is overly broad (e.g., message-substring or regex based), non-auth downstream failures could be misclassified as expected per-user auth states. That would suppress `recordFailure(...)` and `shouldDisable`, hiding real outages behind repeated 401 responses and silently disabling circuit protection. Given the project convention [ID: 9aca6f72-ab03-43f2-bc16-ef06243974f8] to use exact message equality for internally-controlled errors, verify the classifier uses strict equality rather than substring/regex matching to prevent misclassification.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // URL) WITHOUT tripping the breaker or auto-disabling the connection. | ||
| // Handled before recordFailure below, which would otherwise open the | ||
| // circuit and 503 the connection (hiding the OAuth the error asks for). | ||
| if (isPerUserAuthorizationRequiredError(error)) { |
There was a problem hiding this comment.
P1: The isPerUserAuthorizationRequiredError(error) early return gates the circuit-breaker failure accounting and auto-disable logic. If that cross-file classifier is overly broad (e.g., message-substring or regex based), non-auth downstream failures could be misclassified as expected per-user auth states. That would suppress recordFailure(...) and shouldDisable, hiding real outages behind repeated 401 responses and silently disabling circuit protection. Given the project convention [ID: 9aca6f72-ab03-43f2-bc16-ef06243974f8] to use exact message equality for internally-controlled errors, verify the classifier uses strict equality rather than substring/regex matching to prevent misclassification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/api/routes/proxy.ts, line 421:
<comment>The `isPerUserAuthorizationRequiredError(error)` early return gates the circuit-breaker failure accounting and auto-disable logic. If that cross-file classifier is overly broad (e.g., message-substring or regex based), non-auth downstream failures could be misclassified as expected per-user auth states. That would suppress `recordFailure(...)` and `shouldDisable`, hiding real outages behind repeated 401 responses and silently disabling circuit protection. Given the project convention [ID: 9aca6f72-ab03-43f2-bc16-ef06243974f8] to use exact message equality for internally-controlled errors, verify the classifier uses strict equality rather than substring/regex matching to prevent misclassification.</comment>
<file context>
@@ -412,6 +412,15 @@ export const createProxyRoutes = () => {
+ // URL) WITHOUT tripping the breaker or auto-disabling the connection.
+ // Handled before recordFailure below, which would otherwise open the
+ // circuit and 503 the connection (hiding the OAuth the error asks for).
+ if (isPerUserAuthorizationRequiredError(error)) {
+ return renderPerUserAuthorizationRequired(error);
+ }
</file context>
The GatewayClient namespaced each aggregated tool as
`${slugify(connectionId)}_${toolName}`. A connection id slug is ~26 chars, so
when a downstream MCP client adds its OWN prefix (e.g. Hermes prepends
`mcp_<server>_`, ~21 chars) the combined name blew past the 64-char tool-name
limit (`^[A-Za-z0-9_-]{1,64}$`) — ~40 of 91 tools in a 3-connection aggregate
were rejected.
Replace the slug prefix with `namespaceCode()`: a 7-char stable FNV-1a hash
(`a` + 6 base36, no underscore, so resolveToolTarget's split on the first `_`
still works). Worst case drops from ~81 to ~62 chars, fitting even with a second
client prefix. Reversible via the same code in stripToolNamespace + the
slugToKey map; role permissions and selected_tools are unaffected (they key on
connection id, not the namespaced tool name). Aggregated tool names change
(clients re-list on handshake, so it's transparent).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/mcp-utils/src/aggregate/gateway-client.ts">
<violation number="1" location="packages/mcp-utils/src/aggregate/gateway-client.ts:115">
P1: Switching the namespace prefix from a `slugify`-based slug to a hash-based `namespaceCode` breaks resolution for any persisted namespaced identifiers. The `resolveToolTarget`/`resolvePromptTarget` fallback only handles un-namespaced names and new-format namespaced names, but not old-format namespaced names. If external systems or saved configurations store namespaced tool/prompt names (e.g., workflow steps, allow-lists, cached selections), they will fail to resolve after this change.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ): string { | ||
| if (!clientId) return namespacedName; | ||
| const prefix = `${slugify(clientId)}_`; | ||
| const prefix = `${namespaceCode(clientId)}_`; |
There was a problem hiding this comment.
P1: Switching the namespace prefix from a slugify-based slug to a hash-based namespaceCode breaks resolution for any persisted namespaced identifiers. The resolveToolTarget/resolvePromptTarget fallback only handles un-namespaced names and new-format namespaced names, but not old-format namespaced names. If external systems or saved configurations store namespaced tool/prompt names (e.g., workflow steps, allow-lists, cached selections), they will fail to resolve after this change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/mcp-utils/src/aggregate/gateway-client.ts, line 115:
<comment>Switching the namespace prefix from a `slugify`-based slug to a hash-based `namespaceCode` breaks resolution for any persisted namespaced identifiers. The `resolveToolTarget`/`resolvePromptTarget` fallback only handles un-namespaced names and new-format namespaced names, but not old-format namespaced names. If external systems or saved configurations store namespaced tool/prompt names (e.g., workflow steps, allow-lists, cached selections), they will fail to resolve after this change.</comment>
<file context>
@@ -89,7 +112,7 @@ export function stripToolNamespace(
): string {
if (!clientId) return namespacedName;
- const prefix = `${slugify(clientId)}_`;
+ const prefix = `${namespaceCode(clientId)}_`;
return namespacedName.startsWith(prefix)
? namespacedName.slice(prefix.length)
</file context>
|
Thanks for the contribution @AriOliv ! One thing: did you forget to add an Claude: 🔴 Critical finding: this PR does not compile The per-user-auth robustness fix (#5) references two functions and one module that don't exist anywhere on the PR branch:
There is no definition of either function anywhere in the branch. The author almost certainly forgot to git add a new outbound/errors.ts. Consequences:
The only green check on the PR is cubic · AI code reviewer; the actual typecheck/test CI either isn't wired to this PR or hasn't reported — the PR is mergeStateStatus: BLOCKED. This must be fixed before any merge. Ask AriOliv for the missing outbound/errors.ts. |
…e modal Adds a prominent "LINK" button to the app topbar (desktop) that opens a focused "Connect to Claude" modal built around a single action: - Claude Code: one button copies the `claude mcp add … <org-mcp-url>` one-liner (paste in a terminal; OAuth on first use). - Claude Desktop / claude.ai: copy the aggregate MCP URL to add as a custom connector. The modal spells out what Claude gets — Library files, agents, and the ability to enable + call any MCP tool in the org — since the unified `/api/:org/mcp` endpoint already exposes all of it behind OAuth 2.1. Also: - Extract mcpUrl/claudeCodeCommand into components/connect/mcp-url.ts and reuse from the Connect settings page so the two can't drift. - Fix the advertised OAuth protected-resource metadata URL on the Connect settings page: it's served at the aggregate endpoint (`/api/:org/mcp/.well-known/oauth-protected-resource`), not the origin root. Matches the backend contract in #4263. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only request scopes the connection has explicitly configured. offline_access is an OIDC-ism many MCP providers don't advertise; passing it into Dynamic Client Registration makes strict servers reject the registration outright (e.g. Pipedrive returns HTTP 400 on /register). Refresh tokens are already requested via grant_types, so omitting scope lets such servers grant their default set. Cherry-picked from #4263 (AriOliv). The paired circuit-breaker change to lazy-client.ts is omitted here — it depends on an outbound/errors helper that isn't yet on main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e modal Adds a prominent "LINK" button to the app topbar (desktop) that opens a focused "Connect to Claude" modal built around a single action: - Claude Code: one button copies the `claude mcp add … <org-mcp-url>` one-liner (paste in a terminal; OAuth on first use). - Claude Desktop / claude.ai: copy the aggregate MCP URL to add as a custom connector. The modal spells out what Claude gets — Library files, agents, and the ability to enable + call any MCP tool in the org — since the unified `/api/:org/mcp` endpoint already exposes all of it behind OAuth 2.1. Also: - Extract mcpUrl/claudeCodeCommand into components/connect/mcp-url.ts and reuse from the Connect settings page so the two can't drift. - Fix the advertised OAuth protected-resource metadata URL on the Connect settings page: it's served at the aggregate endpoint (`/api/:org/mcp/.well-known/oauth-protected-resource`), not the origin root. Matches the backend contract in #4263. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only request scopes the connection has explicitly configured. offline_access is an OIDC-ism many MCP providers don't advertise; passing it into Dynamic Client Registration makes strict servers reject the registration outright (e.g. Pipedrive returns HTTP 400 on /register). Refresh tokens are already requested via grant_types, so omitting scope lets such servers grant their default set. Cherry-picked from #4263 (AriOliv). The paired circuit-breaker change to lazy-client.ts is omitted here — it depends on an outbound/errors helper that isn't yet on main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
External MCP clients (Claude Desktop / Claude Code, or any RFC 9728 / MCP OAuth client) can't connect to an org's aggregate (
/api/:org/mcp) or virtual-MCP endpoints and call tools. Three independent causes:No usable OAuth authorization server advertised.
oauth-protected-resourcemetadata →404.connection_urlisvirtual://<id>— nothing to fetch →502(protocol must be http/https/s3).oauth-proxyonly accepts Studio's ownredirect_uri, so it can't serve an external client that brings its own callback →invalid_request: redirect_uri is not allowed.WWW-Authenticateadvertises anhttp://resource_metadataURL when Studio runs behind a TLS-terminating reverse proxy (Caddy/nginx forward over http). https-only clients (e.g. Claude) reject the non-https metadata URL.MCP OAuth sessions resolve the member role from the wrong org. External clients don't send
x-org-id/x-org-slug; the org is in the URL path (/api/:org/...). A multi-org member falls through to the "single membership only" guard, resolves to no role, loses the owner/admin bypass, and every connection tool call403sAccess denied to: <tool>.Fix
api/app.ts(mcpAuth)X-Forwarded-Protowhen building theresource_metadataorigin so it advertiseshttps://behind a proxy.api/routes/org-scoped.ts/api/:org/mcp/.well-known/oauth-protected-resource(mounted before the proxy catch-all).api/routes/oauth-proxy.tsvirtual://connections, return Better Auth metadata instead of proxying a nonexistent downstream AS.core/context-factory.tsNet effect: an external client discovers Studio's own Better Auth AS, registers via DCR (its own
redirect_uriaccepted), logs in against Studio, and the token resolves to the caller's real role in the path org. Downstream per-user OAuth still happens lazily when aper_userconnection's tool is first invoked. RBAC is unchanged —owner/adminbypass; non-admins still need an explicit connection grant.Testing
Validated end-to-end against a deployed Studio (behind Caddy TLS) with Claude Code:
POST /api/:org/mcp/<virtual-mcp-id>→401+WWW-Authenticatewith an httpsresource_metadata.200advertising Studio's Better Auth AS (authorization_servers: [<studio>]).claude.airedirect_uri→ accepted;/authorize→302(noredirect_uri is not allowed).403 Access denied).🤖 Generated with Claude Code
Summary by cubic
Enable external MCP OAuth clients (e.g. Claude) to connect to org aggregate
/api/:org/mcpand virtual MCP endpoints. Also shorten aggregated tool name prefixes so names fit the 64-char limit.better-authprotected-resource metadata for aggregate andvirtual://MCP endpoints, enabling DCR and externalredirect_uris.X-Forwarded-Protowhen buildingresource_metadatasohttpsis advertised behind proxies./api/:org/...path when headers are absent to fix RBAC for multi-org members.resourceviaconnection.metadata.oauthResource; default toconnection.connection_url.offline_access; return the 401 challenge without recording failures in both lazy-client and proxy paths.Written for commit e51ad76. Summary will update on new commits.