From 30d7a7625432969c22dfed15a013ab2872d65959 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 06:16:32 -0700 Subject: [PATCH] git-utils: scope config authorization to apply operations --- codex-rs/git-utils/src/apply.rs | 180 +++++--- codex-rs/git-utils/src/apply_filter_tests.rs | 32 ++ codex-rs/git-utils/src/errors.rs | 6 +- codex-rs/git-utils/src/filter_sentinel.rs | 1 + codex-rs/git-utils/src/git_command.rs | 17 + codex-rs/git-utils/src/git_command_tests.rs | 95 ++++ .../git-utils/src/git_config_environment.rs | 136 ++++++ .../src/git_config_environment_tests.rs | 126 +++++ codex-rs/git-utils/src/git_config_sources.rs | 2 +- .../src/git_config_sources/primary_sources.rs | 62 +-- codex-rs/git-utils/src/guarded_config.rs | 434 ++++++++++++++++++ .../git-utils/src/guarded_config_tests.rs | 116 +++++ codex-rs/git-utils/src/lib.rs | 2 + codex-rs/git-utils/src/patch_paths.rs | 135 +++++- .../src/repository_authority/authority.rs | 17 + codex-rs/git-utils/src/safe_git.rs | 258 +++-------- codex-rs/git-utils/src/safe_git_tests.rs | 74 +-- 17 files changed, 1364 insertions(+), 329 deletions(-) create mode 100644 codex-rs/git-utils/src/git_config_environment.rs create mode 100644 codex-rs/git-utils/src/git_config_environment_tests.rs create mode 100644 codex-rs/git-utils/src/guarded_config.rs create mode 100644 codex-rs/git-utils/src/guarded_config_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index d9c68fc2ae..5f3186a70d 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -13,11 +13,10 @@ use std::path::PathBuf; use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_config_sources; -use crate::patch_paths::extract_effective_paths_from_patch; -use crate::patch_paths::stage_effective_paths; +use crate::guarded_config::GuardedGitConfig; +use crate::patch_paths::extract_effective_paths_from_patch_guarded; +use crate::patch_paths::stage_effective_paths_guarded; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::ensure_no_selected_executable_git_filters; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; @@ -47,29 +46,27 @@ pub struct ApplyGitResult { /// 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 { + let cfg_parts = configured_git_config_parts(); + // Construct from the caller's exact route before canonicalization so + // repository authority retains lexical enclosing worktrees. let git = GitRunner::for_cwd_io(&req.cwd)?; - let mut cfg_parts = configured_git_config_parts(); - ensure_no_worktree_config_sources(&git, &req.cwd, &cfg_parts)?; let requested_cwd = std::fs::canonicalize(&req.cwd)?; - let git_root = resolve_git_root(&git, &req.cwd, &cfg_parts)?; - if git_root != requested_cwd { - ensure_no_worktree_config_sources(&git, &git_root, &cfg_parts)?; - } + let expected_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| io::Error::other("not a Git repository")) + .and_then(std::fs::canonicalize)?; + let mut config = GuardedGitConfig::authorize(&git, &expected_root, cfg_parts)?; + resolve_git_root(&config, &requested_cwd)?; // Write unified diff into a temporary file let (tmpdir, patch_path) = write_temp_patch(&req.diff)?; // Keep tmpdir alive until function end to ensure the file exists let _guard = tmpdir; - let patch_paths = - extract_effective_paths_from_patch(&git, &git_root, &patch_path, req.revert, &cfg_parts)?; - let filter_guard = - ensure_no_selected_executable_git_filters(&git, &git_root, &patch_paths, &cfg_parts)?; - cfg_parts.extend(safe_git_config_parts()); - cfg_parts.extend_from_slice(filter_guard.git_config_args()); + let patch_paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, req.revert)?; + config.authorize_filter_paths(&patch_paths)?; if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; + stage_effective_paths_guarded(&config, &patch_paths)?; } // Build git args @@ -87,8 +84,8 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { check_args.push("-R".to_string()); } 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, &git_root, &cfg_parts, &check_args)?; + let rendered = config.render_command_for_log(&check_args)?; + let (c_code, c_out, c_err) = run_guarded_apply(&config, &check_args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&c_out, &c_err); applied_paths.sort(); @@ -108,8 +105,8 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }); } - let cmd_for_log = render_command_for_log(&git_root, &cfg_parts, &args); - let (code, stdout, stderr) = run_git(&git, &git_root, &cfg_parts, &args)?; + let cmd_for_log = config.render_command_for_log(&args)?; + let (code, stdout, stderr) = run_guarded_apply(&config, &args)?; let (mut applied_paths, mut skipped_paths, mut conflicted_paths) = parse_git_apply_output(&stdout, &stderr); @@ -131,18 +128,10 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { }) } -fn resolve_git_root( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], -) -> io::Result { - let requested_cwd = std::fs::canonicalize(cwd)?; - let mut command = git.command_for_cwd(&requested_cwd)?; - command - .args(git_config_args) - .arg("rev-parse") - .arg("--show-toplevel"); - let out = git.output(command)?; +fn resolve_git_root(config: &GuardedGitConfig<'_>, requested_cwd: &Path) -> io::Result { + let mut command = config.rev_parse_command()?; + command.arg("--show-toplevel"); + let out = command.output()?; let code = out.status.code().unwrap_or(-1); if code != 0 { return Err(io::Error::other(format!( @@ -153,18 +142,7 @@ fn resolve_git_root( } let reported_root = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim()); let root = std::fs::canonicalize(&reported_root)?; - let expected_root = crate::get_git_repo_root(&requested_cwd) - .ok_or_else(|| { - io::Error::new( - io::ErrorKind::PermissionDenied, - format!( - "refusing to apply a patch because Git resolved worktree {} without a .git marker above requested cwd {}", - root.display(), - requested_cwd.display() - ), - ) - }) - .and_then(std::fs::canonicalize)?; + let expected_root = config.canonical_root(); if root != expected_root { return Err(io::Error::new( io::ErrorKind::PermissionDenied, @@ -221,6 +199,32 @@ pub(crate) fn run_git( Ok((code, stdout, stderr)) } +fn run_guarded_apply( + config: &GuardedGitConfig<'_>, + args: &[String], +) -> io::Result<(i32, String, String)> { + let Some((subcommand, args)) = args.split_first() else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "missing guarded Git subcommand", + )); + }; + if subcommand != "apply" { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "unexpected guarded Git subcommand", + )); + } + let mut command = config.apply_command()?; + command.args(args); + let output = command.output()?; + Ok(( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + )) +} + pub(crate) fn safe_git_config_parts() -> Vec { vec![ "-c".to_string(), @@ -230,33 +234,6 @@ pub(crate) fn safe_git_config_parts() -> Vec { ] } -fn quote_shell(s: &str) -> String { - let simple = s - .chars() - .all(|c| c.is_ascii_alphanumeric() || "-_.:/@%+".contains(c)); - if simple { - s.to_string() - } else { - format!("'{}'", s.replace('\'', "'\\''")) - } -} - -fn render_command_for_log(cwd: &Path, git_cfg: &[String], args: &[String]) -> String { - let mut parts: Vec = Vec::new(); - parts.push("git".to_string()); - for a in git_cfg { - parts.push(quote_shell(a)); - } - for a in args { - parts.push(quote_shell(a)); - } - format!( - "(cd {} && {})", - quote_shell(&cwd.display().to_string()), - parts.join(" ") - ) -} - #[cfg(all(test, unix))] #[path = "apply_transport_tests.rs"] mod transport_tests; @@ -268,6 +245,8 @@ mod filter_tests; #[cfg(test)] mod tests { use super::*; + use crate::guarded_config::config_source_authorization_count; + use crate::guarded_config::reset_config_source_authorization_count; use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; @@ -412,7 +391,9 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r = apply_git_patch(&req).expect("run apply"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(r.exit_code, 0, "exit code 0"); // File exists now assert!(root.join("hello.txt").exists()); @@ -541,6 +522,41 @@ mod tests { assert!(!marker.exists(), "enclosing Git shim must not run"); } + #[cfg(unix)] + #[test] + fn apply_uses_physical_repository_for_symlinked_nested_cwd() { + let _g = env_lock().lock().unwrap(); + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + let nested = target.join("nested"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&nested).expect("nested"); + let (outer_init, _, outer_error) = run(&outer, &["git", "init", "-q"]); + assert_eq!(outer_init, 0, "outer init: {outer_error}"); + let (target_init, _, target_error) = run(&target, &["git", "init", "-q"]); + assert_eq!(target_init, 0, "target init: {target_error}"); + let lexical_cwd = outer.join("linked-nested"); + std::os::unix::fs::symlink(&nested, &lexical_cwd).expect("nested cwd symlink"); + + reset_config_source_authorization_count(); + let result = apply_git_patch(&ApplyGitRequest { + cwd: lexical_cwd, + diff: "diff --git a/physical.txt b/physical.txt\nnew file mode 100644\n--- /dev/null\n+++ b/physical.txt\n@@ -0,0 +1 @@\n+physical\n".to_string(), + revert: false, + preflight: false, + }) + .expect("apply through symlinked nested cwd"); + + assert_eq!(result.exit_code, 0); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + read_file_normalized(&target.join("physical.txt")), + "physical\n" + ); + assert!(!outer.join("physical.txt").exists()); + } + #[test] fn apply_modify_conflict() { let _g = env_lock().lock().unwrap(); @@ -599,7 +615,9 @@ mod tests { revert: false, preflight: false, }; + reset_config_source_authorization_count(); let res_apply = apply_git_patch(&apply_req).expect("apply ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_apply.exit_code, 0, "forward apply succeeded"); let after_apply = read_file_normalized(&root.join("file.txt")); assert_eq!(after_apply, "ORIG\n"); @@ -611,7 +629,9 @@ mod tests { revert: true, preflight: false, }; + reset_config_source_authorization_count(); let res_revert = apply_git_patch(&revert_req).expect("revert ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_revert.exit_code, 0, "revert apply succeeded"); let after_revert = read_file_normalized(&root.join("file.txt")); assert_eq!(after_revert, "orig\n"); @@ -648,7 +668,9 @@ mod tests { revert: true, preflight: true, }; + reset_config_source_authorization_count(); let res_preflight = apply_git_patch(&preflight_req).expect("preflight ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_preflight.exit_code, 0, "revert preflight succeeded"); let (_code_after, staged_after, _stderr_after) = run(root, &["git", "diff", "--cached", "--name-only"]); @@ -678,7 +700,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: true, }; + reset_config_source_authorization_count(); let r1 = apply_git_patch(&req1).expect("preflight apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r1.exit_code, 0, "preflight reports failure"); assert!( !root.join("ok.txt").exists(), @@ -696,7 +720,9 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert: false, preflight: false, }; + reset_config_source_authorization_count(); let r2 = apply_git_patch(&req2).expect("direct apply"); + assert_eq!(config_source_authorization_count(), 1); assert_ne!(r2.exit_code, 0, "apply is expected to fail overall"); assert!( !r2.cmd_for_log.contains("--check"), @@ -724,7 +750,13 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 revert, preflight, }; + reset_config_source_authorization_count(); let error = apply_git_patch(&request).expect_err("reject configured filter"); + assert_eq!( + config_source_authorization_count(), + 1, + "{revert} {preflight}" + ); assert_eq!(error.kind(), io::ErrorKind::Unsupported); assert!(!configured_filter_ran(root)); assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); @@ -812,8 +844,10 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 assert_eq!(config_code, 0, "configure core.worktree: {config_err}"); let git = GitRunner::for_cwd_io(&attacker).expect("trusted Git"); + let guarded = GuardedGitConfig::authorize(&git, &attacker, Vec::new()) + .expect("authorize attacker repository config"); let error = - resolve_git_root(&git, &attacker, &[]).expect_err("reject redirected worktree"); + resolve_git_root(&guarded, &attacker).expect_err("reject redirected worktree"); assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); assert!(error.to_string().contains("instead of expected worktree")); } diff --git a/codex-rs/git-utils/src/apply_filter_tests.rs b/codex-rs/git-utils/src/apply_filter_tests.rs index 12da828fa6..d2418d4a03 100644 --- a/codex-rs/git-utils/src/apply_filter_tests.rs +++ b/codex-rs/git-utils/src/apply_filter_tests.rs @@ -553,6 +553,38 @@ fn command_scoped_offpath_filter_is_neutralized_after_local_config() { ); } +#[test] +fn filter_override_rejects_process_temp_directory_inside_worktree() { + let root_name = "CODEX_GIT_UTILS_WORKTREE_TMP_ROOT"; + if let Some(root) = std::env::var_os(root_name) { + let root = std::path::PathBuf::from(root); + let error = apply_git_patch(&request( + &root, /*revert*/ false, /*preflight*/ true, + )) + .expect_err("reject worktree-owned filter override"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!(!configured_filter_ran(&root)); + return; + } + + let repo = init_repo(); + let root = repo.path(); + configure_filter(root, "demo"); + let worktree_temp = root.join("process-temp"); + std::fs::create_dir(&worktree_temp).expect("worktree temp directory"); + run_isolated_test( + "apply::filter_tests::filter_override_rejects_process_temp_directory_inside_worktree", + &[ + (root_name, root.as_os_str()), + ("TMPDIR", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TEMP", worktree_temp.as_os_str()), + #[cfg(windows)] + ("TMP", worktree_temp.as_os_str()), + ], + ); +} + #[test] fn apply_neutralizes_filter_activated_for_racy_offpath_by_same_patch() { for (name, command) in [ diff --git a/codex-rs/git-utils/src/errors.rs b/codex-rs/git-utils/src/errors.rs index 574cfc311a..04274135ec 100644 --- a/codex-rs/git-utils/src/errors.rs +++ b/codex-rs/git-utils/src/errors.rs @@ -50,6 +50,8 @@ pub(crate) enum GitReadError { InvalidRepositoryMetadata { path: PathBuf, reason: String }, #[error("{path:?} is not a Git repository")] NotRepository { path: PathBuf }, + #[error("invalid Git configuration environment: {reason}")] + InvalidConfigEnvironment { reason: String }, } impl GitReadError { @@ -59,7 +61,9 @@ impl GitReadError { Self::UnprovenPrimaryAuthority { .. } | Self::UnsafeRepositoryMetadata { .. } => { std::io::ErrorKind::PermissionDenied } - Self::InvalidRepositoryMetadata { .. } => std::io::ErrorKind::InvalidData, + Self::InvalidRepositoryMetadata { .. } | Self::InvalidConfigEnvironment { .. } => { + std::io::ErrorKind::InvalidData + } } } diff --git a/codex-rs/git-utils/src/filter_sentinel.rs b/codex-rs/git-utils/src/filter_sentinel.rs index 7847ac3e63..dc3cd1ee56 100644 --- a/codex-rs/git-utils/src/filter_sentinel.rs +++ b/codex-rs/git-utils/src/filter_sentinel.rs @@ -52,6 +52,7 @@ pub(crate) fn classify_sentinel_filter_probes( } } +#[cfg(test)] pub(crate) fn sentinel_filter_probe_config_args( neutralization_args: &[String], driver: &str, diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 7e637664de..d51b44d04b 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -6,6 +6,7 @@ use std::process::Command; use std::process::Stdio; use crate::errors::GitReadError; +use crate::git_config_environment::GitConfigEnvironmentSnapshot; #[cfg(test)] use crate::git_executable::git_executable_name; use crate::git_executable::harden_git_launch_environment; @@ -31,6 +32,7 @@ pub(crate) struct GitRunner { argv0: PathBuf, safe_path: std::ffi::OsString, authority: RepositoryAuthority, + config_environment: GitConfigEnvironmentSnapshot, } /// A Git command that can only be spawned through [`GitRunner::output`], @@ -95,6 +97,7 @@ impl GitRunner { command.current_dir(parent); } harden_git_launch_environment(&mut command, &self.safe_path); + self.config_environment.apply_to(&mut command); GitCommand { inner: command } } @@ -119,6 +122,14 @@ impl GitRunner { .ensure_config_source_is_not_worktree_controlled(path, description) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_active_worktree_root(root) + } + + pub(crate) fn config_environment_value(&self, name: &str) -> Option<&OsStr> { + self.config_environment.value(name) + } + pub(crate) fn output(&self, mut command: GitCommand) -> io::Result { self.revalidate_active_repository_metadata()?; isolate_git_command_environment(&mut command.inner); @@ -137,12 +148,18 @@ impl GitRunner { ) -> Result { authority.ensure_primary_authority()?; let selected = select_git_executable(&authority, search_path)?; + let config_environment = GitConfigEnvironmentSnapshot::capture().map_err(|error| { + GitReadError::InvalidConfigEnvironment { + reason: error.to_string(), + } + })?; Ok(Self { executable: selected.executable, #[cfg(any(unix, test))] argv0: selected.argv0, safe_path: selected.safe_path, authority, + config_environment, }) } } diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index cf6e1b6f46..3d191735b3 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -52,6 +52,95 @@ fn run_git_stdout(cwd: &Path, args: &[&str]) -> String { .to_string() } +#[test] +fn runner_binds_config_environment_across_ambient_mutation() { + const CHILD: &str = "CODEX_GIT_CONFIG_BINDING_CHILD"; + if std::env::var_os(CHILD).is_none() { + let fixture = tempfile::tempdir().expect("fixture"); + let repo = fixture.path().join("repo"); + let safe_home = fixture.path().join("safe-home"); + let malicious_home = repo.join("malicious-home"); + std::fs::create_dir_all(&repo).expect("repo"); + std::fs::create_dir_all(&safe_home).expect("safe home"); + std::fs::create_dir_all(&malicious_home).expect("malicious home"); + run_git(&repo, &["init", "-q"]); + std::fs::write( + safe_home.join(".gitconfig"), + "[codex]\n\tbound = safe-home\n", + ) + .expect("safe home config"); + let malicious_global = repo.join("malicious.gitconfig"); + std::fs::write(&malicious_global, "[codex]\n\tbound = malicious\n") + .expect("malicious global config"); + std::fs::write( + malicious_home.join(".gitconfig"), + "[codex]\n\tbound = malicious-home\n", + ) + .expect("malicious home config"); + + let mut command = Command::new(std::env::current_exe().expect("test executable")); + isolate_git_command_environment(&mut command); + let output = command + .arg("git_command::tests::runner_binds_config_environment_across_ambient_mutation") + .arg("--exact") + .arg("--nocapture") + .env(CHILD, "1") + .env("RUST_TEST_THREADS", "1") + .env("CODEX_GIT_CONFIG_BINDING_ROOT", &repo) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS", &malicious_global) + .env("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME", &malicious_home) + .env("HOME", &safe_home) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "codex.count") + .env("GIT_CONFIG_VALUE_0", "safe-count") + .env_remove("GIT_CONFIG_GLOBAL") + .env_remove("GIT_CONFIG_SYSTEM") + .env_remove("GIT_CONFIG_PARAMETERS") + .output() + .expect("isolated config binding test"); + assert!( + output.status.success(), + "isolated config binding test failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + return; + } + + let root = PathBuf::from(std::env::var_os("CODEX_GIT_CONFIG_BINDING_ROOT").expect("root")); + let malicious = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS").expect("malicious config"); + let malicious_home = + std::env::var_os("CODEX_GIT_CONFIG_BINDING_MALICIOUS_HOME").expect("malicious home"); + let runner = GitRunner::for_cwd_io(&root).expect("runner with captured config environment"); + + // SAFETY: the parent starts an isolated test process that runs only this + // exact test with one harness thread. No other application thread reads or + // writes these variables, and the process exits after the assertions. + unsafe { + std::env::set_var("GIT_CONFIG_GLOBAL", malicious); + std::env::set_var("HOME", malicious_home); + std::env::set_var("GIT_CONFIG_VALUE_0", "malicious-count"); + std::env::set_var( + "GIT_CONFIG_PARAMETERS", + "'codex.count'='malicious-parameter'", + ); + } + + for (key, expected) in [("codex.bound", "safe-home"), ("codex.count", "safe-count")] { + let mut command = runner.command_for_cwd(&root).expect("bound command"); + command.args(["config", "--get", key]); + let output = runner.output(command).expect("bound config read"); + assert!( + output.status.success(), + "{key}: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), expected); + } +} + fn commit_all(cwd: &Path, message: &str) { run_git( cwd, @@ -505,6 +594,12 @@ fn git_read_error_io_kind_table_is_exhaustive() { }, io::ErrorKind::InvalidData, ), + ( + GitReadError::InvalidConfigEnvironment { + reason: "malformed GIT_CONFIG_COUNT".to_string(), + }, + io::ErrorKind::InvalidData, + ), ] { assert_eq!(error.io_kind(), expected, "{error}"); assert_eq!(error.into_io_error().kind(), expected); diff --git a/codex-rs/git-utils/src/git_config_environment.rs b/codex-rs/git-utils/src/git_config_environment.rs new file mode 100644 index 0000000000..43d80a3afd --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment.rs @@ -0,0 +1,136 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::ffi::OsString; +use std::io; +use std::process::Command; + +const FIXED_CONFIG_ENVIRONMENT_KEYS: &[&str] = &[ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "GIT_CONFIG_NOSYSTEM", + "GIT_CONFIG_COUNT", + "GIT_CONFIG_PARAMETERS", + "HOME", + "XDG_CONFIG_HOME", + #[cfg(windows)] + "APPDATA", + #[cfg(windows)] + "PROGRAMDATA", + #[cfg(windows)] + "USERPROFILE", + #[cfg(windows)] + "HOMEDRIVE", + #[cfg(windows)] + "HOMEPATH", +]; + +// Git config command entries are an untrusted process input. Keep capture +// bounded rather than allocating attacker-selected amounts of memory before +// Git gets a chance to reject an unreasonable count. +const MAX_CONFIG_ENVIRONMENT_ENTRIES: usize = 1024; + +/// Exact config-relevant process environment bound to one +/// [`GitRunner`](crate::git_command::GitRunner). +/// +/// Every ordinary Git child receives these captured values (including explicit +/// removals for variables that were absent). Commands that intentionally +/// isolate a config source may override them after command construction. +pub(crate) struct GitConfigEnvironmentSnapshot { + entries: Box<[(OsString, Option)]>, +} + +impl std::fmt::Debug for GitConfigEnvironmentSnapshot { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("GitConfigEnvironmentSnapshot") + .field("captured_entries", &self.entries.len()) + .finish() + } +} + +impl GitConfigEnvironmentSnapshot { + pub(crate) fn capture() -> io::Result { + let environment = std::env::vars_os().collect::>(); + Self::capture_from(|name| captured_environment_value(&environment, name)) + } + + fn capture_from(mut value_for: impl FnMut(&OsStr) -> Option) -> io::Result { + let mut entries = FIXED_CONFIG_ENVIRONMENT_KEYS + .iter() + .map(|name| (OsString::from(name), value_for(OsStr::new(name)))) + .collect::>(); + + if let Some(raw_count) = value_for(OsStr::new("GIT_CONFIG_COUNT")) { + let count = raw_count + .to_str() + .ok_or_else(|| invalid_environment("non-UTF-8 GIT_CONFIG_COUNT"))? + .parse::() + .map_err(|_| invalid_environment("invalid GIT_CONFIG_COUNT"))?; + if count > MAX_CONFIG_ENVIRONMENT_ENTRIES { + return Err(invalid_environment( + "GIT_CONFIG_COUNT exceeds the supported safety bound", + )); + } + for index in 0..count { + for prefix in ["GIT_CONFIG_KEY_", "GIT_CONFIG_VALUE_"] { + let name = OsString::from(format!("{prefix}{index}")); + let value = value_for(&name); + entries.push((name, value)); + } + } + } + + Ok(Self { + entries: entries.into_boxed_slice(), + }) + } + + pub(crate) fn apply_to(&self, command: &mut Command) { + for (name, value) in &self.entries { + match value { + Some(value) => { + command.env(name, value); + } + None => { + command.env_remove(name); + } + } + } + } + + pub(crate) fn value(&self, name: &str) -> Option<&OsStr> { + self.entries + .iter() + .find_map(|(candidate, value)| (candidate == name).then_some(value.as_deref())) + .flatten() + } +} + +#[cfg(not(windows))] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.get(name).cloned() +} + +#[cfg(windows)] +fn captured_environment_value( + environment: &BTreeMap, + name: &OsStr, +) -> Option { + environment.iter().find_map(|(candidate, value)| { + candidate + .to_string_lossy() + .eq_ignore_ascii_case(&name.to_string_lossy()) + .then(|| value.clone()) + }) +} + +fn invalid_environment(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} + +#[cfg(test)] +#[path = "git_config_environment_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_environment_tests.rs b/codex-rs/git-utils/src/git_config_environment_tests.rs new file mode 100644 index 0000000000..3cdec9a790 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_environment_tests.rs @@ -0,0 +1,126 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn snapshot_binds_present_empty_and_absent_fixed_and_indexed_values() { + let values = BTreeMap::from([ + (OsString::from("GIT_CONFIG_GLOBAL"), OsString::from("")), + ( + OsString::from("GIT_CONFIG_PARAMETERS"), + OsString::from("'safe.parameter'='present'"), + ), + (OsString::from("GIT_CONFIG_NOSYSTEM"), OsString::from("1")), + (OsString::from("HOME"), OsString::from("/safe/home")), + ( + OsString::from("XDG_CONFIG_HOME"), + OsString::from("/safe/xdg"), + ), + (OsString::from("GIT_CONFIG_COUNT"), OsString::from("2")), + ( + OsString::from("GIT_CONFIG_KEY_0"), + OsString::from("safe.one"), + ), + ( + OsString::from("GIT_CONFIG_VALUE_0"), + OsString::from("present"), + ), + ( + OsString::from("GIT_CONFIG_KEY_1"), + OsString::from("safe.two"), + ), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| values.get(name).cloned()) + .expect("capture environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(OsStr::new(""))); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new("/safe/home"))); + assert_eq!(snapshot.value("GIT_CONFIG_SYSTEM"), None); + assert_eq!(snapshot.value("GIT_CONFIG_NOSYSTEM"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_PARAMETERS"), + Some(OsStr::new("'safe.parameter'='present'")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.one")) + ); + assert_eq!(snapshot.value("GIT_CONFIG_VALUE_1"), None); + + let mut command = Command::new("git"); + command + .env("GIT_CONFIG_SYSTEM", "/later/system") + .env("GIT_CONFIG_VALUE_1", "later"); + snapshot.apply_to(&mut command); + let child_environment = command + .get_envs() + .map(|(name, value)| (name.to_owned(), value.map(OsStr::to_owned))) + .collect::>(); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_SYSTEM")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("GIT_CONFIG_VALUE_1")), + Some(&None) + ); + assert_eq!( + child_environment.get(OsStr::new("HOME")), + Some(&Some(OsString::from("/safe/home"))) + ); +} + +#[test] +fn snapshot_rejects_malformed_or_unbounded_command_config_count() { + for count in [OsString::from("invalid"), OsString::from("1025")] { + let error = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_COUNT").then(|| count.clone()) + }) + .expect_err("reject count"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + } +} + +#[cfg(unix)] +#[test] +fn snapshot_preserves_non_utf8_config_path_bytes() { + use std::os::unix::ffi::OsStringExt; + + let raw = OsString::from_vec(vec![b'/', b't', b'm', b'p', b'/', 0xff]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + (name == "GIT_CONFIG_GLOBAL").then(|| raw.clone()) + }) + .expect("capture non-UTF-8 path"); + assert_eq!(snapshot.value("GIT_CONFIG_GLOBAL"), Some(raw.as_os_str())); +} + +#[cfg(windows)] +#[test] +fn snapshot_matches_mixed_case_windows_environment_names() { + let environment = BTreeMap::from([ + (OsString::from("git_config_count"), OsString::from("1")), + ( + OsString::from("Git_Config_Key_0"), + OsString::from("safe.key"), + ), + ( + OsString::from("gIt_CoNfIg_VaLuE_0"), + OsString::from("safe-value"), + ), + (OsString::from("Home"), OsString::from(r"C:\safe-home")), + ]); + let snapshot = GitConfigEnvironmentSnapshot::capture_from(|name| { + captured_environment_value(&environment, name) + }) + .expect("capture mixed-case Windows environment"); + + assert_eq!(snapshot.value("GIT_CONFIG_COUNT"), Some(OsStr::new("1"))); + assert_eq!( + snapshot.value("GIT_CONFIG_KEY_0"), + Some(OsStr::new("safe.key")) + ); + assert_eq!( + snapshot.value("GIT_CONFIG_VALUE_0"), + Some(OsStr::new("safe-value")) + ); + assert_eq!(snapshot.value("HOME"), Some(OsStr::new(r"C:\safe-home"))); +} diff --git a/codex-rs/git-utils/src/git_config_sources.rs b/codex-rs/git-utils/src/git_config_sources.rs index f602644151..bcfec7bdf1 100644 --- a/codex-rs/git-utils/src/git_config_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources.rs @@ -36,7 +36,7 @@ pub(crate) fn ensure_no_worktree_config_sources( // Environment, HOME, and XDG paths are attacker-controlled byte strings. // Classify their exact OsString spelling before `git var` can open or // newline-delimit one of them. - for (description, candidate) in legacy_primary_config_source_candidates()? { + for (description, candidate) in legacy_primary_config_source_candidates(git)? { if !is_disabled_primary_config_path(&candidate) { reject_source(git, &git_root, &candidate, description)?; } diff --git a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs index ed4866e510..31cea3941f 100644 --- a/codex-rs/git-utils/src/git_config_sources/primary_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs @@ -1,3 +1,4 @@ +use std::ffi::OsStr; use std::io; use std::path::Path; use std::path::PathBuf; @@ -11,7 +12,9 @@ pub(super) fn default_system_config_source_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(Vec::new()); } // `GIT_CONFIG_SYSTEM` was added to `git var` in Git 2.42. The PSEC-4394 @@ -33,16 +36,19 @@ pub(super) fn selected_git_home_config_candidates( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if std::env::var_os("GIT_CONFIG_GLOBAL").is_some() { + if git.config_environment_value("GIT_CONFIG_GLOBAL").is_some() { return Ok(Vec::new()); } #[cfg(not(windows))] - if std::env::var_os("HOME").is_none() { + if git.config_environment_value("HOME").is_none() { return Ok(Vec::new()); } let dot_gitconfig = selected_git_path_candidate(git, cwd, "~/.gitconfig")?; let mut candidates = vec![dot_gitconfig.clone()]; - if std::env::var_os("XDG_CONFIG_HOME").is_none_or(|path| path.is_empty()) { + if git + .config_environment_value("XDG_CONFIG_HOME") + .is_none_or(OsStr::is_empty) + { let home = dot_gitconfig .parent() .ok_or_else(|| invalid_config_source("selected Git HOME has no parent"))?; @@ -55,7 +61,9 @@ pub(super) fn selected_git_prefix_system_candidate( git: &GitRunner, cwd: &Path, ) -> io::Result> { - if git_env_bool("GIT_CONFIG_NOSYSTEM")? || std::env::var_os("GIT_CONFIG_SYSTEM").is_some() { + if git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? + || git.config_environment_value("GIT_CONFIG_SYSTEM").is_some() + { return Ok(None); } selected_git_path_candidate(git, cwd, "%(prefix)/etc/gitconfig").map(Some) @@ -149,24 +157,25 @@ fn parse_git_var_paths(output: &[u8]) -> io::Result> { .collect() } -pub(super) fn legacy_primary_config_source_candidates() -> io::Result> -{ +pub(super) fn legacy_primary_config_source_candidates( + git: &GitRunner, +) -> io::Result> { let mut candidates = Vec::new(); - match std::env::var_os("GIT_CONFIG_GLOBAL") { + match git.config_environment_value("GIT_CONFIG_GLOBAL") { Some(path) if !path.is_empty() => { candidates.push(("GIT_CONFIG_GLOBAL", PathBuf::from(path))); } Some(_) => {} None => { - let homes = git_home_directories(); - match std::env::var_os("XDG_CONFIG_HOME") { + let homes = git_home_directories(git); + match git.config_environment_value("XDG_CONFIG_HOME") { Some(xdg) if !xdg.is_empty() => candidates.push(( "XDG_CONFIG_HOME Git config", PathBuf::from(xdg).join("git/config"), )), _ => { #[cfg(windows)] - if let Some(app_data) = std::env::var_os("APPDATA") + if let Some(app_data) = git.config_environment_value("APPDATA") && !app_data.is_empty() { candidates.push(( @@ -187,15 +196,15 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result { candidates.push(("GIT_CONFIG_SYSTEM", PathBuf::from(path))); } Some(_) => {} None => { #[cfg(windows)] - if let Some(program_data) = std::env::var_os("PROGRAMDATA") + if let Some(program_data) = git.config_environment_value("PROGRAMDATA") && !program_data.is_empty() { candidates.push(( @@ -209,24 +218,25 @@ pub(super) fn legacy_primary_config_source_candidates() -> io::Result Vec { - if let Some(home) = std::env::var_os("HOME") { - return vec![home]; +fn git_home_directories(git: &GitRunner) -> Vec { + if let Some(home) = git.config_environment_value("HOME") { + return vec![home.to_owned()]; } #[cfg(windows)] { let mut homes = Vec::new(); - if let (Some(drive), Some(path)) = - (std::env::var_os("HOMEDRIVE"), std::env::var_os("HOMEPATH")) - { - let mut home = drive; + if let (Some(drive), Some(path)) = ( + git.config_environment_value("HOMEDRIVE"), + git.config_environment_value("HOMEPATH"), + ) { + let mut home = drive.to_owned(); home.push(path); homes.push(home); } - if let Some(profile) = std::env::var_os("USERPROFILE") - && !homes.contains(&profile) + if let Some(profile) = git.config_environment_value("USERPROFILE") + && !homes.iter().any(|home| home == profile) { - homes.push(profile); + homes.push(profile.to_owned()); } homes } @@ -242,8 +252,8 @@ fn home_config_path(home: &std::ffi::OsStr, suffix: &str) -> PathBuf { } } -fn git_env_bool(name: &str) -> io::Result { - let Some(value) = std::env::var_os(name) else { +fn git_env_bool(git: &GitRunner, name: &str) -> io::Result { + let Some(value) = git.config_environment_value(name) else { return Ok(false); }; let value = value diff --git a/codex-rs/git-utils/src/guarded_config.rs b/codex-rs/git-utils/src/guarded_config.rs new file mode 100644 index 0000000000..ac8e3d5321 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -0,0 +1,434 @@ +use std::collections::BTreeMap; +use std::ffi::OsStr; +use std::io; +use std::path::Path; +use std::path::PathBuf; +use std::process::Stdio; +use std::sync::Arc; + +use crate::FsmonitorOverride; +use crate::git_command::GitCommand; +use crate::git_command::GitRunner; +use crate::git_config::GitConfigEntry; +use crate::git_config::read_effective_config_with_fallback; +use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::safe_git::DISABLED_HOOKS_PATH; +use crate::safe_git::ExecutableFilterDrivers; +use crate::safe_git::FilterPolicySnapshot; +use crate::safe_git::build_filter_policy_snapshot; + +/// Proof that one exact Git config invocation has no worktree-controlled +/// source routes for one runner and canonical repository root. +/// +/// The capability deliberately owns the ordered base arguments and cannot be +/// cloned or rebound to another runner, root, or command environment. +pub(crate) struct ValidatedConfigSources<'git> { + git: &'git GitRunner, + canonical_root: PathBuf, + base_config_args: Box<[String]>, +} + +impl<'git> ValidatedConfigSources<'git> { + fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + #[cfg(test)] + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(count.get() + 1)); + + validate_base_config_args(&base_config_args)?; + let canonical_root = std::fs::canonicalize(canonical_root)?; + git.ensure_active_worktree_root(&canonical_root)?; + ensure_no_worktree_config_sources(git, &canonical_root, &base_config_args)?; + Ok(Self { + git, + canonical_root, + base_config_args: base_config_args.into_boxed_slice(), + }) + } + + fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + read_effective_config_with_fallback( + self.git, + &self.canonical_root, + &self.base_config_args, + pattern, + probe, + ) + } +} + +fn validate_base_config_args(args: &[String]) -> io::Result<()> { + let mut pairs = args.chunks_exact(2); + for pair in &mut pairs { + let Some((key, _value)) = pair[1].split_once('=') else { + return Err(invalid_base_config_args()); + }; + if pair[0] != "-c" || key.is_empty() { + return Err(invalid_base_config_args()); + } + } + if !pairs.remainder().is_empty() { + return Err(invalid_base_config_args()); + } + Ok(()) +} + +fn invalid_base_config_args() -> io::Error { + io::Error::new( + io::ErrorKind::InvalidInput, + "guarded Git base config must contain only ordered -c key=value pairs", + ) +} + +/// Operation-owned Git configuration capability. +/// +/// All operation children are rooted at the authorized repository, inherit +/// the exact frozen base invocation, receive fixed library safety scalars, and +/// retain any sealed filter override for the capability lifetime. +pub(crate) struct GuardedGitConfig<'git> { + sources: ValidatedConfigSources<'git>, + identity: Arc, + // Ordered, typed snapshots keep each sealed filter overlay alive through + // every later child. Downstream staging may attach a fresh Git-add policy + // without rebuilding or weakening the source authorization. + filters: Vec, +} + +struct CapabilityIdentity; + +#[derive(Clone, Copy)] +enum BoundSubcommand { + AddLiteralPathspecs, + Apply, + CheckAttr, + HashObject, + RevParse, +} + +impl BoundSubcommand { + fn as_str(self) -> &'static str { + match self { + Self::AddLiteralPathspecs => "add", + Self::Apply => "apply", + Self::CheckAttr => "check-attr", + Self::HashObject => "hash-object", + Self::RevParse => "rev-parse", + } + } + + fn append_to(self, command: &mut GitCommand) { + if matches!(self, Self::AddLiteralPathspecs) { + command.arg("--literal-pathspecs"); + } + command.arg(self.as_str()); + } +} + +/// A sealed filter-only include owned for the complete capability lifetime. +/// Its fields and include argument are intentionally inaccessible outside this +/// module; callers can neither forge one nor append it to another command. +pub(crate) struct SealedFilterConfigOverride { + owner: Arc, + include_arg: String, + _config_dir: tempfile::TempDir, +} + +impl SealedFilterConfigOverride { + fn append_to( + &self, + owner: &Arc, + command: &mut GitCommand, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + command.args(["-c", &self.include_arg]); + Ok(()) + } + + fn append_rendered_args( + &self, + owner: &Arc, + args: &mut Vec, + ) -> io::Result<()> { + self.ensure_owner(owner)?; + args.push("-c".to_string()); + args.push(self.include_arg.clone()); + Ok(()) + } + + fn ensure_owner(&self, owner: &Arc) -> io::Result<()> { + if !Arc::ptr_eq(&self.owner, owner) { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "sealed Git filter override belongs to another operation", + )); + } + Ok(()) + } +} + +/// A command whose runner, root, config invocation, overlay lifetime, and +/// fixed subcommand are inseparably bound to one operation capability. +pub(crate) struct GuardedGitCommand<'operation, 'git> { + operation: &'operation GuardedGitConfig<'git>, + inner: GitCommand, +} + +impl GuardedGitCommand<'_, '_> { + pub(crate) fn arg(&mut self, arg: impl AsRef) -> &mut Self { + self.inner.arg(arg); + self + } + + pub(crate) fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + self.inner.args(args); + self + } + + pub(crate) fn disable_optional_locks(&mut self) -> &mut Self { + self.inner.env("GIT_OPTIONAL_LOCKS", "0"); + self + } + + pub(crate) fn stdin(&mut self, config: impl Into) -> &mut Self { + self.inner.stdin(config); + self + } + + pub(crate) fn output(self) -> io::Result { + self.operation.sources.git.output(self.inner) + } +} + +impl<'git> GuardedGitConfig<'git> { + pub(crate) fn authorize( + git: &'git GitRunner, + canonical_root: &Path, + base_config_args: Vec, + ) -> io::Result { + Ok(Self { + sources: ValidatedConfigSources::authorize(git, canonical_root, base_config_args)?, + identity: Arc::new(CapabilityIdentity), + filters: Vec::new(), + }) + } + + pub(crate) fn canonical_root(&self) -> &Path { + &self.sources.canonical_root + } + + pub(crate) fn apply_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::Apply) + } + + pub(crate) fn literal_add_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::AddLiteralPathspecs) + } + + pub(crate) fn rev_parse_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::RevParse) + } + + pub(crate) fn filter_attribute_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::CheckAttr) + } + + fn guarded_command( + &self, + subcommand: BoundSubcommand, + ) -> io::Result> { + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(&self.sources.base_config_args); + append_safe_scalar_overrides(&mut command); + for filter in &self.filters { + if let Some(neutralizer) = filter.neutralizer() { + neutralizer.append_to(&self.identity, &mut command)?; + } + } + subcommand.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + pub(crate) fn command_for_sentinel_filter_probe<'operation>( + &'operation self, + neutralizer: &'operation SealedFilterConfigOverride, + driver: &str, + required: bool, + ) -> io::Result> { + if !matches!(driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(&self.sources.base_config_args); + append_safe_scalar_overrides(&mut command); + for filter in &self.filters { + if let Some(attached) = filter.neutralizer() { + attached.append_to(&self.identity, &mut command)?; + } + } + neutralizer.append_to(&self.identity, &mut command)?; + command.args(["-c", &format!("filter.{driver}.required={required}")]); + BoundSubcommand::HashObject.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + pub(crate) fn read_effective( + &self, + pattern: &str, + probe: &str, + ) -> io::Result> { + self.sources.read_effective(pattern, probe) + } + + fn ensure_owned_config_path(&self, path: &Path, description: &str) -> io::Result<()> { + self.sources + .git + .ensure_config_source_is_not_worktree_controlled(path, description) + } + + pub(crate) fn build_filter_override( + &self, + executable_drivers: &ExecutableFilterDrivers, + ) -> io::Result { + let config_dir = tempfile::tempdir()?; + let config_path = config_dir.path().join("filter-neutralization.gitconfig"); + self.ensure_owned_config_path(&config_path, "owned Git filter neutralization")?; + std::fs::write(&config_path, [])?; + for driver in executable_drivers.iter() { + for name in ["clean", "smudge", "process"] { + self.write_filter_override_value(&config_path, driver, name, "")?; + } + self.write_filter_override_value(&config_path, driver, "required", "false")?; + } + let config_path = config_path.to_str().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidData, "non-UTF-8 filter guard path") + })?; + Ok(SealedFilterConfigOverride { + owner: Arc::clone(&self.identity), + include_arg: format!("include.path={config_path}"), + _config_dir: config_dir, + }) + } + + fn write_filter_override_value( + &self, + config_path: &Path, + driver: &str, + name: &str, + value: &str, + ) -> io::Result<()> { + debug_assert!(matches!(name, "clean" | "smudge" | "process" | "required")); + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command + .args(&self.sources.base_config_args) + .args(["config", "--file"]) + .arg(config_path) + .args(["--add", &format!("filter.{driver}.{name}"), value]); + let output = self.sources.git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to write Git filter neutralization for {driver:?} (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + Ok(()) + } + + pub(crate) fn authorize_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { + let filter = build_filter_policy_snapshot(self, paths)?; + self.filters.push(filter); + Ok(()) + } + + pub(crate) fn render_command_for_log(&self, args: &[String]) -> io::Result { + let mut parts = vec!["git".to_string()]; + parts.extend(self.sources.base_config_args.iter().cloned()); + parts.extend(safe_scalar_override_args()); + for filter in &self.filters { + if let Some(neutralizer) = filter.neutralizer() { + neutralizer.append_rendered_args(&self.identity, &mut parts)?; + } + } + parts.extend(args.iter().cloned()); + Ok(format!( + "(cd {} && {})", + quote_shell(&self.sources.canonical_root.display().to_string()), + parts + .into_iter() + .map(|part| quote_shell(&part)) + .collect::>() + .join(" ") + )) + } +} + +fn append_safe_scalar_overrides(command: &mut GitCommand) { + command.args(safe_scalar_override_args()); +} + +fn safe_scalar_override_args() -> [String; 4] { + [ + "-c".to_string(), + format!("core.hooksPath={DISABLED_HOOKS_PATH}"), + "-c".to_string(), + FsmonitorOverride::Disabled.git_config_arg().to_string(), + ] +} + +fn quote_shell(value: &str) -> String { + let simple = value + .chars() + .all(|character| character.is_ascii_alphanumeric() || "-_.:/@%+".contains(character)); + if simple { + value.to_string() + } else { + format!("'{}'", value.replace('\'', "'\\''")) + } +} + +#[cfg(test)] +thread_local! { + static CONFIG_SOURCE_AUTHORIZATION_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_config_source_authorization_count() { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn config_source_authorization_count() -> usize { + CONFIG_SOURCE_AUTHORIZATION_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +#[path = "guarded_config_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/guarded_config_tests.rs b/codex-rs/git-utils/src/guarded_config_tests.rs new file mode 100644 index 0000000000..70fdad0a79 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -0,0 +1,116 @@ +use super::*; +use crate::safe_git::isolate_git_command_environment; +use pretty_assertions::assert_eq; + +fn run_git(cwd: &Path, args: &[&str]) { + let mut command = std::process::Command::new("git"); + isolate_git_command_environment(&mut command); + let output = command.current_dir(cwd).args(args).output().expect("Git"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn attached_filter_snapshots_retain_multiple_owned_overrides() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + run_git( + repo.path(), + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + config + .authorize_filter_paths(&["first.txt".to_string()]) + .expect("first filter snapshot"); + config + .authorize_filter_paths(&["second.txt".to_string()]) + .expect("second filter snapshot"); + + assert_eq!(config.filters.len(), 2); + assert!( + config + .filters + .iter() + .all(|filter| filter.neutralizer().is_some()) + ); +} + +#[test] +fn authorization_rejects_untyped_global_config_arguments() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + + for args in [ + vec!["--config-env=include.path=UNSNAPSHOTTED".to_string()], + vec!["--git-dir=elsewhere".to_string()], + vec!["-c".to_string(), "missing-value".to_string()], + vec!["-c".to_string()], + ] { + let error = match GuardedGitConfig::authorize(&git, repo.path(), args) { + Ok(_) => panic!("accepted untyped base config arguments"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } +} + +#[test] +fn authorization_rejects_another_runner_repository_and_nested_or_prefix_roots() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("repo"); + let nested = outer.join("nested"); + let prefix = fixture.path().join("repo-evil"); + std::fs::create_dir_all(&nested).expect("nested"); + std::fs::create_dir_all(&prefix).expect("prefix"); + run_git(&outer, &["init", "-q"]); + run_git(&nested, &["init", "-q"]); + run_git(&prefix, &["init", "-q"]); + let git = GitRunner::for_cwd_io(&outer).expect("outer runner"); + + for wrong_root in [&nested, &prefix] { + let error = match GuardedGitConfig::authorize(&git, wrong_root, Vec::new()) { + Ok(_) => panic!("accepted another repository root"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + } +} + +#[test] +fn sealed_filter_override_cannot_cross_operation_capabilities() { + let fixture = tempfile::tempdir().expect("fixture"); + let first_root = fixture.path().join("first"); + let second_root = fixture.path().join("second"); + std::fs::create_dir_all(&first_root).expect("first"); + std::fs::create_dir_all(&second_root).expect("second"); + run_git(&first_root, &["init", "-q"]); + run_git(&second_root, &["init", "-q"]); + run_git( + &first_root, + &["config", "filter.demo.clean", "git hash-object --stdin"], + ); + let first_git = GitRunner::for_cwd_io(&first_root).expect("first runner"); + let second_git = GitRunner::for_cwd_io(&second_root).expect("second runner"); + let mut first = + GuardedGitConfig::authorize(&first_git, &first_root, Vec::new()).expect("first capability"); + let second = GuardedGitConfig::authorize(&second_git, &second_root, Vec::new()) + .expect("second capability"); + first + .authorize_filter_paths(&["unselected.txt".to_string()]) + .expect("first filter policy"); + let sealed = first.filters[0].neutralizer().expect("first override"); + + let error = + match second.command_for_sentinel_filter_probe(sealed, "set", /*required*/ true) { + Ok(_) => panic!("accepted another operation's sealed override"), + Err(error) => error, + }; + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index b63bddd518..0377521aef 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -6,8 +6,10 @@ mod errors; mod fsmonitor; mod git_command; mod git_config; +mod git_config_environment; mod git_config_sources; mod git_executable; +mod guarded_config; mod info; mod local_only; mod operations; diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index b42defd914..3bab04c74a 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -10,6 +10,18 @@ use crate::apply::write_temp_patch; use crate::git_command::GitRunner; use crate::git_config::path_is_within; use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; + +/// Extract effective patch paths through a bound operation configuration. +pub(crate) fn extract_effective_paths_from_patch_guarded( + config: &GuardedGitConfig<'_>, + patch_path: &Path, + revert: bool, +) -> io::Result> { + let forward_paths = git_apply_numstat_paths_guarded(config, patch_path, revert)?; + let reverse_paths = git_apply_numstat_paths_guarded(config, patch_path, !revert)?; + normalize_effective_patch_paths(forward_paths, reverse_paths) +} /// Extract paths with Git from a cwd whose config sources have already been /// authorized for `git_config_args`. @@ -26,6 +38,13 @@ pub(crate) fn extract_effective_paths_from_patch( // opposite orientation too so both endpoints are included in the result. let reverse_paths = git_apply_numstat_paths(git, authorized_cwd, patch_path, !revert, git_config_args)?; + normalize_effective_patch_paths(forward_paths, reverse_paths) +} + +fn normalize_effective_patch_paths( + forward_paths: Vec, + reverse_paths: Vec, +) -> io::Result> { if forward_paths.len() != reverse_paths.len() { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -46,6 +65,30 @@ pub(crate) fn extract_effective_paths_from_patch( .collect() } +fn git_apply_numstat_paths_guarded( + config: &GuardedGitConfig<'_>, + patch_path: &Path, + revert: bool, +) -> io::Result> { + let mut command = config.apply_command()?; + command.args(["--numstat", "-z"]); + if revert { + command.arg("-R"); + } + command.arg("--").arg(patch_path); + let output = command.output()?; + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "failed to parse patch paths: {}", + String::from_utf8_lossy(&output.stderr).trim() + ), + )); + } + parse_numstat_paths(&output.stdout) +} + /// Best-effort extraction of the paths Git would apply. /// /// Security-sensitive callers must use the fallible internal extractor so an @@ -291,6 +334,37 @@ pub(crate) fn stage_effective_paths( Ok(()) } +/// Preserve the existing reverse staging behavior while constructing every +/// Git child from the operation-owned configuration capability. +pub(crate) fn stage_effective_paths_guarded( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result<()> { + let confined = confine_patch_paths_guarded(config, paths)?; + let mut existing = Vec::new(); + for path in confined.into_exact_leaves()? { + let joined = config.canonical_root().join(&path); + if let Ok(metadata) = std::fs::symlink_metadata(&joined) { + if leaf_is_traversable_directory(metadata.file_type()) { + return Err(containment_error( + "refusing to recursively stage a directory patch path", + )); + } + existing.push(path); + } + } + if existing.is_empty() { + return Ok(()); + } + let mut args = vec!["--".to_string()]; + args.extend(existing); + let mut command = config.literal_add_command()?; + command.args(&args); + let _output = command.output()?; + // Preserve the existing best-effort staging contract on non-zero status. + Ok(()) +} + #[cfg(not(windows))] fn leaf_is_traversable_directory(file_type: std::fs::FileType) -> bool { file_type.is_dir() @@ -379,6 +453,25 @@ pub(crate) fn confine_patch_paths( git: &GitRunner, git_root: &Path, paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(git_root)?; + let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; + confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) +} + +fn confine_patch_paths_guarded( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(config.canonical_root())?; + let metadata_dirs = canonical_git_metadata_dirs_guarded(config)?; + confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) +} + +fn confine_patch_paths_with_metadata( + canonical_root: &Path, + paths: &[String], + metadata_dirs: &[PathBuf], ) -> io::Result { if paths.is_empty() { return Ok(ConfinedPatchPaths { @@ -401,8 +494,6 @@ pub(crate) fn confine_patch_paths( }); } - let canonical_root = std::fs::canonicalize(git_root)?; - let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; let mut entries = Vec::with_capacity(paths.len()); let mut prefix_cache = std::collections::BTreeMap::new(); @@ -416,9 +507,9 @@ pub(crate) fn confine_patch_paths( ); let (existing_len, mut projected) = longest_existing_strict_prefix( - &canonical_root, + canonical_root, &components, - &metadata_dirs, + metadata_dirs, &mut prefix_cache, )?; projected.extend( @@ -546,6 +637,42 @@ fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result) -> io::Result> { + let queries = [ + ["rev-parse", "--absolute-git-dir"], + ["rev-parse", "--git-common-dir"], + ]; + let mut metadata_dirs = std::collections::BTreeSet::new(); + for args in queries { + let mut command = config.rev_parse_command()?; + command.args(&args[1..]); + let output = command.output()?; + if !output.status.success() { + return Err(io::Error::other(format!( + "failed to resolve Git repository metadata (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let path = String::from_utf8_lossy(&output.stdout); + let path = path.trim_end_matches(['\r', '\n']); + if path.is_empty() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Git returned an empty repository metadata path", + )); + } + let path = PathBuf::from(path); + let absolute = if path.is_absolute() { + path + } else { + config.canonical_root().join(path) + }; + metadata_dirs.insert(std::fs::canonicalize(absolute)?); + } + Ok(metadata_dirs.into_iter().collect()) +} + fn insert_candidate_prefixes<'a>( components: impl IntoIterator, origin: ConfinedPathOrigin, diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index 05b792d357..cbdd194ce9 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -148,6 +148,23 @@ impl RepositoryAuthority { }) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + let canonical_root = std::fs::canonicalize(root)?; + let same_root = canonical_root == self.active_worktree_root + || same_file::is_same_file(&canonical_root, &self.active_worktree_root)?; + if !same_root { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "guarded Git repository root {} does not match runner repository {}", + canonical_root.display(), + self.active_worktree_root.display() + ), + )); + } + Ok(()) + } + #[cfg(test)] pub(crate) fn from_test_locations( roots: Vec, diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index e7904c001b..dcf9764099 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -3,20 +3,19 @@ use std::collections::BTreeSet; use std::io; use std::io::Seek; use std::io::Write; -use std::path::Path; use std::process::Command; use std::process::Stdio; -use crate::git_command::GitRunner; use crate::git_config::GitConfigEntry; -use crate::git_config::read_effective_config_with_fallback as read_effective_config_unchecked; -use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; +use crate::guarded_config::SealedFilterConfigOverride; #[path = "filter_sentinel.rs"] mod filter_sentinel; pub(crate) use filter_sentinel::SentinelFilterProbeBudget; pub(crate) use filter_sentinel::SentinelFilterProbeResolution; pub(crate) use filter_sentinel::classify_sentinel_filter_probes; +#[cfg(test)] pub(crate) use filter_sentinel::sentinel_filter_probe_config_args; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = @@ -28,21 +27,32 @@ pub(crate) enum FilterAttributeValue { AmbiguousSentinel(String), } -pub(crate) struct GitFilterNeutralization { - git_config_args: Vec, - _config_dir: Option, - filter_config: BTreeMap, -} +/// Complete executable-filter inventory derived from one authorized effective +/// config read. The private field prevents a caller from minting an incomplete +/// inventory and treating its neutralizer as complete. +pub(crate) struct ExecutableFilterDrivers(BTreeSet); -impl GitFilterNeutralization { - pub(crate) fn git_config_args(&self) -> &[String] { - &self.git_config_args +impl ExecutableFilterDrivers { + pub(crate) fn iter(&self) -> impl Iterator { + self.0.iter() } - pub(crate) fn filter_value(&self, driver: &str, name: &str) -> Option<&str> { - self.filter_config - .get(&format!("filter.{driver}.{name}")) - .map(|entry| entry.value.as_str()) + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn contains(&self, driver: &str) -> bool { + self.0.contains(driver) + } +} + +pub(crate) struct FilterPolicySnapshot { + neutralizer: Option, +} + +impl FilterPolicySnapshot { + pub(crate) fn neutralizer(&self) -> Option<&SealedFilterConfigOverride> { + self.neutralizer.as_ref() } } @@ -73,35 +83,22 @@ pub(crate) fn isolate_git_command_environment(command: &mut Command) { } } -pub(crate) fn ensure_no_selected_executable_git_filters( - git: &GitRunner, - cwd: &Path, +pub(crate) fn build_filter_policy_snapshot( + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], -) -> io::Result { - let entries = read_filter_config(git, cwd, git_config_args)?; +) -> io::Result { + let entries = read_filter_config(config)?; let executable_drivers = executable_filter_drivers(&entries)?; if executable_drivers.is_empty() { - return Ok(GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, - }); + return Ok(FilterPolicySnapshot { neutralizer: None }); } - let guard = executable_filter_guard(git, cwd, entries, &executable_drivers)?; + let neutralizer = config.build_filter_override(&executable_drivers)?; let paths = paths .iter() .map(|path| path.as_bytes().to_vec()) .collect::>(); - let attributes = read_filter_attributes( - git, - cwd, - &paths, - git_config_args, - &executable_drivers, - &guard, - )?; - if let Some((driver, path)) = selected_executable_filter(&guard.filter_config, &attributes)? { + let attributes = read_filter_attributes(config, &paths, &executable_drivers, &neutralizer)?; + if let Some((driver, path)) = selected_executable_filter(&entries, &attributes)? { return Err(io::Error::new( io::ErrorKind::Unsupported, format!( @@ -110,105 +107,22 @@ pub(crate) fn ensure_no_selected_executable_git_filters( ), )); } - Ok(guard) -} - -fn executable_filter_guard( - git: &GitRunner, - cwd: &Path, - filter_config: BTreeMap, - executable_drivers: &BTreeSet, -) -> io::Result { - let config_dir = tempfile::tempdir()?; - let config_path = config_dir.path().join("filter-neutralization.gitconfig"); - std::fs::write(&config_path, [])?; - let guard = GitFilterNeutralization { - git_config_args: vec![ - "-c".to_string(), - format!( - "include.path={}", - config_path - .to_str() - .ok_or_else(|| invalid_filter_output("non-UTF-8 filter guard path"))? - ), - ], - _config_dir: Some(config_dir), - filter_config, - }; - for driver in executable_drivers { - debug_assert!(["clean", "smudge", "process"].into_iter().any(|name| { - guard - .filter_value(driver, name) - .is_some_and(|value| !value.is_empty()) - })); - for command in ["clean", "smudge", "process"] { - guard.write_config_value(git, cwd, &config_path, driver, command, "")?; - } - guard.write_config_value(git, cwd, &config_path, driver, "required", "false")?; - } - Ok(guard) -} - -impl GitFilterNeutralization { - fn write_config_value( - &self, - git: &GitRunner, - cwd: &Path, - config_path: &Path, - driver: &str, - name: &str, - value: &str, - ) -> io::Result<()> { - let mut command = git.command_for_cwd(cwd)?; - command.args(["config", "--file"]).arg(config_path).args([ - "--add", - &format!("filter.{driver}.{name}"), - value, - ]); - let output = git.output(command)?; - if !output.status.success() { - return Err(io::Error::other(format!( - "failed to write Git filter neutralization for {driver:?} (status {}): {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - Ok(()) - } + Ok(FilterPolicySnapshot { + neutralizer: Some(neutralizer), + }) } fn read_filter_config( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], + config: &GuardedGitConfig<'_>, ) -> io::Result> { - read_effective_config_with_fallback( - git, - cwd, - git_config_args, - EXECUTABLE_FILTER_CONFIG_PATTERN, - "filter", - ) -} - -pub(crate) fn read_effective_config_with_fallback( - git: &GitRunner, - cwd: &Path, - git_config_args: &[String], - pattern: &str, - probe: &str, -) -> io::Result> { - ensure_no_worktree_config_sources(git, cwd, git_config_args)?; - read_effective_config_unchecked(git, cwd, git_config_args, pattern, probe) + config.read_effective(EXECUTABLE_FILTER_CONFIG_PATTERN, "filter") } fn read_filter_attributes( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, paths: &[Vec], - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { if paths.is_empty() { return Ok(BTreeMap::new()); @@ -217,22 +131,12 @@ fn read_filter_attributes( write_nul_paths(&mut input, paths)?; input.rewind()?; - let mut command = git.command_for_cwd(cwd)?; + let mut command = config.filter_attribute_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - "check-attr", - "--stdin", - "-z", - "filter", - ]) + .disable_optional_locks() + .args(["--stdin", "-z", "filter"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; if !output.status.success() { return Err(io::Error::other(format!( "git filter attribute probe failed with status {}: {}", @@ -241,23 +145,14 @@ fn read_filter_attributes( ))); } let attributes = parse_filter_attributes(&output.stdout, paths)?; - resolve_filter_attribute_sentinels( - git, - cwd, - attributes, - git_config_args, - executable_drivers, - neutralization, - ) + resolve_filter_attribute_sentinels(config, attributes, executable_drivers, neutralization) } fn resolve_filter_attribute_sentinels( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, attributes: BTreeMap, FilterAttributeValue>, - git_config_args: &[String], - executable_drivers: &BTreeSet, - neutralization: &GitFilterNeutralization, + executable_drivers: &ExecutableFilterDrivers, + neutralization: &SealedFilterConfigOverride, ) -> io::Result, String>> { let mut resolved = BTreeMap::new(); let mut probe_budget = SentinelFilterProbeBudget::default(); @@ -269,11 +164,9 @@ fn resolve_filter_attribute_sentinels( FilterAttributeValue::AmbiguousSentinel(driver) => { if executable_drivers.contains(&driver) && sentinel_spelling_selects_filter_driver( - git, - cwd, + config, &path, &driver, - git_config_args, neutralization, &mut probe_budget, )? @@ -289,20 +182,16 @@ fn resolve_filter_attribute_sentinels( /// Disambiguate Git's sentinel spellings with required/optional probes. The /// shared guard blanks every known executable driver before either probe. fn sentinel_spelling_selects_filter_driver( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, path: &[u8], driver: &str, - git_config_args: &[String], - neutralization: &GitFilterNeutralization, + neutralization: &SealedFilterConfigOverride, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { let probe = SentinelSelectionProbe { - git, - cwd, + config, path, driver, - git_config_args, neutralization, }; let required = probe.run(/*required*/ true, probe_budget)?; @@ -328,12 +217,10 @@ fn sentinel_spelling_selects_filter_driver( } struct SentinelSelectionProbe<'a> { - git: &'a GitRunner, - cwd: &'a Path, + config: &'a GuardedGitConfig<'a>, path: &'a [u8], driver: &'a str, - git_config_args: &'a [String], - neutralization: &'a GitFilterNeutralization, + neutralization: &'a SealedFilterConfigOverride, } impl SentinelSelectionProbe<'_> { @@ -342,29 +229,26 @@ impl SentinelSelectionProbe<'_> { required: bool, probe_budget: &mut SentinelFilterProbeBudget, ) -> io::Result { - let probe_config_args = sentinel_filter_probe_config_args( - self.neutralization.git_config_args(), + if !matches!(self.driver, "set" | "unset" | "unspecified") { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "Git filter sentinel probe requested for a non-sentinel driver", + )); + } + let path = git_path_argument(self.path)?; + let mut command = self.config.command_for_sentinel_filter_probe( + self.neutralization, self.driver, required, )?; - let path = git_path_argument(self.path)?; - let mut command = self.git.command_for_cwd(self.cwd)?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(self.git_config_args) - .args([ - "-c", - &format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c", - "core.fsmonitor=false", - ]) - .args(&probe_config_args) - .args(["hash-object", "--stdin"]) + .disable_optional_locks() + .arg("--stdin") .arg("--path") .arg(path) .stdin(Stdio::null()); probe_budget.ensure_probe_available()?; - let output = self.git.output(command)?; + let output = command.output()?; probe_budget.record_completed_probe(); Ok(output) } @@ -397,9 +281,9 @@ fn selected_executable_filter( Ok(None) } -pub(crate) fn executable_filter_drivers( +fn executable_filter_drivers( entries: &BTreeMap, -) -> io::Result> { +) -> io::Result { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { if entry.key.ends_with(".required") { @@ -410,7 +294,7 @@ pub(crate) fn executable_filter_drivers( executable_drivers.insert(driver); } } - Ok(executable_drivers) + Ok(ExecutableFilterDrivers(executable_drivers)) } fn filter_driver_name(key: &str) -> io::Result { diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index b04cd9d6f5..2f45bad4d7 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -1,5 +1,7 @@ use super::*; +use crate::git_command::GitRunner; use crate::git_config::GitConfigScope; +use crate::guarded_config::GuardedGitConfig; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -109,7 +111,7 @@ fn selected_filter_policy_allows_effective_empty_value() { } #[test] -fn filter_snapshot_retains_required_without_treating_it_as_executable() { +fn filter_config_retains_required_without_treating_it_as_executable() { let mut entries = filter_entries( GitConfigScope::Local, Path::new("config"), @@ -123,17 +125,16 @@ fn filter_snapshot_retains_required_without_treating_it_as_executable() { "true", )); assert_eq!( - executable_filter_drivers(&entries).expect("executable drivers"), + executable_filter_drivers(&entries) + .expect("executable drivers") + .0, BTreeSet::from(["demo".to_string()]) ); - let neutralization = GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, - }; assert_eq!( - neutralization.filter_value("demo", "required"), - Some("true") + entries + .get("filter.demo.required") + .map(|entry| entry.value.as_str()), + Some("true"), ); let required_only = filter_entries( @@ -263,9 +264,12 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { let root = repo.path(); configure_marker_filter(root, driver); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let mut config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); std::fs::write(root.join(".gitattributes"), special_rule).expect("write special attribute"); - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]) + config + .authorize_filter_paths(&["file.txt".to_string()]) .expect("allow special attribute state"); assert!(!marker_filter_ran(root), "special {driver}"); @@ -274,8 +278,7 @@ fn sentinel_special_states_and_literal_driver_names_remain_distinct() { format!("file.txt filter={driver}\n"), ) .expect("write literal attribute"); - let result = - ensure_no_selected_executable_git_filters(&git, root, &["file.txt".to_string()], &[]); + let result = config.authorize_filter_paths(&["file.txt".to_string()]); let error = match result { Ok(_) => panic!("accepted literal sentinel-named driver {driver}"), Err(error) => error, @@ -296,9 +299,12 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { .expect("write initial attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let output = run_git_success(root, &["check-attr", "-z", "filter", "--", "file.txt"]); let attributes = parse_filter_attributes(&output.stdout, &[b"file.txt".to_vec()]) @@ -310,10 +316,8 @@ fn sentinel_probe_neutralizes_every_known_driver_after_attribute_swap() { ) .expect("swap attribute after snapshot"); let resolved = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -329,9 +333,12 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { let root = repo.path(); configure_marker_filter(root, "unspecified"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let attributes = (0..=SentinelFilterProbeBudget::max_probes()) .map(|index| { @@ -343,10 +350,8 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { .collect(); let error = resolve_filter_attribute_sentinels( - &git, - root, + &config, attributes, - &[], &executable_drivers, &neutralization, ) @@ -359,36 +364,31 @@ fn high_cardinality_ordinary_sentinels_stop_at_hard_child_probe_budget() { } #[test] -fn unrelated_sentinel_probe_failures_remain_generic_and_fail_closed() { +fn sentinel_probe_rejects_non_sentinel_driver_before_launch() { let repo = init_filter_repo(); let root = repo.path(); configure_marker_filter(root, "set"); std::fs::write(root.join(".gitattributes"), "file.txt filter\n") .expect("write special attribute"); let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - let entries = read_filter_config(&git, root, &[]).expect("filter config"); + let config = + GuardedGitConfig::authorize(&git, root, Vec::new()).expect("authorized config sources"); + let entries = read_filter_config(&config).expect("filter config"); let executable_drivers = executable_filter_drivers(&entries).expect("executable drivers"); - let neutralization = executable_filter_guard(&git, root, entries, &executable_drivers) + let neutralization = config + .build_filter_override(&executable_drivers) .expect("filter neutralization"); let mut budget = SentinelFilterProbeBudget::default(); - let malformed_config = ["-c".to_string(), "=".to_string()]; let error = sentinel_spelling_selects_filter_driver( - &git, - root, + &config, b"file.txt", - "set", - &malformed_config, + "ordinary", &neutralization, &mut budget, ) - .expect_err("malformed config must fail both probes"); - assert_eq!(error.kind(), io::ErrorKind::Other); - assert!( - error - .to_string() - .starts_with("git filter attribute selection probe failed with required status") - ); + .expect_err("reject non-sentinel probe"); + assert_eq!(error.kind(), io::ErrorKind::InvalidInput); assert!(!marker_filter_ran(root)); }