Skip to content

Conversation

@sr-auto
Copy link

@sr-auto sr-auto commented Nov 3, 2025

Enhance MCP authentication handler to correctly handle absolute URIs for resource metadata endpoint

Motivation and Context

This pull request refines how the resource metadata endpoint path is determined in the authentication handler and adds a comprehensive test to verify correct behavior when using absolute URIs with path components. The main focus is to ensure that requests to the metadata endpoint are routed and authenticated properly, especially when the endpoint is configured with an absolute URI that includes a path.

Authentication handler improvements:

  • Improved logic in McpAuthenticationHandler.cs to correctly extract the path component from ResourceMetadataUri when it is an absolute URI, ensuring accurate endpoint matching.

How Has This Been Tested?

  • Added a new test ResourceMetadataEndpoint_ResolvesCorrectly_WithAbsoluteUriIncludingPathComponent in AuthTests.cs to verify:
    • The metadata endpoint responds correctly at the expected path when configured with an absolute URI including a path component.
    • Requests to other endpoints do not return metadata and respond with a 401 Unauthorized.
    • The WWW-Authenticate header includes the full absolute URI with path component.
  • Included System.Net.Http.Json in the test file to support JSON deserialization in the new test.

Breaking Changes

None, existing PRM URIs will not be affected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

.AddMcp(options =>
{
// Set an absolute URI with a path component after the host
options.ResourceMetadataUri = new Uri($"{McpServerUrl}{fullMetadataPath}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for a custom relative path? If not, can we add that too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll add one in.

else
{
// For absolute URIs, extract the path component
expectedMetadataPath = Options.ResourceMetadataUri.AbsolutePath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems wrong to throw away the host name when it's been explicitly configured. If the app developer configures AllowedHosts to something other than *, they are already protected against DNS rebinding, but not everyone does that.

But if the developer chooses a specific host name for the ResourceMetadataUri, it seems that we should require it. If the developer doesn't want to require a specific host name for the resource metadata data document, they can use a relative URI.

Copy link
Contributor

@halter73 halter73 Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've learned via chat that the motivating reason for using an absolute ResourceMetadataUri was to fix the scenario where GetBaseUrl() => $"{Request.Scheme}://{Request.Host}{Request.PathBase}" gave inaccurate results due to a gateway in front of an app without properly configured forwarded headers. The result was that context.Request.Host contained an internal hostname rather than the publicly visible one specified in ResourceMetadataUri. Basically, this is exactly the scenario I suggested we reject for having the "wrong" host name.

For this scenario, I think using app.UseForwardedHeaders() and a relative ResourceMetadataUri is the best approach. Configuring forwarded headers could help fix other scenarios involving absolute link generation too.

Given that an absolute ResourceMetadataUri has never worked, I wonder if the best bet for now is simply to reject absolute URIs by throwing an ArgumentException in the setter. If people really do need to have complete control over the absolute URI generated for the Www-Authentication header, that could probably be a new configuration and/or McpAuthenticationEvents that gets run in HandleChallengeAsync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants