fix(file_ops): a trailing separator must not empty the Delete/Move entry leaf
'.../.ssh/link/' split to an empty basename, so the entry check degenerated to checking the link's TARGET: get_write_denied_error(entry=True) and is_protected_path(follow=False) let the delete remove a link inside ~/.ssh, and _resolve_entry_for_task fell back to full resolution, so a V4A '*** Delete File: dir/link/' deleted the file the link points to (the original bug, trailing-slash form). split_entry() drops trailing separators (keeping a bare '/' or drive root) before the parent/leaf split and is used by all three entry-mode sites. is_protected_path(follow=False) now normcases the joined entry, not only its parent, so a case-variant spelling of the exe/venv entry still matches on Windows.
This commit is contained in:
@@ -262,15 +262,15 @@ def _classify_write_denial(path: str, *, entry: bool = False) -> Optional[str]:
|
||||
# The runtime's own interpreter/venv is never agent-writable (an overwrite
|
||||
# bricks the next start exactly like a delete, #58748) — and this must fire
|
||||
# BEFORE the approval-gated allow so ~/.ssh-style gating cannot re-open it.
|
||||
from agent.runtime_self_protection import is_protected_path
|
||||
from agent.runtime_self_protection import is_protected_path, split_entry
|
||||
|
||||
if is_protected_path(path) or (entry and is_protected_path(path, follow=False)):
|
||||
return "credential"
|
||||
denial = _classify_resolved_write_denial(homes, resolved)
|
||||
if denial or not entry:
|
||||
return denial
|
||||
expanded = os.path.expanduser(str(path))
|
||||
entry_path = os.path.join(os.path.realpath(os.path.dirname(expanded) or "."), os.path.basename(expanded))
|
||||
parent, leaf = split_entry(os.path.expanduser(str(path)))
|
||||
entry_path = os.path.join(os.path.realpath(parent or "."), leaf)
|
||||
return _classify_resolved_write_denial(homes, entry_path)
|
||||
|
||||
|
||||
|
||||
@@ -151,14 +151,25 @@ def _overlaps(a: str, b: str) -> bool:
|
||||
return a == b or a.startswith(b + sep) or b.startswith(a + sep)
|
||||
|
||||
|
||||
def split_entry(path: str) -> tuple[str, str]:
|
||||
"""``os.path.split`` for a directory entry, trailing separators dropped first: an
|
||||
empty leaf (``dir/link/``) would make entry checks degenerate to the link's target.
|
||||
A bare root (``/``, ``C:\\``) is kept as is."""
|
||||
drive, tail = os.path.splitdrive(path)
|
||||
return os.path.split(drive + (tail.rstrip(os.sep + (os.altsep or "")) or tail[:1]))
|
||||
|
||||
|
||||
def is_protected_path(path: str, *, follow: bool = True) -> Optional[str]:
|
||||
"""Description of the protected runtime path ``path`` touches, else ``None``.
|
||||
|
||||
``follow=False`` keeps the final component unresolved (the entry itself, for
|
||||
ops that unlink/rename a symlink rather than its target)."""
|
||||
resolved = _normalize_path(path) if follow else _normalize_path(os.path.dirname(path) or ".")
|
||||
if resolved and not follow:
|
||||
resolved = os.path.join(resolved, os.path.basename(path))
|
||||
if follow:
|
||||
resolved = _normalize_path(path)
|
||||
else:
|
||||
parent, leaf = split_entry(path)
|
||||
resolved = _normalize_path(parent or ".")
|
||||
resolved = resolved and os.path.normcase(os.path.join(resolved, leaf))
|
||||
if not resolved:
|
||||
return None
|
||||
for protected, description in _protected():
|
||||
|
||||
@@ -43,12 +43,13 @@ def test_delete_file_refuses_directory(ops, tmp_path):
|
||||
|
||||
|
||||
@pytest.mark.platforms("posix")
|
||||
@pytest.mark.parametrize("op", ["delete", "move"])
|
||||
@pytest.mark.parametrize("op", ["delete", "delete_trailing_slash", "move"])
|
||||
@pytest.mark.parametrize("layout", ["beside_runtime_venv", "link_in_credential_dir"])
|
||||
def test_delete_and_move_guard_the_entry_itself(ops, tmp_path, monkeypatch, op, layout):
|
||||
"""Delete/Move vet the directory entry (parent resolved, leaf kept): a plain file whose
|
||||
directory merely CONTAINS the runtime venv (``~/notes.txt``) stays deletable/movable,
|
||||
while a link directly inside a credential dir is refused even though it points outside."""
|
||||
while a link directly inside a credential dir is refused even though it points outside.
|
||||
A trailing separator (``.ssh/link/``) must not empty the leaf and skip the entry check."""
|
||||
home, outside = tmp_path / "home", tmp_path / "outside.txt"
|
||||
outside.write_text("keep", encoding="utf-8")
|
||||
monkeypatch.setenv("HOME", str(home))
|
||||
@@ -63,7 +64,10 @@ def test_delete_and_move_guard_the_entry_itself(ops, tmp_path, monkeypatch, op,
|
||||
entry.symlink_to(outside)
|
||||
moved = home / "moved.txt"
|
||||
|
||||
result = ops.delete_file(str(entry)) if op == "delete" else ops.move_file(str(entry), str(moved))
|
||||
if op == "move":
|
||||
result = ops.move_file(str(entry), str(moved))
|
||||
else:
|
||||
result = ops.delete_file(str(entry) + ("/" if op == "delete_trailing_slash" else ""))
|
||||
|
||||
if layout == "beside_runtime_venv":
|
||||
assert result.error is None, result.error
|
||||
|
||||
@@ -305,6 +305,8 @@ def test_v4a_patch_applies_to_resolved_workspace_not_backend_cwd(
|
||||
@pytest.mark.platforms("posix")
|
||||
@pytest.mark.parametrize("header,safe_root,content,dest", [
|
||||
("*** Delete File: local.yaml", False, "shared: true\n", None),
|
||||
# A trailing separator must still name the link, not fall back to its target.
|
||||
("*** 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"),
|
||||
|
||||
@@ -12,6 +12,8 @@ import sys
|
||||
import time
|
||||
from pathlib import Path, PurePosixPath
|
||||
|
||||
from agent.runtime_self_protection import split_entry
|
||||
|
||||
# ``TERMINAL_CWD`` values that mean "not configured" ("." from a stale config;
|
||||
# "auto"/"cwd" are wizard placeholders). gateway/run.py sanitizes the same set.
|
||||
_TERMINAL_CWD_SENTINELS = frozenset({"", ".", "./", "auto", "cwd"})
|
||||
@@ -276,7 +278,7 @@ def _resolve_entry_for_task(filepath: str, task_id: str = "default") -> Path | P
|
||||
"""``_resolve_path_for_task`` for an operation on the directory entry itself (delete,
|
||||
rename): the parent is resolved, but a symlink in the last component is kept, since
|
||||
resolving it aims the operation at the file the link points to."""
|
||||
parent, name = os.path.split(filepath)
|
||||
parent, name = split_entry(filepath)
|
||||
if name in ("", ".", "..") or (not parent and name.startswith("~")):
|
||||
return _resolve_path_for_task(filepath, task_id)
|
||||
return _resolve_path_for_task(parent or ".", task_id) / name
|
||||
|
||||
Reference in New Issue
Block a user