files: follow in-root symlinks via a kernel-faithful confined resolver

plugins/runtime-basic/agentix/files/__init__.py

@@ -20,18 +20,26 @@
 import for type hints.
 
 Writes/reads are confined to `$AGENTIX_UPLOAD_ROOT` (default
-`/workspace`). Paths outside that root raise `PermissionError`.
+`/workspace`). The invariant is on the RESOLVED target: symlinks are
+followed as long as every hop stays under the root (merged-usr images
+make `/bin` a symlink; real repos contain symlinks), and any hop whose
+target lands outside the root raises `PermissionError`.
 """
 
 from __future__ import annotations
 
 import errno
 import os
+from collections import deque
 from dataclasses import dataclass
 from pathlib import Path
 
 UPLOAD_ROOT = Path(os.environ.get("AGENTIX_UPLOAD_ROOT", "/workspace")).resolve()
 
+# Linux's own resolution cap is 40; a cycle inside the root would
+# otherwise loop the walk forever.
+_MAX_SYMLINK_HOPS = 40
+
 
 @dataclass
 class UploadResult:
@@ -41,16 +49,6 @@ class UploadResult:
     size: int
 
 
-def _resolve_within(path: str) -> Path:
-    """Return `path` lexically under `UPLOAD_ROOT`.
-
-    Actual open/read/write calls below walk from an already-open root
-    directory fd with O_NOFOLLOW, so symlink swaps cannot redirect the
-    final file operation outside the upload root.
-    """
-    return UPLOAD_ROOT / Path(*_relative_parts(path))
-
-
 def _relative_parts(path: str) -> tuple[str, ...]:
     raw = os.path.normpath(path)
     if os.path.isabs(raw):
@@ -67,55 +65,120 @@ def _relative_parts(path: str) -> tuple[str, ...]:
     return parts
 
 
-def _open_parent(parts: tuple[str, ...], *, create: bool) -> tuple[int, str]:
-    """Open the parent directory under UPLOAD_ROOT without following symlinks.
+def _is_symlink(exc: OSError, part: str, dir_fd: int) -> bool:
+    """Did an O_NOFOLLOW open fail because `part` is a symlink?
 
-    Returns `(parent_fd, filename)`. The caller owns `parent_fd`.
+    Linux reports ELOOP; macOS reports ENOTDIR for the O_DIRECTORY case —
+    and ENOTDIR is ambiguous with a genuine regular-file-in-the-middle, so
+    readlink is the arbiter.
     """
-    root_fd = os.open(UPLOAD_ROOT, os.O_RDONLY | os.O_DIRECTORY)
-    fd = root_fd
+    if exc.errno not in (errno.ELOOP, errno.ENOTDIR):
+        return False
     try:
-        for part in parts[:-1]:
-            if create:
+        os.readlink(part, dir_fd=dir_fd)
+    except OSError:
+        return False
+    return True
+
+
+def _link_components(dir_fd: int, name: str) -> tuple[bool, list[str]]:
+    """`(is_absolute, components)` of the symlink `name` under `dir_fd`.
+
+    Components keep `..` verbatim — resolution happens against real directory
+    fds in the walk, never by lexical normalization (which would apply `..`
+    before the preceding symlink is resolved and pick the wrong file)."""
+    target = os.readlink(name, dir_fd=dir_fd)
+    comps = [c for c in target.split("/") if c not in ("", ".")]
+    return os.path.isabs(target), comps
+
+
+def _open_file(path: str, flags: int, *, create_parents: bool, mode: int = 0o666) -> tuple[int, Path]:
+    """Open `path` confined under `UPLOAD_ROOT`, following in-root symlinks.
+
+    A kernel-faithful (``RESOLVE_IN_ROOT``-style) walk that makes escape
+    structurally impossible rather than lexically checked:
+
+    - the root dir is opened ONCE with ``O_NOFOLLOW`` and retained for the
+      whole walk (never reopened by path), so a concurrent rename/replace of
+      the root cannot redirect a later step outside the jail;
+    - every component opens with ``O_NOFOLLOW``; a symlink is read and its
+      target's components are pushed onto the queue — absolute targets
+      re-anchor at the root (chroot-like), relative targets resolve against
+      the current directory;
+    - ``..`` pops the open-dir stack, clamped at the root — you can never
+      ascend above it — so ``..`` is applied against actually-resolved
+      directories, not collapsed lexically.
+    """
+    parts = _relative_parts(path)
+    root_fd = os.open(UPLOAD_ROOT, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW)
+    dir_stack = [root_fd]  # owned fds; dir_stack[-1] is the current directory
+    name_stack: list[str] = []  # resolved component names from the root
+    queue = deque(parts)
+    hops = 0
+    try:
+        while queue:
+            comp = queue.popleft()
+            if comp == "..":
+                if len(dir_stack) > 1:
+                    os.close(dir_stack.pop())
+                    name_stack.pop()
+                continue  # at root, `..` clamps to root
+            cur = dir_stack[-1]
+            is_last = not queue
+
+            if is_last:
+                try:
+                    final_fd = os.open(comp, flags | os.O_NOFOLLOW, mode, dir_fd=cur)
+                except OSError as exc:
+                    if not _is_symlink(exc, comp, cur):
+                        raise
+                    hops = _bump_hops(hops, path)
+                    absolute, comps = _link_components(cur, comp)
+                    if absolute:
+                        _reset_to_root(dir_stack, name_stack)
+                    queue.extendleft(reversed(comps))
+                    continue
+                return final_fd, UPLOAD_ROOT.joinpath(*name_stack, comp)
+
+            if create_parents:
                 try:
