Block selected executable Git filters before patch application#30848
Block selected executable Git filters before patch application#30848bookholt-oai wants to merge 17 commits into
Conversation
![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: 0b84913ea8
ℹ️ 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".
| ensure_no_selected_executable_git_filters(&git, &git_root, &patch_paths, &cfg_parts)?; | ||
|
|
||
| if req.revert && !req.preflight { | ||
| // Stage WT paths first to avoid index mismatch on revert. |
There was a problem hiding this comment.
When req.revert && !req.preflight, this guard can pass because CODEX_APPLY_GIT_CFG supplies an effective empty override such as filter.demo.clean=, but the staging step immediately below calls stage_effective_paths without those cfg_parts; that function runs git add with only the safe defaults, so the lower-precedence repository filter is selected again and its clean command executes before the reverse apply. This leaves a bypass specifically for revert operations that rely on command-scoped config to disable a selected executable filter.
Useful? React with 👍 / 👎.
| .find_map(|suffix| remainder.strip_suffix(suffix)) | ||
| .filter(|driver| !driver.is_empty()) | ||
| .ok_or_else(|| invalid_filter_output("malformed filter config key"))?; |
There was a problem hiding this comment.
If a user's effective config contains filter..clean/smudge/process (Git accepts this and filter= can select the empty driver), this rejects the key before comparing it with the probed attributes. That means even an unused empty-name driver, or an effectively empty filter..clean= entry when another nonempty filter such as global LFS is present, makes every patch fail with InvalidData; treat the empty driver name like any other driver and only block it when check-attr actually reports the empty filter value.
Useful? React with 👍 / 👎.
…c-4394-git-05a-apply-filters # Conflicts: # codex-rs/git-utils/src/apply.rs
…c-4394-git-05a-apply-filters
…c-4394-git-05a-apply-filters
bookholt-oai
changed the base branch from
codex/psec-4394-git-03-path-containment
to
codex/psec-4394-repository-authority
…394-git-05a-apply-filters
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80c59de030
ℹ️ 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".
| /// When [`ApplyGitRequest::preflight`] is `true`, this behaves like `git apply --check` and | ||
| /// leaves the working tree untouched while still parsing the command output for diagnostics. | ||
| pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> { | ||
| let cfg_parts = configured_git_config_parts(); |
There was a problem hiding this comment.
This non-mechanical commit changes 25 files with about 5.7k added lines across config-source authorization, guarded Git commands, filter policy, apply/staging integration, and large test suites, which is far beyond the repository's 800-line guidance for ordinary changes and 500-line guidance for complex logic. Please split the smallest coherent stage first—for example, land the config-source/guarded-command capability separately from executable-filter neutralization and apply integration—so the security invariants can be reviewed independently.
AGENTS.md reference: AGENTS.md:L125-L131
Useful? React with 👍 / 👎.
1 participant
Why
Patch apply, preflight, and revert can run repository-selected clean, smudge, or process filters. Git may discover those commands through several configuration scopes and included files before the patch operation reaches its final child.
Checking each child independently would repeat expensive work and leave more room for the repository to change between checks.
Approach
Authorize the allowed configuration sources once for the whole patch operation and bind that proof to the exact runner, worktree, command-level configuration, and captured environment.
Read the current filter values and attributes again at each phase. If an affected path selects an executable filter, stop before mutation. For helper-capable children, neutralize every known filter driver and keep that protection attached through the final command.
Expose only the small set of fixed Git commands the guarded operation needs, so later callers cannot add a raw configuration or repository-selection escape.
This PR depends on #30896 and should land after it.
Testing
codex-git-utilstests passed.