fix(plugins): widen _sanitize_plugin_name for category-namespaced names
Follow-up to PR #28832 — the dashboard plugin routes now accept slashed names like `observability/langfuse` and `image_gen/openai`, but `_sanitize_plugin_name` still rejected forward slash and so dashboard update + remove on those plugins fell through to '404 not found' even though they exist on disk. Adds an opt-in `allow_subdir=True` flag that: - Permits internal forward slashes (category-namespaced plugin keys emitted by `_discover_all_plugins`). - Strips leading and trailing slashes. - Still rejects `..` and backslash, and still asserts the resolved target lives inside `plugins_dir`. Opted in at the two read-paths that operate on installed plugins: `_require_installed_plugin` (CLI update/remove) and `_user_installed_plugin_dir` (dashboard update/remove). The install path keeps the default (`allow_subdir=False`) because freshly-cloned plugins always land top-level under `~/.hermes/plugins/<name>/`. Adds 6 targeted unit tests covering the new flag's allow/reject matrix.
This commit is contained in:
@@ -76,22 +76,42 @@ def _plugins_dir() -> Path:
|
|||||||
return plugins
|
return plugins
|
||||||
|
|
||||||
|
|
||||||
def _sanitize_plugin_name(name: str, plugins_dir: Path) -> Path:
|
def _sanitize_plugin_name(
|
||||||
|
name: str,
|
||||||
|
plugins_dir: Path,
|
||||||
|
*,
|
||||||
|
allow_subdir: bool = False,
|
||||||
|
) -> Path:
|
||||||
"""Validate a plugin name and return the safe target path inside *plugins_dir*.
|
"""Validate a plugin name and return the safe target path inside *plugins_dir*.
|
||||||
|
|
||||||
Raises ``ValueError`` if the name contains path-traversal sequences or would
|
Raises ``ValueError`` if the name contains path-traversal sequences or would
|
||||||
resolve outside the plugins directory.
|
resolve outside the plugins directory.
|
||||||
|
|
||||||
|
``allow_subdir=True`` permits a single forward slash inside *name* so
|
||||||
|
category-namespaced plugin keys like ``observability/langfuse`` or
|
||||||
|
``image_gen/openai`` (the registry keys emitted by ``_discover_all_plugins``)
|
||||||
|
can be looked up. ``..`` and backslash are still rejected, leading and
|
||||||
|
trailing slashes are stripped, and the resolved target must still live
|
||||||
|
inside *plugins_dir*. Install paths leave this at the default ``False``
|
||||||
|
because a freshly-cloned plugin always lands top-level under
|
||||||
|
``~/.hermes/plugins/<name>/``.
|
||||||
"""
|
"""
|
||||||
if not name:
|
if not name:
|
||||||
raise ValueError("Plugin name must not be empty.")
|
raise ValueError("Plugin name must not be empty.")
|
||||||
|
|
||||||
|
if allow_subdir:
|
||||||
|
name = name.strip("/")
|
||||||
|
if not name:
|
||||||
|
raise ValueError("Plugin name must not be empty.")
|
||||||
|
|
||||||
if name in {".", ".."}:
|
if name in {".", ".."}:
|
||||||
raise ValueError(
|
raise ValueError(
|
||||||
f"Invalid plugin name '{name}': must not reference the plugins directory itself."
|
f"Invalid plugin name '{name}': must not reference the plugins directory itself."
|
||||||
)
|
)
|
||||||
|
|
||||||
# Reject obvious traversal characters
|
# Reject obvious traversal characters
|
||||||
for bad in ("/", "\\", ".."):
|
bad_chars = ("\\", "..") if allow_subdir else ("/", "\\", "..")
|
||||||
|
for bad in bad_chars:
|
||||||
if bad in name:
|
if bad in name:
|
||||||
raise ValueError(f"Invalid plugin name '{name}': must not contain '{bad}'.")
|
raise ValueError(f"Invalid plugin name '{name}': must not contain '{bad}'.")
|
||||||
|
|
||||||
@@ -326,7 +346,7 @@ def _display_removed(name: str, plugins_dir: Path) -> None:
|
|||||||
|
|
||||||
def _require_installed_plugin(name: str, plugins_dir: Path, console) -> Path:
|
def _require_installed_plugin(name: str, plugins_dir: Path, console) -> Path:
|
||||||
"""Return the plugin path if it exists, or exit with an error listing installed plugins."""
|
"""Return the plugin path if it exists, or exit with an error listing installed plugins."""
|
||||||
target = _sanitize_plugin_name(name, plugins_dir)
|
target = _sanitize_plugin_name(name, plugins_dir, allow_subdir=True)
|
||||||
if not target.exists():
|
if not target.exists():
|
||||||
installed = ", ".join(d.name for d in plugins_dir.iterdir() if d.is_dir()) or "(none)"
|
installed = ", ".join(d.name for d in plugins_dir.iterdir() if d.is_dir()) or "(none)"
|
||||||
console.print(
|
console.print(
|
||||||
@@ -1508,7 +1528,7 @@ def _user_installed_plugin_dir(name: str) -> Optional[Path]:
|
|||||||
"""Resolved path under ``~/.hermes/plugins/<name>`` if it exists."""
|
"""Resolved path under ``~/.hermes/plugins/<name>`` if it exists."""
|
||||||
plugins_dir = _plugins_dir()
|
plugins_dir = _plugins_dir()
|
||||||
try:
|
try:
|
||||||
target = _sanitize_plugin_name(name, plugins_dir)
|
target = _sanitize_plugin_name(name, plugins_dir, allow_subdir=True)
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return None
|
return None
|
||||||
return target if target.is_dir() else None
|
return target if target.is_dir() else None
|
||||||
|
|||||||
@@ -65,6 +65,36 @@ class TestSanitizePluginName:
|
|||||||
with pytest.raises(ValueError, match="must not be empty"):
|
with pytest.raises(ValueError, match="must not be empty"):
|
||||||
_sanitize_plugin_name("", tmp_path)
|
_sanitize_plugin_name("", tmp_path)
|
||||||
|
|
||||||
|
# ── allow_subdir=True ──
|
||||||
|
|
||||||
|
def test_allow_subdir_accepts_single_slash(self, tmp_path):
|
||||||
|
target = _sanitize_plugin_name(
|
||||||
|
"observability/langfuse", tmp_path, allow_subdir=True
|
||||||
|
)
|
||||||
|
assert target == (tmp_path / "observability" / "langfuse").resolve()
|
||||||
|
|
||||||
|
def test_allow_subdir_strips_leading_trailing_slash(self, tmp_path):
|
||||||
|
target = _sanitize_plugin_name(
|
||||||
|
"/image_gen/openai/", tmp_path, allow_subdir=True
|
||||||
|
)
|
||||||
|
assert target == (tmp_path / "image_gen" / "openai").resolve()
|
||||||
|
|
||||||
|
def test_allow_subdir_still_rejects_dot_dot(self, tmp_path):
|
||||||
|
with pytest.raises(ValueError, match="must not contain"):
|
||||||
|
_sanitize_plugin_name("foo/../bar", tmp_path, allow_subdir=True)
|
||||||
|
|
||||||
|
def test_allow_subdir_still_rejects_backslash(self, tmp_path):
|
||||||
|
with pytest.raises(ValueError, match="must not contain"):
|
||||||
|
_sanitize_plugin_name("foo\\bar", tmp_path, allow_subdir=True)
|
||||||
|
|
||||||
|
def test_allow_subdir_rejects_empty_after_strip(self, tmp_path):
|
||||||
|
with pytest.raises(ValueError, match="must not be empty"):
|
||||||
|
_sanitize_plugin_name("///", tmp_path, allow_subdir=True)
|
||||||
|
|
||||||
|
def test_allow_subdir_resolves_inside_plugins_dir(self, tmp_path):
|
||||||
|
target = _sanitize_plugin_name("a/b/c", tmp_path, allow_subdir=True)
|
||||||
|
assert target.is_relative_to(tmp_path.resolve())
|
||||||
|
|
||||||
|
|
||||||
# ── _resolve_git_url ──────────────────────────────────────────────────────
|
# ── _resolve_git_url ──────────────────────────────────────────────────────
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user