fix(file_ops): guard the directory entry that V4A Delete/Move now act on

Delete and Move remove or rename the directory entry itself (a symlink,
not its target), but get_write_denied_error realpaths its argument, so it
only ever vetted the link's target. A link outside HERMES_WRITE_SAFE_ROOT
(or inside ~/.hermes/sessions) pointing at a file inside the root passed
the guard and the link was deleted/renamed outside the allowed area.

delete_file and move_file now also run the same guard on each entry's
parent directory, which realpaths to where the entry really lives.

The symlink test gains a safe-root case (red without this change), and
its Move cases always assert success, the rename, the link target and
files_modified instead of tolerating a refusing move primitive; expected
values are parameters rather than header introspection, and json is a
module-level import.
This commit is contained in:
kshitijk4poor
2026-09-26 21:54:37 +05:30
committed by kshitij
parent 0a99750128
commit 7d9cb0ac51
2 changed files with 36 additions and 32 deletions

View File

@@ -15,6 +15,7 @@ Core invariant these tests pin:
never left to resolve against whatever the process cwd happens to be.
"""
import json
import os
from pathlib import Path, PurePosixPath
@@ -259,8 +260,6 @@ def test_v4a_patch_applies_to_resolved_workspace_not_backend_cwd(
landed in a different directory than everything the tool reported. The fix
rewrites headers to the resolved absolute paths before apply.
"""
import json
workspace, decoy = _isolated_cwd
task_id = "sess-v4a"
@@ -304,18 +303,20 @@ def test_v4a_patch_applies_to_resolved_workspace_not_backend_cwd(
@pytest.mark.platforms("posix")
@pytest.mark.parametrize("header", [
"*** Delete File: local.yaml",
"*** Move File: local.yaml -> old.yaml",
"*** Update File: local.yaml\n@@\n-shared: true\n+shared: false\n*** Move File: local.yaml -> old.yaml",
@pytest.mark.parametrize("header,safe_root,content,dest", [
("*** Delete File: local.yaml", False, "shared: true\n", None),
("*** Move File: local.yaml -> old.yaml", False, "shared: true\n", "old.yaml"),
("*** Update File: local.yaml\n@@\n-shared: true\n+shared: false\n*** Move File: local.yaml -> old.yaml",
False, "shared: false\n", "old.yaml"),
# The link lives outside HERMES_WRITE_SAFE_ROOT but points inside it: guarding
# only the target would let the delete remove an entry outside the root.
("*** Delete File: local.yaml", True, "shared: true\n", None),
])
def test_v4a_delete_and_move_act_on_a_symlink_not_its_target(_isolated_cwd, monkeypatch, header):
def test_v4a_delete_and_move_act_on_a_symlink_not_its_target(
_isolated_cwd, monkeypatch, header, safe_root, content, dest):
"""Delete and Move act on the directory entry. Resolving a symlinked header to its
target deleted or renamed the real file and left the link dangling; an Update still
edits the target's content through the link. The contract holds whatever the move
primitive allows: the target is never deleted or moved, and the link is never lost."""
import json
edits the target's content through the link. The write guards cover the entry too."""
from tools.environments.local import LocalEnvironment
from tools.file_operations import ShellFileOperations
from tools.registry import registry
@@ -327,26 +328,24 @@ def test_v4a_delete_and_move_act_on_a_symlink_not_its_target(_isolated_cwd, monk
terminal_tool.register_task_env_overrides(task_id, {"cwd": str(workspace)})
env = LocalEnvironment(cwd=str(workspace))
monkeypatch.setattr(ft, "_get_file_ops", lambda task_id="default": ShellFileOperations(env))
target, link = workspace / "base.yaml", workspace / "local.yaml"
target.write_text("shared: true\n")
link.symlink_to("base.yaml")
(workspace / "safe").mkdir()
target, link = workspace / "safe" / "base.yaml", workspace / "local.yaml"
target.write_text("shared: true\n", encoding="utf-8")
link.symlink_to("safe/base.yaml")
if safe_root:
monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", str(workspace / "safe"))
out = json.loads(registry.dispatch(
"patch", {"mode": "patch", "patch": f"*** Begin Patch\n{header}\n*** End Patch\n"}, task_id=task_id))
assert target.is_file() and not target.is_symlink()
assert target.read_text() == ("shared: false\n" if "Update" in header else "shared: true\n")
if "Move" not in header:
assert out.get("success"), out
assert not os.path.lexists(link)
assert str(link) in out["files_modified"]
assert target.read_text(encoding="utf-8") == content
if safe_root:
assert not out.get("success") and "HERMES_WRITE_SAFE_ROOT" in json.dumps(out), out
assert link.is_symlink()
return
# A Move renames the link itself; a no-clobber move primitive may refuse a symlink
# source instead, which leaves it in place. It ends up under exactly one name.
moved = os.path.lexists(workspace / "old.yaml")
assert moved != os.path.lexists(link)
kept = workspace / "old.yaml" if moved else link
assert kept.is_symlink() and os.readlink(kept) == "base.yaml"
if moved:
assert out.get("success"), out
assert str(link) in out["files_modified"]
assert out.get("success"), out
assert not os.path.lexists(link)
assert str(link) in out["files_modified"]
if dest:
assert os.readlink(workspace / dest) == "safe/base.yaml"

View File

@@ -1251,9 +1251,13 @@ class ShellFileOperations(LintMixin, SearchMixin, FileOperations):
"""Delete a single file (directories rejected) via the backend's ``python -c``
so one code path works on local/docker/ssh AND Windows shells (no ``rm``)."""
path = self._expand_path(path)
denied = get_write_denied_error(path, verb="Delete")
if denied:
return WriteResult(error=denied)
# Delete removes the directory entry (a symlink itself, not its target), so
# the guards also run on the entry's directory; realpath(path) alone would
# clear a link outside the safe root that points inside it.
for p in (path, os.path.dirname(path) or "."):
denied = get_write_denied_error(p, verb="Delete")
if denied:
return WriteResult(error=denied)
# Path baked in via repr() for shell-independent quoting; no
# ``unlink(missing_ok=True)`` (a 3.7 remote interpreter lacks it).
snippet = (
@@ -1288,7 +1292,8 @@ class ShellFileOperations(LintMixin, SearchMixin, FileOperations):
def move_file(self, src: str, dst: str) -> WriteResult:
src = self._expand_path(src)
dst = self._expand_path(dst)
for p in (src, dst):
# Entry-level op like delete_file: guard both entries' directories too.
for p in (src, dst, os.path.dirname(src) or ".", os.path.dirname(dst) or "."):
denied = get_write_denied_error(p, verb="Move")
if denied:
return WriteResult(error=denied)