Harden Git workspace integration paths#24138
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: f053a08f4a
ℹ️ 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".
| ]; | ||
| for driver in drivers { | ||
| for (setting, value) in [ | ||
| ("clean", ""), |
There was a problem hiding this comment.
In repositories that use a clean filter such as Git LFS or a custom filter, setting filter.<driver>.clean= makes Git compare the raw worktree bytes against the already-cleaned index contents. A clean checkout with an LFS-style pointer in the index and real file contents in the worktree is then reported as modified, and git diff emits a bogus pointer-vs-content diff; this affects the shared guarded config path used by dirty checks and diff collection.
Useful? React with 👍 / 👎.
| return Ok(()); | ||
| } | ||
| let mut cmd = std::process::Command::new("git"); | ||
| cmd.args(config_overrides); |
There was a problem hiding this comment.
When reverting a patch that touches a file covered by a clean filter, such as Git LFS, this git add now receives overrides that set filter.<driver>.clean=. Git then stages the raw worktree bytes instead of the cleaned/index representation, so the pre-revert staging step can corrupt the index for filtered files rather than just avoiding hook/filter execution.
Useful? React with 👍 / 👎.
| let (tracked_diff_res, untracked_output_res) = tokio::join!( | ||
| run_git_capture_diff(runner, cwd, &["diff", "--color"]), | ||
| run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]), | ||
| run_git_capture_diff(runner, cwd, &config_overrides, &["diff", "--color"]), |
There was a problem hiding this comment.
When a repository has .gitattributes selecting a diff driver with diff.<driver>.command or textconv, this background TUI path still runs git diff --color without --no-ext-diff/--no-textconv. The added config overrides do not suppress worktree .gitattributes, so opening the diff can still execute configured diff commands through app-server without explicit approval.
Useful? React with 👍 / 👎.
| cfg_parts.push(p.to_string()); | ||
| } | ||
| } | ||
| cfg_parts.extend(safe_config_overrides); |
There was a problem hiding this comment.
For patches that touch files covered by a clean filter, adding these safe overrides to git apply --3way makes Git compare and write raw worktree contents while the index still contains the cleaned representation. In an LFS-style repo this causes otherwise valid patches/preflights to fail with index mismatch errors, and successful applications risk bypassing the repository's required clean conversion.
Useful? React with 👍 / 👎.
1 participant
Summary
git statusandgit diffcommandsfsmonitoroverride form that behaves consistently across supported system Git versionsValidation
cargo fmt -- --config imports_granularity=ItemCARGO_TARGET_DIR=/tmp/codex-git-query-hardening-target CARGO_BUILD_JOBS=1 cargo clippy --fix --tests --allow-dirty -p codex-git-utils -p codex-shell-command -p codex-tui -p codex-coreCARGO_TARGET_DIR=/tmp/codex-git-query-hardening-target CARGO_BUILD_JOBS=1 cargo test -p codex-git-utils --libCARGO_TARGET_DIR=/tmp/codex-git-query-hardening-target CARGO_BUILD_JOBS=1 cargo test -p codex-shell-command --libCARGO_TARGET_DIR=/tmp/codex-git-query-hardening-target CARGO_BUILD_JOBS=1 cargo test -p codex-core --lib ignores_configured_filtersCARGO_TARGET_DIR=/tmp/codex-git-query-hardening-target CARGO_BUILD_JOBS=1 RUST_MIN_STACK=8388608 cargo test -p codex-tui --lib get_git_diff