Skip to content

Commit 91f5cfd

Browse files
seratchseratch
authored
fix(sandbox): validate host grants against normalized workspace roots (#5099)
1 parent a4fe85c commit 91f5cfd

2 files changed

Lines changed: 136 additions & 12 deletions

File tree

‎src/agents/sandbox/sandboxes/docker.py‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import io
55
import logging
66
import os
7+
import posixpath
78
import re
89
import socket
910
import tarfile
@@ -15,7 +16,7 @@
1516
from collections.abc import Iterable, Iterator
1617
from concurrent.futures import ThreadPoolExecutor
1718
from dataclasses import dataclass, field
18-
from pathlib import Path
19+
from pathlib import Path, PurePath, PurePosixPath
1920
from typing import Any, Final, Literal, cast
2021

2122
import docker.errors # type: ignore[import-untyped]
@@ -1856,16 +1857,22 @@ def _build_docker_volume_mounts(
18561857
return mounts
18571858

18581859

1860+
def _docker_mount_path_identity(path: str | PurePath) -> PurePosixPath:
1861+
"""Compare Linux mount paths lexically, including leading-slash aliases."""
1862+
# POSIX normpath preserves exactly two leading slashes, but Linux treats them as one.
1863+
return PurePosixPath(posixpath.normpath(re.sub(r"^/+", "/", sandbox_path_str(path))))
1864+
1865+
18591866
def _validate_docker_path_grants(manifest: Manifest) -> None:
1860-
root = coerce_posix_path(manifest.root)
1867+
root = _docker_mount_path_identity(manifest.root)
18611868
seen_targets: set[str] = set()
18621869
explicit_targets: set[str] = set()
18631870
volume_targets = {
1864-
coerce_posix_path(mount_path).as_posix()
1871+
_docker_mount_path_identity(mount_path).as_posix()
18651872
for _artifact, mount_path in _docker_volume_mounts_for_manifest(manifest)
18661873
}
18671874
for grant in manifest.extra_path_grants:
1868-
target = coerce_posix_path(grant.path)
1875+
target = _docker_mount_path_identity(grant.path)
18691876
target_str = target.as_posix()
18701877
if target_str in seen_targets and (
18711878
grant.host_path is not None or target_str in explicit_targets

‎tests/sandbox/test_docker.py‎

Lines changed: 125 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
import time
1313
import uuid
1414
from collections.abc import Callable, Iterator
15-
from pathlib import Path
15+
from pathlib import Path, PureWindowsPath
1616
from typing import cast
1717

1818
import docker.errors # type: ignore[import-untyped]
@@ -1970,23 +1970,26 @@ async def _fake_create_container(
19701970

19711971

19721972
@pytest.mark.asyncio
1973+
@pytest.mark.parametrize("root", ["/workspace", "/mnt/sibling/../workspace"])
19731974
async def test_docker_create_container_mounts_explicit_host_path(
19741975
tmp_path: Path,
19751976
monkeypatch: pytest.MonkeyPatch,
1977+
root: str,
19761978
) -> None:
19771979
host_path = tmp_path / "shared-data"
19781980
host_path.mkdir()
19791981
container = _ResumeContainer(status="created")
19801982
docker_client = _FakeCreateDockerClient(container)
19811983
client = DockerSandboxClient(docker_client=cast(object, docker_client))
19821984
manifest = Manifest(
1985+
root=root,
19831986
extra_path_grants=(
19841987
SandboxPathGrant(
19851988
path="/mnt/shared-data",
19861989
host_path=str(host_path),
19871990
read_only=True,
19881991
),
1989-
)
1992+
),
19901993
)
19911994

19921995
monkeypatch.setattr(client, "image_exists", lambda _image: True)
@@ -2065,14 +2068,33 @@ def _image_exists(_image: str) -> bool:
20652068
[
20662069
("/workspace", "/workspace/shared-data"),
20672070
("/workspace/project", "/workspace"),
2071+
("/mnt/sibling/..", "/mnt/shared-data"),
2072+
("/mnt/sibling/../shared-data", "/mnt/shared-data"),
2073+
("/outside/../workspace/project", "/workspace"),
2074+
("/mnt/sibling/..", PureWindowsPath("/mnt/shared-data")),
2075+
("//mnt/sibling/..", "/mnt/shared-data"),
2076+
("/mnt/sibling/..", "//mnt/shared-data"),
2077+
("//mnt/sibling/../shared-data", "/mnt/shared-data"),
2078+
("/outside/../mnt/shared-data/project", "//mnt/shared-data"),
2079+
],
2080+
ids=[
2081+
"target-inside-workspace",
2082+
"target-contains-workspace",
2083+
"normalized-root-contains-target",
2084+
"normalized-root-equals-target",
2085+
"target-contains-normalized-root",
2086+
"windows-path-target",
2087+
"double-slash-root",
2088+
"double-slash-target",
2089+
"double-slash-root-equals-target",
2090+
"double-slash-target-contains-root",
20682091
],
2069-
ids=["target-inside-workspace", "target-contains-workspace"],
20702092
)
20712093
async def test_docker_rejects_host_path_target_overlapping_workspace_before_image_lookup(
20722094
tmp_path: Path,
20732095
monkeypatch: pytest.MonkeyPatch,
20742096
root: str,
2075-
target: str,
2097+
target: str | PureWindowsPath,
20762098
) -> None:
20772099
client = DockerSandboxClient(docker_client=cast(object, _FakeDockerClient()))
20782100
image_lookups = 0
@@ -2088,13 +2110,13 @@ def _image_exists(_image: str) -> bool:
20882110
ValueError,
20892111
match="host_path target must be outside the workspace root",
20902112
):
2091-
await client._create_container(
2092-
DEFAULT_PYTHON_SANDBOX_IMAGE,
2113+
await client.create(
2114+
options=DockerSandboxClientOptions(image=DEFAULT_PYTHON_SANDBOX_IMAGE),
20932115
manifest=Manifest(
20942116
root=root,
20952117
extra_path_grants=(
20962118
SandboxPathGrant(
2097-
path=target,
2119+
path=cast(str, target),
20982120
host_path=str(tmp_path),
20992121
),
21002122
),
@@ -2104,6 +2126,54 @@ def _image_exists(_image: str) -> bool:
21042126
assert image_lookups == 0
21052127

21062128

2129+
@pytest.mark.asyncio
2130+
@pytest.mark.parametrize("operation", ["create", "resume"])
2131+
@pytest.mark.parametrize("host_grant", [False, True])
2132+
async def test_docker_path_grant_validation_does_not_probe_host_workspace_root(
2133+
tmp_path: Path,
2134+
monkeypatch: pytest.MonkeyPatch,
2135+
operation: str,
2136+
host_grant: bool,
2137+
) -> None:
2138+
client = DockerSandboxClient(docker_client=cast(object, _FakeDockerClient()))
2139+
manifest = Manifest(
2140+
root="//container-only/sibling/../workspace",
2141+
extra_path_grants=(
2142+
(SandboxPathGrant(path="/mnt/shared-data", host_path=str(tmp_path)),)
2143+
if host_grant
2144+
else ()
2145+
),
2146+
)
2147+
state = DockerSandboxSessionState(
2148+
manifest=manifest,
2149+
snapshot=NoopSnapshot(id="snapshot"),
2150+
image=DEFAULT_PYTHON_SANDBOX_IMAGE,
2151+
container_id="container",
2152+
)
2153+
2154+
def unexpected_exists(path: Path) -> bool:
2155+
pytest.fail("Docker path comparison must not probe the host filesystem")
2156+
2157+
def provider_lookup(_container_id: str) -> None:
2158+
raise RuntimeError("provider reached")
2159+
2160+
async def provider_create(*args: object, **kwargs: object) -> None:
2161+
raise RuntimeError("provider reached")
2162+
2163+
with monkeypatch.context() as patch:
2164+
patch.setattr(Path, "exists", unexpected_exists)
2165+
patch.setattr(client, "get_container", provider_lookup)
2166+
patch.setattr(client, "_create_container", provider_create)
2167+
with pytest.raises(RuntimeError, match="provider reached"):
2168+
if operation == "create":
2169+
await client.create(
2170+
manifest=manifest,
2171+
options=DockerSandboxClientOptions(image=DEFAULT_PYTHON_SANDBOX_IMAGE),
2172+
)
2173+
else:
2174+
await client.resume(state)
2175+
2176+
21072177
@pytest.mark.asyncio
21082178
async def test_docker_create_container_mounts_s3_with_volume_driver_ignoring_mount_pattern(
21092179
monkeypatch: pytest.MonkeyPatch,
@@ -4164,19 +4234,66 @@ async def test_docker_resume_reconnects_serialized_credentialless_state() -> Non
41644234

41654235

41664236
@pytest.mark.asyncio
4237+
@pytest.mark.parametrize("root", ["/mnt/sibling/..", "//mnt/sibling/.."])
4238+
async def test_docker_resume_rejects_host_grant_inside_normalized_persisted_root(
4239+
tmp_path: Path,
4240+
monkeypatch: pytest.MonkeyPatch,
4241+
root: str,
4242+
) -> None:
4243+
client = DockerSandboxClient(docker_client=cast(object, _FakeDockerClient()))
4244+
trusted_manifest = Manifest(
4245+
extra_path_grants=(SandboxPathGrant(path="/mnt/shared-data", host_path=str(tmp_path)),),
4246+
)
4247+
state = DockerSandboxSessionState(
4248+
manifest=trusted_manifest,
4249+
snapshot=NoopSnapshot(id="snapshot"),
4250+
image=DEFAULT_PYTHON_SANDBOX_IMAGE,
4251+
container_id="container",
4252+
workspace_root_ready=True,
4253+
)
4254+
payload = client.serialize_session_state(state)
4255+
manifest_payload = payload["manifest"]
4256+
assert isinstance(manifest_payload, dict)
4257+
manifest_payload["root"] = root
4258+
restored = client.deserialize_session_state(payload).rebind_persisted_path_grants(
4259+
trusted_manifest
4260+
)
4261+
assert isinstance(restored, DockerSandboxSessionState)
4262+
4263+
def unexpected_lookup(_container_id: str) -> None:
4264+
pytest.fail("overlapping host grants must be rejected before Docker lookup")
4265+
4266+
async def unexpected_create(*args: object, **kwargs: object) -> None:
4267+
pytest.fail("overlapping host grants must be rejected before Docker creation")
4268+
4269+
monkeypatch.setattr(client, "get_container", unexpected_lookup)
4270+
monkeypatch.setattr(client, "_create_container", unexpected_create)
4271+
4272+
with pytest.raises(ValueError, match="host_path target must be outside the workspace root"):
4273+
await client.resume(restored)
4274+
4275+
assert restored.container_id == state.container_id
4276+
assert restored.session_id == state.session_id
4277+
assert restored.workspace_root_ready is True
4278+
4279+
4280+
@pytest.mark.asyncio
4281+
@pytest.mark.parametrize("root", ["/workspace", "/mnt/sibling/../workspace"])
41674282
async def test_docker_resume_requires_existing_host_mount_to_match_trusted_state(
41684283
tmp_path: Path,
4284+
root: str,
41694285
) -> None:
41704286
host_path = tmp_path / "shared-data"
41714287
host_path.mkdir()
41724288
manifest = Manifest(
4289+
root=root,
41734290
extra_path_grants=(
41744291
SandboxPathGrant(
41754292
path="/mnt/shared-data",
41764293
host_path=str(host_path),
41774294
read_only=True,
41784295
),
4179-
)
4296+
),
41804297
)
41814298
matching_client = DockerSandboxClient(
41824299
docker_client=_ResumeDockerClient(

0 commit comments

Comments
 (0)