diff --git a/libs/deepagents/deepagents/backends/sandbox.py b/libs/deepagents/deepagents/backends/sandbox.py index 276306a671..57e15b5a79 100644 --- a/libs/deepagents/deepagents/backends/sandbox.py +++ b/libs/deepagents/deepagents/backends/sandbox.py @@ -1138,24 +1138,40 @@ async def _aedit_via_upload( return EditResult(path=file_path, occurrences=data.get("count", 1)) def delete(self, file_path: str) -> DeleteResult: - """Delete a file or directory from the sandbox via a server-side ``rm``. + """Delete a file or directory from the sandbox via a server-side `rm`. - Uses ``rm -rf``, so directories are removed recursively along with their - contents, and deleting a path that does not exist succeeds silently. A - non-zero exit (e.g. a permission error) is reported as a failure. + Runs `test -e || test -L` first: a path that does not exist (and is not + a broken symlink) returns a not-found error, matching the contract of + `FilesystemBackend` and `StateBackend`. Because a shell `test` has no + error channel, a non-zero probe conflates "absent" with "unstattable" + (e.g. an unsearchable parent directory); an unknown exit code is not + treated as absent and falls through to the delete. + + Uses `rm -rf`, so directories are removed recursively along with their + contents. A recursive delete may remove some entries before failing + partway; a non-zero `rm` exit (e.g. a permission error) is reported as + a failure. Args: file_path: Absolute path to the file or directory to delete. Returns: `DeleteResult` with the deleted path on success, or an error if the - deletion command fails. + path does not exist or the deletion command fails. """ # `shlex.quote` only neutralizes shell metacharacters so the path is # passed to `rm` as a single literal argument. It is NOT a security # boundary: it does not confine the deletion to any sandbox root or # block traversal. Whatever the sandbox shell can reach, this can delete. quoted = shlex.quote(file_path) + exists = self.execute(f"test -e {quoted} || test -L {quoted}") + # `exit_code` may be None when the backend cannot determine a status; + # only a definite non-zero means the path is absent. Treating None as + # not-found would fabricate a diagnosis and skip the delete, so fall + # through to `rm` on an unknown probe result (matches the `rm` check + # below and `_parse_grep_output`, which both guard `is not None`). + if exists.exit_code is not None and exists.exit_code != 0: + return DeleteResult(error=f"Error: '{file_path}' not found") result = self.execute(f"rm -rf {quoted}") if result.exit_code == 0: diff --git a/libs/deepagents/deepagents/middleware/filesystem.py b/libs/deepagents/deepagents/middleware/filesystem.py index 17307c22a2..b3fa0fa0f1 100644 --- a/libs/deepagents/deepagents/middleware/filesystem.py +++ b/libs/deepagents/deepagents/middleware/filesystem.py @@ -62,6 +62,7 @@ ) from deepagents.backends.sandbox import BaseSandbox from deepagents.backends.utils import ( + _GLOB_WILDCARD_CHARS, _VIDEO_EXTRA_EXTENSIONS, MAX_VIDEO_INPUT_BYTES, FileType, @@ -296,12 +297,72 @@ def _check_fs_permission( return "allow" +def _wildcard_delete_overlap(pattern: str, anchor: str, target: str) -> bool: + """Check whether a wildcard deny pattern overlaps a recursive delete target. + + Args: + pattern: The original glob pattern (e.g. ``/work/*.log``). + anchor: The longest wildcard-free prefix of ``pattern``. + target: The absolute path being recursively deleted. + + Returns: + True if the pattern's matches intersect the delete subtree. + """ + # Root anchor ("/**/x"): pattern can match anywhere, block all. + if anchor == "/": + return True + # Target directly matches the glob: block. + if wcglob.globmatch(target, pattern, flags=_FS_WCMATCH_FLAGS): + return True + # Anchor is inside the delete subtree: recursive delete would remove + # matching descendants — block. + if PurePosixPath(anchor).is_relative_to(PurePosixPath(target)): + return True + # Target is below the anchor: safe to allow ONLY when the pattern suffix + # is a single, non-** component (fixed depth) AND no ancestor of the + # target matches the glob. "/work/*.log" can never match anything under + # "/work/notes.txt". But "/work/*" matches "/work/app", so deleting + # "/work/app/child" mutates a denied path's contents and must be blocked. + # Patterns with directory wildcards ("/work/*/secrets") could match + # descendants of the target, so fail closed for those. + if not PurePosixPath(target).is_relative_to(PurePosixPath(anchor)): + return False + anchor_parts = PurePosixPath(anchor).parts + pattern_parts = PurePosixPath(pattern).parts + suffix = pattern_parts[len(anchor_parts) :] + if len(suffix) != 1 or "**" in suffix[0]: + return True + # Check whether any ancestor of the target (between anchor and target) + # matches the glob. If so, the target is inside a denied directory's + # subtree. + target_parts = PurePosixPath(target).parts + return any( + wcglob.globmatch( + str(PurePosixPath(*target_parts[:depth])), + pattern, + flags=_FS_WCMATCH_FLAGS, + ) + for depth in range(len(anchor_parts), len(target_parts)) + ) + + def _find_delete_deny_patterns(rules: list[FilesystemPermission], target: str) -> list[str]: """Return deny-write patterns that block deleting `target`. - A recursive delete removes `target` and all descendants, so any overlapping - deny-write pattern prevents the operation. The check is based only on - permission rules and returns all matching patterns. + A recursive delete removes `target` and all descendants, so a deny-write + pattern blocks the operation when it could match `target` or anything in + its subtree. Sibling file globs that cannot match anything inside the + deleted subtree (e.g. deny `/work/*.log` when deleting `/work/notes.txt`) + do not block. The check is based only on permission rules and returns all + matching patterns. + + Literal (wildcard-free) deny patterns use a subtree-overlap check: a deny + on a directory blocks deleting anything inside it and blocks deleting an + ancestor that contains it. Wildcard patterns are handled by + `_wildcard_delete_overlap`, which also blocks when the glob matches an + ancestor of `target` (deleting `/work/app/child` under a deny on `/work/*` + mutates the denied `/work/app`), while still allowing siblings that can + never contain a match (deny `/work/*.log` vs `/work/notes.txt`). Args: rules: Filesystem permission rules. @@ -316,7 +377,16 @@ def _find_delete_deny_patterns(rules: list[FilesystemPermission], target: str) - if rule.mode != "deny" or "write" not in rule.operations: continue for pattern in rule.paths: - if pattern not in seen and _paths_overlap(target, _glob_anchor(pattern)): + if pattern in seen: + continue + anchor = _glob_anchor(pattern) + if any(c in _GLOB_WILDCARD_CHARS for c in pattern): + overlaps = _wildcard_delete_overlap(pattern, anchor, target) + else: + # Literal pattern (no wildcards): keep the original subtree-overlap + # check so that a deny on "/work" blocks deletes of "/work/sub". + overlaps = _paths_overlap(target, anchor) + if overlaps: seen.add(pattern) denying.append(pattern) return denying diff --git a/libs/deepagents/tests/unit_tests/backends/test_sandbox_backend.py b/libs/deepagents/tests/unit_tests/backends/test_sandbox_backend.py index 9267706b7c..5f38bd7126 100644 --- a/libs/deepagents/tests/unit_tests/backends/test_sandbox_backend.py +++ b/libs/deepagents/tests/unit_tests/backends/test_sandbox_backend.py @@ -42,10 +42,13 @@ class MockSandbox(BaseSandbox): def __init__(self) -> None: self.last_command: str | None = None + self.commands: list[str] = [] self._next_output: str = "1" self._next_exit_code: int = 0 self._uploaded: list[tuple[str, bytes]] = [] self._file_store: dict[str, bytes] = {} + # exit_code is int | None (a backend may report an unknown status). + self._responses: list[tuple[str, int | None]] = [] @property def id(self) -> str: @@ -53,15 +56,19 @@ def id(self) -> str: def execute(self, command: str, *, timeout: int | None = None) -> ExecuteResponse: self.last_command = command + self.commands.append(command) # Detect temp-file upload path: upload_files() stores .deepagents_edit_* # keys in _file_store before execute() is called. has_tmp = any(".deepagents_edit_" in k for k in self._file_store) if "old_path = base64.b64decode(" in command and has_tmp: return self._simulate_edit_tmpfile(command) - output = self._next_output - exit_code = self._next_exit_code - self._next_output = "1" - self._next_exit_code = 0 + if self._responses: + output, exit_code = self._responses.pop(0) + else: + output = self._next_output + exit_code = self._next_exit_code + self._next_output = "1" + self._next_exit_code = 0 return ExecuteResponse(output=output, exit_code=exit_code, truncated=False) def _simulate_edit_tmpfile(self, command: str) -> ExecuteResponse: @@ -1555,7 +1562,7 @@ def test_parse_grep_output_non_integer_line_number_is_skipped() -> None: class TestSandboxDelete: - """BaseSandbox.delete maps the `rm -f` exit code onto DeleteResult.""" + """BaseSandbox.delete probes existence then maps the `rm -rf` exit onto DeleteResult.""" def test_delete_success(self) -> None: sandbox = MockSandbox() @@ -1580,20 +1587,46 @@ def test_delete_directory_uses_recursive_rm(self) -> None: assert "rm -rf" in sandbox.last_command assert "/some/dir" in sandbox.last_command - def test_delete_missing_is_noop_success(self) -> None: - # `rm -f` ignores a missing path: exit 0, so delete reports success. + def test_delete_missing_returns_not_found(self) -> None: + # `test -e` exits 1 for a missing path; delete must return a not-found error. sandbox = MockSandbox() - sandbox._next_output = "" - sandbox._next_exit_code = 0 + sandbox._next_exit_code = 1 # test -e reports path absent result = sandbox.delete("/missing.txt") + assert result.path is None + assert result.error is not None + assert "not found" in result.error + + def test_delete_probe_checks_broken_symlink(self) -> None: + # The existence probe must also `test -L` so a broken symlink (where + # `test -e` fails but the link exists) is still deleted, not reported + # missing. Guards the `|| test -L` clause, which the mock's single + # exit code cannot otherwise distinguish. + sandbox = MockSandbox() + sandbox._responses = [("", 0), ("", 0)] # probe ok, rm ok + sandbox.delete("/link") + probe = sandbox.commands[0] + assert "test -e" in probe + assert "test -L" in probe + + def test_delete_unknown_probe_exit_is_not_treated_as_missing(self) -> None: + # `exit_code` may be None when the backend cannot determine a status. + # An unknown probe result must NOT be reported as not-found; it falls + # through to `rm` instead of fabricating a diagnosis. + sandbox = MockSandbox() + sandbox._responses = [("", None), ("", 0)] # probe unknown, rm ok + result = sandbox.delete("/file.txt") assert result.error is None - assert result.path == "/missing.txt" + assert result.path == "/file.txt" + assert len(sandbox.commands) == 2 # probe did not short-circuit def test_delete_failure_reports_output(self) -> None: - # A non-zero exit (e.g. a permission error) surfaces rm's stderr. + # A non-zero exit from rm (e.g. a permission error) surfaces rm's stderr. sandbox = MockSandbox() - sandbox._next_output = "rm: cannot remove '/some/dir': Is a directory" - sandbox._next_exit_code = 1 + # Queue: test -e succeeds (file exists), then rm -rf fails with output. + sandbox._responses = [ + ("", 0), + ("rm: cannot remove '/some/dir': Is a directory", 1), + ] result = sandbox.delete("/some/dir") assert result.path is None assert result.error is not None @@ -1601,10 +1634,10 @@ def test_delete_failure_reports_output(self) -> None: assert "Is a directory" in result.error def test_delete_failure_unknown_error(self) -> None: - # Non-zero exit with no output falls back to a generic message. + # Non-zero exit from rm with no output falls back to a generic message. sandbox = MockSandbox() - sandbox._next_output = "" - sandbox._next_exit_code = 1 + # Queue: test -e succeeds (file exists), then rm -rf fails silently. + sandbox._responses = [("", 0), ("", 1)] result = sandbox.delete("/file.txt") assert result.path is None assert result.error is not None @@ -1617,3 +1650,12 @@ async def test_adelete_success(self) -> None: result = await sandbox.adelete("/file.txt") assert result.error is None assert result.path == "/file.txt" + + async def test_adelete_missing_returns_not_found(self) -> None: + # `adelete` delegates to `delete`, so the not-found contract holds async. + sandbox = MockSandbox() + sandbox._next_exit_code = 1 # test -e reports path absent + result = await sandbox.adelete("/missing.txt") + assert result.path is None + assert result.error is not None + assert "not found" in result.error diff --git a/libs/deepagents/tests/unit_tests/test_permissions.py b/libs/deepagents/tests/unit_tests/test_permissions.py index a7e877eac3..6bd5fe238d 100644 --- a/libs/deepagents/tests/unit_tests/test_permissions.py +++ b/libs/deepagents/tests/unit_tests/test_permissions.py @@ -253,6 +253,30 @@ class TestFindDeleteDenyPatterns: pytest.param("/workshop/**", "/work", [], id="sibling-prefix-glob"), pytest.param("/work2", "/work", [], id="sibling-prefix-literal"), pytest.param("/work/secrets", "/work/logs", [], id="sibling-leaf"), + # Single-component file glob: non-matching siblings are allowed + pytest.param("/work/*.log", "/work/notes.txt", [], id="file-glob-does-not-block-non-matching-sibling"), + # Single-component wildcard: ancestor that matches the glob blocks + # delete of its descendants (wildcard-denied dirs get the same + # protection as literal-denied dirs) + pytest.param("/work/*", "/work/app/child", ["/work/*"], id="wildcard-ancestor-blocks-descendant-delete"), + pytest.param("/work/*.log", "/work/app.log/child", ["/work/*.log"], id="file-glob-ancestor-blocks-descendant-delete"), + pytest.param("/work/*", "/work/app/deep/nested", ["/work/*"], id="wildcard-ancestor-blocks-deep-descendant-delete"), + # Directory wildcard after anchor: target below anchor is blocked (fail closed) + pytest.param("/work/*/secrets", "/work/app", ["/work/*/secrets"], id="dir-wildcard-blocks-ancestor-target"), + pytest.param("/work/**/secrets", "/work/app", ["/work/**/secrets"], id="globstar-wildcard-blocks-ancestor-target"), + # Recursive glob with suffix: deleting /work/sub would remove /work/sub/a.log + pytest.param("/work/**/*.log", "/work/sub", ["/work/**/*.log"], id="recursive-glob-blocks-descendant-that-contains-match"), + # --- glob that matches the target itself -> blocked -------------- + # Guards the linchpin: the "allow non-matching sibling" path is only + # safe because a target that matches the glob is blocked first. If + # this direct-match check regresses, a denied file becomes deletable. + pytest.param("/work/*.log", "/work/app.log", ["/work/*.log"], id="file-glob-blocks-matching-target"), + pytest.param("/work/*", "/work/app", ["/work/*"], id="single-wildcard-blocks-matching-child"), + # Non-`*` wildcard classes (brace, char-class, `?`) are supported via + # BRACE|GLOBSTAR flags and the `_GLOB_WILDCARD_CHARS` frozenset. + pytest.param("/work/{secrets,keys}", "/work/secrets", ["/work/{secrets,keys}"], id="brace-glob-blocks-matching"), + pytest.param("/work/[ab].txt", "/work/a.txt", ["/work/[ab].txt"], id="charclass-glob-blocks-matching"), + pytest.param("/work/f?le.txt", "/work/other.txt", [], id="question-glob-non-matching-sibling-allowed"), # --- overlap in either direction -> blocked ---------------------- pytest.param("/work/a.txt", "/work/a.txt", ["/work/a.txt"], id="exact-file"), pytest.param("/work", "/work", ["/work"], id="exact-dir-literal"),