From 28561e6fc1e0431fdea272908ca8c89b56f92cf7 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 10:15:10 +0200 Subject: [PATCH] fix: reject symlinked skills-directory escape in extension skill scans _extension_owned_skill_names() and the fast/fallback paths of its sibling _unregister_extension_skills() called skills_candidate.resolve() and then checked children relative to that already-resolved candidate. If the candidate directory itself (e.g. .gemini/skills) was a symlink pointing outside the project root, both the resolve() call and the subsequent containment check silently passed through the symlink instead of rejecting it: - _extension_owned_skill_names() would falsely attribute ownership to a marker-matching SKILL.md living outside the project. - _unregister_extension_skills()'s fast path (an explicit skills_dir, as passed by the toggle-cleanup call site) and its fallback scan (used during full extension removal) would both shutil.rmtree() the external directory, deleting unrelated content outside the project. Fix by validating the candidate directory itself with the existing _validate_safe_shared_directory() shared-infra helper before any probe or delete: it rejects a symlink at any path component (walking down from the project root, including the final component) without ever resolving through it, and is already used elsewhere in the codebase for the same class of shared-directory containment check. Unsafe candidates are skipped/refused rather than followed. Add red-first security regression tests reproducing each of the three call sites with a `.gemini/skills` symlink pointing at an external directory containing a marker-matching SKILL.md and an unrelated precious_file.txt: provenance inference must not attribute the name, and both the explicit-skills_dir fast path and the None-skills_dir fallback scan must leave the external directory and file untouched. Existing valid shared/deduped directory tests (e.g. agy/amp/codex/zed sharing .agents/skills) continue to pass, confirming legitimate shared directories still clean up correctly. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/extensions/__init__.py | 38 ++++++ tests/test_extension_skills.py | 158 +++++++++++++++++++++++++ 2 files changed, 196 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 2d023fc4b..37d8e2dc3 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1262,10 +1262,26 @@ class ExtensionManager: if not skill_names: return + from ..shared_infra import _validate_safe_shared_directory + if skills_dir is None: skills_dir = self._get_skills_dir() if skills_dir: + # Reject the candidate directory itself (any path component, + # including the final one) if it's a symlink escaping the + # project root, before probing or deleting anything inside it. + # A caller-supplied skills_dir (e.g. a specific agent's + # directory resolved without side effects) could have been + # replaced with a symlink between registration and removal; + # resolving it and only checking children relative to the + # already-resolved candidate (the previous approach) would + # silently follow the symlink instead of rejecting it. + try: + _validate_safe_shared_directory(self.project_root, skills_dir) + except (ValueError, OSError): + return + # Fast path: we know the exact skills directory for skill_name in skill_names: # Guard against path traversal from a corrupted registry entry: @@ -1323,6 +1339,16 @@ class ExtensionManager: for skills_candidate in candidate_dirs: if not skills_candidate.is_dir(): continue + # Reject the candidate directory itself (any path + # component) if it's a symlink escaping the project + # root, before probing or deleting anything inside it — + # same guard as the fast path above. + try: + _validate_safe_shared_directory( + self.project_root, skills_candidate + ) + except (ValueError, OSError): + continue for skill_name in skill_names: # Same path-traversal guard as the fast path above sn_path = Path(skill_name) @@ -1397,6 +1423,7 @@ class ExtensionManager: return [] from .. import AGENT_CONFIG, DEFAULT_SKILLS_DIR + from ..shared_infra import _validate_safe_shared_directory candidate_dirs: set[Path] = set() for cfg in AGENT_CONFIG.values(): @@ -1412,6 +1439,17 @@ class ExtensionManager: break # every name already confirmed owned somewhere if not skills_candidate.is_dir(): continue + # Reject the candidate directory itself (any path component) + # if it's a symlink escaping the project root, before probing + # anything inside it. Resolving it and only checking children + # relative to the already-resolved candidate (the previous + # approach) would silently follow the symlink instead of + # rejecting it, letting a marker-matching SKILL.md outside the + # project be falsely attributed. + try: + _validate_safe_shared_directory(self.project_root, skills_candidate) + except (ValueError, OSError): + continue try: resolved_candidate = skills_candidate.resolve() except OSError: diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index eee68b8f3..17f9d8f1a 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -1448,6 +1448,164 @@ class TestExtensionSkillRegistration: "prematurely dropped during the earlier toggle (#2948)" ) + def test_extension_owned_skill_names_rejects_symlinked_candidate_directory( + self, project_dir, temp_dir + ): + """Provenance probing must not follow a symlinked candidate skills + directory that escapes the project root, even when a marker- + matching SKILL.md exists at the symlink target. + + Both ``_extension_owned_skill_names`` and its sibling + ``_unregister_extension_skills`` previously called + ``skills_candidate.resolve()`` and then checked children relative + to that *already-resolved* candidate — so if the candidate + directory itself (e.g. ``.gemini/skills``) was a symlink pointing + outside the project root, both the resolve and the subsequent + containment check silently passed *through* the symlink instead + of rejecting it. A marker-matching ``SKILL.md`` at the symlink + target would therefore be falsely attributed to the extension. + """ + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + external_dir = temp_dir / "external-skills-root" + external_dir.mkdir() + (external_dir / "precious_file.txt").write_text( + "do not touch", encoding="utf-8" + ) + external_skill_subdir = external_dir / "speckit-sym-escape-ext-hello" + external_skill_subdir.mkdir() + (external_skill_subdir / "SKILL.md").write_text( + "---\n" + "name: speckit-sym-escape-ext-hello\n" + "description: external marker-matching skill\n" + "metadata:\n" + " source: extension:sym-escape-ext\n" + "---\n\n" + "external body\n", + encoding="utf-8", + ) + + gemini_dir = project_dir / ".gemini" + gemini_dir.mkdir() + os.symlink(str(external_dir), str(gemini_dir / "skills")) + + manager = ExtensionManager(project_dir) + owned = manager._extension_owned_skill_names( + ["speckit-sym-escape-ext-hello"], "sym-escape-ext" + ) + + assert owned == [], ( + "a symlinked candidate skills directory escaping the project " + "root must never be followed for provenance attribution, " + "even when a marker-matching SKILL.md exists at its target" + ) + + def test_unregister_extension_skills_fallback_does_not_follow_symlinked_dir( + self, project_dir, temp_dir + ): + """Fallback removal scanning must not delete through a symlinked + candidate skills directory escaping the project root. + + Mirrors the previous test but exercises the actual deletion path: + before the fix, a symlinked ``.gemini/skills`` pointing outside + the project root would be resolved and scanned, and the + marker-matching external ``SKILL.md`` directory would be deleted + via ``shutil.rmtree`` — collateral damage to unrelated external + content (here, ``precious_file.txt`` sitting alongside it). + """ + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + external_dir = temp_dir / "external-skills-root2" + external_dir.mkdir() + precious_file = external_dir / "precious_file.txt" + precious_file.write_text("do not touch", encoding="utf-8") + external_skill_subdir = external_dir / "speckit-sym-escape-ext2-hello" + external_skill_subdir.mkdir() + external_skill_md = external_skill_subdir / "SKILL.md" + external_skill_md.write_text( + "---\n" + "name: speckit-sym-escape-ext2-hello\n" + "description: external marker-matching skill\n" + "metadata:\n" + " source: extension:sym-escape-ext2\n" + "---\n\n" + "external body\n", + encoding="utf-8", + ) + + gemini_dir = project_dir / ".gemini" + gemini_dir.mkdir() + os.symlink(str(external_dir), str(gemini_dir / "skills")) + + manager = ExtensionManager(project_dir) + # Exercise the fallback scan (skills_dir=None) exactly as a full + # `remove()` would invoke it. + manager._unregister_extension_skills( + ["speckit-sym-escape-ext2-hello"], "sym-escape-ext2" + ) + + assert precious_file.exists(), ( + "unrelated external content must survive: the fallback scan " + "must never delete through a symlinked candidate directory " + "escaping the project root" + ) + assert external_skill_md.exists(), ( + "the external marker-matching skill directory itself must " + "not be removed via a symlinked candidate path" + ) + + def test_unregister_extension_skills_fast_path_rejects_symlinked_explicit_dir( + self, project_dir, temp_dir + ): + """Explicit-skills_dir fast path must reject a symlinked directory + escaping the project root, mirroring the register-time call site + where a caller resolves a specific agent's directory without + side effects and passes it straight through. + """ + if not _can_create_symlink(temp_dir): + pytest.skip("Current platform/user cannot create symlinks") + + external_dir = temp_dir / "external-skills-root3" + external_dir.mkdir() + precious_file = external_dir / "precious_file.txt" + precious_file.write_text("do not touch", encoding="utf-8") + external_skill_subdir = external_dir / "speckit-sym-escape-ext3-hello" + external_skill_subdir.mkdir() + (external_skill_subdir / "SKILL.md").write_text( + "---\n" + "name: speckit-sym-escape-ext3-hello\n" + "description: external marker-matching skill\n" + "metadata:\n" + " source: extension:sym-escape-ext3\n" + "---\n\n" + "external body\n", + encoding="utf-8", + ) + + gemini_dir = project_dir / ".gemini" + gemini_dir.mkdir() + symlinked_skills_dir = gemini_dir / "skills" + os.symlink(str(external_dir), str(symlinked_skills_dir)) + + manager = ExtensionManager(project_dir) + manager._unregister_extension_skills( + ["speckit-sym-escape-ext3-hello"], + "sym-escape-ext3", + skills_dir=symlinked_skills_dir, + ) + + assert precious_file.exists(), ( + "unrelated external content must survive: the fast path must " + "refuse to delete through an explicit but symlinked skills_dir " + "escaping the project root" + ) + assert external_skill_subdir.exists(), ( + "the external marker-matching skill directory must not be " + "removed via an explicit symlinked directory argument" + ) + def test_existing_agent_command_path_file_is_not_detected( self, project_dir, temp_dir ):