Merge pull request #58222 from kshitijk4poor/salvage/dashboard-credential-guard

security(dashboard): widen managed-files credential guard past .env + close dir-tree gap

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.<suffix>`` 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/<server>.json``
+# (live MCP OAuth tokens) and ``pairing/<x>``. 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.<suffix>`` / ``.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:

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.<suffix> 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