diff --git a/coding-agent/.opencode/prompts/tasks/adr-0019-pr6-workspace-archive-limits.md b/coding-agent/.opencode/prompts/tasks/adr-0019-pr6-workspace-archive-limits.md new file mode 100644 index 000000000..a67bbff64 --- /dev/null +++ b/coding-agent/.opencode/prompts/tasks/adr-0019-pr6-workspace-archive-limits.md @@ -0,0 +1,36 @@ +Goal: +Add explicit archive size and extracted-content limits to ADR-0019 workspace transfer so oversized uploads/downloads fail fast instead of consuming unbounded memory. + +Scope: +- Enforce shared workspace archive byte limits in `coding_agent.workspace_archive` for both archive creation and extraction. +- Reject oversized snapshot uploads and oversized workspace exports through the existing HTTP surfaces. +- Cover the helper and HTTP transfer paths with focused regression tests. + +Out of scope: +- Do not change archive format, owner fencing, or remote client protocol. +- Do not add streaming sync or chunked upload/download. +- Do not refactor unrelated Docker workspace provider or CLI workflow code. + +Context: +- ADRs: + - `docs/adr/0019-remote-client-cloud-workspace-deployment.md` +- Relevant files: + - `src/coding_agent/workspace_archive.py` + - `tests/coding_agent/environment/test_workspace_archive.py` + - `tests/ui/test_http_server_workspace_transfer.py` + +Target tests: +- `uv run pytest tests/coding_agent/environment/test_workspace_archive.py -v` +- `uv run pytest tests/ui/test_http_server_workspace_transfer.py -v` + +Loop policy: +- Engineer implements the smallest correct change and runs the target tests. +- Reviewer reviews only the resulting diff and affected tests. +- Reviewer reports only P1/P2 findings. +- Engineer fixes only accepted P1/P2 findings and reruns the same target tests. +- Verifier reruns the exact target tests and reports pass/fail only. + +Stop conditions: +- At most one review/fix/retest cycle. +- Escalate any protocol or API-shape changes beyond size-limit hardening. +- Ignore non-blocking optimization suggestions. diff --git a/coding-agent/src/coding_agent/workspace_archive.py b/coding-agent/src/coding_agent/workspace_archive.py index a5ed7b36d..a61683793 100644 --- a/coding-agent/src/coding_agent/workspace_archive.py +++ b/coding-agent/src/coding_agent/workspace_archive.py @@ -1,14 +1,22 @@ from __future__ import annotations import base64 +import gzip import io import os import shutil import tarfile +import tempfile +import zlib from pathlib import Path +from typing import Protocol, override _PRESERVED_ROOT_NAMES = frozenset({".git"}) +_MAX_WORKSPACE_ARCHIVE_BYTES = 8 * 1024 * 1024 +_MAX_WORKSPACE_ARCHIVE_BASE64_CHARS = 4 * ((_MAX_WORKSPACE_ARCHIVE_BYTES + 2) // 3) +_MAX_WORKSPACE_TAR_STREAM_BYTES = 16 * 1024 * 1024 +_MAX_WORKSPACE_ARCHIVE_MEMBERS = 4096 def create_workspace_archive_base64(workspace_root: Path) -> str: @@ -17,23 +25,30 @@ def create_workspace_archive_base64(workspace_root: Path) -> str: raise ValueError(f"workspace root does not exist: {root}") buffer = io.BytesIO() + total_input_bytes = 0 with tarfile.open(fileobj=buffer, mode="w:gz") as archive: for path in sorted(root.rglob("*")): relative = path.relative_to(root) if not relative.parts or relative.parts[0] == ".git": continue if path.is_symlink(): - raise ValueError(f"workspace archive does not support symlinks: {relative}") + raise ValueError( + f"workspace archive does not support symlinks: {relative}" + ) if not path.is_file(): continue stat_result = path.stat() + total_input_bytes += stat_result.st_size + _raise_if_archive_too_large(total_input_bytes) data = path.read_bytes() info = tarfile.TarInfo(name=relative.as_posix()) info.size = len(data) info.mode = stat_result.st_mode & 0o777 info.mtime = int(stat_result.st_mtime) archive.addfile(info, io.BytesIO(data)) - return base64.b64encode(buffer.getvalue()).decode("ascii") + archive_bytes = buffer.getvalue() + _raise_if_archive_too_large(len(archive_bytes)) + return base64.b64encode(archive_bytes).decode("ascii") def extract_workspace_archive_base64(workspace_root: Path, archive_base64: str) -> None: @@ -43,29 +58,40 @@ def extract_workspace_archive_base64(workspace_root: Path, archive_base64: str) if not root.is_dir(): raise ValueError(f"workspace root is not a directory: {root}") + _raise_if_archive_base64_too_large(archive_base64) try: archive_bytes = base64.b64decode(archive_base64.encode("ascii"), validate=True) except ValueError as exc: raise ValueError("workspace archive must be valid base64") from exc + _raise_if_archive_too_large(len(archive_bytes)) + + with tempfile.TemporaryDirectory() as temp_dir: + temp_root = Path(temp_dir).resolve() + try: + _extract_workspace_archive_to_temp(temp_root, archive_bytes) + except (tarfile.TarError, gzip.BadGzipFile, EOFError, zlib.error) as exc: + raise ValueError("workspace archive is not a valid tar.gz") from exc + + _clear_extract_target(root) + _copy_extracted_workspace(temp_root, root) + +def _extract_workspace_archive_to_temp(temp_root: Path, archive_bytes: bytes) -> None: archive_buffer = io.BytesIO(archive_bytes) - try: - with tarfile.open(fileobj=archive_buffer, mode="r:gz") as archive: - for member in archive.getmembers(): - _ = _safe_archive_target(root, member.name) - if not member.isdir() and not member.isfile(): - raise ValueError( - f"workspace archive only supports regular files and directories: {member.name}" - ) - except tarfile.TarError as exc: - raise ValueError("workspace archive is not a valid tar.gz") from exc - - _clear_extract_target(root) - _ = archive_buffer.seek(0) - - with tarfile.open(fileobj=archive_buffer, mode="r:gz") as archive: - for member in archive.getmembers(): - target_path = _safe_archive_target(root, member.name) + gzip_stream = gzip.GzipFile(fileobj=archive_buffer, mode="rb") + counted_stream = _CountingReader( + gzip_stream, + max_bytes=_MAX_WORKSPACE_TAR_STREAM_BYTES, + ) + extracted_bytes = 0 + member_count = 0 + with tarfile.open(fileobj=counted_stream, mode="r|") as archive: + for member in archive: + member_count += 1 + if member_count > _MAX_WORKSPACE_ARCHIVE_MEMBERS: + raise ValueError("workspace archive contains too many members") + _reject_preserved_root_member(member.name) + target_path = _safe_archive_target(temp_root, member.name) if member.isdir(): target_path.mkdir(parents=True, exist_ok=True) target_path.chmod(_member_mode(member)) @@ -74,13 +100,48 @@ def extract_workspace_archive_base64(workspace_root: Path, archive_base64: str) raise ValueError( f"workspace archive only supports regular files and directories: {member.name}" ) + if member.size < 0: + raise ValueError("workspace archive is not a valid tar.gz") + extracted_bytes += member.size + _raise_if_archive_too_large(extracted_bytes) file_obj = archive.extractfile(member) if file_obj is None: - raise ValueError(f"workspace archive member is unreadable: {member.name}") - target_path.parent.mkdir(parents=True, exist_ok=True) - _ = target_path.write_bytes(file_obj.read()) - target_path.chmod(_member_mode(member)) - os.utime(target_path, (member.mtime, member.mtime)) + raise ValueError( + f"workspace archive member is unreadable: {member.name}" + ) + _copy_archive_file(file_obj, target_path, member) + + +def _copy_archive_file( + file_obj: _ReadableBinary, target_path: Path, member: tarfile.TarInfo +) -> None: + target_path.parent.mkdir(parents=True, exist_ok=True) + copied_bytes = 0 + with target_path.open("wb") as target_file: + while True: + chunk = file_obj.read(64 * 1024) + if not chunk: + break + copied_bytes += len(chunk) + if copied_bytes > member.size: + raise ValueError("workspace archive is not a valid tar.gz") + _ = target_file.write(chunk) + if copied_bytes != member.size: + raise ValueError("workspace archive is not a valid tar.gz") + target_path.chmod(_member_mode(member)) + os.utime(target_path, (member.mtime, member.mtime)) + + +def _copy_extracted_workspace(temp_root: Path, workspace_root: Path) -> None: + for source_path in sorted(temp_root.rglob("*")): + relative = source_path.relative_to(temp_root) + target_path = workspace_root / relative + if source_path.is_dir(): + target_path.mkdir(parents=True, exist_ok=True) + target_path.chmod(source_path.stat().st_mode & 0o777) + continue + target_path.parent.mkdir(parents=True, exist_ok=True) + _ = shutil.copy2(source_path, target_path) def _clear_extract_target(workspace_root: Path) -> None: @@ -105,7 +166,9 @@ def _member_mode(member: tarfile.TarInfo) -> int: def _safe_archive_target(workspace_root: Path, member_name: str) -> Path: member_path = Path(member_name) if member_path.is_absolute() or ".." in member_path.parts: - raise ValueError(f"workspace archive member escapes workspace root: {member_name}") + raise ValueError( + f"workspace archive member escapes workspace root: {member_name}" + ) if not member_path.parts: raise ValueError("workspace archive member name must be non-empty") candidate = (workspace_root / member_path).resolve() @@ -116,3 +179,46 @@ def _safe_archive_target(workspace_root: Path, member_name: str) -> Path: f"workspace archive member escapes workspace root: {member_name}" ) from exc return candidate + + +def _reject_preserved_root_member(member_name: str) -> None: + member_path = Path(member_name) + if member_path.parts and member_path.parts[0] in _PRESERVED_ROOT_NAMES: + raise ValueError( + f"workspace archive must not contain preserved root entry: {member_name}" + ) + + +def _raise_if_archive_too_large(size_bytes: int) -> None: + if size_bytes > _MAX_WORKSPACE_ARCHIVE_BYTES: + raise ValueError("workspace archive exceeds 8 MiB limit") + + +def _raise_if_archive_base64_too_large(archive_base64: str) -> None: + if len(archive_base64) > _MAX_WORKSPACE_ARCHIVE_BASE64_CHARS: + raise ValueError("workspace archive exceeds 8 MiB limit") + + +class _ReadableBinary(Protocol): + def read(self, size: int = -1, /) -> bytes: ... + + +class _CountingReader(io.BufferedIOBase): + def __init__(self, stream: _ReadableBinary, *, max_bytes: int) -> None: + self._stream: _ReadableBinary = stream + self._max_bytes: int = max_bytes + self._bytes_read: int = 0 + + @override + def readable(self) -> bool: + return True + + @override + def read(self, size: int | None = -1) -> bytes: + if size is None: + size = -1 + chunk = self._stream.read(size) + self._bytes_read += len(chunk) + if self._bytes_read > self._max_bytes: + raise ValueError("workspace archive exceeds 8 MiB limit") + return chunk diff --git a/coding-agent/tests/coding_agent/environment/test_workspace_archive.py b/coding-agent/tests/coding_agent/environment/test_workspace_archive.py index b37ac12a6..7078511ac 100644 --- a/coding-agent/tests/coding_agent/environment/test_workspace_archive.py +++ b/coding-agent/tests/coding_agent/environment/test_workspace_archive.py @@ -1,6 +1,7 @@ from __future__ import annotations import base64 +import gzip import io import os import tarfile @@ -18,10 +19,10 @@ def test_workspace_archive_round_trips_nested_files(tmp_path: Path) -> None: source = tmp_path / "source" (source / "nested").mkdir(parents=True) - (source / "README.md").write_text("hello\n", encoding="utf-8") - (source / "nested" / "data.txt").write_text("cloud\n", encoding="utf-8") + _ = (source / "README.md").write_text("hello\n", encoding="utf-8") + _ = (source / "nested" / "data.txt").write_text("cloud\n", encoding="utf-8") script = source / "nested" / "run.sh" - script.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + _ = script.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") script.chmod(0o755) timestamp = int(time.time()) - 120 os.utime(script, (timestamp, timestamp)) @@ -54,10 +55,12 @@ def test_workspace_archive_rejects_path_traversal_members(tmp_path: Path) -> Non ) -def test_workspace_archive_rejects_invalid_tar_without_clearing_target(tmp_path: Path) -> None: +def test_workspace_archive_rejects_invalid_tar_without_clearing_target( + tmp_path: Path, +) -> None: target = tmp_path / "repo" target.mkdir() - (target / "keep.txt").write_text("keep\n", encoding="utf-8") + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") with pytest.raises(ValueError, match=r"valid tar\.gz"): _ = extract_workspace_archive_base64( @@ -73,7 +76,7 @@ def test_workspace_archive_rejects_tar_header_error_without_clearing_target( ) -> None: target = tmp_path / "repo" target.mkdir() - (target / "keep.txt").write_text("keep\n", encoding="utf-8") + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") buffer = io.BytesIO() with tarfile.open(fileobj=buffer, mode="w:gz") as archive: @@ -105,7 +108,7 @@ def test_workspace_archive_rejects_symlink_without_clearing_target( ) -> None: target = tmp_path / "repo" target.mkdir() - (target / "keep.txt").write_text("keep\n", encoding="utf-8") + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") buffer = io.BytesIO() with tarfile.open(fileobj=buffer, mode="w:gz") as archive: @@ -144,17 +147,183 @@ def test_workspace_archive_defaults_missing_member_mode_to_readable_file( assert extracted.stat().st_mode & 0o777 == 0o644 -def test_workspace_archive_extract_reconciles_deletions_but_preserves_git(tmp_path: Path) -> None: +def test_workspace_archive_extract_reconciles_deletions_but_preserves_git( + tmp_path: Path, +) -> None: target = tmp_path / "repo" (target / ".git").mkdir(parents=True) - (target / ".git" / "HEAD").write_text("ref: refs/heads/main\n", encoding="utf-8") - (target / "deleted.txt").write_text("stale\n", encoding="utf-8") + _ = (target / ".git" / "HEAD").write_text( + "ref: refs/heads/main\n", encoding="utf-8" + ) + _ = (target / "deleted.txt").write_text("stale\n", encoding="utf-8") source = tmp_path / "source" source.mkdir() - (source / "kept.txt").write_text("fresh\n", encoding="utf-8") + _ = (source / "kept.txt").write_text("fresh\n", encoding="utf-8") extract_workspace_archive_base64(target, create_workspace_archive_base64(source)) assert not (target / "deleted.txt").exists() assert (target / "kept.txt").read_text(encoding="utf-8") == "fresh\n" - assert (target / ".git" / "HEAD").read_text(encoding="utf-8") == "ref: refs/heads/main\n" + assert (target / ".git" / "HEAD").read_text( + encoding="utf-8" + ) == "ref: refs/heads/main\n" + + +def test_workspace_archive_create_rejects_input_larger_than_limit( + tmp_path: Path, +) -> None: + source = tmp_path / "source" + source.mkdir() + _ = (source / "large.bin").write_bytes(b"a" * (8 * 1024 * 1024 + 1)) + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = create_workspace_archive_base64(source) + + +def test_workspace_archive_extract_accepts_payload_at_limit(tmp_path: Path) -> None: + buffer = io.BytesIO() + data = b"a" * (8 * 1024 * 1024) + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + info = tarfile.TarInfo(name="limit.bin") + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + + target = tmp_path / "target" + extract_workspace_archive_base64( + target, + base64.b64encode(buffer.getvalue()).decode("ascii"), + ) + + assert (target / "limit.bin").stat().st_size == 8 * 1024 * 1024 + + +def test_workspace_archive_create_rejects_oversized_file_before_reading( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + source = tmp_path / "source" + source.mkdir() + oversized = source / "large.bin" + _ = oversized.write_bytes(b"a" * (8 * 1024 * 1024 + 1)) + + def fail_if_read(path: Path) -> bytes: + if path == oversized: + raise AssertionError("oversized file should be rejected before read") + return original_read_bytes(path) + + original_read_bytes = Path.read_bytes + monkeypatch.setattr(Path, "read_bytes", fail_if_read) + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = create_workspace_archive_base64(source) + + +def test_workspace_archive_extract_rejects_decoded_archive_larger_than_limit( + tmp_path: Path, +) -> None: + archive_base64 = base64.b64encode(b"a" * (8 * 1024 * 1024 + 1)).decode("ascii") + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = extract_workspace_archive_base64(tmp_path / "target", archive_base64) + + +def test_workspace_archive_extract_rejects_encoded_archive_larger_than_limit_before_decode( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + archive_base64 = "A" * (4 * (((8 * 1024 * 1024) + 2) // 3) + 4) + + def fail_if_decode_is_called(*_args: object, **_kwargs: object) -> bytes: + raise AssertionError("oversized base64 should be rejected before decode") + + monkeypatch.setattr(base64, "b64decode", fail_if_decode_is_called) + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = extract_workspace_archive_base64(tmp_path / "target", archive_base64) + + +def test_workspace_archive_extract_rejects_member_payload_larger_than_limit( + tmp_path: Path, +) -> None: + buffer = io.BytesIO() + data = b"a" * (8 * 1024 * 1024 + 1) + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + info = tarfile.TarInfo(name="large.bin") + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = extract_workspace_archive_base64( + tmp_path / "target", + base64.b64encode(buffer.getvalue()).decode("ascii"), + ) + + +def test_workspace_archive_extract_rejects_tar_stream_larger_than_limit_without_clearing_target( + tmp_path: Path, +) -> None: + target = tmp_path / "repo" + target.mkdir() + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") + tar_stream = io.BytesIO() + with tarfile.open(fileobj=tar_stream, mode="w:") as archive: + for index in range(4096): + info = tarfile.TarInfo(name=f"dir-{index}") + info.type = tarfile.DIRTYPE + info.pax_headers = {"comment": "x" * 4096} + archive.addfile(info) + file_data = b"ok\n" + file_info = tarfile.TarInfo(name="ok.txt") + file_info.size = len(file_data) + archive.addfile(file_info, io.BytesIO(file_data)) + _ = tar_stream.write(b"\0" * (16 * 1024 * 1024)) + archive_base64 = base64.b64encode(gzip.compress(tar_stream.getvalue())).decode( + "ascii" + ) + + with pytest.raises(ValueError, match="exceeds 8 MiB limit"): + _ = extract_workspace_archive_base64(target, archive_base64) + + assert (target / "keep.txt").read_text(encoding="utf-8") == "keep\n" + + +def test_workspace_archive_extract_rejects_too_many_members_without_clearing_target( + tmp_path: Path, +) -> None: + target = tmp_path / "repo" + target.mkdir() + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + for index in range(4097): + info = tarfile.TarInfo(name=f"dir-{index}") + info.type = tarfile.DIRTYPE + archive.addfile(info) + + with pytest.raises(ValueError, match="too many members"): + _ = extract_workspace_archive_base64( + target, + base64.b64encode(buffer.getvalue()).decode("ascii"), + ) + + assert (target / "keep.txt").read_text(encoding="utf-8") == "keep\n" + + +def test_workspace_archive_extract_rejects_git_members_without_clearing_target( + tmp_path: Path, +) -> None: + target = tmp_path / "repo" + target.mkdir() + _ = (target / "keep.txt").write_text("keep\n", encoding="utf-8") + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + data = b"ref: refs/heads/main\n" + info = tarfile.TarInfo(name=".git/HEAD") + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + + with pytest.raises(ValueError, match="preserved root entry"): + _ = extract_workspace_archive_base64( + target, + base64.b64encode(buffer.getvalue()).decode("ascii"), + ) + + assert (target / "keep.txt").read_text(encoding="utf-8") == "keep\n" diff --git a/coding-agent/tests/ui/test_http_server_workspace_transfer.py b/coding-agent/tests/ui/test_http_server_workspace_transfer.py index 48b05f3ac..b981431f8 100644 --- a/coding-agent/tests/ui/test_http_server_workspace_transfer.py +++ b/coding-agent/tests/ui/test_http_server_workspace_transfer.py @@ -388,3 +388,68 @@ async def test_get_workspace_archive_returns_500_for_unexpected_runtime_error( response = await client_500.get("/sessions/sess-fail/workspace") assert response.status_code == 500 + + +async def test_create_session_rejects_snapshot_archive_larger_than_limit( + client: AsyncClient, monkeypatch, tmp_path: Path +) -> None: + monkeypatch.setattr( + "coding_agent.environment.docker_workspace_provider._start_docker_workspace_container", + lambda provider_config, binding: None, + ) + monkeypatch.setattr( + "coding_agent.ui.http_server._load_cloud_workspace_config", + lambda: { + "enabled": True, + "provider": "docker", + "workspace_root": str(tmp_path), + "container_name_prefix": "agent-", + }, + ) + + response = await client.post( + "/sessions", + json={ + "workspace_source": { + "kind": "docker", + "snapshot_archive_base64": base64.b64encode( + b"a" * (8 * 1024 * 1024 + 1) + ).decode("ascii"), + } + }, + ) + + assert response.status_code == 400 + assert response.json()["detail"] == "workspace archive exceeds 8 MiB limit" + + +async def test_get_workspace_archive_returns_400_for_oversized_workspace_export( + client: AsyncClient, monkeypatch, tmp_path: Path +) -> None: + monkeypatch.setattr( + "coding_agent.environment.docker_workspace_provider._start_docker_workspace_container", + lambda provider_config, binding: None, + ) + monkeypatch.setattr( + "coding_agent.ui.http_server._load_cloud_workspace_config", + lambda: { + "enabled": True, + "provider": "docker", + "workspace_root": str(tmp_path), + "container_name_prefix": "agent-", + }, + ) + + binding = CloudWorkspaceBinding( + workspace_url="docker://agent-ws-large/workspace", + workspace_id="ws-large", + ) + workspace_root = tmp_path / binding.workspace_id + workspace_root.mkdir(parents=True) + (workspace_root / "large.bin").write_bytes(b"a" * (8 * 1024 * 1024 + 1)) + _register_cloud_session("sess-large", binding) + + response = await client.get("/sessions/sess-large/workspace") + + assert response.status_code == 400 + assert response.json()["detail"] == "workspace archive exceeds 8 MiB limit"