mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(dashboard): suffix-allowlist plugin assets + denylist subprocess-influencing env vars (#32277)
Two posture fixes surfaced by the web-pentest skill self-test against the dashboard (issue #32267). 1. /dashboard-plugins/<name>/<path> previously returned 200 for any file inside the plugin's dashboard directory — including plugin_api.py and __pycache__/*.pyc. The path is unauthenticated by architecture (SPA loads JS via <script src> and CSS via <link href>, neither of which can attach a custom auth header), so the fix is not "require token" — it's "restrict to browser-fetchable suffixes." Allowlist now: .js .mjs .css .json .html .svg .png .jpg .jpeg .gif .webp .ico .woff .woff2 .ttf .otf .map. Everything else → 404. This stops a private user-installed plugin's Python source from being readable by anyone reachable on the dashboard's loopback port (other local users on a shared box, sidecar containers sharing the host netns). 2. save_env_value() now refuses to persist env-var names that influence how the next subprocess executes: LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, DYLD_*, PYTHONPATH, PYTHONHOME, PYTHONSTARTUP, NODE_OPTIONS, NODE_PATH, PATH, SHELL, EDITOR, VISUAL, PAGER, BROWSER, GIT_SSH_COMMAND, GIT_EXEC_PATH; plus HERMES_HOME / HERMES_PROFILE / HERMES_CONFIG / HERMES_ENV. PUT /api/env is authed but the session token lives in the SPA HTML where any future plugin XSS or local process can read it. Without this gate, a token-holder could plant LD_PRELOAD in .env and the next hermes process start would load attacker code via the dotenv to os.environ chain. This is enforced on write only — pre-existing .env values are left alone (the gate is in save_env_value, not in load_env). PUT /api/env now returns 400 with the explanatory message instead of an opaque 500. IMPORTANT: HERMES_* overall is NOT blocked — only the four runtime location names. Integration credentials following the HERMES_* convention (HERMES_GEMINI_*, HERMES_LANGFUSE_*, HERMES_SPOTIFY_*, HERMES_QWEN_BASE_URL, ...) keep working. Regression tests cover both fixes (30 new test cases). No existing tests changed; 257 passing in tests/hermes_cli/. Closes #32267.
This commit is contained in:
Teknium
GitHub
parent
27df4b3882
commit
30928f945f
4 changed files with 305 additions and 2 deletions
|
|
@ -74,6 +74,82 @@ def _warn_config_parse_failure(config_path: Path, exc: Exception) -> None:
|
|||
|
||||
_IS_WINDOWS = platform.system() == "Windows"
|
||||
_ENV_VAR_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
|
||||
|
||||
# Env var names that influence how the next subprocess executes —
|
||||
# never writable through ``save_env_value``. Anything that controls
|
||||
# the loader, interpreter, shell, or replacement editor counts:
|
||||
#
|
||||
# * ``LD_PRELOAD`` / ``LD_LIBRARY_PATH`` / ``LD_AUDIT`` — Linux dynamic
|
||||
# loader. ``DYLD_*`` — macOS equivalent. Planting a path here means
|
||||
# the next ``subprocess.run([...])`` Hermes makes loads attacker code
|
||||
# before main().
|
||||
# * ``PYTHONPATH`` / ``PYTHONHOME`` / ``PYTHONSTARTUP`` /
|
||||
# ``PYTHONUSERBASE`` — Python interpreter init. Hermes itself starts
|
||||
# from one of these on every restart.
|
||||
# * ``NODE_OPTIONS`` / ``NODE_PATH`` — Node interpreter; affects npm,
|
||||
# ``hermes update``, the TUI build.
|
||||
# * ``PATH`` — too broad to allow. The dashboard never needs to rewrite
|
||||
# the operator's PATH; if a tool can't be found, the fix is to add an
|
||||
# absolute path in the integration config, not to mutate PATH globally.
|
||||
# * ``GIT_SSH_COMMAND`` / ``GIT_EXEC_PATH`` — git rewrites that fire
|
||||
# on every plugin install / ``hermes update``.
|
||||
# * ``BROWSER`` / ``EDITOR`` / ``VISUAL`` / ``PAGER`` — commands the
|
||||
# shell or CLI invokes implicitly. Wrong values here = RCE on next
|
||||
# ``$EDITOR``.
|
||||
# * ``SHELL`` — what subprocess uses with ``shell=True`` (we try to
|
||||
# avoid that, but defense in depth).
|
||||
# * ``HERMES_HOME`` / ``HERMES_PROFILE`` / ``HERMES_CONFIG`` /
|
||||
# ``HERMES_ENV`` — Hermes runtime location flags. Writing these into
|
||||
# ``.env`` would relocate state in ways the user did not request from
|
||||
# the dashboard. ``config.yaml`` is the supported surface for these.
|
||||
#
|
||||
# IMPORTANT: ``HERMES_*`` overall is NOT blocked. Many legitimate
|
||||
# integration credentials follow that prefix (HERMES_GEMINI_CLIENT_ID,
|
||||
# HERMES_LANGFUSE_PUBLIC_KEY, HERMES_SPOTIFY_CLIENT_ID, ...). The
|
||||
# denylist is name-by-name on purpose so the gate stays narrow and
|
||||
# doesn't accidentally break provider setup wizards.
|
||||
#
|
||||
# This is enforced on *write* only — values already in ``.env`` (set
|
||||
# by the operator out-of-band, or pre-existing) keep working. The
|
||||
# point is that the dashboard's writable surface cannot escalate by
|
||||
# planting them.
|
||||
_ENV_VAR_NAME_DENYLIST: frozenset[str] = frozenset({
|
||||
# Loader / linker
|
||||
"LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT", "LD_DEBUG",
|
||||
"DYLD_INSERT_LIBRARIES", "DYLD_LIBRARY_PATH", "DYLD_FRAMEWORK_PATH",
|
||||
"DYLD_FALLBACK_LIBRARY_PATH", "DYLD_FALLBACK_FRAMEWORK_PATH",
|
||||
# Python
|
||||
"PYTHONPATH", "PYTHONHOME", "PYTHONSTARTUP", "PYTHONUSERBASE",
|
||||
"PYTHONEXECUTABLE", "PYTHONNOUSERSITE",
|
||||
# Node
|
||||
"NODE_OPTIONS", "NODE_PATH",
|
||||
# General
|
||||
"PATH", "SHELL", "BROWSER", "EDITOR", "VISUAL", "PAGER",
|
||||
# Git
|
||||
"GIT_SSH_COMMAND", "GIT_EXEC_PATH", "GIT_SHELL",
|
||||
# Hermes runtime location — never via dashboard env writer.
|
||||
# NOT a HERMES_* blanket: integration credentials (HERMES_GEMINI_*,
|
||||
# HERMES_LANGFUSE_*, HERMES_SPOTIFY_*, ...) ARE allowed.
|
||||
"HERMES_HOME", "HERMES_PROFILE", "HERMES_CONFIG", "HERMES_ENV",
|
||||
})
|
||||
|
||||
|
||||
def _reject_denylisted_env_var(key: str) -> None:
|
||||
"""Raise if ``key`` is in :data:`_ENV_VAR_NAME_DENYLIST`.
|
||||
|
||||
Centralised so both the regular and "secure" env writers share the
|
||||
same gate, and so the message is consistent for callers.
|
||||
"""
|
||||
if key in _ENV_VAR_NAME_DENYLIST:
|
||||
raise ValueError(
|
||||
f"Environment variable {key!r} is on the writer denylist. "
|
||||
"Names that influence subprocess execution (LD_PRELOAD, "
|
||||
"PYTHONPATH, PATH, EDITOR, ...) or Hermes runtime location "
|
||||
"(HERMES_HOME, HERMES_PROFILE, ...) cannot be persisted via "
|
||||
"the env writer. If you really need this, edit "
|
||||
"~/.hermes/.env directly."
|
||||
)
|
||||
|
||||
_LAST_EXPANDED_CONFIG_BY_PATH: Dict[str, Any] = {}
|
||||
# (path, mtime_ns, size) -> cached expanded config dict.
|
||||
# load_config() returns a deepcopy of the cached value when the file
|
||||
|
|
@ -4874,6 +4950,7 @@ def save_env_value(key: str, value: str):
|
|||
return
|
||||
if not _ENV_VAR_NAME_RE.match(key):
|
||||
raise ValueError(f"Invalid environment variable name: {key!r}")
|
||||
_reject_denylisted_env_var(key)
|
||||
value = value.replace("\n", "").replace("\r", "")
|
||||
# API keys / tokens must be ASCII — strip non-ASCII with a warning.
|
||||
value = _check_non_ascii_credential(key, value)
|
||||
|
|
|
|||
|
|
@ -1223,6 +1223,12 @@ async def set_env_var(body: EnvVarUpdate):
|
|||
try:
|
||||
save_env_value(body.key, body.value)
|
||||
return {"ok": True, "key": body.key}
|
||||
except ValueError as exc:
|
||||
# save_env_value raises ValueError for invalid names and for keys
|
||||
# on the denylist (LD_PRELOAD, PATH, PYTHONPATH, …). Surface the
|
||||
# message to the SPA so the user understands why the write was
|
||||
# refused instead of seeing an opaque 500.
|
||||
raise HTTPException(status_code=400, detail=str(exc)) from exc
|
||||
except Exception:
|
||||
_log.exception("PUT /api/env failed")
|
||||
raise HTTPException(status_code=500, detail="Internal server error")
|
||||
|
|
@ -4543,6 +4549,17 @@ async def serve_plugin_asset(plugin_name: str, file_path: str):
|
|||
|
||||
Only serves files from the plugin's ``dashboard/`` subdirectory.
|
||||
Path traversal is blocked by checking ``resolve().is_relative_to()``.
|
||||
|
||||
Restricted to a browser-fetchable suffix allowlist (JS/CSS/JSON/HTML/
|
||||
SVG/PNG/JPG/WOFF). The dashboard loads plugin JS via ``<script src>``
|
||||
and CSS via ``<link href>``, neither of which can attach a custom
|
||||
auth header — so this route stays unauthenticated to keep the SPA
|
||||
working. But user-installed plugins ship a ``plugin_api.py``
|
||||
backend module that the browser never fetches; it's only imported
|
||||
by :func:`_mount_plugin_api_routes` at startup. Without a suffix
|
||||
allowlist, anyone on the loopback port can curl the ``.py`` source
|
||||
of a private third-party plugin. Reject everything outside the
|
||||
browser-asset set.
|
||||
"""
|
||||
plugins = _get_dashboard_plugins()
|
||||
plugin = next((p for p in plugins if p["name"] == plugin_name), None)
|
||||
|
|
@ -4557,7 +4574,11 @@ async def serve_plugin_asset(plugin_name: str, file_path: str):
|
|||
if not target.exists() or not target.is_file():
|
||||
raise HTTPException(status_code=404, detail="File not found")
|
||||
|
||||
# Guess content type
|
||||
# Browser-asset suffix allowlist. Everything outside this set is
|
||||
# rejected with 404 so we don't leak ``.py`` backend sources, README
|
||||
# files, ``.env.example`` templates, etc. — none of which the SPA
|
||||
# actually fetches. Add to this set deliberately when a new asset
|
||||
# type comes up; do NOT change the default fallback.
|
||||
suffix = target.suffix.lower()
|
||||
content_types = {
|
||||
".js": "application/javascript",
|
||||
|
|
@ -4568,10 +4589,22 @@ async def serve_plugin_asset(plugin_name: str, file_path: str):
|
|||
".svg": "image/svg+xml",
|
||||
".png": "image/png",
|
||||
".jpg": "image/jpeg",
|
||||
".jpeg": "image/jpeg",
|
||||
".gif": "image/gif",
|
||||
".webp": "image/webp",
|
||||
".ico": "image/x-icon",
|
||||
".woff2": "font/woff2",
|
||||
".woff": "font/woff",
|
||||
".ttf": "font/ttf",
|
||||
".otf": "font/otf",
|
||||
".map": "application/json",
|
||||
}
|
||||
media_type = content_types.get(suffix, "application/octet-stream")
|
||||
if suffix not in content_types:
|
||||
raise HTTPException(
|
||||
status_code=404,
|
||||
detail="File not found",
|
||||
)
|
||||
media_type = content_types[suffix]
|
||||
return FileResponse(
|
||||
target,
|
||||
media_type=media_type,
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import os
|
|||
from pathlib import Path
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
import pytest
|
||||
import yaml
|
||||
|
||||
from hermes_cli.config import (
|
||||
|
|
@ -775,3 +776,120 @@ class TestUserMessagePreviewConfig:
|
|||
preview = DEFAULT_CONFIG["display"]["user_message_preview"]
|
||||
assert preview["first_lines"] == 2
|
||||
assert preview["last_lines"] == 2
|
||||
|
||||
|
||||
class TestEnvWriteDenylist:
|
||||
"""``save_env_value`` refuses to persist env-var names that
|
||||
influence how subprocesses execute — ``LD_PRELOAD``, ``PYTHONPATH``,
|
||||
``PATH``, ``EDITOR``, etc. — or any ``HERMES_*`` runtime flag.
|
||||
|
||||
The dashboard exposes ``PUT /api/env`` to any authed caller (and
|
||||
the session token lives in the SPA's HTML where any future plugin
|
||||
XSS or local process could exfiltrate it). Without this gate, an
|
||||
attacker who steals the token could plant
|
||||
``LD_PRELOAD=/tmp/evil.so`` in ``.env`` and own the next Hermes
|
||||
process on next startup via the dotenv → ``os.environ`` chain in
|
||||
``hermes_cli/env_loader.py``.
|
||||
|
||||
Regression test for the dashboard pentest finding filed alongside
|
||||
the ``web-pentest`` skill (PR #32265 / issue #32267).
|
||||
"""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _hermes_home(self, tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
ensure_hermes_home()
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"denied_key",
|
||||
[
|
||||
"LD_PRELOAD",
|
||||
"LD_LIBRARY_PATH",
|
||||
"LD_AUDIT",
|
||||
"DYLD_INSERT_LIBRARIES",
|
||||
"DYLD_LIBRARY_PATH",
|
||||
"PYTHONPATH",
|
||||
"PYTHONHOME",
|
||||
"PYTHONSTARTUP",
|
||||
"NODE_OPTIONS",
|
||||
"NODE_PATH",
|
||||
"PATH",
|
||||
"SHELL",
|
||||
"EDITOR",
|
||||
"VISUAL",
|
||||
"PAGER",
|
||||
"BROWSER",
|
||||
"GIT_SSH_COMMAND",
|
||||
"GIT_EXEC_PATH",
|
||||
"HERMES_HOME",
|
||||
"HERMES_PROFILE",
|
||||
"HERMES_CONFIG",
|
||||
"HERMES_ENV",
|
||||
],
|
||||
)
|
||||
def test_denylisted_keys_rejected(self, denied_key):
|
||||
"""Each denylisted name raises ``ValueError`` and never reaches
|
||||
the on-disk ``.env`` file."""
|
||||
with pytest.raises(ValueError, match="denylist"):
|
||||
save_env_value(denied_key, "anything")
|
||||
|
||||
# And nothing landed on disk either.
|
||||
env = load_env()
|
||||
assert denied_key not in env
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"allowed_key",
|
||||
[
|
||||
"HERMES_GEMINI_CLIENT_ID",
|
||||
"HERMES_LANGFUSE_PUBLIC_KEY",
|
||||
"HERMES_SPOTIFY_CLIENT_ID",
|
||||
"HERMES_QWEN_BASE_URL",
|
||||
"HERMES_MAX_ITERATIONS",
|
||||
],
|
||||
)
|
||||
def test_hermes_integration_keys_still_writable(self, allowed_key):
|
||||
"""``HERMES_*`` overall is NOT blocked — only the four runtime
|
||||
location names (HOME/PROFILE/CONFIG/ENV) are. Integration
|
||||
credentials following the ``HERMES_*`` convention must keep
|
||||
working or we'd regress every provider setup wizard that
|
||||
currently writes one of these (auth.py, Spotify, Langfuse, …)."""
|
||||
save_env_value(allowed_key, "test-value-123")
|
||||
env = load_env()
|
||||
assert env[allowed_key] == "test-value-123"
|
||||
|
||||
def test_legitimate_provider_key_still_works(self):
|
||||
"""The denylist must not regress on real provider key writes."""
|
||||
save_env_value("OPENROUTER_API_KEY", "sk-or-test-1234")
|
||||
env = load_env()
|
||||
assert env["OPENROUTER_API_KEY"] == "sk-or-test-1234"
|
||||
|
||||
def test_arbitrary_user_key_still_works(self):
|
||||
"""Plugin / user-defined env vars (anything outside the
|
||||
denylist and outside ``HERMES_*``) keep working. The denylist
|
||||
is narrow on purpose."""
|
||||
save_env_value("MY_PLUGIN_TOKEN", "plugin-secret-123")
|
||||
env = load_env()
|
||||
assert env["MY_PLUGIN_TOKEN"] == "plugin-secret-123"
|
||||
|
||||
def test_save_env_value_secure_inherits_denylist(self):
|
||||
"""The ``_secure`` variant goes through ``save_env_value`` so
|
||||
it inherits the gate — verify, don't assume."""
|
||||
with pytest.raises(ValueError, match="denylist"):
|
||||
save_env_value_secure("LD_PRELOAD", "/tmp/evil.so")
|
||||
|
||||
def test_pre_existing_value_in_env_file_is_left_alone(self, tmp_path):
|
||||
"""The gate is on *write*. If ``.env`` already contains
|
||||
``LD_PRELOAD`` (set out-of-band by the operator before this
|
||||
change shipped, or hand-edited), we don't blow up — we just
|
||||
refuse to add or update it via the API."""
|
||||
env_path = tmp_path / ".env"
|
||||
env_path.write_text("LD_PRELOAD=/something/legit.so\n")
|
||||
|
||||
# load_env returns it (the read path is intentionally permissive)
|
||||
env = load_env()
|
||||
assert env["LD_PRELOAD"] == "/something/legit.so"
|
||||
|
||||
# But the write path still refuses to update it
|
||||
with pytest.raises(ValueError, match="denylist"):
|
||||
save_env_value("LD_PRELOAD", "/tmp/evil.so")
|
||||
|
||||
|
|
|
|||
|
|
@ -2375,3 +2375,78 @@ class TestPtyWebSocket:
|
|||
):
|
||||
pass
|
||||
assert exc.value.code == 4400
|
||||
|
||||
|
||||
class TestDashboardPluginStaticAssetAllowlist:
|
||||
"""``/dashboard-plugins/<name>/<path>`` is unauthenticated by design —
|
||||
the SPA loads plugin JS via ``<script src>`` and CSS via
|
||||
``<link href>``, neither of which can attach a custom auth header.
|
||||
Instead the route restricts file types to the browser-asset
|
||||
allowlist (JS/CSS/JSON/images/fonts) so that user-installed
|
||||
plugins shipping a ``plugin_api.py`` backend module don't leak
|
||||
their Python source to anyone reachable on the loopback port.
|
||||
|
||||
Regression test for the dashboard pentest finding filed alongside
|
||||
the ``web-pentest`` skill (PR #32265 / issue #32267).
|
||||
"""
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _setup_test_client(self, monkeypatch, _isolate_hermes_home):
|
||||
try:
|
||||
from starlette.testclient import TestClient
|
||||
except ImportError:
|
||||
pytest.skip("fastapi/starlette not installed")
|
||||
|
||||
from hermes_cli.web_server import app
|
||||
|
||||
self.client = TestClient(app)
|
||||
|
||||
def test_python_source_is_404(self):
|
||||
"""The example plugin's ``plugin_api.py`` must NOT be served as
|
||||
a static asset, even though the file exists under the plugin's
|
||||
dashboard directory. Suffix not in the allowlist → 404."""
|
||||
resp = self.client.get("/dashboard-plugins/example/plugin_api.py")
|
||||
assert resp.status_code == 404
|
||||
|
||||
def test_pycache_is_404(self):
|
||||
"""Same protection for compiled Python (``.pyc``) inside the
|
||||
plugin's ``__pycache__/``. Real plugins ship these as a
|
||||
side-effect of running tests / dashboard once."""
|
||||
# __pycache__ files are only generated after the api file has
|
||||
# been imported once. Use the path the example plugin actually
|
||||
# generates during the dashboard test boot.
|
||||
resp = self.client.get(
|
||||
"/dashboard-plugins/example/__pycache__/plugin_api.cpython-311.pyc"
|
||||
)
|
||||
# 404 either way (file may not exist on this CI Python version);
|
||||
# what matters is we never get a 200 with the bytes.
|
||||
assert resp.status_code == 404
|
||||
|
||||
def test_manifest_json_still_served(self):
|
||||
"""JSON files remain browser-fetchable — manifests, localized
|
||||
data, source maps, etc. all sit in this bucket."""
|
||||
resp = self.client.get("/dashboard-plugins/example/manifest.json")
|
||||
assert resp.status_code == 200
|
||||
assert resp.headers["content-type"].startswith("application/json")
|
||||
# And the body is actually the manifest, not the SPA fallback.
|
||||
body = resp.json()
|
||||
assert body.get("name") == "example"
|
||||
|
||||
def test_unknown_plugin_is_404(self):
|
||||
"""Existing behaviour preserved: nonexistent plugin name → 404."""
|
||||
resp = self.client.get(
|
||||
"/dashboard-plugins/_definitely_not_a_plugin_/manifest.json"
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
|
||||
def test_path_traversal_still_blocked(self):
|
||||
"""The allowlist is on top of the existing ``.resolve()`` /
|
||||
``is_relative_to()`` check — a ``.js`` named file at an
|
||||
out-of-base path is still rejected as traversal, not served."""
|
||||
resp = self.client.get(
|
||||
"/dashboard-plugins/example/..%2Fplugin_api.py"
|
||||
)
|
||||
# 403 traversal-blocked OR 404 (depending on URL decode order)
|
||||
# — never 200.
|
||||
assert resp.status_code in (403, 404)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue