GitInspectionRepository::snapshot reads HEAD/index/objects by path, not by pinned handle

[BunsDev]

What is wrong

Found in independent review of the #424 fix (PR #421). git_dir_for_worktree/git_repository_paths now classify and read the .git marker safely (symlink-metadata-based classification, hardened bounded reads for the linked-worktree file), but the directory identity they return is not pinned by handle. GitInspectionRepository::snapshot then does:

let (work_tree, actual_git_dir) = git_repository_paths(Path::new(root))?;
let common_dir = git_common_dir(&actual_git_dir)?;
let index = work_tree.as_ref().map(|_| actual_git_dir.join("index"));
let alternate_objects = common_dir.join("objects");
...
let actual_head = std::fs::read_to_string(actual_git_dir.join("HEAD"))
    .map_err(|e| format!("snapshot Git HEAD: {e}"))?;

HEAD is read with a plain, symlink-following, unbounded read_to_string from a path, not through GitMetadataDirectory/read_bounded_git_metadata_file. If a hostile repository swaps its .git directory (or objects/index) for a symlink between the classification in git_dir_for_worktree and these later reads, the path-based reads can still be redirected -- the same TOCTOU class the module's header describes defending against ("a .git directory can be swapped for a symlink ... between one command and the next, so directory identity is verified by handle rather than by path"), except this particular read path is not.

Scope

This gap is pre-existing and identical before and after the #424 fix -- git_dir_for_worktree's directory-marker branch has always returned a path without opening/pinning a handle, and snapshot()'s HEAD/index/objects path construction and reads have always been path-based. It is not introduced by the #424 fix, which only hardened .git marker classification and the linked-worktree marker file read.

Suggested direction

Pin the git/common directory with a no-follow handle (the GitMetadataDirectory pattern already used elsewhere in this module) at the point git_repository_paths resolves it, and thread that handle (or a validated identity check immediately before each read) through to the HEAD, index, and objects reads in snapshot(), rather than re-deriving plain paths.

Severity

Filed at the same trust boundary as #424: reachable from git_log/inspection commands with a caller-chosen, untrusted repository root. Exploitation requires winning a narrow race between classification and read, which is why this is filed separately rather than blocking #424's narrower, already-tested fix.

Related

#424 (fixed in #421). Found during independent review of that fix.

[BunsDev]

Additional deterministic reproduction from the independent review: against published aed41b0, resolve a real .git using git_dir_for_worktree, rename that directory, replace .git with a directory symlink to a disposable second refs tree, then call git_common_dir and snapshot_git_refs on the resolved path. The production helpers return the replacement refs rather than rejecting the changed directory. Focused regression git_metadata_rejects_directory_replacement_after_marker_resolution fails exactly on the rejection assertion (one test, 454 filtered). This proves redirected common-dir/ref reads after resolution; it is not an end-to-end git_log exploitation or exfiltration claim. The reproduction also supports retaining no-follow identity through HEAD/ref/common-dir reads rather than relying on a path-only precheck. No unrelated author files were changed; the bounded reproduction patch is retained in session artifacts.

[BunsDev]

Fixed and merged as 90e6dc3 (PR #431).

GitInspectionRepository::snapshot() now opens a no-follow GitMetadataDirectory handle on the resolved Git directory as soon as it's known and threads that handle through both reads that were previously done via plain, symlink-following, unbounded paths:

• HEAD is read through the pinned handle (bounded by a new MAX_GIT_HEAD_BYTES), with the handle re-validated afterward.
• commondir is read through the pinned handle via a new git_common_dir_pinned() (bounded by a new MAX_GIT_COMMONDIR_FILE_BYTES). Following independent review feedback, existence/type classification for commondir is now also decided by the same fd-relative open that performs the read (via a new GitMetadataReadError::NotFound variant threaded through all three platform implementations), not a second, separately racy path-based stat.

index/alternate_objects were left as-is: this code only ever passes them as paths to git subprocesses, it never reads their content directly.

Regression tests added: git_inspection_rejects_a_symlinked_head_marker, git_inspection_rejects_a_symlinked_commondir_marker — each swaps the respective marker for a symlink after a valid repository is set up and asserts the read is rejected ("not a regular file") rather than followed.

Full cargo test --lib --locked: 456 passed, 0 failed.

[BunsDev]

Reopening for the remaining original acceptance scope after reconciling #431 and #433 at current main a42dc61. #431 correctly bounds and pins HEAD and commondir marker reads; #433 adds a static symlink-at-entry regression and explicitly makes no production changes or mid-flight-swap claim. Those improvements do not bind snapshot_git_refs and later common-directory consumers to the opened directory identity, nor retain identity when Git later consumes GIT_INDEX_FILE and objects/info/alternates paths. The earlier independent review of #431 documents both remaining boundaries. Passing the path to a Git subprocess moves the read boundary; it does not preserve it. Keep the completed HEAD/commondir improvement credited, but retain this outcome until a swap during snapshot and a swap before subprocess consumption either use the originally accepted inputs or fail explicitly. No new end-to-end exploit claim is made; the outstanding boundary is established by current source ordering, while the earlier helper reproduction was against its named older source. Do not close this broader outcome solely on static symlink-marker tests.