mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(dashboard): close credential-dir-tree gap + .git-credentials in managed-files guard
Follow-up to @srojk34's basename-denylist widening. Two gaps the basename-only guard left, both covered by the two canonical guards it mirrors: - Directory-tree stores mcp-tokens/ (live MCP OAuth tokens) and pairing/ are denied as whole trees by gateway.platforms.base._ROOT_CREDENTIAL_DIRS and agent.file_safety, but the dashboard files API descends into subdirs, so mcp-tokens/<server>.json (non-canonical basename) stayed listable/readable/downloadable. Add _is_sensitive_path(), a path-aware check that blocks any path with a credential-directory component, and route all three call sites (list/read/download) through it. - Add .git-credentials to the basename set (agent.file_safety blocks it too). - Correct the docstring: it now says it mirrors the credential-FILE basenames of the canonical guards, with the directory trees handled by the new path-aware helper (the prior wording overstated parity). Scope stays on the read/list/download exfil surface (#57505); the write endpoints (upload/mkdir/delete) are a separate threat and out of scope. Tests: dir-tree descent blocked (mcp-tokens/pairing per-server files), .git-credentials blocked, plus a positive control that a benign subdir file stays browsable. Mutation-checked (neuter _is_sensitive_path -> new tests fail). 39 web_server_files + fs tests pass, ruff clean.
This commit is contained in:
parent
43ec69cef3
commit
8b24376d63
2 changed files with 123 additions and 6 deletions
|
|
@ -1194,8 +1194,9 @@ _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. The set mirrors the two canonical credential guards
|
||||
# elsewhere in the codebase (agent.file_safety.get_read_block_error and
|
||||
# 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
|
||||
|
|
@ -1211,11 +1212,27 @@ _SENSITIVE_MANAGED_FILE_BASENAMES = frozenset({
|
|||
"google_oauth.json",
|
||||
"webhook_subscriptions.json",
|
||||
"bws_cache.json",
|
||||
# git's credential-store helper cache (agent.file_safety blocks this too).
|
||||
".git-credentials",
|
||||
})
|
||||
|
||||
# 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 filename the managed-files API must never expose.
|
||||
"""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
|
||||
|
|
@ -1224,11 +1241,39 @@ def _is_sensitive_filename(name: str) -> bool:
|
|||
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()
|
||||
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
|
||||
|
|
@ -1662,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")
|
||||
|
|
@ -1689,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:
|
||||
|
|
@ -1733,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:
|
||||
|
|
|
|||
|
|
@ -606,3 +606,75 @@ def test_other_credential_store_basenames_blocked(forced_files_client):
|
|||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue