From 4d279ab6dd671cbca9a42018c7a52aa7f0542a5b Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 20 Jul 2026 18:14:39 -0700 Subject: [PATCH 1/2] Validate OAuth authorization state Generate a unique state for each authorization code flow and require callbacks to return the matching redirect state before token exchange. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- samples/ProtectedMcpClient/Program.cs | 3 +- .../Authentication/AuthorizationResult.cs | 19 ++++- .../Authentication/ClientOAuthOptions.cs | 18 ++-- .../Authentication/ClientOAuthProvider.cs | 52 ++++++++++-- .../OAuth/AuthTests.cs | 85 +++++++++++++++++-- .../OAuth/OAuthTestBase.cs | 1 + .../Program.cs | 9 +- .../Program.cs | 5 ++ 8 files changed, 168 insertions(+), 24 deletions(-) diff --git a/samples/ProtectedMcpClient/Program.cs b/samples/ProtectedMcpClient/Program.cs index 9b805ad36..6cda8e795 100644 --- a/samples/ProtectedMcpClient/Program.cs +++ b/samples/ProtectedMcpClient/Program.cs @@ -98,6 +98,7 @@ var context = await listener.GetContextAsync(); var query = HttpUtility.ParseQueryString(context.Request.Url?.Query ?? string.Empty); var code = query["code"]; + var state = query["state"]; var iss = query["iss"]; var error = query["error"]; @@ -121,7 +122,7 @@ } Console.WriteLine("Authorization code received successfully."); - return new AuthorizationResult { Code = code, Iss = iss }; + return new AuthorizationResult { Code = code, State = state, Iss = iss }; } catch (Exception ex) { diff --git a/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs b/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs index d5bebc502..4da082f7b 100644 --- a/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs +++ b/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs @@ -1,11 +1,16 @@ namespace ModelContextProtocol.Authentication; /// -/// Represents the result of an OAuth authorization redirect, containing the authorization code -/// and optionally the issuer identifier from the authorization response. +/// Represents the result of an OAuth authorization redirect, containing the authorization code, +/// state, and optionally the issuer identifier from the authorization response. /// /// /// +/// The property must be populated from the state query parameter in the +/// redirect URI. The SDK validates it against the value sent in the authorization request to bind +/// the response to the initiating transaction and mitigate cross-site request forgery attacks. +/// +/// /// The property should be populated from the iss query parameter in the /// redirect URI when present, as specified by /// RFC 9207. @@ -20,6 +25,16 @@ public sealed class AuthorizationResult /// public string? Code { get; init; } + /// + /// Gets the state value returned in the authorization response. + /// + /// + /// Implementations of must populate this + /// property from the state query parameter of the redirect URI callback. The SDK requires an + /// exact match with the state sent in the authorization request before exchanging the authorization code. + /// + public string? State { get; init; } + /// /// Gets the issuer identifier returned in the authorization response per /// RFC 9207. diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs index b536207c3..df1c44c1f 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs @@ -80,8 +80,10 @@ public sealed class ClientOAuthOptions /// /// /// Custom implementations might open a browser, start an HTTP listener, or use other mechanisms to capture - /// the authorization response. They should return both the code and iss query parameters - /// from the redirect URI callback. This enables the SDK to validate the iss parameter per + /// the authorization response. They must return the code and state query parameters, + /// and should return the iss query parameter when present, from the redirect URI callback. + /// The SDK requires an exact state match before exchanging the code. Returning iss enables + /// the SDK to validate the parameter per /// RFC 9207, which mitigates /// mix-up attacks. /// @@ -96,9 +98,10 @@ public sealed class ClientOAuthOptions /// /// /// - /// This delegate returns only the authorization code and cannot provide the iss parameter from - /// the authorization response. Consequently, RFC 9207 issuer validation is skipped when this delegate - /// is used. Use for issuer-aware authorization flows. + /// This delegate returns only the authorization code and cannot provide the state or iss + /// parameter from the authorization response. Consequently, state and RFC 9207 issuer validation are + /// skipped when this delegate is used. Use for response-bound, + /// issuer-aware authorization flows. /// /// /// This property cannot be configured together with . @@ -139,8 +142,9 @@ public sealed class ClientOAuthOptions /// /// /// - /// Parameters specified cannot override or append to any automatically set parameters like the "redirect_uri", - /// which should instead be configured via . + /// Parameters specified cannot override or append to any automatically set parameters like + /// redirect_uri or state. The redirect URI should instead be configured via + /// , while state is generated uniquely for each authorization transaction. /// /// public IDictionary AdditionalAuthorizationParameters { get; set; } = new Dictionary(); diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index 632fa9e19..fc8c5a525 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -32,6 +32,7 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient private readonly IDictionary _additionalAuthorizationParameters; private readonly Func, Uri?> _authServerSelector; private readonly Func> _authorizationCallbackHandler; + private readonly bool _validateAuthorizationResponseState; private readonly bool _validateAuthorizationResponseIssuer; private readonly Uri? _clientMetadataDocumentUri; @@ -120,6 +121,7 @@ public ClientOAuthProvider( if (options.AuthorizationCallbackHandler is not null) { _authorizationCallbackHandler = options.AuthorizationCallbackHandler; + _validateAuthorizationResponseState = true; _validateAuthorizationResponseIssuer = true; } else if (authorizationRedirectDelegate is not null) @@ -131,11 +133,13 @@ public ClientOAuthProvider( context.RedirectUri, cancellationToken).ConfigureAwait(false), }; + _validateAuthorizationResponseState = false; _validateAuthorizationResponseIssuer = false; } else { _authorizationCallbackHandler = DefaultAuthorizationUrlHandler; + _validateAuthorizationResponseState = true; _validateAuthorizationResponseIssuer = true; } @@ -155,11 +159,11 @@ public ClientOAuthProvider( private static Uri? DefaultAuthServerSelector(IReadOnlyList availableServers) => availableServers.FirstOrDefault(); /// - /// Default authorization URL handler that displays the URL to the user for manual input. + /// Default authorization URL handler that displays the URL to the user and parses the resulting redirect URL. /// /// The context containing the authorization and redirect URIs. /// The to monitor for cancellation requests. - /// The authorization result entered by the user, or null if none was provided. + /// The authorization result parsed from the redirect URL, or null if no valid URL was provided. private static Task DefaultAuthorizationUrlHandler( AuthorizationCallbackContext context, CancellationToken cancellationToken) @@ -179,6 +183,7 @@ public ClientOAuthProvider( return Task.FromResult(new() { Code = queryParams["code"], + State = queryParams["state"], Iss = queryParams["iss"], }); } @@ -687,10 +692,11 @@ private async Task InitiateAuthorizationCodeFlowAsync( AuthorizationServerMetadata authServerMetadata, CancellationToken cancellationToken) { - var codeVerifier = GenerateCodeVerifier(); + var codeVerifier = GenerateRandomBase64UrlValue(); var codeChallenge = GenerateCodeChallenge(codeVerifier); + var state = GenerateRandomBase64UrlValue(); - var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge); + var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state); var authResult = await _authorizationCallbackHandler( new AuthorizationCallbackContext @@ -700,7 +706,17 @@ private async Task InitiateAuthorizationCodeFlowAsync( }, cancellationToken).ConfigureAwait(false); - if (authResult is null || string.IsNullOrEmpty(authResult.Code)) + if (authResult is null) + { + ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null authorization result."); + } + + if (_validateAuthorizationResponseState) + { + ValidateStateResponse(authResult!.State, state); + } + + if (string.IsNullOrEmpty(authResult.Code)) { ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null or empty authorization code."); } @@ -721,7 +737,8 @@ private async Task InitiateAuthorizationCodeFlowAsync( private Uri BuildAuthorizationUrl( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, - string codeChallenge) + string codeChallenge, + string state) { var resourceUri = GetResourceUri(protectedResourceMetadata); @@ -732,6 +749,7 @@ private Uri BuildAuthorizationUrl( ["response_type"] = "code", ["code_challenge"] = codeChallenge, ["code_challenge_method"] = "S256", + ["state"] = state, }; if (resourceUri is not null) @@ -1107,6 +1125,26 @@ private bool ChallengeIntroducesNewScopes(ProtectedResourceMetadata protectedRes return scope + " " + OfflineAccess; } + /// + /// Validates that an authorization response is bound to the transaction that initiated it. + /// + /// The state returned in the authorization response. + /// The state sent in the authorization request. + private static void ValidateStateResponse(string? state, string expectedState) + { + if (string.IsNullOrEmpty(state)) + { + ThrowFailedToHandleUnauthorizedResponse( + "The authorization response did not include the required state parameter."); + } + + if (!string.Equals(state, expectedState, StringComparison.Ordinal)) + { + ThrowFailedToHandleUnauthorizedResponse( + "The authorization response state did not match the state sent in the authorization request."); + } + } + /// /// Validates the iss parameter from an authorization response per /// RFC 9207. @@ -1378,7 +1416,7 @@ private async Task ExtractProtectedResourceMetadata(H yield return (new Uri($"{hostBase}{ProtectedResourceMetadataWellKnownPath}"), new Uri(hostBase)); } - private static string GenerateCodeVerifier() + private static string GenerateRandomBase64UrlValue() { #if NET9_0_OR_GREATER Span bytes = stackalloc byte[32]; diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index 2aca606e1..693c77943 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -176,6 +176,77 @@ public void HttpClientTransport_RejectsBothAuthorizationCallbacks() #pragma warning restore MCP9007 } + [Fact] + public async Task CanAuthenticate_WhenAuthorizationResponseStateMatches() + { + await using var app = await StartMcpServerAsync(); + + string? requestedState = null; + await using var transport = CreateOAuthTransport((context, cancellationToken) => + { + requestedState = QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["state"]; + return HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.NotNull(requestedState); + Assert.True(requestedState.Length >= 43); + Assert.Equal(1, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Fact] + public async Task CannotAuthenticate_WhenAuthorizationResponseStateIsMissing() + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport( + (_, _) => Task.FromResult(new() { Code = "unused-code" })); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains("did not include the required state parameter", ex.Message); + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Fact] + public async Task CannotAuthenticate_WhenAuthorizationResponseStateMismatches() + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport( + (_, _) => Task.FromResult( + new() { Code = "unused-code", State = "unexpected-state" })); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains("state did not match", ex.Message); + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Fact] + public async Task AuthorizationRequests_UseUniqueStateValues() + { + await using var app = await StartMcpServerAsync(); + List requestedStates = []; + + for (var i = 0; i < 2; i++) + { + await using var transport = CreateOAuthTransport((context, cancellationToken) => + { + requestedStates.Add(QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["state"].ToString()); + return HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + } + + Assert.Equal(2, requestedStates.Count); + Assert.Equal(2, requestedStates.Distinct(StringComparer.Ordinal).Count()); + } + [Fact] public async Task CannotAuthenticate_WithoutOAuthConfiguration() { @@ -550,8 +621,10 @@ public async Task CanAuthenticate_WithExtraParams() Assert.Contains("custom_param=custom_value", lastAuthorizationUri?.Query); } - [Fact] - public async Task CannotOverrideExistingParameters_WithExtraParams() + [Theory] + [InlineData("redirect_uri")] + [InlineData("state")] + public async Task CannotOverrideExistingParameters_WithExtraParams(string parameterName) { await using var app = await StartMcpServerAsync(); @@ -566,7 +639,7 @@ public async Task CannotOverrideExistingParameters_WithExtraParams() AuthorizationCallbackHandler = HandleAuthorizationUrlAsync, AdditionalAuthorizationParameters = new Dictionary { - ["redirect_uri"] = "custom_value", + [parameterName] = "custom_value", } }, }, HttpClient, LoggerFactory); @@ -2434,7 +2507,9 @@ public async Task AuthorizationFlow_ScopeSelector_ReturningEmpty_OmitsScopeParam Assert.False(scopePresent); } - private HttpClientTransport CreateOAuthTransport() => + private HttpClientTransport CreateOAuthTransport( + Func>? + authorizationCallbackHandler = null) => new(new() { Endpoint = new(McpServerUrl), @@ -2443,7 +2518,7 @@ private HttpClientTransport CreateOAuthTransport() => ClientId = "demo-client", ClientSecret = "demo-secret", RedirectUri = new Uri("http://localhost:1179/callback"), - AuthorizationCallbackHandler = HandleAuthorizationUrlAsync, + AuthorizationCallbackHandler = authorizationCallbackHandler ?? HandleAuthorizationUrlAsync, }, }, HttpClient, LoggerFactory); diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs index 5f150e99d..80167b0c9 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs @@ -120,6 +120,7 @@ protected async Task StartMcpServerAsync(string path = "", strin return new ModelContextProtocol.Authentication.AuthorizationResult { Code = queryParams["code"], + State = queryParams["state"], Iss = queryParams.TryGetValue("iss", out var iss) ? (string?)iss : null, }; } diff --git a/tests/ModelContextProtocol.ConformanceClient/Program.cs b/tests/ModelContextProtocol.ConformanceClient/Program.cs index a52e8cfb5..f9beea94a 100644 --- a/tests/ModelContextProtocol.ConformanceClient/Program.cs +++ b/tests/ModelContextProtocol.ConformanceClient/Program.cs @@ -360,9 +360,10 @@ if (location is not null && !string.IsNullOrEmpty(location.Query)) { - // Parse query string to extract "code" and "iss" parameters + // Parse query string to extract "code", "state", and "iss" parameters var query = location.Query.TrimStart('?'); string? code = null; + string? state = null; string? iss = null; foreach (var pair in query.Split('&')) { @@ -373,6 +374,10 @@ { code = HttpUtility.UrlDecode(parts[1]); } + else if (parts[0] == "state") + { + state = HttpUtility.UrlDecode(parts[1]); + } else if (parts[0] == "iss") { iss = HttpUtility.UrlDecode(parts[1]); @@ -382,7 +387,7 @@ if (code is not null) { - return new AuthorizationResult { Code = code, Iss = iss }; + return new AuthorizationResult { Code = code, State = state, Iss = iss }; } } diff --git a/tests/ModelContextProtocol.TestOAuthServer/Program.cs b/tests/ModelContextProtocol.TestOAuthServer/Program.cs index 6bedbc4f5..73663dc94 100644 --- a/tests/ModelContextProtocol.TestOAuthServer/Program.cs +++ b/tests/ModelContextProtocol.TestOAuthServer/Program.cs @@ -28,6 +28,7 @@ public sealed class Program private readonly ConcurrentDictionary _clients = new(); private readonly ConcurrentQueue _metadataRequests = new(); + private int _authorizationCodeTokenRequestCount; private readonly RSA _rsa; private readonly string _keyId; @@ -137,6 +138,9 @@ public Program(ILoggerProvider? loggerProvider = null, IConnectionListenerFactor public HashSet DisabledMetadataPaths { get; } = new(StringComparer.OrdinalIgnoreCase); public IReadOnlyCollection MetadataRequests => _metadataRequests.ToArray(); + /// Gets the number of authorization-code token exchange requests received. + public int AuthorizationCodeTokenRequestCount => Volatile.Read(ref _authorizationCodeTokenRequestCount); + /// Gets the scope field from the most recent Dynamic Client Registration request. public string? LastRegistrationScope { get; private set; } @@ -460,6 +464,7 @@ IResult HandleMetadataRequest(HttpContext context, string? issuerPath = null) if (grant_type == "authorization_code") { + Interlocked.Increment(ref _authorizationCodeTokenRequestCount); var code = form["code"].ToString(); var code_verifier = form["code_verifier"].ToString(); var redirect_uri = form["redirect_uri"].ToString(); From 059a2092b307a2f6bbf80417f1c3af1a15e87ec6 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 27 Jul 2026 11:28:17 -0700 Subject: [PATCH 2/2] Address OAuth state review feedback Clarify the default callback contract, use callback-neutral diagnostics, and document the state-validation limitation of the obsolete callback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/list-of-diagnostics.md | 2 +- .../Authentication/ClientOAuthProvider.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/list-of-diagnostics.md b/docs/list-of-diagnostics.md index deddad8e1..13aab7ebc 100644 --- a/docs/list-of-diagnostics.md +++ b/docs/list-of-diagnostics.md @@ -45,4 +45,4 @@ When APIs are marked as obsolete, a diagnostic is emitted to warn users that the | `MCP9004` | In place | opts into the legacy SSE transport which has no built-in HTTP-level backpressure. Use Streamable HTTP instead. See [Stateless — Legacy SSE transport](xref:stateless#legacy-sse-transport) for details. | | `MCP9005` | In place | The Roots, Sampling, and Logging features are deprecated as of specification version 2026-07-28 and may be removed in a future version. See SEP-2577 for more information. | | `MCP9006` | In place | The stateful Streamable HTTP configuration knobs on — `EventStreamStore`, `SessionMigrationHandler`, `PerSessionExecutionContext`, `IdleTimeout`, and `MaxIdleSessionCount` — only apply when `Stateless = false`. Starting with the `2026-07-28` protocol revision, Streamable HTTP no longer supports sessions, and the SDK now defaults `Stateless` to `true`. These knobs remain available for back-compat with the legacy stateful Streamable HTTP transport but new code should target the stateless path. | -| `MCP9007` | In place | `AuthorizationRedirectDelegate` and `ClientOAuthOptions.AuthorizationRedirectDelegate` are retained for source and binary compatibility but cannot provide the RFC 9207 authorization-response issuer. Use `ClientOAuthOptions.AuthorizationCallbackHandler` for issuer-aware authorization flows. | +| `MCP9007` | In place | `AuthorizationRedirectDelegate` and `ClientOAuthOptions.AuthorizationRedirectDelegate` are retained for source and binary compatibility but cannot provide the authorization-response state or RFC 9207 issuer. State and issuer validation are skipped when these APIs are used. Use `ClientOAuthOptions.AuthorizationCallbackHandler` for response-bound, issuer-aware authorization flows. | diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index fc8c5a525..a20ef9f51 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -163,7 +163,7 @@ public ClientOAuthProvider( /// /// The context containing the authorization and redirect URIs. /// The to monitor for cancellation requests. - /// The authorization result parsed from the redirect URL, or null if no valid URL was provided. + /// The authorization result parsed from the redirect URL. private static Task DefaultAuthorizationUrlHandler( AuthorizationCallbackContext context, CancellationToken cancellationToken) @@ -718,7 +718,7 @@ private async Task InitiateAuthorizationCodeFlowAsync( if (string.IsNullOrEmpty(authResult.Code)) { - ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null or empty authorization code."); + ThrowFailedToHandleUnauthorizedResponse("The authorization callback returned a null or empty authorization code."); } if (_validateAuthorizationResponseIssuer)