fix nested extra path issue

Author: qiyaoq-oai

src/agents/sandbox/sandboxes/unix_local.py

@@ -757,6 +757,11 @@ def _literal(path: Path | str) -> str:
                 for path, read_only in extra_path_grants
                 if not read_only
             ],
+            *[
+                f"(deny file-write* (subpath {_literal(path)}))"
+                for path, read_only in extra_path_grants
+                if read_only
+            ],
             '(allow file-read-data file-read-metadata (subpath "/usr/bin"))',
             '(allow file-read-data file-read-metadata (subpath "/usr/lib"))',
             '(allow file-read-data file-read-metadata (subpath "/bin"))',

src/agents/sandbox/session/base_sandbox_session.py

@@ -706,12 +706,17 @@ async def _validate_remote_path_access(
         path_policy = self._workspace_path_policy()
         workspace_path = path_policy.normalize_path(original_path, for_write=for_write)
         helper_path = await self._ensure_runtime_helper_installed(RESOLVE_WORKSPACE_PATH_HELPER)
-        extra_roots = path_policy.extra_path_grant_roots(writable_only=for_write)
+        extra_grant_args = tuple(
+            arg
+            for root, read_only in path_policy.extra_path_grant_rules()
+            for arg in (str(root), "1" if read_only else "0")
+        )
         command = (
             str(helper_path),
             str(root),
             str(workspace_path),
-            *(str(extra_root) for extra_root in extra_roots),
+            "1" if for_write else "0",
+            *extra_grant_args,
         )
         result = await self.exec(*command, shell=False)
         if result.ok():
@@ -721,7 +726,13 @@ async def _validate_remote_path_access(
                 # semantics while the remote realpath check still enforces path confinement.
                 return workspace_path
             raise ExecTransportError(
-                command=("resolve_workspace_path", str(root), str(workspace_path), *extra_roots),
+                command=(
+                    "resolve_workspace_path",
+                    str(root),
+                    str(workspace_path),
+                    "1" if for_write else "0",
+                    *extra_grant_args,
+                ),
                 context={
                     "reason": "empty_stdout",
                     "exit_code": result.exit_code,
@@ -743,8 +754,24 @@ async def _validate_remote_path_access(
             )
         if result.exit_code == 113:
             raise ValueError(result.stderr.decode("utf-8", errors="replace").strip())
+        if result.exit_code == 114:
+            stderr = result.stderr.decode("utf-8", errors="replace")
+            context: dict[str, object] = {"reason": "read_only_extra_path_grant"}
+            for line in stderr.splitlines():
+                if line.startswith("read-only extra path grant: "):
+                    context["grant_path"] = line.removeprefix("read-only extra path grant: ")
+                elif line.startswith("resolved path: "):
+                    context["resolved_path"] = line.removeprefix("resolved path: ")
+            raise WorkspaceArchiveWriteError(path=workspace_path, context=context)
         raise ExecNonZeroError(
-            result, command=("resolve_workspace_path", str(root), str(workspace_path), *extra_roots)
+            result,
+            command=(
+                "resolve_workspace_path",
+                str(root),
+                str(workspace_path),
+                "1" if for_write else "0",
+                *extra_grant_args,
+            ),
         )
 
     @abc.abstractmethod

src/agents/sandbox/session/runtime_helpers.py

@@ -15,9 +15,23 @@
 
 root="$1"
 candidate="$2"
-shift 2
+for_write="$3"
+shift 3
 max_symlink_depth=64
 
+case "$for_write" in
+    0|1) ;;
+    *)
+        printf 'for_write must be 0 or 1: %s\\n' "$for_write" >&2
+        exit 64
+        ;;
+esac
+
+if [ $(( $# % 2 )) -ne 0 ]; then
+    printf 'extra path grants must be root/read_only pairs\\n' >&2
+    exit 64
+fi
+
 resolve_path() {
     path="$1"
     depth="${2:-0}"
@@ -69,6 +83,10 @@
 }
 
 resolved_candidate=$(resolve_path "$candidate" 0)
+best_grant_root=""
+best_grant_original=""
+best_grant_read_only="0"
+best_grant_len=0
 
 check_root() {
     allowed_root="$1"
@@ -90,14 +108,47 @@
     fi
 }
 
-for extra_root in "$@"; do
-    reject_root_grant "$extra_root"
+consider_extra_grant() {
+    allowed_root="$1"
+    read_only="$2"
+    case "$read_only" in
+        0|1) ;;
+        *)
+            printf 'extra path grant read_only must be 0 or 1: %s\\n' "$read_only" >&2
+            exit 64
+            ;;
+    esac
+
+    reject_root_grant "$allowed_root"
+    resolved_root=$(resolve_path "$allowed_root" 0)
+    case "$resolved_candidate" in
+        "$resolved_root"|"$resolved_root"/*)
+            root_len=${#resolved_root}
+            if [ "$root_len" -gt "$best_grant_len" ]; then
+                best_grant_root="$resolved_root"
+                best_grant_original="$allowed_root"
+                best_grant_read_only="$read_only"
+                best_grant_len="$root_len"
+            fi
+            ;;
+    esac
+}
+
+while [ "$#" -gt 0 ]; do
+    consider_extra_grant "$1" "$2"
+    shift 2
 done
 
 check_root "$root"
-for extra_root in "$@"; do
-    check_root "$extra_root"
-done
+if [ -n "$best_grant_root" ]; then
+    if [ "$for_write" = "1" ] && [ "$best_grant_read_only" = "1" ]; then
+        printf 'read-only extra path grant: %s\\nresolved path: %s\\n' \
+            "$best_grant_original" "$resolved_candidate" >&2
+        exit 114
+    fi
+    printf '%s\\n' "$resolved_candidate"
+    exit 0
+fi
 
 printf 'workspace escape: %s\\n' "$resolved_candidate" >&2
 exit 111

src/agents/sandbox/workspace_paths.py

@@ -159,17 +159,15 @@ def _raise_if_read_only_grant(
             },
         )
 
-    def extra_path_grant_roots(self, *, writable_only: bool = False) -> tuple[Path, ...]:
-        """Return normalized extra grant roots for remote realpath checks."""
+    def extra_path_grant_rules(self) -> tuple[tuple[Path, bool], ...]:
+        """Return normalized extra grant roots and access modes for remote realpath checks."""
 
-        roots: list[Path] = []
+        rules: list[tuple[Path, bool]] = []
         for grant in self._extra_path_grants:
-            if writable_only and grant.read_only:
-                continue
             root = Path(grant.path)
             _raise_if_filesystem_root(root)
-            roots.append(root)
-        return tuple(roots)
+            rules.append((root, grant.read_only))
+        return tuple(rules)
 
     def _absolute_workspace_posix_path(self, path: Path) -> PurePosixPath:
         normalized = self._absolute_posix_path(path)

tests/extensions/test_sandbox_e2b.py

@@ -301,7 +301,7 @@ async def kill(self) -> None:
         if _is_helper_present_command(command):
             return _FakeE2BResult()
         if parts and parts[0] == str(RESOLVE_WORKSPACE_PATH_HELPER.install_path):
-            return _FakeE2BResult(stdout=parts[-1])
+            return _FakeE2BResult(stdout=parts[2])
         if parts and parts[0] == str(WORKSPACE_FINGERPRINT_HELPER.install_path):
             return _FakeE2BResult(
                 stdout='{"fingerprint":"fake-workspace-fingerprint","version":"workspace_tar_sha256_v1"}\n'

tests/extensions/test_sandbox_modal.py

@@ -227,7 +227,7 @@ def __init__(self, payload: bytes = b"") -> None:
                     wait=_with_aio(lambda: 0),
                 )
             if command and command[0] == resolve_helper_path:
-                stdout = str(command[-1]).encode("utf-8")
+                stdout = str(command[2]).encode("utf-8")
             if command and command[0] == fingerprint_helper_path:
                 stdout = (
                     b'{"fingerprint":"fake-workspace-fingerprint",'
@@ -1758,7 +1758,7 @@ async def _fake_exec(
     assert any(cmd and cmd[0] == helper_path for cmd in second_run_commands)
     assert commands == [
         ["test", "-x", helper_path],
-        [helper_path, "/workspace", "/workspace/link.txt"],
+        [helper_path, "/workspace", "/workspace/link.txt", "0"],
     ]
 
 

tests/sandbox/test_docker.py

@@ -327,13 +327,57 @@ async def _exec_internal(
         if cmd == ["test", "-x", helper_path]:
             return ExecResult(stdout=b"", stderr=b"", exit_code=0)
         if cmd and cmd[0] == helper_path:
+            for_write = cmd[3]
             candidate = self._host_path(cmd[2]).resolve(strict=False)
-            roots = [self._host_path(root).resolve(strict=False) for root in [cmd[1], *cmd[3:]]]
-            for root in roots:
+            workspace_root = self._host_path(cmd[1]).resolve(strict=False)
+            try:
+                candidate.relative_to(workspace_root)
+            except ValueError:
+                pass
+            else:
+                return ExecResult(
+                    stdout=str(self._container_path(candidate)).encode("utf-8"),
+                    stderr=b"",
+                    exit_code=0,
+                )
+
+            best_root: Path | None = None
+            best_original = ""
+            best_read_only = False
+            grant_args = cmd[4:]
+            assert len(grant_args) % 2 == 0
+            for original_root, read_only_text in zip(
+                grant_args[::2],
+                grant_args[1::2],
+                strict=False,
+            ):
+                root = self._host_path(original_root).resolve(strict=False)
+                if root == root.parent:
+                    return ExecResult(
+                        stdout=b"",
+                        stderr=(
+                            f"extra path grant must not resolve to filesystem root: {original_root}"
+                        ).encode(),
+                        exit_code=113,
+                    )
                 try:
                     candidate.relative_to(root)
                 except ValueError:
                     continue
+                if best_root is None or len(root.parts) > len(best_root.parts):
+                    best_root = root
+                    best_original = original_root
+                    best_read_only = read_only_text == "1"
+            if best_root is not None:
+                if for_write == "1" and best_read_only:
+                    return ExecResult(
+                        stdout=b"",
+                        stderr=(
+                            f"read-only extra path grant: {best_original}\n"
+                            f"resolved path: {self._container_path(candidate)}\n"
+                        ).encode(),
+                        exit_code=114,
+                    )
                 return ExecResult(
                     stdout=str(self._container_path(candidate)).encode("utf-8"),
                     stderr=b"",
@@ -1006,14 +1050,56 @@ async def test_docker_write_rejects_workspace_symlink_to_read_only_extra_path_gr
         ),
     )
 
-    with pytest.raises(InvalidManifestPathError) as exc_info:
+    with pytest.raises(WorkspaceArchiveWriteError) as exc_info:
         await session.write(Path("tmp-link/result.txt"), io.BytesIO(b"scratch output"))
 
-    assert str(exc_info.value) == "manifest path must not escape root: tmp-link/result.txt"
+    assert str(exc_info.value) == "failed to write archive for path: /workspace/tmp-link/result.txt"
+    assert exc_info.value.context == {
+        "path": "/workspace/tmp-link/result.txt",
+        "reason": "read_only_extra_path_grant",
+        "grant_path": "/tmp",
+        "resolved_path": "/tmp/result.txt",
+    }
+
+
+@pytest.mark.asyncio
+async def test_docker_write_rejects_workspace_symlink_to_nested_read_only_extra_path_grant(
+    tmp_path: Path,
+) -> None:
+    host_root = tmp_path / "container"
+    workspace = host_root / "workspace"
+    extra_root = host_root / "tmp"
+    protected_root = extra_root / "protected"
+    workspace.mkdir(parents=True)
+    protected_root.mkdir(parents=True)
+    (workspace / "tmp-link").symlink_to(extra_root, target_is_directory=True)
+
+    session = _HostBackedDockerSession(
+        host_root=host_root,
+        manifest=Manifest(
+            root="/workspace",
+            extra_path_grants=(
+                SandboxPathGrant(path="/tmp"),
+                SandboxPathGrant(path="/tmp/protected", read_only=True),
+            ),
+        ),
+    )
+
+    with pytest.raises(WorkspaceArchiveWriteError) as exc_info:
+        await session.write(
+            Path("tmp-link/protected/result.txt"),
+            io.BytesIO(b"scratch output"),
+        )
+
+    assert (
+        str(exc_info.value)
+        == "failed to write archive for path: /workspace/tmp-link/protected/result.txt"
+    )
     assert exc_info.value.context == {
-        "rel": "tmp-link/result.txt",
-        "reason": "escape_root",
-        "resolved_path": "workspace escape",
+        "path": "/workspace/tmp-link/protected/result.txt",
+        "reason": "read_only_extra_path_grant",
+        "grant_path": "/tmp/protected",
+        "resolved_path": "/tmp/protected/result.txt",
     }
 
 

tests/sandbox/test_runtime.py

@@ -422,15 +422,15 @@ async def test_remote_realpath_empty_success_output_is_transport_error() -> None
         await session._validate_remote_path_access("file.txt")  # noqa: SLF001
 
     assert exc_info.value.context == {
-        "command": ("resolve_workspace_path", "/workspace", "/workspace/file.txt"),
-        "command_str": "resolve_workspace_path /workspace /workspace/file.txt",
+        "command": ("resolve_workspace_path", "/workspace", "/workspace/file.txt", "0"),
+        "command_str": "resolve_workspace_path /workspace /workspace/file.txt 0",
         "reason": "empty_stdout",
         "exit_code": 0,
         "stdout": "",
         "stderr": "",
     }
     assert session.exec_commands == [
-        ("/tmp/resolve_workspace_path", "/workspace", "/workspace/file.txt")
+        ("/tmp/resolve_workspace_path", "/workspace", "/workspace/file.txt", "0")
     ]
 
 
@@ -3843,6 +3843,43 @@ def test_unix_local_darwin_exec_profile_allows_extra_path_grants(tmp_path: Path)
     assert f'(allow file-write* (subpath "{read_only_root}"))' not in profile_lines
 
 
+def test_unix_local_darwin_exec_profile_denies_nested_read_only_extra_path_grant(
+    tmp_path: Path,
+) -> None:
+    workspace_root = tmp_path / "workspace"
+    read_write_root = tmp_path / "read-write"
+    read_only_root = read_write_root / "protected"
+    workspace_root.mkdir()
+    read_only_root.mkdir(parents=True)
+    session = UnixLocalSandboxSession.from_state(
+        UnixLocalSandboxSessionState(
+            session_id=uuid.uuid4(),
+            manifest=Manifest(
+                root=str(workspace_root),
+                extra_path_grants=(
+                    SandboxPathGrant(path=str(read_write_root)),
+                    SandboxPathGrant(path=str(read_only_root), read_only=True),
+                ),
+            ),
+            snapshot=NoopSnapshot(id="darwin-nested-extra-path-grant"),
+            workspace_root_owned=False,
+        )
+    )
+
+    profile = session._darwin_exec_profile(
+        workspace_root,
+        extra_path_grants=session._darwin_extra_path_grant_roots(),
+    )
+    profile_lines = profile.splitlines()
+    parent_write_allow = f'(allow file-write* (subpath "{read_write_root}"))'
+    child_write_deny = f'(deny file-write* (subpath "{read_only_root}"))'
+
+    assert parent_write_allow in profile_lines
+    assert child_write_deny in profile_lines
+    assert profile_lines.index(parent_write_allow) < profile_lines.index(child_write_deny)
+    assert f'(allow file-write* (subpath "{read_only_root}"))' not in profile_lines
+
+
 def test_unix_local_darwin_exec_profile_rejects_extra_path_grant_symlink_to_root(
     tmp_path: Path,
 ) -> None: