fix(mcp): discover OAuth endpoints per spec and support dynamic client registration - #988
Open
omarmciver wants to merge 1 commit into
Open
omarmciver wants to merge 1 commit into
omarmciver wants to merge 1 commit into
Conversation
…t registration
MCP OAuth login failed with an opaque HTTP 500 ("Internal server error.")
for public servers such as monday.com and Microsoft 365.
Two defects:
1. Discovery appended `.well-known/...` to the full server URL, so
`https://mcp.monday.com/sse` was probed at
`https://mcp.monday.com/sse/.well-known/oauth-authorization-server`,
which those servers answer with 401. RFC 8414 places the well-known
segment between the origin and the resource path
(`/.well-known/oauth-authorization-server/sse`). Discovery now follows
the RFC 9728 protected-resource pointer first, then probes the
origin-relative RFC 8414 and OIDC locations, keeping the previous
appended spelling as a last-resort fallback.
2. The client ID was hardcoded to "aionui". Servers that require RFC 7591
dynamic client registration reject it, so login could not complete even
once discovery succeeded. Clients are now registered dynamically when
the server advertises a registration endpoint, and the resulting
credentials are reused for the token exchange and refresh.
Discovery failures also no longer map to a 500: they are caused by the
remote server or the configured URL, so they now return 502 with the
actual reason instead of being scrubbed to "Internal server error."
Verified against the live monday.com MCP server: the two URLs the old code
probed return 401, while the new discovery chain resolves the endpoints and
reports the advertised registration endpoint.
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
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
MCP OAuth login fails with an opaque
HTTP 500 {"success":false,"error":"Internal server error.","code":"INTERNAL_ERROR"}for public MCP servers that require OAuth, such as monday.com and Microsoft 365. No browser window ever opens.Reported downstream as iOfficeAI/AionUi#3685.
Reproduces with this server config:
{ "mcpServers": { "monday": { "type": "http", "url": "https://mcp.monday.com/sse" } } }Root cause
Two independent defects in
crates/aionui-mcp/src/oauth_service.rs.1. Well-known discovery used the wrong URLs.
discover_endpointsappended the well-known path to the full server URL:For
https://mcp.monday.com/ssethat probeshttps://mcp.monday.com/sse/.well-known/oauth-authorization-server. RFC 8414 §3.1 places the well-known segment between the origin and the resource path instead. Probing what the old code actually requested:https://mcp.monday.com/sse/.well-known/oauth-authorization-serverhttps://mcp.monday.com/sse/.well-known/openid-configurationhttps://mcp.monday.com/.well-known/oauth-authorization-serverhttps://mcp.monday.com/.well-known/oauth-protected-resource/sseBoth probes fail, so discovery returns
McpError::OAuth, which maps toApiError::Internal→ HTTP 500, andpublic_message()scrubs the cause to"Internal server error."That is why the popup carries no actionable detail.2. The client ID was hardcoded.
const DEFAULT_CLIENT_ID: &str = "aionui"was used for authorize, token exchange, and refresh. Servers requiring RFC 7591 dynamic client registration do not know that ID, so login still could not complete even once discovery succeeded. monday advertisesregistration_endpoint, and a live registration call returns a realclient_id.Changes
oauth_discoverymodule implementing the spec discovery chain: RFC 9728 protected-resource metadata to locate the authorization server, then RFC 8414 / OIDC metadata. Well-known URLs are resolved against the origin with the resource path as a suffix; the previous appended spelling is retained as a last-resort fallback, so servers that only answer there keep working.client_iddiffers per server. Servers without a registration endpoint keep the existing static client ID.502with a coded error (MCP_OAUTH_DISCOVERY_FAILED) that preserves the real reason, rather than being scrubbed into a generic 500. They are caused by the remote server or the configured URL, not an internal fault.Testing
cargo test -p aionui-mcp --lib— 298 passed, including 8 new discovery unit tests and an error-mapping test.cargo clippy -p aionui-mcp --all-targets— clean.cargo fmt --all --check— clean.cargo build --workspace— clean.tests/oauth_discovery_live.rs(#[ignore]by default) verifies discovery against the real monday.com server. It passes, and the two URLs the old code probed still return 401 — so the test exercises the actual regression rather than passing trivially.Note: two pre-existing failures in
tests/connection_test_integration.rs(http_unreachable_url_returns_connection_error,sse_unreachable_url_returns_connection_error) reproduce on an unmodifiedmainin my environment and are unrelated to this change — they depend on unreachable hosts being refused rather than resolved.Known limitation
Registered client credentials are cached in-process, not persisted. After a backend restart the cache is empty, so a token refresh against a registration-only server falls back to the static client ID and fails, requiring a fresh login. Persisting the registration alongside the token row would remove that re-login; that needs a DB migration, so I left it as a documented follow-up to keep this change focused. Happy to add it here if you would prefer.