From b6f8fc2a5cdd93ef556fde5a8d6eb1d1e6bc5c9d Mon Sep 17 00:00:00 2001 From: hinotoi-agent Date: Thu, 18 Jun 2026 13:36:06 +0800 Subject: [PATCH 1/4] fix(rollback): avoid symlinked checkpoint diff reads --- CHANGELOG.md | 4 + api/rollback.py | 84 +++++++++++++++---- .../test_rollback_diff_symlink_disclosure.py | 84 +++++++++++++++++++ 3 files changed, 154 insertions(+), 18 deletions(-) create mode 100644 tests/test_rollback_diff_symlink_disclosure.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cf104f46..83d48b0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## [Unreleased] +### Security + +- **Rollback checkpoint diffs no longer follow checkpoint or workspace symlinks when rendering file contents.** 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 and use the workspace's anchored file reader for workspace-side content; restore also skips symlink checkpoint sources. + ## [v0.51.489] — 2026-06-18 — Release QY (outline button no longer collides with the scroll control) ### Fixed diff --git a/api/rollback.py b/api/rollback.py index d026a9fd..9ccd3020 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,64 @@ def _find_git() -> str: return shutil.which("git") or "git" +def _checkpoint_entry_mode(git: str, ckpt_dir: Path, rel_path: str) -> int | None: + """Return the git index mode for a tracked checkpoint path.""" + result = subprocess.run( + [git, "-C", str(ckpt_dir), "ls-files", "-s", "--", rel_path], + capture_output=True, text=True, timeout=10, + ) + if result.returncode != 0 or not result.stdout.strip(): + return None + first = result.stdout.splitlines()[0].split(maxsplit=1)[0] + try: + return int(first, 8) + except ValueError: + return None + + +def _checkpoint_entry_is_regular(git: str, ckpt_dir: Path, 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 = _checkpoint_entry_mode(git, ckpt_dir, rel_path) + return mode is not None and stat.S_ISREG(mode) + + +def _read_checkpoint_text(git: str, ckpt_dir: Path, rel_path: str) -> str | None: + """Read a regular tracked checkpoint file without following worktree links.""" + if not _checkpoint_entry_is_regular(git, ckpt_dir, rel_path): + return None + result = subprocess.run( + [git, "-C", str(ckpt_dir), "show", f"HEAD:{rel_path}"], + capture_output=True, text=True, errors="replace", timeout=10, + ) + if result.returncode != 0: + return None + return result.stdout + + +def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None: + """Read workspace text through the anchored workspace file API. + + The normal workspace helpers reject traversal and symlink escapes. Returning + an empty string for invalid/unreadable files keeps the diff useful without + echoing bytes from an escaping symlink target. + """ + from api.workspace import read_file_content + + try: + return read_file_content(workspace_root, rel_path)["content"] + except FileNotFoundError: + return None + except (OSError, ValueError): + return "" + + # ── Public API functions (called from routes.py) ──────────────────────────── @@ -211,26 +270,15 @@ def get_checkpoint_diff(workspace: str, checkpoint: str) -> dict[str, Any]: 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, 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) @@ -323,7 +371,7 @@ def restore_checkpoint(workspace: str, checkpoint: str) -> dict[str, Any]: for rel_path in ckpt_files: ckpt_file = ckpt_dir / rel_path - if not ckpt_file.is_file(): + if not _checkpoint_entry_is_regular(git, ckpt_dir, rel_path): continue try: diff --git a/tests/test_rollback_diff_symlink_disclosure.py b/tests/test_rollback_diff_symlink_disclosure.py new file mode 100644 index 00000000..17491c6b --- /dev/null +++ b/tests/test_rollback_diff_symlink_disclosure.py @@ -0,0 +1,84 @@ +"""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() + ) From f42248d32137b474333761ecb5a1f468ec014998 Mon Sep 17 00:00:00 2001 From: hinotoi-agent Date: Thu, 18 Jun 2026 16:04:39 +0800 Subject: [PATCH 2/4] fix: harden rollback restore checkpoint reads --- api/rollback.py | 109 +++++++++--------- .../test_rollback_diff_symlink_disclosure.py | 18 +++ 2 files changed, 74 insertions(+), 53 deletions(-) diff --git a/api/rollback.py b/api/rollback.py index 9ccd3020..a89d0197 100644 --- a/api/rollback.py +++ b/api/rollback.py @@ -97,22 +97,28 @@ def _find_git() -> str: return shutil.which("git") or "git" -def _checkpoint_entry_mode(git: str, ckpt_dir: Path, rel_path: str) -> int | None: - """Return the git index mode for a tracked checkpoint path.""" +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", "--", rel_path], + [git, "-C", str(ckpt_dir), "ls-files", "-s"], capture_output=True, text=True, timeout=10, ) - if result.returncode != 0 or not result.stdout.strip(): - return None - first = result.stdout.splitlines()[0].split(maxsplit=1)[0] - try: - return int(first, 8) - except ValueError: - return None + 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(git: str, ckpt_dir: Path, rel_path: str) -> bool: +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 @@ -121,38 +127,44 @@ def _checkpoint_entry_is_regular(git: str, ckpt_dir: Path, rel_path: str) -> boo mode as the source of truth and refuse symlink/special entries before any filesystem open. """ - mode = _checkpoint_entry_mode(git, ckpt_dir, rel_path) + mode = modes.get(rel_path) return mode is not None and stat.S_ISREG(mode) -def _read_checkpoint_text(git: str, ckpt_dir: Path, rel_path: str) -> str | None: - """Read a regular tracked checkpoint file without following worktree links.""" - if not _checkpoint_entry_is_regular(git, ckpt_dir, rel_path): - return None +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, text=True, errors="replace", timeout=10, + 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 through the anchored workspace file API. - The normal workspace helpers reject traversal and symlink escapes. Returning - an empty string for invalid/unreadable files keeps the diff useful without - echoing bytes from an escaping symlink target. + The normal workspace helpers reject traversal and symlink escapes. Treat + invalid or unreadable files as absent so the diff does not imply that the + workspace contains an empty file. """ from api.workspace import read_file_content try: return read_file_content(workspace_root, rel_path)["content"] - except FileNotFoundError: + except (FileNotFoundError, OSError, ValueError): return None - except (OSError, ValueError): - return "" # ── Public API functions (called from routes.py) ──────────────────────────── @@ -257,15 +269,12 @@ 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 = [] @@ -273,7 +282,7 @@ def get_checkpoint_diff(workspace: str, checkpoint: str) -> dict[str, Any]: # 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, rel_path) + ckpt_content = _read_checkpoint_text(git, ckpt_dir, entry_modes, rel_path) if ckpt_content is None: continue @@ -310,8 +319,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) @@ -320,19 +329,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]: @@ -356,26 +359,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 _checkpoint_entry_is_regular(git, ckpt_dir, rel_path): + 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 index 17491c6b..dcc57600 100644 --- a/tests/test_rollback_diff_symlink_disclosure.py +++ b/tests/test_rollback_diff_symlink_disclosure.py @@ -82,3 +82,21 @@ def test_restore_checkpoint_skips_checkpoint_symlink_sources(tmp_path, monkeypat 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" + ) From 94909590ad46305469f6a9ab384c25f033fac7a6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 18 Jun 2026 19:18:11 +0000 Subject: [PATCH 3/4] fix(#4410): read workspace diff content uncapped + symlink-safe (Codex gate) Codex regression gate found that routing workspace reads through read_file_content() introduced a 400KB MAX_FILE_BYTES cap not present on master (Path.read_text), so a large but legitimate workspace file raised ValueError -> caught -> None -> the rollback diff falsely reported it as DELETED. Replace with an uncapped anchored O_NOFOLLOW read (safe_resolve_ws + open_anchored_fd + S_ISREG fstat check) that keeps the symlink-escape protection but has no size cap. Adds a >400KB regression test (RED on the capped path, GREEN with the fix). Co-authored-by: Hinotoi-agent --- api/rollback.py | 34 +++++++++++++++---- .../test_rollback_diff_symlink_disclosure.py | 25 ++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/api/rollback.py b/api/rollback.py index a89d0197..004c743c 100644 --- a/api/rollback.py +++ b/api/rollback.py @@ -153,17 +153,37 @@ def _read_checkpoint_text(git: str, ckpt_dir: Path, modes: dict[str, int], rel_p def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None: - """Read workspace text through the anchored workspace file API. + """Read workspace text symlink-safely without the size cap of read_file_content. - The normal workspace helpers reject traversal and symlink escapes. Treat - invalid or unreadable files as absent so the diff does not imply that the - workspace contains an empty file. + 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 read_file_content + from api.workspace import open_anchored_fd, safe_resolve_ws try: - return read_file_content(workspace_root, rel_path)["content"] - except (FileNotFoundError, OSError, ValueError): + target = safe_resolve_ws(workspace_root, rel_path) + except (ValueError, OSError): + return None + try: + fd = open_anchored_fd(workspace_root, target, want_dir=False) + except (FileNotFoundError, ValueError, OSError): + return None + try: + st = os.fstat(fd) + if not stat.S_ISREG(st.st_mode): + return None + with os.fdopen(fd, "rb") as fh: + return fh.read().decode(errors="replace") + except OSError: + try: + os.close(fd) + except OSError: + pass return None diff --git a/tests/test_rollback_diff_symlink_disclosure.py b/tests/test_rollback_diff_symlink_disclosure.py index dcc57600..4206801b 100644 --- a/tests/test_rollback_diff_symlink_disclosure.py +++ b/tests/test_rollback_diff_symlink_disclosure.py @@ -100,3 +100,28 @@ def test_restore_checkpoint_reads_git_blob_after_worktree_symlink_swap(tmp_path, 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}" + ) From 9a54fe9aeb88b8213011adfdd73f0e9b535e4f9b Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Thu, 18 Jun 2026 19:22:29 +0000 Subject: [PATCH 4/4] fix(#4410): pre-check workspace leaf type via lstat + close fd on non-regular (Codex gate v2) Codex re-gate found two issues in the uncapped-read fix: (1) open_anchored_fd opens blocking O_RDONLY, which HANGS on a workspace FIFO/special file swapped in at a regular-file path; (2) the non-regular fstat branch returned without closing the fd (leak). Fix: lstat the symlink-resolved target BEFORE any open to skip non-regular leaves (no blocking open on a FIFO), and close the fd on the non-regular fstat branch. Adds a threaded FIFO-no-hang regression test. --- api/rollback.py | 21 ++++++++++++-- .../test_rollback_diff_symlink_disclosure.py | 28 +++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/api/rollback.py b/api/rollback.py index 004c743c..d89dee76 100644 --- a/api/rollback.py +++ b/api/rollback.py @@ -169,22 +169,39 @@ def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None: 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 - with os.fdopen(fd, "rb") as fh: - return fh.read().decode(errors="replace") + 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) ──────────────────────────── diff --git a/tests/test_rollback_diff_symlink_disclosure.py b/tests/test_rollback_diff_symlink_disclosure.py index 4206801b..c5bab2ad 100644 --- a/tests/test_rollback_diff_symlink_disclosure.py +++ b/tests/test_rollback_diff_symlink_disclosure.py @@ -125,3 +125,31 @@ def test_checkpoint_diff_renders_large_workspace_file_as_modified_not_deleted(tm "(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"