mirrors/hermes-agent
1
0
Fork 0
mirror of https://github.com/NousResearch/hermes-agent.git synced 2026-05-29 06:31:32 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 14

fix(skills): guard uninstall lock paths

Browse source 
Validate Skills Hub lock-file install paths at both ends of the
lifecycle so a poisoned or malformed lock.json entry cannot drive
shutil.rmtree to a location outside SKILLS_DIR:

- HubLockFile.record_install rejects empty/'.'/absolute/traversal/
  Windows-drive paths at write time, and requires the final path
  component to match the skill name (shape: '<skill>' or
  '<category>/<skill>').
- install_from_quarantine resolves its destination through the same
  validator, catching symlink/junction redirects inside skills/.
- uninstall_skill resolves the lock entry through the new validator
  before rmtree. Refuses anything that resolves to SKILLS_DIR itself
  (empty/dot paths) or to a target outside SKILLS_DIR (absolute paths,
  traversal, symlinked dirs in skills/ pointing outward).
- 14 focused regression tests covering each rejection class plus a
  symlink-redirect case.

E2E verified: hand-crafted poisoned lock.json entries (absolute path,
empty install_path, traversal) all refuse and leave the targeted
victim untouched; legitimate uninstall still succeeds.

Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com>
This commit is contained in:
峯岸 亮 2026-05-25 05:58:47 -07:00 • committed by Teknium
parent 0d137f1039
commit 3b9b9a7ad7
2 changed files with 311 additions and 11 deletions
Download patch file Download diff file Expand all files Collapse all files

220
tests/tools/test_skills_hub.py
View file

@ -1693,3 +1693,223 @@ class TestDownloadDirectoryRecursive:
assert "SKILL.md" in files
assert "scripts/run.py" not in files # lost due to rate limit
# ---------------------------------------------------------------------------
# Install-path safety (lock-file → uninstall rmtree boundary)
# ---------------------------------------------------------------------------
class TestInstallPathSafety:
"""Guard the lock-file → ``uninstall_skill`` rmtree path.
The destructive boundary is ``shutil.rmtree(SKILLS_DIR / install_path)``.
Lock-file ``install_path`` values that are absolute, contain ``..``,
point at the skills root itself, or are redirected via a symlink/junction
inside ``skills/`` must be rejected before they reach rmtree.
"""
@pytest.fixture
def isolated_skills_dir(self, tmp_path, monkeypatch):
skills_dir = tmp_path / "skills"
skills_dir.mkdir()
monkeypatch.setattr("tools.skills_hub.SKILLS_DIR", skills_dir)
return skills_dir
@pytest.fixture
def patch_lock_file(self, monkeypatch):
"""Redirect HubLockFile's default path to a test-controlled file.
HubLockFile.__init__ captures LOCK_FILE as a default arg at class
definition time, so monkeypatching the module-level LOCK_FILE doesn't
affect later HubLockFile() calls. Patch __defaults__ instead.
"""
def _apply(lock_path):
monkeypatch.setattr(HubLockFile.__init__, "__defaults__", (lock_path,))
return _apply
@pytest.mark.parametrize(
"bad_install_path",
[
"",
".",
"..",
"../../etc/passwd",
"/etc/passwd",
"skills/../../tmp",
"C:/Windows/System32",
],
)
def test_record_install_rejects_unsafe_paths(self, tmp_path, bad_install_path):
"""record_install must reject malformed install_path values at write time."""
lock = HubLockFile(path=tmp_path / "lock.json")
with pytest.raises(ValueError, match="Unsafe"):
lock.record_install(
name="evil",
source="github",
identifier="x",
trust_level="trusted",
scan_verdict="pass",
skill_hash="h1",
install_path=bad_install_path,
files=["SKILL.md"],
)
def test_record_install_rejects_mismatched_last_component(self, tmp_path):
"""The final component of install_path MUST equal the skill name."""
lock = HubLockFile(path=tmp_path / "lock.json")
with pytest.raises(ValueError, match="Unsafe install path"):
lock.record_install(
name="legit-skill",
source="github",
identifier="x",
trust_level="trusted",
scan_verdict="pass",
skill_hash="h1",
install_path="legit-skill/evil-suffix",
files=["SKILL.md"],
)
def test_record_install_accepts_bare_name(self, tmp_path):
lock = HubLockFile(path=tmp_path / "lock.json")
lock.record_install(
name="good", source="github", identifier="x",
trust_level="trusted", scan_verdict="pass",
skill_hash="h", install_path="good", files=["SKILL.md"],
)
assert lock.get_installed("good")["install_path"] == "good"
def test_record_install_accepts_category_and_name(self, tmp_path):
lock = HubLockFile(path=tmp_path / "lock.json")
lock.record_install(
name="good", source="github", identifier="x",
trust_level="trusted", scan_verdict="pass",
skill_hash="h", install_path="devops/good", files=["SKILL.md"],
)
assert lock.get_installed("good")["install_path"] == "devops/good"
def test_uninstall_rejects_poisoned_absolute_path(self, tmp_path, isolated_skills_dir, patch_lock_file):
"""Hand-edited lock.json with absolute install_path must not delete anything."""
from tools.skills_hub import uninstall_skill
lock_path = tmp_path / "lock.json"
target = tmp_path / "victim"
target.mkdir()
(target / "file.txt").write_text("important")
# Bypass record_install's validator to simulate a poisoned lock file.
lock_path.write_text(json.dumps({
"installed": {
"evil": {
"source": "github",
"identifier": "x",
"trust_level": "trusted",
"scan_verdict": "pass",
"content_hash": "h",
"install_path": str(target),
"files": [],
"metadata": {},
"installed_at": "now",
"updated_at": "now",
}
}
}))
patch_lock_file(lock_path)
ok, msg = uninstall_skill("evil")
assert ok is False
assert "Unsafe" in msg or "Refusing" in msg
assert target.exists()
assert (target / "file.txt").read_text() == "important"
def test_uninstall_rejects_traversal(self, tmp_path, isolated_skills_dir, patch_lock_file):
from tools.skills_hub import uninstall_skill
lock_path = tmp_path / "lock.json"
sibling = tmp_path / "sibling"
sibling.mkdir()
(sibling / "data").write_text("nope")
lock_path.write_text(json.dumps({
"installed": {
"evil": {
"source": "github", "identifier": "x",
"trust_level": "trusted", "scan_verdict": "pass",
"content_hash": "h",
"install_path": "../sibling",
"files": [], "metadata": {},
"installed_at": "now", "updated_at": "now",
}
}
}))
patch_lock_file(lock_path)
ok, msg = uninstall_skill("evil")
assert ok is False
assert sibling.exists()
assert (sibling / "data").read_text() == "nope"
def test_uninstall_rejects_empty_install_path(self, tmp_path, isolated_skills_dir, patch_lock_file):
"""Empty install_path resolves to SKILLS_DIR itself — must be refused."""
from tools.skills_hub import uninstall_skill
# Put a sibling skill alongside to prove rmtree doesn't fire.
(isolated_skills_dir / "bystander").mkdir()
(isolated_skills_dir / "bystander" / "SKILL.md").write_text("safe")
lock_path = tmp_path / "lock.json"
lock_path.write_text(json.dumps({
"installed": {
"evil": {
"source": "github", "identifier": "x",
"trust_level": "trusted", "scan_verdict": "pass",
"content_hash": "h",
"install_path": "",
"files": [], "metadata": {},
"installed_at": "now", "updated_at": "now",
}
}
}))
patch_lock_file(lock_path)
ok, msg = uninstall_skill("evil")
assert ok is False
assert (isolated_skills_dir / "bystander" / "SKILL.md").read_text() == "safe"
def test_uninstall_rejects_symlink_redirect_inside_skills(
self, tmp_path, isolated_skills_dir, patch_lock_file
):
"""A symlinked skill dir that points outside skills/ must not be followed."""
from tools.skills_hub import uninstall_skill
# Outside-tree victim
victim = tmp_path / "victim"
victim.mkdir()
(victim / "important").write_text("don't delete me")
# Symlink in skills/ pointing to the victim
link = isolated_skills_dir / "evil"
try:
link.symlink_to(victim, target_is_directory=True)
except (OSError, NotImplementedError):
pytest.skip("symlink creation unsupported on this platform")
lock_path = tmp_path / "lock.json"
lock_path.write_text(json.dumps({
"installed": {
"evil": {
"source": "github", "identifier": "x",
"trust_level": "trusted", "scan_verdict": "pass",
"content_hash": "h",
"install_path": "evil",
"files": [], "metadata": {},
"installed_at": "now", "updated_at": "now",
}
}
}))
patch_lock_file(lock_path)
ok, msg = uninstall_skill("evil")
assert ok is False
assert victim.exists()
assert (victim / "important").read_text() == "don't delete me"

102
tools/skills_hub.py
View file

