Keep interactive OAuth flows alive when the triggering request is canceled - #1831
Open
PederHP wants to merge 1 commit into
Open
Conversation
…celed During the dual-path connect, the server/discover probe's 401 challenge can start an interactive authorization flow. When DiscoverProbeTimeout elapsed while the user was still completing that flow in a browser, the probe's cancellation aborted the flow, and the initialize fallback's challenge then started a second flow with a fresh state and PKCE verifier that the redirect the user eventually completed could never satisfy, failing the connect with 'The authorization response state did not match the state sent in the authorization request'. Memoize the in-flight authorization-code flow in ClientOAuthProvider and detach it from the triggering request's cancellation token, bounding it by provider disposal instead. Challenge handlers await the shared flow with their own token, so a canceled request abandons only its wait while a later challenge joins the flow and reuses its result. HttpClientTransport disposal cancels any flow still pending. Fixes modelcontextprotocol#1830 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011bjA7zRNnUXnh19qSqgpTY
Contributor
There was a problem hiding this comment.
Pull request overview
Keeps interactive OAuth authorization alive across canceled requests while preserving transport-lifetime cancellation.
Changes:
- Memoizes and shares in-flight authorization flows.
- Cancels detached flows when the HTTP transport is disposed.
- Documents timeout behavior and adds regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
ClientOAuthProvider.cs |
Shares detached OAuth flows. |
HttpClientTransport.cs |
Cancels flows during disposal. |
McpClientOptions.cs |
Documents OAuth timeout behavior. |
AuthTests.cs |
Tests cancellation and disposal scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2625
to
+2629
| // Simulate a user who finishes the browser flow only after DiscoverProbeTimeout has | ||
| // elapsed and the initialize fallback has raised its own challenge. The delay is | ||
| // deliberately not bound to cancellationToken so that, before the fix, the second | ||
| // flow ran to completion and the test observed both invocations. | ||
| await Task.Delay(TimeSpan.FromSeconds(2), CancellationToken.None); |
Comment on lines
+512
to
+515
| var flow = _inFlightAuthorizationCodeFlow; | ||
| if (flow is null || flow.IsCompleted) | ||
| { | ||
| _inFlightAuthorizationCodeFlow = flow = InitiateAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); |
Comment on lines
+93
to
+96
| /// When the transport authenticates via OAuth with an interactive | ||
| /// <see cref="Authentication.ClientOAuthOptions.AuthorizationCallbackHandler"/>, the user's browser-based | ||
| /// authorization runs within this budget: increase this value to cover the time a person | ||
| /// needs to complete the login, not just the network round-trips. |
|
|
||
| await transport.DisposeAsync(); | ||
|
|
||
| await handlerCanceled.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); |
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.
Fixes #1830
Problem
During the dual-path connect, the
server/discoverprobe's 401 challenge can start an interactive authorization flow viaAuthorizationCallbackHandler. WhenDiscoverProbeTimeout(default 5s) elapses while the user is still completing that flow in a browser, the probe's cancellation aborts the flow. Theinitializefallback's challenge then starts a second flow with a fresh state and PKCE verifier — with no way to surface a new authorization URL to the user, who is still completing the first flow. The redirect the user eventually completes carries the first flow's state, and the connect fails with:The user-approved code is unusable by the second flow regardless (different PKCE verifier), so this failure is unrecoverable. Observed in production against a real OAuth-protected MCP server; full analysis in #1830.
Fix
ClientOAuthProvidernow memoizes the in-flight authorization-code flow and detaches it from the triggering request's cancellation token, bounding it by provider disposal instead:Task.WaitAsync, so a canceled request (e.g. the probe) abandons only its wait.initializefallback here, or any concurrent request — joins the in-flight flow and reuses its result. The existing_tokenAcquisitionLock+ cached-token re-check already covers the "token arrived while I waited" case.ClientOAuthProvideris nowIDisposable;HttpClientTransport.DisposeAsyncdisposes it, canceling any flow still pending so a parkedAuthorizationCallbackHandlerobserves cancellation.Docs for
DiscoverProbeTimeoutandInitializationTimeoutnow describe the interactive-authorization interplay (notably that the user's login must fit withinInitializationTimeout).Tests
InteractiveAuthorization_SurvivesCancellationOfTriggeringRequest— deterministic regression test for the general mechanism: connect 1 is canceled while its flow waits on the user; the user then completes the original flow; connect 2 must reuse its outcome with exactly one handler invocation.InteractiveAuthorization_SurvivesDiscoverProbeTimeout— end-to-end dual-path scenario: probe 401 → probe canceled byDiscoverProbeTimeout→ initialize fallback joins the pending flow.DisposingTransport_CancelsDetachedAuthorizationFlow— the detached flow outlives canceled connects but is canceled by transport disposal.Both regression tests fail against the previous provider behavior and pass with this change; the full OAuth suite (101 tests) and the July 2026 protocol fallback suite (53 tests) pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_011bjA7zRNnUXnh19qSqgpTY