diff --git a/src/fastmcp/utilities/skills.py b/src/fastmcp/utilities/skills.py index a84728fe3..49b13d859 100644 --- a/src/fastmcp/utilities/skills.py +++ b/src/fastmcp/utilities/skills.py @@ -164,7 +164,11 @@ async def download_skill( ``` """ target_dir = Path(target_dir).expanduser().resolve() - skill_dir = target_dir / skill_name + skill_dir = (target_dir / skill_name).resolve() + + # Security: ensure skill_dir stays within target_dir + if not skill_dir.is_relative_to(target_dir): + raise ValueError(f"Skill name {skill_name!r} would escape the target directory") # Check if directory exists if skill_dir.exists() and not overwrite: diff --git a/tests/utilities/test_skills.py b/tests/utilities/test_skills.py index 28a3f4fd1..46170529d 100644 --- a/tests/utilities/test_skills.py +++ b/tests/utilities/test_skills.py @@ -267,3 +267,24 @@ class TestSyncSkills: assert isinstance(path, Path) assert path.exists() assert (path / "SKILL.md").exists() + + +class TestPathTraversal: + @pytest.mark.parametrize( + "malicious_name", + [ + "../escape", + "../../root", + "../../../etc/passwd", + "foo/../../escape", + ], + ) + async def test_malicious_skill_name_raises( + self, skills_server: FastMCP, tmp_path: Path, malicious_name: str + ): + target = tmp_path / "downloaded" + target.mkdir() + + async with Client(skills_server) as client: + with pytest.raises(ValueError, match="would escape the target directory"): + await download_skill(client, malicious_name, target)