fix(security): Harden skill activation and loading flows

Harden batch activation, dev refresh gating, Microsoft sync path handling, and Jetski skill loading against command injection, symlink traversal, and client-side star tampering. Add regression coverage for the security-sensitive paths and update the internal triage addendum for the Jetski loader fix.
2026-03-18 18:49:15 +01:00
parent 55033462ff
commit 4883b0dbb4
21 changed files with 410 additions and 96 deletions
--- a/tools/scripts/get-bundle-skills.py
+++ b/tools/scripts/get-bundle-skills.py
@@ -3,8 +3,29 @@ import sys
 import re
 from pathlib import Path

-def get_bundle_skills(bundle_queries):
-    bundles_path = Path(__file__).parent.parent.parent / "docs" / "users" / "bundles.md"
+SAFE_SKILL_ID_PATTERN = re.compile(r"^[A-Za-z0-9._-]+$")
+
+
+def is_safe_skill_id(skill_id):
+    return bool(SAFE_SKILL_ID_PATTERN.fullmatch(skill_id or ""))
+
+
+def filter_safe_skill_ids(skill_ids):
+    return [skill_id for skill_id in skill_ids if is_safe_skill_id(skill_id)]
+
+
+def format_skills_for_batch(skill_ids):
+    safe_skill_ids = filter_safe_skill_ids(skill_ids)
+    if not safe_skill_ids:
+        return ""
+    return "\n".join(safe_skill_ids) + "\n"
+
+
+def get_bundle_skills(bundle_queries, bundles_path=None):
+    if bundles_path is None:
+        bundles_path = Path(__file__).parent.parent.parent / "docs" / "users" / "bundles.md"
+    else:
+        bundles_path = Path(bundles_path)
    if not bundles_path.exists():
        print(f"Error: {bundles_path} not found", file=sys.stderr)
        return []
@@ -25,12 +46,13 @@ def get_bundle_skills(bundle_queries):
                found = True
                # Extract skill names from bullet points: - [`skill-name`](../../skills/skill-name/)
                skills = re.findall(r'- \[`([^`]+)`\]', section)
-                selected_skills.update(skills)
+                selected_skills.update(filter_safe_skill_ids(skills))
        
        if not found:
            # If query not found in any header, check if it's a skill name itself
            # (Just in case the user passed a skill name instead of a bundle)
-            selected_skills.add(query)
+            if is_safe_skill_id(query):
+                selected_skills.add(query)

    return sorted(list(selected_skills))

@@ -43,4 +65,4 @@ if __name__ == "__main__":
    
    skills = get_bundle_skills(queries)
    if skills:
-        print(" ".join(skills))
+        sys.stdout.write(format_skills_for_batch(skills))
--- a/tools/scripts/sync_microsoft_skills.py
+++ b/tools/scripts/sync_microsoft_skills.py
@@ -79,6 +79,17 @@ def is_path_within(base_dir: Path, target_path: Path) -> bool:
        return False


+def is_safe_regular_file(file_path: Path, source_root: Path) -> bool:
+    try:
+        if file_path.is_symlink():
+            return False
+        if not file_path.is_file():
+            return False
+        return is_path_within(source_root, file_path.resolve())
+    except OSError:
+        return False
+
+
 def sanitize_flat_name(candidate: str | None, fallback: str) -> str:
    """Accept only flat skill directory names; fall back on unsafe values."""
    if not candidate:
@@ -102,14 +113,9 @@ def sanitize_flat_name(candidate: str | None, fallback: str) -> str:
 def copy_safe_skill_files(source_dir: Path, target_dir: Path, source_root: Path):
    """Copy regular files only when their resolved path stays inside source_root."""
    for file_item in source_dir.iterdir():
-        if file_item.name == "SKILL.md" or file_item.is_symlink() or not file_item.is_file():
+        if file_item.name == "SKILL.md" or not is_safe_regular_file(file_item, source_root):
            continue
-
-        resolved = file_item.resolve()
-        if not is_path_within(source_root, resolved):
-            continue
-
-        shutil.copy2(resolved, target_dir / file_item.name)
+        shutil.copy2(file_item.resolve(), target_dir / file_item.name)

 def extract_skill_name(skill_md_path: Path) -> str | None:
    """Extract the 'name' field from SKILL.md YAML frontmatter using PyYAML."""
