diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index d9c68fc2ae..e79be238a3 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -10,14 +10,15 @@ use std::io; use std::path::Path; use std::path::PathBuf; +#[cfg(test)] 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_from_apply; +#[cfg(test)] 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 +48,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_from_apply(&mut config, &patch_paths)?; } // Build git args @@ -87,8 +86,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 +107,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 +130,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 +144,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, @@ -201,26 +181,33 @@ pub(crate) fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, Pat Ok((dir, path)) } -pub(crate) fn run_git( - git: &GitRunner, - cwd: &Path, - git_cfg: &[String], +fn run_guarded_apply( + config: &GuardedGitConfig<'_>, args: &[String], ) -> io::Result<(i32, String, String)> { - let mut cmd = git.command_for_cwd(cwd)?; - for p in git_cfg { - cmd.arg(p); + 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", + )); } - for a in args { - cmd.arg(a); - } - let out = git.output(cmd)?; - let code = out.status.code().unwrap_or(-1); - let stdout = String::from_utf8_lossy(&out.stdout).into_owned(); - let stderr = String::from_utf8_lossy(&out.stderr).into_owned(); - Ok((code, stdout, stderr)) + 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(), + )) } +#[cfg(test)] pub(crate) fn safe_git_config_parts() -> Vec { vec![ "-c".to_string(), @@ -230,33 +217,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 +228,11 @@ 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 crate::safe_git::filter_policy_overlay_count; + use crate::safe_git::filter_policy_read_count; + use crate::safe_git::reset_filter_policy_counts; use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; @@ -412,7 +377,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 +508,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 +601,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 +615,12 @@ mod tests { revert: true, preflight: false, }; + reset_config_source_authorization_count(); + reset_filter_policy_counts(); let res_revert = apply_git_patch(&revert_req).expect("revert ok"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(filter_policy_overlay_count(), 0); 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 +657,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 +689,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 +709,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 +739,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 +833,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/exact_index_policy.rs b/codex-rs/git-utils/src/exact_index_policy.rs index dd04aa455a..e7c2b6947f 100644 --- a/codex-rs/git-utils/src/exact_index_policy.rs +++ b/codex-rs/git-utils/src/exact_index_policy.rs @@ -6,7 +6,7 @@ use std::io; use std::path::Path; use crate::exact_filesystem_policy::filesystem_spelling_conflicts; -use crate::git_command::GitRunner; +use crate::guarded_config::GuardedGitConfig; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) enum ExactIndexPolicy { @@ -90,16 +90,14 @@ impl<'a> IndexEvidence<'a> { } pub(crate) fn resolve_exact_index_policy( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], content_filter_paths: &[String], - git_config_args: &[String], ) -> io::Result { - let output = read_index(git, git_root, git_config_args)?; + let output = read_index(config)?; let entries = parse_index_entries(&output)?; - let ignore_case = - effective_git_bool(git, git_root, git_config_args, "core.ignoreCase")?.unwrap_or(false); + let ignore_case = config.read_bool("core.ignoreCase")?.unwrap_or(false); + let git_root = config.canonical_root(); let (paths, content_filter_paths) = if ignore_case { let mapped = match map_ignore_case_paths(git_root, &entries, paths, content_filter_paths) { @@ -171,15 +169,14 @@ pub(crate) fn resolve_exact_index_policy( }) } -fn read_index(git: &GitRunner, git_root: &Path, git_config_args: &[String]) -> io::Result> { +fn read_index(config: &GuardedGitConfig<'_>) -> io::Result> { // One full, byte-oriented scan avoids invoking Git once per patch path and // lets one parse enforce aliases, directory prefixes, and flags together. - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.ls_files_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["ls-files", "--cached", "-v", "-z"]); - let output = git.output(command)?; + .disable_optional_locks() + .args(["--cached", "-v", "-z"]); + let output = command.output()?; if !output.status.success() { return Err(io::Error::other(format!( "git exact-index probe failed with status {}: {}", @@ -423,38 +420,6 @@ fn ascii_fold(value: &[u8]) -> Vec { value.iter().map(u8::to_ascii_lowercase).collect() } -pub(crate) fn effective_git_bool( - git: &GitRunner, - git_root: &Path, - git_config_args: &[String], - key: &str, -) -> io::Result> { - let mut command = git.command_for_cwd(git_root)?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["config", "--type=bool", "--get", key]); - let output = git.output(command)?; - if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { - return Ok(None); - } - if !output.status.success() { - return Err(io::Error::other(format!( - "git boolean config probe for {key} failed with status {}: {}", - output.status, - String::from_utf8_lossy(&output.stderr).trim() - ))); - } - match String::from_utf8_lossy(&output.stdout).trim() { - "true" => Ok(Some(true)), - "false" => Ok(Some(false)), - value => Err(io::Error::new( - io::ErrorKind::InvalidData, - format!("unexpected normalized Git boolean value for {key}: {value:?}"), - )), - } -} - fn quote_paths(paths: &BTreeSet) -> String { let shown = paths .iter() diff --git a/codex-rs/git-utils/src/exact_staging.rs b/codex-rs/git-utils/src/exact_staging.rs index 1ae755363a..8ff94ae796 100644 --- a/codex-rs/git-utils/src/exact_staging.rs +++ b/codex-rs/git-utils/src/exact_staging.rs @@ -4,17 +4,12 @@ use std::collections::BTreeSet; use std::io; use std::io::Seek; use std::io::Write; -use std::path::Path; use std::process::Stdio; -use crate::apply::run_git; use crate::exact_index_policy::ExactIndexPolicy; -use crate::exact_index_policy::effective_git_bool; use crate::exact_index_policy::resolve_exact_index_policy; -use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::guarded_config::GuardedGitConfig; use crate::patch_paths::validate_patch_path; -use crate::safe_git::ensure_no_selected_git_add_filters; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct StagePathsResult { @@ -22,6 +17,12 @@ pub(crate) struct StagePathsResult { pub(crate) stderr: String, } +#[derive(Clone, Copy)] +enum StagingProvenance { + FromApply, + Standalone, +} + /// Stage only the literal paths supplied by a caller that has already applied /// the operation-specific containment and filesystem policy. /// @@ -35,19 +36,38 @@ pub(crate) struct StagePathsResult { /// bytes read for an already-confined index pathname. It cannot make this /// exact command recurse into, or create index entries for, descendant paths; /// the filter neutralizer also remains in force for the entire command. -pub(crate) fn update_index_exact_paths( - git: &GitRunner, - git_root: &Path, +pub(crate) fn update_index_exact_paths_from_apply( + config: &mut GuardedGitConfig<'_>, paths: &[String], content_filter_paths: &[String], - git_config_args: &[String], ) -> io::Result { - if paths.is_empty() { - return Ok(StagePathsResult { - exit_code: 0, - stderr: String::new(), - }); - } + update_index_exact_paths_common( + config, + paths, + content_filter_paths, + StagingProvenance::FromApply, + ) +} + +pub(crate) fn update_index_exact_paths_standalone( + config: &mut GuardedGitConfig<'_>, + paths: &[String], + content_filter_paths: &[String], +) -> io::Result { + update_index_exact_paths_common( + config, + paths, + content_filter_paths, + StagingProvenance::Standalone, + ) +} + +fn update_index_exact_paths_common( + config: &mut GuardedGitConfig<'_>, + paths: &[String], + content_filter_paths: &[String], + provenance: StagingProvenance, +) -> io::Result { for path in paths { if path.as_bytes().contains(&0) { return Err(io::Error::new( @@ -67,29 +87,34 @@ pub(crate) fn update_index_exact_paths( "content-filter staging path is not in the exact staging set", )); } + if matches!(provenance, StagingProvenance::FromApply) { + config.ensure_apply_filter_path_subset(content_filter_paths)?; + } + if paths.is_empty() { + return Ok(StagePathsResult { + exit_code: 0, + stderr: String::new(), + }); + } - ensure_no_worktree_config_sources(git, git_root, git_config_args)?; + let (paths, content_filter_paths) = + match resolve_exact_index_policy(config, paths, content_filter_paths)? { + ExactIndexPolicy::Proceed { + paths, + content_filter_paths, + } => (paths, content_filter_paths), + ExactIndexPolicy::Refuse { stderr } => { + return Ok(StagePathsResult { + exit_code: 1, + stderr, + }); + } + }; + if matches!(provenance, StagingProvenance::FromApply) { + config.ensure_apply_filter_path_subset(&content_filter_paths)?; + } - let (paths, content_filter_paths) = match resolve_exact_index_policy( - git, - git_root, - paths, - content_filter_paths, - git_config_args, - )? { - ExactIndexPolicy::Proceed { - paths, - content_filter_paths, - } => (paths, content_filter_paths), - ExactIndexPolicy::Refuse { stderr } => { - return Ok(StagePathsResult { - exit_code: 1, - stderr, - }); - } - }; - - let ignored = ignored_untracked_paths(git, git_root, &paths, git_config_args)?; + let ignored = ignored_untracked_paths(config, &paths)?; if !ignored.is_empty() { return Ok(StagePathsResult { exit_code: 1, @@ -99,32 +124,26 @@ pub(crate) fn update_index_exact_paths( ), }); } - if let Some(result) = sparse_checkout_policy(git, git_root, &paths, git_config_args)? { + if let Some(result) = sparse_checkout_policy(config, &paths)? { return Ok(result); } - let filter_guard = - ensure_no_selected_git_add_filters(git, git_root, &content_filter_paths, git_config_args)?; - let mut guarded_config = git_config_args.to_vec(); - guarded_config.extend_from_slice(filter_guard.git_config_args()); - - let mut update_args = vec![ - "--literal-pathspecs".to_string(), - "update-index".to_string(), - "--add".to_string(), - "--remove".to_string(), - "--".to_string(), - ]; - update_args.extend_from_slice(&paths); - let (exit_code, _stdout, stderr) = run_git(git, git_root, &guarded_config, &update_args)?; - Ok(StagePathsResult { exit_code, stderr }) + config.authorize_git_add_filter_paths(&content_filter_paths)?; + let mut command = config.update_index_literal_pathspecs_command()?; + command + .disable_optional_locks() + .args(["--add", "--remove", "--"]) + .args(&paths); + let output = command.output()?; + Ok(StagePathsResult { + exit_code: output.status.code().unwrap_or(-1), + stderr: String::from_utf8_lossy(&output.stderr).into_owned(), + }) } fn ignored_untracked_paths( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result> { if paths.is_empty() { return Ok(BTreeSet::new()); @@ -138,15 +157,14 @@ fn ignored_untracked_paths( .map(|path| format!("./{path}")) .collect::>(); let input = nul_path_input(&probe_paths)?; - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.check_ignore_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) + .disable_optional_locks() // `check-ignore --stdin` consumes pathnames rather than pathspecs and // rejects the global literal-pathspec mode as unsupported magic. - .args(["check-ignore", "--stdin", "-z"]) + .args(["--stdin", "-z"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; match output.status.code() { Some(0) => { let ignored = parse_reported_paths(&output.stdout, &probe_paths, "ignored-path")? @@ -171,13 +189,10 @@ fn ignored_untracked_paths( } fn sparse_checkout_policy( - git: &GitRunner, - git_root: &Path, + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result> { - if !effective_git_bool(git, git_root, git_config_args, "core.sparseCheckout")?.unwrap_or(false) - { + if !config.read_bool("core.sparseCheckout")?.unwrap_or(false) { return Ok(None); } @@ -185,12 +200,7 @@ fn sparse_checkout_policy( // `git-sparse-checkout` helper. Repository-controlled PATH entries are // outside the trusted executable boundary. `--list-cmds=builtins` is a // main-program capability query and cannot dispatch an external command. - let mut builtins_command = git.command_for_cwd(git_root)?; - builtins_command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .arg("--list-cmds=builtins"); - let builtins = git.output(builtins_command)?; + let builtins = config.list_builtin_commands()?; let sparse_checkout_is_builtin = builtins.status.success() && std::str::from_utf8(&builtins.stdout) .is_ok_and(|output| output.lines().any(|command| command == "sparse-checkout")); @@ -203,13 +213,12 @@ fn sparse_checkout_policy( } let input = nul_path_input(paths)?; - let mut command = git.command_for_cwd(git_root)?; + let mut command = config.sparse_checkout_command()?; command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["sparse-checkout", "check-rules", "-z"]) + .disable_optional_locks() + .args(["check-rules", "-z"]) .stdin(Stdio::from(input)); - let output = git.output(command)?; + let output = command.output()?; if !output.status.success() { return Ok(Some(StagePathsResult { exit_code: output.status.code().unwrap_or(-1), 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..eb39ded9e7 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,18 @@ 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 ensure_repository_root_route(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_repository_root_route(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 +152,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..892add580d --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -0,0 +1,573 @@ +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::FilterExecution; +use crate::safe_git::FilterPolicyRole; +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 read_bool(&self, key: &str) -> io::Result> { + let mut command = self.git.command_for_cwd(&self.canonical_root)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(&self.base_config_args) + .args(["config", "--type=bool", "--get", key]); + let output = self.git.output(command)?; + if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { + return Ok(None); + } + if !output.status.success() { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "Git boolean config probe for {key:?} failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ), + )); + } + match String::from_utf8_lossy(&output.stdout).trim() { + "true" => Ok(Some(true)), + "false" => Ok(Some(false)), + value => Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("unexpected normalized Git boolean value {value:?} for {key:?}"), + )), + } + } +} + +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 { + Apply, + CheckAttr, + CheckIgnore, + HashObject, + ListBuiltinCommands, + LsFiles, + RevParse, + SparseCheckout, + UpdateIndexLiteralPathspecs, +} + +impl BoundSubcommand { + fn append_to(self, command: &mut GitCommand) { + match self { + Self::Apply => { + command.arg("apply"); + } + Self::CheckAttr => { + command.arg("check-attr"); + } + Self::CheckIgnore => { + command.arg("check-ignore"); + } + Self::HashObject => { + command.arg("hash-object"); + } + Self::ListBuiltinCommands => { + command.arg("--list-cmds=builtins"); + } + Self::LsFiles => { + command.arg("ls-files"); + } + Self::RevParse => { + command.arg("rev-parse"); + } + Self::SparseCheckout => { + command.arg("sparse-checkout"); + } + Self::UpdateIndexLiteralPathspecs => { + command.args(["--literal-pathspecs", "update-index"]); + } + } + } +} + +/// 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 rev_parse_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::RevParse) + } + + pub(crate) fn check_ignore_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::CheckIgnore) + } + + pub(crate) fn list_builtin_commands(&self) -> io::Result { + let mut command = self.guarded_command(BoundSubcommand::ListBuiltinCommands)?; + command.disable_optional_locks(); + command.output() + } + + pub(crate) fn ls_files_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::LsFiles) + } + + pub(crate) fn sparse_checkout_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::SparseCheckout) + } + + pub(crate) fn update_index_literal_pathspecs_command( + &self, + ) -> io::Result> { + self.guarded_command(BoundSubcommand::UpdateIndexLiteralPathspecs) + } + + pub(crate) fn pending_filter_attribute_command<'operation>( + &'operation self, + neutralizer: &'operation SealedFilterConfigOverride, + ) -> io::Result> { + let mut command = self.command_with_attached_overlays()?; + neutralizer.append_to(&self.identity, &mut command)?; + BoundSubcommand::CheckAttr.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + fn guarded_command( + &self, + subcommand: BoundSubcommand, + ) -> io::Result> { + let mut command = self.command_with_attached_overlays()?; + subcommand.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + fn command_with_attached_overlays(&self) -> 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)?; + } + } + Ok(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) + } + + pub(crate) fn read_bool(&self, key: &str) -> io::Result> { + self.sources.read_bool(key) + } + + 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<()> { + if !self.filters.is_empty() { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "apply filter policy must be the first operation snapshot", + )); + } + let filter = + build_filter_policy_snapshot(self, paths, FilterExecution::AnyWorktreeOperation)?; + self.filters.push(filter); + Ok(()) + } + + pub(crate) fn ensure_apply_filter_path_subset(&self, paths: &[String]) -> io::Result<()> { + let [apply] = self.filters.as_slice() else { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "composed exact staging requires exactly one apply filter snapshot", + )); + }; + if apply.role() != FilterPolicyRole::Apply { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "composed exact staging requires an apply filter snapshot", + )); + } + if let Some(path) = paths + .iter() + .find(|path| !apply.contains_checked_path(path.as_str())) + { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "content-filter staging path {path:?} was not authorized by the apply snapshot" + ), + )); + } + Ok(()) + } + + pub(crate) fn authorize_git_add_filter_paths(&mut self, paths: &[String]) -> io::Result<()> { + match self.filters.as_slice() { + [] => {} + [apply] if apply.role() == FilterPolicyRole::Apply => {} + _ => { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Git-add filter policy requires zero snapshots or exactly one apply snapshot", + )); + } + } + let filter = build_filter_policy_snapshot(self, paths, FilterExecution::GitAdd)?; + 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..3d0256c6d1 --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -0,0 +1,224 @@ +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"]); + std::fs::write(repo.path().join("second.txt"), "second\n").expect("write sink input"); + 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_git_add_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()) + ); + + let apply_include = config.filters[0] + .neutralizer() + .expect("apply overlay") + .include_arg + .clone(); + let staging_include = config.filters[1] + .neutralizer() + .expect("staging overlay") + .include_arg + .clone(); + assert_ne!(apply_include, staging_include); + let apply_path = PathBuf::from( + apply_include + .strip_prefix("include.path=") + .expect("apply include path"), + ); + let staging_path = PathBuf::from( + staging_include + .strip_prefix("include.path=") + .expect("staging include path"), + ); + assert!(apply_path.is_file()); + assert!(staging_path.is_file()); + + let rendered = config + .render_command_for_log(&["update-index".to_string()]) + .expect("render ordered overlays"); + assert!( + rendered.find(&apply_include).expect("render apply overlay") + < rendered + .find(&staging_include) + .expect("render staging overlay") + ); + + let mut sink = config + .update_index_literal_pathspecs_command() + .expect("bound sink"); + sink.disable_optional_locks() + .args(["--add", "--remove", "--", "second.txt"]); + let output = sink.output().expect("run sink"); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(apply_path.is_file(), "apply overlay must outlive the sink"); + assert!( + staging_path.is_file(), + "staging overlay must outlive the sink" + ); + + drop(config); + assert!(!apply_path.exists()); + assert!(!staging_path.exists()); +} + +#[test] +fn composed_staging_subset_proof_requires_exactly_one_apply_snapshot() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let mut config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + let error = config + .ensure_apply_filter_path_subset(&[]) + .expect_err("zero snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + config + .authorize_filter_paths(&["allowed.txt".to_string()]) + .expect("apply snapshot"); + config + .ensure_apply_filter_path_subset(&["allowed.txt".to_string()]) + .expect("authorized subset"); + let error = config + .ensure_apply_filter_path_subset(&["outside.txt".to_string()]) + .expect_err("off-universe path must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + config + .authorize_git_add_filter_paths(&[]) + .expect("Git-add snapshot"); + let error = config + .ensure_apply_filter_path_subset(&[]) + .expect_err("two snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + +#[test] +fn global_builtin_query_is_a_fixed_no_argument_operation() { + let repo = tempfile::tempdir().expect("repo"); + run_git(repo.path(), &["init", "-q"]); + let git = GitRunner::for_cwd_io(repo.path()).expect("runner"); + let config = + GuardedGitConfig::authorize(&git, repo.path(), Vec::new()).expect("authorized config"); + + // The capability exposes only this no-argument execution method; callers + // never receive the global-only command and therefore cannot append a + // second global option or an external command name. + let output = config + .list_builtin_commands() + .expect("fixed builtin-command query"); + assert!(output.status.success()); + let output = std::str::from_utf8(&output.stdout).expect("UTF-8 builtin names"); + assert!(output.lines().any(|name| name == "add")); +} + +#[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 892e2767e2..5812e778be 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -9,8 +9,10 @@ mod exact_staging; 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 6e4604f5a6..4c719ffde6 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -4,29 +4,28 @@ use std::io; use std::path::Path; use std::path::PathBuf; -use crate::apply::run_git; -use crate::apply::safe_git_config_parts; use crate::apply::write_temp_patch; -use crate::exact_staging::update_index_exact_paths; +use crate::exact_staging::update_index_exact_paths_from_apply; +use crate::exact_staging::update_index_exact_paths_standalone; 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 paths with Git from a cwd whose config sources have already been -/// authorized for `git_config_args`. -pub(crate) fn extract_effective_paths_from_patch( - git: &GitRunner, - authorized_cwd: &Path, +/// 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, - git_config_args: &[String], ) -> io::Result> { - let forward_paths = - git_apply_numstat_paths(git, authorized_cwd, patch_path, revert, git_config_args)?; - // `git apply --numstat` reports only the destination of a rename. Parse the - // 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)?; + 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) +} + +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, @@ -47,6 +46,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 @@ -64,45 +87,18 @@ fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec }; let paths = (|| -> io::Result> { let git = GitRunner::for_cwd_io(cwd)?; - let git_root = crate::get_git_repo_root(cwd) + let requested_cwd = std::fs::canonicalize(cwd)?; + let git_root = crate::get_git_repo_root(&requested_cwd) .ok_or_else(|| io::Error::other("not a Git repository"))?; let git_root = std::fs::canonicalize(git_root)?; - ensure_no_worktree_config_sources(&git, &git_root, &[])?; - extract_effective_paths_from_patch(&git, &git_root, &patch_path, /*revert*/ false, &[]) + let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?; + extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ false) })() .unwrap_or_default(); drop(tmpdir); paths } -fn git_apply_numstat_paths( - git: &GitRunner, - authorized_cwd: &Path, - patch_path: &Path, - revert: bool, - git_config_args: &[String], -) -> io::Result> { - let mut cmd = git.command_for_cwd(authorized_cwd)?; - cmd.args(git_config_args); - cmd.args(["apply", "--numstat", "-z"]); - if revert { - cmd.arg("-R"); - } - cmd.arg("--").arg(patch_path); - let out = git.output(cmd)?; - if !out.status.success() { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - format!( - "failed to parse patch paths: {}", - String::from_utf8_lossy(&out.stderr).trim() - ), - )); - } - - parse_numstat_paths(&out.stdout) -} - fn parse_numstat_paths(output: &[u8]) -> io::Result> { if !output.is_empty() && !output.ends_with(&[0]) { return Err(io::Error::new( @@ -245,31 +241,47 @@ fn invalid_windows_patch_component(component: &str) -> bool { /// Stage only the files that actually exist on disk for the given diff. pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> { let git = GitRunner::for_cwd_io(git_root)?; - let git_config_args = safe_git_config_parts(); - ensure_no_worktree_config_sources(&git, git_root, &git_config_args)?; + git.ensure_repository_root_route(git_root)?; + let canonical_root = std::fs::canonicalize(git_root)?; + let mut config = GuardedGitConfig::authorize(&git, &canonical_root, Vec::new())?; let (tmpdir, patch_path) = write_temp_patch(diff)?; - let paths = extract_effective_paths_from_patch( - &git, - git_root, - &patch_path, - /*revert*/ true, - &git_config_args, - )?; + let paths = + extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ true)?; let _guard = tmpdir; - stage_effective_paths(&git, git_root, &paths, &git_config_args) + stage_effective_paths_standalone(&mut config, &paths) } -pub(crate) fn stage_effective_paths( - git: &GitRunner, - git_root: &Path, +pub(crate) fn stage_effective_paths_from_apply( + config: &mut GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result<()> { - let confined = confine_patch_paths(git, git_root, paths)?; + let (existing, content_filter_paths) = classify_exact_staging_leaves(config, paths)?; + let _result = update_index_exact_paths_from_apply(config, &existing, &content_filter_paths)?; + // Preserve the public helper's historical best-effort treatment of a + // non-zero staging command. Security and probe failures still propagate. + Ok(()) +} + +fn stage_effective_paths_standalone( + config: &mut GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result<()> { + let (existing, content_filter_paths) = classify_exact_staging_leaves(config, paths)?; + let _result = update_index_exact_paths_standalone(config, &existing, &content_filter_paths)?; + // Preserve the public helper's historical best-effort treatment of a + // non-zero staging command. Security and probe failures still propagate. + Ok(()) +} + +fn classify_exact_staging_leaves( + config: &GuardedGitConfig<'_>, + paths: &[String], +) -> io::Result<(Vec, Vec)> { + let confined = confine_patch_paths_guarded(config, paths)?; let mut existing = Vec::new(); let mut content_filter_paths = Vec::new(); for path in confined.into_exact_leaves()? { - let joined = git_root.join(&path); + let joined = config.canonical_root().join(&path); if let Ok(metadata) = std::fs::symlink_metadata(&joined) { let file_type = metadata.file_type(); if leaf_is_traversable_directory(file_type) { @@ -283,19 +295,7 @@ pub(crate) fn stage_effective_paths( existing.push(path); } } - if existing.is_empty() { - return Ok(()); - } - let _result = update_index_exact_paths( - git, - git_root, - &existing, - &content_filter_paths, - git_config_args, - )?; - // Preserve the public helper's historical best-effort treatment of a - // non-zero staging command. Security and probe failures still propagate. - Ok(()) + Ok((existing, content_filter_paths)) } #[cfg(not(windows))] @@ -397,10 +397,30 @@ impl ConfinedPatchPaths { } } +#[cfg(test)] pub(crate) fn confine_patch_paths( git: &GitRunner, git_root: &Path, paths: &[String], +) -> io::Result { + let canonical_root = std::fs::canonicalize(git_root)?; + let config = GuardedGitConfig::authorize(git, &canonical_root, Vec::new())?; + confine_patch_paths_guarded(&config, paths) +} + +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 { @@ -423,8 +443,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(); @@ -438,9 +456,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( @@ -535,22 +553,25 @@ fn containment_error(message: &'static str) -> io::Error { io::Error::new(io::ErrorKind::PermissionDenied, message) } -fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result> { - let config_parts = safe_git_config_parts(); +fn canonical_git_metadata_dirs_guarded(config: &GuardedGitConfig<'_>) -> io::Result> { let queries = [ - vec!["rev-parse".to_string(), "--absolute-git-dir".to_string()], - vec!["rev-parse".to_string(), "--git-common-dir".to_string()], + ["rev-parse", "--absolute-git-dir"], + ["rev-parse", "--git-common-dir"], ]; let mut metadata_dirs = std::collections::BTreeSet::new(); for args in queries { - let (code, stdout, stderr) = run_git(git, git_root, &config_parts, &args)?; - if code != 0 { + 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 (exit {code}): {}", - stderr.trim() + "failed to resolve Git repository metadata (status {}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() ))); } - let path = stdout.trim_end_matches(['\r', '\n']); + 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, @@ -561,7 +582,7 @@ fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result io::Result> { let git_root = crate::get_git_repo_root(cwd).ok_or_else(|| io::Error::other("not a Git repository"))?; let git_root = std::fs::canonicalize(git_root)?; - ensure_no_worktree_config_sources(&git, &git_root, &[])?; - let paths = extract_effective_paths_from_patch(&git, &git_root, &patch_path, revert, &[])?; + let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?; + let paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, revert)?; drop(tmpdir); Ok(paths) } @@ -81,6 +88,152 @@ fn best_effort_paths(diff: &str) -> Vec { extract_paths_from_patch_from_cwd(diff, repo.path()) } +#[cfg(unix)] +#[test] +fn best_effort_extraction_selects_the_physical_repo_from_a_symlinked_nested_cwd() { + 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"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + let lexical_cwd = outer.join("linked-nested"); + std::os::unix::fs::symlink(&nested, &lexical_cwd).expect("symlink nested cwd"); + + assert_eq!( + extract_paths_from_patch_from_cwd(&new_file_diff("physical.txt"), &lexical_cwd), + vec!["physical.txt".to_string()] + ); +} + +#[cfg(unix)] +#[test] +fn standalone_stage_paths_preserves_a_symlinked_repository_route() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = fixture.path().join("linked-root"); + std::os::unix::fs::symlink(&target, &lexical_root).expect("symlink repository root"); + + reset_config_source_authorization_count(); + stage_paths(&lexical_root, &new_file_diff("routed.txt")).expect("stage through symlink route"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&outer, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(windows)] +#[test] +fn standalone_stage_paths_preserves_a_junction_repository_route() { + let fixture = tempfile::tempdir().expect("fixture"); + let outer = fixture.path().join("outer"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&outer).expect("outer"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&outer, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = fixture.path().join("linked-root"); + let _junction = create_dir_alias(&target, &lexical_root); + + reset_config_source_authorization_count(); + stage_paths(&lexical_root, &new_file_diff("routed.txt")).expect("stage through junction route"); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&outer, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(any(unix, windows))] +#[test] +fn standalone_stage_paths_preserves_an_independent_repository_alias() { + let fixture = tempfile::tempdir().expect("fixture"); + let independent = fixture.path().join("independent"); + let target = fixture.path().join("target"); + std::fs::create_dir_all(&independent).expect("independent"); + std::fs::create_dir_all(&target).expect("target"); + assert_eq!(run(&independent, &["git", "init", "-q"]).0, 0); + assert_eq!(run(&target, &["git", "init", "-q"]).0, 0); + std::fs::write(target.join("routed.txt"), "routed\n").expect("write target file"); + let lexical_root = independent.join("linked-root"); + let _route = create_dir_alias(&target, &lexical_root); + + stage_paths(&lexical_root, &new_file_diff("routed.txt")) + .expect("independent repository alias remains supported"); + assert_eq!( + run(&target, &["git", "diff", "--cached", "--name-only"]).1, + "routed.txt\n" + ); + assert!( + run(&independent, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + +#[cfg(any(unix, windows))] +#[test] +fn standalone_stage_paths_rejects_a_route_crossing_a_related_worktree() { + let fixture = tempfile::tempdir().expect("fixture"); + let primary = fixture.path().join("primary"); + let linked = fixture.path().join("linked"); + std::fs::create_dir_all(&primary).expect("primary"); + assert_eq!(run(&primary, &["git", "init", "-q"]).0, 0); + assert_eq!( + run( + &primary, + &["git", "config", "user.email", "codex@example.com"] + ) + .0, + 0 + ); + assert_eq!(run(&primary, &["git", "config", "user.name", "Codex"]).0, 0); + std::fs::write(primary.join("seed.txt"), "seed\n").expect("write seed"); + assert_eq!(run(&primary, &["git", "add", "seed.txt"]).0, 0); + assert_eq!(run(&primary, &["git", "commit", "-m", "seed"]).0, 0); + let linked_arg = linked.to_string_lossy().into_owned(); + assert_eq!( + run( + &primary, + &["git", "worktree", "add", "--detach", &linked_arg] + ) + .0, + 0 + ); + std::fs::write(linked.join("routed.txt"), "routed\n").expect("write linked file"); + let crossing_route = primary.join("linked-route"); + let _route = create_dir_alias(&linked, &crossing_route); + + let error = stage_paths(&crossing_route, &new_file_diff("routed.txt")) + .expect_err("related-worktree crossing must reject"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + assert!( + run(&linked, &["git", "diff", "--cached", "--name-only"]) + .1 + .is_empty() + ); +} + #[test] fn effective_paths_cover_supported_patch_headers() { let cases = [ @@ -533,9 +686,11 @@ fn index_entry(root: &Path, path: &str) -> String { fn exact_stage(root: &Path, paths: &[&str], content_filter_paths: &[&str]) -> StagePathsResult { let git = GitRunner::for_cwd_io(root).expect("trusted Git"); - update_index_exact_paths( - &git, - root, + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorized config"); + update_index_exact_paths_standalone( + &mut config, &paths .iter() .map(|path| (*path).to_string()) @@ -544,11 +699,219 @@ fn exact_stage(root: &Path, paths: &[&str], content_filter_paths: &[&str]) -> St .iter() .map(|path| (*path).to_string()) .collect::>(), - &safe_git_config_parts(), ) .expect("run exact staging primitive") } +fn stage_effective_paths( + git: &GitRunner, + root: &Path, + paths: &[String], + legacy_config_args: &[String], +) -> io::Result<()> { + let _ = legacy_config_args; + let canonical_root = std::fs::canonicalize(root)?; + let mut config = GuardedGitConfig::authorize(git, &canonical_root, Vec::new())?; + stage_effective_paths_standalone(&mut config, paths) +} + +#[test] +fn standalone_stage_paths_uses_one_source_authorization_and_one_filter_read() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("single.txt"), "single\n").expect("write staged file"); + + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + stage_paths(root, &new_file_diff("single.txt")).expect("standalone exact staging"); + + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 1); + assert_eq!(filter_policy_overlay_count(), 0); +} + +#[test] +fn composed_staging_freshly_observes_a_new_clean_driver_and_attribute() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "base\n").expect("write base file"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(root.join("file.txt"), "changed\n").expect("modify file"); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("apply snapshot without filters"); + + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.fresh.clean", + "git config codex.stage-filter-ran true && git hash-object --stdin", + ], + ) + .0, + 0 + ); + std::fs::write(root.join(".gitattributes"), "file.txt filter=fresh\n") + .expect("select fresh filter"); + + let error = update_index_exact_paths_from_apply( + &mut config, + &["file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("fresh clean driver must refuse"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(filter_policy_overlay_count(), 1); + assert_ne!( + run(root, &["git", "config", "--get", "codex.stage-filter-ran"]).0, + 0, + "fresh policy must not execute the filter" + ); +} + +#[test] +fn composed_staging_required_read_ignores_the_apply_overlay_and_is_fresh() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("file.txt"), "base\n").expect("write base file"); + assert_eq!(run(root, &["git", "add", "file.txt"]).0, 0); + assert_eq!(run(root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(root.join("file.txt"), "changed\n").expect("modify file"); + assert_eq!( + run( + root, + &[ + "git", + "config", + "filter.demo.smudge", + "git config codex.stage-smudge-ran true && git hash-object --stdin", + ], + ) + .0, + 0 + ); + + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + reset_config_source_authorization_count(); + reset_filter_policy_counts(); + let mut config = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("unselected apply filter snapshot"); + + std::fs::write(root.join(".gitattributes"), "file.txt filter=demo\n") + .expect("select smudge-only filter"); + assert_eq!( + run(root, &["git", "config", "filter.demo.required", "true"]).0, + 0 + ); + + let error = update_index_exact_paths_from_apply( + &mut config, + &["file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("fresh required=true must refuse"); + assert_eq!(error.kind(), io::ErrorKind::Unsupported); + assert_eq!(config_source_authorization_count(), 1); + assert_eq!(filter_policy_read_count(), 2); + assert_eq!(filter_policy_overlay_count(), 2); + assert_ne!( + run(root, &["git", "config", "--get", "codex.stage-smudge-ran"]).0, + 0, + "smudge helper must remain neutralized" + ); +} + +#[test] +fn composed_exact_staging_refuses_requested_and_mapped_paths_outside_apply_universe() { + let requested_repo = init_repo(); + let requested_root = requested_repo.path(); + let requested_git = GitRunner::for_cwd_io(requested_root).expect("trusted Git"); + let mut requested_config = GuardedGitConfig::authorize( + &requested_git, + &std::fs::canonicalize(requested_root).expect("canonical root"), + Vec::new(), + ) + .expect("authorization"); + requested_config + .authorize_filter_paths(&["allowed.txt".to_string()]) + .expect("apply snapshot"); + let requested_error = update_index_exact_paths_from_apply( + &mut requested_config, + &["allowed.txt".to_string(), "outside.txt".to_string()], + &["outside.txt".to_string()], + ) + .expect_err("requested off-universe path must refuse"); + assert_eq!(requested_error.kind(), io::ErrorKind::PermissionDenied); + + let mapped_repo = init_repo(); + let mapped_root = mapped_repo.path(); + std::fs::write(mapped_root.join("File.txt"), "tracked\n").expect("write tracked spelling"); + assert_eq!(run(mapped_root, &["git", "add", "File.txt"]).0, 0); + assert_eq!(run(mapped_root, &["git", "commit", "-m", "base"]).0, 0); + std::fs::write(mapped_root.join("file.txt"), "alias\n").expect("write alias spelling"); + assert_eq!( + run(mapped_root, &["git", "config", "core.ignoreCase", "true"]).0, + 0 + ); + let mapped_git = GitRunner::for_cwd_io(mapped_root).expect("trusted Git"); + let mut mapped_config = GuardedGitConfig::authorize( + &mapped_git, + &std::fs::canonicalize(mapped_root).expect("canonical root"), + Vec::new(), + ) + .expect("authorization"); + mapped_config + .authorize_filter_paths(&["file.txt".to_string()]) + .expect("narrow apply snapshot"); + let mapped_error = update_index_exact_paths_from_apply( + &mut mapped_config, + &["File.txt".to_string(), "file.txt".to_string()], + &["file.txt".to_string()], + ) + .expect_err("mapped off-universe path must refuse"); + assert_eq!(mapped_error.kind(), io::ErrorKind::PermissionDenied); +} + +#[test] +fn composed_exact_staging_refuses_zero_or_two_prior_snapshots_even_when_empty() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("trusted Git"); + let canonical_root = std::fs::canonicalize(root).expect("canonical root"); + + let mut zero = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + let error = update_index_exact_paths_from_apply(&mut zero, &[], &[]) + .expect_err("zero snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); + + let mut two = + GuardedGitConfig::authorize(&git, &canonical_root, Vec::new()).expect("authorization"); + two.authorize_filter_paths(&[]).expect("apply snapshot"); + two.authorize_git_add_filter_paths(&[]) + .expect("Git-add snapshot"); + let error = update_index_exact_paths_from_apply(&mut two, &[], &[]) + .expect_err("two snapshots must refuse"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied); +} + fn assert_exact_refused_unchanged( root: &Path, paths: &[&str], 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/repository_authority/authority/policy.rs b/codex-rs/git-utils/src/repository_authority/authority/policy.rs index e5f2379460..e38668e81c 100644 --- a/codex-rs/git-utils/src/repository_authority/authority/policy.rs +++ b/codex-rs/git-utils/src/repository_authority/authority/policy.rs @@ -16,6 +16,40 @@ use super::super::repository_common_dir_for_candidate_root; use super::RepositoryAuthority; impl RepositoryAuthority { + pub(crate) fn ensure_repository_root_route(&self, root: &Path) -> io::Result<()> { + self.ensure_active_worktree_root(root)?; + let crosses_worktree = self.route_boundaries.inspect_route(root)?.crosses_worktree; + let crosses_related_worktree = match root.parent() { + Some(parent) => self.has_active_common_dir_ancestor(parent)?, + None => false, + }; + if crosses_worktree && crosses_related_worktree { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "refusing repository root route that crosses a worktree boundary: {}", + root.display() + ), + )); + } + Ok(()) + } + + fn has_active_common_dir_ancestor(&self, path: &Path) -> io::Result { + let Some(active) = &self.active_metadata else { + return Ok(false); + }; + for ancestor in path.ancestors() { + let Some(common_dir) = repository_common_dir_for_candidate_root(ancestor)? else { + continue; + }; + if directories_refer_to_same_location(&common_dir, &active.common_dir)? { + return Ok(true); + } + } + Ok(false) + } + pub(crate) fn canonical_command_cwd(&self, cwd: &Path) -> io::Result { let canonical = std::fs::canonicalize(cwd)?; let inspection = self.route_boundaries.inspect_route(cwd)?; diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 2ddcbea37f..67eae888c1 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,19 +27,48 @@ 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> { - effective_filter_value(&self.filter_config, driver, name) + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn contains(&self, driver: &str) -> bool { + self.0.contains(driver) + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum FilterPolicyRole { + Apply, + GitAdd, +} + +pub(crate) struct FilterPolicySnapshot { + role: FilterPolicyRole, + checked_paths: BTreeSet, + neutralizer: Option, +} + +impl FilterPolicySnapshot { + pub(crate) fn neutralizer(&self) -> Option<&SealedFilterConfigOverride> { + self.neutralizer.as_ref() + } + + pub(crate) fn role(&self) -> FilterPolicyRole { + self.role + } + + pub(crate) fn contains_checked_path(&self, path: &str) -> bool { + self.checked_paths.contains(path) } } @@ -71,44 +99,12 @@ 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 { - ensure_no_selected_executable_git_filters_for( - git, - cwd, - paths, - git_config_args, - FilterExecution::AnyWorktreeOperation, - ) -} - -pub(crate) fn ensure_no_selected_git_add_filters( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], -) -> io::Result { - ensure_no_selected_executable_git_filters_for( - git, - cwd, - paths, - git_config_args, - FilterExecution::GitAdd, - ) -} - -fn ensure_no_selected_executable_git_filters_for( - git: &GitRunner, - cwd: &Path, - paths: &[String], - git_config_args: &[String], execution: FilterExecution, -) -> io::Result { - let entries = read_filter_config(git, cwd, git_config_args).map_err(|error| { +) -> io::Result { + let entries = read_filter_config(config).map_err(|error| { if matches!(execution, FilterExecution::GitAdd) && error.kind() == io::ErrorKind::InvalidData { @@ -121,26 +117,26 @@ fn ensure_no_selected_executable_git_filters_for( } })?; let executable_drivers = executable_filter_drivers(&entries)?; + let role = match execution { + FilterExecution::AnyWorktreeOperation => FilterPolicyRole::Apply, + FilterExecution::GitAdd => FilterPolicyRole::GitAdd, + }; + let checked_paths = paths.iter().cloned().collect(); if executable_drivers.is_empty() { - return Ok(GitFilterNeutralization { - git_config_args: Vec::new(), - _config_dir: None, - filter_config: entries, + return Ok(FilterPolicySnapshot { + role, + checked_paths, + neutralizer: None, }); } - let guard = executable_filter_guard(git, cwd, entries, &executable_drivers)?; + let neutralizer = config.build_filter_override(&executable_drivers)?; + #[cfg(test)] + FILTER_POLICY_OVERLAY_COUNT.with(|count| count.set(count.get() + 1)); 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, - )?; + let attributes = read_filter_attributes(config, &paths, &executable_drivers, &neutralizer)?; let mut required_cache = BTreeMap::new(); for (path, driver) in &attributes { if !executable_drivers.contains(driver) { @@ -148,14 +144,9 @@ fn ensure_no_selected_executable_git_filters_for( } let refused = match execution { FilterExecution::AnyWorktreeOperation => true, - FilterExecution::GitAdd => git_add_filter_is_refused( - git, - cwd, - &guard.filter_config, - driver, - git_config_args, - &mut required_cache, - )?, + FilterExecution::GitAdd => { + git_add_filter_is_refused(config, &entries, driver, &mut required_cache)? + } }; if refused { return Err(io::Error::new( @@ -167,15 +158,17 @@ fn ensure_no_selected_executable_git_filters_for( )); } } - Ok(guard) + Ok(FilterPolicySnapshot { + role, + checked_paths, + neutralizer: Some(neutralizer), + }) } fn git_add_filter_is_refused( - git: &GitRunner, - cwd: &Path, + config: &GuardedGitConfig<'_>, entries: &BTreeMap, driver: &str, - git_config_args: &[String], required_cache: &mut BTreeMap, ) -> io::Result { if ["clean", "process"].into_iter().any(|name| { @@ -186,77 +179,23 @@ fn git_add_filter_is_refused( if let Some(required) = required_cache.get(driver) { return Ok(*required); } - let required = git_filter_required(git, cwd, driver, git_config_args)?; + let required = config + .read_bool(&format!("filter.{driver}.required")) + .map_err(|error| { + io::Error::new( + io::ErrorKind::Unsupported, + format!( + "refusing selected Git filter {driver:?} with malformed required value: {error}" + ), + ) + })? + .unwrap_or(false); required_cache.insert(driver.to_string(), required); Ok(required) } -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(()) - } -} - #[derive(Clone, Copy)] -enum FilterExecution { +pub(crate) enum FilterExecution { AnyWorktreeOperation, GitAdd, } @@ -269,37 +208,18 @@ enum FilterCommand { } 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) + #[cfg(test)] + FILTER_POLICY_READ_COUNT.with(|count| count.set(count.get() + 1)); + 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()); @@ -308,22 +228,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.pending_filter_attribute_command(neutralization)?; 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 {}: {}", @@ -332,23 +242,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(); @@ -360,11 +261,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, )? @@ -380,20 +279,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)?; @@ -419,12 +314,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<'_> { @@ -433,29 +326,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) } @@ -485,6 +375,28 @@ fn selected_executable_filter_for( Ok(selected_filter(&executable_drivers, attributes)) } +#[cfg(test)] +fn executable_filter_drivers_for( + entries: &BTreeMap, + execution: FilterExecution, +) -> io::Result> { + let mut drivers = BTreeSet::new(); + for entry in entries.values() { + if entry.key.ends_with(".required") { + continue; + } + let (driver, command) = filter_driver_and_command(&entry.key)?; + let relevant = match execution { + FilterExecution::AnyWorktreeOperation => true, + FilterExecution::GitAdd => command != FilterCommand::Smudge, + }; + if relevant && !entry.value.is_empty() { + drivers.insert(driver); + } + } + Ok(drivers) +} + #[cfg(test)] fn selected_filter( drivers: &BTreeSet, @@ -506,31 +418,20 @@ fn selected_executable_filter( selected_executable_filter_for(entries, attributes, FilterExecution::AnyWorktreeOperation) } -pub(crate) fn executable_filter_drivers( +fn executable_filter_drivers( entries: &BTreeMap, -) -> io::Result> { - executable_filter_drivers_for(entries, FilterExecution::AnyWorktreeOperation) -} - -fn executable_filter_drivers_for( - entries: &BTreeMap, - execution: FilterExecution, -) -> io::Result> { +) -> io::Result { let mut executable_drivers = BTreeSet::new(); for entry in entries.values() { if entry.key.ends_with(".required") { continue; } - let (driver, command) = filter_driver_and_command(&entry.key)?; - let relevant = match execution { - FilterExecution::AnyWorktreeOperation => true, - FilterExecution::GitAdd => command != FilterCommand::Smudge, - }; - if relevant && !entry.value.is_empty() { + let (driver, _command) = filter_driver_and_command(&entry.key)?; + if !entry.value.is_empty() { executable_drivers.insert(driver); } } - Ok(executable_drivers) + Ok(ExecutableFilterDrivers(executable_drivers)) } fn effective_filter_value<'a>( @@ -543,43 +444,6 @@ fn effective_filter_value<'a>( .map(|entry| entry.value.as_str()) } -fn git_filter_required( - git: &GitRunner, - cwd: &Path, - driver: &str, - git_config_args: &[String], -) -> io::Result { - let mut command = git.command_for_cwd(cwd)?; - command - .env("GIT_OPTIONAL_LOCKS", "0") - .args(git_config_args) - .args(["config", "--type=bool", "--get"]) - .arg(format!("filter.{driver}.required")); - let output = git.output(command)?; - if output.status.code() == Some(1) && output.stdout.is_empty() && output.stderr.is_empty() { - return Ok(false); - } - if !output.status.success() { - return Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing selected Git filter {driver:?} with malformed required value: {}", - String::from_utf8_lossy(&output.stderr).trim() - ), - )); - } - match String::from_utf8_lossy(&output.stdout).trim() { - "true" => Ok(true), - "false" => Ok(false), - normalized => Err(io::Error::new( - io::ErrorKind::Unsupported, - format!( - "refusing selected Git filter {driver:?} with unexpected normalized required value {normalized:?}" - ), - )), - } -} - #[cfg(test)] fn filter_driver_name(key: &str) -> io::Result { filter_driver_and_command(key).map(|(driver, _command)| driver) @@ -671,6 +535,28 @@ fn invalid_filter_output(message: &str) -> io::Error { io::Error::new(io::ErrorKind::InvalidData, message) } +#[cfg(test)] +thread_local! { + static FILTER_POLICY_READ_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; + static FILTER_POLICY_OVERLAY_COUNT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +pub(crate) fn reset_filter_policy_counts() { + FILTER_POLICY_READ_COUNT.with(|count| count.set(0)); + FILTER_POLICY_OVERLAY_COUNT.with(|count| count.set(0)); +} + +#[cfg(test)] +pub(crate) fn filter_policy_read_count() -> usize { + FILTER_POLICY_READ_COUNT.with(std::cell::Cell::get) +} + +#[cfg(test)] +pub(crate) fn filter_policy_overlay_count() -> usize { + FILTER_POLICY_OVERLAY_COUNT.with(std::cell::Cell::get) +} + #[cfg(test)] #[path = "safe_git_tests.rs"] mod tests; diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index 02bb253a94..a4b3c8cfcb 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -1,7 +1,9 @@ use super::*; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; +use crate::git_command::GitRunner; use crate::git_config::GitConfigScope; +use crate::guarded_config::GuardedGitConfig; #[cfg(unix)] use crate::patch_paths::stage_paths; use pretty_assertions::assert_eq; @@ -156,7 +158,9 @@ 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 selected = BTreeMap::from([(b"file.txt".to_vec(), "demo".to_string())]); @@ -165,14 +169,11 @@ fn filter_snapshot_retains_required_without_treating_it_as_executable() { .expect("Git add filter policy"), None ); - 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( @@ -302,9 +303,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}"); @@ -313,8 +317,9 @@ 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 mut literal_config = GuardedGitConfig::authorize(&git, root, Vec::new()) + .expect("authorize literal-attribute operation"); + let result = literal_config.authorize_filter_paths(&["file.txt".to_string()]); let error = match result { Ok(_) => panic!("accepted literal sentinel-named driver {driver}"), Err(error) => error, @@ -335,9 +340,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()]) @@ -349,10 +357,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, ) @@ -368,9 +374,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| { @@ -382,10 +391,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, ) @@ -398,36 +405,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)); }