openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-20 12:47:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fail closed on executable Git helpers

Browse Source
This commit is contained in:
Chris Bookholt
2026-06-17 10:44:42 -07:00
parent ce502e9c17
commit 6fcb33401b
4 changed files with 101 additions and 189 deletions
Download Patch File Download Diff File Expand all files Collapse all files

116
codex-rs/git-utils/src/apply.rs
View File

@@ -14,8 +14,9 @@ use std::path::PathBuf;
use crate::FsmonitorOverride;
use crate::safe_git::DISABLED_HOOKS_PATH;
use crate::safe_git::GitConfigOverride;
use crate::safe_git::configured_executable_git_config_overrides;
use crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN;
use crate::safe_git::EXECUTABLE_PATCH_CONFIG_PATTERN;
use crate::safe_git::ensure_no_executable_git_config;
/// Parameters for invoking [`apply_git_patch`].
#[derive(Debug, Clone)]
@@ -44,6 +45,7 @@ pub struct ApplyGitResult {
/// leaves the working tree untouched while still parsing the command output for diagnostics.
pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
let git_root = resolve_git_root(&req.cwd)?;
ensure_no_executable_git_config(&git_root, EXECUTABLE_PATCH_CONFIG_PATTERN)?;
// Write unified diff into a temporary file
let (tmpdir, patch_path) = write_temp_patch(&req.diff)?;
@@ -54,8 +56,6 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
// Stage WT paths first to avoid index mismatch on revert.
stage_paths(&git_root, &req.diff)?;
}
let executable_git_config_overrides = configured_executable_git_config_overrides(&git_root)?;
// Build git args
let mut args: Vec<String> = vec!["apply".into(), "--3way".into()];
if req.revert {
@@ -86,12 +86,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
}
check_args.push(patch_path.to_string_lossy().to_string());
let rendered = render_command_for_log(&git_root, &cfg_parts, &check_args);
let (c_code, c_out, c_err) = run_git(
&git_root,
&cfg_parts,
&executable_git_config_overrides,
&check_args,
)?;
let (c_code, c_out, c_err) = run_git(&git_root, &cfg_parts, &check_args)?;
let (mut applied_paths, mut skipped_paths, mut conflicted_paths) =
parse_git_apply_output(&c_out, &c_err);
applied_paths.sort();
@@ -112,12 +107,7 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result<ApplyGitResult> {
}
let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args);
let (code, stdout, stderr) = run_git(
&git_root,
&cfg_parts,
&executable_git_config_overrides,
&args,
)?;
let (code, stdout, stderr) = run_git(&git_root, &cfg_parts, &args)?;
let (mut applied_paths, mut skipped_paths, mut conflicted_paths) =
parse_git_apply_output(&stdout, &stderr);
@@ -164,12 +154,7 @@ fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, PathBuf)> {
Ok((dir, path))
}
fn run_git(
cwd: &Path,
git_cfg: &[String],
executable_git_config_overrides: &[GitConfigOverride],
args: &[String],
) -> io::Result<(i32, String, String)> {
fn run_git(cwd: &Path, git_cfg: &[String], args: &[String]) -> io::Result<(i32, String, String)> {
let mut cmd = std::process::Command::new("git");
for p in git_cfg {
cmd.arg(p);
@@ -177,7 +162,6 @@ fn run_git(
for a in args {
cmd.arg(a);
}
add_executable_git_config_overrides(&mut cmd, executable_git_config_overrides);
let out = cmd.current_dir(cwd).output()?;
let code = out.status.code().unwrap_or(-1);
let stdout = String::from_utf8_lossy(&out.stdout).into_owned();
@@ -194,21 +178,6 @@ fn safe_git_config_parts() -> Vec<String> {
]
}
fn add_executable_git_config_overrides(
command: &mut std::process::Command,
config_overrides: &[GitConfigOverride],
) {
if config_overrides.is_empty() {
return;
}
command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string());
for (index, (key, value)) in config_overrides.iter().enumerate() {
command
.env(format!("GIT_CONFIG_KEY_{index}"), key)
.env(format!("GIT_CONFIG_VALUE_{index}"), value);
}
}
fn quote_shell(s: &str) -> String {
let simple = s
.chars()
@@ -364,6 +333,7 @@ fn unescape_c_string(input: &str) -> String {
/// Stage only the files that actually exist on disk for the given diff.
pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> {
ensure_no_executable_git_config(git_root, EXECUTABLE_FILTER_CONFIG_PATTERN)?;
let paths = extract_paths_from_patch(diff);
let mut existing: Vec<String> = Vec::new();
for p in paths {
@@ -378,13 +348,7 @@ pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> {
let mut args = vec!["add".to_string(), "--".to_string()];
args.extend(existing);
let config_parts = safe_git_config_parts();
let executable_git_config_overrides = configured_executable_git_config_overrides(git_root)?;
let (_code, _, _) = run_git(
git_root,
&config_parts,
&executable_git_config_overrides,
&args,
)?;
let (_code, _, _) = run_git(git_root, &config_parts, &args)?;
// We do not hard fail staging; best-effort is OK. Return Ok even on non-zero.
Ok(())
}
@@ -708,6 +672,28 @@ mod tests {
code == 0
}
fn configure_merge_driver(root: &Path, tracked_path: &str) {
std::fs::write(
root.join(".gitattributes"),
format!("{tracked_path} merge=codex-test\n"),
)
.expect("write attributes");
let (add_code, _, add_err) = run(root, &["git", "add", ".gitattributes"]);
assert_eq!(add_code, 0, "add attributes: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "attributes"]);
assert_eq!(commit_code, 0, "commit attributes: {commit_err}");
let (config_code, _, config_err) = run(
root,
&[
"git",
"config",
"merge.codex-test.driver",
"git config codex.mergeran true && false",
],
);
assert_eq!(config_code, 0, "configure merge driver: {config_err}");
}
#[test]
fn extract_paths_handles_quoted_headers() {
let diff = "diff --git \"a/hello world.txt\" \"b/hello world.txt\"\nnew file mode 100644\n--- /dev/null\n+++ b/hello world.txt\n@@ -0,0 +1 @@\n+hi\n";
@@ -921,7 +907,7 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1
}
#[test]
fn apply_ignores_configured_clean_filter() {
fn apply_rejects_configured_clean_filter_without_running_it() {
let _g = env_lock().lock().unwrap();
let repo = init_repo();
let root = repo.path();
@@ -939,23 +925,39 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1
revert: false,
preflight: true,
};
let preflight = apply_git_patch(&preflight_req).expect("preflight apply");
assert_eq!(preflight.exit_code, 0, "preflight apply succeeded");
let error = apply_git_patch(&preflight_req).expect_err("reject configured filter");
assert_eq!(error.kind(), io::ErrorKind::Unsupported);
assert!(!configured_filter_ran(root));
assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n");
let apply_req = ApplyGitRequest {
let stage_error = stage_paths(root, diff).expect_err("reject configured filter");
assert_eq!(stage_error.kind(), io::ErrorKind::Unsupported);
assert!(!configured_filter_ran(root));
}
#[test]
fn apply_rejects_configured_merge_driver_without_running_it() {
let _g = env_lock().lock().unwrap();
let repo = init_repo();
let root = repo.path();
std::fs::write(root.join("file.txt"), "orig\n").expect("write file");
let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]);
assert_eq!(add_code, 0, "add file: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]);
assert_eq!(commit_code, 0, "commit file: {commit_err}");
configure_merge_driver(root, "file.txt");
let diff = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n";
let request = ApplyGitRequest {
cwd: root.to_path_buf(),
diff: diff.to_string(),
revert: false,
preflight: false,
};
let applied = apply_git_patch(&apply_req).expect("apply");
assert_eq!(
applied.exit_code, 0,
"apply succeeded\nstdout:\n{}\nstderr:\n{}",
applied.stdout, applied.stderr
);
assert!(!configured_filter_ran(root));
assert_eq!(read_file_normalized(&root.join("file.txt")), "next\n");
let error = apply_git_patch(&request).expect_err("reject configured merge driver");
assert_eq!(error.kind(), io::ErrorKind::Unsupported);
let (marker_code, _, _) = run(root, &["git", "config", "--get", "codex.mergeran"]);
assert_ne!(marker_code, 0, "merge driver must not run");
assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n");
}
}

57
codex-rs/git-utils/src/info.rs
View File

@@ -18,9 +18,8 @@ use ts_rs::TS;
use crate::GitSha;
use crate::safe_git::DISABLED_HOOKS_PATH;
use crate::safe_git::EXECUTABLE_GIT_CONFIG_PATTERN;
use crate::safe_git::GitConfigOverride;
use crate::safe_git::executable_git_config_overrides_from_output;
use crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN;
use crate::safe_git::config_output_has_entries;
/// Return `true` if the project folder specified by the `Config` is inside a
/// Git repository.
@@ -285,6 +284,9 @@ fn trim_git_suffix(value: &str) -> &str {
pub async fn get_has_changes(cwd: &Path) -> Option<bool> {
let git = Path::new("git");
if has_configured_executable_filters_from(git, cwd).await? {
return None;
}
let fsmonitor = detect_local_fsmonitor_override(git, cwd).await;
let output =
run_git_command_with_timeout_from(git, &["status", "--porcelain"], cwd, fsmonitor).await?;
@@ -438,20 +440,17 @@ async fn run_git_command_with_timeout_from(
cwd: &Path,
fsmonitor: crate::FsmonitorOverride,
) -> Option<std::process::Output> {
let executable_git_config_overrides =
configured_executable_git_config_overrides_from(git, cwd).await?;
let disabled_hooks = format!("core.hooksPath={DISABLED_HOOKS_PATH}");
let mut command = Command::new(git);
command
.env("GIT_OPTIONAL_LOCKS", "0")
// Keep internal Git commands independent of repository-selected hooks
// and executable Git helpers while preserving built-in fsmonitor acceleration.
// while preserving built-in fsmonitor acceleration.
.args(["-c", &disabled_hooks])
.args(["-c", fsmonitor.git_config_arg()])
.args(args)
.current_dir(cwd)
.kill_on_drop(true);
add_executable_git_config_overrides(&mut command, &executable_git_config_overrides);
command.args(args);
let result = timeout(GIT_COMMAND_TIMEOUT, command.output()).await;
match result {
@@ -460,10 +459,7 @@ async fn run_git_command_with_timeout_from(
}
}
async fn configured_executable_git_config_overrides_from(
git: &Path,
cwd: &Path,
) -> Option<Vec<GitConfigOverride>> {
async fn has_configured_executable_filters_from(git: &Path, cwd: &Path) -> Option<bool> {
let mut command = Command::new(git);
command
.args([
@@ -471,7 +467,7 @@ async fn configured_executable_git_config_overrides_from(
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_GIT_CONFIG_PATTERN,
EXECUTABLE_FILTER_CONFIG_PATTERN,
])
.current_dir(cwd)
.kill_on_drop(true);
@@ -487,22 +483,7 @@ async fn configured_executable_git_config_overrides_from(
return None;
}
Some(executable_git_config_overrides_from_output(&output.stdout))
}
fn add_executable_git_config_overrides(
command: &mut Command,
config_overrides: &[GitConfigOverride],
) {
if config_overrides.is_empty() {
return;
}
command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string());
for (index, (key, value)) in config_overrides.iter().enumerate() {
command
.env(format!("GIT_CONFIG_KEY_{index}"), key)
.env(format!("GIT_CONFIG_VALUE_{index}"), value);
}
Some(config_output_has_entries(&output.stdout))
}
async fn get_git_remotes(cwd: &Path) -> Option<Vec<String>> {
@@ -783,6 +764,9 @@ async fn find_closest_sha(cwd: &Path, branches: &[String], remotes: &[String]) -
async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option<String> {
let git = Path::new("git");
if has_configured_executable_filters_from(git, cwd).await? {
return None;
}
let fsmonitor = detect_local_fsmonitor_override(git, cwd).await;
let output = run_git_command_with_timeout_from(
git,
@@ -1070,17 +1054,17 @@ mod tests {
}
#[tokio::test]
async fn get_has_changes_ignores_configured_clean_filter() {
async fn get_has_changes_rejects_configured_clean_filter_without_running_it() {
let temp_dir = tempfile::tempdir().expect("create temp dir");
let repo_path = create_test_git_repo(&temp_dir).await;
configure_clean_filter(&repo_path, "test.txt").await;
assert_eq!(get_has_changes(&repo_path).await, Some(false));
assert_eq!(get_has_changes(&repo_path).await, None);
assert!(!configured_filter_ran(&repo_path).await);
}
#[tokio::test]
async fn git_diff_to_remote_ignores_configured_clean_filter() {
async fn git_diff_to_remote_rejects_configured_clean_filter_without_running_it() {
let temp_dir = tempfile::tempdir().expect("create temp dir");
let repo_path = create_test_git_repo(&temp_dir).await;
let remote_path = temp_dir.path().join("remote.git");
@@ -1110,10 +1094,7 @@ mod tests {
configure_clean_filter(&repo_path, "test.txt").await;
run_git(&repo_path, &["push", "origin", &branch]).await;
let state = git_diff_to_remote(&repo_path)
.await
.expect("collect working tree state");
assert!(state.diff.is_empty());
assert!(git_diff_to_remote(&repo_path).await.is_none());
assert!(!configured_filter_ran(&repo_path).await);
}
@@ -1167,9 +1148,6 @@ mod tests {
"config --null --get core.fsmonitor".to_string(),
"config --null --type=bool --fixed-value --get core.fsmonitor /tmp/fsmonitor-helper"
.to_string(),
format!(
"config --null --name-only --get-regexp {EXECUTABLE_GIT_CONFIG_PATTERN}"
),
format!("-c {disabled_hooks} -c core.fsmonitor=false status --porcelain"),
]
);
@@ -1257,7 +1235,6 @@ mod tests {
vec![
"config --null --get core.fsmonitor".to_string(),
"version --build-options".to_string(),
format!("config --null --name-only --get-regexp {EXECUTABLE_GIT_CONFIG_PATTERN}"),
format!("-c {disabled_hooks} -c core.fsmonitor=true status --porcelain"),
]
);

19
codex-rs/git-utils/src/operations.rs
View File

@@ -6,8 +6,6 @@ use std::process::Command;
use crate::GitToolingError;
use crate::safe_git::DISABLED_HOOKS_PATH;
use crate::safe_git::GitConfigOverride;
use crate::safe_git::configured_executable_git_config_overrides;
pub(crate) fn ensure_git_repository(path: &Path) -> Result<(), GitToolingError> {
match run_git_for_stdout(
@@ -110,7 +108,6 @@ where
for arg in iterator {
args_vec.push(OsString::from(arg.as_ref()));
}
let executable_git_config_overrides = configured_executable_git_config_overrides(dir)?;
let command_string = build_command_string(&args_vec);
let mut command = Command::new("git");
command.current_dir(dir);
@@ -119,7 +116,6 @@ where
command.env(key, value);
}
}
add_executable_git_config_overrides(&mut command, &executable_git_config_overrides);
command.args(&args_vec);
let output = command.output()?;
if !output.status.success() {
@@ -136,21 +132,6 @@ where
})
}
fn add_executable_git_config_overrides(
command: &mut Command,
config_overrides: &[GitConfigOverride],
) {
if config_overrides.is_empty() {
return;
}
command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string());
for (index, (key, value)) in config_overrides.iter().enumerate() {
command
.env(format!("GIT_CONFIG_KEY_{index}"), key)
.env(format!("GIT_CONFIG_VALUE_{index}"), value);
}
}
fn build_command_string(args: &[OsString]) -> String {
if args.is_empty() {
return "git".to_string();

98
codex-rs/git-utils/src/safe_git.rs
View File

@@ -1,98 +1,50 @@
use std::collections::BTreeSet;
use std::io;
use std::path::Path;
use std::process::Command;
pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" };
pub(crate) const EXECUTABLE_GIT_CONFIG_PATTERN: &str =
r"^(filter\..*\.(clean|smudge|process|required)|merge\..*\.driver)$";
pub(crate) type GitConfigOverride = (String, String);
pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$";
pub(crate) const EXECUTABLE_PATCH_CONFIG_PATTERN: &str =
r"^(filter\..*\.(clean|smudge|process)|merge\..*\.driver)$";
pub(crate) fn configured_executable_git_config_overrides(
cwd: &Path,
) -> io::Result<Vec<GitConfigOverride>> {
pub(crate) fn ensure_no_executable_git_config(cwd: &Path, pattern: &str) -> io::Result<()> {
let output = Command::new("git")
.args([
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_GIT_CONFIG_PATTERN,
])
.env("GIT_OPTIONAL_LOCKS", "0")
.args(["config", "--null", "--name-only", "--get-regexp", pattern])
.current_dir(cwd)
.output()?;
if output
if !output
.status
.code()
.is_some_and(|code| code == 0 || code == 1)
{
return Ok(executable_git_config_overrides_from_output(&output.stdout));
return Err(io::Error::other(format!(
"git config probe failed with status {}: {}",
output.status,
String::from_utf8_lossy(&output.stderr).trim()
)));
}
Err(io::Error::other(format!(
"git config probe failed with status {}: {}",
output.status,
String::from_utf8_lossy(&output.stderr).trim()
)))
if config_output_has_entries(&output.stdout) {
return Err(io::Error::new(
io::ErrorKind::Unsupported,
"refusing to run an internal Git worktree operation with executable Git helpers configured",
));
}
Ok(())
}
pub(crate) fn executable_git_config_overrides_from_output(stdout: &[u8]) -> Vec<GitConfigOverride> {
let mut filter_drivers = BTreeSet::new();
let mut merge_drivers = BTreeSet::new();
for key in stdout
.split(|byte| *byte == 0)
.filter(|key| !key.is_empty())
.filter_map(|key| std::str::from_utf8(key).ok())
{
if let Some(driver) = key
.strip_suffix(".clean")
.or_else(|| key.strip_suffix(".smudge"))
.or_else(|| key.strip_suffix(".process"))
.or_else(|| key.strip_suffix(".required"))
{
filter_drivers.insert(driver.to_string());
} else if key.starts_with("merge.") && key.ends_with(".driver") {
merge_drivers.insert(key.to_string());
}
}
filter_drivers
.into_iter()
.flat_map(|driver| {
[
(format!("{driver}.clean"), String::new()),
(format!("{driver}.smudge"), String::new()),
(format!("{driver}.process"), String::new()),
(format!("{driver}.required"), "false".to_string()),
]
})
.chain(
merge_drivers
.into_iter()
.map(|driver| (driver, String::new())),
)
.collect()
pub(crate) fn config_output_has_entries(stdout: &[u8]) -> bool {
stdout.iter().any(|byte| *byte != 0)
}
#[cfg(test)]
mod tests {
use super::*;
use pretty_assertions::assert_eq;
#[test]
fn executable_git_config_overrides_clear_filters_and_merge_drivers() {
let output = b"filter.x=y.clean\0filter.x=y.required\0merge.pwn.driver\0";
assert_eq!(
executable_git_config_overrides_from_output(output),
vec![
("filter.x=y.clean".to_string(), String::new()),
("filter.x=y.smudge".to_string(), String::new()),
("filter.x=y.process".to_string(), String::new()),
("filter.x=y.required".to_string(), "false".to_string()),
("merge.pwn.driver".to_string(), String::new()),
]
);
fn detects_nonempty_null_delimited_config_output() {
assert!(!config_output_has_entries(b""));
assert!(!config_output_has_entries(b"\0"));
assert!(config_output_has_entries(b"filter.example.clean\0"));
}
}