diff --git a/skills/skill-creator/scripts/package_skill.py b/skills/skill-creator/scripts/package_skill.py index 123475ac6a0..926f4239ad2 100644 --- a/skills/skill-creator/scripts/package_skill.py +++ b/skills/skill-creator/scripts/package_skill.py @@ -17,6 +17,14 @@ from pathlib import Path from quick_validate import validate_skill +def _is_within(path: Path, root: Path) -> bool: + try: + path.relative_to(root) + return True + except ValueError: + return False + + def package_skill(skill_path, output_dir=None): """ Package a skill folder into a .skill file. @@ -73,18 +81,21 @@ def package_skill(skill_path, output_dir=None): for file_path in skill_path.rglob("*"): # Security: never follow or package symlinks. if file_path.is_symlink(): - print(f"[ERROR] Symlinks are not allowed in skills: {file_path}") - print(" This is a security restriction to prevent including arbitrary files.") - return None + print(f"[WARN] Skipping symlink: {file_path}") + continue rel_parts = file_path.relative_to(skill_path).parts if any(part in EXCLUDED_DIRS for part in rel_parts): continue if file_path.is_file(): - # Calculate the relative path within the zip - arcname = file_path.relative_to(skill_path.parent) + resolved_file = file_path.resolve() + if not _is_within(resolved_file, skill_path): + print(f"[ERROR] File escapes skill root: {file_path}") + return None + # Calculate the relative path within the zip. + arcname = Path(skill_name) / file_path.relative_to(skill_path) zipf.write(file_path, arcname) print(f" Added: {arcname}") diff --git a/skills/skill-creator/scripts/test_package_skill.py b/skills/skill-creator/scripts/test_package_skill.py index e8567beaaf6..73a6d275b7c 100644 --- a/skills/skill-creator/scripts/test_package_skill.py +++ b/skills/skill-creator/scripts/test_package_skill.py @@ -9,6 +9,7 @@ import types import zipfile from pathlib import Path from unittest import TestCase, main +from unittest.mock import patch SCRIPT_DIR = Path(__file__).resolve().parent if str(SCRIPT_DIR) not in sys.path: @@ -19,6 +20,7 @@ fake_quick_validate = types.ModuleType("quick_validate") fake_quick_validate.validate_skill = lambda _path: (True, "Skill is valid!") sys.modules["quick_validate"] = fake_quick_validate +import package_skill as package_skill_module from package_skill import package_skill @@ -54,7 +56,7 @@ class TestPackageSkillSecurity(TestCase): self.assertIn("normal-skill/SKILL.md", names) self.assertIn("normal-skill/script.py", names) - def test_rejects_symlink_to_external_file(self): + def test_skips_symlink_to_external_file(self): skill_dir = self.create_skill("symlink-file-skill") outside = self.temp_dir / "outside-secret.txt" outside.write_text("super-secret\n") @@ -68,9 +70,16 @@ class TestPackageSkillSecurity(TestCase): self.skipTest("symlink unsupported on this platform") result = package_skill(str(skill_dir), str(out_dir)) - self.assertIsNone(result) + self.assertIsNotNone(result) + skill_file = out_dir / "symlink-file-skill.skill" + self.assertTrue(skill_file.exists()) + with zipfile.ZipFile(skill_file, "r") as archive: + names = set(archive.namelist()) + self.assertIn("symlink-file-skill/SKILL.md", names) + self.assertIn("symlink-file-skill/script.py", names) + self.assertNotIn("symlink-file-skill/loot.txt", names) - def test_rejects_symlink_directory(self): + def test_skips_symlink_directory(self): skill_dir = self.create_skill("symlink-dir-skill") outside_dir = self.temp_dir / "outside" outside_dir.mkdir() @@ -85,6 +94,29 @@ class TestPackageSkillSecurity(TestCase): self.skipTest("symlink unsupported on this platform") result = package_skill(str(skill_dir), str(out_dir)) + self.assertIsNotNone(result) + skill_file = out_dir / "symlink-dir-skill.skill" + with zipfile.ZipFile(skill_file, "r") as archive: + names = set(archive.namelist()) + self.assertIn("symlink-dir-skill/SKILL.md", names) + self.assertIn("symlink-dir-skill/script.py", names) + self.assertNotIn("symlink-dir-skill/docs/secret.txt", names) + + def test_rejects_resolved_path_outside_skill_root(self): + skill_dir = self.create_skill("escape-skill") + out_dir = self.temp_dir / "out" + out_dir.mkdir() + + original_within = package_skill_module._is_within + + def fake_is_within(path_obj: Path, root: Path): + if path_obj.name == "script.py": + return False + return original_within(path_obj, root) + + with patch.object(package_skill_module, "_is_within", fake_is_within): + result = package_skill(str(skill_dir), str(out_dir)) + self.assertIsNone(result) def test_allows_nested_regular_files(self):