Require approval for generic Git commands#28714
Conversation
bookholt-oai
changed the title
bookholt-oai
force-pushed
the
codex/harden-git-command-safety
branch
from
7a02a0d to
b72cf96
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b72cf96d66
ℹ️ 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".
| if std::path::Path::new(cmd0).components().count() != 1 { | ||
| // A workspace executable can impersonate an allowlisted utility by | ||
| // reusing its basename. Only bare names resolved through the trusted | ||
| // process PATH are eligible for generic safe-command classification. | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When ShellZshFork/UnifiedExecZshFork is enabled, the execve policy path passes the resolved absolute program path (for example /usr/bin/ls or /bin/cat) into render_decision_for_unmatched_command. This new component-count check makes those intercepted commands miss the basename safelist, so under AskForApproval::UnlessTrusted they become Prompt and users are asked to approve every safe child command even though the original bash -lc 'ls' request is still auto-approved. The path-qualified rejection should only apply to agent-supplied argv, or intercepted host paths should be normalized before safelist evaluation.
Useful? React with 👍 / 👎.
| let Some(executable_name) = command | ||
| .first() | ||
| .and_then(|executable| Path::new(executable).file_name()) | ||
| .and_then(|name| name.to_str()) |
There was a problem hiding this comment.
In cross-OS sessions where the app-server is not Windows but the exec-server command uses a Windows path such as C:\Program Files\Git\cmd\git.exe status, Path::file_name() uses the app-server's path separators, so the whole path is tested and stripping .exe leaves c:\...\git instead of git. That means suppress_repo_sensitive_git_amendment can still offer/persist a durable allow rule for repo-sensitive Git commands, reintroducing the cross-checkout approval cache this commit is meant to prevent for supported remote Windows environments.
AGENTS.md reference: AGENTS.md:L321-L322
Useful? React with 👍 / 👎.
| #[test] | ||
| fn git_status_requires_approval_for_untrusted_projects() { |
There was a problem hiding this comment.
This change alters the agent-facing approval behavior for Git commands, but the added coverage here is unit-level only; repository guidance requires integration coverage under core/suite for agent logic changes. Please add an integration test that drives an actual shell/unified-exec tool call and verifies the approval request/proposed-amendment behavior, otherwise the event wiring can regress while these policy-unit checks still pass.
AGENTS.md reference: AGENTS.md:L114-L118
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b15eb8cf5b
ℹ️ 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".
| fn suppress_repo_sensitive_git_amendment( | ||
| amendment: Option<ExecPolicyAmendment>, | ||
| ) -> Option<ExecPolicyAmendment> { | ||
| amendment.filter(|amendment| !starts_with_git_executable(&amendment.command)) |
There was a problem hiding this comment.
When the model runs Git through a common wrapper such as env git status or /usr/bin/env git status, the heuristics prompt still proposes an amendment for the whole wrapper command because this filter only looks at the first token. If the user accepts that “always allow” prompt in one checkout, Codex persists a rule for the wrapper form and silently allows the same repository-sensitive Git command in other checkouts, which defeats the cross-repo protection this suppression is meant to provide. Detect Git after simple exec wrappers (or suppress amendments for wrapper commands that delegate to Git) before offering the durable rule.
Useful? React with 👍 / 👎.
| fn trusted_executable_dirs( | ||
| env: &HashMap<String, String>, | ||
| file_system_sandbox_policy: &FileSystemSandboxPolicy, | ||
| cwd: &AbsolutePathBuf, | ||
| ) -> Vec<TrustedExecutableDir> { |
There was a problem hiding this comment.
This file is already over 1.2k lines, and this change adds the trusted executable directory/name resolution helpers directly to it. The repository guidance asks that files over roughly 800 LoC receive new functionality in a new module unless there is a documented reason not to; this path-trust logic is self-contained enough to move to a sibling module and keep unix_escalation.rs focused on orchestration.
AGENTS.md reference: AGENTS.md:L49-L53
Useful? React with 👍 / 👎.
| pub(crate) fn is_safe_git_command(_command: &[String]) -> bool { | ||
| // Git behavior depends on repository config, attributes, the discovered | ||
| // repository, environment, and TTY state. This argv-only classifier cannot | ||
| // prove that any Git command avoids repository-selected executables. | ||
| false |
There was a problem hiding this comment.
When ShellZshFork/UnifiedExecZshFork is active and the user runs a shell command like git status under UnlessTrusted, the parent shell approval now prompts because Git is no longer safe, and after that approval the execve interceptor evaluates the resolved /usr/bin/git child and prompts again for the same command. This regression only affects generic Git commands routed through zsh-fork; either carry the parent approval into the intercepted child or avoid the top-level unmatched-command prompt when execve interception will make the authoritative decision.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Why
Git is more programmable than it looks. Even a command such as
git statusorgit diffcan read repository configuration and start helper programs. Treating these commands as automatically safe based only on their command line is therefore unsafe.A one-time approval is also tied to the current repository. It should not silently become a permanent rule that applies in every checkout.
Approach
Require approval for agent-issued Git commands under
UnlessTrusted. NormalOnRequestandGranularcommands remain sandboxed, and explicit sandbox expansion still requires approval.Do not propose permanent approval rules for Git or generic wrappers such as
env,sudo,command, andnice. Only preserve safe-command treatment for bare executables resolved from a stable entry in the original host PATH.For zsh-fork, reuse an accepted parent approval once when the child is the exact Git command that was approved. This avoids asking twice without broadening the approval.
This PR covers approval behavior. Internal Git helpers and transport isolation are handled by the other PRs in this series. It should land after #30628 and #30631.
Testing
Focused shell, policy, trusted-executable, escalation, integration, cross-checkout, and real zsh-fork tests passed. Formatting and fix checks also passed.