fix(skill-creator): harden skill packaging path handling (#24260)

* fix(skill-creator): skip symlinks during skill packaging

* test(skill-creator): cover symlink skipping and root-escape guard

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}")
 

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):