fix(mcp): build RFC 8414-compliant well-known discovery URLs for OAuth login - #875
Open
haniakrim wants to merge 7 commits into
Open
fix(mcp): build RFC 8414-compliant well-known discovery URLs for OAuth login#875haniakrim wants to merge 7 commits into
haniakrim wants to merge 7 commits into
Conversation
…h login MCP OAuth login always returned a 500 for any server URL with a path component (e.g. https://host/mcp), which is nearly all real MCP servers. discover_endpoints() appended /.well-known/... after the full server URL, producing https://host/mcp/.well-known/ oauth-authorization-server — which 404s against any RFC 8414- compliant authorization server, since the well-known suffix must be inserted right after the origin, with the original path appended after it (https://host/.well-known/oauth-authorization-server/mcp). Extract the URL-building into a pure, unit-tested well_known_urls() helper and try both the RFC 8414 path-aware form and a bare-origin fallback for servers that only publish there. Verified against https://mcp.higgsfield.ai/mcp: the old code's URL 404s, the new code's first candidate returns 200 with valid metadata.
Even with a correct discovery URL, MCP OAuth login could never actually complete on a remote/web deployment: prepare_login_flow bound a TCP listener on 127.0.0.1 and built a redirect_uri pointing at it, then login() tried to open a system browser and blocked up to 120s waiting for a redirect on that listener. None of that is reachable from outside the process it runs in - not from the OAuth provider, not from a user's actual browser, not from anywhere but this exact container. This only ever worked when the server and the browser completing OAuth were the same machine (desktop/Electron). Replace it with the standard pattern: the redirect_uri is this server's own public origin (derived from the request's Origin/Host header) plus a new GET /api/mcp/oauth/callback route on the same already-listening HTTP server. login() returns immediately with the authorize_url instead of blocking; the caller sends a browser there; the OAuth provider's redirect naturally reaches this server wherever it's actually reachable from, same as any other request. Correlation still runs through the same (user_id, csrf_state)-keyed pending map - only how the callback arrives changed, not the security model. Removes the now-dead per-login TcpListener/raw-HTTP-parsing code (wait_for_callback, handle_callback_connection, parse_callback_query, url_decode) and the redundant csrf_token field it stored (the (user_id, state) map key already is the CSRF check).
The callback page told the user to close the tab manually, leaving them stranded outside the app after a full-page redirect through the authorize_url. Add a 2s meta-refresh back to the app root so the common case (things worked) returns them automatically.
…ject unsafe OAuth endpoint schemes The well_known_urls()/WellKnownCandidates fix from the first commit on this branch was silently reverted in the working tree by a `git checkout main -- .` run between that commit and the next one, and the regression went uncaught because its own tests were reverted with it (the passing "0 failed" test runs afterward just meant those specific assertions no longer existed, not that they passed). This restores that fix intact, with its 5 original regression tests. Also fixes a real finding from an independent security review of the authorize_url-navigation change: `authorization_endpoint` and `token_endpoint` come from the MCP server's own self-published OAuth discovery document, so they're attacker-influenced input, not values this server generates. Without a scheme check, a malicious or compromised MCP server could publish a `javascript:`/`data:` URL as its authorization_endpoint, which would flow through unchanged into the authorize_url returned to the client and executed by `window.location.href = authorize_url` on the frontend. Reject anything but http(s) centrally in fetch_metadata, once, rather than at each of the several downstream places that build a client or hand a URL to a browser from these fields.
Login against Higgsfield's MCP server (Clerk-backed auth) failed at the
authorize step with "invalid_client ... The requested OAuth 2.0 Client
does not exist" — aioncore always used a fixed shared client_id
("aionui") for every MCP server's OAuth flow, but Higgsfield's
discovery document advertises a registration_endpoint, meaning RFC
7591 Dynamic Client Registration is mandatory there, not optional; a
static client_id is simply never recognized.
When discovery returns a registration_endpoint, register a client
against it (public client, PKCE-secured, no secret requested) and use
the issued client_id (and secret, if the server returns one anyway)
for both the authorize_url and the later token exchange - a mismatch
between the two would fail the same way. Servers without a
registration_endpoint keep using DEFAULT_CLIENT_ID exactly as before.
Registered clients are cached in memory only, not persisted: adding a
new DB migration for this felt like the wrong tradeoff to reach for
again in the same session a migration-version mismatch already caused
a production crash loop. A process restart re-registers, which RFC
7591 is designed to tolerate.
Author
Update: this PR now covers three fixes, verified end-to-end live
Also fixed a real finding from an independent security review: Full sign-in verified live: 314 lib tests passing (up from 294 at the start of this PR — 20 new regression tests across all three fixes), |
Login worked, the token got stored — and nothing ever used it. POST /api/mcp/oauth/login through the callback correctly stores an access token, but when the session stack actually builds an HTTP/SSE MCP server's transport for an agent to call its tools, that path (mcp_resolve::row_to_session_mcp_server) only ever forwards a server's static configured headers. There is no OAuth lookup anywhere in it. check_oauth_status can report authenticated=true while every real tool call the agent makes still gets no Authorization header at all and fails as unauthenticated — OAuth login was, in effect, decorative for headless/agent tool use up to this point. Thread an OAuth token repository through the same path mcp_server_repo already takes (AgentFactoryDeps -> SessionBuildInputs -> resolve_session_mcp_servers -> row_to_session_mcp_server), and attach a stored, non-expired token as `Authorization: Bearer <token>` when building an HTTP/SSE transport. A server-configured Authorization header (case-insensitive) always wins over an OAuth token — a user who set one explicitly presumably knows what they're doing. Expired tokens are omitted rather than sent and rejected: refresh happens lazily via the check-status/get-token API paths, not this session-build path, so an unrefreshed expired token would just fail auth anyway. 5 new tests: token attached when valid, omitted when expired, omitted with no repo/no stored token, and the header-precedence case.
…ests
The claude/codex/antigravity backends resolve MCP servers through
mcp_resolve::row_to_session_mcp_server, which now attaches a stored
OAuth bearer token to HTTP/SSE transports. The aionrs ("Nabd CLI")
backend has its own separate MCP-loading path in
factory/aionrs.rs::row_to_mcp_server_config that never got the same
treatment, so an OAuth-authenticated MCP server (e.g. Higgsfield)
would work from claude/codex but still call tools unauthenticated
from aionrs, the default chat mode.
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.
Closes #874
Summary
Two related fixes to make MCP OAuth login actually work — it currently fails on every deployment where the server and the browser completing OAuth aren't the same machine, and it 500s outright for the vast majority of real MCP servers even before that.
Fix 1: RFC 8414-compliant discovery URL (
discover_endpoints)POST /api/mcp/oauth/loginreturns a generic 500 for any MCP server whose URL has a path component (e.g.https://mcp.higgsfield.ai/mcp) — which is nearly all real-world MCP servers.discover_endpoints()built the well-known discovery URL by appending/.well-known/oauth-authorization-serverdirectly after the full server URL:For
server_url = "https://mcp.higgsfield.ai/mcp", this produces:Per RFC 8414 §3.1, when the issuer URL has a path component, the well-known suffix must be inserted right after the origin, with the original path appended after it:
The 404 mapped to
McpError::OAuth(...)→ApiError::Internal(...), which by design returns a generic"Internal server error."to the client, masking the real cause entirely.Fix: extracted a pure
well_known_urls()helper that builds the RFC 8414 path-aware candidate first, falling back to the bare-origin form for servers that only publish there.Verified against the real
https://mcp.higgsfield.ai/mcpserver: old URL 404s, new URL's first candidate returns 200 with validauthorization_endpoint/token_endpoint. Also verified live: after deploying this fix alone, the 500 was gone and discovery/authorize-URL construction succeeded — but login still couldn't complete, which is fix 2.Fix 2: route the OAuth callback through this server, not localhost
Even with a correct discovery URL, login could never actually complete on a remote/web deployment.
prepare_login_flowbound aTcpListeneron127.0.0.1:0and set that as the OAuthredirect_uri, thenlogin()tried to open a system browser and blocked up to 120s waiting for a redirect on that listener. None of that is reachable from outside the process it runs in — not from the OAuth provider, not from a user's actual browser, not from anywhere but that exact container. This only ever worked when the server and the browser completing OAuth were the same machine (desktop/Electron).Confirmed live: after fix 1 alone, the authorize URL built correctly but contained
redirect_uri=http://127.0.0.1:44709/callback— unreachable from the internet.Fix: the redirect_uri is now this server's own public origin (derived from the request's
Origin/Hostheader) plus a newGET /api/mcp/oauth/callbackroute on the same already-listening HTTP server.login()returns immediately with theauthorize_url(new field onOAuthLoginResponse) instead of blocking; the caller sends a browser there; the OAuth provider's redirect naturally reaches this server wherever it's actually reachable from, same as any other request. Correlation still runs through the same(user_id, csrf_state)-keyed pending map — only how the callback arrives changed, not the security model (the map-key match is the CSRF check; the previously separately-storedcsrf_tokenfield was redundant and is removed).This also removes the now-dead per-login
TcpListener/raw-HTTP-parsing code (wait_for_callback,handle_callback_connection,parse_callback_query,url_decode).Note for consumers: this changes
POST /api/mcp/oauth/login's contract — it now returns{success: true, authorize_url: "..."}immediately rather than blocking until login completes. A client (e.g. a web frontend) needs to navigate a browser toauthorize_urland re-check/api/mcp/oauth/check-statusafterward, rather than awaiting a blocking success/failure. Desktop/Electron clients that relied on the old "opens local browser and blocks" behavior will need to switch to this pattern too.Verification
cargo clippy --all-targets -- -D warnings: cleancargo fmt --check: cleanhttps://mcp.higgsfield.ai/mcpserverTest plan
cargo test -p aionui-mcp -p aionui-api-types— all passingcargo clippy -p aionui-mcp -p aionui-api-types --all-targets -- -D warnings— cleancargo fmt -p aionui-mcp -p aionui-api-types -- --check— clean