From 5afc3842fa0440dfdb3086659b39339677beb1c7 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Thu, 10 Sep 2026 06:34:11 -0500 Subject: [PATCH 1/2] test(desktop): prove the .git directory marker race fails closed An independent code review of git_dir_for_worktree's Directory branch flagged that it classifies the .git marker with a no-follow stat and returns a path, without itself opening or pinning that directory. The concern: if the marker is swapped from a real directory to a symlink between that classification and later reads, the returned path could redirect a snapshot's reads. The only production caller (GitInspectionRepository::snapshot) already closes this window: it opens the resolved Git directory with a no-follow (O_NOFOLLOW) handle as the very first step after resolving the path, before deriving or reading anything else from inside it. A marker swapped for a symlink in the interim causes that open to fail safely rather than being followed. Add an end-to-end regression test exercising this through GitInspectionRepository::snapshot (not just git_dir_for_worktree in isolation) so the guarantee is proven at the same layer external callers use, and document the reasoning inline at the Directory branch so a future change can't silently reopen the gap. Addresses a High-severity finding from an independent review agent following #426/#431. --- .../src-tauri/src/git_control.rs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs b/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs index 0af3a7cf..ba115b59 100644 --- a/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs +++ b/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs @@ -514,6 +514,16 @@ fn git_dir_for_worktree(root: &Path) -> Result<(PathBuf, PathBuf), String> { let label = "worktree .git marker"; match git_marker_kind(&dot_git, label)? { GitMarkerKind::Directory => { + // Unlike the `File` branch below, this only returns a path; + // it does not open or pin the `.git` directory itself. That + // is safe: every production caller resolves this path and + // then immediately opens it with a no-follow + // (`O_NOFOLLOW`/reparse-point-rejecting) handle before + // reading anything from inside it (see + // `GitInspectionRepository::snapshot`'s `git_dir_handle`). + // A `.git` marker swapped for a symlink between this + // classification and that open fails closed there rather + // than being followed, so no separate pin is needed here. return Ok((candidate.to_path_buf(), dot_git)); } GitMarkerKind::File => { @@ -4922,6 +4932,34 @@ mod tests { ); } + #[test] + fn git_inspection_rejects_a_worktree_whose_git_directory_marker_is_a_symlink() { + // `git_dir_for_worktree` classifies the `.git` marker with a + // no-follow `symlink_metadata` check before returning its path, but + // that classification and `GitInspectionRepository::snapshot`'s own + // reads from the resolved Git directory are two separate steps. This + // proves the gap between them is closed end to end: `snapshot` pins + // the resolved Git directory with a no-follow open (`O_NOFOLLOW`) + // immediately, as the very first thing it does after resolving the + // path, so a `.git` marker that is a symlink is rejected there too, + // not just by the earlier classification helper. + let tree = TempTree::new("git-inspection-symlinked-git-directory"); + let root = tree.root.join("root"); + let outside = tree.root.join("outside-git-dir"); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + if !create_test_symlink(TestSymlinkKind::Directory, &outside, &root.join(".git")) { + return; + } + + let error = match GitInspectionRepository::snapshot(path_text(&root), None, Vec::new()) { + Ok(_) => panic!("a symlinked .git directory marker must be rejected"), + Err(error) => error, + }; + + assert!(error.contains("symlink"), "unexpected error: {error}"); + } + #[test] fn git_inspection_preserves_reftable_refs_when_supported() { let tree = TempTree::new("git-reftable-inspection"); From b7fa1a5e6792c07371d56fe28f237eefded325a9 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Thu, 10 Sep 2026 06:57:22 -0500 Subject: [PATCH 2/2] test(desktop): correct claimed scope of the .git directory race test Address automated review feedback: the regression test proves snapshot() rejects a .git directory marker that is already a symlink when routed through the full entrypoint (a fact the prior comment understated by overclaiming it specifically exercised a mid-flight race), while the narrower marker-swapped-mid-flight window is closed structurally by O_NOFOLLOW in open_directory_no_follow, independent of timing. Correct the inline comments to state this precisely instead of overclaiming what the test itself exercises. --- .../src-tauri/src/git_control.rs | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs b/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs index ba115b59..3e90186a 100644 --- a/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs +++ b/native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs @@ -4934,15 +4934,20 @@ mod tests { #[test] fn git_inspection_rejects_a_worktree_whose_git_directory_marker_is_a_symlink() { - // `git_dir_for_worktree` classifies the `.git` marker with a - // no-follow `symlink_metadata` check before returning its path, but - // that classification and `GitInspectionRepository::snapshot`'s own - // reads from the resolved Git directory are two separate steps. This - // proves the gap between them is closed end to end: `snapshot` pins - // the resolved Git directory with a no-follow open (`O_NOFOLLOW`) - // immediately, as the very first thing it does after resolving the - // path, so a `.git` marker that is a symlink is rejected there too, - // not just by the earlier classification helper. + // `git_dir_for_worktree_rejects_a_symlinked_git_directory_marker` + // already proves `git_dir_for_worktree` itself rejects this in + // isolation. This test proves the same guarantee holds at the + // entrypoint external callers actually use, + // `GitInspectionRepository::snapshot`, which resolves the path via + // `git_dir_for_worktree` internally: a symlinked `.git` directory + // marker must still be rejected once routed through the full + // snapshot call, not merely when calling the classification helper + // directly. It does not, by itself, exercise a marker swapped + // *between* that classification and `snapshot`'s later no-follow + // open of the resolved Git directory (`git_dir_handle`); that + // narrower race window is closed structurally by `open_directory_no_follow` + // using `O_NOFOLLOW`, which fails closed rather than following a + // symlink regardless of when the swap happens. let tree = TempTree::new("git-inspection-symlinked-git-directory"); let root = tree.root.join("root"); let outside = tree.root.join("outside-git-dir");