Skip to content

Commit be15d59

Browse files
Reject Windows path syntax in FilesystemSkillProvider.read_file and tighten wording
1 parent 4402d2a commit be15d59

4 files changed

Lines changed: 50 additions & 9 deletions

File tree

‎docs/advanced/skills.md‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,8 @@ Each response then holds at most 50 entries and carries a `nextCursor` while mor
254254
`SkillsClient.list_skills()` and `read_directory()` follow it for you. A skill entry is never split
255255
across pages.
256256

257-
The cursor is opaque. One the catalog didn't issue is rejected with Invalid params (`-32602`), and
258-
so is any cursor when `page_size` isn't set. A `page_size` below 1 raises `ValueError`.
257+
The cursor is opaque. A cursor the catalog would not issue is rejected with Invalid params
258+
(`-32602`), and so is any cursor when `page_size` isn't set. A `page_size` below 1 raises `ValueError`.
259259

260260
### Invalid skills
261261

@@ -310,9 +310,12 @@ or `..` segment, is listed more than once, or lies beneath another listed file.
310310
regardless of `strict`, because it indicates an error in the provider rather than in a skill.
311311

312312
`FilesystemSkillProvider(root)` is the provider for a local directory. It serves regular files
313-
only. Symbolic links and Windows junctions are never followed: they are omitted, with a warning,
314-
when listing, and refused when read. A skill directory therefore cannot expose files outside
315-
itself. Blocking file system calls run in a worker thread.
313+
only. Symbolic links and Windows junctions found while listing are omitted, with a warning, and
314+
ones found while reading are refused, so a skill directory doesn't expose files outside itself.
315+
316+
The root is trusted. This is a best-effort check, not a defence against someone who can modify the
317+
directory while the server runs. As a backstop, the catalog refuses to serve any file whose bytes
318+
no longer match the digest taken at `load()`. Blocking file system calls run in a worker thread.
316319

317320
## Protocol version and caching
318321

‎src/mcp/server/skills/filesystem.py‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@
1818
_READ_FLAGS = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) | getattr(os, "O_NONBLOCK", 0)
1919

2020

21+
# Windows reads `\` as a separator and `:` as drive syntax, so a "/"-only check lets `..\x` or `C:x` leave the root.
22+
# On POSIX both are ordinary file name characters.
23+
_FORBIDDEN_SEGMENT_CHARS = "\\:" if os.name == "nt" else ""
24+
2125
# Windows junctions and symlinks are reparse points tagged IO_REPARSE_TAG_MOUNT_POINT / IO_REPARSE_TAG_SYMLINK,
2226
# constants that `stat` only types on Windows. Other reparse points (cloud placeholders, dedup) are ordinary files.
2327
_WINDOWS_LINK_REPARSE_TAGS = frozenset({0xA0000003, 0xA000000C})
@@ -53,12 +57,16 @@ async def read_file(self, path: str) -> bytes:
5357
"""Return the bytes of the regular file at `path`, relative to the root.
5458
5559
Raises:
56-
ValueError: If `path` has an empty, `.` or `..` segment.
60+
ValueError: If `path` has an empty, `.` or `..` segment, or, on Windows, a segment
61+
containing `\\` or `:`.
5762
OSError: If the file is missing, can't be read, is a symlink, or isn't a
5863
regular file, or if a directory on the way to it is a symlink.
5964
"""
6065
segments = path.split("/")
61-
if any(segment in ("", ".", "..") for segment in segments):
66+
if any(
67+
segment in ("", ".", "..") or any(char in segment for char in _FORBIDDEN_SEGMENT_CHARS)
68+
for segment in segments
69+
):
6270
raise ValueError(f"invalid skill file path {path!r}")
6371
return await anyio.to_thread.run_sync(self._read_file, segments)
6472

‎src/mcp/server/skills/provider.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@ class SkillProvider(Protocol):
1616
"""
1717

1818
async def list_files(self) -> Sequence[str]:
19-
"""Return the path of every file, recursively.
19+
"""Return the path of every file the provider serves, recursively.
2020
21-
Directories are not listed, and each path appears once.
21+
Directories are not listed, and each path appears once. A provider may leave files out;
22+
`FilesystemSkillProvider` omits any name that starts with `.`.
2223
"""
2324
...
2425

‎tests/server/skills/test_filesystem.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
# bodies are excluded from coverage by a pragma on each decorator.
2323
requires_file_symlinks = pytest.mark.skipif(sys.platform == "win32", reason="needs symlink privilege")
2424
requires_fifos = pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="POSIX-only")
25+
requires_posix_names = pytest.mark.skipif(sys.platform == "win32", reason="Windows forbids `\\` and `:` in names")
2526

2627
IO_REPARSE_TAG_MOUNT_POINT = 0xA0000003
2728
IO_REPARSE_TAG_SYMLINK = 0xA000000C
@@ -199,6 +200,34 @@ async def test_read_file_rejects_empty_dot_and_dot_dot_segments(tmp_path: Path,
199200
await FilesystemSkillProvider(tmp_path).read_file(path)
200201

201202

203+
@pytest.mark.parametrize("path", ["..\\secret.txt", "skill\\SKILL.md", "C:secret.txt", "skill/C:SKILL.md"])
204+
async def test_read_file_refuses_windows_path_syntax_that_could_leave_the_root(
205+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch, path: str
206+
) -> None:
207+
"""SDK-defined: on Windows `\\` separates and `:` names a drive, so a "/"-only check would let these escape.
208+
209+
The Windows rule is switched on here because the escape can't be shown from a POSIX path.
210+
"""
211+
root = tmp_path / "root"
212+
root.mkdir()
213+
(tmp_path / "secret.txt").write_bytes(b"outside the root")
214+
monkeypatch.setattr("mcp.server.skills.filesystem._FORBIDDEN_SEGMENT_CHARS", "\\:")
215+
216+
with pytest.raises(ValueError):
217+
await FilesystemSkillProvider(root).read_file(path)
218+
219+
220+
@requires_posix_names # pragma: lax no cover
221+
async def test_names_containing_a_colon_or_backslash_are_served_off_windows(tmp_path: Path) -> None:
222+
"""SDK-defined: `:` and `\\` are ordinary name characters on POSIX, so the Windows rule must not reject them."""
223+
(tmp_path / "skill").mkdir()
224+
(tmp_path / "skill" / "a:b\\c.md").write_bytes(b"content")
225+
provider = FilesystemSkillProvider(tmp_path)
226+
227+
assert await provider.list_files() == ["skill/a:b\\c.md"]
228+
assert await provider.read_file("skill/a:b\\c.md") == b"content"
229+
230+
202231
async def test_read_file_raises_os_error_for_a_missing_file(tmp_path: Path) -> None:
203232
with pytest.raises(OSError):
204233
await FilesystemSkillProvider(tmp_path).read_file("missing.md")

0 commit comments

Comments
 (0)