diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index eb6dbe51ca..6ac4ffc5f3 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -9,7 +9,7 @@ import re from typing import Annotated, Any, Literal -from urllib.parse import urlsplit +from urllib.parse import unquote, urlsplit from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource from pydantic import AfterValidator, BaseModel, ConfigDict, Field, model_validator @@ -250,7 +250,8 @@ def _validate_resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: root = skill_parts.path[: -len("/SKILL.md")] if resource_parts.path != skill_parts.path and not resource_parts.path.startswith(root + "/"): raise ValueError(f"resource URI {resource_uri!r} is outside the skill root {skill_uri!r}") - if any(segment in (".", "..") for segment in resource_parts.path.split("/")): + unquoted_path = unquote(resource_parts.path) + if any(segment in (".", "..") for segment in unquoted_path.split("/")): raise ValueError(f"resource URI {resource_uri!r} contains a traversal segment") @@ -290,7 +291,13 @@ def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None: if child.scheme != scheme or child.netloc != netloc: raise ValueError(f"resource {resource.uri!r} is not a child of directory {uri!r}") relative = child.path.removeprefix(prefix) - if relative == child.path or not relative or "/" in relative or relative in (".", ".."): + unquoted_relative = unquote(relative) + if ( + relative == child.path + or not relative + or "/" in unquoted_relative + or unquoted_relative in (".", "..") + ): raise ValueError(f"resource {resource.uri!r} is not a direct child of directory {uri!r}") if resource.uri in seen_uris: raise ValueError(f"directory {uri!r} contains a duplicate child {resource.uri!r}") diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index 64f7d6214b..04fb13dc8d 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -149,9 +149,18 @@ def test_skill_rejects_a_resource_with_the_same_authority_but_a_sibling_path() - ) -def test_skill_rejects_a_traversal_segment_in_a_resource_uri() -> None: +@pytest.mark.parametrize( + "evil_uri", + [ + "skill://git-workflow/../evil.md", + "skill://git-workflow/%2e%2e/evil.md", + "skill://git-workflow/%2E%2E/evil.md", + "skill://git-workflow/%2e/evil.md", + ], +) +def test_skill_rejects_a_traversal_segment_in_a_resource_uri(evil_uri: str) -> None: with pytest.raises(ValidationError, match="traversal segment"): - _skill(extra_resources=[_resource("skill://git-workflow/../evil.md")]) + _skill(extra_resources=[_resource(evil_uri)]) def test_skill_rejects_a_resource_uri_with_a_trailing_slash() -> None: @@ -342,7 +351,16 @@ def test_validate_directory_result_rejects_a_grandchild() -> None: validate_directory_result("skill://pdf/templates", result) -@pytest.mark.parametrize("child_uri", ["skill://pdf/templates/.", "skill://pdf/templates/.."]) +@pytest.mark.parametrize( + "child_uri", + [ + "skill://pdf/templates/.", + "skill://pdf/templates/..", + "skill://pdf/templates/%2e", + "skill://pdf/templates/%2e%2e", + "skill://pdf/templates/%2E%2E", + ], +) def test_validate_directory_result_rejects_a_dot_segment_child(child_uri: str) -> None: """A `.`/`..` child is a traversal segment, not a real direct child of the directory.""" result = ReadDirectoryResult(resources=[Resource(uri=child_uri, name="x")])