Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion samples/ProtectedMcpClient/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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"];

Expand All @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
namespace ModelContextProtocol.Authentication;

/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <para>
/// The <see cref="State"/> property must be populated from the <c>state</c> 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.
/// </para>
/// <para>
/// The <see cref="Iss"/> property should be populated from the <c>iss</c> query parameter in the
/// redirect URI when present, as specified by
/// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>.
Expand All @@ -20,6 +25,16 @@ public sealed class AuthorizationResult
/// </summary>
public string? Code { get; init; }

/// <summary>
/// Gets the state value returned in the authorization response.
/// </summary>
/// <remarks>
/// Implementations of <see cref="ClientOAuthOptions.AuthorizationCallbackHandler"/> must populate this
/// property from the <c>state</c> 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.
/// </remarks>
public string? State { get; init; }

/// <summary>
/// Gets the issuer identifier returned in the authorization response per
/// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>.
Expand Down
18 changes: 11 additions & 7 deletions src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,10 @@ public sealed class ClientOAuthOptions
/// </para>
/// <para>
/// Custom implementations might open a browser, start an HTTP listener, or use other mechanisms to capture
/// the authorization response. They should return both the <c>code</c> and <c>iss</c> query parameters
/// from the redirect URI callback. This enables the SDK to validate the <c>iss</c> parameter per
/// the authorization response. They must return the <c>code</c> and <c>state</c> query parameters,
/// and should return the <c>iss</c> query parameter when present, from the redirect URI callback.
/// The SDK requires an exact state match before exchanging the code. Returning <c>iss</c> enables
/// the SDK to validate the parameter per
/// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>, which mitigates
/// mix-up attacks.
/// </para>
Expand All @@ -96,9 +98,10 @@ public sealed class ClientOAuthOptions
/// </summary>
/// <remarks>
/// <para>
/// This delegate returns only the authorization code and cannot provide the <c>iss</c> parameter from
/// the authorization response. Consequently, RFC 9207 issuer validation is skipped when this delegate
/// is used. Use <see cref="AuthorizationCallbackHandler"/> for issuer-aware authorization flows.
/// This delegate returns only the authorization code and cannot provide the <c>state</c> or <c>iss</c>
/// parameter from the authorization response. Consequently, state and RFC 9207 issuer validation are
Comment thread
halter73 marked this conversation as resolved.
/// skipped when this delegate is used. Use <see cref="AuthorizationCallbackHandler"/> for response-bound,
/// issuer-aware authorization flows.
/// </para>
/// <para>
/// This property cannot be configured together with <see cref="AuthorizationCallbackHandler"/>.
Expand Down Expand Up @@ -139,8 +142,9 @@ public sealed class ClientOAuthOptions
/// </summary>
/// <remarks>
/// <para>
/// Parameters specified cannot override or append to any automatically set parameters like the "redirect_uri",
/// which should instead be configured via <see cref="RedirectUri"/>.
/// Parameters specified cannot override or append to any automatically set parameters like
/// <c>redirect_uri</c> or <c>state</c>. The redirect URI should instead be configured via
/// <see cref="RedirectUri"/>, while state is generated uniquely for each authorization transaction.
/// </para>
/// </remarks>
public IDictionary<string, string> AdditionalAuthorizationParameters { get; set; } = new Dictionary<string, string>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ internal sealed partial class ClientOAuthProvider : McpHttpClient
private readonly IDictionary<string, string> _additionalAuthorizationParameters;
private readonly Func<IReadOnlyList<Uri>, Uri?> _authServerSelector;
private readonly Func<AuthorizationCallbackContext, CancellationToken, Task<AuthorizationResult?>> _authorizationCallbackHandler;
private readonly bool _validateAuthorizationResponseState;
private readonly bool _validateAuthorizationResponseIssuer;
private readonly Uri? _clientMetadataDocumentUri;

Expand Down Expand Up @@ -120,6 +121,7 @@ public ClientOAuthProvider(
if (options.AuthorizationCallbackHandler is not null)
{
_authorizationCallbackHandler = options.AuthorizationCallbackHandler;
_validateAuthorizationResponseState = true;
Comment thread
halter73 marked this conversation as resolved.
_validateAuthorizationResponseIssuer = true;
}
else if (authorizationRedirectDelegate is not null)
Expand All @@ -131,11 +133,13 @@ public ClientOAuthProvider(
context.RedirectUri,
cancellationToken).ConfigureAwait(false),
};
_validateAuthorizationResponseState = false;
_validateAuthorizationResponseIssuer = false;
}
else
{
_authorizationCallbackHandler = DefaultAuthorizationUrlHandler;
_validateAuthorizationResponseState = true;
_validateAuthorizationResponseIssuer = true;
}

Expand All @@ -155,11 +159,11 @@ public ClientOAuthProvider(
private static Uri? DefaultAuthServerSelector(IReadOnlyList<Uri> availableServers) => availableServers.FirstOrDefault();

/// <summary>
/// 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.
/// </summary>
/// <param name="context">The context containing the authorization and redirect URIs.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for cancellation requests.</param>
/// <returns>The authorization result entered by the user, or null if none was provided.</returns>
/// <returns>The authorization result parsed from the redirect URL, or null if no valid URL was provided.</returns>
Comment thread
Copilot marked this conversation as resolved.
Outdated
private static Task<AuthorizationResult?> DefaultAuthorizationUrlHandler(
AuthorizationCallbackContext context,
CancellationToken cancellationToken)
Expand All @@ -179,6 +183,7 @@ public ClientOAuthProvider(
return Task.FromResult<AuthorizationResult?>(new()
{
Code = queryParams["code"],
State = queryParams["state"],
Iss = queryParams["iss"],
});
}
Expand Down Expand Up @@ -687,10 +692,11 @@ private async Task<string> 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
Expand All @@ -700,7 +706,17 @@ private async Task<string> 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.");
}
Comment thread
halter73 marked this conversation as resolved.

if (_validateAuthorizationResponseState)
{
ValidateStateResponse(authResult!.State, state);
}

if (string.IsNullOrEmpty(authResult.Code))
{
ThrowFailedToHandleUnauthorizedResponse($"The {nameof(ClientOAuthOptions.AuthorizationCallbackHandler)} returned a null or empty authorization code.");
}
Expand All @@ -721,7 +737,8 @@ private async Task<string> InitiateAuthorizationCodeFlowAsync(
private Uri BuildAuthorizationUrl(
ProtectedResourceMetadata protectedResourceMetadata,
AuthorizationServerMetadata authServerMetadata,
string codeChallenge)
string codeChallenge,
string state)
{
var resourceUri = GetResourceUri(protectedResourceMetadata);

Expand All @@ -732,6 +749,7 @@ private Uri BuildAuthorizationUrl(
["response_type"] = "code",
["code_challenge"] = codeChallenge,
["code_challenge_method"] = "S256",
["state"] = state,
};

if (resourceUri is not null)
Expand Down Expand Up @@ -1107,6 +1125,26 @@ private bool ChallengeIntroducesNewScopes(ProtectedResourceMetadata protectedRes
return scope + " " + OfflineAccess;
}

/// <summary>
/// Validates that an authorization response is bound to the transaction that initiated it.
/// </summary>
/// <param name="state">The state returned in the authorization response.</param>
/// <param name="expectedState">The state sent in the authorization request.</param>
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.");
}
}

/// <summary>
/// Validates the <c>iss</c> parameter from an authorization response per
/// <see href="https://datatracker.ietf.org/doc/html/rfc9207">RFC 9207</see>.
Expand Down Expand Up @@ -1378,7 +1416,7 @@ private async Task<ProtectedResourceMetadata> 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<byte> bytes = stackalloc byte[32];
Expand Down
85 changes: 80 additions & 5 deletions tests/ModelContextProtocol.AspNetCore.Tests/OAuth/AuthTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ModelContextProtocol.Authentication.AuthorizationResult?>(new() { Code = "unused-code" }));

var ex = await Assert.ThrowsAsync<McpException>(() => 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<ModelContextProtocol.Authentication.AuthorizationResult?>(
new() { Code = "unused-code", State = "unexpected-state" }));

var ex = await Assert.ThrowsAsync<McpException>(() => 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<string> 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()
{
Expand Down Expand Up @@ -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();

Expand All @@ -566,7 +639,7 @@ public async Task CannotOverrideExistingParameters_WithExtraParams()
AuthorizationCallbackHandler = HandleAuthorizationUrlAsync,
AdditionalAuthorizationParameters = new Dictionary<string, string>
{
["redirect_uri"] = "custom_value",
[parameterName] = "custom_value",
}
},
}, HttpClient, LoggerFactory);
Expand Down Expand Up @@ -2434,7 +2507,9 @@ public async Task AuthorizationFlow_ScopeSelector_ReturningEmpty_OmitsScopeParam
Assert.False(scopePresent);
}

private HttpClientTransport CreateOAuthTransport() =>
private HttpClientTransport CreateOAuthTransport(
Func<AuthorizationCallbackContext, CancellationToken, Task<ModelContextProtocol.Authentication.AuthorizationResult?>>?
authorizationCallbackHandler = null) =>
new(new()
{
Endpoint = new(McpServerUrl),
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ protected async Task<WebApplication> 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,
};
}
Expand Down
9 changes: 7 additions & 2 deletions tests/ModelContextProtocol.ConformanceClient/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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('&'))
{
Expand All @@ -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]);
Expand All @@ -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 };
}
}

Expand Down
Loading
Loading