Conversation
Static credential injection (Chainlit#2292) covers servers where the user already holds a token. This adds the authorization flow itself, opt-in per connection via `useOAuth` so existing connections are unaffected. Discovery, dynamic client registration and PKCE come from the MCP SDK's OAuthClientProvider. What the SDK cannot supply is the part that only matters on a multi-user server: its TokenStorage takes no arguments, so a single storage shared across requests would hand one user's access token to the next caller. McpOAuthTokenStore keys tokens by (user identifier, server) and hands the SDK a view fixed to one pair. Servers are compared on scheme, host and path, so a token issued for one server mounted on a host is never sent to another mounted beside it. The redirect returns on a route shared by every user, so PendingAuthorizations resolves a callback only for the user who started it; a state belonging to someone else is refused rather than completed against the wrong account. States are single-use, expire, and are URL-safe — random_secret's alphabet contains %, /, = and ?, which do not survive a query string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The pending map was keyed on a state Chainlit generated, but the SDK mints its own, embeds it in the authorization URL and compares the returned value with compare_digest. The browser therefore echoed the SDK's state, resolve() raised KeyError, and no flow could ever complete. Register the owner when the redirect is handed over, keyed by the state already in that URL, and return that same state from the callback handler so the SDK's comparison passes. That also removes the reason for generating a URL-safe state here: the SDK owns state generation now, so random_secret's alphabet is no longer involved. A failed or malformed callback now abandons the caller's own flow, so the waiting connection fails fast instead of hanging until the state expires. Ownership is checked there for the same reason resolve() checks it: otherwise anyone holding a state could cancel someone else's connection. Scope the flow to the HTTP-authenticated caller rather than the session user; the callback route sees the former, so the two must agree or every callback is refused as belonging to someone else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four addressed in 0fc62eb. The first one was correct and serious — thanks. P1, The owner is now recorded in the This also retires the URL-safe-state change from the first commit: the SDK owns state generation, so P1, P2, P3, VerificationBoth fixes are mutation-tested. Reverting the state fix to a self-generated value reproduces the failure exactly as described: and dropping the 7 new tests (42 total in the file), driving the provider's real |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@r0h1tb This might (or might not) be redundant with the recent release. We had to keep it under quarantine due to the security risks, hence I didn't tell you. |
|
OAuth support is still needed, it would be great to see this rebased on the new MCP code and progressed! |
Makes sense. @r0h1tb any chance for a bump? If not, @cyanidium feel free to shoot a new PR. |
|
Any progress on updating this with the new mcp setup? @r0h1tb @cyanidium @dokterbob |
Brings in the 2.12.0 MCP rework (named servers from config, opt-in user servers behind allowed_urls, destination-checked httpx clients). Conflicts resolved in favour of main's request model and transport wiring; the OAuth wiring is ported onto it in the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rebuilds the OAuth wiring on the named-server / user-server model from 2.12.0 instead of the removed per-transport request models. - Configured sse and streamable-http servers opt in with an `oauth` table on their [[features.mcp.servers]] entry. `useOAuth` remains for user-provided servers, and is refused for a named one so the browser cannot switch OAuth on for a server the developer configured. - The SDK's discovery, registration and token requests go through the destination-checked client like the connection itself: a named server keeps to its own origin plus the `authorization_origins` granted in config, and a user-provided server stays inside allowed_urls. - The connect wait grows by AUTHORIZATION_TIMEOUT when OAuth is on, since the user signs in and consents in the browser first. - A refused, abandoned or expired authorization now raises McpAuthorizationError with its reason instead of cancelling a future, and fails the connect through the same side channel as a blocked destination, because the SDK transports swallow it. A connection that gives up drops its pending state, so a late callback is refused. - connect_mcp takes the Request to build the callback URL; tests that call it directly now pass one. Also clears the formatting and mypy failures CI reported on test_mcp_oauth.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@KoenDesplenter @cyanidium @dokterbob this is now updated to the 2.12 MCP setup. I merged How it maps onto 2.12. The browser no longer sends connection details, so OAuth follows the same split:
How it fits the SSRF fix. The SDK's discovery, client registration and token requests run in this process, so they go through the same destination-checked client as the connection. A named server stays pinned to its own origin, and Two things the new connect flow needed:
cubic's last open finding (a rejected OAuth request evicting a working connection) is covered by 2.12's reordering: eviction happens only after a successful connect, and every OAuth check runs before launch. Checks: ruff, ruff format and mypy are clean on the whole backend, and the full backend suite passes (1004). The new tests cover the config model, the origin grant, the connect wiring and the fail-fast path, and each new guarantee was mutation-checked (8 of 8 fail a test). Still open, as before: the UI. The backend emits |
There was a problem hiding this comment.
2 issues found across 7 files
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="backend/chainlit/mcp_oauth.py">
<violation number="1" location="backend/chainlit/mcp_oauth.py:343">
P1: `callback_handler()` returns a `(code, state)` tuple, but the MCP SDK consumes this callback's result as an `AuthorizationCodeResult` object, reading `result.code`, `result.state` and `result.iss` (see `mcp/client/auth/oauth2.py`: `result = await self.context.callback_handler()` then `result.state is None` / `result.code`). After a user successfully authorizes in the browser, the token exchange will crash with `AttributeError: 'tuple' object has no attribute 'state'` (or TypeError against a TypedDict on older 1.x), so the completed OAuth flow never exchanges the code. No test catches this: the transport fakes only compare the returned tuple to itself and never drive the real SDK consumer. Return an `AuthorizationCodeResult` (import it from `mcp.shared.auth`) built from the awaited values, and update `test_the_callback_handler_returns_the_sdk_state` accordingly.</violation>
</file>
<file name="backend/tests/test_mcp.py">
<violation number="1" location="backend/tests/test_mcp.py:2563">
P3: This assertion hardcodes the TestClient host/scheme and assumes `CHAINLIT_URL` is unset. `get_user_facing_url()` (chainlit/server.py:440) rewrites the base URL from the `CHAINLIT_URL` environment variable, so when it is set this exact URI assertion fails even though the redirect is built correctly. Pin the expected URI using `get_user_facing_url(request.url)` semantics, or assert only that the redirect path ends with `/mcp/oauth/callback`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if pending is None: # pragma: no cover - the SDK always redirects first | ||
| raise RuntimeError("MCP authorization was awaited before it started.") | ||
| try: | ||
| return await pending.wait() |
There was a problem hiding this comment.
P1: callback_handler() returns a (code, state) tuple, but the MCP SDK consumes this callback's result as an AuthorizationCodeResult object, reading result.code, result.state and result.iss (see mcp/client/auth/oauth2.py: result = await self.context.callback_handler() then result.state is None / result.code). After a user successfully authorizes in the browser, the token exchange will crash with AttributeError: 'tuple' object has no attribute 'state' (or TypeError against a TypedDict on older 1.x), so the completed OAuth flow never exchanges the code. No test catches this: the transport fakes only compare the returned tuple to itself and never drive the real SDK consumer. Return an AuthorizationCodeResult (import it from mcp.shared.auth) built from the awaited values, and update test_the_callback_handler_returns_the_sdk_state accordingly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/chainlit/mcp_oauth.py, line 343:
<comment>`callback_handler()` returns a `(code, state)` tuple, but the MCP SDK consumes this callback's result as an `AuthorizationCodeResult` object, reading `result.code`, `result.state` and `result.iss` (see `mcp/client/auth/oauth2.py`: `result = await self.context.callback_handler()` then `result.state is None` / `result.code`). After a user successfully authorizes in the browser, the token exchange will crash with `AttributeError: 'tuple' object has no attribute 'state'` (or TypeError against a TypedDict on older 1.x), so the completed OAuth flow never exchanges the code. No test catches this: the transport fakes only compare the returned tuple to itself and never drive the real SDK consumer. Return an `AuthorizationCodeResult` (import it from `mcp.shared.auth`) built from the awaited values, and update `test_the_callback_handler_returns_the_sdk_state` accordingly.</comment>
<file context>
@@ -0,0 +1,365 @@
+ if pending is None: # pragma: no cover - the SDK always redirects first
+ raise RuntimeError("MCP authorization was awaited before it started.")
+ try:
+ return await pending.wait()
+ except McpAuthorizationError as exc:
+ if on_failure is not None:
</file context>
| assert storage.server_key == "https://mcp.example.com/mcp" | ||
| redirect_uris = auth.context.client_metadata.redirect_uris or [] | ||
| assert [str(u) for u in redirect_uris] == [ | ||
| "http://testserver/mcp/oauth/callback" |
There was a problem hiding this comment.
P3: This assertion hardcodes the TestClient host/scheme and assumes CHAINLIT_URL is unset. get_user_facing_url() (chainlit/server.py:440) rewrites the base URL from the CHAINLIT_URL environment variable, so when it is set this exact URI assertion fails even though the redirect is built correctly. Pin the expected URI using get_user_facing_url(request.url) semantics, or assert only that the redirect path ends with /mcp/oauth/callback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/test_mcp.py, line 2563:
<comment>This assertion hardcodes the TestClient host/scheme and assumes `CHAINLIT_URL` is unset. `get_user_facing_url()` (chainlit/server.py:440) rewrites the base URL from the `CHAINLIT_URL` environment variable, so when it is set this exact URI assertion fails even though the redirect is built correctly. Pin the expected URI using `get_user_facing_url(request.url)` semantics, or assert only that the redirect path ends with `/mcp/oauth/callback`.</comment>
<file context>
@@ -2440,3 +2476,312 @@ def test_stdio_server_secrets_not_disclosed(
+ assert storage.server_key == "https://mcp.example.com/mcp"
+ redirect_uris = auth.context.client_metadata.redirect_uris or []
+ assert [str(u) for u in redirect_uris] == [
+ "http://testserver/mcp/oauth/callback"
+ ]
+
</file context>
…consent Addresses the review of d807b0e. - canonical_server_key now keeps what the MCP SDK keeps in the RFC 8707 resource URL: the exact path (trailing slash included) and the query, either of which can select a different server or tenant on one host. It still folds scheme/host case, a default port and an empty path, and brackets an IPv6 host so a port cannot run into the address ("[::1]:8443" and "[::1:8443]" no longer share a key). - The connect wait grows by AUTHORIZATION_TIMEOUT only once the user has actually been sent to authorize. With a cached or refreshed token, a server that is down fails on the ordinary HTTP budget again. - A test now runs the SDK's own _perform_authorization against the provider, so the callback contract is checked end to end (mcp 1.x unpacks a (code, state) tuple; 2.0.0 changes it, noted in code). - Tests clear the process-wide token store and pending map, and the redirect test no longer depends on CHAINLIT_URL being unset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review findings addressed in 65630d9:
Not changed: returning Backend suite: 1011 passed. |
Part of #2197. Opt-in, additive, and safe to land on its own.
Problem
#2292 shipped the static-credential half of #2197: headers on SSE and Streamable HTTP cover any server where the user already holds a long-lived token. The authorization flow — Chainlit obtaining a token on the user's behalf — is still missing.
Discovery, dynamic client registration and PKCE are already solved by the MCP SDK's
OAuthClientProvider, so this does not reimplement them. What the SDK cannot solve is the part that only exists on a multi-user server:get_tokens()takes no arguments. The SDK assumes one storage per authorization context, which holds for the single-user desktop clients it targets. Chainlit serves many users from one process, so a storage shared across requests hands one user's access token to the next caller.Fix
chainlit/mcp_oauth.py:McpOAuthTokenStorekeys tokens by(user identifier, server)and only exposes them throughscoped(user, server), which fixes both halves of the key up front. The SDK receives a view it cannot read outside.canonical_server_keykeys a token the way the SDK's RFC 8707 resource URL identifies the server: the exact path (trailing slash included) and the query are kept, since either can select a different server or tenant on one host. Only what RFC 3986 makes equivalent is folded (scheme/host case, a default port, an empty path), and IPv6 hosts keep their brackets. A token issued for/jirais never presented to/confluence.PendingAuthorizationscorrelates the redirect back to the user who started it. The callback route is shared, sostateis the only link: a state belonging to another user is refused rather than completed against the wrong account. States are single-use and expire.OAuth is opt-in and follows the 2.12 MCP model. A configured SSE or streamable-http server enables it with an
oauthtable on its[[features.mcp.servers]]entry; a user-provided server enables it withuseOAuthon the connect request (refused for a named server). Without either, connections are unaffected.The SDK's discovery, registration and token requests go through the same destination-checked client as the connection: a named server's own origin plus the
authorization_originsit grants (bare origins, validated at load), orallowed_urlsfor a user-provided server. Redirects stay disabled.Two details worth flagging in review:
stateis the SDK's own. The owner is recorded inredirect_handler, keyed by the state in the authorization URL, because the SDK compares the returned state against the one it minted.McpAuthorizationErrorwith its reason and fails the connect at once, through the same side channel ason_blocked: the SDK transports swallow errors from their send loops. Once the user has been sent to authorize, the connect wait grows byAUTHORIZATION_TIMEOUT(300s) to cover consent in the browser; with a cached token it stays on the ordinary HTTP budget.The interactive half — surfacing
mcp_authorization_requiredin the UI — is left for a follow-up; the backend emits it today.Tests
70 tests in
backend/tests/test_mcp_oauth.py(canonicalisation, the SDK protocol contract — including a run of the SDK's own_perform_authorizationagainst the provider — isolation, expiry/replay, fail-fast, the config model, the origin grant, the callback route) and 9 inTestConnectMcpOAuthinbackend/tests/test_mcp.py(connect wiring, the destination grant, auth required, the fail-fast path, the extended wait and when it does not apply).The isolation guarantee is mutation-tested. Keying the store on the server alone — the SDK's own assumption — fails 5 tests, including the leak itself:
On the 2.12 port, each new guarantee was mutation-checked too (no fail-fast channel, no origin grant, a wait extended without a redirect or never extended, a given-up flow kept,
useOAuthon a named server, cancel instead of raise, an unbracketed IPv6 key, a collapsed trailing slash or dropped query, a 2.x-style callback result): every mutant fails a test.Full backend suite: 1011 passed.
ruff check,ruff format --checkandmypyare clean on the whole backend.Summary by cubic
Adds per-user OAuth for HTTP
mcpconnections so Chainlit can obtain and reuse tokens per logged-in user. Previously only static headers were sent; HTTP transports can now opt into OAuth viauseOAuth, default behavior is unchanged, and Stdio remains unsupported.New Features
McpOAuthTokenStorekeyed by (user identifier, canonical server) and exposed via a scopedTokenStorage.PendingAuthorizationskeyed on the SDK-generatedstate; states are single-use, expire, and enforce ownership./mcp/oauth/callback: returns 401 if unauthenticated, 403 if thestatebelongs to another user, and 400 on provider errors or missing/unknown/expiredstate/code; errors abandon the owner's flow.ConnectSseMCPRequestandConnectStreamableHttpMCPRequestwithuseOAuth: false; when true, builds a per-userOAuthClientProvider, emitsmcp_authorization_requiredwith the authorization URL, and attaches it to HTTP clients; flows are scoped to the HTTP-authenticated caller. Once the user is sent to authorize, the connect wait extends by 300s; a server that is down still fails on the ordinary HTTP budget.Migration
useOAuth: trueon HTTP MCP connections and handling themcp_authorization_requiredevent in the UI.Written for commit 65630d9. Summary will update on new commits.