mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-20 10:11:58 +00:00
fix(skills): refuse SKILLS_DIR root in rmtree guard, not just outside-tree
The salvaged guard allowed _rmtree_writable(SKILLS_DIR) itself. No call site ever passes the root — every site passes a skill subdir or its .bak sibling — so allowing the root only preserves the #48200 footgun (a dest that collapses to the root wipes every installed skill). Require a strict strict-child relationship and update the test that documented the nonexistent 'full reset' capability.
This commit is contained in:
parent
f1254c8eaf
commit
25c590ccd0
2 changed files with 22 additions and 8 deletions
|
|
@ -237,16 +237,23 @@ class TestRmtreeWritableScopeGuard:
|
|||
with pytest.raises(ValueError, match="refusing to rmtree"):
|
||||
_rmtree_writable(not_skills)
|
||||
|
||||
def test_allows_skills_root_itself(self, tmp_path):
|
||||
"""The skills root directory IS allowed (used for full reset)."""
|
||||
def test_refuses_skills_root_itself(self, tmp_path):
|
||||
"""The skills root directory itself must be refused.
|
||||
|
||||
No caller in skills_sync.py ever passes SKILLS_DIR directly — every
|
||||
site passes a skill subdirectory or its ``.bak`` sibling. Removing
|
||||
the root would wipe every installed skill, and a ``dest`` that
|
||||
collapses to the root is exactly the degenerate path #48200 guards
|
||||
against. Require a strict-child relationship.
|
||||
"""
|
||||
from tools.skills_sync import _rmtree_writable
|
||||
|
||||
skills = tmp_path / "skills"
|
||||
skills.mkdir()
|
||||
(skills / "keep").mkdir(parents=True)
|
||||
with patch("tools.skills_sync.SKILLS_DIR", skills):
|
||||
# No exception — rmtree is allowed on the root itself.
|
||||
_rmtree_writable(skills)
|
||||
assert not skills.exists()
|
||||
with pytest.raises(ValueError, match="refusing to rmtree"):
|
||||
_rmtree_writable(skills)
|
||||
assert (skills / "keep").exists() # nothing was wiped
|
||||
|
||||
def test_allows_subdirectory_of_skills(self, tmp_path):
|
||||
"""Any directory strictly under SKILLS_DIR is allowed."""
|
||||
|
|
|
|||
|
|
@ -684,9 +684,16 @@ def _rmtree_writable(path: Path) -> None:
|
|||
# instead of silently destroying the user's install.
|
||||
target = Path(path).resolve()
|
||||
skills_root = SKILLS_DIR.resolve()
|
||||
if not (target == skills_root or skills_root in target.parents):
|
||||
# Every legitimate caller passes a skill directory or its ``.bak``
|
||||
# sibling — always a strict child of the skills root. The skills root
|
||||
# itself must never be removed: a ``dest`` that collapses to
|
||||
# ``SKILLS_DIR`` (e.g. a relative path resolving to ``.``) would wipe
|
||||
# every installed skill, and its ``.bak`` sibling lands one level up in
|
||||
# ``HERMES_HOME``. Require a strict-child relationship so both escape
|
||||
# into the skills root and out of it are refused.
|
||||
if skills_root not in target.parents:
|
||||
raise ValueError(
|
||||
f"refusing to rmtree {target!r}: not under {skills_root!r} "
|
||||
f"refusing to rmtree {target!r}: not strictly under {skills_root!r} "
|
||||
f"(scope guard — see #48200)"
|
||||
)
|
||||
import stat
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue