diff --git a/samples/ProtectedMcpClient/Program.cs b/samples/ProtectedMcpClient/Program.cs index eba792556..79eb45988 100644 --- a/samples/ProtectedMcpClient/Program.cs +++ b/samples/ProtectedMcpClient/Program.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.Logging; +using ModelContextProtocol.Authentication; using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; using System.Diagnostics; @@ -32,7 +33,7 @@ OAuth = new() { RedirectUri = new Uri("http://localhost:1179/callback"), - AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + AuthorizationCallbackHandler = HandleAuthorizationUrlAsync, DynamicClientRegistration = new() { ClientName = "ProtectedMcpClient", @@ -65,15 +66,19 @@ Console.WriteLine(); } +/// /// Handles the OAuth authorization URL by starting a local HTTP server and opening a browser. /// This implementation demonstrates how SDK consumers can provide their own authorization flow. /// -/// The authorization URL to open in the browser. -/// The redirect URI where the authorization code will be sent. +/// The authorization and redirect URIs for this request. /// The to monitor for cancellation requests. The default is . -/// The authorization code extracted from the callback, or null if the operation failed. -static async Task HandleAuthorizationUrlAsync(Uri authorizationUrl, Uri redirectUri, CancellationToken cancellationToken) +/// The authorization code and state extracted from the callback, or null if the operation failed. +static async Task HandleAuthorizationUrlAsync( + AuthorizationCallbackContext authorizationContext, + CancellationToken cancellationToken) { + var authorizationUrl = authorizationContext.AuthorizationUri; + var redirectUri = authorizationContext.RedirectUri; Console.WriteLine("Starting OAuth authorization flow..."); Console.WriteLine($"Opening browser to: {authorizationUrl}"); @@ -93,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 error = query["error"]; string responseHtml = "

Authentication complete

You can close this window now.

"; @@ -115,7 +121,7 @@ } Console.WriteLine("Authorization code received successfully."); - return code; + return new AuthorizationResult { Code = code, State = state }; } catch (Exception ex) { diff --git a/samples/ProtectedMcpClient/README.md b/samples/ProtectedMcpClient/README.md index 81ae67cee..413df05d6 100644 --- a/samples/ProtectedMcpClient/README.md +++ b/samples/ProtectedMcpClient/README.md @@ -56,12 +56,14 @@ dotnet run 3. The client initiates OAuth 2.0 authorization code flow: - Opens a browser to the authorization URL at the OAuth server - Starts a local HTTP listener on `http://localhost:1179/callback` to receive the authorization code - - Exchanges the authorization code for an access token + - Validates that the returned `state` matches the authorization request before exchanging the code for an access token 4. The client uses the access token to authenticate with the MCP server 5. The client lists available tools and calls the `GetAlerts` tool for Washington state ## OAuth Configuration +The sample uses `AuthorizationCallbackHandler` to return both the `code` and `state` parameters from the redirect to the SDK. State is generated uniquely for each authorization transaction and must match exactly before the SDK exchanges the code. The legacy `AuthorizationRedirectDelegate` remains supported, but returns only a code and does not enable SDK state validation. For all callback paths, `state` is SDK-generated and cannot be overridden through `AdditionalAuthorizationParameters`. When neither callback is configured, the built-in handler prompts for the full redirect URL rather than just the code. + The client is configured with: - **Client ID**: `demo-client` - **Client Secret**: `demo-secret` diff --git a/src/ModelContextProtocol.Core/Authentication/AuthorizationCallbackContext.cs b/src/ModelContextProtocol.Core/Authentication/AuthorizationCallbackContext.cs new file mode 100644 index 000000000..cbc453802 --- /dev/null +++ b/src/ModelContextProtocol.Core/Authentication/AuthorizationCallbackContext.cs @@ -0,0 +1,17 @@ +namespace ModelContextProtocol.Authentication; + +/// +/// Provides the information needed to complete an OAuth authorization request. +/// +public sealed class AuthorizationCallbackContext +{ + /// + /// Gets the authorization URI that the user needs to visit. + /// + public required Uri AuthorizationUri { get; init; } + + /// + /// Gets the redirect URI where the authorization response will be sent. + /// + public required Uri RedirectUri { get; init; } +} diff --git a/src/ModelContextProtocol.Core/Authentication/AuthorizationRedirectDelegate.cs b/src/ModelContextProtocol.Core/Authentication/AuthorizationRedirectDelegate.cs index a811e51cc..4df6eb3af 100644 --- a/src/ModelContextProtocol.Core/Authentication/AuthorizationRedirectDelegate.cs +++ b/src/ModelContextProtocol.Core/Authentication/AuthorizationRedirectDelegate.cs @@ -24,5 +24,10 @@ namespace ModelContextProtocol.Authentication; /// the authorization code from the callback. The authorization code is typically provided as /// a query parameter in the redirect URI callback. /// +/// +/// This delegate returns only the authorization code, so the SDK cannot validate the response's +/// state parameter. Use to return +/// both the code and state and enable SDK state validation. +/// /// public delegate Task AuthorizationRedirectDelegate(Uri authorizationUri, Uri redirectUri, CancellationToken cancellationToken); \ No newline at end of file diff --git a/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs b/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs new file mode 100644 index 000000000..76bad45dc --- /dev/null +++ b/src/ModelContextProtocol.Core/Authentication/AuthorizationResult.cs @@ -0,0 +1,28 @@ +namespace ModelContextProtocol.Authentication; + +/// +/// Represents the result of an OAuth authorization redirect, containing the authorization code +/// and state 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. +/// +public sealed class AuthorizationResult +{ + /// + /// Gets the authorization code returned by the authorization server. + /// + 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; } +} diff --git a/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs b/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs index 483e3643e..f2a1ed7ea 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs @@ -49,16 +49,37 @@ public sealed class ClientOAuthOptions public IEnumerable? Scopes { get; set; } /// - /// Gets or sets the authorization redirect delegate for handling the OAuth authorization flow. + /// Gets or sets the callback that handles the OAuth authorization flow. /// /// /// - /// This delegate is responsible for handling the OAuth authorization URL and obtaining the authorization code. - /// If not specified, a default implementation will be used that prompts the user to enter the code manually. + /// This callback receives the authorization and redirect URIs in an + /// and returns the authorization response. + /// If neither this callback nor is specified, a default + /// implementation prompts the user to enter the full redirect URL manually. /// /// /// Custom implementations might open a browser, start an HTTP listener, or use other mechanisms to capture - /// the authorization code from the OAuth redirect. + /// the authorization response. They must return the code and state query parameters + /// from the redirect URI callback. The SDK requires an exact state match before exchanging the code. + /// + /// + /// This property cannot be configured together with . + /// + /// + public Func>? AuthorizationCallbackHandler { get; set; } + + /// + /// Gets or sets the legacy authorization redirect delegate for handling the OAuth authorization flow. + /// + /// + /// + /// This delegate returns only the authorization code and cannot provide the state parameter + /// from the authorization response. Consequently, SDK state validation is skipped when this delegate + /// is used. Use to enable SDK state validation. + /// + /// + /// This property cannot be configured together with . /// /// public AuthorizationRedirectDelegate? AuthorizationRedirectDelegate { get; set; } @@ -92,8 +113,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 ecef8e15e..0e4b91178 100644 --- a/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs +++ b/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs @@ -30,7 +30,8 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient private readonly string? _configuredScopes; private readonly IDictionary _additionalAuthorizationParameters; private readonly Func, Uri?> _authServerSelector; - private readonly AuthorizationRedirectDelegate _authorizationRedirectDelegate; + private readonly Func> _authorizationCallbackHandler; + private readonly bool _validateAuthorizationResponseState; private readonly Uri? _clientMetadataDocumentUri; // _dcrClientName, _dcrClientUri, _dcrInitialAccessToken and _dcrResponseDelegate are used for dynamic client registration (RFC 7591) @@ -82,8 +83,36 @@ public ClientOAuthProvider( // Set up authorization server selection strategy _authServerSelector = options.AuthServerSelector ?? DefaultAuthServerSelector; - // Set up authorization URL handler (use default if not provided) - _authorizationRedirectDelegate = options.AuthorizationRedirectDelegate ?? DefaultAuthorizationUrlHandler; + var authorizationRedirectDelegate = options.AuthorizationRedirectDelegate; + if (options.AuthorizationCallbackHandler is not null && authorizationRedirectDelegate is not null) + { + throw new ArgumentException( + $"{nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} and {nameof(ClientOAuthOptions.AuthorizationRedirectDelegate)} cannot both be configured.", + nameof(options)); + } + + if (options.AuthorizationCallbackHandler is not null) + { + _authorizationCallbackHandler = options.AuthorizationCallbackHandler; + _validateAuthorizationResponseState = true; + } + else if (authorizationRedirectDelegate is not null) + { + _authorizationCallbackHandler = async (context, cancellationToken) => new AuthorizationResult + { + Code = await authorizationRedirectDelegate( + context.AuthorizationUri, + context.RedirectUri, + cancellationToken).ConfigureAwait(false), + }; + // Code-only callbacks cannot provide response state. Preserve their existing behavior. + _validateAuthorizationResponseState = false; + } + else + { + _authorizationCallbackHandler = DefaultAuthorizationUrlHandler; + _validateAuthorizationResponseState = true; + } _dcrClientName = options.DynamicClientRegistration?.ClientName; _dcrClientUri = options.DynamicClientRegistration?.ClientUri; @@ -100,20 +129,32 @@ 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 authorization URL to handle. - /// The redirect URI where the authorization code will be sent. + /// The context containing the authorization and redirect URIs. /// The to monitor for cancellation requests. - /// The authorization code entered by the user, or null if none was provided. - private static Task DefaultAuthorizationUrlHandler(Uri authorizationUrl, Uri redirectUri, CancellationToken cancellationToken) + /// The authorization result parsed from the redirect URL. + private static Task DefaultAuthorizationUrlHandler( + AuthorizationCallbackContext context, + CancellationToken cancellationToken) { Console.WriteLine($"Please open the following URL in your browser to authorize the application:"); - Console.WriteLine($"{authorizationUrl}"); + Console.WriteLine($"{context.AuthorizationUri}"); Console.WriteLine(); - Console.Write("Enter the authorization code from the redirect URL: "); - var authorizationCode = Console.ReadLine(); - return Task.FromResult(authorizationCode); + Console.Write("Enter the full redirect URL: "); + var redirectUrl = Console.ReadLine(); + if (!Uri.TryCreate(redirectUrl, UriKind.Absolute, out var responseUri)) + { + ThrowFailedToHandleUnauthorizedResponse( + "The entered redirect URL is not a valid absolute URL. Paste the full redirect URL from the browser address bar."); + } + + var queryParams = HttpUtility.ParseQueryString(responseUri.Query); + return Task.FromResult(new() + { + Code = queryParams["code"], + State = queryParams["state"], + }); } internal override async Task SendAsync(HttpRequestMessage request, JsonRpcMessage? message, CancellationToken cancellationToken) @@ -456,24 +497,42 @@ 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 authCode = await _authorizationRedirectDelegate(authUrl, _redirectUri, cancellationToken).ConfigureAwait(false); + var authUrl = BuildAuthorizationUrl(protectedResourceMetadata, authServerMetadata, codeChallenge, state); + var authResult = await _authorizationCallbackHandler( + new AuthorizationCallbackContext + { + AuthorizationUri = authUrl, + RedirectUri = _redirectUri, + }, + cancellationToken).ConfigureAwait(false); - if (string.IsNullOrEmpty(authCode)) + if (authResult is null) { - ThrowFailedToHandleUnauthorizedResponse($"The {nameof(AuthorizationRedirectDelegate)} returned a null or empty authorization code."); + ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null authorization result."); } - return await ExchangeCodeForTokenAsync(protectedResourceMetadata, authServerMetadata, authCode!, codeVerifier, cancellationToken).ConfigureAwait(false); + if (_validateAuthorizationResponseState) + { + ValidateStateResponse(authResult!.State, state); + } + + if (string.IsNullOrEmpty(authResult.Code)) + { + ThrowFailedToHandleUnauthorizedResponse("The authorization callback returned a null or empty authorization code."); + } + + return await ExchangeCodeForTokenAsync(protectedResourceMetadata, authServerMetadata, authResult.Code!, codeVerifier, cancellationToken).ConfigureAwait(false); } private Uri BuildAuthorizationUrl( ProtectedResourceMetadata protectedResourceMetadata, AuthorizationServerMetadata authServerMetadata, - string codeChallenge) + string codeChallenge, + string state) { var resourceUri = GetResourceUri(protectedResourceMetadata); @@ -484,6 +543,7 @@ private Uri BuildAuthorizationUrl( ["response_type"] = "code", ["code_challenge"] = codeChallenge, ["code_challenge_method"] = "S256", + ["state"] = state, }; if (resourceUri is not null) @@ -934,7 +994,22 @@ private async Task ExtractProtectedResourceMetadata(H yield return (new Uri($"{hostBase}{ProtectedResourceMetadataWellKnownPath}"), new Uri(hostBase)); } - private static string GenerateCodeVerifier() + 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."); + } + } + + 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 c4979fb10..3058ac210 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs @@ -28,6 +28,20 @@ public AuthTests(ITestOutputHelper outputHelper) { } + private HttpClientTransport CreateOAuthTransport( + Func> authorizationCallbackHandler) => + new(new() + { + Endpoint = new(McpServerUrl), + OAuth = new() + { + ClientId = "demo-client", + ClientSecret = "demo-secret", + RedirectUri = new Uri("http://localhost:1179/callback"), + AuthorizationCallbackHandler = authorizationCallbackHandler, + }, + }, HttpClient, LoggerFactory); + [Fact] public async Task CanAuthenticate() { @@ -49,6 +63,148 @@ public async Task CanAuthenticate() transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); } + [Fact] + public void HttpClientTransport_RejectsBothAuthorizationCallbacks() + { + var ex = Assert.Throws(() => new HttpClientTransport(new() + { + Endpoint = new(McpServerUrl), + OAuth = new() + { + RedirectUri = new Uri("http://localhost:1179/callback"), + AuthorizationCallbackHandler = (_, _) => Task.FromResult(new()), + AuthorizationRedirectDelegate = (_, _, _) => Task.FromResult("code"), + }, + }, HttpClient, LoggerFactory)); + + Assert.Contains(nameof(ClientOAuthOptions.AuthorizationCallbackHandler), ex.Message); + Assert.Contains(nameof(ClientOAuthOptions.AuthorizationRedirectDelegate), ex.Message); + } + + [Fact] + public async Task CanAuthenticate_WhenAuthorizationResponseStateMatches() + { + await using var app = await StartMcpServerAsync(); + + AuthorizationCallbackContext? callbackContext = null; + string? requestedState = null; + await using var transport = CreateOAuthTransport((context, cancellationToken) => + { + callbackContext = context; + var query = QueryHelpers.ParseQuery(context.AuthorizationUri.Query); + Assert.Single(query["state"]); + requestedState = query["state"].ToString(); + return HandleAuthorizationUrlAsync(context, cancellationToken); + }); + + await using var client = await McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); + + Assert.NotNull(callbackContext); + Assert.Equal(new Uri("http://localhost:1179/callback"), callbackContext.RedirectUri); + Assert.NotNull(requestedState); + Assert.Matches("^[A-Za-z0-9_-]{43}$", requestedState); + Assert.Equal(32, Convert.FromBase64String(requestedState.Replace('-', '+').Replace('_', '/') + "=").Length); + Assert.Equal(1, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Theory] + [InlineData(null, "did not include the required state parameter")] + [InlineData("", "did not include the required state parameter")] + [InlineData("unexpected-state", "state did not match")] + public async Task CannotAuthenticate_WhenAuthorizationResponseStateIsInvalid(string? state, string expectedMessage) + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport( + (_, _) => Task.FromResult( + new() { Code = "unused-code", State = state })); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains(expectedMessage, ex.Message); + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Fact] + public async Task CannotAuthenticate_WhenAuthorizationResponseStateIsNotAnExactMatch() + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport((context, _) => + { + var state = QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["state"].ToString(); + // Alter the first character's case (or replace a non-letter) without changing its length. + var first = state[0]; + var different = char.IsUpper(first) ? char.ToLowerInvariant(first) : + char.IsLower(first) ? char.ToUpperInvariant(first) : 'a'; + return Task.FromResult( + new() { Code = "unused-code", State = different + state.Substring(1) }); + }); + + 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 CannotAuthenticate_WhenAuthorizationCallbackReturnsNull() + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport( + (_, _) => Task.FromResult(null)); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains("returned a null authorization result", ex.Message); + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public async Task CannotAuthenticate_WhenAuthorizationCallbackReturnsEmptyCode(string? code) + { + await using var app = await StartMcpServerAsync(); + await using var transport = CreateOAuthTransport((context, _) => + Task.FromResult(new() + { + Code = code, + State = QueryHelpers.ParseQuery(context.AuthorizationUri.Query)["state"], + })); + + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains("null or empty authorization code", 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()); + Assert.Equal(2, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + [Fact] public async Task CannotAuthenticate_WithoutOAuthConfiguration() { @@ -309,8 +465,14 @@ public async Task CanAuthenticate_WithExtraParams() Assert.Contains("custom_param=custom_value", lastAuthorizationUri?.Query); } - [Fact] - public async Task CannotOverrideExistingParameters_WithExtraParams() + [Theory] + [InlineData("redirect_uri", "default")] + [InlineData("redirect_uri", "callback")] + [InlineData("redirect_uri", "legacy")] + [InlineData("state", "default")] + [InlineData("state", "callback")] + [InlineData("state", "legacy")] + public async Task CannotOverrideExistingParameters_WithExtraParams(string parameterName, string callbackKind) { await using var app = await StartMcpServerAsync(); @@ -322,16 +484,19 @@ public async Task CannotOverrideExistingParameters_WithExtraParams() ClientId = "demo-client", ClientSecret = "demo-secret", RedirectUri = new Uri("http://localhost:1179/callback"), - AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, + AuthorizationCallbackHandler = callbackKind == "callback" ? HandleAuthorizationUrlAsync : null, + AuthorizationRedirectDelegate = callbackKind == "legacy" ? HandleAuthorizationUrlAsync : null, AdditionalAuthorizationParameters = new Dictionary { - ["redirect_uri"] = "custom_value", + [parameterName] = "custom_value", } }, }, HttpClient, LoggerFactory); await Assert.ThrowsAsync(() => McpClient.CreateAsync( transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); } [Fact] diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/DefaultAuthorizationUrlHandlerTests.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/DefaultAuthorizationUrlHandlerTests.cs new file mode 100644 index 000000000..bc57dc949 --- /dev/null +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/DefaultAuthorizationUrlHandlerTests.cs @@ -0,0 +1,47 @@ +using ModelContextProtocol.Client; + +namespace ModelContextProtocol.AspNetCore.Tests.OAuth; + +[CollectionDefinition(nameof(DisableConsoleParallelization), DisableParallelization = true)] +public sealed class DisableConsoleParallelization; + +[Collection(nameof(DisableConsoleParallelization))] +public class DefaultAuthorizationUrlHandlerTests(ITestOutputHelper outputHelper) : OAuthTestBase(outputHelper) +{ + [Theory] + [InlineData("authorization-code", "not a valid absolute URL")] + [InlineData("http://localhost:1179/callback?code=unused-code", "did not include the required state parameter")] + [InlineData("http://localhost:1179/callback?code=unused-code&state=unexpected-state", "state did not match")] + public async Task RejectsInvalidAuthorizationResponse(string input, string expectedMessage) + { + 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"), + }, + }, HttpClient, LoggerFactory); + + var originalInput = Console.In; + using var consoleInput = new StringReader(input); + Console.SetIn(consoleInput); + + try + { + var ex = await Assert.ThrowsAsync(() => McpClient.CreateAsync( + transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); + + Assert.Contains(expectedMessage, ex.Message); + Assert.Equal(0, TestOAuthServer.AuthorizationCodeTokenRequestCount); + } + finally + { + Console.SetIn(originalInput); + } + } +} diff --git a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs index 3c1919b0b..a40f49beb 100644 --- a/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs +++ b/tests/ModelContextProtocol.AspNetCore.Tests/OAuth/OAuthTestBase.cs @@ -107,15 +107,26 @@ protected async Task StartMcpServerAsync(string path = "", strin } protected async Task HandleAuthorizationUrlAsync(Uri authorizationUri, Uri redirectUri, CancellationToken cancellationToken) + => (await HandleAuthorizationUrlAsync( + new AuthorizationCallbackContext { AuthorizationUri = authorizationUri, RedirectUri = redirectUri }, + cancellationToken))?.Code; + + protected async Task HandleAuthorizationUrlAsync( + AuthorizationCallbackContext context, + CancellationToken cancellationToken) { - using var redirectResponse = await HttpClient.GetAsync(authorizationUri, cancellationToken); + using var redirectResponse = await HttpClient.GetAsync(context.AuthorizationUri, cancellationToken); Assert.Equal(HttpStatusCode.Redirect, redirectResponse.StatusCode); var location = redirectResponse.Headers.Location; if (location is not null && !string.IsNullOrEmpty(location.Query)) { var queryParams = QueryHelpers.ParseQuery(location.Query); - return queryParams["code"]; + return new ModelContextProtocol.Authentication.AuthorizationResult + { + Code = queryParams["code"], + State = queryParams["state"], + }; } return null; diff --git a/tests/ModelContextProtocol.ConformanceClient/Program.cs b/tests/ModelContextProtocol.ConformanceClient/Program.cs index b5e048dd0..acb7347e9 100644 --- a/tests/ModelContextProtocol.ConformanceClient/Program.cs +++ b/tests/ModelContextProtocol.ConformanceClient/Program.cs @@ -3,6 +3,7 @@ using System.Text.Json; using System.Web; using Microsoft.Extensions.Logging; +using ModelContextProtocol.Authentication; using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; @@ -81,7 +82,7 @@ RedirectUri = clientRedirectUri, // Configure the metadata document URI for CIMD. ClientMetadataDocumentUri = new Uri("https://conformance-test.local/client-metadata.json"), - AuthorizationRedirectDelegate = (authUrl, redirectUri, ct) => HandleAuthorizationUrlAsync(authUrl, redirectUri, ct), + AuthorizationCallbackHandler = HandleAuthorizationUrlAsync, }; if (preRegisteredClientId is not null) @@ -199,8 +200,11 @@ // Copied from ProtectedMcpClient sample // Simulate a user opening the browser and logging in // Copied from OAuthTestBase -static async Task HandleAuthorizationUrlAsync(Uri authorizationUrl, Uri redirectUri, CancellationToken cancellationToken) +static async Task HandleAuthorizationUrlAsync( + AuthorizationCallbackContext authorizationContext, + CancellationToken cancellationToken) { + var authorizationUrl = authorizationContext.AuthorizationUri; Console.WriteLine("Starting OAuth authorization flow..."); Console.WriteLine($"Simulating opening browser to: {authorizationUrl}"); @@ -214,15 +218,10 @@ if (location is not null && !string.IsNullOrEmpty(location.Query)) { - // Parse query string to extract "code" parameter - var query = location.Query.TrimStart('?'); - foreach (var pair in query.Split('&')) + var query = HttpUtility.ParseQueryString(location.Query); + if (query["code"] is { } code) { - var parts = pair.Split('=', 2); - if (parts.Length == 2 && parts[0] == "code") - { - return HttpUtility.UrlDecode(parts[1]); - } + return new AuthorizationResult { Code = code, State = query["state"] }; } } diff --git a/tests/ModelContextProtocol.TestOAuthServer/Program.cs b/tests/ModelContextProtocol.TestOAuthServer/Program.cs index 1da81c2a7..4ea20fe6c 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; @@ -93,6 +94,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); + /// /// Entry point for the application. /// @@ -401,6 +405,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();