Change ProtectedResourceMetadata URI properties to strings and build resource strings directly#1264
Conversation
…ormalization Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
…trailing slashes Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
… assertion Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
halter73
left a comment
There was a problem hiding this comment.
I think using Uri in ProtectedResourceMetadata was a mistake since it does unnecessary normalization during deserialization that prevents us from seeing the original value we are supposed to round trip.
This should fix #1122.
|
@copilot Can we just remove the properties from |
Per feedback, removed properties that are not used for client OAuth authentication: - BearerMethodsSupported - JwksUri - ResourceSigningAlgValuesSupported - ResourceName - ResourceDocumentation - ResourcePolicyUri - ResourceTosUri - TlsClientCertificateBoundAccessTokens - AuthorizationDetailsTypesSupported - DpopSigningAlgValuesSupported - DpopBoundAccessTokensRequired Kept only the properties necessary for OAuth client authentication: - Resource - AuthorizationServers - ScopesSupported - WwwAuthenticateScope (internal) Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Done. Removed all unused properties from |
This reverts commit f18c391.
|
@stephentoub Can you reapprove this? It's exactly as you saw it last. I reverted the property removals I asked copilot to make, because I noticed/remembered that we use the same DTO to produce the PRM on the server, so it is nice to have all those properties like |
An alternative would be to use Uri.OriginalString when passing the resource indicator in the token/authorize requests. Unless there is some edge cases am unaware of. |
|
Thanks @FICTURE7. I considered that after seeing it in your Since we do validate that the That does make me notice we used |
Added [StringSyntax(StringSyntaxAttribute.Uri)] to all URI string properties in ProtectedResourceMetadata: - Resource - JwksUri - ResourceDocumentation - ResourcePolicyUri - ResourceTosUri This provides better IDE support and validation for URI strings, consistent with other protocol types like ReadResourceRequestParams. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Done. Added |
Changes per MCP spec recommendation: - Remove trailing slash variants from TestOAuthServer.ValidResources (kept only no-slash versions) - Make ValidResources public static to allow test customization - Update McpAuthenticationHandler to trim trailing slashes when deriving resource URIs - Update ProtectedResourceMetadata.Clone to trim trailing slashes from derived URIs - Improve ResourceMetadata_DoesNotAddTrailingSlash test to actually authenticate (not just fetch metadata) - Add ResourceMetadata_PreservesExplicitTrailingSlash test to verify explicitly configured slashes work Per MCP spec: implementations SHOULD use URIs without trailing slashes unless semantically significant. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
Per feedback: - Changed ProtectedResourceMetadata.Clone() to take string? instead of Uri? parameter - Changed ValidResources from static to instance property (no need for try/finally) - Updated HandleDefaultResourceMetadataRequestAsync to build resource string directly without intermediate Uri - Trim trailing slashes from path when building derived resource string - Added manual HTTP metadata checks to both tests to verify PRM document contents - Removed try/finally from ResourceMetadata_PreservesExplicitTrailingSlash (not needed with instance property) Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
src/ModelContextProtocol.AspNetCore/Authentication/McpAuthenticationHandler.cs
Outdated
Show resolved
Hide resolved
Use StringComparison.OrdinalIgnoreCase for scheme comparisons to handle HTTP/HTTPS in any case. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Fix ProtectedResourceMetadata.Resource URI Normalization
This PR fixes an issue where using
Uritype for theProtectedResourceMetadata.Resourceproperty and related URI properties caused unwanted normalization during JSON serialization/deserialization, which added trailing slashes to URIs likehttp://localhost:5000.Changes
Protocol types
ProtectedResourceMetadatafromUri?/List<Uri>tostring?/List<string>Resource,AuthorizationServers[StringSyntax(StringSyntaxAttribute.Uri)]attributes to URI string properties for better IDE support and validationResource,JwksUri,ResourceDocumentation,ResourcePolicyUri,ResourceTosUriClone()method to takestring?instead ofUri?to avoid intermediate Uri creation and normalizationOAuth provider
ClientOAuthProvider.GetRequiredResourceUri()to returnstringNormalizeUri(string)overload withUri.TryCreatevalidationMCP spec compliance
McpAuthenticationHandler.HandleDefaultResourceMetadataRequestAsync()to build resource string directly using string interpolation, avoiding intermediateUriobjectsIsDefaultPort()helper with case-insensitive scheme comparison to properly format URIs with/without port numbersTests
ResourceMetadata_DoesNotAddTrailingSlashtest to:ResourceMetadata_PreservesExplicitTrailingSlashtest to:ValidResourcesproperty (no try/finally needed)TestOAuthServer.ValidResourcesto only accept URIs without trailing slashes by defaultValidResourcesfrom static to instance propertyExample
The
[StringSyntax]attribute provides IDE intellisense and validation for URI strings while avoiding the normalization issues that come with theSystem.Uritype. Building resource strings directly without intermediateUriobjects ensures no unwanted normalization occurs.Fixes #1122
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.