From 55d826cceff1a024d141085cacf8a2fad02cd9f7 Mon Sep 17 00:00:00 2001 From: HexLab98 Date: Sun, 12 Jul 2026 20:34:26 +0700 Subject: [PATCH] fix(file-safety): distinguish safe-root write denial from credential blocks Return actionable errors when HERMES_WRITE_SAFE_ROOT blocks a path instead of labeling every denial as a protected credential file. Wire the helper through write_file, patch, delete/move, and the Copilot ACP shim; sync docs examples. --- agent/copilot_acp_client.py | 9 +++-- agent/file_safety.py | 35 ++++++++++++++----- tests/agent/test_copilot_acp_client.py | 7 +++- tests/tools/test_file_write_safety.py | 32 +++++++++++++++++ tools/file_operations.py | 21 ++++++----- .../docs/reference/environment-variables.md | 2 +- website/docs/user-guide/configuration.md | 4 +-- website/docs/user-guide/security.md | 2 +- .../reference/environment-variables.md | 2 +- 9 files changed, 87 insertions(+), 27 deletions(-) diff --git a/agent/copilot_acp_client.py b/agent/copilot_acp_client.py index ce3ec2c5c400..5e095af3902b 100644 --- a/agent/copilot_acp_client.py +++ b/agent/copilot_acp_client.py @@ -26,7 +26,7 @@ from openai.types.chat.chat_completion_message_tool_call import ( Function, ) -from agent.file_safety import get_read_block_error, is_write_denied +from agent.file_safety import get_read_block_error, get_write_denied_error from agent.redact import redact_sensitive_text from tools.environments.local import hermes_subprocess_env @@ -727,10 +727,9 @@ class CopilotACPClient: elif method == "fs/write_text_file": try: path = _ensure_path_within_cwd(str(params.get("path") or ""), cwd) - if is_write_denied(str(path)): - raise PermissionError( - f"Write denied: '{path}' is a protected system/credential file." - ) + denied = get_write_denied_error(str(path)) + if denied: + raise PermissionError(denied) path.parent.mkdir(parents=True, exist_ok=True) path.write_text(str(params.get("content") or "")) response = { diff --git a/agent/file_safety.py b/agent/file_safety.py index d7e20ee5f0b9..2f4b3666e28d 100644 --- a/agent/file_safety.py +++ b/agent/file_safety.py @@ -95,16 +95,16 @@ def get_safe_write_roots() -> set[str]: return roots -def is_write_denied(path: str) -> bool: - """Return True if path is blocked by the write denylist or safe root.""" +def _classify_write_denial(path: str) -> Optional[str]: + """Return ``'credential'``, ``'safe_root'``, or ``None`` if writes are allowed.""" home = os.path.realpath(os.path.expanduser("~")) resolved = os.path.realpath(os.path.expanduser(str(path))) if resolved in build_write_denied_paths(home): - return True + return "credential" for prefix in build_write_denied_prefixes(home): if resolved.startswith(prefix): - return True + return "credential" mcp_tokens_dir_name = "mcp-tokens" @@ -121,13 +121,13 @@ def is_write_denied(path: str) -> bool: try: mcp_real = os.path.realpath(os.path.join(base_real, mcp_tokens_dir_name)) if resolved == mcp_real or resolved.startswith(mcp_real + os.sep): - return True + return "credential" except Exception: pass try: pairing_real = os.path.realpath(os.path.join(base_real, "pairing")) if resolved == pairing_real or resolved.startswith(pairing_real + os.sep): - return True + return "credential" except Exception: pass @@ -139,9 +139,28 @@ def is_write_denied(path: str) -> bool: allowed = True break if not allowed: - return True + return "safe_root" - return False + return None + + +def is_write_denied(path: str) -> bool: + """Return True if path is blocked by the write denylist or safe root.""" + return _classify_write_denial(path) is not None + + +def get_write_denied_error(path: str, *, verb: str = "Write") -> Optional[str]: + """Return a user/model-facing error when writes to ``path`` are blocked.""" + denial = _classify_write_denial(path) + if denial is None: + return None + if denial == "safe_root": + roots_display = os.pathsep.join(sorted(get_safe_write_roots())) + return ( + f"{verb} denied: '{path}' is outside HERMES_WRITE_SAFE_ROOT " + f"({roots_display}). Unset the variable or add this path's directory prefix." + ) + return f"{verb} denied: '{path}' is a protected system/credential file." # Common secret-bearing project-local environment file basenames. diff --git a/tests/agent/test_copilot_acp_client.py b/tests/agent/test_copilot_acp_client.py index 5f2d3c234fe3..1d1d28cc5421 100644 --- a/tests/agent/test_copilot_acp_client.py +++ b/tests/agent/test_copilot_acp_client.py @@ -209,7 +209,11 @@ class CopilotACPClientSafetyTests(unittest.TestCase): target = home / ".ssh" / "id_rsa" target.parent.mkdir(parents=True, exist_ok=True) - with patch("agent.copilot_acp_client.is_write_denied", return_value=True, create=True): + with patch( + "agent.copilot_acp_client.get_write_denied_error", + return_value="Write denied: protected", + create=True, + ): response = self._dispatch( { "jsonrpc": "2.0", @@ -248,6 +252,7 @@ class CopilotACPClientSafetyTests(unittest.TestCase): ) self.assertIn("error", response) + self.assertIn("HERMES_WRITE_SAFE_ROOT", str(response["error"])) self.assertFalse(outside.exists()) diff --git a/tests/tools/test_file_write_safety.py b/tests/tools/test_file_write_safety.py index ae766a7a723e..655ba407b652 100644 --- a/tests/tools/test_file_write_safety.py +++ b/tests/tools/test_file_write_safety.py @@ -168,6 +168,38 @@ class TestMultipleSafeWriteRoots: assert _is_write_denied(str(inside)) is False +class TestGetWriteDeniedError: + """get_write_denied_error() should distinguish credential vs safe-root blocks.""" + + def test_credential_path_message(self): + from agent.file_safety import get_write_denied_error + + err = get_write_denied_error(os.path.expanduser("~/.ssh/id_rsa")) + assert err is not None + assert "protected system/credential file" in err + assert "HERMES_WRITE_SAFE_ROOT" not in err + + def test_safe_root_message(self, tmp_path: Path, monkeypatch): + from agent.file_safety import get_write_denied_error + + safe_root = tmp_path / "workspace" + outside = tmp_path / "outside.txt" + os.makedirs(safe_root, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", str(safe_root)) + err = get_write_denied_error(str(outside)) + assert err is not None + assert "outside HERMES_WRITE_SAFE_ROOT" in err + assert str(safe_root) in err + assert "protected system/credential file" not in err + + def test_allowed_path_returns_none(self, tmp_path: Path): + from agent.file_safety import get_write_denied_error + + target = tmp_path / "ok.txt" + assert get_write_denied_error(str(target)) is None + + class TestCheckSensitivePathMacOSBypass: """Verify _check_sensitive_path blocks /private/etc paths (issue #8734).""" diff --git a/tools/file_operations.py b/tools/file_operations.py index 76446befaa5e..994723cf583b 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -37,6 +37,7 @@ from tools.binary_extensions import BINARY_EXTENSIONS from agent.file_safety import ( build_write_denied_paths, build_write_denied_prefixes, + get_write_denied_error, is_write_denied as _shared_is_write_denied, ) @@ -1289,8 +1290,9 @@ class ShellFileOperations(FileOperations): def _python_delete(self, path: str, recursive: bool) -> WriteResult: path = self._expand_path(path) - if _is_write_denied(path): - return WriteResult(error=f"Delete denied: {path} is a protected path") + denied = get_write_denied_error(path, verb="Delete") + if denied: + return WriteResult(error=denied) # We can't shell out to ``rm`` here — it doesn't exist on Windows # ``cmd.exe`` or PowerShell, so this code path is what's left when @@ -1335,8 +1337,9 @@ class ShellFileOperations(FileOperations): src = self._expand_path(src) dst = self._expand_path(dst) for p in (src, dst): - if _is_write_denied(p): - return WriteResult(error=f"Move denied: {p} is a protected path") + denied = get_write_denied_error(p, verb="Move") + if denied: + return WriteResult(error=denied) result = self._exec( f"mv {self._escape_shell_arg(src)} {self._escape_shell_arg(dst)}" ) @@ -1383,8 +1386,9 @@ class ShellFileOperations(FileOperations): path = self._expand_path(path) # Block writes to sensitive paths - if _is_write_denied(path): - return WriteResult(error=f"Write denied: '{path}' is a protected system/credential file.") + denied = get_write_denied_error(path) + if denied: + return WriteResult(error=denied) # ── Fail-closed pre-write syntax gate ─────────────────────────── # Validate the CANDIDATE content BEFORE any bytes touch disk — @@ -1566,8 +1570,9 @@ class ShellFileOperations(FileOperations): path = self._expand_path(path) # Block writes to sensitive paths - if _is_write_denied(path): - return PatchResult(error=f"Write denied: '{path}' is a protected system/credential file.") + denied = get_write_denied_error(path) + if denied: + return PatchResult(error=denied) # Read current content read_cmd = f"cat {self._escape_shell_arg(path)} 2>/dev/null" diff --git a/website/docs/reference/environment-variables.md b/website/docs/reference/environment-variables.md index 5e6f1fc0de0c..0a449252b634 100644 --- a/website/docs/reference/environment-variables.md +++ b/website/docs/reference/environment-variables.md @@ -766,7 +766,7 @@ When this variable is set, `write_file` and `patch` may only target paths inside The official Docker image sets `HERMES_WRITE_SAFE_ROOT=/opt/data` alongside `HERMES_HOME=/opt/data` so the agent cannot escape the mounted data volume. -**Do not add this to `~/.hermes/.env` unless you intend to sandbox writes.** A common mistake is pointing it at a project directory while expecting the agent to edit `~/.hermes/cron/jobs.json`, `~/.hermes/skills/`, or scripts under a profile — those paths are outside the sandbox and every `write_file`/`patch` to them fails. The tool error currently reads `Write denied: '…' is a protected system/credential file.` for all blocked writes (including safe-root violations); check `echo $HERMES_WRITE_SAFE_ROOT` when you see that message. +**Do not add this to `~/.hermes/.env` unless you intend to sandbox writes.** A common mistake is pointing it at a project directory while expecting the agent to edit `~/.hermes/cron/jobs.json`, `~/.hermes/skills/`, or scripts under a profile — those paths are outside the sandbox and every `write_file`/`patch` to them fails with an `outside HERMES_WRITE_SAFE_ROOT` error. To allow both a workspace and Hermes state, list both prefixes (order does not matter): diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 1d2e3b672979..40748ce23197 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -1457,8 +1457,8 @@ Example footer when writes are blocked: ``` ⚠️ File-mutation verifier: 2 file(s) were NOT modified this turn despite any wording above that may suggest otherwise. Run `git status` or `read_file` to confirm. - • ~/.hermes/cron/jobs.json — [patch] Write denied: '…' is a protected system/credential file. - • ~/.hermes/scripts/monitor.py — [write_file] Write denied: '…' is a protected system/credential file. + • ~/.hermes/cron/jobs.json — [patch] Write denied: '…' is outside HERMES_WRITE_SAFE_ROOT (/path/to/project) + • ~/.hermes/scripts/monitor.py — [write_file] Write denied: '…' is outside HERMES_WRITE_SAFE_ROOT (/path/to/project) ``` If writes to Hermes state (cron jobs, skills, scripts under `~/.hermes/`) are failing, check whether `HERMES_WRITE_SAFE_ROOT` is set in your environment. For cron changes, use the `cronjob` tool or `hermes cron edit` instead of patching `jobs.json` directly. diff --git a/website/docs/user-guide/security.md b/website/docs/user-guide/security.md index d2dd8306512b..a5488b6250c8 100644 --- a/website/docs/user-guide/security.md +++ b/website/docs/user-guide/security.md @@ -249,7 +249,7 @@ These categories are always denied, even when `HERMES_WRITE_SAFE_ROOT` is unset: Sensitive paths inside the safe root are still blocked — pointing `HERMES_WRITE_SAFE_ROOT` at `$HOME` does not allow writing `~/.ssh/id_rsa`. -The tool error message is always `Write denied: '…' is a protected system/credential file.` even when the actual reason is the safe-root sandbox below. If the path does not look like a credential file, check `echo $HERMES_WRITE_SAFE_ROOT`. +Safe-root violations return `Write denied: '…' is outside HERMES_WRITE_SAFE_ROOT (…)`. Credential-path blocks use `Write denied: '…' is a protected system/credential file.` ### HERMES_WRITE_SAFE_ROOT (optional sandbox) diff --git a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/environment-variables.md b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/environment-variables.md index c405e6312de5..4b15a846f147 100644 --- a/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/environment-variables.md +++ b/website/i18n/zh-Hans/docusaurus-plugin-content-docs/current/reference/environment-variables.md @@ -580,7 +580,7 @@ Graph 事件（Teams 会议、日历、聊天等）的入站变更通知监听 官方 Docker 镜像会设置 `HERMES_WRITE_SAFE_ROOT=/opt/data` 与 `HERMES_HOME=/opt/data`，防止 agent 逃出挂载的数据卷。 -**除非有意沙箱化写入，否则不要将此变量加入 `~/.hermes/.env`。** 常见错误是将其指向项目目录，却期望 agent 编辑 `~/.hermes/cron/jobs.json`、`~/.hermes/skills/` 或 profile 下的脚本——这些路径在沙箱外，每次 `write_file`/`patch` 都会失败。当前所有被阻止的写入（包括 safe-root 违规）都显示 `Write denied: '…' is a protected system/credential file.`；看到此消息时请检查 `echo $HERMES_WRITE_SAFE_ROOT`。 +**除非有意沙箱化写入，否则不要将此变量加入 `~/.hermes/.env`。** 常见错误是将其指向项目目录，却期望 agent 编辑 `~/.hermes/cron/jobs.json`、`~/.hermes/skills/` 或 profile 下的脚本——这些路径在沙箱外，每次 `write_file`/`patch` 都会失败并返回 `outside HERMES_WRITE_SAFE_ROOT` 错误。 若需同时允许工作区和 Hermes 状态目录，列出两个前缀（顺序无关）：