-                    os.mkdir(part, mode=0o777, dir_fd=fd)
+                    os.mkdir(comp, mode=0o777, dir_fd=cur)
                 except FileExistsError:
                     pass
             try:
-                next_fd = os.open(
-                    part,
-                    os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW,
-                    dir_fd=fd,
-                )
+                next_fd = os.open(comp, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, dir_fd=cur)
             except OSError as exc:
-                if exc.errno == errno.ELOOP:
-                    raise PermissionError(
-                        f"Refusing to follow symlink inside {UPLOAD_ROOT}"
-                    ) from exc
-                raise
+                if not _is_symlink(exc, comp, cur):
+                    raise
+                hops = _bump_hops(hops, path)
+                absolute, comps = _link_components(cur, comp)
+                if absolute:
+                    _reset_to_root(dir_stack, name_stack)
+                queue.extendleft(reversed(comps))
+                continue
+            dir_stack.append(next_fd)
+            name_stack.append(comp)
+        # The path resolved to a directory (the root itself, or a resolved
+        # dir) with no final file component.
+        raise IsADirectoryError(f"{path!r} resolves to a directory under {UPLOAD_ROOT}")
+    finally:
+        for fd in dir_stack:
             os.close(fd)
-            fd = next_fd
-        return fd, parts[-1]
-    except Exception:
-        os.close(fd)
-        raise
 
 
-def _open_file(path: str, flags: int, *, create_parents: bool, mode: int = 0o666) -> tuple[int, Path]:
-    parts = _relative_parts(path)
-    parent_fd, name = _open_parent(parts, create=create_parents)
-    try:
-        try:
-            fd = os.open(name, flags | os.O_NOFOLLOW, mode, dir_fd=parent_fd)
-        except OSError as exc:
-            if exc.errno == errno.ELOOP:
-                raise PermissionError(
-                    f"Refusing to follow symlink inside {UPLOAD_ROOT}"
-                ) from exc
-            raise
-        return fd, _resolve_within(path)
-    finally:
-        os.close(parent_fd)
+def _bump_hops(hops: int, path: str) -> int:
+    hops += 1
+    if hops > _MAX_SYMLINK_HOPS:
+        raise PermissionError(f"Too many symlink hops resolving {path!r} under {UPLOAD_ROOT}")
+    return hops
+
+
+def _reset_to_root(dir_stack: list[int], name_stack: list[str]) -> None:
+    """Drop every resolved directory back to the retained root fd — an
+    absolute symlink target is interpreted relative to the jail root."""
+    while len(dir_stack) > 1:
+        os.close(dir_stack.pop())
+    name_stack.clear()
 
 
 async def upload(path: str, content: bytes) -> UploadResult:

plugins/runtime-basic/tests/test_primitives.py

@@ -12,22 +12,21 @@
 
 
 @pytest.mark.asyncio
-async def test_files_upload_refuses_symlink_escape(tmp_path, monkeypatch):
+async def test_files_upload_symlink_to_outside_cannot_touch_the_real_target(tmp_path, monkeypatch):
+    """An absolute symlink pointing outside the root is re-anchored at the root
+    (chroot / RESOLVE_IN_ROOT semantics): the write lands inside the jail and
+    the real out-of-jail file is never touched — escape is structurally
+    impossible, not merely refused."""
     outside = tmp_path / "outside.txt"
     root = tmp_path / "workspace"
     root.mkdir()
-    monkeypatch.setenv("AGENTIX_UPLOAD_ROOT", str(root))
-    # files reads UPLOAD_ROOT from env at import time; force a reload so
-    # the patched env takes effect for this test.
-    import importlib
-    importlib.reload(files)
+    monkeypatch.setattr(files, "UPLOAD_ROOT", root.resolve())
 
     link = root / "link"
-    link.symlink_to(outside)
-
-    with pytest.raises(PermissionError):
-        await files.upload(str(link), b"escape")
-    assert not outside.exists()
+    link.symlink_to(outside)  # absolute target, outside the root
+    result = await files.upload("link", b"escape")
+    assert not outside.exists()  # the real out-of-jail path is untouched
+    assert Path(result.path).resolve().is_relative_to(root.resolve())
 
 
 @pytest.mark.asyncio
@@ -153,3 +152,154 @@ def test_shell_executable_prefers_image_bash_in_clean_mode(tmp_path):
     # without clean mode the bundled-bash preference is unchanged (falls back
     # to PATH lookup here because /nix/runtime/bin/bash does not exist)
     assert bash._shell_executable(None, {"PATH": str(fakebin)}) == str(image_bash)
+def _set_files_root(monkeypatch, root):
+    # UPLOAD_ROOT is computed from env at import time; patch the module
+    # attribute directly — reloading the module would re-create its classes
+    # and break identity for everything that imported them.
+    monkeypatch.setattr(files, "UPLOAD_ROOT", root.resolve())
+
+
+@pytest.mark.asyncio
+async def test_files_follows_directory_symlinks_inside_root(tmp_path, monkeypatch):
+    """merged-usr layout: /bin -> usr/bin. Symlinks whose targets stay under
+    the root are legitimate filesystem structure, not an escape."""
+    root = tmp_path / "rootfs"
+    (root / "usr" / "bin").mkdir(parents=True)
+    (root / "bin").symlink_to("usr/bin")  # relative, as real merged-usr images store it
+    _set_files_root(monkeypatch, root)
+
+    result = await files.upload("bin/tool.sh", b"#!/bin/sh\n")
+    assert (root / "usr" / "bin" / "tool.sh").read_bytes() == b"#!/bin/sh\n"
+    assert result.size == 10
+    assert await files.download("bin/tool.sh") == b"#!/bin/sh\n"
+
+
+@pytest.mark.asyncio
+async def test_files_follows_relative_symlinks_inside_root(tmp_path, monkeypatch):
+    root = tmp_path / "rootfs"
+    (root / "usr" / "lib").mkdir(parents=True)
+    (root / "lib").symlink_to("usr/lib")  # relative target, like real images
+    _set_files_root(monkeypatch, root)
+
+    await files.upload("lib/marker", b"x")
+    assert (root / "usr" / "lib" / "marker").read_bytes() == b"x"
+
+
+@pytest.mark.asyncio
+async def test_files_follows_file_symlink_inside_root(tmp_path, monkeypatch):
+    root = tmp_path / "rootfs"
+    root.mkdir()
+    (root / "real.txt").write_bytes(b"data")
+    (root / "alias.txt").symlink_to("real.txt")
+    _set_files_root(monkeypatch, root)
+
+    assert await files.download("alias.txt") == b"data"
+    await files.upload("alias.txt", b"new")
+    assert (root / "real.txt").read_bytes() == b"new"
+
+
+@pytest.mark.asyncio
+async def test_files_symlinks_pointing_outside_cannot_read_or_write_the_real_target(tmp_path, monkeypatch):
+    """Absolute out-of-jail symlinks re-anchor at the root: reads miss the real
+    file and writes land inside the jail — the real out-of-jail data is
+    never reached."""
+    outside = tmp_path / "outside"
+    outside.mkdir()
+    (outside / "secret.txt").write_bytes(b"SECRET")
+    root = tmp_path / "rootfs"
+    root.mkdir()
+    (root / "evil").symlink_to(outside)  # absolute -> re-anchored under root
+    (root / "evil_file").symlink_to(outside / "secret.txt")
+    _set_files_root(monkeypatch, root)
+
+    await files.upload("evil/x.txt", b"boom")
+    assert not (outside / "x.txt").exists()  # write stayed inside the jail
+
+    with pytest.raises(FileNotFoundError):
+        await files.download("evil_file")
+    assert (outside / "secret.txt").read_bytes() == b"SECRET"  # untouched
+
+
+@pytest.mark.asyncio
+async def test_files_symlink_cycle_is_refused(tmp_path, monkeypatch):
+    root = tmp_path / "rootfs"
+    root.mkdir()
+    (root / "a").symlink_to("b")
+    (root / "b").symlink_to("a")
+    _set_files_root(monkeypatch, root)
+
+    with pytest.raises(PermissionError, match="symlink"):
+        await files.download("a")
+
+
+@pytest.mark.asyncio
+async def test_files_nested_link_with_dotdot_resolves_like_posix(tmp_path, monkeypatch):
+    """`alias -> inner/../victim.txt` where `inner -> dest/sub`: POSIX resolves
+    to dest/victim.txt (`..` applies AFTER the inner symlink), not root-level
+    victim.txt. Lexical normpath of the target gets this wrong."""
+    root = tmp_path / "rootfs"
+    (root / "dest" / "sub").mkdir(parents=True)
+    (root / "dest" / "victim.txt").write_bytes(b"correct")
+    (root / "victim.txt").write_bytes(b"WRONG")
+    (root / "inner").symlink_to("dest/sub")
+    (root / "alias").symlink_to("inner/../victim.txt")
+    _set_files_root(monkeypatch, root)
+
+    assert await files.download("alias") == b"correct"
+
+
+@pytest.mark.asyncio
+async def test_files_link_to_link_chain_resolves(tmp_path, monkeypatch):
+    root = tmp_path / "rootfs"
+    (root / "a" / "b").mkdir(parents=True)
+    (root / "a" / "b" / "real.txt").write_bytes(b"deep")
+    (root / "hop1").symlink_to("hop2")
+    (root / "hop2").symlink_to("a/b/real.txt")
+    _set_files_root(monkeypatch, root)
+
+    assert await files.download("hop1") == b"deep"
+
+
+@pytest.mark.asyncio
+async def test_files_symlink_target_dotdot_cannot_escape_root(tmp_path, monkeypatch):
+    """A `..`-bearing target that would climb above root is clamped at root
+    (kernel RESOLVE_IN_ROOT semantics), never followed outside."""
+    outside = tmp_path / "outside"
+    outside.mkdir()
+    (outside / "secret.txt").write_bytes(b"SECRET")
+    root = tmp_path / "rootfs"
+    root.mkdir()
+    (root / "escape").symlink_to("../outside/secret.txt")
+    _set_files_root(monkeypatch, root)
+
+    with pytest.raises(FileNotFoundError):
+        # ../outside clamps to root/outside (does not exist) — never the real
+        # sibling outside the jail.
+        await files.download("escape")
+    assert (outside / "secret.txt").read_bytes() == b"SECRET"  # untouched
+
+
+@pytest.mark.asyncio
+async def test_files_absolute_symlink_target_is_reanchored_at_root(tmp_path, monkeypatch):
+    """An absolute symlink target is interpreted relative to the jail root
+    (chroot-like), so /etc/x -> root/etc/x, never the host's /etc."""
+    root = tmp_path / "rootfs"
+    (root / "etc").mkdir(parents=True)
+    (root / "etc" / "conf").write_bytes(b"jailed")
+    (root / "link").symlink_to("/etc/conf")
+    _set_files_root(monkeypatch, root)
+
+    assert await files.download("link") == b"jailed"
+
+
+@pytest.mark.asyncio
+async def test_files_link_resolving_exactly_to_root_then_suffix(tmp_path, monkeypatch):
+    """A dir symlink whose target is the root itself must still allow a suffix
+    under it (empty resolved prefix is valid, not an escape)."""
+    root = tmp_path / "rootfs"
+    (root / "sub").mkdir(parents=True)
+    (root / "sub" / "up").symlink_to("..")  # -> root
+    (root / "target.txt").write_bytes(b"hit")
+    _set_files_root(monkeypatch, root)
+
+    assert await files.download("sub/up/target.txt") == b"hit"