fix(security): write-deny HERMES_HOME secret stores, keep control files writable
Narrow the fix to secret material and stop reverting #45947. The earlier revision added every read-denied name to the write denylist, which re-blocked auth.json and webhook_subscriptions.json and rewrote the test guarding them. #45947 freed those control files deliberately: containment belongs in Docker/remote backends and OS permissions, not an expanding hardcoded denylist. What #45947 kept blocked is secret material, and that list had drifted: auth/google_oauth.json (OAuth token store), the plaintext Bitwarden cache, vault/ (key + ciphertext side by side) and browser-profile/ (copied cookies / Login Data) were writable via write_file / patch. - Write-deny those four; control files stay writable and read-denied. - _WRITE_DENIED_SECRET_DIRS is its own tuple rather than _READ_DENIED_DIRS, so a future read-only convenience deny cannot silently become a write deny. - tests/tools/test_write_deny.py is restored unchanged from main. - New tests pin both halves: secrets denied, control files writable, and write denies stay a subset of read denies. Fixes #110464
This commit is contained in:
NUXER
+23
-7
@@ -157,11 +157,20 @@ def build_write_denied_paths(home: str) -> set[str]:
|
||||
(".ssh", "authorized_keys"), (".ssh", "id_rsa"), (".ssh", "id_ed25519"),
|
||||
(".netrc",), (".pgpass",), (".npmrc",), (".pypirc",), (".git-credentials",),
|
||||
)
|
||||
# Same HERMES_HOME credential files the read guard already blocks, on both
|
||||
# the active profile and the global root. bws_cache.enc.json is write-only
|
||||
# extra: the plaintext sibling is in ``_CREDENTIAL_FILE_NAMES``.
|
||||
# Secret material under HERMES_HOME, on both the active profile and the global
|
||||
# root: overwriting the root .env leaks credentials across every profile that
|
||||
# inherits it, and the root Anthropic PKCE store is still read by default /
|
||||
# non-profile sessions when a profile is active. google_oauth.json is an OAuth
|
||||
# token store; both Bitwarden caches hold Secrets Manager material.
|
||||
#
|
||||
# auth.json, auth.lock, config.yaml and webhook_subscriptions.json are
|
||||
# deliberately NOT here: #45947 freed those control files on purpose
|
||||
# ("true containment belongs in Docker/remote backends and OS permissions,
|
||||
# not an expanding hardcoded denylist"). They stay read-denied, not write-denied.
|
||||
hermes_files = (
|
||||
*_CREDENTIAL_FILE_NAMES,
|
||||
".env", ".anthropic_oauth.json",
|
||||
os.path.join("auth", "google_oauth.json"),
|
||||
os.path.join("cache", "bws_cache.json"),
|
||||
os.path.join("cache", "bws_cache.enc.json"),
|
||||
)
|
||||
paths = [
|
||||
@@ -208,6 +217,11 @@ def build_write_approval_paths(home: str) -> set[str]:
|
||||
# mcp-tokens/ and pairing/ hold credential material.
|
||||
_HERMES_PROTECTED_SUBPATHS = ("state.db", "sessions", "mcp-tokens", "pairing")
|
||||
|
||||
# Read-denied directories that are also secret material, so writes are blocked
|
||||
# too. Kept as its own tuple (not derived from _READ_DENIED_DIRS) so adding a
|
||||
# read-only *convenience* deny later cannot silently become a write deny.
|
||||
_WRITE_DENIED_SECRET_DIRS = ("vault", "browser-profile")
|
||||
|
||||
|
||||
def _classify_write_denial(path: str) -> Optional[str]:
|
||||
"""Return ``'credential'``, ``'safe_root'``, ``'nt_namespace'``, or ``None`` if writes are allowed."""
|
||||
@@ -233,9 +247,11 @@ def _classify_write_denial(path: str) -> Optional[str]:
|
||||
with suppress(Exception):
|
||||
if _is_under(resolved, os.path.realpath(os.path.join(str(base), sub))):
|
||||
return "credential"
|
||||
# vault/ and browser-profile/ are credential dirs on the read path;
|
||||
# mcp-tokens/ is already in _HERMES_PROTECTED_SUBPATHS.
|
||||
for sub, _, _ in _READ_DENIED_DIRS:
|
||||
# vault/ (key + ciphertext side by side) and browser-profile/ (copied
|
||||
# cookies / Login Data) are secret stores, not control files, so the
|
||||
# #45947 relaxation does not cover them. mcp-tokens/ is already in
|
||||
# _HERMES_PROTECTED_SUBPATHS.
|
||||
for sub in _WRITE_DENIED_SECRET_DIRS:
|
||||
with suppress(Exception):
|
||||
if _is_under(resolved, os.path.realpath(os.path.join(str(base), sub))):
|
||||
return "credential"
|
||||
|
||||
@@ -1,13 +1,16 @@
|
||||
"""Write denylist must cover the same HERMES_HOME credential stores as reads.
|
||||
"""Secret stores under HERMES_HOME must be write-denied, control files must not.
|
||||
|
||||
Reads already refuse auth.json, webhook HMAC secrets, google_oauth.json,
|
||||
the Bitwarden plaintext cache, vault/, and browser-profile/. Writes only
|
||||
blocked .env, the Anthropic PKCE store, and bws_cache.enc.json, so
|
||||
write_file/patch could replace the rest.
|
||||
``is_read_denied`` refuses every credential store. The write side is
|
||||
deliberately narrower: #45947 freed ``auth.json``, ``config.yaml`` and
|
||||
``webhook_subscriptions.json`` on the grounds that containment belongs in
|
||||
Docker/remote backends and OS permissions rather than an expanding denylist.
|
||||
|
||||
The invariant: every name in ``_CREDENTIAL_FILE_NAMES`` and every directory
|
||||
in ``_READ_DENIED_DIRS`` is write-denied under both the active home and the
|
||||
global root. Nested same-basename files (skill mocks) stay writable.
|
||||
What #45947 kept blocked is secret *material*, and that list had drifted:
|
||||
``auth/google_oauth.json`` (an OAuth token store), the plaintext Bitwarden
|
||||
cache, ``vault/`` (key + ciphertext side by side) and ``browser-profile/``
|
||||
(copied cookies / Login Data) were writable through ``write_file`` / ``patch``.
|
||||
|
||||
These tests pin both halves so neither can drift again.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -18,6 +21,18 @@ import pytest
|
||||
|
||||
import agent.file_safety as fs
|
||||
|
||||
# Secret material: read-denied AND write-denied.
|
||||
SECRET_FILES = (
|
||||
".env",
|
||||
".anthropic_oauth.json",
|
||||
"auth/google_oauth.json",
|
||||
"cache/bws_cache.json",
|
||||
"cache/bws_cache.enc.json",
|
||||
)
|
||||
|
||||
# Read-denied control files that #45947 deliberately left writable.
|
||||
WRITABLE_CONTROL_FILES = ("auth.json", "auth.lock", "config.yaml", "webhook_subscriptions.json")
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def hermes_layout(tmp_path, monkeypatch):
|
||||
@@ -37,63 +52,53 @@ def _touch(base: Path, rel: str) -> Path:
|
||||
return p
|
||||
|
||||
|
||||
def test_every_read_credential_file_is_write_denied(hermes_layout):
|
||||
root, profile = hermes_layout
|
||||
for name in fs._CREDENTIAL_FILE_NAMES:
|
||||
for base in (profile, root):
|
||||
path = _touch(base, name)
|
||||
assert fs.is_write_denied(str(path)), f"write allowed: {path}"
|
||||
|
||||
|
||||
def test_every_read_denied_dir_is_write_denied(hermes_layout):
|
||||
root, profile = hermes_layout
|
||||
for sub, _, _ in fs._READ_DENIED_DIRS:
|
||||
for base in (profile, root):
|
||||
path = _touch(base, f"{sub}/inside.bin")
|
||||
assert fs.is_write_denied(str(path)), f"write allowed: {path}"
|
||||
|
||||
|
||||
def test_encrypted_bitwarden_cache_stays_write_denied(hermes_layout):
|
||||
@pytest.mark.parametrize("name", SECRET_FILES)
|
||||
def test_secret_files_are_write_denied(hermes_layout, name):
|
||||
root, profile = hermes_layout
|
||||
for base in (profile, root):
|
||||
path = _touch(base, "cache/bws_cache.enc.json")
|
||||
assert fs.is_write_denied(str(path))
|
||||
path = _touch(base, name)
|
||||
assert fs.is_write_denied(str(path)), f"write allowed: {path}"
|
||||
|
||||
|
||||
def test_config_yaml_stays_writable(hermes_layout):
|
||||
@pytest.mark.parametrize("sub", fs._WRITE_DENIED_SECRET_DIRS)
|
||||
def test_secret_dirs_are_write_denied(hermes_layout, sub):
|
||||
root, profile = hermes_layout
|
||||
for base in (profile, root):
|
||||
path = _touch(base, f"{sub}/inside.bin")
|
||||
assert fs.is_write_denied(str(path)), f"write allowed: {path}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name", WRITABLE_CONTROL_FILES)
|
||||
def test_control_files_stay_writable(hermes_layout, name):
|
||||
"""#45947 freed these on purpose; re-blocking them is a policy regression."""
|
||||
root, profile = hermes_layout
|
||||
for base in (profile, root):
|
||||
path = _touch(base, name)
|
||||
assert fs.is_write_denied(str(path)) is False, f"write denied: {path}"
|
||||
|
||||
|
||||
def test_every_write_denied_secret_is_read_denied(hermes_layout):
|
||||
"""Write denies are a subset of read denies — never a superset."""
|
||||
_, profile = hermes_layout
|
||||
path = _touch(profile, "config.yaml")
|
||||
for name in SECRET_FILES:
|
||||
if name == "cache/bws_cache.enc.json":
|
||||
continue # encrypted sibling: write-only extra, plaintext is the read-denied one
|
||||
path = _touch(profile, name)
|
||||
assert fs.get_read_block_error(str(path)) is not None, f"not read-denied: {name}"
|
||||
|
||||
|
||||
def test_nested_secret_basename_stays_writable(hermes_layout):
|
||||
_, profile = hermes_layout
|
||||
path = _touch(profile, "skills/my-skill/.env.example")
|
||||
assert fs.is_write_denied(str(path)) is False
|
||||
|
||||
|
||||
def test_nested_auth_json_stays_writable(hermes_layout):
|
||||
_, profile = hermes_layout
|
||||
path = _touch(profile, "skills/my-skill/auth.json")
|
||||
assert fs.is_write_denied(str(path)) is False
|
||||
|
||||
|
||||
def test_auth_json_outside_hermes_home_stays_writable(hermes_layout, tmp_path):
|
||||
def test_secret_outside_hermes_home_is_not_denied_by_this_rule(hermes_layout, tmp_path):
|
||||
project = tmp_path / "myproject"
|
||||
path = _touch(project, "auth.json")
|
||||
path = _touch(project, "cache/bws_cache.json")
|
||||
assert fs.is_write_denied(str(path)) is False
|
||||
|
||||
|
||||
def test_write_file_does_not_replace_auth_json(hermes_layout):
|
||||
_, profile = hermes_layout
|
||||
from tools.environments.local import LocalEnvironment
|
||||
from tools.file_operations import ShellFileOperations
|
||||
|
||||
target = _touch(profile, "auth.json")
|
||||
target.write_text("{\"ok\": true}\n", encoding="utf-8")
|
||||
ops = ShellFileOperations(
|
||||
LocalEnvironment(cwd=str(profile)), cwd=str(profile)
|
||||
)
|
||||
res = ops.write_file(str(target), "{\"pwned\": true}\n")
|
||||
assert res.error is not None
|
||||
assert "protected system/credential file" in res.error
|
||||
assert target.read_text(encoding="utf-8") == "{\"ok\": true}\n"
|
||||
|
||||
|
||||
def test_write_file_does_not_replace_vault_key(hermes_layout):
|
||||
_, profile = hermes_layout
|
||||
from tools.environments.local import LocalEnvironment
|
||||
@@ -101,10 +106,22 @@ def test_write_file_does_not_replace_vault_key(hermes_layout):
|
||||
|
||||
target = _touch(profile, "vault/vault.key")
|
||||
target.write_text("not-a-real-key\n", encoding="utf-8")
|
||||
ops = ShellFileOperations(
|
||||
LocalEnvironment(cwd=str(profile)), cwd=str(profile)
|
||||
)
|
||||
ops = ShellFileOperations(LocalEnvironment(cwd=str(profile)), cwd=str(profile))
|
||||
res = ops.write_file(str(target), "stolen\n")
|
||||
assert res.error is not None
|
||||
assert "protected system/credential file" in res.error
|
||||
assert target.read_text(encoding="utf-8") == "not-a-real-key\n"
|
||||
|
||||
|
||||
def test_write_file_does_not_replace_google_oauth(hermes_layout):
|
||||
_, profile = hermes_layout
|
||||
from tools.environments.local import LocalEnvironment
|
||||
from tools.file_operations import ShellFileOperations
|
||||
|
||||
target = _touch(profile, "auth/google_oauth.json")
|
||||
target.write_text('{"ok": true}\n', encoding="utf-8")
|
||||
ops = ShellFileOperations(LocalEnvironment(cwd=str(profile)), cwd=str(profile))
|
||||
res = ops.write_file(str(target), '{"pwned": true}\n')
|
||||
assert res.error is not None
|
||||
assert "protected system/credential file" in res.error
|
||||
assert target.read_text(encoding="utf-8") == '{"ok": true}\n'
|
||||
|
||||
@@ -87,8 +87,9 @@ class TestWriteAllowed:
|
||||
assert _is_write_denied("/tmp/safe_file.txt") is False
|
||||
|
||||
|
||||
def test_hermes_config_yaml_requested_writable(self):
|
||||
def test_hermes_control_files_requested_writable(self):
|
||||
from hermes_constants import get_hermes_home
|
||||
|
||||
home = get_hermes_home()
|
||||
assert _is_write_denied(str(home / "config.yaml")) is False, "config.yaml should be writable"
|
||||
for name in ["auth.json", "config.yaml", "webhook_subscriptions.json"]:
|
||||
assert _is_write_denied(str(home / name)) is False, f"{name} should be writable"
|
||||
|
||||
@@ -347,7 +347,7 @@ These categories are always denied, even when `HERMES_WRITE_SAFE_ROOT` is unset:
|
||||
| Category | Examples |
|
||||
|----------|----------|
|
||||
| OS credential stores | `~/.ssh/` (keys, `authorized_keys`), `~/.aws/`, `~/.kube/`, `/etc/sudoers`, `~/.netrc` |
|
||||
| Hermes credential stores | `auth.json`, `.env`, `.anthropic_oauth.json`, `webhook_subscriptions.json`, `auth/google_oauth.json`, Bitwarden cache (`cache/bws_cache.json`, `cache/bws_cache.enc.json`), `vault/`, `browser-profile/`, `mcp-tokens/`, `pairing/` under HERMES_HOME (active profile and global root) |
|
||||
| Hermes secret stores | `.env`, `.anthropic_oauth.json`, `auth/google_oauth.json`, Bitwarden cache (`cache/bws_cache.json`, `cache/bws_cache.enc.json`), `vault/`, `browser-profile/`, `mcp-tokens/`, `pairing/` under HERMES_HOME (active profile and global root). Control files (`auth.json`, `config.yaml`, `webhook_subscriptions.json`) are read-denied but stay writable. |
|
||||
| Project secret files | `.env`, `.env.local`, `.env.production`, `.envrc` anywhere on disk |
|
||||
| Windows NT/device-namespace paths | `\??\...`, `\\.\...`, `\\?\UNC\...`, `\\?\GLOBALROOT...` — rejected for both reads and writes on every platform. On Windows, merely *resolving* such a path (e.g. `\??\UNC\host\share`) triggers outbound SMB authentication and can leak the user's NTLM hash; the prefixes also bypass normal path normalization. Ordinary extended-length local paths (`\\?\C:\...`) and plain UNC shares (`\\server\share`) are unaffected. |
|
||||
|
||||
|
||||
Reference in New Issue
Block a user