Fix: Relax resource URI validation to accept base URL#1517
Conversation
Per MCP spec, authorization operates at base URL level (path discarded). The SDK now accepts OAuth metadata when the 'resource' field contains either the full MCP endpoint URI or the base URL (authority only).
JBallan
left a comment
JBallan
left a comment
There was a problem hiding this comment.
Hi! The workflows are currently awaiting approval, so CodeQL checks can't run yet and the PR is blocked. Could a maintainer approve the workflows and review the PR when possible? Thanks!
JBallan
left a comment
JBallan
left a comment
There was a problem hiding this comment.
Could a maintainer review the PR when possible? Thanks!
|
Hope somebody reviews this soon... the current strict evaluation of URI's breaks the SDK Client for a lot of popular MCP services (Linear for example) |
|
This is a real issue could a reviewer have a look? @tarekgh @halter73 @mikekistler @jeffhandley |
|
I'll take a look today. |
|
I suggest if we can edit the section https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#canonical-server-uri (in another PR) relaxing the statement aligns with the resource parameter in RFC 9728 to something like effectively extending the RFC 9728's resource parameter validation model |
| /// </summary> | ||
| [Fact] | ||
| public async Task CannotAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath() | ||
| public async Task CanAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath() |
There was a problem hiding this comment.
nit: The comment in CannotAuthenticate_WhenResourceMetadataResourceIsNonRootParentPath at line 771 still references the old test name and makes the opposite claim:
// 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.
| string normalizedResourceLocation = NormalizeUri(resourceLocation); | ||
|
|
||
| return string.Equals(normalizedMetadataResource, normalizedResourceLocation, StringComparison.OrdinalIgnoreCase); | ||
| // Accept exact match with the full MCP endpoint URI |
There was a problem hiding this comment.
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.
|
nit (minor): The comment at // Per RFC: The resource value must be identical to the URL that the client used to make the request to the resource serverSince // Per RFC 9728, validate that the resource URI in metadata corresponds to the server we're connecting to.
// VerifyResourceMatch accepts both exact URI match and authority-level (base URL) match per MCP spec. |
| return true; | ||
| } | ||
|
|
||
| // Per MCP spec: "The authorization base URL MUST be derived by discarding the path component from the MCP server URL" |
There was a problem hiding this comment.
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 https://mcp.example.com/mcp and https://mcp.example.com as valid canonical URIs for resource identification. Consider updating the comment to reference that section instead.
| /// use OAuth tokens intended for one server to access a different server. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task CannotAuthenticate_WhenResourceMetadataUriDoesNotMatch() |
There was a problem hiding this comment.
suggestion (minor): Consider adding a test for same-authority but different-path resource mismatch, e.g. resource=https://example.com/service-a vs endpoint https://example.com/service-b. The existing tests cover different authorities and parent-path mismatches, but this scenario (unrelated path on the same host) would strengthen confidence that the authority-level fallback only accepts authority-only resources and not arbitrary paths on the same host.
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
Added minor comment, LGTM otherwise.
4 participants
Fix #1119
Summary
Per MCP specification, OAuth authorization operates at the base URL level (path discarded). The SDK now accepts OAuth metadata when the
resourcefield contains either:https://server.example.com/mcp/tools)https://server.example.com)This aligns with RFC 9728 § 3.1 which states that protected resource metadata can be scoped at the authority level.
Motivation and Context
The SDK's OAuth client was throwing a validation error when the
resourcefield in OAuth protected resource metadata returned the base URL instead of the full MCP endpoint path. This prevented legitimate OAuth flows where servers declare protected resources at the authority level, which is:Example scenario that now works:
↑ Now validates successfully
Implementation Details
Updated
OAuthClientHandler.TryGetResourceMetadataAsync:How Has This Been Tested?
Test Changes
Renamed test:
CannotAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPath→CanAuthenticate_WhenWwwAuthenticateResourceMetadataIsRootPathNew test:
CannotAuthenticate_WhenResourceMetadataUriDoesNotMatchBreaking Changes
No breaking changes. This is a behavioral enhancement that makes OAuth resource URI validation more flexible while maintaining backward compatibility.
Behavior Change Details
Previous behavior:
New behavior:
Types of changes
Checklist