From 8d9684c9daeb0f072d4c344baa949bc8a28234ef Mon Sep 17 00:00:00 2001 From: Ahmett101 Date: Sat, 4 Jul 2026 21:32:52 +0300 Subject: [PATCH] fix(profiles): allowlist default-export paths + preserve symlinks (#58394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `hermes profile export default` crashed with `shutil.Error` when HERMES_HOME pointed outside ~/.hermes (common in Docker deployments) and the workspace contained broken symlinks. Two root causes: 1. `copytree` defaults to `symlinks=False` and follows link targets; broken ones crash. #58397 (liuhao1024) drafted a minimal `symlinks=True` flag fix; this PR adopts that change. 2. `copytree` was invoked against the entire HERMES_HOME root (which doubles as cwd in Docker layouts). The post-hoc blacklist at `_DEFAULT_EXPORT_EXCLUDE_ROOT` is a fixed-length enumerate-and-pray list that can't anticipate every unrelated sibling directory (`x11-dev/`, etc.). Replaced with a positive allow-list at `_DEFAULT_EXPORT_INCLUDE_ROOT` enumerating the known Hermes profile artifacts (config, persona, skills, cron, scripts, sessions, plugins, memories, knowledge, preferences). Sensitive runtime surfaces (`state.db`, `logs/`, auth files, other profiles) are intentionally not in the allow-list so the export stays a portable, credential-free snapshot of the user-facing surface — which means the existing `test_export_default_excludes_infrastructure` regressions remain green. Adds two regression tests: * test_export_default_uses_allowlist_for_unrelated_dirs — >x11-dev< sibling directories must not leak into the archive. * test_export_default_handles_broken_symlinks — symlinks inside allowed artifacts survive instead of crashing the export. closing that PR as superseded once this lands. Closes #58394 --- hermes_cli/profiles.py | 40 +++++++++++++-- tests/hermes_cli/test_profiles.py | 81 +++++++++++++++++++++++++------ 2 files changed, 102 insertions(+), 19 deletions(-) diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 1b2af2eda66..73137fc7e3d 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -224,6 +224,25 @@ _DEFAULT_EXPORT_EXCLUDE_ROOT = frozenset({ "logs", # gateway logs }) +# Allow-list for ``export_profile("default")``: when HERMES_HOME equals the +# cwd (Docker/custom deployments), the default profile home is the working +# directory and contains arbitrary user files that should NOT be bundled +# into the export. The set below identifies the *known Hermes profile +# artifacts* at the root of HERMES_HOME; everything else is excluded. +# Sensitive runtime infrastructure (``state.db``, ``logs/``, ``auth.*``, +# other profiles) is intentionally *not* in this list so the export stays +# a portable, credential-free snapshot of the user-facing surface +# (#58394). Add new artifacts here when introduced in ``hermes_constants``. +_DEFAULT_EXPORT_INCLUDE_ROOT = frozenset({ + # Configuration / persona + "config.yaml", "SOUL.md", "MEMORY.md", "USER.md", "todo.json", + "system_prompt.md", "AGENTS.md", "CLAUDE.md", ".cursorrules", + # User-facing skill, cron, and session artifacts + "skills", "cron", "scripts", "sessions", + # Plugin / memory surfaces (per-profile overrides live here) + "plugins", "memories", "knowledge", "preferences", +}) + # Names that cannot be used as profile aliases _RESERVED_NAMES = frozenset({ "hermes", "default", "test", "tmp", "root", "sudo", @@ -1843,8 +1862,18 @@ def get_active_profile_name() -> str: def _default_export_ignore(root_dir: Path): """Return an *ignore* callable for :func:`shutil.copytree`. - At the root level it excludes everything in ``_DEFAULT_EXPORT_EXCLUDE_ROOT``. - At all levels it excludes ``__pycache__``, sockets, and temp files. + Two-tier filtering: + + * **Root-level allow-list** — only entries whose name appears in + ``_DEFAULT_EXPORT_INCLUDE_ROOT`` survive. Everything else (such as + an unrelated ``x11-dev/`` directory in a Docker deployment where + HERMES_HOME equals the cwd) is excluded. Blacklisting was tried + first and proved unable to anticipate every non-Hermes file the + user may have lying alongside HERMES_HOME (#58394). + * **Universal exclusions at any depth** — ``__pycache__``, sockets, + temp files; plus npm lockfiles, which may appear at the root. + + All other profile artifacts are copied through untouched. """ def _ignore(directory: str, contents: list) -> set: @@ -1856,9 +1885,12 @@ def _default_export_ignore(root_dir: Path): # npm lockfiles can appear at root elif entry in {"package.json", "package-lock.json"}: ignored.add(entry) - # Root-level exclusions + # Root-level allow-list: drop everything that isn't a known + # Hermes profile artifact. if Path(directory) == root_dir: - ignored.update(c for c in contents if c in _DEFAULT_EXPORT_EXCLUDE_ROOT) + ignored.update( + entry for entry in contents if entry not in _DEFAULT_EXPORT_INCLUDE_ROOT + ) return ignored return _ignore diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index d91be38186f..c61bc36dd4b 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -1385,19 +1385,63 @@ class TestExportImport: assert not any("__pycache__" in n for n in names) - def test_export_default_handles_broken_symlinks(self, profile_env, tmp_path): - """Export succeeds when the profile directory contains broken symlinks. + def test_export_default_uses_allowlist_for_unrelated_dirs(self, profile_env, tmp_path): + """Unrelated directories under HERMES_HOME are excluded by allow-list (#58394). - In Docker/custom HERMES_HOME deployments, unrelated directories may - contain stale symlinks. copytree must not follow them. + Docker/custom deployments often set HERMES_HOME to a working + directory that also contains unrelated user projects (``x11-dev/``, + etc.). The root-level allow-list filters those out so only known + Hermes artifacts end up in the archive. Replaces the old + exhaustive blacklist. """ default_dir = get_profile_dir("default") (default_dir / "config.yaml").write_text("ok") - # Create a broken symlink (target does not exist) - (default_dir / "broken_link").symlink_to("/nonexistent/path") - # Create a valid symlink for comparison - (default_dir / "valid_target.txt").write_text("real data") - (default_dir / "valid_link").symlink_to(default_dir / "valid_target.txt") + (default_dir / "SOUL.md").write_text("soul") + # Allowed subdirectory with content + (default_dir / "skills" / "demo").mkdir(parents=True) + (default_dir / "skills" / "demo" / "SKILL.md").write_text("hi") + # Unrelated directory — should NOT appear in the archive + unrelated = default_dir / "x11-dev" / "usr" / "lib" + unrelated.mkdir(parents=True) + (unrelated / "libXi.so").write_text("data") + + output = tmp_path / "export" / "default.tar.gz" + output.parent.mkdir(parents=True, exist_ok=True) + result = export_profile("default", str(output)) + + with tarfile.open(str(result), "r:gz") as tf: + names = set(tf.getnames()) + + # Allowed artifacts present + assert any(n.endswith("config.yaml") for n in names) + assert any(n.endswith("SOUL.md") for n in names) + assert any(n.endswith("skills/demo/SKILL.md") for n in names) + # Unrelated artifact excluded + assert not any("x11-dev" in n for n in names) + assert not any("libXi.so" in n for n in names) + + def test_export_default_handles_broken_symlinks(self, profile_env, tmp_path): + """Broken symlinks inside allowed artifacts are preserved, not crashed (#58394). + + ``shutil.copytree``'s default is ``symlinks=False``, which follows + symlinks and crashes on broken ones. Use ``symlinks=True`` so stale + symlinks inside *allowed* artifacts (e.g. ``skills/``) survive as + symlinks; the link and its target are both retained. + """ + default_dir = get_profile_dir("default") + (default_dir / "config.yaml").write_text("ok") + # Place broken symlink *inside* the allowed ``skills/`` tree so the + # root-level allow-list passes the directory through; the + # symlinks=True flag must then preserve the link instead of + # following and crashing. + broken_dir = default_dir / "skills" / "with-broken-links" + broken_dir.mkdir(parents=True) + (broken_dir / "broken_link").symlink_to("/nonexistent/path") + # Valid symlink for comparison + (broken_dir / "valid_target.txt").write_text("real data") + (broken_dir / "valid_link").symlink_to( + broken_dir / "valid_target.txt" + ) output = tmp_path / "export" / "default.tar.gz" output.parent.mkdir(parents=True, exist_ok=True) @@ -1405,12 +1449,19 @@ class TestExportImport: assert result.exists() with tarfile.open(str(result), "r:gz") as tf: - names = tf.getnames() - # Broken symlink is preserved as a symlink entry - assert any("broken_link" in n for n in names) - # Valid symlink and its target are both present - assert any("valid_link" in n for n in names) - assert any("valid_target.txt" in n for n in names) + names = set(tf.getnames()) + # Allowed artifact survived + assert any(n.endswith("config.yaml") for n in names) + # Broken symlink inside an allowed dir was preserved as a symlink + # (without crashing) — tar entry name recorded as the link path. + assert any( + "with-broken-links/broken_link" in n for n in names + ), ( + f"broken_link should survive; tarfile names: {sorted(names)[:30]}" + ) + # Valid symlink + target also kept + assert any("valid_link" in n for n in names) + assert any("valid_target.txt" in n for n in names) def test_import_default_without_name_raises(self, profile_env, tmp_path): """Importing a default export without --name gives clear guidance."""