Merge pull request #4433 from nesquena/release/stage-4410

Release v0.51.497 — RG (rollback checkpoint diffs no longer follow symlinks, #4410) [security]

CHANGELOG.md

@@ -3,6 +3,12 @@
 
 ## [Unreleased]
 
+## [v0.51.497] — 2026-06-18 — Release RG (rollback checkpoint diffs no longer follow symlinks)
+
+### Security
+
+- **Rollback checkpoint diffs no longer follow checkpoint or workspace symlinks when rendering file contents (#4410).** The diff path previously read tracked checkpoint files and workspace files through pathname APIs, so a symlink entry could redirect the rendered diff to a readable file outside the checkpoint/workspace boundary. Diff reads now reject non-regular checkpoint entries (using the git index mode as source of truth) and read content from the git object database rather than the worktree path; workspace-side content goes through the workspace's anchored file reader, and restore also skips symlink checkpoint sources. Companion to the v0.51.491 restore-write containment (#4405). Thanks @Hinotoi-agent.
+
 ## [v0.51.496] — 2026-06-18 — Release RF (compact provider quota chip on narrow composer)
 
 ### Fixed

api/rollback.py

@@ -11,6 +11,7 @@ import logging
 import os
 import re
 import shutil
+import stat
 import subprocess
 from datetime import datetime
 from pathlib import Path
@@ -96,6 +97,113 @@ def _find_git() -> str:
     return shutil.which("git") or "git"
 
 
+def _checkpoint_entry_modes(git: str, ckpt_dir: Path) -> dict[str, int]:
+    """Return git index modes for tracked checkpoint paths in one pass."""
+    result = subprocess.run(
+        [git, "-C", str(ckpt_dir), "ls-files", "-s"],
+        capture_output=True, text=True, timeout=10,
+    )
+    if result.returncode != 0:
+        raise ValueError("Failed to list checkpoint files")
+
+    modes: dict[str, int] = {}
+    for line in result.stdout.splitlines():
+        parts = line.split(maxsplit=3)
+        if len(parts) != 4:
+            continue
+        try:
+            modes[parts[3]] = int(parts[0], 8)
+        except ValueError:
+            continue
+    return modes
+
+
+def _checkpoint_entry_is_regular(modes: dict[str, int], rel_path: str) -> bool:
+    """Return True only for regular tracked checkpoint files.
+
+    Checkpoint worktrees can contain tracked symlinks. Pathname reads such as
+    Path.is_file()/read_text() follow those symlinks and can disclose host files
+    outside the checkpoint/workspace root in rollback diffs. Use the git index
+    mode as the source of truth and refuse symlink/special entries before any
+    filesystem open.
+    """
+    mode = modes.get(rel_path)
+    return mode is not None and stat.S_ISREG(mode)
+
+
+def _read_checkpoint_blob(git: str, ckpt_dir: Path, rel_path: str) -> bytes | None:
+    """Read a regular tracked checkpoint blob without opening the worktree path."""
+    result = subprocess.run(
+        [git, "-C", str(ckpt_dir), "show", f"HEAD:{rel_path}"],
+        capture_output=True, timeout=10,
+    )
+    if result.returncode != 0:
+        return None
+    return result.stdout
+
+
+def _read_checkpoint_text(git: str, ckpt_dir: Path, modes: dict[str, int], rel_path: str) -> str | None:
+    """Read a regular tracked checkpoint file without following worktree links."""
+    if not _checkpoint_entry_is_regular(modes, rel_path):
+        return None
+    blob = _read_checkpoint_blob(git, ckpt_dir, rel_path)
+    if blob is None:
+        return None
+    return blob.decode(errors="replace")
+
+
+def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None:
+    """Read workspace text symlink-safely without the size cap of read_file_content.
+
+    Resolves the path under the workspace (rejecting traversal/symlink escapes)
+    and opens it through an anchored, O_NOFOLLOW file descriptor so a symlink
+    component cannot redirect the read outside the workspace boundary. Unlike
+    ``read_file_content``, this has no MAX_FILE_BYTES cap — a large but legitimate
+    regular file must render as *modified*, not be silently dropped (which would
+    make the rollback diff falsely report it as *deleted*). Treat missing /
+    invalid / escape / non-regular paths as absent (None).
+    """
+    from api.workspace import open_anchored_fd, safe_resolve_ws
+
+    try:
+        target = safe_resolve_ws(workspace_root, rel_path)
+    except (ValueError, OSError):
+        return None
+    # Pre-check the leaf type WITHOUT opening it: open_anchored_fd opens with
+    # blocking O_RDONLY, which would HANG on a FIFO/special file swapped in at a
+    # regular-file path. lstat on the already-symlink-resolved target tells us the
+    # type without an open, so we never block on a non-regular leaf. (The old
+    # Path.is_file() guard likewise never opened a FIFO.)
+    try:
+        if not stat.S_ISREG(os.lstat(target).st_mode):
+            return None
+    except OSError:
+        return None
+    try:
+        fd = open_anchored_fd(workspace_root, target, want_dir=False)
+    except (FileNotFoundError, ValueError, OSError):
+        return None
+    # From here the fd is owned; make sure every path closes it exactly once.
+    try:
+        st = os.fstat(fd)
+        if not stat.S_ISREG(st.st_mode):
+            os.close(fd)
+            return None
+        fh = os.fdopen(fd, "rb")
+    except OSError:
+        try:
+            os.close(fd)
+        except OSError:
+            pass
+        return None
+    # os.fdopen succeeded → the file object owns the fd and will close it.
+    try:
+        with fh:
+            return fh.read().decode(errors="replace")
+    except OSError:
+        return None
+
+
 # ── Public API functions (called from routes.py) ────────────────────────────
 
 
@@ -198,39 +306,25 @@ def get_checkpoint_diff(workspace: str, checkpoint: str) -> dict[str, Any]:
 
     git = _find_git()
 
-    # Get list of files in the checkpoint
-    ls_result = subprocess.run(
-        [git, "-C", str(ckpt_dir), "ls-files"],
-        capture_output=True, text=True, timeout=10,
-    )
-    if ls_result.returncode != 0:
-        raise ValueError("Failed to list checkpoint files")
+    try:
+        entry_modes = _checkpoint_entry_modes(git, ckpt_dir)
+    except ValueError as e:
+        raise ValueError("Failed to list checkpoint files") from e
 
-    ckpt_files = [f for f in ls_result.stdout.strip().split("\n") if f]
+    ckpt_files = list(entry_modes)
     files_changed = []
     diff_lines = []
 
     for rel_path in ckpt_files:
-        ckpt_file = ckpt_dir / rel_path
-        ws_file = Path(resolved) / rel_path
-
-        if not ckpt_file.is_file():
-            continue
-
-        # Read checkpoint version
-        try:
-            ckpt_content = ckpt_file.read_text(errors="replace")
-        except OSError:
+        # Read checkpoint version from the git object database, not via the
+        # checkout path. This prevents tracked symlinks in the checkpoint from
+        # redirecting diff reads to arbitrary host files.
+        ckpt_content = _read_checkpoint_text(git, ckpt_dir, entry_modes, rel_path)
+        if ckpt_content is None:
             continue
 
         # Read workspace version (if exists)
-        if ws_file.is_file():
-            try:
-                ws_content = ws_file.read_text(errors="replace")
-            except OSError:
-                ws_content = ""
-        else:
-            ws_content = None  # File was deleted in workspace
+        ws_content = _read_workspace_text(Path(resolved), rel_path)
 
         if ws_content is None:
             # File exists in checkpoint but not in workspace (deleted)
@@ -262,8 +356,8 @@ def get_checkpoint_diff(workspace: str, checkpoint: str) -> dict[str, Any]:
     }
 
 
-def _restore_checkpoint_file(workspace_root: Path, ckpt_file: Path, rel_path: str) -> None:
-    """Restore one checkpoint file without following workspace symlinks outward."""
+def _restore_checkpoint_file(workspace_root: Path, rel_path: str, content: bytes, mode: int) -> None:
+    """Restore one checkpoint blob without following checkpoint or workspace symlinks."""
     from api.workspace import open_anchored_create_fd, open_anchored_write_fd, safe_resolve_ws
 
     target = safe_resolve_ws(workspace_root, rel_path)
@@ -272,19 +366,13 @@ def _restore_checkpoint_file(workspace_root: Path, ckpt_file: Path, rel_path: st
     else:
         fd = open_anchored_create_fd(workspace_root, target)
 
-    src_stat = ckpt_file.stat()
     with os.fdopen(fd, "wb") as out:
-        with ckpt_file.open("rb") as src:
-            shutil.copyfileobj(src, out)
+        out.write(content)
         out.flush()
         try:
-            os.fchmod(out.fileno(), src_stat.st_mode & 0o777)
+            os.fchmod(out.fileno(), mode & 0o777)
         except (AttributeError, OSError):
             logger.debug("Failed to apply restored mode to %s", target, exc_info=True)
-        try:
-            os.utime(out.fileno(), ns=(src_stat.st_atime_ns, src_stat.st_mtime_ns))
-        except OSError:
-            logger.debug("Failed to apply restored timestamp to %s", target, exc_info=True)
 
 
 def restore_checkpoint(workspace: str, checkpoint: str) -> dict[str, Any]:
@@ -308,26 +396,26 @@ def restore_checkpoint(workspace: str, checkpoint: str) -> dict[str, Any]:
 
     git = _find_git()
 
-    # Get list of files in the checkpoint
-    ls_result = subprocess.run(
-        [git, "-C", str(ckpt_dir), "ls-files"],
-        capture_output=True, text=True, timeout=10,
-    )
-    if ls_result.returncode != 0:
-        raise ValueError("Failed to list checkpoint files")
+    try:
+        entry_modes = _checkpoint_entry_modes(git, ckpt_dir)
+    except ValueError as e:
+        raise ValueError("Failed to list checkpoint files") from e
 
-    ckpt_files = [f for f in ls_result.stdout.strip().split("\n") if f]
+    ckpt_files = list(entry_modes)
     restored = []
     errors = []
 
     for rel_path in ckpt_files:
-        ckpt_file = ckpt_dir / rel_path
+        mode = entry_modes.get(rel_path)
+        if mode is None or not stat.S_ISREG(mode):
+            continue
 
-        if not ckpt_file.is_file():
+        content = _read_checkpoint_blob(git, ckpt_dir, rel_path)
+        if content is None:
             continue
 
         try:
-            _restore_checkpoint_file(workspace_root, ckpt_file, rel_path)
+            _restore_checkpoint_file(workspace_root, rel_path, content, mode)
             restored.append(rel_path)
         except (OSError, ValueError) as e:
             errors.append({"file": rel_path, "error": str(e)})

tests/test_rollback_diff_symlink_disclosure.py

@@ -0,0 +1,155 @@
+"""Regression tests for rollback checkpoint diff symlink disclosure."""
+
+from __future__ import annotations
+
+import os
+import subprocess
+from pathlib import Path
+
+from api import rollback
+import api.workspace as workspace_mod
+
+
+def _commit_checkpoint_file(repo: Path, rel: str, content: str) -> None:
+    path = repo / rel
+    path.parent.mkdir(parents=True, exist_ok=True)
+    path.write_text(content, encoding="utf-8")
+    subprocess.run(["git", "-C", str(repo), "add", rel], check=True)
+    subprocess.run(["git", "-C", str(repo), "commit", "-m", "checkpoint"], check=True)
+
+
+def _init_checkpoint(tmp_path, monkeypatch):
+    hermes_home = tmp_path / "home"
+    workspace = tmp_path / "workspace"
+    hermes_home.mkdir()
+    workspace.mkdir()
+    ws_hash = rollback._workspace_hash(str(workspace.resolve()))
+    checkpoint = "abc123"
+    ckpt_dir = hermes_home / "checkpoints" / ws_hash / checkpoint
+    ckpt_dir.mkdir(parents=True)
+    subprocess.run(["git", "-C", str(ckpt_dir), "init"], check=True)
+    subprocess.run(["git", "-C", str(ckpt_dir), "config", "user.email", "test@example.invalid"], check=True)
+    subprocess.run(["git", "-C", str(ckpt_dir), "config", "user.name", "Test"], check=True)
+    monkeypatch.setattr(rollback, "_hermes_home", lambda: hermes_home)
+    monkeypatch.setattr(workspace_mod, "load_workspaces", lambda: [{"path": str(workspace)}])
+    return workspace, ckpt_dir, checkpoint
+
+
+def test_checkpoint_diff_does_not_follow_checkpoint_symlink(tmp_path, monkeypatch):
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    secret = tmp_path / "outside-secret.txt"
+    secret.write_text("SAFE_SECRET_MARKER_SHOULD_NOT_APPEAR\n", encoding="utf-8")
+
+    os.symlink(secret, ckpt_dir / "leak.txt")
+    subprocess.run(["git", "-C", str(ckpt_dir), "add", "leak.txt"], check=True)
+    subprocess.run(["git", "-C", str(ckpt_dir), "commit", "-m", "checkpoint"], check=True)
+
+    result = rollback.get_checkpoint_diff(str(workspace), checkpoint)
+
+    assert result["files_changed"] == []
+    assert "SAFE_SECRET_MARKER_SHOULD_NOT_APPEAR" not in result["diff"]
+    assert "leak.txt" not in result["diff"]
+
+
+def test_checkpoint_diff_does_not_follow_workspace_symlink_escape(tmp_path, monkeypatch):
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    _commit_checkpoint_file(ckpt_dir, "leak.txt", "checkpoint content\n")
+    secret = tmp_path / "outside-secret.txt"
+    secret.write_text("WORKSPACE_SECRET_MARKER_SHOULD_NOT_APPEAR\n", encoding="utf-8")
+    os.symlink(secret, workspace / "leak.txt")
+
+    result = rollback.get_checkpoint_diff(str(workspace), checkpoint)
+
+    assert "WORKSPACE_SECRET_MARKER_SHOULD_NOT_APPEAR" not in result["diff"]
+    assert "checkpoint content" in result["diff"]
+
+
+def test_restore_checkpoint_skips_checkpoint_symlink_sources(tmp_path, monkeypatch):
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    secret = tmp_path / "outside-secret.txt"
+    secret.write_text("RESTORE_SECRET_MARKER_SHOULD_NOT_COPY\n", encoding="utf-8")
+
+    os.symlink(secret, ckpt_dir / "leak.txt")
+    subprocess.run(["git", "-C", str(ckpt_dir), "add", "leak.txt"], check=True)
+    subprocess.run(["git", "-C", str(ckpt_dir), "commit", "-m", "checkpoint"], check=True)
+
+    result = rollback.restore_checkpoint(str(workspace), checkpoint)
+
+    assert result["files_restored"] == []
+    assert not (workspace / "leak.txt").exists()
+    assert "RESTORE_SECRET_MARKER_SHOULD_NOT_COPY" not in "\n".join(
+        p.read_text(encoding="utf-8", errors="replace")
+        for p in workspace.rglob("*")
+        if p.is_file()
+    )
+
+
+def test_restore_checkpoint_reads_git_blob_after_worktree_symlink_swap(tmp_path, monkeypatch):
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    _commit_checkpoint_file(ckpt_dir, "file.txt", "checkpoint blob content\n")
+    secret = tmp_path / "outside-secret.txt"
+    secret.write_text("POST_COMMIT_SECRET_MARKER_SHOULD_NOT_COPY\n", encoding="utf-8")
+
+    (ckpt_dir / "file.txt").unlink()
+    os.symlink(secret, ckpt_dir / "file.txt")
+
+    result = rollback.restore_checkpoint(str(workspace), checkpoint)
+
+    assert result["files_restored"] == ["file.txt"]
+    assert (workspace / "file.txt").read_text(encoding="utf-8") == "checkpoint blob content\n"
+    assert "POST_COMMIT_SECRET_MARKER_SHOULD_NOT_COPY" not in (workspace / "file.txt").read_text(
+        encoding="utf-8"
+    )
+
+
+def test_checkpoint_diff_renders_large_workspace_file_as_modified_not_deleted(tmp_path, monkeypatch):
+    """Regression (Codex gate): a large but legitimate workspace file (> the
+    read_file_content MAX_FILE_BYTES 400KB cap) that differs from the checkpoint
+    must render as MODIFIED, not be silently dropped to None and reported as
+    DELETED. The symlink-safe workspace reader must have no size cap.
+    """
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    # Checkpoint has a small version of the file.
+    _commit_checkpoint_file(ckpt_dir, "big.txt", "old small checkpoint content\n")
+    # Workspace has a LARGE (> 400KB) modified version of the same file.
+    big_content = "X" * (500 * 1024) + "\nMODIFIED_LARGE_MARKER\n"
+    (workspace / "big.txt").write_text(big_content, encoding="utf-8")
+
+    result = rollback.get_checkpoint_diff(str(workspace), checkpoint)
+
+    # The file must appear in the diff as a change, NOT be treated as deleted.
+    assert "big.txt" in result["diff"]
+    statuses = {f["file"]: f.get("status") for f in result["files_changed"]}
+    assert statuses.get("big.txt") == "modified", (
+        "a large modified workspace file must render as modified, not deleted "
+        "(the capped read_file_content path regressed this): got "
+        f"{statuses.get('big.txt')!r}"
+    )
+
+
+def test_checkpoint_diff_skips_workspace_fifo_without_hanging(tmp_path, monkeypatch):
+    """Regression (Codex gate): a workspace FIFO/special file at a checkpoint-
+    tracked regular-file path must NOT hang the diff (open_anchored_fd opens
+    blocking O_RDONLY, which blocks forever on a FIFO with no writer) and must
+    not leak an fd — the leaf type is pre-checked via lstat before any open.
+    """
+    import threading
+
+    workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
+    _commit_checkpoint_file(ckpt_dir, "pipe.txt", "checkpoint content\n")
+    # Replace the workspace-side file with a FIFO (no writer → blocking open hangs).
+    os.mkfifo(workspace / "pipe.txt")
+
+    result_box = {}
+
+    def _run():
+        result_box["r"] = rollback.get_checkpoint_diff(str(workspace), checkpoint)
+
+    t = threading.Thread(target=_run, daemon=True)
+    t.start()
+    t.join(timeout=10)
+    assert not t.is_alive(), "get_checkpoint_diff hung on a workspace FIFO"
+    # FIFO workspace side is treated as absent → checkpoint file renders as deleted,
+    # never blocks and never leaks the secret/hangs.
+    statuses = {f["file"]: f.get("status") for f in result_box["r"]["files_changed"]}
+    assert statuses.get("pipe.txt") == "deleted"