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 1 commit into
base: v1.x
Choose a base branch
from
+6 −1
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
1 commit
Select commit
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/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 @@
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):

Check notice on line 80 in src/mcp/shared/tool_name_validation.py

View check run for this annotation

Claude / Claude Code Review

Same re.match + $ trailing-newline bug remains in ResourceTemplate.matches() on v1.x 

Pre-existing issue, not introduced by this PR: the same `$`-with-`re.match` trailing-newline bug fixed here for tool names also exists on v1.x in `ResourceTemplate.matches()` (src/mcp/server/fastmcp/resources/templates.py:89), where `'resource://user/123\n'` matches template `'resource://user/{id}'` with the newline captured into the extracted parameter. The wire path is protected by pydantic `AnyUrl` normalization, but `FastMCP.read_resource`/`Context.read_resource` accept raw `str`, so conside

@claude claude Bot Jul 10, 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 same $-with-re.match trailing-newline bug fixed here for tool names also exists on v1.x in ResourceTemplate.matches() (src/mcp/server/fastmcp/resources/templates.py:89), where 'resource://user/123\n' matches template 'resource://user/{id}' with the newline captured into the extracted parameter. The wire path is protected by pydantic AnyUrl normalization, but FastMCP.read_resource/Context.read_resource accept raw str, so consider a follow-up switching it to re.fullmatch — note this also means the PR description's claim that the URI half of #3076 doesn't apply to v1.x isn't quite complete.

Extended reasoning...

The bug

This PR fixes the classic Python regex pitfall — $ matches not only at end-of-string but also just before a single trailing \n, so re.match(r'^...$', s) accepts strings with a trailing newline — for TOOL_NAME_REGEX in tool_name_validation.py. The identical pattern remains on v1.x in ResourceTemplate.matches():

# src/mcp/server/fastmcp/resources/templates.py:85-92
def matches(self, uri: str) -> dict[str, Any] | None:
    """Check if URI matches template and extract parameters."""
    pattern = self.uri_template.replace("{", "(?P<").replace("}", ">[^/]+)")
    match = re.match(f"^{pattern}$", uri)
    if match:
        return match.groupdict()
    return None

Step-by-step proof

For template resource://user/{id} and input 'resource://user/123\n':

  1. The template is converted to the pattern ^resource://user/(?P<id>[^/]+)$.
  2. [^/]+ matches any non-slash character — including \n — so it greedily consumes 123\n.
  3. $ (without re.MULTILINE) matches at end-of-string or just before a trailing newline at end-of-string; here it matches at the very end after [^/]+ consumed the newline.
  4. Result: match.groupdict() returns {'id': '123\n'} — the URI is accepted and the trailing newline is smuggled into the extracted parameter, which is then passed as an argument to the resource function.

(Even if [^/]+ were non-greedy, step 3 would let $ match before the newline — either way the input is accepted when it should be rejected.)

Why the impact is limited

On the wire-protocol path, ReadResourceRequestParams.uri is a pydantic AnyUrl, and pydantic-core normalization strips a raw trailing newline ('resource://user/123\n' validates to 'resource://user/123'), so a remote client sending a compliant resources/read request cannot trigger this. However, the server-side entry points FastMCP.read_resource, Context.read_resource, and ResourceManager.get_resource all accept AnyUrl | str and pass a plain str through unnormalized, so in-process/programmatic callers still hit the bug.

Relation to this PR

This PR does not touch templates.py, so this is squarely pre-existing and should not block merge. It is worth noting, though, because the PR description states that the URI half of #3076 doesn't apply to v1.x (since uri_template.py is v2-only) — but this separate fastmcp instance of the same bug class does exist on v1.x, so the backport leaves one instance unfixed.

How to fix

Mirror this PR: use re.fullmatch(pattern, uri) instead of re.match(f'^{pattern}$', uri). Alternatively, anchor with \A...\Z — which src/mcp/server/experimental/task_scope.py:31-34 already does, with a comment explicitly warning about this exact $/trailing-newline hazard, so the fix direction is already established in this codebase.

# Find all invalid characters (unique, preserving order)
invalid_chars: list[str] = []
seen: set[str] = set()
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