fix(sandbox): symlink-safe host file transfer (P0-2)

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.
 

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 <working_dir>/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:

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:

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")

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