fix(plugins): reject plugin names that resolve to the plugins root
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 <yusufalweshdemir@gmail.com>
This commit is contained in:
@@ -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."
|
||||
)
|
||||
|
||||
@@ -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 ─────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
Reference in New Issue
Block a user