diff --git a/src/agentic_cli/file_utils.py b/src/agentic_cli/file_utils.py index aa25e75..a9212c2 100644 --- a/src/agentic_cli/file_utils.py +++ b/src/agentic_cli/file_utils.py @@ -4,6 +4,7 @@ import json import os import re +import stat import tempfile import time from contextlib import contextmanager @@ -88,6 +89,45 @@ def path_is_within(path: Path, root: Path) -> bool: return False +def copy_regular_file_no_follow(src: Path, dst: Path) -> None: + """Copy a regular file src -> dst without following symlinks at either end. + + Source: opened O_RDONLY | O_NOFOLLOW | O_NONBLOCK (final component); fstat + must be a regular file (rejects symlink / dir / FIFO / socket / device). + O_NOFOLLOW closes the check->open TOCTOU on the final component; O_NONBLOCK + ensures a FIFO/device source fails fast instead of blocking the open + (regular files ignore O_NONBLOCK for reads). + Dest: written to a private temp file in dst.parent, fsync'd, then + os.replace()'d over dst — a pre-planted symlink at dst is replaced, not + written through. + + Raises OSError (source open incl. ELOOP for a final-component symlink) or + ValueError (source not a regular file). POSIX (macOS/Linux). + """ + fd = os.open(src, os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK) + try: + if not stat.S_ISREG(os.fstat(fd).st_mode): + raise ValueError(f"not a regular file: {src}") + dst.parent.mkdir(parents=True, exist_ok=True) + tfd, tmp = tempfile.mkstemp(dir=str(dst.parent), prefix=f".{dst.name}.", suffix=".tmp") + tmp_path = Path(tmp) + try: + with os.fdopen(tfd, "wb") as out: + while True: + chunk = os.read(fd, 1 << 20) + if not chunk: + break + out.write(chunk) + out.flush() + os.fsync(out.fileno()) + os.replace(tmp_path, dst) + except BaseException: + tmp_path.unlink(missing_ok=True) + raise + finally: + os.close(fd) + + def _atomic_write(path: Path, content: str) -> None: """Write content to a file atomically and durably. diff --git a/src/agentic_cli/tools/sandbox/backends/jupyter_docker.py b/src/agentic_cli/tools/sandbox/backends/jupyter_docker.py index 68a718e..958ee77 100644 --- a/src/agentic_cli/tools/sandbox/backends/jupyter_docker.py +++ b/src/agentic_cli/tools/sandbox/backends/jupyter_docker.py @@ -5,14 +5,14 @@ import json import os import queue -import shutil +import stat import threading import time from pathlib import Path from agentic_cli.logging import Loggers from agentic_cli.tools.sandbox.models import ExecutionResult, SessionStatus -from agentic_cli.file_utils import sanitize_filename +from agentic_cli.file_utils import copy_regular_file_no_follow, sanitize_filename from agentic_cli.tools.sandbox.backends.base import SandboxBackend from agentic_cli.tools.sandbox.backends import kernel_exec from agentic_cli.tools.sandbox.backends.container_runtime import ( @@ -224,11 +224,39 @@ def _outputs_dir(self) -> Path: base = Path(configured) if configured else Path(self._settings.workspace_dir) / "artifacts" base.mkdir(parents=True, exist_ok=True) try: - os.chmod(base, 0o777) # container runs as host uid; keep writable across sessions + os.chmod(base, 0o700) # single-user; not world-accessible except OSError: pass return base + def _collect_outputs(self, working_dir: Path) -> list[str]: + """Copy regular files from /outputs into the shared outputs + dir without following symlinks. Skips symlinks / special files (a kernel + could plant `outputs/x -> /host/secret`) and a symlinked `outputs` dir. + Never raises — best-effort contract so execute() is not disrupted.""" + session_outs = Path(working_dir) / "outputs" + try: + if not stat.S_ISDIR(os.lstat(session_outs).st_mode): + return [] # 'outputs' is a symlink or not a directory + except OSError: + return [] + collected: list[str] = [] + try: + shared = self._outputs_dir() + entries = sorted(session_outs.iterdir()) + except OSError: + logger.warning("sandbox_outputs_unreadable", path=str(session_outs)) + return collected + for src in entries: + dst = shared / src.name + try: + copy_regular_file_no_follow(src, dst) + except (OSError, ValueError): + logger.warning("sandbox_output_skipped", entry=str(src)) + continue + collected.append(str(dst)) + return collected + def _parse_data_mounts(self) -> list[tuple[str, str]]: """Parse sandbox_data_mounts into (host_path, sanitized_name) pairs.""" result = [] @@ -340,17 +368,9 @@ def execute(self, code, session_id, timeout_seconds=120, working_dir=None, input return ExecutionResult(success=False, error=f"Failed to start sandbox: {exc}") result = session.execute(code, timeout_seconds) if result.success and working_dir is not None: - session_outs = Path(working_dir) / "outputs" - if session_outs.is_dir(): - shared = self._outputs_dir() - extra: list[str] = [] - for src in sorted(session_outs.iterdir()): - if src.is_file(): - dst = shared / src.name - shutil.copy2(src, dst) - extra.append(str(dst)) - if extra: - result.artifacts = list(result.artifacts) + extra + extra = self._collect_outputs(Path(working_dir)) + if extra: + result.artifacts = list(result.artifacts) + extra return result def reset_session(self, session_id: str) -> None: diff --git a/src/agentic_cli/tools/sandbox/manager.py b/src/agentic_cli/tools/sandbox/manager.py index f5f6f4d..5fa83da 100644 --- a/src/agentic_cli/tools/sandbox/manager.py +++ b/src/agentic_cli/tools/sandbox/manager.py @@ -7,13 +7,14 @@ from __future__ import annotations import atexit -import shutil +import os +import stat from dataclasses import dataclass, field from pathlib import Path from typing import Any, TYPE_CHECKING from agentic_cli.logging import Loggers -from agentic_cli.file_utils import sanitize_filename +from agentic_cli.file_utils import copy_regular_file_no_follow, sanitize_filename from agentic_cli.tools.sandbox.models import ExecutionResult if TYPE_CHECKING: @@ -33,16 +34,23 @@ def stage_inputs(session_dir: Path, inputs: list[str]) -> None: return inputs_dir = Path(session_dir) / "inputs" inputs_dir.mkdir(parents=True, exist_ok=True) + if not stat.S_ISDIR(os.lstat(inputs_dir).st_mode): + # 'inputs' is a symlink / not a real dir (kernel-plantable) — O_NOFOLLOW + # and os.replace only guard the final component, not this parent. + raise ValueError("inputs staging directory is not a real directory (symlink?)") seen: set[str] = set() for src in inputs: p = Path(src).expanduser() - if not p.is_file(): - raise ValueError(f"input file not found: {src}") name = p.name if name in seen: raise ValueError(f"duplicate input basename {name!r}; rename one of the source files") seen.add(name) - shutil.copy2(p, inputs_dir / name) + try: + copy_regular_file_no_follow(p, inputs_dir / name) + except FileNotFoundError as exc: + raise ValueError(f"input file not found: {src}") from exc + except (OSError, ValueError) as exc: + raise ValueError(f"input must be a regular file: {src}") from exc def sandbox_disabled_reason(settings) -> str: diff --git a/tests/test_file_utils_nofollow.py b/tests/test_file_utils_nofollow.py new file mode 100644 index 0000000..2d3b977 --- /dev/null +++ b/tests/test_file_utils_nofollow.py @@ -0,0 +1,50 @@ +import os + +import pytest + +from agentic_cli.file_utils import copy_regular_file_no_follow + + +def test_copies_regular_file(tmp_path): + src = tmp_path / "a.txt"; src.write_bytes(b"hello") + dst = tmp_path / "out" / "a.txt" + copy_regular_file_no_follow(src, dst) + assert dst.read_bytes() == b"hello" + assert not dst.is_symlink() + + +def test_rejects_symlink_source(tmp_path): + real = tmp_path / "real.txt"; real.write_bytes(b"secret") + link = tmp_path / "link.txt"; link.symlink_to(real) + with pytest.raises(OSError): # O_NOFOLLOW -> ELOOP on the final component + copy_regular_file_no_follow(link, tmp_path / "out.txt") + + +def test_rejects_fifo_source_without_hanging(tmp_path): + import threading + fifo = tmp_path / "pipe"; os.mkfifo(fifo) + result = {} + def run(): + try: + copy_regular_file_no_follow(fifo, tmp_path / "out.txt") + except (OSError, ValueError): + result["rejected"] = True + t = threading.Thread(target=run, daemon=True); t.start(); t.join(5) + assert not t.is_alive(), "copy hung on a FIFO source (O_NONBLOCK missing?)" + assert result.get("rejected") + + +def test_does_not_follow_dest_symlink(tmp_path): + src = tmp_path / "src.txt"; src.write_bytes(b"NEW") + outside = tmp_path / "outside.txt"; outside.write_bytes(b"ORIGINAL") + deliver = tmp_path / "deliver"; deliver.mkdir() + dst = deliver / "x.txt"; dst.symlink_to(outside) + copy_regular_file_no_follow(src, dst) + assert outside.read_bytes() == b"ORIGINAL" # symlink target untouched + assert dst.read_bytes() == b"NEW" # dst replaced by a real file + assert not dst.is_symlink() + + +def test_missing_source_raises_filenotfound(tmp_path): + with pytest.raises(FileNotFoundError): + copy_regular_file_no_follow(tmp_path / "nope.txt", tmp_path / "out.txt") diff --git a/tests/tools/test_sandbox_transfer.py b/tests/tools/test_sandbox_transfer.py new file mode 100644 index 0000000..b3975da --- /dev/null +++ b/tests/tools/test_sandbox_transfer.py @@ -0,0 +1,107 @@ +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from agentic_cli.tools.sandbox.manager import stage_inputs +from agentic_cli.tools.sandbox.backends.jupyter_docker import JupyterDockerBackend + + +def _backend(shared: Path): + return JupyterDockerBackend( + settings=SimpleNamespace(sandbox_outputs_dir=str(shared), workspace_dir=str(shared)) + ) + + +def test_stage_inputs_copies_regular_file(tmp_path): + session = tmp_path / "session"; session.mkdir() + src = tmp_path / "data.csv"; src.write_bytes(b"col1,col2") + stage_inputs(session, [str(src)]) + staged = session / "inputs" / "data.csv" + assert staged.read_bytes() == b"col1,col2" and not staged.is_symlink() + + +def test_stage_inputs_rejects_symlink_source(tmp_path): + session = tmp_path / "session"; session.mkdir() + secret = tmp_path / "secret.txt"; secret.write_bytes(b"TOP SECRET") + link = tmp_path / "data.csv"; link.symlink_to(secret) + with pytest.raises(ValueError): + stage_inputs(session, [str(link)]) + + +def test_stage_inputs_does_not_follow_dest_symlink(tmp_path): + # Simulate a kernel that pre-planted inputs/data.csv -> a host file. + session = tmp_path / "session"; session.mkdir() + inputs_dir = session / "inputs"; inputs_dir.mkdir() + outside = tmp_path / "host_secret"; outside.write_bytes(b"ORIGINAL") + (inputs_dir / "data.csv").symlink_to(outside) + src = tmp_path / "data.csv"; src.write_bytes(b"INPUT") + stage_inputs(session, [str(src)]) + assert outside.read_bytes() == b"ORIGINAL" # not written through + assert (inputs_dir / "data.csv").read_bytes() == b"INPUT" # replaced by a real file + assert not (inputs_dir / "data.csv").is_symlink() + + +def test_stage_inputs_duplicate_basename_errors(tmp_path): + session = tmp_path / "session"; session.mkdir() + a = tmp_path / "a" / "x.csv"; a.parent.mkdir(); a.write_text("1") + b = tmp_path / "b" / "x.csv"; b.parent.mkdir(); b.write_text("2") + with pytest.raises(ValueError): + stage_inputs(session, [str(a), str(b)]) + + +def test_collect_outputs_copies_regular_files(tmp_path): + shared = tmp_path / "shared" + wd = tmp_path / "wd"; (wd / "outputs").mkdir(parents=True) + (wd / "outputs" / "plot.png").write_bytes(b"PNG") + got = _backend(shared)._collect_outputs(wd) + assert (shared / "plot.png").read_bytes() == b"PNG" + assert any("plot.png" in g for g in got) + + +def test_collect_outputs_skips_symlink_to_host_secret(tmp_path): + shared = tmp_path / "shared" + wd = tmp_path / "wd"; (wd / "outputs").mkdir(parents=True) + secret = tmp_path / "aws_credentials"; secret.write_bytes(b"AKIA-SECRET") + (wd / "outputs" / "result.txt").symlink_to(secret) # kernel exfil attempt + got = _backend(shared)._collect_outputs(wd) + assert got == [] + assert not (shared / "result.txt").exists() # secret not copied out + + +def test_collect_outputs_skips_when_outputs_is_symlink(tmp_path): + shared = tmp_path / "shared" + wd = tmp_path / "wd"; wd.mkdir() + elsewhere = tmp_path / "elsewhere"; elsewhere.mkdir() + (elsewhere / "x.txt").write_text("data") + (wd / "outputs").symlink_to(elsewhere) # 'outputs' itself a symlink + assert _backend(shared)._collect_outputs(wd) == [] + + +def test_outputs_dir_is_0700(tmp_path): + shared = tmp_path / "shared" + base = _backend(shared)._outputs_dir() + assert (base.stat().st_mode & 0o777) == 0o700 + + +def test_stage_inputs_rejects_symlinked_inputs_dir(tmp_path): + # Kernel plants session/inputs as a symlink to a host dir -> must be rejected, + # not written through (O_NOFOLLOW guards only the final component). + session = tmp_path / "session"; session.mkdir() + outside = tmp_path / "host_dir"; outside.mkdir() + (session / "inputs").symlink_to(outside, target_is_directory=True) + src = tmp_path / "data.csv"; src.write_bytes(b"INPUT") + with pytest.raises(ValueError): + stage_inputs(session, [str(src)]) + assert list(outside.iterdir()) == [] # nothing written into the host dir + + +def test_collect_outputs_never_raises_on_outputs_dir_error(tmp_path, monkeypatch): + shared = tmp_path / "shared" + wd = tmp_path / "wd"; (wd / "outputs").mkdir(parents=True) + (wd / "outputs" / "a.txt").write_text("x") + b = _backend(shared) + def boom(): + raise OSError("boom") + monkeypatch.setattr(b, "_outputs_dir", boom) + assert b._collect_outputs(wd) == [] # best-effort: never raises