From db25a7852e67837c62a8d5b82b00403d8d5b9d06 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 19:59:43 -0700 Subject: [PATCH] fix(plugins): install refuses to ship an unreadable plugin tree (#111804) A clone can land unreadable (Windows ACL inheritance -> WinError 5, a mode-000 file). Discovery now skips such a dir instead of aborting (#112293), but the install that produced it still exited 0, so the user got a plugin that silently never loads. After the clone and before anything moves into place, walk the staged tree and open every file / list every dir. On failure repair u+rX where the OS honours mode bits; if still unreadable raise PluginOperationError naming the file and the fix (icacls / chmod). The staging dir is cleaned up, nothing is installed, exit is non-zero. Fixes #111804 (its discovery half landed in #112293). --- hermes_cli/plugins_cmd.py | 38 +++++++++++++++++++++++ tests/hermes_cli/test_plugins_cmd.py | 45 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 70262aa383..a93cc5a963 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -611,6 +611,43 @@ def _read_manifest_for_install(plugin_dir: Path) -> dict: return manifest +def _probe_readable(path: Path) -> None: + """Raise ``OSError`` unless *path* can actually be listed (dir) or opened for reading (file).""" + if path.is_dir(): + os.listdir(path) + else: + with open(path, "rb"): + pass + + +def _ensure_tree_readable(root: Path, plugins_dir: Path) -> None: + """Refuse to ship a tree Hermes cannot read back. A clone can land unreadable (Windows ACL + inheritance -> WinError 5, a mode-000 file) and discovery would then skip the plugin forever + (#111804); repair ``u+rX`` where the OS supports it, otherwise fail before anything moves.""" + paths = [root] + for dirpath, dirnames, filenames in os.walk(root): + paths.extend(Path(dirpath) / name for name in (*dirnames, *filenames)) + for path in paths: + try: + _probe_readable(path) + continue + except OSError: + if os.name != "nt": # chmod only toggles the read-only bit on Windows; ACLs need icacls + try: + os.chmod(path, os.stat(path).st_mode | (0o500 if path.is_dir() else 0o400)) + except OSError: + pass + try: + _probe_readable(path) + except OSError as exc: + fix = (f'icacls "{plugins_dir}" /grant:r "%USERNAME%":(OI)(CI)F /T' if os.name == "nt" + else f"chmod -R u+rX {plugins_dir}") + raise PluginOperationError( + f"Installed file {path.relative_to(root)} is not readable ({exc.strerror or exc}); " + f"nothing was installed. Fix permissions on {plugins_dir} (e.g. `{fix}`) and retry." + ) from exc + + def _swap_in_plugin(tmp_target: Path, target: Path, backup: Path, old_metadata: dict, new_metadata: dict) -> None: """Move the validated clone into place and persist metadata; on any failure restore the previous tree (if one was replaced) and the previous metadata sidecar, then re-raise.""" @@ -661,6 +698,7 @@ def _install_plugin_core( tmp_clone = Path(tmp) / "plugin" installed_revision = _clone_plugin_repo(tmp_clone, git_url, requested_revision) tmp_target = _resolve_subdir_within(tmp_clone, subdir) if subdir else tmp_clone + _ensure_tree_readable(tmp_target, plugins_dir) manifest = _read_manifest_for_install(tmp_target) plugin_name = manifest.get("name") or ( subdir.rstrip("/").rsplit("/", 1)[-1] if subdir else _repo_name_from_url(git_url)) diff --git a/tests/hermes_cli/test_plugins_cmd.py b/tests/hermes_cli/test_plugins_cmd.py index 8c1a303d92..d912075d00 100644 --- a/tests/hermes_cli/test_plugins_cmd.py +++ b/tests/hermes_cli/test_plugins_cmd.py @@ -798,6 +798,51 @@ class TestSubdirInstallE2E: assert pc._resolve_plugin_key("portable.test") == "portable.test" +class TestInstallReadabilityGate: + """A clone that lands unreadable is repaired or rolled back, never shipped (#111804).""" + + def _clone_with_unreadable_manifest(self, monkeypatch, pc): + real_chmod = os.chmod # the rollback test replaces os.chmod after this fixture runs + + def fake_clone(tmp_clone, git_url, revision): + tmp_clone.mkdir() + (tmp_clone / "plugin.yaml").write_text("name: badperm\nmanifest_version: 1\n", encoding="utf-8") + real_chmod(tmp_clone / "plugin.yaml", 0) + return "0" * 40 + + monkeypatch.setattr(pc, "_clone_plugin_repo", fake_clone) + monkeypatch.setattr(pc, "_scan_plugin_tree", lambda *a, **k: None) + + @pytest.mark.skipif(os.name == "nt" or os.geteuid() == 0, reason="POSIX mode bits, non-root") + def test_unreadable_file_is_repaired_before_install(self, tmp_path, monkeypatch): + from hermes_cli import plugins_cmd as pc + + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + monkeypatch.setattr(pc, "_plugins_dir", lambda: plugins_dir) + self._clone_with_unreadable_manifest(monkeypatch, pc) + + target, manifest, name = pc._install_plugin_core("file:///tmp/x", force=False) + + assert name == "badperm" # manifest read after repair, not the URL fallback + assert (target / "plugin.yaml").read_text(encoding="utf-8").startswith("name: badperm") + + @pytest.mark.skipif(os.name == "nt" or os.geteuid() == 0, reason="POSIX mode bits, non-root") + def test_unrepairable_tree_rolls_back_and_names_the_fix(self, tmp_path, monkeypatch): + from hermes_cli import plugins_cmd as pc + + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + monkeypatch.setattr(pc, "_plugins_dir", lambda: plugins_dir) + self._clone_with_unreadable_manifest(monkeypatch, pc) + monkeypatch.setattr(pc.os, "chmod", lambda *a, **k: (_ for _ in ()).throw(PermissionError(1, "nope"))) + + with pytest.raises(PluginOperationError, match=r"plugin.yaml is not readable.*chmod -R u\+rX"): + pc._install_plugin_core("file:///tmp/x", force=False) + + assert list(plugins_dir.iterdir()) == [] # no half-installed dir, no staging leftovers + + def test_portable_manifest_is_visible_to_plugin_cli(tmp_path): import json