From ec6bd7440608ed9c0c2f28b7c760b9d8bc19eca5 Mon Sep 17 00:00:00 2001 From: DanielC000 <91308079+DanielC000@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:24:36 +0200 Subject: [PATCH 1/2] Robustly parse quoted WWW-Authenticate parameters The naive comma-split in ParseWwwAuthenticateParameters broke on commas or escaped quotes inside a quoted parameter value, and could even throw on a value like "error=\",\"". Replaced it with a small RFC 9110 auth-param/quoted-string tokenizer. Closes #1088. --- .../Authentication/ClientOAuthProvider.cs | 94 +++++++++++++++++-- .../WwwAuthenticateParameterParsingTests.cs | 56 +++++++++++ 2 files changed, 140 insertions(+), 10 deletions(-) create mode 100644 tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs index a20ef9f51..d29dfcf26 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -1368,6 +1368,10 @@ private async Task ExtractProtectedResourceMetadata(H /// The parameter string from the WWW-Authenticate header. /// The name of the parameter to extract. /// The value of the parameter, or null if not found. + /// + /// Follows the auth-param and quoted-string grammar from RFC 9110 section 11.6.1 / section 5.6.4, + /// so commas and escaped quotes (\") inside a quoted value don't get mistaken for parameter boundaries. + /// private static string? ParseWwwAuthenticateParameters(string parameters, string parameterName) { if (parameters.IndexOf(parameterName, StringComparison.OrdinalIgnoreCase) == -1) @@ -1375,33 +1379,103 @@ private async Task ExtractProtectedResourceMetadata(H return null; } - foreach (var part in parameters.Split(',')) + int i = 0; + int length = parameters.Length; + + while (i < length) { - var trimmedPart = part.AsSpan().Trim(); - int equalsIndex = trimmedPart.IndexOf('='); + while (i < length && (parameters[i] == ',' || IsOptionalWhitespace(parameters[i]))) + { + i++; + } - if (equalsIndex <= 0) + if (i >= length) { + break; + } + + int nameStart = i; + while (i < length && parameters[i] != '=' && parameters[i] != ',' && !IsOptionalWhitespace(parameters[i])) + { + i++; + } + + var name = parameters.AsSpan(nameStart, i - nameStart); + + while (i < length && IsOptionalWhitespace(parameters[i])) + { + i++; + } + + if (i >= length || parameters[i] != '=') + { + // Not a well-formed "token = value" auth-param; skip past it to the next one. + while (i < length && parameters[i] != ',') + { + i++; + } continue; } - var key = trimmedPart[..equalsIndex].Trim(); + i++; // Consume '='. - if (key.Equals(parameterName, StringComparison.OrdinalIgnoreCase)) + while (i < length && IsOptionalWhitespace(parameters[i])) { - var value = trimmedPart[(equalsIndex + 1)..].Trim(); - if (value.Length > 0 && value[0] == '"' && value[^1] == '"') + i++; + } + + string value; + if (i < length && parameters[i] == '"') + { + i++; // Consume the opening quote. + var sb = new StringBuilder(); + + while (i < length && parameters[i] != '"') + { + if (parameters[i] == '\\' && i + 1 < length) + { + // quoted-pair: the backslash is dropped and the following octet is literal. + i++; + } + + sb.Append(parameters[i]); + i++; + } + + if (i < length) + { + i++; // Consume the closing quote. + } + + value = sb.ToString(); + } + else + { + int valueStart = i; + while (i < length && parameters[i] != ',') { - value = value[1..^1]; + i++; } - return value.ToString(); + value = parameters.AsSpan(valueStart, i - valueStart).TrimEnd().ToString(); + } + + if (name.Equals(parameterName, StringComparison.OrdinalIgnoreCase)) + { + return value; + } + + while (i < length && parameters[i] != ',') + { + i++; } } return null; } + private static bool IsOptionalWhitespace(char c) => c is ' ' or '\t'; + private static IEnumerable<(Uri WellKnownUri, Uri ExpectedResourceUri)> GetWellKnownResourceMetadataUris(Uri resourceUri) { var builder = new UriBuilder(resourceUri); diff --git a/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs b/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs new file mode 100644 index 000000000..07023c587 --- /dev/null +++ b/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs @@ -0,0 +1,56 @@ +using System.Reflection; + +namespace ModelContextProtocol.Tests.Authentication; + +// RFC 9110 section 11.6.1 / section 5.6.4 quoted-string parsing for WWW-Authenticate auth-params. +// See https://github.com/modelcontextprotocol/csharp-sdk/issues/1088. +// +// ClientOAuthProvider is internal and ParseWwwAuthenticateParameters is private, so it's invoked +// via reflection here rather than through the public API surface (which would require driving a +// full OAuth challenge/metadata-fetch flow just to exercise a pure string-parsing helper). +public class WwwAuthenticateParameterParsingTests +{ + private static readonly MethodInfo ParseMethod = Type + .GetType("ModelContextProtocol.Authentication.ClientOAuthProvider, ModelContextProtocol.Core")! + .GetMethod("ParseWwwAuthenticateParameters", BindingFlags.NonPublic | BindingFlags.Static)!; + + private static string? Parse(string parameters, string parameterName) => + (string?)ParseMethod.Invoke(null, [parameters, parameterName]); + + [Theory] + [InlineData( + "error=\"invalid_token\", error_description=\"The access token expired\"", + "error", "invalid_token")] + [InlineData( + "error=\"invalid_token\", error_description=\"The access token expired\"", + "error_description", "The access token expired")] + [InlineData( + "resource_metadata=\"https://example.com/.well-known/oauth-protected-resource\"", + "resource_metadata", "https://example.com/.well-known/oauth-protected-resource")] + // A comma inside a quoted value must not split the parameter. + [InlineData( + "error_description=\"expired, try again\", resource_metadata=\"https://example.com/x\"", + "resource_metadata", "https://example.com/x")] + [InlineData( + "error_description=\"expired, try again\", resource_metadata=\"https://example.com/x\"", + "error_description", "expired, try again")] + // The exact repro from the issue: a quoted value that is only a comma. + [InlineData("scope=\",\"", "scope", ",")] + // Escaped double quote inside a quoted value. + [InlineData("scope=\"read \\\"write\\\" execute\"", "scope", "read \"write\" execute")] + // Escaped backslash inside a quoted value. + [InlineData("scope=\"a\\\\b\"", "scope", "a\\b")] + // Extra whitespace around '=' and ',' (BWS / OWS). + [InlineData("error = \"invalid_token\" , resource_metadata = \"https://x\"", "resource_metadata", "https://x")] + // Unquoted token value is legal per the auth-param grammar (token / quoted-string). + [InlineData("resource_metadata=https://example.com/x", "resource_metadata", "https://example.com/x")] + // Missing parameter returns null. + [InlineData("error=\"invalid_token\"", "resource_metadata", null)] + public void ParseWwwAuthenticateParameters_HandlesRfc9110QuotedStrings( + string parameters, string parameterName, string? expected) + { + var actual = Parse(parameters, parameterName); + + Assert.Equal(expected, actual); + } +} From f1486cd1850b5199704e3930ffa31a28e0a78d80 Mon Sep 17 00:00:00 2001 From: DanielC000 <91308079+DanielC000@users.noreply.github.com> Date: Tue, 28 Jul 2026 18:44:46 +0200 Subject: [PATCH 2/2] Test WWW-Authenticate quoted-parameter parsing end to end Swap the reflection-based unit test for coverage in AuthTests.cs, in the repo's existing idiom of driving a real 401 challenge and checking what reaches the outgoing authorization request. Covers the scope="," crash from the issue and a value with both a comma and an escaped quote. --- .../OAuth/AuthTests.cs | 108 ++++++++++++++++++ .../WwwAuthenticateParameterParsingTests.cs | 56 --------- 2 files changed, 108 insertions(+), 56 deletions(-) delete mode 100644 tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs index 693c77943..ce8f4a909 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -779,6 +779,114 @@ public async Task AuthorizationFlow_UsesScopeFromChallengeHeader() Assert.Equal(challengeScopes, requestedScope); } + [Fact] + public async Task AuthorizationFlow_UsesScopeFromChallengeHeader_WhenScopeIsAQuotedComma() + { + // Regression test for https://github.com/modelcontextprotocol/csharp-sdk/issues/1088. + // ParseWwwAuthenticateParameters used to split the header on every comma, including ones + // inside a quoted value. When a quoted value was exactly a comma, that produced a bare `"` + // fragment that the old code then tried to unquote, throwing ArgumentOutOfRangeException + // instead of authenticating. + await using var app = Builder.Build(); + app.Use(next => + { + return async context => + { + await next(context); + + if (context.Response.StatusCode != 401) + { + return; + } + + context.Response.Headers.WWWAuthenticate = $"Bearer resource_metadata=\"{McpServerUrl}/.well-known/oauth-protected-resource\", scope=\",\""; + }; + }); + app.UseAuthentication(); + app.UseAuthorization(); + + app.MapMcp().RequireAuthorization(); + await app.StartAsync(TestContext.Current.CancellationToken); + + string? requestedScope = null; + + 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"), + AuthorizationCallbackHandler = (context, ct) => + { + var query = QueryHelpers.ParseQuery(context.AuthorizationUri.Query); + requestedScope = query["scope"].ToString(); + return HandleAuthorizationUrlAsync(context, ct); + }, + }, + }, HttpClient, LoggerFactory); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(",", requestedScope); + } + + [Fact] + public async Task AuthorizationFlow_UsesScopeFromChallengeHeader_WithCommaAndEscapedQuoteInValue() + { + // Regression test for https://github.com/modelcontextprotocol/csharp-sdk/issues/1088: a comma + // or an escaped double-quote inside a quoted auth-param value must not be mistaken for the end + // of the value or a new parameter boundary. + const string expectedScope = "read,write\"admin\""; + + await using var app = Builder.Build(); + app.Use(next => + { + return async context => + { + await next(context); + + if (context.Response.StatusCode != 401) + { + return; + } + + context.Response.Headers.WWWAuthenticate = $"Bearer resource_metadata=\"{McpServerUrl}/.well-known/oauth-protected-resource\", scope=\"read,write\\\"admin\\\"\""; + }; + }); + app.UseAuthentication(); + app.UseAuthorization(); + + app.MapMcp().RequireAuthorization(); + await app.StartAsync(TestContext.Current.CancellationToken); + + string? requestedScope = null; + + 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"), + AuthorizationCallbackHandler = (context, ct) => + { + var query = QueryHelpers.ParseQuery(context.AuthorizationUri.Query); + requestedScope = query["scope"].ToString(); + return HandleAuthorizationUrlAsync(context, ct); + }, + }, + }, HttpClient, LoggerFactory); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.Equal(expectedScope, requestedScope); + } + [Fact] public async Task AuthorizationFlow_UsesScopeFromForbiddenHeader() { diff --git a/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs b/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs deleted file mode 100644 index 07023c587..000000000 --- a/tests/ModelContextProtocol.Tests/Authentication/WwwAuthenticateParameterParsingTests.cs +++ /dev/null @@ -1,56 +0,0 @@ -using System.Reflection; - -namespace ModelContextProtocol.Tests.Authentication; - -// RFC 9110 section 11.6.1 / section 5.6.4 quoted-string parsing for WWW-Authenticate auth-params. -// See https://github.com/modelcontextprotocol/csharp-sdk/issues/1088. -// -// ClientOAuthProvider is internal and ParseWwwAuthenticateParameters is private, so it's invoked -// via reflection here rather than through the public API surface (which would require driving a -// full OAuth challenge/metadata-fetch flow just to exercise a pure string-parsing helper). -public class WwwAuthenticateParameterParsingTests -{ - private static readonly MethodInfo ParseMethod = Type - .GetType("ModelContextProtocol.Authentication.ClientOAuthProvider, ModelContextProtocol.Core")! - .GetMethod("ParseWwwAuthenticateParameters", BindingFlags.NonPublic | BindingFlags.Static)!; - - private static string? Parse(string parameters, string parameterName) => - (string?)ParseMethod.Invoke(null, [parameters, parameterName]); - - [Theory] - [InlineData( - "error=\"invalid_token\", error_description=\"The access token expired\"", - "error", "invalid_token")] - [InlineData( - "error=\"invalid_token\", error_description=\"The access token expired\"", - "error_description", "The access token expired")] - [InlineData( - "resource_metadata=\"https://example.com/.well-known/oauth-protected-resource\"", - "resource_metadata", "https://example.com/.well-known/oauth-protected-resource")] - // A comma inside a quoted value must not split the parameter. - [InlineData( - "error_description=\"expired, try again\", resource_metadata=\"https://example.com/x\"", - "resource_metadata", "https://example.com/x")] - [InlineData( - "error_description=\"expired, try again\", resource_metadata=\"https://example.com/x\"", - "error_description", "expired, try again")] - // The exact repro from the issue: a quoted value that is only a comma. - [InlineData("scope=\",\"", "scope", ",")] - // Escaped double quote inside a quoted value. - [InlineData("scope=\"read \\\"write\\\" execute\"", "scope", "read \"write\" execute")] - // Escaped backslash inside a quoted value. - [InlineData("scope=\"a\\\\b\"", "scope", "a\\b")] - // Extra whitespace around '=' and ',' (BWS / OWS). - [InlineData("error = \"invalid_token\" , resource_metadata = \"https://x\"", "resource_metadata", "https://x")] - // Unquoted token value is legal per the auth-param grammar (token / quoted-string). - [InlineData("resource_metadata=https://example.com/x", "resource_metadata", "https://example.com/x")] - // Missing parameter returns null. - [InlineData("error=\"invalid_token\"", "resource_metadata", null)] - public void ParseWwwAuthenticateParameters_HandlesRfc9110QuotedStrings( - string parameters, string parameterName, string? expected) - { - var actual = Parse(parameters, parameterName); - - Assert.Equal(expected, actual); - } -}