Enforce PKCE metadata compliance in OAuth client (#730) (#1700)

src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs

@@ -366,8 +366,15 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
 
     private async Task<AuthorizationServerMetadata> 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;
             try
             {
                 var response = await _httpClient.GetAsync(wellKnownEndpoint, cancellationToken).ConfigureAwait(false);
@@ -377,7 +384,7 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
                 }
 
                 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)
                 {
@@ -395,23 +402,63 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
                     ThrowFailedToHandleUnauthorizedResponse($"AuthorizationEndpoint must use HTTP or HTTPS. '{metadata.AuthorizationEndpoint}' does not meet this requirement.");
                 }
 
-                metadata.ResponseTypesSupported ??= ["code"];
+                // A structurally valid metadata document was discovered. Even if it fails PKCE validation
-                metadata.GrantTypesSupported ??= ["authorization_code", "refresh_token"];
+                // below, its existence disqualifies the legacy fallback that would otherwise synthesize S256.
-                metadata.TokenEndpointAuthMethodsSupported ??= ["client_secret_post"];
+                metadataDocumentFound = true;
-                metadata.CodeChallengeMethodsSupported ??= ["S256"];
-
-                return metadata;
             }
             catch (Exception ex)
             {
                 LogErrorFetchingAuthServerMetadata(ex, wellKnownEndpoint);
+                continue;
             }
+
+            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.");
+                }
+
+                if (!codeChallengeMethods.Contains("S256"))
+                {
+                    var advertisedMethods = codeChallengeMethods.Count > 0
+                        ? string.Join(", ", codeChallengeMethods)
+                        : "<none>";
+                    ThrowFailedToHandleUnauthorizedResponse(
+                        $"Authorization server metadata from '{wellKnownEndpoint}' does not advertise required PKCE method 'S256' in 'code_challenge_methods_supported'. Advertised methods: {advertisedMethods}.");
+                }
+            }
+            catch (Exception ex)
+            {
+                LogAuthServerMetadataMissingPkceSupport(ex, wellKnownEndpoint);
+                pkceValidationFailure = ex as McpException ?? new McpException(ex.Message, ex);
+                continue;
+            }
+
+            metadata.ResponseTypesSupported ??= ["code"];
+            metadata.GrantTypesSupported ??= ["authorization_code", "refresh_token"];
+            metadata.TokenEndpointAuthMethodsSupported ??= ["client_secret_post"];
+
+            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
+            // 2025-03-26 backcompat: when PRM is unavailable and no auth server metadata document was
-            // also fails, fall back to default endpoint paths per the 2025-03-26 spec.
+            // discovered, fall back to default endpoint paths per the 2025-03-26 spec.
             return BuildDefaultAuthServerMetadata(authServerUri);
         }
 
@@ -1155,6 +1202,9 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
     [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);
 

tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs

@@ -131,6 +131,85 @@ public class AuthTests : OAuthTestBase
             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<McpException>(() => 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<McpException>(() => 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()
     {
@@ -1671,6 +1750,81 @@ public class AuthTests : OAuthTestBase
             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<AuthenticationOptions>(options => options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme);
+
+        Builder.Services.Configure<JwtBearerOptions>(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<McpException>(() => 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()
     {

tests/ModelContextProtocol.TestOAuthServer/Program.cs

@@ -100,6 +100,20 @@ public sealed class Program
     /// </remarks>
     public bool IncludeOfflineAccessInMetadata { get; set; }
 
+    /// <summary>
+    /// Gets or sets the code challenge methods advertised by metadata endpoints.
+    /// </summary>
+    /// <remarks>
+    /// The default value is <c>["S256"]</c>.
+    /// </remarks>
+    public List<string>? CodeChallengeMethodsSupported { get; set; } = ["S256"];
+
+    /// <summary>
+    /// Gets the set of metadata paths that should omit <c>code_challenge_methods_supported</c> from their
+    /// response, simulating a server whose discovery endpoints advertise differing PKCE support.
+    /// </summary>
+    public HashSet<string> MetadataPathsWithoutPkceSupport { get; } = new(StringComparer.OrdinalIgnoreCase);
+
     public HashSet<string> DisabledMetadataPaths { get; } = new(StringComparer.OrdinalIgnoreCase);
     public IReadOnlyCollection<string> MetadataRequests => _metadataRequests.ToArray();
 
@@ -237,7 +251,9 @@ public sealed class Program
                     : ["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",