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
PSECOP-111 showed that an argv-only “read-only Git” classification is unsafe. Repository discovery, config, attributes, environment, and TTY state can make commands such as
git status,git diff,git show,git log, andgit branchexecute repository-selected helpers without approval.The same generic classifier also relied on an executable's basename, so a direct path such as
./catcould inherit the trust of barecat(#28732). That path check is defense in depth here; the outer-shell identity and escalated-execution cases are handled separately.Finally, a one-shot Git approval could be offered as a durable global exec-policy amendment. Because Git's behavior depends on the discovered repository, approval in one checkout must not silently authorize the same argv in another checkout. Generic delegators such as
envcan hide Git behind a persistent wrapper rule, and zsh-fork previously prompted once for the parent Git command and again for the same resolved child.What
UnlessTrusted.OnRequestandGranularcommands inside the sandbox, and continue prompting for explicit sandbox escalation.env,sudo,command, andniceentrypoints; one-shot approval and explicit user-authored policy rules still work.PATH.How
env,sudo,command, ornice, rather than duplicating platform-specific wrapper parsers.PATHprovenance, canonical paths, sandbox reachability, OS writability, and replaceable ancestors.UnlessTrusted+UseDefault+ no additional permissions + one plain Git command. Explicit policy decisions run first; the carry is consumed once only after full trusted-path and argv equality checks, and never applies to sandbox expansion.envoptions/assignments, PATH-shadow, symlink, and exact one-prompt zsh-fork variants.Custom launchers and complex/model-selected outer-shell provenance remain explicit residuals rather than being claimed fixed here. The latter belongs to #30631, along with outer fake-shell identity, shell approval-cache provenance, and end-to-end
RequireEscalatedbehavior.Codex-owned internal Git helpers remain in #27914. Marketplace and local-only transport paths remain in #28760, #28761, and #29470. Trusted PowerShell parser selection remains in #30628.
Recommended landing order is #30628, then #30631, then this PR after rebasing over both. The shared
exec_policysurface has expected textual overlap, but the behavior owned by each PR is distinct.Testing
just test -p codex-shell-command— 142 passed.just test -p codex-core exec_policy::tests— 89 passed.just test -p codex-core tools::runtimes::shell::trusted_executable::tests— 4 passed.just test -p codex-core tools::runtimes::shell::unix_escalation::tests— 23 passed.just test -p codex-core suite::exec_policy— 9 passed, including direct andenv-wrapped Git through shell and unified exec.just test -p codex-core suite::generic_git_zsh_fork --no-capture— 2 passed with the real execve wrapper, proving one parent approval and no child re-prompt for both backends.just fix -p codex-shell-commandjust fix -p codex-corejust fmt