diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index c3af49f2531..401dfa6506a 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -1194,15 +1194,86 @@ _FS_READDIR_HIDDEN = { # Filenames that must never be listed, read, or downloaded through the # managed-files API. These typically contain credentials (API keys, tokens) # and exposing them through the dashboard file browser is a security leak — -# see issue #57505. -def _is_sensitive_filename(name: str) -> bool: - """Return True for ``.env`` and any ``.env.`` variant. +# see issue #57505. The set mirrors the credential-file basenames of the two +# canonical credential guards elsewhere in the codebase +# (agent.file_safety.get_read_block_error and +# gateway.platforms.base._ROOT_CREDENTIAL_FILES) so the dashboard Files tab +# doesn't lag behind them — an operator can point the managed root at +# HERMES_HOME itself, at which point every one of these basenames is a live +# secret store sitting in the browsable tree. +_SENSITIVE_MANAGED_FILE_BASENAMES = frozenset({ + "auth.json", + "auth.lock", + "credentials", + "config.yaml", + ".anthropic_oauth.json", + "google_token.json", + "google_oauth_pending.json", + "google_oauth.json", + "webhook_subscriptions.json", + "bws_cache.json", + # git's credential-store helper cache (agent.file_safety blocks this too). + ".git-credentials", +}) - Case-insensitive so ``.ENV`` / ``.Env.local`` on case-insensitive - filesystems (macOS/Windows mounts) can't slip past the guard. +# Directory names whose entire subtree is credential material. Both canonical +# guards deny these as directory trees, not basenames: +# * gateway.platforms.base._ROOT_CREDENTIAL_DIRS = {"pairing", "mcp-tokens"} +# * agent.file_safety.get_read_block_error (mcp-tokens/ prefix match) +# The managed-files API lets the browser descend into subdirs, so a +# basename-only guard would still expose e.g. ``mcp-tokens/.json`` +# (live MCP OAuth tokens) and ``pairing/``. We match on ANY path component +# so these trees are blocked wherever they appear under the browsable root, +# without needing to resolve them relative to HERMES_HOME. +_SENSITIVE_MANAGED_DIR_NAMES = frozenset({ + "mcp-tokens", + "pairing", +}) + + +def _is_sensitive_filename(name: str) -> bool: + """Return True for a basename the managed-files API must never expose. + + Covers ``.env`` / ``.env.`` / ``.envrc`` variants plus the + canonical Hermes credential-store basenames (see + ``_SENSITIVE_MANAGED_FILE_BASENAMES`` above). + + Case-insensitive so ``.ENV`` / ``.Env.local`` / ``Auth.JSON`` on + case-insensitive filesystems (macOS/Windows mounts) can't slip past + the guard. + + Basename-only: for the directory-tree credential stores + (``mcp-tokens/``, ``pairing/``) that the canonical guards also deny, + use :func:`_is_sensitive_path`, which the API call sites route through. """ lowered = name.lower() - return lowered == ".env" or lowered.startswith(".env.") + if lowered == ".env" or lowered.startswith(".env.") or lowered == ".envrc": + return True + return lowered in _SENSITIVE_MANAGED_FILE_BASENAMES + + +def _is_sensitive_path(path: Path) -> bool: + """Return True for any path the managed-files API must never expose. + + Combines the basename denylist (:func:`_is_sensitive_filename`) with a + credential-directory-tree check: a path is sensitive if its own basename + is sensitive OR any of its path components is a credential directory + (``mcp-tokens`` / ``pairing``). The component match is case-insensitive + and needs no HERMES_HOME resolution, so it blocks these trees wherever + they sit under the operator-configured managed root — closing the gap + the canonical guards cover as directory trees but a basename-only check + would miss. + + Read-side only: this guards list/read/download (the #57505 exfil surface). + The write endpoints (upload/mkdir/delete) are a separate threat class + handled by the write-path checks; extending this guard to them is out of + scope for this fix. + """ + if _is_sensitive_filename(path.name): + return True + return any(part.lower() in _SENSITIVE_MANAGED_DIR_NAMES for part in path.parts) + + _FS_DATA_URL_MAX_BYTES = 16 * 1024 * 1024 _FS_TEXT_SOURCE_MAX_BYTES = 64 * 1024 * 1024 _FS_TEXT_PREVIEW_MAX_BYTES = 512 * 1024 @@ -1636,7 +1707,7 @@ async def list_managed_files(request: Request, path: Optional[str] = None): entries = [ _managed_file_entry(policy, child) for child in target.iterdir() - if not _is_sensitive_filename(child.name) + if not _is_sensitive_path(child) ] except PermissionError: raise HTTPException(status_code=403, detail="Directory is not readable") @@ -1663,7 +1734,7 @@ async def read_managed_file(request: Request, path: str): raise HTTPException(status_code=404, detail="File not found") if not target.is_file(): raise HTTPException(status_code=400, detail="Path is not a file") - if _is_sensitive_filename(target.name): + if _is_sensitive_path(target): raise HTTPException(status_code=403, detail="Access to sensitive files is not allowed") try: @@ -1707,7 +1778,7 @@ async def download_managed_file(request: Request, path: str): raise HTTPException(status_code=404, detail="File not found") if not target.is_file(): raise HTTPException(status_code=400, detail="Path is not a file") - if _is_sensitive_filename(target.name): + if _is_sensitive_path(target): raise HTTPException(status_code=403, detail="Access to sensitive files is not allowed") try: diff --git a/tests/hermes_cli/test_web_server_files.py b/tests/hermes_cli/test_web_server_files.py index 63a8b39ff07..d1e46e0f82a 100644 --- a/tests/hermes_cli/test_web_server_files.py +++ b/tests/hermes_cli/test_web_server_files.py @@ -560,3 +560,121 @@ def test_sensitive_env_case_insensitive_blocked(forced_files_client): p.write_text("SECRET=abc123") assert client.get("/api/files/read", params={"path": str(p)}).status_code == 403 assert client.get("/api/files/download", params={"path": str(p)}).status_code == 403 + + +def test_envrc_blocked(forced_files_client): + """Regression: .envrc (direnv) is a distinct basename from .env. and + was not caught by the old ``== ".env" or startswith(".env.")`` check.""" + client, root = forced_files_client + + root.mkdir(parents=True, exist_ok=True) + p = root / ".envrc" + p.write_text("export SECRET_KEY=abc123") + + listing = client.get("/api/files", params={"path": str(root)}) + assert ".envrc" not in [e["name"] for e in listing.json()["entries"]] + assert client.get("/api/files/read", params={"path": str(p)}).status_code == 403 + assert client.get("/api/files/download", params={"path": str(p)}).status_code == 403 + + +def test_other_credential_store_basenames_blocked(forced_files_client): + """Regression: the managed-files guard must cover the same credential + basenames as gateway.platforms.base._ROOT_CREDENTIAL_FILES and + agent.file_safety.get_read_block_error, not just .env — an operator can + point the managed root at HERMES_HOME itself (#57505), which contains + all of these live secret stores.""" + client, root = forced_files_client + root.mkdir(parents=True, exist_ok=True) + + for name in ( + "auth.json", + "auth.lock", + "credentials", + "config.yaml", + ".anthropic_oauth.json", + "google_token.json", + "google_oauth_pending.json", + "google_oauth.json", + "webhook_subscriptions.json", + "bws_cache.json", + ): + p = root / name + p.write_text("SECRET=abc123") + assert client.get("/api/files/read", params={"path": str(p)}).status_code == 403, name + assert client.get("/api/files/download", params={"path": str(p)}).status_code == 403, name + + listing = client.get("/api/files", params={"path": str(root)}) + names = [e["name"] for e in listing.json()["entries"]] + assert names == [] + + +def test_git_credentials_blocked(forced_files_client): + """Regression: .git-credentials (git's credential-store helper cache) is + blocked by agent.file_safety; the dashboard guard must cover it too.""" + client, root = forced_files_client + + root.mkdir(parents=True, exist_ok=True) + p = root / ".git-credentials" + p.write_text("https://user:token@github.com\n") + + listing = client.get("/api/files", params={"path": str(root)}) + assert ".git-credentials" not in [e["name"] for e in listing.json()["entries"]] + assert client.get("/api/files/read", params={"path": str(p)}).status_code == 403 + assert client.get("/api/files/download", params={"path": str(p)}).status_code == 403 + + +def test_credential_dir_trees_blocked_on_subdir_descent(forced_files_client): + """Regression: mcp-tokens/ (live MCP OAuth tokens) and pairing/ are denied + as whole directory trees by both canonical guards + (gateway.platforms.base._ROOT_CREDENTIAL_DIRS and + agent.file_safety). A basename-only check would still expose their + per-server files (e.g. ``mcp-tokens/github.json``) once the browser + descends into the subdir. The managed-files guard must block any path with + a credential-directory component, not just leaf basenames.""" + client, root = forced_files_client + root.mkdir(parents=True, exist_ok=True) + + # A per-server MCP token file with a NON-canonical basename that the + # basename denylist alone would not catch. + mcp_dir = root / "mcp-tokens" + mcp_dir.mkdir(parents=True, exist_ok=True) + mcp_file = mcp_dir / "github.json" + mcp_file.write_text('{"access_token": "SECRET"}\n') + + pairing_dir = root / "pairing" + pairing_dir.mkdir(parents=True, exist_ok=True) + pairing_file = pairing_dir / "device-abc" + pairing_file.write_text("PAIRING-SECRET\n") + + # The token dirs themselves must not appear in the root listing. + root_names = [e["name"] for e in client.get( + "/api/files", params={"path": str(root)}).json()["entries"]] + assert "mcp-tokens" not in root_names + assert "pairing" not in root_names + + # Read/download of the per-server files must be denied even though their + # basenames aren't in _SENSITIVE_MANAGED_FILE_BASENAMES. + for p in (mcp_file, pairing_file): + assert client.get("/api/files/read", params={"path": str(p)}).status_code == 403, str(p) + assert client.get("/api/files/download", params={"path": str(p)}).status_code == 403, str(p) + + # Listing the credential dir itself yields nothing exploitable: every child + # is filtered because the parent component is a credential dir. + mcp_listing = client.get("/api/files", params={"path": str(mcp_dir)}) + assert [e["name"] for e in mcp_listing.json()["entries"]] == [] + + +def test_benign_subdir_file_still_browsable(forced_files_client): + """Positive control: the directory-component guard must NOT over-block a + benign subdir. A normal file under a normal subdir stays listable/readable.""" + client, root = forced_files_client + root.mkdir(parents=True, exist_ok=True) + + sub = root / "notes" + sub.mkdir(parents=True, exist_ok=True) + p = sub / "todo.txt" + p.write_text("buy milk\n") + + listing = client.get("/api/files", params={"path": str(sub)}) + assert "todo.txt" in [e["name"] for e in listing.json()["entries"]] + assert client.get("/api/files/read", params={"path": str(p)}).status_code == 200