@ -124,6 +124,69 @@ def _validate_category_name(category: str) -> str:
return _normalize_bundle_path(category, field_name="category", allow_nested=False)
def _normalize_lock_install_path(install_path: str, skill_name: str) -> str:
"""Validate a skill install path before it touches the lock file or disk.
Lock-file ``install_path`` entries are the source-of-truth for where
``uninstall_skill`` will call ``shutil.rmtree``. A poisoned or buggy
entry — empty string, ``"."``, an absolute path, ``../..`` traversal,
or anything whose final component doesn't match the skill name — would
let ``rmtree`` wipe either the entire ``skills/`` tree or content
outside it.
Enforce that ``install_path`` is exactly ``<skill_name>`` or
``<category>/<skill_name>``. Reject anything else.
"""
safe_skill_name = _validate_skill_name(skill_name)
normalized = _normalize_bundle_path(
install_path,
field_name="install path",
allow_nested=True,
)
parts = normalized.split("/")
if len(parts) not in {1, 2} or parts[-1] != safe_skill_name:
raise ValueError(f"Unsafe install path: {install_path}")
return normalized
def _is_path_redirect(path: Path) -> bool:
"""True when ``path`` is a symlink or (on Windows) a directory junction.
Either form lets an attacker who can write into the ``skills/`` tree
redirect a subsequent ``rmtree`` to content outside it. ``is_junction``
only exists on Python 3.12+ Windows; gate with ``hasattr``.
"""
return path.is_symlink() or (hasattr(path, "is_junction") and path.is_junction())
def _resolve_lock_install_path(install_path: str, skill_name: str) -> Path:
"""Resolve a lock-file install path without allowing escapes from ``SKILLS_DIR``.
Two layers of defence on top of the existing ``is_relative_to`` check
that's been on main:
1. Walk the path component-by-component and refuse if any intermediate
component is a symlink/junction (a path resolution that follows a
symlink to outside skills/ would otherwise be hidden by Path.resolve).
2. After resolve(), reject not just escape-out but also ``resolved == SKILLS_DIR``
— an empty/``"."``/``""`` install_path resolves to the skills root itself,
and ``rmtree(SKILLS_DIR)`` would wipe every installed skill.
"""
normalized = _normalize_lock_install_path(install_path, skill_name)
skills_root = SKILLS_DIR.resolve()
target = SKILLS_DIR
for part in normalized.split("/"):
target = target / part
if _is_path_redirect(target):
raise ValueError(f"Unsafe install path: {install_path}")
target = target.resolve()
if target == skills_root or not target.is_relative_to(skills_root):
raise ValueError(f"Unsafe install path: {install_path}")
return target
def _guarded_http_get(url: str, *, timeout: int = 20) -> Optional[httpx.Response]:
"""Fetch a URL with SSRF and redirect-target validation."""
current_url = url
@ -2788,14 +2851,20 @@ class HubLockFile:
files: List[str],
metadata: Optional[Dict[str, Any]] = None,
) -> None:
# Validate both the skill name and the install path SHAPE before
# writing into lock.json. A poisoned lock entry is the precondition
# for the uninstall_skill rmtree-escape; reject malformed input at
# write time so the file never carries the bad state.
safe_name = _validate_skill_name(name)
safe_install_path = _normalize_lock_install_path(install_path, safe_name)
data = self.load()
data["installed"][name] = {
data["installed"][safe_name] = {
"source": source,
"identifier": identifier,
"trust_level": trust_level,
"scan_verdict": scan_verdict,
"content_hash": skill_hash,
"install_path": install_path,
"install_path": safe_install_path,
"files": files,
"metadata": metadata or {},
"installed_at": datetime.now(timezone.utc).isoformat(),
@ -2943,9 +3012,14 @@ def install_from_quarantine(
raise ValueError(f"Unsafe quarantine path: {quarantine_path}")
if safe_category:
install_dir = SKILLS_DIR / safe_category / safe_skill_name
install_rel_path = f"{safe_category}/{safe_skill_name}"
else:
install_dir = SKILLS_DIR / safe_skill_name
install_rel_path = safe_skill_name
# Resolve via the same lock-path validator the uninstaller uses. Catches
# symlink-in-skills-tree redirects at install time so the lock entry's
# path can never refer to a redirected target.
install_dir = _resolve_lock_install_path(install_rel_path, safe_skill_name)
if install_dir.exists():
shutil.rmtree(install_dir)
@ -2999,14 +3073,20 @@ def uninstall_skill(skill_name: str) -> Tuple[bool, str]:
if not entry:
return False, f"'{skill_name}' is not a hub-installed skill (may be a builtin)"
install_path = SKILLS_DIR / entry["install_path"]
# Prevent path traversal from poisoned lock.json entries
# Validate the lock entry's install_path against the skill name. This is
# the destructive boundary — anything that falls through to the rmtree
# below MUST be inside SKILLS_DIR and MUST NOT be SKILLS_DIR itself
# (an empty/"."/"/" install_path would otherwise wipe the entire tree).
# _resolve_lock_install_path enforces shape (<skill_name> or
# <category>/<skill_name>), rejects absolute/traversal paths, and walks
# the path component-by-component refusing symlink/junction redirects.
try:
resolved = install_path.resolve()
if not resolved.is_relative_to(SKILLS_DIR.resolve()):
return False, f"Refusing to remove '{entry['install_path']}': resolves outside skills directory"
except (ValueError, OSError):
return False, f"Refusing to remove '{entry['install_path']}': path resolution failed"
install_path = _resolve_lock_install_path(
entry.get("install_path", ""), skill_name
)
except ValueError as exc:
return False, f"Refusing to uninstall '{skill_name}': {exc}"
if install_path.exists():
shutil.rmtree(install_path)