From e9ddfee4fd8964a34493e896c44286f7209bc3d0 Mon Sep 17 00:00:00 2001 From: Dusk1e Date: Sun, 5 Apr 2026 18:25:32 -0700 Subject: [PATCH] fix(plugins): reject plugin names that resolve to the plugins root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reject "." as a plugin name — it resolves to the plugins directory itself, which in force-install flows causes shutil.rmtree to wipe the entire plugins tree. - reject "." early with a clear error message - explicit check for target == plugins_resolved (raise instead of allow) - switch boundary check from string-prefix to Path.relative_to() - add regression tests for sanitizer + install flow Co-authored-by: Dusk1e --- hermes_cli/plugins_cmd.py | 17 +++++++++++++---- tests/test_plugins_cmd.py | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index c3717bfa3..68a31544c 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -41,6 +41,11 @@ def _sanitize_plugin_name(name: str, plugins_dir: Path) -> Path: if not name: raise ValueError("Plugin name must not be empty.") + if name in (".", ".."): + raise ValueError( + f"Invalid plugin name '{name}': must not reference the plugins directory itself." + ) + # Reject obvious traversal characters for bad in ("/", "\\", ".."): if bad in name: @@ -49,10 +54,14 @@ def _sanitize_plugin_name(name: str, plugins_dir: Path) -> Path: target = (plugins_dir / name).resolve() plugins_resolved = plugins_dir.resolve() - if ( - not str(target).startswith(str(plugins_resolved) + os.sep) - and target != plugins_resolved - ): + if target == plugins_resolved: + raise ValueError( + f"Invalid plugin name '{name}': resolves to the plugins directory itself." + ) + + try: + target.relative_to(plugins_resolved) + except ValueError: raise ValueError( f"Invalid plugin name '{name}': resolves outside the plugins directory." ) diff --git a/tests/test_plugins_cmd.py b/tests/test_plugins_cmd.py index ac95571be..492f94ad0 100644 --- a/tests/test_plugins_cmd.py +++ b/tests/test_plugins_cmd.py @@ -40,9 +40,13 @@ class TestSanitizePluginName: _sanitize_plugin_name("../../etc/passwd", tmp_path) def test_rejects_single_dot_dot(self, tmp_path): - with pytest.raises(ValueError, match="must not contain"): + with pytest.raises(ValueError, match="must not reference the plugins directory itself"): _sanitize_plugin_name("..", tmp_path) + def test_rejects_single_dot(self, tmp_path): + with pytest.raises(ValueError, match="must not reference the plugins directory itself"): + _sanitize_plugin_name(".", tmp_path) + def test_rejects_forward_slash(self, tmp_path): with pytest.raises(ValueError, match="must not contain"): _sanitize_plugin_name("foo/bar", tmp_path) @@ -228,6 +232,38 @@ class TestCmdInstall: cmd_install("invalid") assert exc_info.value.code == 1 + @patch("hermes_cli.plugins_cmd._display_after_install") + @patch("hermes_cli.plugins_cmd.shutil.move") + @patch("hermes_cli.plugins_cmd.shutil.rmtree") + @patch("hermes_cli.plugins_cmd._plugins_dir") + @patch("hermes_cli.plugins_cmd._read_manifest") + @patch("hermes_cli.plugins_cmd.subprocess.run") + def test_install_rejects_manifest_name_pointing_at_plugins_root( + self, + mock_run, + mock_read_manifest, + mock_plugins_dir, + mock_rmtree, + mock_move, + mock_display_after_install, + tmp_path, + ): + from hermes_cli.plugins_cmd import cmd_install + + plugins_dir = tmp_path / "plugins" + plugins_dir.mkdir() + mock_plugins_dir.return_value = plugins_dir + mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + mock_read_manifest.return_value = {"name": "."} + + with pytest.raises(SystemExit) as exc_info: + cmd_install("owner/repo", force=True) + + assert exc_info.value.code == 1 + assert plugins_dir not in [call.args[0] for call in mock_rmtree.call_args_list] + mock_move.assert_not_called() + mock_display_after_install.assert_not_called() + # ── cmd_update tests ─────────────────────────────────────────────────────────