fix(file-safety): one coordinate helper for the guard homes; trim tests to two invariants
Follow-up to the salvaged #113629 commit: - `_homes_and_resolved()` replaces `_home_and_resolved()`: both write-guard predicates now read the same (guard homes, resolved path) pair instead of computing an unused process-home coordinate next to `_guard_homes()`. - Drop the `tests/tools/test_write_deny.py` end-to-end hunk: it drove `write_file_tool` at probe files under the REAL user's `~/.ssh` / `~/.aws` (created-then-unlinked). Tests never touch the real home; the invariant is pinned by the read-only predicate tests instead. - Collapse the five predicate tests into two invariants: every home a write can land in is guarded (real home, profile home, `~`, `~root`, `~/.ssh/config` still approval-gated), and benign paths stay writable.
This commit is contained in:
@@ -69,11 +69,6 @@ def _resolve_target(path: str) -> Optional[Path]:
|
||||
return None
|
||||
|
||||
|
||||
def _home_and_resolved(path: str) -> tuple[str, str]:
|
||||
"""``(realpath(~), realpath(expanduser(path)))`` — the write-guard coordinate pair."""
|
||||
return tuple(os.path.realpath(os.path.expanduser(p)) for p in ("~", str(path)))
|
||||
|
||||
|
||||
def _guard_homes(path: str = "") -> set[str]:
|
||||
"""Every home the write guards must cover. Process ``~`` alone is wrong whenever the
|
||||
process HOME is not the OS user's real home — ``TERMINAL_HOME_MODE=profile``,
|
||||
@@ -100,6 +95,11 @@ def _guard_homes(path: str = "") -> set[str]:
|
||||
return {os.path.realpath(h) for h in homes}
|
||||
|
||||
|
||||
def _homes_and_resolved(path: str) -> tuple[set[str], str]:
|
||||
"""``(guard homes, realpath(expanduser(path)))`` — the write-guard coordinate pair."""
|
||||
return _guard_homes(path), os.path.realpath(os.path.expanduser(str(path)))
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Windows NT-namespace path guard
|
||||
#
|
||||
@@ -254,11 +254,10 @@ def _classify_write_denial(path: str) -> Optional[str]:
|
||||
# prefixes defeat string-prefix denylist comparison after normalization.
|
||||
if is_nt_namespace_path(path):
|
||||
return "nt_namespace"
|
||||
_home, resolved = _home_and_resolved(path)
|
||||
homes, resolved = _homes_and_resolved(path)
|
||||
|
||||
# Approval-gated paths are allowed at this layer so interactive tools can
|
||||
# prompt; checked first so the ``.ssh/`` prefix deny doesn't swallow them.
|
||||
homes = _guard_homes(path)
|
||||
if any(resolved in build_write_approval_paths(home) for home in homes):
|
||||
return None
|
||||
|
||||
@@ -304,8 +303,8 @@ def get_write_denied_error(path: str, *, verb: str = "Write") -> Optional[str]:
|
||||
def is_write_approval_required(path: str) -> bool:
|
||||
"""True if ``path`` is approval-gated (``~/.ssh/config``): interactive callers
|
||||
prompt, callers without a channel treat it as a block (fail closed)."""
|
||||
_home, resolved = _home_and_resolved(path)
|
||||
return any(resolved in build_write_approval_paths(home) for home in _guard_homes(path))
|
||||
homes, resolved = _homes_and_resolved(path)
|
||||
return any(resolved in build_write_approval_paths(home) for home in homes)
|
||||
|
||||
|
||||
# Secret-bearing project-local env file basenames, blocked anywhere on disk.
|
||||
|
||||
@@ -9,6 +9,7 @@ drifted: ``auth/google_oauth.json``, the plaintext Bitwarden cache, ``vault/`` a
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
@@ -58,12 +59,9 @@ def test_control_files_and_lookalikes_outside_home_stay_writable(hermes_layout,
|
||||
|
||||
|
||||
class TestProfileHomeProcessHome:
|
||||
"""The write guard must cover the OS user's real home even when the process HOME is
|
||||
pinned to ``{HERMES_HOME}/home`` (TERMINAL_HOME_MODE=profile / container / spawned
|
||||
worker). Anchoring the deny and approval lists on ``expanduser("~")`` alone left the
|
||||
real home's credential paths writable via absolute paths — while file tools resolve
|
||||
``~`` through ``get_subprocess_home()`` and can land a ``~``-spelled write on the real
|
||||
home too."""
|
||||
"""With the process HOME pinned to ``{HERMES_HOME}/home`` (TERMINAL_HOME_MODE=profile,
|
||||
containers, spawned workers) the write guards must still cover every home a write can
|
||||
land in: the OS user's real home, the profile home and ``~name`` accounts."""
|
||||
|
||||
@pytest.fixture()
|
||||
def profile_home_env(self, tmp_path, monkeypatch):
|
||||
@@ -75,29 +73,24 @@ class TestProfileHomeProcessHome:
|
||||
monkeypatch.setattr(fs, "_hermes_root_path", lambda: profile.parent)
|
||||
return profile
|
||||
|
||||
def test_real_home_credentials_denied(self, profile_home_env):
|
||||
def test_every_home_is_guarded(self, profile_home_env):
|
||||
import pwd
|
||||
|
||||
real_home = Path(pwd.getpwuid(__import__("os").getuid()).pw_dir)
|
||||
real_home = Path(pwd.getpwuid(os.getuid()).pw_dir)
|
||||
for rel in (".aws/credentials", ".ssh/id_ed25519", ".netrc", ".config/gh/hosts.yml"):
|
||||
assert fs.is_write_denied(str(real_home / rel)), rel
|
||||
assert fs.is_write_denied(str(profile_home_env / "home" / rel)), rel
|
||||
assert fs.is_write_denied("~/.aws/credentials")
|
||||
assert fs.is_write_denied("~root/.ssh/authorized_keys")
|
||||
# ``~/.ssh/config`` stays approval-gated (not hard-denied) on the real home too.
|
||||
assert fs.is_write_approval_required(str(real_home / ".ssh" / "config"))
|
||||
assert fs.is_write_denied(str(real_home / ".ssh" / "config")) is False
|
||||
|
||||
def test_real_home_ssh_config_still_approval_gated(self, profile_home_env):
|
||||
def test_benign_paths_stay_writable(self, profile_home_env, tmp_path):
|
||||
import pwd
|
||||
|
||||
real_home = Path(pwd.getpwuid(__import__("os").getuid()).pw_dir)
|
||||
assert fs.is_write_approval_required(str(real_home / ".ssh" / "config"))
|
||||
|
||||
def test_profile_home_credentials_also_denied(self, profile_home_env):
|
||||
"""A pinned-HOME process still guards the profile home children see as ``~``."""
|
||||
for rel in (".aws/credentials", ".ssh/id_rsa"):
|
||||
assert fs.is_write_denied(str(profile_home_env / "home" / rel)), rel
|
||||
|
||||
def test_tilde_spelling_still_gated(self, profile_home_env):
|
||||
assert fs.is_write_denied("~/.aws/credentials")
|
||||
assert fs.is_write_approval_required("~/.ssh/config")
|
||||
|
||||
def test_benign_path_unaffected(self, profile_home_env, tmp_path):
|
||||
benign = tmp_path / "scratch" / "notes.txt"
|
||||
assert fs.is_write_denied(str(benign)) is False
|
||||
assert fs.is_write_approval_required(str(benign)) is False
|
||||
real_home = Path(pwd.getpwuid(os.getuid()).pw_dir)
|
||||
for benign in (tmp_path / "scratch" / "notes.txt", real_home / "projects" / "notes.md"):
|
||||
assert fs.is_write_denied(str(benign)) is False, benign
|
||||
assert fs.is_write_approval_required(str(benign)) is False, benign
|
||||
assert fs.is_write_denied("~nosuchuser-hopefully/.ssh/authorized_keys") is False
|
||||
|
||||
@@ -1,13 +1,10 @@
|
||||
"""Tests for _is_write_denied() — verifies deny list blocks sensitive paths on all platforms."""
|
||||
|
||||
import json
|
||||
import os
|
||||
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from agent.file_safety import is_write_denied as _is_write_denied
|
||||
|
||||
|
||||
@@ -96,64 +93,3 @@ class TestWriteAllowed:
|
||||
home = get_hermes_home()
|
||||
for name in ["auth.json", "config.yaml", "webhook_subscriptions.json"]:
|
||||
assert _is_write_denied(str(home / name)) is False, f"{name} should be writable"
|
||||
|
||||
|
||||
class TestProfileHomeE2E:
|
||||
"""End-to-end through ``write_file_tool``: with the process HOME pinned to
|
||||
``{HERMES_HOME}/home`` (TERMINAL_HOME_MODE=profile / container / spawned worker),
|
||||
a write aimed at the OS user's real home must still hit the credential guards —
|
||||
before the fix, absolute real-home paths sailed through untouched."""
|
||||
|
||||
@pytest.fixture()
|
||||
def pinned_profile_home(self, tmp_path, monkeypatch):
|
||||
profile = tmp_path / "profile"
|
||||
(profile / "home").mkdir(parents=True)
|
||||
monkeypatch.setenv("HERMES_HOME", str(profile))
|
||||
monkeypatch.setenv("HOME", str(profile / "home"))
|
||||
return profile
|
||||
|
||||
@staticmethod
|
||||
def _real_home() -> Path:
|
||||
import pwd
|
||||
|
||||
return Path(pwd.getpwuid(os.getuid()).pw_dir)
|
||||
|
||||
def test_absolute_real_home_write_denied_end_to_end(self, pinned_profile_home):
|
||||
from tools.file_tools import write_file_tool
|
||||
|
||||
target = self._real_home() / ".ssh" / "e2e_guard_probe_key"
|
||||
assert not target.exists()
|
||||
try:
|
||||
result = json.loads(write_file_tool(str(target), "not-a-key"))
|
||||
assert result.get("error"), f"write slipped through: {result}"
|
||||
assert not target.exists()
|
||||
finally:
|
||||
target.unlink(missing_ok=True)
|
||||
|
||||
def test_tilde_write_denied_end_to_end(self, pinned_profile_home):
|
||||
"""``~`` resolves to the real home via _expand_tilde's repair — and is denied."""
|
||||
from tools.file_tools import write_file_tool
|
||||
|
||||
target = self._real_home() / ".aws" / "e2e_guard_probe_credentials"
|
||||
assert not target.exists()
|
||||
try:
|
||||
result = json.loads(write_file_tool("~/.aws/e2e_guard_probe_credentials", "x"))
|
||||
assert result.get("error"), f"write slipped through: {result}"
|
||||
assert not target.exists()
|
||||
finally:
|
||||
target.unlink(missing_ok=True)
|
||||
|
||||
def test_named_user_tilde_denied(self, pinned_profile_home):
|
||||
"""``~root/...`` resolves to another account's home — still a credential write."""
|
||||
import agent.file_safety as fs
|
||||
assert fs.is_write_denied("~root/.ssh/authorized_keys") is True
|
||||
assert fs.is_write_denied("~nosuchuser-hopefully/.ssh/authorized_keys") is False
|
||||
|
||||
def test_benign_write_still_lands(self, pinned_profile_home, tmp_path):
|
||||
from tools.file_tools import write_file_tool
|
||||
|
||||
target = tmp_path / "scratch" / "ok.txt"
|
||||
target.parent.mkdir(parents=True)
|
||||
result = json.loads(write_file_tool(str(target), "hello"))
|
||||
assert not result.get("error"), result
|
||||
assert target.read_text() == "hello"
|
||||
|
||||
Reference in New Issue
Block a user