Skip to content

[v1.x] fix: reject trailing newline in tool-name validation #3086

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
maxisbey wants to merge 2 commits into
base: v1.x
Choose a base branch
from
+22 −2
Open

[v1.x] fix: reject trailing newline in tool-name validation #3086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6b2fc0
fix: reject trailing newline in tool-name validation
maxisbey Jul 10, 2026
5052de7
fix: reject trailing newline in resource template matching
maxisbey Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/mcp/server/fastmcp/resources/templates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@
def matches(self, uri: str) -> dict[str, Any] | None:
"""Check if URI matches template and extract parameters."""
# Convert template to regex pattern
pattern = self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)")
match = re.match(f"^{pattern}$", uri)
match = re.fullmatch(pattern, uri)

Check notice on line 89 in src/mcp/server/fastmcp/resources/templates.py

View check run for this annotation

Claude / Claude Code Review

Template literal segments not regex-escaped in ResourceTemplate.matches() 

Pre-existing issue, not introduced by this PR: the template-to-regex conversion on line 88 never `re.escape()`s the literal (non-parameter) segments, so regex metacharacters in a template are interpreted as regex syntax — e.g. template `test://a.b/{x}` matches `test://aXb/foo` because `.` is a wildcard, and literals containing `(` raise `re.error` at match time. Since this PR already tightens this exact line via `re.fullmatch`, a natural follow-up is to split the template on `{param}` boundaries
Comment on lines 88 to +89

@claude claude Bot Jul 16, 2026

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: the template-to-regex conversion on line 88 never re.escape()s the literal (non-parameter) segments, so regex metacharacters in a template are interpreted as regex syntax — e.g. template test://a.b/{x} matches test://aXb/foo because . is a wildcard, and literals containing ( raise re.error at match time. Since this PR already tightens this exact line via re.fullmatch, a natural follow-up is to split the template on {param} boundaries and re.escape() the literal chunks before joining them with the named-group patterns.

Extended reasoning...

The bug

ResourceTemplate.matches() builds its regex by naive string replacement:

pattern = self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)")
match = re.fullmatch(pattern, uri)

Only the {/} delimiters are transformed — the literal (non-parameter) segments of the URI template are spliced into the regex verbatim, with no re.escape(). Any regex metacharacter in a literal segment (., +, ?, *, (, ), [, |, ...) is therefore interpreted as regex syntax instead of being matched literally.

Step-by-step proof

Take the template test://a.b/{x}:

  1. The .replace() calls produce the pattern test://a.b/(?P<x>[^/]+).
  2. The . in a.b is an unescaped regex wildcard, so it matches any character, not just a literal dot.
  3. re.fullmatch('test://a.b/(?P<x>[^/]+)', 'test://aXb/foo') succeeds — . consumes X.
  4. matches() returns {'x': 'foo'}, and the URI is dispatched to a handler whose template it does not actually match.

Two worse variants were verified empirically:

  • Semantics change: template test://a+b/{x} compiles a+ as "one or more a", so it matches test://ab/foo and test://aab/foo — and, depending on the surrounding literal, can fail to match its own literal form test://a+b/foo.
  • Crash: a template containing a bare ( in a literal segment (e.g. test://a(b/{x}) raises re.error: missing ), unterminated subpattern at match time, turning a registration-time template typo into a runtime exception on every read_resource lookup.

Why nothing prevents it

There is no escaping or validation anywhere on this path: ResourceTemplate.from_function stores uri_template as a plain string, and matches() is the only place it is interpreted. The wire path's pydantic AnyUrl normalization does not help here — it normalizes the incoming URI, not the server-side template, and dots/plus signs are perfectly legal URL characters that survive normalization.

Relation to this PR

This is squarely pre-existing: the naive .replace() construction on line 88 is untouched by this PR, which only switched the adjacent re.match(f"^{pattern}$", uri) to re.fullmatch(pattern, uri) — a strictly tighter change that neither introduces nor worsens the escaping gap. It's worth noting only because the PR modifies this exact function and fixes the same bug class (overly-permissive template matching), so a follow-up here would be a natural companion.

How to fix

Split the template on {param} boundaries and escape the literal chunks before reassembling, e.g.:

parts = re.split(r"\{(\w+)\}", self.uri_template)
pattern = "".join(
    f"(?P<{part}>[^/]+)" if i % 2 else re.escape(part)
    for i, part in enumerate(parts)
)
match = re.fullmatch(pattern, uri)

This keeps the existing [^/]+ parameter semantics while making literal segments match literally, and eliminates the re.error crash for templates containing regex metacharacters.

if match:
return match.groupdict()
return None
Expand Down
2 changes: 1 addition & 1 deletion src/mcp/shared/tool_name_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def validate_tool_name(name: str) -> ToolNameValidationResult:
warnings.append("Tool name starts or ends with a dot, which may cause parsing issues in some contexts")

# Check for invalid characters
if not TOOL_NAME_REGEX.match(name):
if not TOOL_NAME_REGEX.fullmatch(name):
Comment thread
claude[bot] marked this conversation as resolved.
# Find all invalid characters (unique, preserving order)
invalid_chars: list[str] = []
seen: set[str] = set()
Expand Down
15 changes: 15 additions & 0 deletions tests/server/fastmcp/resources/test_resource_template.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ def my_func(key: str, value: int) -> dict[str, Any]: # pragma: no cover
assert template.matches("test://foo") is None
assert template.matches("other://foo/123") is None

def test_template_matches_rejects_trailing_newline_after_literal(self):
"""A trailing newline after a literal segment slipped past `$` with re.match."""

def my_func(key: str) -> str: # pragma: no cover
return key

template = ResourceTemplate.from_function(
fn=my_func,
uri_template="test://{key}/data",
name="test",
)

assert template.matches("test://foo/data") == {"key": "foo"}
assert template.matches("test://foo/data\n") is None

@pytest.mark.anyio
async def test_create_resource(self):
"""Test creating a resource from a template."""
Expand Down
5 changes: 5 additions & 0 deletions tests/shared/test_tool_name_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,17 @@ def test_rejects_name_exceeding_max_length(self) -> None:
("get,user,profile", "','"),
("user/profile/update", "'/'"),
("user@domain.com", "'@'"),
# a single trailing newline slipped past `$` with re.match
("valid_name\n", "'\\n'"),
("a" * 127 + "\n", "'\\n'"),
],
ids=[
"with_spaces",
"with_commas",
"with_slashes",
"with_at_symbol",
"with_trailing_newline",
"max_length_with_trailing_newline",
],
)
def test_rejects_invalid_characters(self, tool_name: str, expected_char: str) -> None:
Expand Down
Loading