Harden git snapshot ref identity handoff - #437
Conversation
Addresses #426 (remaining scope after #431/#433) Pin snapshot_git_refs and recursive loose-ref traversal to opened metadata directory handles, reject symlinked packed-refs, and revalidate git/common directory identity immediately before subprocess handoff for index and alternates paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
🟡 Changes recommended
Fix the Windows non-directory error handling before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens desktop Git inspection against filesystem identity races during ref traversal and subprocess handoff.
Changes:
- Pins packed and loose ref reads to validated metadata directories.
- Adds cross-platform directory helpers and path revalidation.
- Adds regression tests for replacement and symlink scenarios.
File summaries
| File | Summary |
|---|---|
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs |
Implements the Git metadata hardening, but Windows non-directory errors can cause normal loose refs to abort inspection. |
Review details
Suppressed comments (5)
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs:1410
- [P2] Propagate
readdirfailures instead of treating them as EOF.readdirreturns null both at end-of-directory and on an I/O error; this loop never clears or checkserrno, so a directory read failure can produce a successful partial ref snapshot. The previousstd::fs::read_dirpath propagated such errors. Clearerrnobefore the loop and reject a nonzero error afterreaddirreturns null.
let mut entries = Vec::new();
loop {
let entry = unsafe { libc::readdir(stream) };
if entry.is_null() {
break;
}
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs:1409
- [P2] Avoid materializing an unbounded directory listing. This new helper stores every entry in a
Vecbeforecollect_loose_refscan discard invalid names or.lockfiles; a hostile repository can therefore force memory proportional to all names inrefseven when no refs are retained. The oldstd::fs::read_dirloop streamed entries one at a time. Iterate the directory stream while it is open or enforce a bounded listing size/count before allocating the whole vector.
let mut entries = Vec::new();
loop {
let entry = unsafe { libc::readdir(stream) };
if entry.is_null() {
break;
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs:2429
- [P1] Validate the index path itself before handing it to Git.
GIT_INDEX_FILEpoints toactual_git_dir/index, but this check only reopens the parent directory; replacing that child with a symlink leaves the directory identity unchanged, so validation passes and the subprocess can follow the link. Add a no-follow regular-file identity check for the index path, with an explicit absent-index case.
if self.index.is_some() {
self.git_dir_handle
.validate_path_identity("Git directory", "before subprocess handoff")?;
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs:2311
- [P1] Revalidate the object directory that is actually handed to Git.
alternate_objectsiscommon_dir/objects, but this check only comparescommon_dir; replacingobjectswith a symlink leaves the parent identity unchanged and Git can later follow the replacement throughobjects/info/alternates. Validate theobjectschild with the same no-follow identity check, or snapshot it into the isolated directory.
common_dir_handle
.validate_path_identity("Git common directory", "before subprocess handoff")?;
native/desktop/psyche-build-tauri/src-tauri/src/git_control.rs:1673
- [P1] Enumerate Windows refs from the pinned directory handle. This implementation still calls path-based
read_dir, so replacing a refs directory with a junction betweenopen_directoryand this listing can supply names from the replacement while the subsequent reads use the old handle; if the path is restored before validation, the snapshot can silently omit or alter refs. Use a handle-relative directory enumeration like the Unix implementation.
fn list_entries(&self, label: &str) -> Result<Vec<std::ffi::OsString>, String> {
git_metadata_path_entries(&self.path, label)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses #426 (remaining scope after #431/#433) Follow the independent review by snapshotting the live index into the isolated inspection repository, writing alternates immediately before each Git subprocess handoff from a pinned objects-directory handle, pinning shallow/info snapshot reads to the opened common-directory identity, and preserving Unix directory enumeration errors during recursive ref traversal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eliminate the remaining live alternates and attribute-source object handoff by copying the pinned objects directory tree into the isolated inspection repository before any subprocess reads it. Addresses #426 (remaining scope after #431/#433) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace git_repository_paths path-based marker checks with no-follow marker classification and pinned bare-repository detection so symlinked HEAD, objects, and commondir markers are rejected before repository selection. Also confirmed the latest pushed head already keeps snapshot_git_refs handle-based; the existing ref-swap regressions remain unchanged. Addresses #426 (remaining scope after #431/#433) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Map Windows ERROR_DIRECTORY to an explicit GitMetadataReadError::NotDirectory, use that in loose-ref traversal, and replace Windows path-based directory enumeration with handle-relative NtQueryDirectoryFile listing. Also add focused regressions for the actual index file and objects directory snapshot boundaries and keep the existing handle-based snapshot_git_refs coverage in place. Addresses #426 (remaining scope after #431/#433) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
ERROR_DIRECTORYto an explicitGitMetadataReadError::NotDirectory, socollect_loose_refsnow falls through from a non-directory child to the existing file read path instead of aborting on normal loose-ref fileslist_entriespath reopening with handle-relativeNtQueryDirectoryFileenumeration, keeping recursive ref and object traversal anchored to the opened directory handle instead of a second path-based walksnapshot_git_refshandle-based at the current branch head:packed-refsis read throughGitMetadataDirectory::read_file, and recursiverefs/descent still usesopen_directoryplus handle-relative reads rather than plain pathsindexchild and the openedobjectschild so the source inputs copied into the isolated inspection repo are explicitly coveredTests
git_ref_snapshot_rejects_common_dir_replacement_after_open: proves a common-dir replacement that is already in place after the handle opens is rejected instead of redirecting loose-ref traversalgit_ref_snapshot_rejects_symlinked_packed_refs: provespacked-refsis rejected when the entry is a symlink instead of a regular filegit_metadata_windows_directory_open_error_maps_not_directory: proves WindowsERROR_DIRECTORYmaps toGitMetadataReadError::NotDirectory, which is the branchcollect_loose_refsnow treats as a file fallback rather than a hard failuregit_index_snapshot_rejects_a_symlinked_index_marker: proves the actualindexchild is validated as a regular file at snapshot time and a symlink replacement is rejected rather than copiedgit_objects_snapshot_anchors_to_open_objects_directory: proves replacing the sourceobjectspath after the openedobjectshandle is captured does not redirect the object snapshot to replacement contentsgit_repository_paths_rejects_a_symlinked_bare_head_marker: proves bare-repository detection rejects a symlinkedHEADgit_repository_paths_rejects_a_symlinked_bare_objects_directory: proves bare-repository detection rejects a symlinkedobjectsdirectory markergit_repository_paths_rejects_a_symlinked_bare_commondir_marker: proves bare-repository detection rejects a symlinkedcommondirmarkerValidation
cargo fmt --check --manifest-path native/desktop/psyche-build-tauri/src-tauri/Cargo.tomlcargo test --lib --locked --manifest-path native/desktop/psyche-build-tauri/src-tauri/Cargo.toml→ 470 passed, 0 failedbash ./scripts/agent-check full→ Rust 470 passed / 0 failed; Vitest 5348 passed / 11 skipped; beads render 221 passed; agent repository contract 1 passedAddresses the remaining scope of #426 without auto-closing it.