From f1254c8eafa453142abd04158e46d6e081e3c63a Mon Sep 17 00:00:00 2001 From: Kewe63 Date: Thu, 18 Jun 2026 11:55:01 +0300 Subject: [PATCH] fix(skills): rmtree scope guard + default pre_update_backup to true (#48200) Defense-in-depth fix for the silent wipe of ~/.hermes/ documented in #48200. A `hermes update --yes` run silently destroyed a user's .env, MEMORY.md, kanban.db, custom skills, and scripts. Two changes: 1. `_rmtree_writable` in tools/skills_sync.py now refuses to rmtree anything outside SKILLS_DIR (the HERMES_HOME/skills/ root). All five call sites pass paths under SKILLS_DIR, so the guard is a no-op for current code and a loud, recoverable failure for any future regression (bad path join, malicious bundled manifest, stale path in scope after an exception). 2. The default `updates.pre_update_backup` flips from false to true in hermes_cli/config.py. A few minutes of zip per update is negligible compared to silent total data loss. Still overridable; --no-backup still works for one-off opt-out. Five new tests in TestRmtreeWritableScopeGuard (root path, hermes home, sibling dir, skills root itself, subdir) plus a flipped `test_default_enabled_creates_backup` in test_backup.py. 178/178 tests pass in the two affected files. Public method signatures unchanged, no test-stub blast radius. Closes #48200 --- hermes_cli/config.py | 13 +++-- hermes_cli/main.py | 8 +++- tests/hermes_cli/test_backup.py | 19 +++++--- tests/tools/test_skills_sync.py | 85 +++++++++++++++++++++++++++++++++ tools/skills_sync.py | 18 +++++++ 5 files changed, 130 insertions(+), 13 deletions(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 30d4b07384d..a9e8248e946 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2496,11 +2496,14 @@ DEFAULT_CONFIG = { "updates": { # Run a full ``hermes backup``-style zip of HERMES_HOME before every # ``hermes update``. Backups land in ``/backups/`` and - # can be restored with ``hermes import ``. Off by default — - # on large HERMES_HOME directories the zip can add minutes to every - # update. Set to true to re-enable, or pass ``--backup`` to opt in - # for a single update run. - "pre_update_backup": False, + # can be restored with ``hermes import ``. Defaults to true + # after the #48200 incident: a ``hermes update --yes`` run that + # computed a wrong path silently wiped the user's ``.env``, + # ``MEMORY.md``, ``kanban.db``, custom skills, and scripts in one + # go. The cost of a few minutes of zip time per update is + # negligible compared to the alternative. Set to false to opt + # out, or pass ``--no-backup`` for a single update run. + "pre_update_backup": True, # How many pre-update backup zips to retain. Older ones are pruned # automatically after each successful backup. Values below 1 are # floored to 1 — the backup just created is always preserved. To diff --git a/hermes_cli/main.py b/hermes_cli/main.py index c69cd60b42a..4508642d0cb 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8118,7 +8118,13 @@ def _run_pre_update_backup(args) -> None: cfg = {} updates_cfg = cfg.get("updates", {}) if isinstance(cfg, dict) else {} - enabled = updates_cfg.get("pre_update_backup", False) + # The default config ships with ``pre_update_backup: true`` (see + # ``hermes_cli/config.py``). Fall back to true if the key is missing + # (e.g. a user has an older custom config without the field). The + # ``False`` default from before #48200 caused silent data loss when + # an update step computed a wrong path — the cost of a few minutes + # of zip time per update is negligible compared to the alternative. + enabled = updates_cfg.get("pre_update_backup", True) keep = updates_cfg.get("backup_keep", 5) if not enabled and not force_backup: diff --git a/tests/hermes_cli/test_backup.py b/tests/hermes_cli/test_backup.py index cf98655e449..762af37069c 100644 --- a/tests/hermes_cli/test_backup.py +++ b/tests/hermes_cli/test_backup.py @@ -1726,16 +1726,21 @@ class TestRunPreUpdateBackup: backups = list((hermes_home / "backups").glob("pre-update-*.zip")) assert len(backups) == 1 - def test_default_disabled_is_silent(self, hermes_home, capsys): - """With the default-off config and no --backup flag, the hook is silent - and creates no backup. This is the common case for every update.""" + def test_default_enabled_creates_backup(self, hermes_home, capsys): + """With the new safe default (``pre_update_backup: true``), every + ``hermes update`` creates a backup before any destructive step + runs — the cost is a few minutes of zip time vs. the alternative + of silent total data loss of ``~/.hermes/`` observed in #48200 + when an update step computes a wrong path and the user had no + safety net. + """ from hermes_cli.main import _run_pre_update_backup _run_pre_update_backup(Namespace(no_backup=False, backup=False)) out = capsys.readouterr().out - assert out == "" - assert not (hermes_home / "backups").exists() or not list( - (hermes_home / "backups").glob("pre-update-*.zip") - ) + assert "Creating pre-update backup" in out + assert "Saved:" in out + backups = list((hermes_home / "backups").glob("pre-update-*.zip")) + assert len(backups) == 1 def test_no_backup_flag_skips(self, hermes_home, capsys): from hermes_cli.main import _run_pre_update_backup diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 6be3e3705e8..88e8b20940a 100644 --- a/tests/tools/test_skills_sync.py +++ b/tests/tools/test_skills_sync.py @@ -2,6 +2,7 @@ import shutil import json +import pytest from pathlib import Path from unittest.mock import patch @@ -180,6 +181,90 @@ class TestComputeRelativeDest: assert dest.name == "simple" +class TestRmtreeWritableScopeGuard: + """``_rmtree_writable`` must refuse to remove anything outside + ``HERMES_HOME/skills/``. + + The previous implementation called ``shutil.rmtree(path)`` on whatever + argument the caller passed. If any of the five call sites in + ``tools/skills_sync.py`` ever computes a path outside the skills + root — through a bad join, a missing default, a malicious + bundled-manifest entry, or a stale path in scope after an + exception — the result is a silent ``shutil.rmtree(~/.hermes/)`` + that destroys the user's ``.env``, ``MEMORY.md``, ``kanban.db``, + custom skills, scripts, and the rest of the install in one go + (#48200). + + The scope guard turns that into a loud ``ValueError`` so the + failure is observable, reproducible, and recoverable rather than + a data-loss incident. + """ + + def test_refuses_root_path(self, tmp_path): + """``Path('/')`` is the entire filesystem — must always be rejected.""" + from tools.skills_sync import _rmtree_writable, SKILLS_DIR + + skills = tmp_path / "skills" + skills.mkdir() + with patch("tools.skills_sync.SKILLS_DIR", skills): + with pytest.raises(ValueError, match="refusing to rmtree"): + _rmtree_writable(Path("/")) + + def test_refuses_hermes_home_itself(self, tmp_path): + """``~/.hermes/`` itself is what the #48200 wipe destroyed.""" + from tools.skills_sync import _rmtree_writable + + hermes = tmp_path / "home" + hermes.mkdir() + (hermes / "skills").mkdir() + with patch("tools.skills_sync.SKILLS_DIR", hermes / "skills"): + with pytest.raises(ValueError, match="refusing to rmtree"): + _rmtree_writable(hermes) + + def test_refuses_sibling_directory(self, tmp_path): + """A directory that is a sibling of SKILLS_DIR (e.g. a wrong + ``bundled_dir`` computation) must be rejected, not silently rmtree'd. + """ + from tools.skills_sync import _rmtree_writable + + hermes = tmp_path / "home" + hermes.mkdir() + skills = hermes / "skills" + skills.mkdir() + not_skills = hermes / "kanban.db" # any non-skills path + not_skills.mkdir() + with patch("tools.skills_sync.SKILLS_DIR", skills): + 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).""" + from tools.skills_sync import _rmtree_writable + + skills = tmp_path / "skills" + skills.mkdir() + with patch("tools.skills_sync.SKILLS_DIR", skills): + # No exception — rmtree is allowed on the root itself. + _rmtree_writable(skills) + assert not skills.exists() + + def test_allows_subdirectory_of_skills(self, tmp_path): + """Any directory strictly under SKILLS_DIR is allowed.""" + from tools.skills_sync import _rmtree_writable + + skills = tmp_path / "skills" + skills.mkdir() + sub = skills / "category" / "old-skill" + sub.mkdir(parents=True) + (sub / "SKILL.md").write_text("# old") + + with patch("tools.skills_sync.SKILLS_DIR", skills): + _rmtree_writable(sub) + + assert skills.exists() + assert not sub.exists() + + class TestSyncSkills: def _setup_bundled(self, tmp_path): """Create a fake bundled skills directory.""" diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 091c56e2d4e..da3c684c509 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -671,6 +671,24 @@ def _rmtree_writable(path: Path) -> None: parent directory, so the retry handler makes the failing path **and its parent** writable before re-attempting. See #34860, #34972. """ + # Defense in depth (#48200): refuse to rmtree anything outside + # ``HERMES_HOME/skills/`` to prevent the catastrophic wipe of + # ``~/.hermes/`` (``.env``, ``MEMORY.md``, ``kanban.db``, custom + # skills, scripts, …) that an earlier incident observed. Five call + # sites in this file invoke this helper; if any one of them ever + # computes a destination outside the skills root — through a bad + # path join, a missing ``HERMES_HOME`` default, a malicious + # bundled-manifest entry, or a mid-flight exception that leaves a + # stale path in scope — this guard turns the resulting + # ``shutil.rmtree(~/.hermes)`` into a loud, recoverable ``ValueError`` + # 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): + raise ValueError( + f"refusing to rmtree {target!r}: not under {skills_root!r} " + f"(scope guard — see #48200)" + ) import stat def _on_error(func, fpath, exc_info):