-
Notifications
You must be signed in to change notification settings - Fork 710
Fix: Relax resource URI validation to accept base URL #1517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e475574
7a65e0d
8d1b2b9
2d5ce70
fe712e6
990b776
6d12467
fb001db
12dcccb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -790,14 +790,21 @@ private static bool VerifyResourceMatch(ProtectedResourceMetadata protectedResou | |
| return false; | ||
| } | ||
|
|
||
| // Per RFC: The resource value must be identical to the URL that the client used | ||
| // to make the request to the resource server. Compare entire URIs, not just the host. | ||
|
|
||
| // Normalize the URIs to ensure consistent comparison | ||
| string normalizedMetadataResource = NormalizeUri(protectedResourceMetadata.Resource); | ||
| string normalizedResourceLocation = NormalizeUri(resourceLocation); | ||
|
|
||
| return string.Equals(normalizedMetadataResource, normalizedResourceLocation, StringComparison.OrdinalIgnoreCase); | ||
| // Accept exact match with the full MCP endpoint URI | ||
| if (string.Equals(normalizedMetadataResource, normalizedResourceLocation, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Per MCP spec: "The authorization base URL MUST be derived by discarding the path component from the MCP server URL" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The quoted MCP spec sentence ("The authorization base URL MUST be derived by discarding the path component from the MCP server URL") is about how to derive the authorization server's well-known URL for discovery, not about resource metadata validation. The more relevant justification is the Canonical Server URI section, which explicitly lists both |
||
| // Accept match with the base URL (authority only, path discarded) as this is the expected behavior per MCP spec | ||
|
|
||
| string normalizedBaseUrl = NormalizeUri(new Uri(resourceLocation.GetLeftPart(UriPartial.Authority))); | ||
| return string.Equals(normalizedMetadataResource, normalizedBaseUrl, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -807,8 +807,14 @@ await McpClient.CreateAsync( | |
| Assert.Contains("does not match", ex.Message); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that OAuth authentication succeeds when the protected resource metadata URI | ||
| /// matches the root server URL, even when the actual MCP endpoint is at a subpath. | ||
| /// This tests the flexible URI matching behavior where the resource URI can be less specific | ||
| /// than the actual endpoint being accessed. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task CannotAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath() | ||
| public async Task CanAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The comment in // CannotAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath validates we won't fall back to root in this case.Since this test now validates that root-level resource is accepted, that cross-reference comment should be updated to reflect the new name and the new behavior. |
||
| { | ||
| const string requestedResourcePath = "/mcp/tools"; | ||
|
|
||
|
|
@@ -839,12 +845,52 @@ public async Task CannotAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPa | |
| }, | ||
| }, HttpClient, LoggerFactory); | ||
|
|
||
| var ex = await Assert.ThrowsAsync<McpException>(async () => | ||
| await using var client = await McpClient.CreateAsync( | ||
| transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that OAuth authentication fails when the protected resource metadata URI | ||
| /// does not match the requested MCP server endpoint. This ensures that clients cannot | ||
| /// use OAuth tokens intended for one server to access a different server. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task CannotAuthenticate_WhenResourceMetadataUriDoesNotMatch() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (minor): Consider adding a test for same-authority but different-path resource mismatch, e.g. |
||
| { | ||
| const string requestedResourcePath = "/mcp/tools"; | ||
| const string differentResourceUri = "http://different-server.example.com"; | ||
|
|
||
| Builder.Services.Configure<McpAuthenticationOptions>(McpAuthenticationDefaults.AuthenticationScheme, options => | ||
| { | ||
| await McpClient.CreateAsync( | ||
| transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken); | ||
| options.ResourceMetadata = new ProtectedResourceMetadata | ||
| { | ||
| Resource = differentResourceUri, | ||
| AuthorizationServers = { OAuthServerUrl }, | ||
| }; | ||
| }); | ||
|
|
||
| await using var app = Builder.Build(); | ||
|
|
||
| app.MapMcp(requestedResourcePath).RequireAuthorization(); | ||
|
|
||
| await app.StartAsync(TestContext.Current.CancellationToken); | ||
|
|
||
| await using var transport = new HttpClientTransport(new() | ||
| { | ||
| Endpoint = new Uri($"{McpServerUrl}{requestedResourcePath}"), | ||
| OAuth = new() | ||
| { | ||
| ClientId = "demo-client", | ||
| ClientSecret = "demo-secret", | ||
| RedirectUri = new Uri("http://localhost:1179/callback"), | ||
| AuthorizationRedirectDelegate = HandleAuthorizationUrlAsync, | ||
| }, | ||
| }, HttpClient, LoggerFactory); | ||
|
|
||
| // This should fail because the resource URI doesn't match | ||
| var ex = await Assert.ThrowsAsync<McpException>(() => McpClient.CreateAsync( | ||
| transport, loggerFactory: LoggerFactory, cancellationToken: TestContext.Current.CancellationToken)); | ||
|
|
||
| Assert.Contains("does not match", ex.Message); | ||
| } | ||
|
|
||
|
|
@@ -853,7 +899,7 @@ public async Task ResourceMetadata_DoesNotAddTrailingSlash() | |
| { | ||
| // This test verifies that automatically derived resource URIs don't have trailing slashes | ||
| // and that the client doesn't add them during authentication | ||
|
|
||
| // Don't explicitly set Resource - let it be derived from the request | ||
| await using var app = await StartMcpServerAsync(); | ||
|
|
||
|
|
@@ -993,10 +1039,10 @@ public async Task ResourceMetadata_PreservesExplicitTrailingSlash() | |
| { | ||
| // This test verifies that explicitly configured trailing slashes are preserved | ||
| const string resourceWithTrailingSlash = "http://localhost:5000/"; | ||
|
|
||
| // Configure ValidResources to accept the trailing slash version for this test | ||
| TestOAuthServer.ValidResources = [resourceWithTrailingSlash, "http://localhost:5000/mcp"]; | ||
|
|
||
| Builder.Services.Configure<McpAuthenticationOptions>(McpAuthenticationDefaults.AuthenticationScheme, options => | ||
| { | ||
| options.ResourceMetadata = new ProtectedResourceMetadata | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The XML doc comment on
VerifyResourceMatch(lines 776-785 above the diff) still says:Verifies that the resource URI in the metadata exactly matches the original request URL as required by the RFC.Per RFC: The resource value must be identical to the URL that the client used to make the request to the resource server.<returns>True if the resource URI exactly matches the original request URL, otherwise false.</returns>Since the method now also accepts authority-level (base URL) matches, the doc comment should be updated to reflect the new matching behavior.