diff --git a/agent/file_safety.py b/agent/file_safety.py index 789cb3d747..28320edd70 100644 --- a/agent/file_safety.py +++ b/agent/file_safety.py @@ -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) diff --git a/agent/runtime_self_protection.py b/agent/runtime_self_protection.py index d6917d3a12..fb38911795 100644 --- a/agent/runtime_self_protection.py +++ b/agent/runtime_self_protection.py @@ -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(): diff --git a/tests/tools/test_file_operations_delete.py b/tests/tools/test_file_operations_delete.py index b76d091ff2..81ccb3f3b0 100644 --- a/tests/tools/test_file_operations_delete.py +++ b/tests/tools/test_file_operations_delete.py @@ -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 diff --git a/tests/tools/test_file_tools_cwd_resolution.py b/tests/tools/test_file_tools_cwd_resolution.py index 82f2cf045e..92b2541d45 100644 --- a/tests/tools/test_file_tools_cwd_resolution.py +++ b/tests/tools/test_file_tools_cwd_resolution.py @@ -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"), diff --git a/tools/file_tools_paths.py b/tools/file_tools_paths.py index 4e459a5e01..952862ae2d 100644 --- a/tools/file_tools_paths.py +++ b/tools/file_tools_paths.py @@ -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