Fix regression matching templated McpServerResources #897
+89
−34
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
halter73
commented
Oct 17, 2025
| /// </summary> | ||
| /// <param name="uri">The URI being evaluated for this resource.</param> | ||
| /// <returns><see langword="true"/> if the resource is able to handle the URI; otherwise, <see langword="false"/>.</returns> | ||
| public abstract bool CanReadUri(string uri); |
There was a problem hiding this comment.
I don't like the name CanReadUri. Would people prefer MatchesUri? Or something else?
There was a problem hiding this comment.
Matches is much better than CanRead for this, imo.
There was a problem hiding this comment.
I'm fine with both. Others like HandlesUri would be fine, too.
halter73
changed the title
stephentoub
reviewed
Oct 18, 2025
| public override bool CanReadUri(string uri) | ||
| { | ||
| Throw.IfNull(uri); | ||
| return TryMatch(uri, out _); |
There was a problem hiding this comment.
This is going to create the Match object if it does match. It'd be better if the Can path used IsMatch instead
stephentoub
approved these changes
Oct 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a regression reported in #821 that was caused by changes in #733 which caused resource template matching to break when there were multiple templated resources.
It's important for the auth pipeline to determine which resource is matched to make authorization decisions without executing the handlers associated with reading the resource. So rather than call McpServerResource.ReadAsync prior to running the filter pipeline, #733 wrongly assumed the first templated McpServerResource it saw would be able to handle any templated request. To fix this while still avoiding calling McpServerResource.ReadAsync just to see if the resource matched before auth filter can prevent it, this PR adds a new McpServerResource.CanReadUri method.
Unlike before, this will call Regex.Match twice for the matched resource. We could flow the Match to the eventual ReadAsync call via the RequestContext, but I don't think this one additional call is a huge concern though, considering that we already call Regex.Match O(N) times where N is the number of templated resources. I figure we'll probably use some sort of trie if/when we further optimize this path at which point we could look at preserving the matches.
Fixes #821