Skip to content

Bind Git worktree helpers to a trusted executable#30833

Open
bookholt-oai wants to merge 6 commits into
bookholt/deny-transport-for-local-gitfrom
codex/psec-4394-trusted-git-runner
Open

Bind Git worktree helpers to a trusted executable#30833
bookholt-oai wants to merge 6 commits into
bookholt/deny-transport-for-local-gitfrom
codex/psec-4394-trusted-git-runner

Conversation

@bookholt-oai

@bookholt-oai bookholt-oai commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Why

Internal worktree helpers selected git from PATH at 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

  • Select one absolute native Git executable outside the active repository, every enclosing repository, their linked worktrees, and their common metadata.
  • Reuse that executable for synchronous patch and repository operations.
  • Remove inherited repository, index, pathspec, executable-path, and legacy config selectors immediately before spawn.
  • Reject raw Windows PATH spellings whose canonical resolution could otherwise conceal traversal through repository-controlled paths, including parent traversal, unsupported device namespaces, Unicode/prefix aliases, and repository junctions that resolve outside.
  • Keep the initial refusal reasons private until the later checked-read API layer needs a public contract.

How

The resolver accumulates untrusted locations from the canonical repository, the requested lexical path, and a logical process PWD only 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-dir layouts, 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\..\outside can 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-utils
  • just fmt
  • git diff --check
  • Independent product/security rereview passed for nested repositories, submodules, linked/common Git directories, lexical .., symlink/junction aliases, logical/missing/relative/mismatched PWD, standard/verbatim/device/UNC Windows paths, Unicode and 8.3 aliases, unsupported namespaces, filesystem errors, TOCTOU bounds, and unrelated-repository compatibility
  • Windows-only junction and namespace variants run in blocking CI

https://linear.app/openai/issue/PSEC-4394

@bookholt-oai bookholt-oai marked this pull request as ready for review July 1, 2026 15:04

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread codex-rs/git-utils/src/operations.rs Outdated
}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge 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 👍 / 👎.

Comment on lines +52 to +53
if path_is_untrusted(&candidate, untrusted) {
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge 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 👍 / 👎.

Comment thread codex-rs/git-utils/src/git_command.rs Outdated
Comment on lines +108 to +111
locations
.common_dir
.as_deref()
.is_some_and(|common_dir| path_is_in_worktree_for_common_dir(path, common_dir))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant