Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions src/mcp/shared/skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")


Expand Down Expand Up @@ -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}")
Expand Down
24 changes: 21 additions & 3 deletions tests/shared/test_skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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")])
Expand Down
Loading