mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
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>
This commit is contained in:
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user