From 1c0d95badbac31fc6bc720bb366897da47824bd1 Mon Sep 17 00:00:00 2001 From: NUXER <262510595+Rockey011@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:47:14 +0200 Subject: [PATCH] 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 --- agent/file_safety.py | 30 +++- .../test_file_safety_write_credentials.py | 129 ++++++++++-------- tests/tools/test_write_deny.py | 5 +- website/docs/user-guide/security.md | 2 +- 4 files changed, 100 insertions(+), 66 deletions(-) diff --git a/agent/file_safety.py b/agent/file_safety.py index bda39ad27f..f72fe27054 100644 --- a/agent/file_safety.py +++ b/agent/file_safety.py @@ -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" diff --git a/tests/agent/test_file_safety_write_credentials.py b/tests/agent/test_file_safety_write_credentials.py index b83e4ce4f3..c30de8f096 100644 --- a/tests/agent/test_file_safety_write_credentials.py +++ b/tests/agent/test_file_safety_write_credentials.py @@ -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' diff --git a/tests/tools/test_write_deny.py b/tests/tools/test_write_deny.py index d31903048f..c71a509692 100644 --- a/tests/tools/test_write_deny.py +++ b/tests/tools/test_write_deny.py @@ -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" diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index 77b3ef1935..bc765ed352 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -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. |