From fc8697738289ce96aa1751997b54414ca17fbe2f Mon Sep 17 00:00:00 2001 From: Pranav Senthilnathan Date: Mon, 13 Jul 2026 13:42:24 -0400 Subject: [PATCH 1/2] Enforce PKCE metadata compliance in OAuth client (#730) The OAuth client defaulted code_challenge_methods_supported to S256 when absent from authorization server metadata, masking non-compliant servers. Per the MCP 2025-11-25 spec, clients MUST verify PKCE support via metadata and refuse to proceed without it. Discovery now skips any well-known endpoint whose metadata omits PKCE support or lacks S256, falling through to the next endpoint (OAuth 2.0 vs OpenID Connect documents can differ), and fails only once all endpoints are exhausted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Authentication/ClientOAuthProvider.cs | 46 +++++++++-- .../OAuth/AuthTests.cs | 79 +++++++++++++++++++ .../Program.cs | 18 ++++- 3 files changed, 135 insertions(+), 8 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index c3b1af736..c10b2f73f 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -368,6 +368,7 @@ private async Task GetAuthServerMetadataAsync(Uri a { foreach (var wellKnownEndpoint in GetWellKnownAuthorizationServerMetadataUris(authServerUri)) { + AuthorizationServerMetadata? metadata; try { var response = await _httpClient.GetAsync(wellKnownEndpoint, cancellationToken).ConfigureAwait(false); @@ -377,7 +378,7 @@ private async Task GetAuthServerMetadataAsync(Uri a } using var stream = await response.Content.ReadAsStreamAsync(cancellationToken).ConfigureAwait(false); - var metadata = await JsonSerializer.DeserializeAsync(stream, McpJsonUtilities.JsonContext.Default.AuthorizationServerMetadata, cancellationToken).ConfigureAwait(false); + metadata = await JsonSerializer.DeserializeAsync(stream, McpJsonUtilities.JsonContext.Default.AuthorizationServerMetadata, cancellationToken).ConfigureAwait(false); if (metadata is null) { @@ -394,18 +395,46 @@ private async Task GetAuthServerMetadataAsync(Uri a { ThrowFailedToHandleUnauthorizedResponse($"AuthorizationEndpoint must use HTTP or HTTPS. '{metadata.AuthorizationEndpoint}' does not meet this requirement."); } + } + catch (Exception ex) + { + LogErrorFetchingAuthServerMetadata(ex, wellKnownEndpoint); + continue; + } - metadata.ResponseTypesSupported ??= ["code"]; - metadata.GrantTypesSupported ??= ["authorization_code", "refresh_token"]; - metadata.TokenEndpointAuthMethodsSupported ??= ["client_secret_post"]; - metadata.CodeChallengeMethodsSupported ??= ["S256"]; + try + { + // Per the MCP spec, clients MUST verify PKCE support via authorization server metadata and refuse + // to proceed without it. Rather than failing on the first document that lacks it, skip to the next + // discovery endpoint: different well-known endpoints (OAuth 2.0 vs OpenID Connect) can return + // different documents for the same server, and OpenID Connect metadata commonly includes the field. + var codeChallengeMethods = metadata.CodeChallengeMethodsSupported; + if (codeChallengeMethods is null) + { + ThrowFailedToHandleUnauthorizedResponse( + $"Authorization server metadata from '{wellKnownEndpoint}' does not include 'code_challenge_methods_supported'. MCP clients require PKCE support and must refuse to proceed."); + } - return metadata; + if (!codeChallengeMethods.Contains("S256")) + { + var advertisedMethods = codeChallengeMethods.Count > 0 + ? string.Join(", ", codeChallengeMethods) + : ""; + ThrowFailedToHandleUnauthorizedResponse( + $"Authorization server metadata from '{wellKnownEndpoint}' does not advertise required PKCE method 'S256' in 'code_challenge_methods_supported'. Advertised methods: {advertisedMethods}."); + } } catch (Exception ex) { - LogErrorFetchingAuthServerMetadata(ex, wellKnownEndpoint); + LogAuthServerMetadataMissingPkceSupport(ex, wellKnownEndpoint); + continue; } + + metadata.ResponseTypesSupported ??= ["code"]; + metadata.GrantTypesSupported ??= ["authorization_code", "refresh_token"]; + metadata.TokenEndpointAuthMethodsSupported ??= ["client_secret_post"]; + + return metadata; } if (resourceUri is null) @@ -1155,6 +1184,9 @@ private static void ThrowFailedToHandleUnauthorizedResponse(string message) => [LoggerMessage(Level = LogLevel.Error, Message = "Error fetching auth server metadata from {Endpoint}")] partial void LogErrorFetchingAuthServerMetadata(Exception ex, Uri endpoint); + [LoggerMessage(Level = LogLevel.Warning, Message = "Authorization server metadata from {Endpoint} does not satisfy PKCE requirements; skipping it and trying the next discovery endpoint.")] + partial void LogAuthServerMetadataMissingPkceSupport(Exception ex, Uri endpoint); + [LoggerMessage(Level = LogLevel.Information, Message = "Performing dynamic client registration with {RegistrationEndpoint}")] partial void LogPerformingDynamicClientRegistration(Uri registrationEndpoint); diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index e49cb5bf5..f1920aa10 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -131,6 +131,85 @@ public async Task CanAuthenticate_WithClientMetadataDocument() transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); } + [Fact] + public async Task CannotAuthenticate_WhenMetadataOmitsPkceMethods() + { + TestOAuthServer.CodeChallengeMethodsSupported = null; + await using var app = await StartMcpServerAsync(); + + 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"), + AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + }, + }, HttpClient, LoggerFactory); + + await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + // No discovery endpoint advertises PKCE, so metadata discovery is exhausted. The precise PKCE reason + // is logged as each endpoint is skipped. + Assert.Contains( + MockLoggerProvider.LogMessages, + m => m.Exception?.Message.Contains("code_challenge_methods_supported") == true); + } + + [Fact] + public async Task CannotAuthenticate_WhenMetadataLacksS256PkceMethod() + { + TestOAuthServer.CodeChallengeMethodsSupported = ["plain"]; + await using var app = await StartMcpServerAsync(); + + 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"), + AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + }, + }, HttpClient, LoggerFactory); + + await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains( + MockLoggerProvider.LogMessages, + m => m.Exception?.Message.Contains("required PKCE method 'S256'") == true); + } + + [Fact] + public async Task CanAuthenticate_WhenFirstMetadataEndpointOmitsPkce_ButAnotherAdvertisesIt() + { + // The OAuth 2.0 authorization server metadata endpoint is tried before the OpenID Connect one. + // Simulate a server where only the OpenID Connect document advertises PKCE support, and verify the + // client falls through to it rather than failing on the first PKCE-less document. + TestOAuthServer.MetadataPathsWithoutPkceSupport.Add("/.well-known/oauth-authorization-server"); + await using var app = await StartMcpServerAsync(); + + 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"), + AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + }, + }, HttpClient, LoggerFactory); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + } + [Fact] public async Task UsesDynamicClientRegistration_WhenCimdNotSupported() { diff --git a/tests/ModelContextProtocol.TestOAuthServer/Program.cs b/tests/ModelContextProtocol.TestOAuthServer/Program.cs index 69eb60683..f9c40d9dd 100644 --- a/tests/ModelContextProtocol.TestOAuthServer/Program.cs +++ b/tests/ModelContextProtocol.TestOAuthServer/Program.cs @@ -100,6 +100,20 @@ public Program(ILoggerProvider? loggerProvider = null, IConnectionListenerFactor /// public bool IncludeOfflineAccessInMetadata { get; set; } + /// + /// Gets or sets the code challenge methods advertised by metadata endpoints. + /// + /// + /// The default value is ["S256"]. + /// + public List? CodeChallengeMethodsSupported { get; set; } = ["S256"]; + + /// + /// Gets the set of metadata paths that should omit code_challenge_methods_supported from their + /// response, simulating a server whose discovery endpoints advertise differing PKCE support. + /// + public HashSet MetadataPathsWithoutPkceSupport { get; } = new(StringComparer.OrdinalIgnoreCase); + public HashSet DisabledMetadataPaths { get; } = new(StringComparer.OrdinalIgnoreCase); public IReadOnlyCollection MetadataRequests => _metadataRequests.ToArray(); @@ -237,7 +251,9 @@ IResult HandleMetadataRequest(HttpContext context, string? issuerPath = null) : ["openid", "profile", "email", "mcp:tools"], TokenEndpointAuthMethodsSupported = ["client_secret_post"], ClaimsSupported = ["sub", "iss", "name", "email", "aud"], - CodeChallengeMethodsSupported = ["S256"], + CodeChallengeMethodsSupported = MetadataPathsWithoutPkceSupport.Contains(context.Request.Path) + ? null + : CodeChallengeMethodsSupported, GrantTypesSupported = ["authorization_code", "refresh_token"], IntrospectionEndpoint = $"{_url}/introspect", RegistrationEndpoint = $"{_url}/register", From 65445ef892eaee49ba9686bd2e4755d8d869d713 Mon Sep 17 00:00:00 2001 From: Pranav Senthilnathan Date: Thu, 16 Jul 2026 12:31:22 -0700 Subject: [PATCH 2/2] Limit legacy auth metadata fallback to when no document is found Previously, when PRM was unavailable, a discovered auth server metadata document that lacked 'code_challenge_methods_supported' would be skipped and the client would synthesize default metadata advertising S256, letting non-compliant servers proceed. Now the legacy default fallback only applies when no metadata document is discovered; a document that fails PKCE validation surfaces the specific failure instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57bac4ff-fb62-4b33-88c3-342ce65e9d0a --- .../Authentication/ClientOAuthProvider.cs | 22 +++++- .../OAuth/AuthTests.cs | 75 +++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index c10b2f73f..b49bb51c9 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -366,6 +366,12 @@ static bool IsValidClientMetadataDocumentUri(Uri uri) private async Task GetAuthServerMetadataAsync(Uri authServerUri, string? resourceUri, CancellationToken cancellationToken) { + // Tracks whether at least one well-known endpoint returned a structurally valid metadata document. + // This distinguishes "no metadata document was found" (eligible for the 2025-03-26 legacy fallback) + // from "a metadata document exists but failed PKCE validation" (must not fall back to synthesized defaults). + var metadataDocumentFound = false; + McpException? pkceValidationFailure = null; + foreach (var wellKnownEndpoint in GetWellKnownAuthorizationServerMetadataUris(authServerUri)) { AuthorizationServerMetadata? metadata; @@ -395,6 +401,10 @@ private async Task GetAuthServerMetadataAsync(Uri a { ThrowFailedToHandleUnauthorizedResponse($"AuthorizationEndpoint must use HTTP or HTTPS. '{metadata.AuthorizationEndpoint}' does not meet this requirement."); } + + // A structurally valid metadata document was discovered. Even if it fails PKCE validation + // below, its existence disqualifies the legacy fallback that would otherwise synthesize S256. + metadataDocumentFound = true; } catch (Exception ex) { @@ -427,6 +437,7 @@ private async Task GetAuthServerMetadataAsync(Uri a catch (Exception ex) { LogAuthServerMetadataMissingPkceSupport(ex, wellKnownEndpoint); + pkceValidationFailure = ex as McpException ?? new McpException(ex.Message, ex); continue; } @@ -437,10 +448,17 @@ private async Task GetAuthServerMetadataAsync(Uri a return metadata; } + // A discovered metadata document that failed PKCE validation must not be replaced with synthesized + // defaults. Surface the specific PKCE failure regardless of whether PRM was available. + if (metadataDocumentFound && pkceValidationFailure is not null) + { + throw pkceValidationFailure; + } + if (resourceUri is null) { - // 2025-03-26 backcompat: when PRM is unavailable and auth server metadata discovery - // also fails, fall back to default endpoint paths per the 2025-03-26 spec. + // 2025-03-26 backcompat: when PRM is unavailable and no auth server metadata document was + // discovered, fall back to default endpoint paths per the 2025-03-26 spec. return BuildDefaultAuthServerMetadata(authServerUri); } diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index f1920aa10..18e1a8e34 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -1750,6 +1750,81 @@ public async Task CanAuthenticate_WithLegacyServerUsingDefaultEndpointFallback() transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); } + [Fact] + public async Task CannotAuthenticate_WithLegacyServerWhoseMetadataOmitsPkceMethods() + { + // 2025-03-26 backcompat regression guard: PRM is unavailable (resourceUri is null), but the server + // DOES serve an auth server metadata document that omits 'code_challenge_methods_supported'. + // The client must refuse to proceed rather than falling back to synthesized S256 defaults, since a + // discovered metadata document that fails PKCE validation disqualifies the legacy default fallback. + TestOAuthServer.ExpectResource = false; + + // Use JwtBearer as the challenge scheme so the 401 response does NOT include resource_metadata. + Builder.Services.Configure(options => options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme); + + Builder.Services.Configure(JwtBearerDefaults.AuthenticationScheme, options => + { + options.TokenValidationParameters.ValidateAudience = false; + }); + + await using var app = Builder.Build(); + + app.Use(async (context, next) => + { + // Return 404 for PRM to simulate a legacy server that doesn't support RFC 9728. + if (context.Request.Path.StartsWithSegments("/.well-known/oauth-protected-resource")) + { + context.Response.StatusCode = StatusCodes.Status404NotFound; + return; + } + + // Serve auth server metadata that omits 'code_challenge_methods_supported' entirely. + if (context.Request.Path.StartsWithSegments("/.well-known/oauth-authorization-server") || + context.Request.Path.StartsWithSegments("/.well-known/openid-configuration")) + { + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync($$""" + { + "issuer": "{{OAuthServerUrl}}", + "authorization_endpoint": "{{McpServerUrl}}/authorize", + "token_endpoint": "{{McpServerUrl}}/token", + "registration_endpoint": "{{McpServerUrl}}/register", + "response_types_supported": ["code"], + "grant_types_supported": ["authorization_code", "refresh_token"], + "token_endpoint_auth_methods_supported": ["client_secret_post"] + } + """); + return; + } + + await next(); + }); + + app.UseAuthentication(); + app.UseAuthorization(); + app.MapMcp().RequireAuthorization(); + await app.StartAsync(TestContext.Current.CancellationToken); + + 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"), + AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + }, + }, HttpClient, LoggerFactory); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + // The specific PKCE failure reason is surfaced rather than a generic discovery failure or a + // silently-synthesized S256 fallback. + Assert.Contains("code_challenge_methods_supported", ex.Message); + } + [Fact] public async Task AuthorizationFlow_AppendsOfflineAccess_WhenServerAdvertisesIt() {