Add protected resource metadata response inspect & modify support #1262
Conversation
| } | ||
| } | ||
|
|
||
| private static Uri GetRequiredResourceUri(ProtectedResourceMetadata protectedResourceMetadata) |
There was a problem hiding this comment.
We need to keep this validation particularly for the non-legacy OAuth flow where the authorization server and resource server are hosted separately.
8. Resource Parameter Implementation
MCP clients MUST implement Resource Indicators for OAuth 2.0 as defined in RFC 8707
to explicitly specify the target resource for which the token is being requested. Theresourceparameter:
- MUST be included in both authorization requests and token requests.
- MUST identify the MCP server that the client intends to use the token with.
- MUST use the canonical URI of the MCP server as defined in RFC 8707 Section 2.
The spec mandates this, because otherwise the OAuth server has no way to know what MCP server the client plans to connect to using the access token it's given (since the MCP server URI and redirect URI are completely independent) and therefore could not prevent a phishing attack causing the client to send tokens to an untrusted MCP server. This is explained in https://den.dev/blog/mcp-authorization-resource/.
You can find previous discussion about this at #1041 (comment)
This may not be an issue specifically for the legacy flow where the resource server and the authorization are one and the same, so there's no risk of sending a token to a malicious resource server, but this is shared code.
This kind of issue is why we've been hesitant to support the legacy OAuth flow, and why cohosting the OAuth server and the resource server implicitly is no longer part of the official MCP spec.
Frankly, I'm surprised Atlassian hasn't just hosted a PRM JSON document at https://mcp.atlassian.com/.well-known/oauth-protected-resource yet. It could point at the existing cohosted OAuth server.
Given that Atlassian has the only major MCP server requiring OAuth that I'm aware of without a PRM JSON document, I'm hesitant to add extra logic to the ClientOAuthProvider just for that. I wonder if writing a DelegatingHandler to create a fake /.well-known/oauth-protected-resource document might be a reasonable workaround.
There was a problem hiding this comment.
Frankly, I'm surprised Atlassian hasn't just hosted a PRM JSON document at https://mcp.atlassian.com/.well-known/oauth-protected-resource yet. It could point at the existing cohosted OAuth server.
Likewise 👀
Given that Atlassian has the only major MCP server requiring OAuth that I'm aware of without a PRM JSON document, I'm hesitant to add extra logic to the ClientOAuthProvider just for that.
I agree with the sentiment here as well.
I wonder if writing a DelegatingHandler to create a fake /.well-known/oauth-protected-resource document might be a reasonable workaround.
It seems reasonable to me based on the notion that alternatives such as: making the optionality of the resource parameter predicated on the availability of the PRM document - is adding too much complexity for little gains.
There was a problem hiding this comment.
Here is a more surgical fix main...FICTURE7:csharp-sdk:fallback-2. A delegating handler would offer more flexibility than this approach.
This patch adds `ProtectedResourceMetadataResponseDelegate` on `ClientOAuthOptions` which enables consumers to inspect and modify the response of the PRM request. This new delegate is also called when a PRM is not found, allowing consumers to supply one. This is useful for MCP servers which are not compliant with the latest MCP spec. For example Atlassian's MCP server that currently does not provide a PRM.
FICTURE7
changed the title
|
@halter73, I've pushed a new change set that adds a I tried to avoid to change the default behavior. It was not clear to me if we wanted a default behavior similar to main...FICTURE7:csharp-sdk:fallback-2. |
|
|
||
| if (metadata is null) | ||
| // Allow delegate to inspect or modify the metadata, and perform a defensive copy at the end. | ||
| metadata = _protectedResourceMetadataResponseDelegate(metadata)?.Clone(); |
There was a problem hiding this comment.
Performed a defensive copy here to prevent shenanigans from consumers capturing and mutating the PRM object outside of the PRM response delegate. That PRM object is potentially long lived.
Plus the clone method was already around.
2 participants
This patch adds support for legacy behavior defined in MCP 2025-03-26, where by if the MCP server does not provide a PRM, the MCP client uses the MCP server's base URL as the authorization server.
Since a PRM is not specified, a
resourceindicator is also not specified, this means the code has to updated to handle this optionality.Motivation and Context
There exists multiple MCP server implementation out there, and many of them are not compliant with the latest MCP specification. For example, the official Atlassian MCP server.
Without this patch, the C# MCP SDK is not compatible with Atlassian's MCP server (https://mcp.atlassian.com/v1/mcp), and would throw an
McpExceptionwithFailed to find protected resource metadata at a well-known location for {...}.csharp-sdk/src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs
Lines 790 to 793 in 27b5eb8
On the other hand, the TypeScript MCP SDK seems to handle this case well.
https://github.com/modelcontextprotocol/typescript-sdk/blob/91f6a277c43babe8962c7782030162530a82b2f5/packages/client/src/client/auth.ts#L410-L416
How Has This Been Tested?
Performed a successful authentication against Atlassian's MCP server.
Types of changes
Checklist