diff --git a/CHANGELOG.md b/CHANGELOG.md index af70ab9d..fc1d25d5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/api/rollback.py b/api/rollback.py index d026a9fd..d89dee76 100644 --- a/api/rollback.py +++ b/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)}) diff --git a/tests/test_rollback_diff_symlink_disclosure.py b/tests/test_rollback_diff_symlink_disclosure.py new file mode 100644 index 00000000..c5bab2ad --- /dev/null +++ b/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"