From cdb1dfbc494a7ce2f44c4ef5b5909fd15683de07 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 27 Jun 2026 03:33:57 +0530 Subject: [PATCH] fix: use os.pathsep, add tests, update tips for multi-root support - Use os.pathsep instead of literal ':' so Windows paths (C:\dir) and the Windows separator ';' work correctly. - Add 9 tests covering multi-root behavior: writes inside first/second root, writes outside all roots, trailing/leading/double separators, all-separators edge case, static deny priority, duplicate dedup. - Update hermes_cli/tips.py tip string to mention multiple paths. - Update docs to mention os.pathsep / ; on Windows. Follow-up for salvaged PR #49557. --- agent/file_safety.py | 8 +- hermes_cli/tips.py | 2 +- tests/tools/test_file_write_safety.py | 89 +++++++++++++++++++ .../docs/reference/environment-variables.md | 2 +- .../reference/environment-variables.md | 2 +- 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/agent/file_safety.py b/agent/file_safety.py index 0d71400c86e..482c4217c85 100644 --- a/agent/file_safety.py +++ b/agent/file_safety.py @@ -79,17 +79,18 @@ def build_write_denied_prefixes(home: str) -> list[str]: def get_safe_write_roots() -> set[str]: """Return resolved HERMES_WRITE_SAFE_ROOT paths. Supports multiple directories - separated by ':' (Unix PATH-style). E.g., '/opt/data:/var/www/html'.""" + separated by ``os.pathsep`` (``:`` on Unix, ``;`` on Windows). + E.g., ``/opt/data:/var/www/html`` on Unix, ``C:\\data;D:\\www`` on Windows.""" env = os.getenv("HERMES_WRITE_SAFE_ROOT", "") if not env: return set() roots: set[str] = set() - for path in env.split(":"): + for path in env.split(os.pathsep): if path: try: resolved = os.path.realpath(os.path.expanduser(path)) roots.add(resolved) - except Exception: + except (OSError, ValueError): continue return roots @@ -132,7 +133,6 @@ def is_write_denied(path: str) -> bool: safe_roots = get_safe_write_roots() if safe_roots: - # Allow write if path is under ANY of the safe roots allowed = False for safe_root in safe_roots: if resolved == safe_root or resolved.startswith(safe_root + os.sep): diff --git a/hermes_cli/tips.py b/hermes_cli/tips.py index fed6fe36e4e..d62bc35fb8b 100644 --- a/hermes_cli/tips.py +++ b/hermes_cli/tips.py @@ -389,7 +389,7 @@ TIPS = [ # --- Env Vars & Config Gates --- "display.tool_progress_command: true exposes /verbose on messaging platforms; it's CLI-only by default.", 'HERMES_BACKGROUND_NOTIFICATIONS=result only pings when background tasks finish (vs all/error/off).', - 'HERMES_WRITE_SAFE_ROOT restricts write_file and patch to a directory prefix; writes outside require approval.', + 'HERMES_WRITE_SAFE_ROOT restricts write_file/patch to directory prefixes; multiple paths via os.pathsep (: or ;).', 'HERMES_IGNORE_RULES skips auto-injection of AGENTS.md, SOUL.md, .cursorrules, memory, and preloaded skills.', 'HERMES_ACCEPT_HOOKS auto-approves unseen shell hooks declared in config.yaml without a TTY prompt.', 'auxiliary.goal_judge.model routes the /goal judge to a cheap fast model to keep loop cost near zero.', diff --git a/tests/tools/test_file_write_safety.py b/tests/tools/test_file_write_safety.py index ac44dd1bc6b..ae766a7a723 100644 --- a/tests/tools/test_file_write_safety.py +++ b/tests/tools/test_file_write_safety.py @@ -79,6 +79,95 @@ class TestSafeWriteRoot: assert _is_write_denied(os.path.expanduser("~/.ssh/id_rsa")) is True +class TestMultipleSafeWriteRoots: + """HERMES_WRITE_SAFE_ROOT with multiple colon-separated directories.""" + + def test_write_inside_first_root_allowed(self, tmp_path: Path, monkeypatch): + root_a = tmp_path / "workspace_a" + root_b = tmp_path / "workspace_b" + child = root_a / "subdir" / "file.txt" + os.makedirs(child.parent, exist_ok=True) + os.makedirs(root_b, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{root_a}{os.pathsep}{root_b}") + assert _is_write_denied(str(child)) is False + + def test_write_inside_second_root_allowed(self, tmp_path: Path, monkeypatch): + root_a = tmp_path / "workspace_a" + root_b = tmp_path / "workspace_b" + child = root_b / "subdir" / "file.txt" + os.makedirs(child.parent, exist_ok=True) + os.makedirs(root_a, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{root_a}{os.pathsep}{root_b}") + assert _is_write_denied(str(child)) is False + + def test_write_outside_all_roots_denied(self, tmp_path: Path, monkeypatch): + root_a = tmp_path / "workspace_a" + root_b = tmp_path / "workspace_b" + outside = tmp_path / "other" / "file.txt" + os.makedirs(root_a, exist_ok=True) + os.makedirs(root_b, exist_ok=True) + os.makedirs(outside.parent, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{root_a}{os.pathsep}{root_b}") + assert _is_write_denied(str(outside)) is True + + def test_trailing_separator_ignored(self, tmp_path: Path, monkeypatch): + root = tmp_path / "workspace" + inside = root / "file.txt" + os.makedirs(root, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{root}{os.pathsep}") + assert _is_write_denied(str(inside)) is False + + def test_leading_separator_ignored(self, tmp_path: Path, monkeypatch): + root = tmp_path / "workspace" + inside = root / "file.txt" + os.makedirs(root, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{os.pathsep}{root}") + assert _is_write_denied(str(inside)) is False + + def test_double_separator_ignored(self, tmp_path: Path, monkeypatch): + root_a = tmp_path / "workspace_a" + root_b = tmp_path / "workspace_b" + os.makedirs(root_a, exist_ok=True) + os.makedirs(root_b, exist_ok=True) + + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", f"{root_a}{os.pathsep}{os.pathsep}{root_b}") + # Both roots should still be active + assert _is_write_denied(str(root_a / "file.txt")) is False + assert _is_write_denied(str(root_b / "file.txt")) is False + + def test_all_separators_yields_empty_set(self, tmp_path: Path, monkeypatch): + target = tmp_path / "regular.txt" + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", os.pathsep * 3) + assert _is_write_denied(str(target)) is False + + def test_static_deny_still_wins_with_multiple_roots(self, tmp_path: Path, monkeypatch): + """Static deny list takes priority even when multiple safe roots include home.""" + root = tmp_path / "workspace" + os.makedirs(root, exist_ok=True) + + monkeypatch.setenv( + "HERMES_WRITE_SAFE_ROOT", + f"{root}{os.pathsep}{os.path.expanduser('~')}", + ) + assert _is_write_denied(os.path.expanduser("~/.ssh/id_rsa")) is True + + def test_duplicate_roots_deduplicated(self, tmp_path: Path, monkeypatch): + root = tmp_path / "workspace" + inside = root / "file.txt" + os.makedirs(root, exist_ok=True) + + monkeypatch.setenv( + "HERMES_WRITE_SAFE_ROOT", + f"{root}{os.pathsep}{root}", + ) + assert _is_write_denied(str(inside)) is False + + class TestCheckSensitivePathMacOSBypass: """Verify _check_sensitive_path blocks /private/etc paths (issue #8734).""" diff --git a/website/docs/reference/environment-variables.md b/website/docs/reference/environment-variables.md index 8a4a2df677e..646b6320b1f 100644 --- a/website/docs/reference/environment-variables.md +++ b/website/docs/reference/environment-variables.md @@ -632,7 +632,7 @@ Advanced per-platform knobs for throttling the outbound message batcher. Most us | `HERMES_PREFILL_MESSAGES_FILE` | Path to a JSON file of ephemeral prefill messages injected at API-call time. | | `HERMES_ALLOW_PRIVATE_URLS` | `true`/`false` — allow tools to fetch localhost/private-network URLs. Off by default in gateway mode. | | `HERMES_REDACT_SECRETS` | `true`/`false` — control secret redaction in tool output, logs, and chat responses (default: `true`). | -| `HERMES_WRITE_SAFE_ROOT` | Optional directory prefix that restricts `write_file`/`patch` writes; paths outside require approval. Supports multiple directories separated by `:` (e.g., `/opt/data:/var/www/html`). | +| `HERMES_WRITE_SAFE_ROOT` | Optional directory prefix that restricts `write_file`/`patch` writes; paths outside require approval. Supports multiple directories separated by `os.pathsep` (`:` on Unix, `;` on Windows). | | `HERMES_DISABLE_LAZY_INSTALLS` | Internal bridge var set automatically in the official Docker image to prevent runtime dependency installs into the immutable `/opt/hermes` tree. The user-facing equivalent is `security.allow_lazy_installs: false` in `config.yaml`; do not set this in `.env`. | | `HERMES_DISABLE_FILE_STATE_GUARD` | Set to `1` to turn off the "file changed since you read it" guard on `patch`/`write_file`. | | `HERMES_CORE_TOOLS` | Comma-separated override for the canonical core tool list (advanced; rarely needed). | 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 9be47d41f50..7ee79f76511 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 @@ -559,7 +559,7 @@ Graph 事件（Teams 会议、日历、聊天等）的入站变更通知监听 | `HERMES_PREFILL_MESSAGES_FILE` | 包含在 API 调用时注入的临时预填消息的 JSON 文件路径。 | | `HERMES_ALLOW_PRIVATE_URLS` | `true`/`false`——允许工具获取 localhost/私有网络 URL。gateway 模式下默认关闭。 | | `HERMES_REDACT_SECRETS` | `true`/`false`——控制工具输出、日志和聊天响应中的密钥脱敏（默认：`true`）。 | -| `HERMES_WRITE_SAFE_ROOT` | 可选目录前缀，限制 `write_file`/`patch` 写入；超出范围的路径需要审批。支持多个目录，使用 `:` 分隔（例如：`/opt/data:/var/www/html`）。 | +| `HERMES_WRITE_SAFE_ROOT` | 可选目录前缀，限制 `write_file`/`patch` 写入；超出范围的路径需要审批。支持多个目录，使用 `os.pathsep` 分隔（Unix 为 `:`，Windows 为 `;`）。 | | `HERMES_DISABLE_LAZY_INSTALLS` | 官方 Docker 镜像中自动设置的内部桥接变量，用于阻止运行时将依赖安装到不可变的 `/opt/hermes` 树。面向用户的等价配置是 `config.yaml` 中的 `security.allow_lazy_installs: false`；不要在 `.env` 中手动设置此变量。 | | `HERMES_DISABLE_FILE_STATE_GUARD` | 设为 `1` 可关闭 `patch`/`write_file` 上的"文件自上次读取后已更改"保护。 | | `HERMES_CORE_TOOLS` | 规范核心工具列表的逗号分隔覆盖（高级；极少需要）。 |