@@ -220,13 +226,17 @@ def find_github_skills(source_dir: Path, already_synced_names: set):
        return results

    for skill_dir in github_skills.iterdir():
-        if not skill_dir.is_dir() or not (skill_dir / "SKILL.md").exists():
+        if skill_dir.is_symlink() or not skill_dir.is_dir():
+            continue
+
+        skill_md = skill_dir / "SKILL.md"
+        if not is_safe_regular_file(skill_md, source_dir):
            continue

        if skill_dir.name not in already_synced_names:
            results.append({
                "relative_path": Path(".github/skills") / skill_dir.name,
-                "skill_md": skill_dir / "SKILL.md",
+                "skill_md": skill_md,
                "source_dir": skill_dir,
            })

--- a/tools/scripts/sync_recommended_skills.sh
+++ b/tools/scripts/sync_recommended_skills.sh
@@ -9,6 +9,16 @@ GITHUB_REPO="/Users/nicco/Antigravity Projects/antigravity-awesome-skills/skills
 LOCAL_LIBRARY="/Users/nicco/.gemini/antigravity/scratch/.agent/skills"
 BACKUP_DIR="/Users/nicco/.gemini/antigravity/scratch/.agent/skills_backup_$(date +%Y%m%d_%H%M%S)"

+remove_local_skill_dirs() {
+    find "$1" -mindepth 1 -maxdepth 1 -type d | while IFS= read -r item; do
+        if [ -L "$item" ]; then
+            echo "  ⚠️  Skipping symlinked directory: $(basename "$item")"
+            continue
+        fi
+        rm -rf -- "$item"
+    done
+}
+
 # 35 Recommended Skills
 RECOMMENDED_SKILLS=(
    # Tier S - Core Development (13)
@@ -76,10 +86,7 @@ echo ""

 # Clear local library (keep README.md if exists)
 echo "🗑️  Clearing local library..."
-cd "$LOCAL_LIBRARY"
-for item in */; do
-    rm -rf "$item"
-done
+remove_local_skill_dirs "$LOCAL_LIBRARY"
 echo "✅ Local library cleared"
 echo ""

--- a/tools/scripts/tests/activate_skills_batch_security.test.js
+++ b/tools/scripts/tests/activate_skills_batch_security.test.js
@@ -0,0 +1,20 @@
+const assert = require("assert");
+const fs = require("fs");
+const path = require("path");
+
+const repoRoot = path.resolve(__dirname, "../..", "..");
+const batchScript = fs.readFileSync(
+  path.join(repoRoot, "scripts", "activate-skills.bat"),
+  "utf8",
+);
+
+assert.doesNotMatch(
+  batchScript,
+  /for %%s in \(!ESSENTIALS!\) do \(/,
+  "activate-skills.bat must not iterate untrusted skills with tokenized FOR syntax",
+);
+assert.match(
+  batchScript,
+  /for \/f .*%%s in \("%SKILLS_LIST_FILE%"\) do \(/i,
+  "activate-skills.bat should read one validated skill id per line from the temp file",
+);
--- a/tools/scripts/tests/docs_security_content.test.js
+++ b/tools/scripts/tests/docs_security_content.test.js
@@ -8,6 +8,10 @@ const apifySkill = fs.readFileSync(
  path.join(repoRoot, 'skills', 'apify-actorization', 'SKILL.md'),
  'utf8',
 );
+const apifyCliReference = fs.readFileSync(
+  path.join(repoRoot, 'skills', 'apify-actorization', 'references', 'cli-actorization.md'),
+  'utf8',
+);
 const audioExample = fs.readFileSync(
  path.join(repoRoot, 'skills', 'audio-transcriber', 'examples', 'basic-transcription.sh'),
  'utf8',
@@ -165,6 +169,7 @@ assert.match(audioExample, /AUDIO_FILE_ENV/, 'audio example should pass shell va
 assert.strictEqual(/\|\s*(bash|sh)\b/.test(apifySkill), false, 'SKILL.md must not recommend pipe-to-shell installs');
 assert.strictEqual(/\|\s*iex\b/i.test(apifySkill), false, 'SKILL.md must not recommend PowerShell pipe-to-iex installs');
 assert.strictEqual(/apify login -t\b/.test(apifySkill), false, 'SKILL.md must not put tokens on the command line');
+assert.strictEqual(/\bcurl\b[\s\S]*?\|\s*(?:bash|sh)\b/i.test(apifyCliReference), false, 'cli reference must not recommend pipe-to-shell installs');

 function violationCount(list) {
  return list.length;
--- a/tools/scripts/tests/jetski_gemini_loader.test.js
+++ b/tools/scripts/tests/jetski_gemini_loader.test.js
@@ -105,6 +105,24 @@ async function main() {
        ]),
      /Skill path escapes skills root/,
    );
+
+    const symlinkedDir = path.join(fixtureRoot, "skills", "symlinked");
+    const outsideDir = path.join(fixtureRoot, "outside-symlink");
+    fs.mkdirSync(symlinkedDir, { recursive: true });
+    fs.mkdirSync(outsideDir, { recursive: true });
+    fs.writeFileSync(path.join(outsideDir, "secret.md"), "# secret\n", "utf8");
+    fs.symlinkSync(
+      path.join(outsideDir, "secret.md"),
+      path.join(symlinkedDir, "SKILL.md"),
+    );
+
+    await assert.rejects(
+      () =>
+        loadSkillBodies(fixtureRoot, [
+          { id: "symlinked", path: "skills/symlinked", name: "symlinked" },
+        ]),
+      /symlink|outside the skills root|regular file/i,
+    );
  } finally {
    fs.rmSync(fixtureRoot, { recursive: true, force: true });
  }
--- a/tools/scripts/tests/repo_hygiene_security.test.js
+++ b/tools/scripts/tests/repo_hygiene_security.test.js
@@ -19,4 +19,6 @@ assert.strictEqual(
  "tracked Python bytecode should not ship in skill directories",
 );
 assert.match(syncRecommended, /cp -RP/, "recommended skills sync should preserve symlinks instead of dereferencing them");
+assert.doesNotMatch(syncRecommended, /for item in \*\/; do\s+rm -rf "\$item"/, "recommended skills sync must not delete matched paths via naive glob iteration");
+assert.match(syncRecommended, /readlink|test -L|find .* -type d/, "recommended skills sync should explicitly avoid following directory symlinks during cleanup");
 assert.doesNotMatch(alphaVantage, /--- Unknown/, "alpha-vantage frontmatter should not contain malformed delimiters");
--- a/tools/scripts/tests/run-test-suite.js
+++ b/tools/scripts/tests/run-test-suite.js
@@ -8,12 +8,15 @@ const ENABLED_VALUES = new Set(["1", "true", "yes", "on"]);
 const TOOL_SCRIPTS = path.join("tools", "scripts");
 const TOOL_TESTS = path.join(TOOL_SCRIPTS, "tests");
 const LOCAL_TEST_COMMANDS = [
+  [path.join(TOOL_TESTS, "activate_skills_batch_security.test.js")],
  [path.join(TOOL_TESTS, "claude_plugin_marketplace.test.js")],
  [path.join(TOOL_TESTS, "jetski_gemini_loader.test.js")],
  [path.join(TOOL_TESTS, "npm_package_contents.test.js")],
  [path.join(TOOL_TESTS, "validate_skills_headings.test.js")],
  [path.join(TOOL_TESTS, "workflow_contracts.test.js")],
  [path.join(TOOL_TESTS, "docs_security_content.test.js")],
+  [path.join(TOOL_SCRIPTS, "run-python.js"), path.join(TOOL_TESTS, "test_bundle_activation_security.py")],
+  [path.join(TOOL_SCRIPTS, "run-python.js"), path.join(TOOL_TESTS, "test_sync_microsoft_skills_security.py")],
  [path.join(TOOL_SCRIPTS, "run-python.js"), path.join(TOOL_TESTS, "test_validate_skills_headings.py")],
 ];
 const NETWORK_TEST_COMMANDS = [
--- a/tools/scripts/tests/test_bundle_activation_security.py
+++ b/tools/scripts/tests/test_bundle_activation_security.py
@@ -0,0 +1,60 @@
+import importlib.util
+import pathlib
+import sys
+import tempfile
+import unittest
+
+
+REPO_ROOT = pathlib.Path(__file__).resolve().parents[3]
+TOOLS_SCRIPTS = REPO_ROOT / "tools" / "scripts"
+
+
+def load_module(module_path: pathlib.Path, module_name: str):
+    spec = importlib.util.spec_from_file_location(module_name, module_path)
+    module = importlib.util.module_from_spec(spec)
+    spec.loader.exec_module(module)
+    return module
+
+
+get_bundle_skills = load_module(
+    TOOLS_SCRIPTS / "get-bundle-skills.py",
+    "get_bundle_skills",
+)
+
+
+class BundleActivationSecurityTests(unittest.TestCase):
+    def test_format_skills_for_batch_emits_newline_delimited_safe_ids(self):
+        formatted = get_bundle_skills.format_skills_for_batch([
+            "safe-skill",
+            "nested.skill_2",
+            "unsafe&calc",
+            "another|bad",
+        ])
+
+        self.assertEqual(formatted, "safe-skill\nnested.skill_2\n")
+
+    def test_get_bundle_skills_rejects_unsafe_bundle_entries(self):
+        with tempfile.TemporaryDirectory() as temp_dir:
+            bundles_path = pathlib.Path(temp_dir) / "bundles.md"
+            bundles_path.write_text(
+                "\n".join(
+                    [
+                        "### Essentials",
+                        "- [`safe-skill`](../../skills/safe-skill/)",
+                        "- [`unsafe&calc`](../../skills/unsafe/)",
+                        "- [`safe_two`](../../skills/safe_two/)",
+                    ]
+                ),
+                encoding="utf-8",
+            )
+
+            skills = get_bundle_skills.get_bundle_skills(
+                ["Essentials"],
+                bundles_path=bundles_path,
+            )
+
+            self.assertEqual(skills, ["safe-skill", "safe_two"])
+
+
+if __name__ == "__main__":
+    unittest.main()
--- a/tools/scripts/tests/test_sync_microsoft_skills_security.py
+++ b/tools/scripts/tests/test_sync_microsoft_skills_security.py
@@ -41,6 +41,60 @@ class SyncMicrosoftSkillsSecurityTests(unittest.TestCase):
                    child.unlink()
                outside.rmdir()

+    def test_find_github_skills_ignores_symlinked_directories(self):
+        with tempfile.TemporaryDirectory() as temp_dir:
+            root = Path(temp_dir)
+            github_skills = root / ".github" / "skills"
+            github_skills.mkdir(parents=True)
+
+            safe_skill = github_skills / "safe-skill"
+            safe_skill.mkdir()
+            (safe_skill / "SKILL.md").write_text("---\nname: safe-skill\n---\n", encoding="utf-8")
+
+            outside = Path(tempfile.mkdtemp())
+            try:
+                escaped = outside / "escaped-skill"
+                escaped.mkdir()
+                (escaped / "SKILL.md").write_text("---\nname: escaped\n---\n", encoding="utf-8")
+                (github_skills / "escape").symlink_to(escaped, target_is_directory=True)
+
+                entries = sms.find_github_skills(root, set())
+                relative_paths = {str(entry["relative_path"]) for entry in entries}
+
+                self.assertEqual(relative_paths, {".github/skills/safe-skill"})
+            finally:
+                for child in escaped.iterdir():
+                    child.unlink()
+                escaped.rmdir()
+                outside.rmdir()
+
+    def test_find_github_skills_ignores_symlinked_skill_markdown(self):
+        with tempfile.TemporaryDirectory() as temp_dir:
+            root = Path(temp_dir)
+            github_skills = root / ".github" / "skills"
+            github_skills.mkdir(parents=True)
+
+            safe_skill = github_skills / "safe-skill"
+            safe_skill.mkdir()
+            (safe_skill / "SKILL.md").write_text("---\nname: safe-skill\n---\n", encoding="utf-8")
+
+            linked_skill = github_skills / "linked-skill"
+            linked_skill.mkdir()
+
+            outside = Path(tempfile.mkdtemp())
+            try:
+                target = outside / "SKILL.md"
+                target.write_text("---\nname: escaped\n---\n", encoding="utf-8")
+                (linked_skill / "SKILL.md").symlink_to(target)
+
+                entries = sms.find_github_skills(root, set())
+                relative_paths = {str(entry["relative_path"]) for entry in entries}
+
+                self.assertEqual(relative_paths, {".github/skills/safe-skill"})
+            finally:
+                target.unlink()
+                outside.rmdir()
+

 if __name__ == "__main__":
    unittest.main()