Existing MCP servers trigger panics for resource templates #365
Conversation
|
I just re-read the resources section of the spec. There is nothing about requiring absolute URIs. I think we should remove all our checks for schemes. And even if we kept the check, it's a bug to use url.Parse for it in Do you want to repurpose this PR to do this? |
|
Okay, I'll update this PR and remove the scheme check for both resources and resource templates. For resourceTemplates, I'll remove the Url parse entirely because it's not clear we should be trying to make any additional assumptions about these strings in the SDK. |
slimslenderslacks
force-pushed
the
slim/panics
branch
from
05bca41 to
9a8084d
Compare
mcp/server.go
Outdated
| s.changeAndNotify(notificationResourceListChanged, &ResourceListChangedParams{}, | ||
| func() bool { | ||
| u, err := url.Parse(r.URI) | ||
| _, err := url.Parse(r.URI) |
There was a problem hiding this comment.
nit: if _, err := ...; err != nil
There was a problem hiding this comment.
Oh ya! Updated that
|
@jba I think it's probably okay to just remove those failing tests since they we're not going assume anything about ResourceTemplate strings for now. |
remove checks for URI scheme; nothing in the spec requires one
# Background
When adding resource templates, we are currently doing checks for
schemes on absolute uris. It is still common for MCP servers to use
custom uris that would not pass these checks. For example, if the
official GitHub MCP server were to use this sdk, it would show failures
like:
```
"repo://{owner}/{repo}/contents{/path*}": parse "repo://{owner}/{repo}/contents{/path*}": invalid character "{" in host name
"repo://{owner}/{repo}/refs/heads/{branch}/contents{/path*}": parse "repo://{owner}/{repo}/refs/heads/{branch}/contents{/path*}": invalid character "{" in host name
"repo://{owner}/{repo}/sha/{sha}/contents{/path*}": parse "repo://{owner}/{repo}/sha/{sha}/contents{/path*}": invalid character "{" in host name
invalid resource template uri "repo://{owner}/{repo}/refs/pull/{prNumber}/head/contents{/path*}": parse "repo://{owner}/{repo}/refs/pull/{prNumber}/head/contents{/path*}": invalid character "{" in host name
"repo://{owner}/{repo}/refs/tags/{tag}/contents{/path*}": parse "repo://{owner}/{repo}/refs/tags/{tag}/contents{/path*}": invalid character "{" in host name
```
The wikipedia MCP would also show problems with missing schemes.
```
"/search/{query}": <nil>
"/article/{title}": <nil>
"/summary/{title}": <nil>
"/summary/{title}/query/{query}/length/{max_length}": <nil>
"/summary/{title}/section/{section_title}/length/{max_length}": <nil>
"/sections/{title}": <nil>
"/links/{title}": <nil>
"/facts/{title}/topic/{topic_within_article}/count/{count}": <nil>
"/coordinates/{title}": <nil>
```
2 participants
Background
When adding resource templates, we are currently doing checks for schemes on absolute uris. It is still common for MCP servers to use custom uris that would not pass these checks. For example, if the official GitHub MCP server were to use this sdk, it would show failures like:
The wikipedia MCP would also show problems with missing schemes.
What I did
I have left the URI.parse check in the AddResource but have removed the scheme check for absolute uris.
For AddResourceTemplate, I have removed the Uri.parse entirely because it's not clear we can make any assumptions about these strings.