Skip to content

Match URI template literals literally in ResourceTemplate.matches#2991

Closed
Kludex wants to merge 1 commit into
mainfrom
fix-resource-template-literal-matching
Closed

Match URI template literals literally in ResourceTemplate.matches#2991
Kludex wants to merge 1 commit into
mainfrom
fix-resource-template-literal-matching

Conversation

@Kludex

@Kludex Kludex commented Jun 26, 2026

Copy link
Copy Markdown
Member

Summary

ResourceTemplate.matches builds its matching regex by replacing the {param} braces but leaves the surrounding literal text unescaped. Any regex metacharacter present in a template (., +, (, [, ...) is therefore interpreted as a pattern instead of a literal, so templates match URIs they shouldn't.

For example, the template resource://a.b+c/{id}:

  • should match only resource://a.b+c/<id>
  • but currently also matches resource://aXbYc/<id> (the . matches any char, + quantifies)

Change

re.escape the template before substituting placeholders, so literal characters match literally and only {param} segments become capture groups. The placeholder substitution tolerates the optional backslash re.escape may add around the braces.

Tests

Added two cases to tests/server/mcpserver/resources/test_resource_template.py:

  • a template containing ./+ matches the literal URI and rejects a metacharacter-expanded one
  • a template containing (.*) is treated literally, not as a wildcard

Existing template/url-decoding tests still pass.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

The template-to-regex conversion only substituted the `{param}` braces and
left the surrounding literal text unescaped, so any regex metacharacter in a
template (`.`, `+`, `(`, ...) was interpreted as a pattern rather than a
literal. A template like `resource://a.b+c/{id}` would also match
`resource://aXbYc/...`.

Escape the template with `re.escape` before substituting placeholders so
literal characters match literally; only `{param}` segments become capture
groups.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 2 files

Re-trigger cubic

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — small, correct fix that escapes the URI template before building the match regex, with focused tests covering the new behavior.

Extended reasoning...

Overview

This PR touches a single line of logic in ResourceTemplate.matches (src/mcp/server/mcpserver/resources/templates.py) plus two new unit tests. It applies re.escape to the URI template before converting {param} placeholders into named capture groups, so literal characters like ., +, and ( in templates no longer act as regex metacharacters. The placeholder substitution regex (\\?\{(\w+)\\?\}) correctly tolerates the backslashes that re.escape adds around braces (Python 3.7+ escapes {/}), and parameter names are \w+ so they are unaffected by escaping.

Security risks

None introduced — if anything this slightly tightens behavior, since templates with metacharacters previously over-matched URIs they shouldn't. There is no auth, crypto, or input-handling change beyond making matching stricter and more predictable.

Level of scrutiny

This is a small, self-contained correctness fix in the server resource-template matching path. The change is easy to verify by inspection, the new tests directly exercise the previously broken cases (metacharacters and a (.*) literal), and existing template/URL-decoding tests continue to cover the normal path. The pre-existing duplicated-parameter-name issue noted in the inline comment is a developer-authoring error that exists independently of this PR and should not block it.

Other factors

The diff is narrowly scoped (2 files, ~25 lines), the intent matches the implementation, and behavior for well-formed templates without metacharacters is unchanged.

Comment on lines +99 to 100
pattern = re.sub(r"\\?\{(\w+)\\?\}", r"(?P<\1>[^/]+)", re.escape(self.uri_template))
match = re.match(f"^{pattern}$", uri)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟣 Pre-existing issue (not introduced by this PR): a URI template that repeats a parameter name (e.g. test://{id}/{id}) produces a regex with two (?P<id>...) groups, so re.match raises re.error: redefinition of group name instead of matches() returning None — and since registration only validates the set of placeholders, the error surfaces unhandled at read_resource time. Since this PR rewrites this exact line, it may be worth either rejecting duplicate placeholders at registration or guarding the re.match call here.

Extended reasoning...

What the bug is. ResourceTemplate.matches converts the URI template into a regex by turning every {param} placeholder into a named capture group (?P<param>[^/]+). Python's re module forbids two groups with the same name in one pattern, so any template that repeats a parameter name — e.g. test://{id}/{id} — produces an invalid pattern, and re.match at templates.py:100 raises re.error('redefinition of group name id ...') instead of matches() returning None.

Why registration doesn't catch it. The @mcp.resource decorator (server.py:686) validates templates by comparing set(re.findall(r"{(\w+)}", uri)) against the function's parameter names. Because the comparison is over sets, a duplicated placeholder collapses to a single entry and passes validation as long as the function has a matching parameter. So @mcp.resource("test://{id}/{id}") over def fn(id: str) registers cleanly.

Where it blows up. ResourceManager.get_resource (resource_manager.py:97-99) iterates all registered templates and calls matches() on each one for every read_resource request. There is no try/except around that call, so a single template with a duplicated placeholder makes read_resource fail with an unhandled re.error — including for URIs that target other, perfectly valid templates — rather than producing a clean "unknown resource" or validation error.

Step-by-step proof.

  1. Register @mcp.resource("test://{id}/{id}") with def fn(id: str) -> str. Registration passes: set(re.findall(...)) == {"id"} == {"id"}.
  2. A client calls read_resource("anything://else") (or any URI). ResourceManager.get_resource loops over templates and calls template.matches(uri) on the broken one.
  3. re.escape("test://{id}/{id}")re.sub(...) yields test://(?P<id>[^/]+)/(?P<id>[^/]+).
  4. re.match(f"^{pattern}$", uri) raises re.error: redefinition of group name 'id' as group 2; was group 1.
  5. The exception propagates out of get_resource and the read_resource request fails with an internal error.

Why this is pre-existing. The previous implementation, self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)"), generated the exact same duplicate-group pattern, so the behavior is identical before and after this PR. The PR merely rewrites the line where the pattern is built; it neither introduces nor worsens the problem. It also requires a developer to author a template that repeats a parameter name, which is an unusual mistake that fails loudly on first use — so this should not block the PR.

How to fix. Either (a) reject duplicate placeholders at registration time — e.g. in the @resource decorator and ResourceTemplate.from_function, check re.findall(r"{(\w+)}", uri_template) for duplicates and raise a ValueError with a clear message — or (b) defensively wrap the re.match/pattern construction in matches() so an invalid template returns None (or logs and skips) instead of taking down unrelated read_resource calls. Option (a) is preferable since it surfaces the authoring mistake immediately.

@maxisbey maxisbey closed this Jun 26, 2026
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