Bind Git worktree helpers to a trusted executable#30833
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca4f55c0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Move the new operations tests to a sibling file
When adding this new test module, the repo guidance requires putting the test contents in a separate sibling file with a descriptive #[path = "..._tests.rs"] declaration rather than defining the module inline. Keeping these new tests in operations.rs makes this file inconsistent with the adjacent git_command_tests.rs/git_config_tests.rs organization and harder to navigate.
AGENTS.md reference: AGENTS.md:L167-L178
Useful? React with 👍 / 👎.
| if path_is_untrusted(&candidate, untrusted) { | ||
| continue; |
There was a problem hiding this comment.
Normalize lexical PATH candidates before rejecting them
When a trusted Git binary is outside the repo but the PATH entry is spelled through the repo with .. (for example /work/repo/../tools/bin), this pre-canonical path_is_untrusted check treats the candidate as being inside /work/repo and skips it before the later canonical checks can prove it is outside. In environments where that is the only usable Git on PATH, all of these helpers now fail with NoTrustedGit even though a trusted executable is available.
Useful? React with 👍 / 👎.
| locations | ||
| .common_dir | ||
| .as_deref() | ||
| .is_some_and(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir)) |
There was a problem hiding this comment.
Reject Git binaries from enclosing repositories
When Codex is run inside a submodule or nested repo and PATH contains an absolute bin directory from the enclosing checkout, the candidate is outside roots and path_is_in_worktree_for_common_dir returns false because the parent repo has a different common dir. That lets a parent-repository-controlled git be selected, leaving the PATH hijack this resolver is meant to prevent open for that common setup.
Useful? React with 👍 / 👎.
Why
Internal worktree helpers selected
gitfromPATHat each call site. A repository-controlled executable or linked worktree could therefore run in place of the host Git binary before the helper applied its normal safety checks.This is the first reviewable replacement layer for the broader Git-helper hardening work. It builds on the existing local-only transport policy without duplicating it.
What
How
The resolver accumulates untrusted locations from the canonical repository, the requested lexical path, and a logical process
PWDonly when it is absolute and resolves to both the requested and actual process working directory. Every discovered repository contributes its lexical and canonical roots, common Git directory, and related linked worktrees. This covers independent nested repositories, submodules, conventional and bare-backed linked worktrees,--separate-git-dirlayouts, symlink spellings, and the normal process-cwd path without trusting arbitrary environment data.PATH directories are checked in their original spelling before candidate canonicalization. This matters on Windows because a path such as
repo\..\outsidecan resolve to an external directory; checking only the resolved parent or executable would lose the fact that the PATH entry traversed a repository-controlled path. Windows entries with parent traversal or unsupported namespaces fail closed; each existing lexical ancestor is canonicalized so filesystem identity catches standard/verbatim/device aliases, Unicode case aliases, and a junction inside a repository that points to an external binary.The final spawn boundary reapplies the lower-layer local-only transport environment after caller configuration. The effective change is above the repository's usual size guidance because the trust decision and its first real spawn-boundary consumers form one invariant; most of the excess is focused variant coverage and a mechanical test-module move.
Testing
just test -p codex-git-utils(42/42 passed locally)just test -p codex-git-utils git_command::tests(9/9 passed in independent rereview)just fix -p codex-git-utilsjust fmtgit diff --check.., symlink/junction aliases, logical/missing/relative/mismatchedPWD, standard/verbatim/device/UNC Windows paths, Unicode and 8.3 aliases, unsupported namespaces, filesystem errors, TOCTOU bounds, and unrelated-repository compatibilityhttps://linear.app/openai/issue/PSEC-4394