From c5cae122a1c1c25b7a638e7326c2ff667103ec85 Mon Sep 17 00:00:00 2001 From: Peder Date: Thu, 20 Aug 2026 20:31:04 +0200 Subject: [PATCH 1/6] Keep interactive OAuth flows alive when the triggering request is canceled 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 #1830 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011bjA7zRNnUXnh19qSqgpTY --- .../Authentication/ClientOAuthProvider.cs | 40 +++++- .../Client/HttpClientTransport.cs | 2 + .../Client/McpClientOptions.cs | 15 ++ .../OAuth/AuthTests.cs | 132 ++++++++++++++++++ 4 files changed, 186 insertions(+), 3 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index 785e3cc2e..d4a543f26 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -17,7 +17,7 @@ namespace ModelContextProtocol.Authentication; /// /// A generic implementation of an OAuth authorization provider. /// -internal sealed partial class ClientOAuthProvider : McpHttpClient +internal sealed partial class ClientOAuthProvider : McpHttpClient, IDisposable { /// /// The Bearer authentication scheme. @@ -73,6 +73,20 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient private readonly HashSet _accumulatedScopes = new(StringComparer.Ordinal); private bool _hasAttemptedStepUp; + // The single in-flight authorization-code flow, if any. Written only while holding + // _tokenAcquisitionLock. The flow is deliberately detached from the cancellation of the request + // whose challenge started it: the user may already be completing the authorization in a browser, + // and canceling one HTTP request — for example a server/discover probe canceled by + // McpClientOptions.DiscoverProbeTimeout during the dual-path connect — must not abort that flow. + // If it did, the next challenge would start a second flow with a fresh state and PKCE verifier + // that the redirect the user eventually completes can never satisfy. Instead, a later challenge + // joins the in-flight flow and shares its result, while each caller observes its own cancellation + // via WaitAsync. The flow itself is bounded by the authorization callback handler's own + // completion and canceled on provider disposal. + private Task? _inFlightAuthorizationCodeFlow; + private readonly CancellationTokenSource _disposeCts = new(); + private int _disposed; + /// /// Initializes a new instance of the class using the specified options. /// @@ -191,6 +205,18 @@ public ClientOAuthProvider( }); } + /// + /// Cancels any in-flight detached authorization-code flow (see ). + /// + public void Dispose() + { + if (Interlocked.Exchange(ref _disposed, 1) == 0) + { + _disposeCts.Cancel(); + _disposeCts.Dispose(); + } + } + internal override async Task SendAsync(HttpRequestMessage request, JsonRpcMessage? message, CancellationToken cancellationToken) { bool attemptedRefresh = false; @@ -480,8 +506,16 @@ private async Task GetAccessTokenCoreAsync(HttpResponseMessage response, // Store auth server metadata for future refresh operations _authServerMetadata = authServerMetadata; - // Perform the OAuth flow - return await InitiateAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, cancellationToken).ConfigureAwait(false); + // Perform the OAuth flow. A caller that reaches this point after a previous caller's request + // was canceled mid-flow (releasing the lock with the flow still pending) joins the in-flight + // flow instead of starting a competing one; see the _inFlightAuthorizationCodeFlow comment. + var flow = _inFlightAuthorizationCodeFlow; + if (flow is null || flow.IsCompleted) + { + _inFlightAuthorizationCodeFlow = flow = InitiateAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); + } + + return await flow.WaitAsync(cancellationToken).ConfigureAwait(false); } private void ApplyClientIdMetadataDocument(Uri metadataUri) diff --git a/src/ModelContextProtocol.Core/Client/HttpClientTransport.cs b/src/ModelContextProtocol.Core/Client/HttpClientTransport.cs index 14044d2d7..a61026809 100644 --- a/src/ModelContextProtocol.Core/Client/HttpClientTransport.cs +++ b/src/ModelContextProtocol.Core/Client/HttpClientTransport.cs @@ -105,6 +105,8 @@ private async Task ConnectSseTransportAsync(CancellationToken cancel /// public ValueTask DisposeAsync() { + // Cancels any authorization-code flow still running detached from a canceled request. + (_mcpHttpClient as IDisposable)?.Dispose(); _ownedHttpClient?.Dispose(); return default; } diff --git a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs index 61a0613df..5a6f111c1 100644 --- a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs +++ b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs @@ -89,6 +89,12 @@ public sealed class McpClientOptions /// Setting an appropriate timeout prevents the client from hanging indefinitely when /// connecting to unresponsive servers. /// + /// + /// When the transport authenticates via OAuth with an interactive + /// , 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. + /// /// public TimeSpan InitializationTimeout { get; set; } = TimeSpan.FromSeconds(60); @@ -121,6 +127,15 @@ public sealed class McpClientOptions /// greater than or equal to , the probe is effectively bounded by /// alone. /// + /// + /// A server that requires OAuth answers the probe with a 401 challenge, which can start an + /// interactive authorization via . + /// If this timeout then elapses while the user is still authorizing, only the probe request is + /// canceled: the authorization flow keeps running, and the challenge raised by the + /// initialize fallback joins that same flow and reuses its token instead of starting a + /// second flow the user never sees. The connect attempt overall remains bounded by + /// , and disposing the transport cancels the flow. + /// /// /// /// The value is not positive and is not . diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index 693c77943..c1aa6bbba 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -2549,4 +2549,136 @@ public async Task DynamicClientRegistration_ScopeSelector_AppliesToDcrScope() Assert.Equal("mcp:tools", TestOAuthServer.LastRegistrationScope); } + + [Fact] + public async Task InteractiveAuthorization_SurvivesCancellationOfTriggeringRequest() + { + // A challenge raised while a previous challenge's interactive flow is still pending must + // join that flow rather than start a second one: the user is already completing the first + // flow's authorization URL in a browser, and a second flow's state and PKCE verifier could + // never match the redirect the user eventually completes. Canceling the request whose + // challenge started the flow must therefore not cancel the flow itself. + await using var app = await StartMcpServerAsync(); + + var handlerInvocations = 0; + var handlerEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var completeAuthorization = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var transport = CreateOAuthTransport(async (context, cancellationToken) => + { + Interlocked.Increment(ref handlerInvocations); + handlerEntered.TrySetResult(); + + // Hold the flow open, like a user mid-login. Before the fix, canceling the first + // connect canceled this wait via cancellationToken, and the second connect re-invoked + // the handler for a fresh flow. + await completeAuthorization.Task.WaitAsync(cancellationToken); + return await HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + var clientOptions = new McpClientOptions { ProtocolVersion = "2025-06-18" }; + + using var firstConnectCts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + var firstConnect = McpClient.CreateAsync( + transport, clientOptions, loggerFactory: LoggerFactory, cancellationToken: firstConnectCts.Token); + + await handlerEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + firstConnectCts.Cancel(); + await Assert.ThrowsAnyAsync(() => firstConnect); + + // The user now completes the original flow's authorization. The flow must still be alive + // to receive it, and the next connect must reuse its outcome (via the in-flight flow or + // the token it caches) instead of starting a second flow. + completeAuthorization.TrySetResult(); + + await using var client = await McpClient.CreateAsync( + transport, clientOptions, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(1, handlerInvocations); + } + + [Fact] + public async Task InteractiveAuthorization_SurvivesDiscoverProbeTimeout() + { + // End-to-end version of the dual-path connect scenario: the server/discover probe draws the + // 401 that starts the interactive flow, DiscoverProbeTimeout cancels the probe while the + // flow waits on the user, and the challenge raised by the initialize fallback must join the + // pending flow instead of starting a second one the user never sees. + await using var app = await StartMcpServerAsync(); + + // Warm the server pipeline (JIT, auth handlers) so the in-test latencies are dominated by + // the configured probe timeout rather than first-request overhead. + using (var warmup = await HttpClient.PostAsync( + McpServerUrl, + new StringContent("{}", System.Text.Encoding.UTF8, "application/json"), + TestContext.Current.CancellationToken)) + { + Assert.Equal(HttpStatusCode.Unauthorized, warmup.StatusCode); + } + + var handlerInvocations = 0; + + await using var transport = CreateOAuthTransport(async (context, cancellationToken) => + { + Interlocked.Increment(ref handlerInvocations); + + // 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); + return await HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, + new McpClientOptions { DiscoverProbeTimeout = TimeSpan.FromMilliseconds(500) }, + loggerFactory: LoggerFactory, + cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(1, handlerInvocations); + } + + [Fact] + public async Task DisposingTransport_CancelsDetachedAuthorizationFlow() + { + await using var app = await StartMcpServerAsync(); + + var handlerCanceled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var transport = CreateOAuthTransport(async (context, cancellationToken) => + { + try + { + // Park the flow past the entire connect attempt, as if the user never finishes + // the browser login. + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + } + catch (OperationCanceledException) + { + handlerCanceled.TrySetResult(); + throw; + } + + return null; + }); + + await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, + new McpClientOptions + { + DiscoverProbeTimeout = TimeSpan.FromMilliseconds(100), + InitializationTimeout = TimeSpan.FromSeconds(1), + }, + loggerFactory: LoggerFactory, + cancellationToken: TestContext.Current.CancellationToken)); + + // The flow is detached from the canceled connect requests; only disposing the transport + // cancels it. + Assert.False(handlerCanceled.Task.IsCompleted); + + await transport.DisposeAsync(); + + await handlerCanceled.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + } } From e8507531c1618d6c01600ccfe6823c4d760a7300 Mon Sep 17 00:00:00 2001 From: Peder Date: Sat, 29 Aug 2026 20:06:06 +0200 Subject: [PATCH 2/6] Address review: scope-aware reuse of in-flight OAuth flows and deterministic tests Only reuse a pending authorization-code flow for a challenge whose scopes it already requested. A challenge that needs a scope the flow did not request could never be satisfied by its token, so it now waits for the pending flow to settle and then runs its own step-up for the accumulated scopes, keeping at most one interactive prompt in front of the user at a time. The flow's requested scope set is recorded when the flow starts. Document InitializationTimeout as bounding how long the connect attempt waits for browser authorization rather than the flow itself, which only transport disposal cancels. Replace the fixed delay in the DiscoverProbeTimeout regression test with a signal from server middleware when the initialize fallback arrives, use the standard test timeout for the disposal test, and add a regression test for the files:read / files:write scope-mismatch scenario. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01C9kt1V3wMcWWkSMJEkrXKK --- .../Authentication/ClientOAuthProvider.cs | 80 ++++++- .../Client/McpClientOptions.cs | 9 +- .../OAuth/AuthTests.cs | 206 +++++++++++++++++- 3 files changed, 268 insertions(+), 27 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index d4a543f26..4eb0b2e23 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -73,17 +73,21 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient, IDisposable private readonly HashSet _accumulatedScopes = new(StringComparer.Ordinal); private bool _hasAttemptedStepUp; - // The single in-flight authorization-code flow, if any. Written only while holding - // _tokenAcquisitionLock. The flow is deliberately detached from the cancellation of the request - // whose challenge started it: the user may already be completing the authorization in a browser, - // and canceling one HTTP request — for example a server/discover probe canceled by + // The single in-flight authorization-code flow, if any, and the scopes it requested. Written only + // while holding _tokenAcquisitionLock. The flow is deliberately detached from the cancellation of + // the request whose challenge started it: the user may already be completing the authorization in + // a browser, and canceling one HTTP request — for example a server/discover probe canceled by // McpClientOptions.DiscoverProbeTimeout during the dual-path connect — must not abort that flow. // If it did, the next challenge would start a second flow with a fresh state and PKCE verifier // that the redirect the user eventually completes can never satisfy. Instead, a later challenge - // joins the in-flight flow and shares its result, while each caller observes its own cancellation - // via WaitAsync. The flow itself is bounded by the authorization callback handler's own - // completion and canceled on provider disposal. + // whose scopes the flow already requested joins it and shares its result, while each caller + // observes its own cancellation via WaitAsync. A challenge that needs a scope the flow did not + // request cannot be satisfied by its token, so it waits for the flow to settle and then runs its + // own step-up; at most one interactive flow is ever presented to the user at a time. The flow + // itself is bounded by the authorization callback handler's own completion and canceled on + // provider disposal. private Task? _inFlightAuthorizationCodeFlow; + private HashSet? _inFlightAuthorizationCodeFlowScopes; private readonly CancellationTokenSource _disposeCts = new(); private int _disposed; @@ -506,18 +510,52 @@ private async Task GetAccessTokenCoreAsync(HttpResponseMessage response, // Store auth server metadata for future refresh operations _authServerMetadata = authServerMetadata; - // Perform the OAuth flow. A caller that reaches this point after a previous caller's request - // was canceled mid-flow (releasing the lock with the flow still pending) joins the in-flight - // flow instead of starting a competing one; see the _inFlightAuthorizationCodeFlow comment. + // Perform the OAuth flow. A caller that reaches this point while a previous caller's flow is + // still pending (that caller's request was canceled mid-flow, releasing the lock) joins the + // in-flight flow instead of starting a competing one, provided the flow requested every scope + // this challenge needs; see the _inFlightAuthorizationCodeFlow comment. var flow = _inFlightAuthorizationCodeFlow; + if (flow is { IsCompleted: false } && !InFlightAuthorizationCodeFlowCoversChallenge(protectedResourceMetadata)) + { + // The pending flow's token cannot satisfy this challenge, but two interactive flows must + // never be presented at once, so let it settle first. Its outcome, success or failure, is + // reported to the callers that joined it and is irrelevant here (Task.WhenAny never + // faults); this challenge then runs its own step-up for the accumulated scopes. + await Task.WhenAny(flow).WaitAsync(cancellationToken).ConfigureAwait(false); + flow = null; + } + if (flow is null || flow.IsCompleted) { - _inFlightAuthorizationCodeFlow = flow = InitiateAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); + _inFlightAuthorizationCodeFlow = flow = StartAuthorizationCodeFlow(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); } return await flow.WaitAsync(cancellationToken).ConfigureAwait(false); } + /// + /// Returns whether the in-flight authorization-code flow requested every scope the current challenge + /// requires, so that joining it can satisfy the challenge. A challenge that names no concrete scope is + /// satisfied by whatever token the flow yields. + /// + private bool InFlightAuthorizationCodeFlowCoversChallenge(ProtectedResourceMetadata protectedResourceMetadata) + { + if (_inFlightAuthorizationCodeFlowScopes is not { } requestedScopes) + { + return false; + } + + foreach (var scope in GetCurrentOperationScopes(protectedResourceMetadata)) + { + if (!requestedScopes.Contains(scope)) + { + return false; + } + } + + return true; + } + private void ApplyClientIdMetadataDocument(Uri metadataUri) { if (!IsValidClientMetadataDocumentUri(metadataUri)) @@ -732,7 +770,11 @@ private static IEnumerable GetWellKnownAuthorizationServerMetadataUris(Uri return tokens.AccessToken; } - private async Task InitiateAuthorizationCodeFlowAsync( + /// + /// Starts an authorization-code flow for the current challenge and records the scopes it requests in + /// . Callers must hold _tokenAcquisitionLock. + /// + private Task StartAuthorizationCodeFlow( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) @@ -741,8 +783,22 @@ private async Task InitiateAuthorizationCodeFlowAsync( var codeChallenge = GenerateCodeChallenge(codeVerifier); var state = GenerateRandomBase64UrlValue(); + // Building the URL folds this challenge's scopes into _accumulatedScopes, so the accumulated set + // is now exactly the set of scopes this flow asks the authorization server for. var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state); + _inFlightAuthorizationCodeFlowScopes = new HashSet(_accumulatedScopes, StringComparer.Ordinal); + return CompleteAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, authUrl, state, codeVerifier, cancellationToken); + } + + private async Task CompleteAuthorizationCodeFlowAsync( + ProtectedResourceMetadata protectedResourceMetadata, + AuthorizationServerMetadata authServerMetadata, + Uri authUrl, + string state, + string codeVerifier, + CancellationToken cancellationToken) + { var authResult = await _authorizationCallbackHandler( new AuthorizationCallbackContext { diff --git a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs index 5a6f111c1..904c6040d 100644 --- a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs +++ b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs @@ -91,9 +91,12 @@ public sealed class McpClientOptions /// /// /// When the transport authenticates via OAuth with an interactive - /// , 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. + /// , this timeout also bounds + /// how long the connect attempt waits for the user to complete the browser-based authorization, so + /// increase this value to cover the time a person needs to complete the login, not just the network + /// round-trips. Reaching it fails the connect attempt but does not cancel the authorization flow + /// itself: the flow keeps running so that a later challenge on the same transport reuses its outcome + /// rather than prompting the user again, and only disposing the transport cancels it. /// /// public TimeSpan InitializationTimeout { get; set; } = TimeSpan.FromSeconds(60); diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index c1aa6bbba..81aa0c06b 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -2602,15 +2602,54 @@ public async Task InteractiveAuthorization_SurvivesDiscoverProbeTimeout() { // End-to-end version of the dual-path connect scenario: the server/discover probe draws the // 401 that starts the interactive flow, DiscoverProbeTimeout cancels the probe while the - // flow waits on the user, and the challenge raised by the initialize fallback must join the + // flow waits on the user, and the challenge raised by the initialize fallback must reuse the // pending flow instead of starting a second one the user never sees. - await using var app = await StartMcpServerAsync(); + // + // The user finishes the browser login only once the initialize fallback has reached the + // server unauthenticated, which can only happen after the probe was canceled. The flow the + // probe started is therefore still pending when the fallback is challenged, and the fallback + // must reuse it (by joining it, or by finding the token it caches) rather than start its own. + var initializeFallbackReachedServer = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var app = await StartMcpServerAsync(configureMiddleware: app => + { + // Registered ahead of the authentication and authorization middleware (added explicitly + // below instead of being auto-inserted at the front of the pipeline), which would + // otherwise challenge the unauthenticated request before it reached this observer. + app.Use(async (context, next) => + { + if (context.Request.Method == HttpMethods.Post && + context.Request.Path == "/" && + context.Request.Headers.Authorization.Count == 0) + { + context.Request.EnableBuffering(); + + var message = await JsonSerializer.DeserializeAsync( + context.Request.Body, + McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(JsonRpcMessage)), + context.RequestAborted) as JsonRpcMessage; + + context.Request.Body.Position = 0; + + if (message is JsonRpcRequest { Method: "initialize" }) + { + initializeFallbackReachedServer.TrySetResult(); + } + } + + await next(context); + }); - // Warm the server pipeline (JIT, auth handlers) so the in-test latencies are dominated by - // the configured probe timeout rather than first-request overhead. + app.UseAuthentication(); + app.UseAuthorization(); + }); + + // Warm the server pipeline (JIT, auth handlers) so the probe's challenge reaches the handler + // well within the probe timeout; otherwise the probe would time out before any flow starts and + // the fallback would simply run the only flow, passing without exercising the scenario. using (var warmup = await HttpClient.PostAsync( McpServerUrl, - new StringContent("{}", System.Text.Encoding.UTF8, "application/json"), + new StringContent("""{"jsonrpc":"2.0","id":1,"method":"ping"}""", System.Text.Encoding.UTF8, "application/json"), TestContext.Current.CancellationToken)) { Assert.Equal(HttpStatusCode.Unauthorized, warmup.StatusCode); @@ -2621,12 +2660,7 @@ public async Task InteractiveAuthorization_SurvivesDiscoverProbeTimeout() await using var transport = CreateOAuthTransport(async (context, cancellationToken) => { Interlocked.Increment(ref handlerInvocations); - - // 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); + await initializeFallbackReachedServer.Task.WaitAsync(cancellationToken); return await HandleAuthorizationUrlAsync(context, cancellationToken); }); @@ -2639,6 +2673,154 @@ public async Task InteractiveAuthorization_SurvivesDiscoverProbeTimeout() Assert.Equal(1, handlerInvocations); } + [Fact] + public async Task InteractiveAuthorization_PendingFlowIsNotReusedForChallengeRequiringMoreScopes() + { + // A pending flow may only be reused by a challenge whose scopes it requested. Here a canceled + // read-tool call leaves its "files:read" step-up pending while the user is still logging in; + // a write-tool call challenged for "files:write" must not reuse that flow, since its token + // could never satisfy the write. It must instead let the pending flow settle and then run its + // own step-up for the accumulated scopes, so the user still sees only one prompt at a time. + Builder.Services.AddMcpServer() + .WithTools([ + McpServerTool.Create([McpServerTool(Name = "read-tool")] + (ClaimsPrincipal user) => + { + return "Read tool executed."; + }), + McpServerTool.Create([McpServerTool(Name = "write-tool")] + (ClaimsPrincipal user) => + { + return "Write tool executed."; + }), + ]); + + var writeChallengeRaised = 0; + var writeChallengeBeingHandled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var app = await StartMcpServerAsync(configureMiddleware: app => + { + // Fetching the protected resource metadata is the client's first step in handling a + // challenge, so the first fetch after the write challenge means it is being handled. The + // authentication handler serves that document itself, so this observer must sit ahead of + // the authentication middleware (added explicitly below rather than auto-inserted at the + // front of the pipeline), while the challenge middleware below it needs the authenticated user. + app.Use(async (context, next) => + { + if (context.Request.Path == "/.well-known/oauth-protected-resource" && Volatile.Read(ref writeChallengeRaised) == 1) + { + writeChallengeBeingHandled.TrySetResult(); + } + + await next(context); + }); + + app.UseAuthentication(); + app.UseAuthorization(); + + app.Use(async (context, next) => + { + if (context.Request.Method == HttpMethods.Post && context.Request.Path == "/") + { + context.Request.EnableBuffering(); + + var message = await JsonSerializer.DeserializeAsync( + context.Request.Body, + McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(JsonRpcMessage)), + context.RequestAborted) as JsonRpcMessage; + + context.Request.Body.Position = 0; + + if (message is JsonRpcRequest request && request.Method == "tools/call") + { + var toolCallParams = JsonSerializer.Deserialize( + request.Params, + McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(CallToolRequestParams))) as CallToolRequestParams; + + var scopeClaim = context.User.FindFirst("scope")?.Value ?? ""; + var scopeSet = new HashSet(scopeClaim.Split(' ')); + + var missingScope = toolCallParams?.Name switch + { + "read-tool" when !scopeSet.Contains("files:read") => "files:read", + "write-tool" when !scopeSet.Contains("files:write") => "files:write", + _ => null, + }; + + if (missingScope is not null) + { + if (missingScope == "files:write") + { + // Set before the response goes out so the observer above sees the flag + // when the client's metadata fetch arrives. + Volatile.Write(ref writeChallengeRaised, 1); + } + + context.Response.StatusCode = StatusCodes.Status403Forbidden; + context.Response.Headers.WWWAuthenticate = $"Bearer error=\"insufficient_scope\", resource_metadata=\"{McpServerUrl}/.well-known/oauth-protected-resource\", scope=\"{missingScope}\""; + await context.Response.StartAsync(context.RequestAborted); + await context.Response.Body.FlushAsync(context.RequestAborted); + return; + } + } + } + + await next(context); + }); + }); + + List requestedScopes = []; + var readStepUpEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var completeReadStepUp = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var transport = CreateOAuthTransport(async (context, cancellationToken) => + { + int invocation; + lock (requestedScopes) + { + requestedScopes.Add(QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["scope"].ToString()); + invocation = requestedScopes.Count; + } + + if (invocation == 2) + { + // The read step-up: hold it open, like a user mid-login. + readStepUpEntered.TrySetResult(); + await completeReadStepUp.Task.WaitAsync(cancellationToken); + } + + return await HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + // The read-tool call is canceled while its step-up waits on the user, leaving that flow pending. + using var readCts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + var readCall = client.CallToolAsync("read-tool", cancellationToken: readCts.Token).AsTask(); + await readStepUpEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + readCts.Cancel(); + await Assert.ThrowsAnyAsync(() => readCall); + + // The write-tool call is challenged for "files:write" and starts handling that challenge while + // the read step-up is still pending; only then does the user complete the read step-up. + var writeCall = client.CallToolAsync("write-tool", cancellationToken: TestContext.Current.CancellationToken).AsTask(); + await writeChallengeBeingHandled.Task.WaitAsync(TestContext.Current.CancellationToken); + completeReadStepUp.TrySetResult(); + + var writeResult = await writeCall; + Assert.Equal("Write tool executed.", writeResult.Content[0].ToString()); + + // Three prompts in total: the initial connect, the read step-up, and a separate write step-up + // that carries the accumulated scopes instead of reusing the read step-up's token. + Assert.Equal(["mcp:tools", "files:read mcp:tools", "files:read files:write mcp:tools"], requestedScopes); + + // The stepped-up token now covers the read tool as well, with no further prompt. + var readResult = await client.CallToolAsync("read-tool", cancellationToken: TestContext.Current.CancellationToken); + Assert.Equal("Read tool executed.", readResult.Content[0].ToString()); + Assert.Equal(3, requestedScopes.Count); + } + [Fact] public async Task DisposingTransport_CancelsDetachedAuthorizationFlow() { @@ -2679,6 +2861,6 @@ await Assert.ThrowsAsync(() => McpClient.CreateAsync( await transport.DisposeAsync(); - await handlerCanceled.Task.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken); + await handlerCanceled.Task.WaitAsync(TestConstants.DefaultTimeout, TestContext.Current.CancellationToken); } } From 851d59ab8324638f1082f903bfb206785a137eea Mon Sep 17 00:00:00 2001 From: Peder Date: Sat, 29 Aug 2026 20:44:49 +0200 Subject: [PATCH 3/6] Join a pending same-scope step-up instead of rejecting the repeated challenge Once a step-up has been attempted, a 403 insufficient_scope challenge that adds no new scope is rejected as unproductive. That guard ran before the in-flight flow was considered, so when the step-up's caller canceled its request mid-login and another request drew the same challenge, the second request failed immediately even though the pending flow would have satisfied it. Join a pending flow that covers the challenge before applying the repeated-step-up rejection, and add a regression test for the case. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01C9kt1V3wMcWWkSMJEkrXKK --- .../Authentication/ClientOAuthProvider.cs | 9 ++ .../OAuth/AuthTests.cs | 125 ++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index 4eb0b2e23..44d4a16df 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -417,6 +417,15 @@ private async Task GetAccessTokenCoreAsync(HttpResponseMessage response, return steppedUpToken.AccessToken; } + // The step-up that already requested these scopes may still be pending: the request + // that started it was canceled while the user was completing it. Its outcome is what + // will satisfy this challenge, so join it rather than reject the challenge as repeated. + if (_inFlightAuthorizationCodeFlow is { IsCompleted: false } pendingStepUp && + InFlightAuthorizationCodeFlowCoversChallenge(protectedResourceMetadata)) + { + return await pendingStepUp.WaitAsync(cancellationToken).ConfigureAwait(false); + } + ThrowFailedToHandleUnauthorizedResponse( "A repeated insufficient_scope challenge added no scope beyond those already requested, " + "so step-up authorization cannot satisfy the request."); diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index 81aa0c06b..13c798deb 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -2821,6 +2821,131 @@ public async Task InteractiveAuthorization_PendingFlowIsNotReusedForChallengeReq Assert.Equal(3, requestedScopes.Count); } + [Fact] + public async Task InteractiveAuthorization_RepeatedChallengeForSameScopes_JoinsPendingStepUp() + { + // A step-up abandoned by its caller (canceled while the user is mid-login) is still pending. + // Another request challenged for the same scopes must join that flow instead of being rejected + // as an unproductive repeated step-up: the pending flow is exactly what will satisfy it. + Builder.Services.AddMcpServer() + .WithTools([ + McpServerTool.Create([McpServerTool(Name = "read-tool")] + (ClaimsPrincipal user) => + { + return "Read tool executed."; + }), + ]); + + var readChallengesRaised = 0; + var secondChallengeBeingHandled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var app = await StartMcpServerAsync(configureMiddleware: app => + { + // Fetching the protected resource metadata is the client's first step in handling a + // challenge. The authentication handler serves that document itself, so this observer must + // sit ahead of the authentication middleware (added explicitly below rather than + // auto-inserted at the front of the pipeline), while the challenge middleware below it + // needs the authenticated user. + app.Use(async (context, next) => + { + if (context.Request.Path == "/.well-known/oauth-protected-resource" && Volatile.Read(ref readChallengesRaised) == 2) + { + secondChallengeBeingHandled.TrySetResult(); + } + + await next(context); + }); + + app.UseAuthentication(); + app.UseAuthorization(); + + app.Use(async (context, next) => + { + if (context.Request.Method == HttpMethods.Post && context.Request.Path == "/") + { + context.Request.EnableBuffering(); + + var message = await JsonSerializer.DeserializeAsync( + context.Request.Body, + McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(JsonRpcMessage)), + context.RequestAborted) as JsonRpcMessage; + + context.Request.Body.Position = 0; + + if (message is JsonRpcRequest request && request.Method == "tools/call") + { + var toolCallParams = JsonSerializer.Deserialize( + request.Params, + McpJsonUtilities.DefaultOptions.GetTypeInfo(typeof(CallToolRequestParams))) as CallToolRequestParams; + + var scopeClaim = context.User.FindFirst("scope")?.Value ?? ""; + var scopeSet = new HashSet(scopeClaim.Split(' ')); + + if (toolCallParams?.Name == "read-tool" && !scopeSet.Contains("files:read")) + { + // Counted before the response goes out so the observer above sees it when + // the client's metadata fetch arrives. + Interlocked.Increment(ref readChallengesRaised); + + context.Response.StatusCode = StatusCodes.Status403Forbidden; + context.Response.Headers.WWWAuthenticate = $"Bearer error=\"insufficient_scope\", resource_metadata=\"{McpServerUrl}/.well-known/oauth-protected-resource\", scope=\"files:read\""; + await context.Response.StartAsync(context.RequestAborted); + await context.Response.Body.FlushAsync(context.RequestAborted); + return; + } + } + } + + await next(context); + }); + }); + + List requestedScopes = []; + var stepUpEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var completeStepUp = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await using var transport = CreateOAuthTransport(async (context, cancellationToken) => + { + int invocation; + lock (requestedScopes) + { + requestedScopes.Add(QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["scope"].ToString()); + invocation = requestedScopes.Count; + } + + if (invocation == 2) + { + // The step-up: hold it open, like a user mid-login. + stepUpEntered.TrySetResult(); + await completeStepUp.Task.WaitAsync(cancellationToken); + } + + return await HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + // The first read-tool call is canceled while its step-up waits on the user, leaving it pending. + using var firstCallCts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + var firstCall = client.CallToolAsync("read-tool", cancellationToken: firstCallCts.Token).AsTask(); + await stepUpEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + firstCallCts.Cancel(); + await Assert.ThrowsAnyAsync(() => firstCall); + + // The second call draws the same challenge and starts handling it while the step-up is still + // pending; only then does the user complete the step-up. + var secondCall = client.CallToolAsync("read-tool", cancellationToken: TestContext.Current.CancellationToken).AsTask(); + await secondChallengeBeingHandled.Task.WaitAsync(TestContext.Current.CancellationToken); + completeStepUp.TrySetResult(); + + var result = await secondCall; + Assert.Equal("Read tool executed.", result.Content[0].ToString()); + + // Two prompts in total: the initial connect and the single step-up both calls shared. + Assert.Equal(["mcp:tools", "files:read mcp:tools"], requestedScopes); + } + [Fact] public async Task DisposingTransport_CancelsDetachedAuthorizationFlow() { From 7e390ecf443afdb617140fa596e09711a0a539ec Mon Sep 17 00:00:00 2001 From: Peder Date: Sat, 29 Aug 2026 21:56:49 +0200 Subject: [PATCH 4/6] Reuse a detached flow that completes while a challenge is being handled A detached authorization-code flow can complete after a later challenge re-checked the token cache on acquiring the lock but before that challenge decided whether to join the flow (the metadata fetches in between make the window real). The challenge then saw a completed flow and started a second one, prompting the user again for a token that was already cached, unless a refresh token happened to be available. The same window existed in the repeated-step-up guard. Fold both sites into one helper that joins the in-flight flow while it is pending and otherwise reuses the token it cached, provided the flow requested every scope the challenge needs. Record those scopes from the effective scope placed in the authorization URL rather than the accumulated set, since offline_access augmentation and a ScopeSelector can make the two differ. Add a regression test that holds the second connect's metadata fetch until the first connect's detached flow has stored its token, using a cache that issues no refresh token so the cached access token is the only alternative to a second prompt. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01C9kt1V3wMcWWkSMJEkrXKK --- .../Authentication/ClientOAuthProvider.cs | 85 +++++++++----- .../OAuth/AuthTests.cs | 104 +++++++++++++++++- 2 files changed, 161 insertions(+), 28 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index 44d4a16df..bdfe31f28 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -417,13 +417,13 @@ private async Task GetAccessTokenCoreAsync(HttpResponseMessage response, return steppedUpToken.AccessToken; } - // The step-up that already requested these scopes may still be pending: the request - // that started it was canceled while the user was completing it. Its outcome is what - // will satisfy this challenge, so join it rather than reject the challenge as repeated. - if (_inFlightAuthorizationCodeFlow is { IsCompleted: false } pendingStepUp && - InFlightAuthorizationCodeFlowCoversChallenge(protectedResourceMetadata)) + // The step-up that already requested these scopes may still be pending (the request + // that started it was canceled while the user was completing it) or may have completed + // since the cache was read above. Either way its outcome is what satisfies this + // challenge, so reuse it rather than reject the challenge as repeated. + if (await TryReuseInFlightAuthorizationCodeFlowAsync(protectedResourceMetadata, usedAccessToken, cancellationToken).ConfigureAwait(false) is { } steppedUpAccessToken) { - return await pendingStepUp.WaitAsync(cancellationToken).ConfigureAwait(false); + return steppedUpAccessToken; } ThrowFailedToHandleUnauthorizedResponse( @@ -519,29 +519,60 @@ private async Task GetAccessTokenCoreAsync(HttpResponseMessage response, // Store auth server metadata for future refresh operations _authServerMetadata = authServerMetadata; - // Perform the OAuth flow. A caller that reaches this point while a previous caller's flow is - // still pending (that caller's request was canceled mid-flow, releasing the lock) joins the - // in-flight flow instead of starting a competing one, provided the flow requested every scope - // this challenge needs; see the _inFlightAuthorizationCodeFlow comment. - var flow = _inFlightAuthorizationCodeFlow; - if (flow is { IsCompleted: false } && !InFlightAuthorizationCodeFlowCoversChallenge(protectedResourceMetadata)) + // Perform the OAuth flow, unless the in-flight flow already satisfies this challenge: a caller + // that reaches this point while a previous caller's flow is still pending (that caller's + // request was canceled mid-flow, releasing the lock) joins it instead of starting a competing + // one, and a flow that completed during the metadata work above has cached the token this + // challenge needs. See the _inFlightAuthorizationCodeFlow comment. + if (await TryReuseInFlightAuthorizationCodeFlowAsync(protectedResourceMetadata, usedAccessToken, cancellationToken).ConfigureAwait(false) is { } reusedAccessToken) { - // The pending flow's token cannot satisfy this challenge, but two interactive flows must - // never be presented at once, so let it settle first. Its outcome, success or failure, is - // reported to the callers that joined it and is irrelevant here (Task.WhenAny never - // faults); this challenge then runs its own step-up for the accumulated scopes. - await Task.WhenAny(flow).WaitAsync(cancellationToken).ConfigureAwait(false); - flow = null; + return reusedAccessToken; } - if (flow is null || flow.IsCompleted) + if (_inFlightAuthorizationCodeFlow is { IsCompleted: false } pendingFlow) { - _inFlightAuthorizationCodeFlow = flow = StartAuthorizationCodeFlow(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); + // The pending flow did not request a scope this challenge needs, so its token cannot + // satisfy it, but two interactive flows must never be presented at once: let it settle + // first. Its outcome, success or failure, is reported to the callers that joined it and + // is irrelevant here (Task.WhenAny never faults). + await Task.WhenAny(pendingFlow).WaitAsync(cancellationToken).ConfigureAwait(false); } + var flow = _inFlightAuthorizationCodeFlow = StartAuthorizationCodeFlow(protectedResourceMetadata, authServerMetadata, _disposeCts.Token); return await flow.WaitAsync(cancellationToken).ConfigureAwait(false); } + /// + /// Satisfies the current challenge from the in-flight authorization-code flow when that flow requested + /// every scope the challenge needs: joins the flow while it is pending, or reuses the token it cached + /// if it completed after the caller last consulted the cache. Returns when there + /// is no such flow or nothing usable came of it, in which case the caller runs a flow of its own. + /// + private async Task TryReuseInFlightAuthorizationCodeFlowAsync(ProtectedResourceMetadata protectedResourceMetadata, string? usedAccessToken, CancellationToken cancellationToken) + { + if (_inFlightAuthorizationCodeFlow is not { } flow || !InFlightAuthorizationCodeFlowCoversChallenge(protectedResourceMetadata)) + { + return null; + } + + if (!flow.IsCompleted) + { + return await flow.WaitAsync(cancellationToken).ConfigureAwait(false); + } + + // A flow that ran to completion stored its token, and a token other than the one this challenge + // rejected is worth retrying with. A flow that faulted or was canceled stored nothing, and a + // long-completed flow's token is the rejected one itself. + if (flow.Status == TaskStatus.RanToCompletion && + await _tokenCache.GetTokensAsync(cancellationToken).ConfigureAwait(false) is { IsExpired: false } cached && + !string.Equals(cached.AccessToken, usedAccessToken, StringComparison.Ordinal)) + { + return cached.AccessToken; + } + + return null; + } + /// /// Returns whether the in-flight authorization-code flow requested every scope the current challenge /// requires, so that joining it can satisfy the challenge. A challenge that names no concrete scope is @@ -792,10 +823,12 @@ private Task StartAuthorizationCodeFlow( var codeChallenge = GenerateCodeChallenge(codeVerifier); var state = GenerateRandomBase64UrlValue(); - // Building the URL folds this challenge's scopes into _accumulatedScopes, so the accumulated set - // is now exactly the set of scopes this flow asks the authorization server for. - var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state); - _inFlightAuthorizationCodeFlowScopes = new HashSet(_accumulatedScopes, StringComparer.Ordinal); + // Record the scopes this flow actually asks the authorization server for: the effective scope + // placed in the URL, which offline_access augmentation and any ScopeSelector can make differ + // from _accumulatedScopes. + var scope = ComputeEffectiveScope(protectedResourceMetadata, authServerMetadata); + var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state, scope); + _inFlightAuthorizationCodeFlowScopes = new HashSet(scope is null ? [] : SplitScopes(scope), StringComparer.Ordinal); return CompleteAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, authUrl, state, codeVerifier, cancellationToken); } @@ -848,7 +881,8 @@ private Uri BuildAuthorizationUrl( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, string codeChallenge, - string state) + string state, + string? scope) { var resourceUri = GetResourceUri(protectedResourceMetadata); @@ -867,7 +901,6 @@ private Uri BuildAuthorizationUrl( queryParamsDictionary["resource"] = resourceUri; } - var scope = ComputeEffectiveScope(protectedResourceMetadata, authServerMetadata); if (!string.IsNullOrEmpty(scope)) { queryParamsDictionary["scope"] = scope!; diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index 13c798deb..073be8b39 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -2507,6 +2507,32 @@ public async Task AuthorizationFlow_ScopeSelector_ReturningEmpty_OmitsScopeParam Assert.False(scopePresent); } + /// + /// An in-memory token cache that signals when tokens are first stored and can model an authorization + /// server that issues no refresh token by discarding it. + /// + private sealed class SignalingTokenCache(bool discardRefreshTokens = false) : ITokenCache + { + private TokenContainer? _tokens; + + public TaskCompletionSource TokensStored { get; } = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public ValueTask StoreTokensAsync(TokenContainer tokens, CancellationToken cancellationToken) + { + if (discardRefreshTokens) + { + tokens.RefreshToken = null; + } + + Volatile.Write(ref _tokens, tokens); + TokensStored.TrySetResult(); + return default; + } + + public ValueTask GetTokensAsync(CancellationToken cancellationToken) => + new(Volatile.Read(ref _tokens)); + } + private HttpClientTransport CreateOAuthTransport( Func>? authorizationCallbackHandler = null) => @@ -2707,7 +2733,7 @@ public async Task InteractiveAuthorization_PendingFlowIsNotReusedForChallengeReq // front of the pipeline), while the challenge middleware below it needs the authenticated user. app.Use(async (context, next) => { - if (context.Request.Path == "/.well-known/oauth-protected-resource" && Volatile.Read(ref writeChallengeRaised) == 1) + if (context.Request.Path.StartsWithSegments("/.well-known/oauth-protected-resource") && Volatile.Read(ref writeChallengeRaised) == 1) { writeChallengeBeingHandled.TrySetResult(); } @@ -2848,7 +2874,7 @@ public async Task InteractiveAuthorization_RepeatedChallengeForSameScopes_JoinsP // needs the authenticated user. app.Use(async (context, next) => { - if (context.Request.Path == "/.well-known/oauth-protected-resource" && Volatile.Read(ref readChallengesRaised) == 2) + if (context.Request.Path.StartsWithSegments("/.well-known/oauth-protected-resource") && Volatile.Read(ref readChallengesRaised) == 2) { secondChallengeBeingHandled.TrySetResult(); } @@ -2946,6 +2972,80 @@ public async Task InteractiveAuthorization_RepeatedChallengeForSameScopes_JoinsP Assert.Equal(["mcp:tools", "files:read mcp:tools"], requestedScopes); } + [Fact] + public async Task InteractiveAuthorization_FlowCompletingDuringChallengeHandling_IsReusedNotRestarted() + { + // A detached flow can complete while a later challenge is already being handled, after that + // challenge re-checked the token cache on acquiring the lock but before it decides whether to + // join the flow. Its token must then be reused; starting a second flow would prompt the user + // again for a token that is already cached. With no refresh token available (the test + // authorization server always issues one, so the cache discards it), the cached access token + // is the only alternative to a second prompt. + var tokenCache = new SignalingTokenCache(discardRefreshTokens: true); + var handlerInvocations = 0; + var handlerEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var completeAuthorization = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var secondConnectStarted = 0; + + await using var app = await StartMcpServerAsync(configureMiddleware: app => + { + // Fetching the protected resource metadata is the client's first step in handling a + // challenge. The authentication handler serves that document itself, so this observer must + // sit ahead of the authentication middleware (added explicitly below rather than + // auto-inserted at the front of the pipeline). Once the second connect's challenge fetches + // it, the user completes the first connect's flow, and the response is held back until that + // flow has stored its token, so the challenge finds the flow completed when it decides. + app.Use(async (context, next) => + { + if (context.Request.Path.StartsWithSegments("/.well-known/oauth-protected-resource") && Volatile.Read(ref secondConnectStarted) == 1) + { + completeAuthorization.TrySetResult(); + await tokenCache.TokensStored.Task.WaitAsync(TestConstants.DefaultTimeout, context.RequestAborted); + } + + await next(context); + }); + + app.UseAuthentication(); + app.UseAuthorization(); + }); + + await using var transport = new HttpClientTransport(new() + { + Endpoint = new(McpServerUrl), + OAuth = new() + { + ClientId = "demo-client", + ClientSecret = "demo-secret", + RedirectUri = new Uri("http://localhost:1179/callback"), + TokenCache = tokenCache, + AuthorizationCallbackHandler = async (context, cancellationToken) => + { + Interlocked.Increment(ref handlerInvocations); + handlerEntered.TrySetResult(); + await completeAuthorization.Task.WaitAsync(cancellationToken); + return await HandleAuthorizationUrlAsync(context, cancellationToken); + }, + }, + }, HttpClient, LoggerFactory); + + var clientOptions = new McpClientOptions { ProtocolVersion = "2025-06-18" }; + + using var firstConnectCts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken); + var firstConnect = McpClient.CreateAsync( + transport, clientOptions, loggerFactory: LoggerFactory, cancellationToken: firstConnectCts.Token); + + await handlerEntered.Task.WaitAsync(TestContext.Current.CancellationToken); + firstConnectCts.Cancel(); + await Assert.ThrowsAnyAsync(() => firstConnect); + + Volatile.Write(ref secondConnectStarted, 1); + await using var client = await McpClient.CreateAsync( + transport, clientOptions, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(1, handlerInvocations); + } + [Fact] public async Task DisposingTransport_CancelsDetachedAuthorizationFlow() { From 93fd21e0fa7d99c656a5f6ba2b469bbdef8cc42e Mon Sep 17 00:00:00 2001 From: Peder Date: Sat, 29 Aug 2026 22:23:49 +0200 Subject: [PATCH 5/6] Snapshot client credentials for the lifetime of a detached authorization flow A detached authorization-code flow outlives the token acquisition lock once its initiating request is canceled, yet its code exchange and token persistence read the provider's mutable credential fields. A later challenge holding the lock can rebind and re-register those fields for a different authorization server while the flow is pending, so the original code would be exchanged with the wrong client credentials and cached under the wrong issuer. Capture an immutable ClientCredentials snapshot under the lock when a flow starts (and when a refresh runs) and use it for the authorization URL, the token request, and the persisted registration. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01C9kt1V3wMcWWkSMJEkrXKK --- .../Authentication/ClientOAuthProvider.cs | 63 +++++++++++++------ 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index bdfe31f28..17e449c16 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -85,7 +85,9 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient, IDisposable // request cannot be satisfied by its token, so it waits for the flow to settle and then runs its // own step-up; at most one interactive flow is ever presented to the user at a time. The flow // itself is bounded by the authorization callback handler's own completion and canceled on - // provider disposal. + // provider disposal. Because it outlives the lock, it carries its own snapshot of the client + // credentials (see ClientCredentials) rather than reading the mutable fields a later challenge may + // rebind for another authorization server while it is pending. private Task? _inFlightAuthorizationCodeFlow; private HashSet? _inFlightAuthorizationCodeFlowScopes; private readonly CancellationTokenSource _disposeCts = new(); @@ -785,6 +787,8 @@ private static IEnumerable GetWellKnownAuthorizationServerMetadataUris(Uri private async Task RefreshTokensAsync(string refreshToken, string? resourceUri, AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) { + var credentials = CaptureClientCredentials(); + Dictionary formFields = new() { ["grant_type"] = "refresh_token", @@ -796,7 +800,7 @@ private static IEnumerable GetWellKnownAuthorizationServerMetadataUris(Uri formFields["resource"] = resourceUri; } - using var request = CreateTokenRequest(authServerMetadata.TokenEndpoint, formFields); + using var request = CreateTokenRequest(authServerMetadata.TokenEndpoint, formFields, credentials); using var httpResponse = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); @@ -805,7 +809,7 @@ private static IEnumerable GetWellKnownAuthorizationServerMetadataUris(Uri return null; } - var tokens = await HandleSuccessfulTokenResponseAsync(httpResponse, cancellationToken).ConfigureAwait(false); + var tokens = await HandleSuccessfulTokenResponseAsync(httpResponse, credentials, cancellationToken).ConfigureAwait(false); LogOAuthTokenRefreshCompleted(); return tokens.AccessToken; } @@ -819,6 +823,9 @@ private Task StartAuthorizationCodeFlow( AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) { + // The flow outlives this lock scope, so it works from a snapshot of the client credentials + // rather than the mutable fields; see the _inFlightAuthorizationCodeFlow comment. + var credentials = CaptureClientCredentials(); var codeVerifier = GenerateRandomBase64UrlValue(); var codeChallenge = GenerateCodeChallenge(codeVerifier); var state = GenerateRandomBase64UrlValue(); @@ -827,15 +834,16 @@ private Task StartAuthorizationCodeFlow( // placed in the URL, which offline_access augmentation and any ScopeSelector can make differ // from _accumulatedScopes. var scope = ComputeEffectiveScope(protectedResourceMetadata, authServerMetadata); - var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state, scope); + var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, credentials.ClientId, codeChallenge, state, scope); _inFlightAuthorizationCodeFlowScopes = new HashSet(scope is null ? [] : SplitScopes(scope), StringComparer.Ordinal); - return CompleteAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, authUrl, state, codeVerifier, cancellationToken); + return CompleteAuthorizationCodeFlowAsync(protectedResourceMetadata, authServerMetadata, credentials, authUrl, state, codeVerifier, cancellationToken); } private async Task CompleteAuthorizationCodeFlowAsync( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, + ClientCredentials credentials, Uri authUrl, string state, string codeVerifier, @@ -872,6 +880,7 @@ private async Task CompleteAuthorizationCodeFlowAsync( return await ExchangeCodeForTokenAsync( protectedResourceMetadata, authServerMetadata, + credentials, authResult.Code!, codeVerifier, cancellationToken).ConfigureAwait(false); @@ -880,6 +889,7 @@ private async Task CompleteAuthorizationCodeFlowAsync( private Uri BuildAuthorizationUrl( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, + string clientId, string codeChallenge, string state, string? scope) @@ -888,7 +898,7 @@ private Uri BuildAuthorizationUrl( var queryParamsDictionary = new Dictionary { - ["client_id"] = GetClientIdOrThrow(), + ["client_id"] = clientId, ["redirect_uri"] = _redirectUri.ToString(), ["response_type"] = "code", ["code_challenge"] = codeChallenge, @@ -929,6 +939,7 @@ private Uri BuildAuthorizationUrl( private async Task ExchangeCodeForTokenAsync( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, + ClientCredentials credentials, string authorizationCode, string codeVerifier, CancellationToken cancellationToken) @@ -948,33 +959,45 @@ private async Task ExchangeCodeForTokenAsync( formFields["resource"] = resourceUri; } - using var request = CreateTokenRequest(authServerMetadata.TokenEndpoint, formFields); + using var request = CreateTokenRequest(authServerMetadata.TokenEndpoint, formFields, credentials); using var httpResponse = await _httpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); await httpResponse.EnsureSuccessStatusCodeWithResponseBodyAsync(cancellationToken).ConfigureAwait(false); - var tokens = await HandleSuccessfulTokenResponseAsync(httpResponse, cancellationToken).ConfigureAwait(false); + var tokens = await HandleSuccessfulTokenResponseAsync(httpResponse, credentials, cancellationToken).ConfigureAwait(false); LogOAuthAuthorizationCompleted(); return tokens.AccessToken; } + /// + /// The client registration a token request is made with and persisted alongside the tokens it yields. + /// Captured under _tokenAcquisitionLock via so that work + /// which outlives the lock, such as a detached authorization-code flow, is unaffected by a later + /// challenge rebinding the provider's mutable credential fields to another authorization server. + /// + private sealed record ClientCredentials(string ClientId, string? ClientSecret, string? TokenEndpointAuthMethod, string? AuthorizationServer); + + /// Snapshots the current client registration. Callers must hold _tokenAcquisitionLock. + private ClientCredentials CaptureClientCredentials() => + new(GetClientIdOrThrow(), _clientSecret, _tokenEndpointAuthMethod, _clientCredentialsAuthorizationServer); + /// /// Creates an HTTP request to the token endpoint, applying the appropriate authentication - /// method based on . + /// method based on . /// - private HttpRequestMessage CreateTokenRequest(Uri tokenEndpoint, Dictionary formFields) + private HttpRequestMessage CreateTokenRequest(Uri tokenEndpoint, Dictionary formFields, ClientCredentials credentials) { HttpRequestMessage request = new(HttpMethod.Post, tokenEndpoint); - var clientId = GetClientIdOrThrow(); - if (string.Equals(_tokenEndpointAuthMethod, "client_secret_basic", StringComparison.Ordinal)) + var clientId = credentials.ClientId; + if (string.Equals(credentials.TokenEndpointAuthMethod, "client_secret_basic", StringComparison.Ordinal)) { // Per RFC 6749 §2.3.1: send client_id:client_secret as HTTP Basic auth. request.Headers.Authorization = new( "Basic", - Convert.ToBase64String(Encoding.UTF8.GetBytes($"{Uri.EscapeDataString(clientId)}:{Uri.EscapeDataString(_clientSecret ?? string.Empty)}"))); + Convert.ToBase64String(Encoding.UTF8.GetBytes($"{Uri.EscapeDataString(clientId)}:{Uri.EscapeDataString(credentials.ClientSecret ?? string.Empty)}"))); } - else if (string.Equals(_tokenEndpointAuthMethod, "none", StringComparison.Ordinal)) + else if (string.Equals(credentials.TokenEndpointAuthMethod, "none", StringComparison.Ordinal)) { // Public client: include client_id in the body but no secret. formFields["client_id"] = clientId; @@ -983,14 +1006,14 @@ private HttpRequestMessage CreateTokenRequest(Uri tokenEndpoint, Dictionary HandleSuccessfulTokenResponseAsync(HttpResponseMessage response, CancellationToken cancellationToken) + private async Task HandleSuccessfulTokenResponseAsync(HttpResponseMessage response, ClientCredentials credentials, CancellationToken cancellationToken) { using var stream = await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false); var tokenResponse = await JsonSerializer.DeserializeAsync(stream, McpJsonUtilities.JsonContext.Default.TokenResponse, cancellationToken).ConfigureAwait(false); @@ -1015,10 +1038,10 @@ private async Task HandleSuccessfulTokenResponseAsync(HttpRespon ObtainedAt = DateTimeOffset.UtcNow, // Persist the client registration alongside the tokens so a durable cache can use the // refresh token after a process restart without re-running dynamic client registration. - ClientId = _clientId, - ClientSecret = _clientSecret, - TokenEndpointAuthMethod = _tokenEndpointAuthMethod, - AuthorizationServer = _clientCredentialsAuthorizationServer, + ClientId = credentials.ClientId, + ClientSecret = credentials.ClientSecret, + TokenEndpointAuthMethod = credentials.TokenEndpointAuthMethod, + AuthorizationServer = credentials.AuthorizationServer, }; await _tokenCache.StoreTokensAsync(tokens, cancellationToken).ConfigureAwait(false); From 41fe07e9340b3d4eaf05296cc3619a1703918113 Mon Sep 17 00:00:00 2001 From: Peder Date: Sat, 29 Aug 2026 22:47:50 +0200 Subject: [PATCH 6/6] Qualify InitializationTimeout docs: only a compatible challenge reuses the flow Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01C9kt1V3wMcWWkSMJEkrXKK --- src/ModelContextProtocol.Core/Client/McpClientOptions.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs index 904c6040d..3f5f6cb7d 100644 --- a/src/ModelContextProtocol.Core/Client/McpClientOptions.cs +++ b/src/ModelContextProtocol.Core/Client/McpClientOptions.cs @@ -95,8 +95,9 @@ public sealed class McpClientOptions /// how long the connect attempt waits for the user to complete the browser-based authorization, so /// increase this value to cover the time a person needs to complete the login, not just the network /// round-trips. Reaching it fails the connect attempt but does not cancel the authorization flow - /// itself: the flow keeps running so that a later challenge on the same transport reuses its outcome - /// rather than prompting the user again, and only disposing the transport cancels it. + /// itself: the flow keeps running so that a compatible later challenge on the same transport (one + /// whose scopes the flow requested) reuses its outcome rather than prompting the user again, and only + /// disposing the transport cancels it. /// /// public TimeSpan InitializationTimeout { get; set; } = TimeSpan.FromSeconds(60);