diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 5f3186a70d..45099ad5de 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -10,13 +10,11 @@ use std::io; use std::path::Path; use std::path::PathBuf; -use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; use crate::guarded_config::GuardedGitConfig; use crate::patch_paths::extract_effective_paths_from_patch_guarded; use crate::patch_paths::stage_effective_paths_guarded; -use crate::safe_git::DISABLED_HOOKS_PATH; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; @@ -179,26 +177,6 @@ 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], - args: &[String], -) -> io::Result<(i32, String, String)> { - let mut cmd = git.command_for_cwd(cwd)?; - for p in git_cfg { - cmd.arg(p); - } - 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)) -} - fn run_guarded_apply( config: &GuardedGitConfig<'_>, args: &[String], @@ -225,15 +203,6 @@ fn run_guarded_apply( )) } -pub(crate) fn safe_git_config_parts() -> Vec { - vec![ - "-c".to_string(), - format!("core.hooksPath={DISABLED_HOOKS_PATH}"), - "-c".to_string(), - FsmonitorOverride::Disabled.git_config_arg().to_string(), - ] -} - #[cfg(all(test, unix))] #[path = "apply_transport_tests.rs"] mod transport_tests; @@ -309,57 +278,6 @@ mod tests { .replace("\r\n", "\n") } - fn commit_filter_attributes(root: &Path, tracked_path: &str) { - std::fs::write( - root.join(".gitattributes"), - format!("{tracked_path} filter=x=y\n"), - ) - .expect("write attributes"); - let (add_code, _, add_err) = run(root, &["git", "add", ".gitattributes"]); - assert_eq!(add_code, 0, "add attributes: {add_err}"); - let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "attributes"]); - assert_eq!(commit_code, 0, "commit attributes: {commit_err}"); - } - - fn configure_clean_filter(root: &Path, tracked_path: &str) { - commit_filter_attributes(root, tracked_path); - let (config_code, _, config_err) = run( - root, - &[ - "git", - "config", - "filter.x=y.clean", - "git config codex.filterran true && git hash-object --stdin", - ], - ); - assert_eq!(config_code, 0, "configure filter: {config_err}"); - } - - fn configure_worktree_clean_filter(root: &Path, tracked_path: &str) { - commit_filter_attributes(root, tracked_path); - let (extension_code, _, extension_err) = run( - root, - &["git", "config", "extensions.worktreeConfig", "true"], - ); - assert_eq!(extension_code, 0, "enable worktree config: {extension_err}"); - let (config_code, _, config_err) = run( - root, - &[ - "git", - "config", - "--worktree", - "filter.x=y.clean", - "git config codex.filterran true && git hash-object --stdin", - ], - ); - assert_eq!(config_code, 0, "configure worktree filter: {config_err}"); - } - - fn configured_filter_ran(root: &Path) -> bool { - let (code, _, _) = run(root, &["git", "config", "--get", "codex.filterran"]); - code == 0 - } - #[cfg(unix)] fn trusted_git_directory() -> PathBuf { std::env::split_paths(&std::env::var_os("PATH").expect("PATH")) @@ -730,101 +648,6 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1 ); } - #[test] - fn apply_rejects_configured_clean_filter_without_running_it() { - let _g = env_lock().lock().unwrap(); - let repo = init_repo(); - let root = repo.path(); - std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); - let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); - assert_eq!(add_code, 0, "add file: {add_err}"); - let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]); - assert_eq!(commit_code, 0, "commit file: {commit_err}"); - configure_clean_filter(root, "file.txt"); - - let diff = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n"; - for (revert, preflight) in [(false, false), (false, true), (true, false), (true, true)] { - let request = ApplyGitRequest { - cwd: root.to_path_buf(), - diff: diff.to_string(), - 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"); - } - } - - #[test] - fn apply_rejects_worktree_scoped_clean_filter_without_running_it() { - let _g = env_lock().lock().unwrap(); - let repo = init_repo(); - let root = repo.path(); - std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); - let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]); - assert_eq!(add_code, 0, "add file: {add_err}"); - let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]); - assert_eq!(commit_code, 0, "commit file: {commit_err}"); - configure_worktree_clean_filter(root, "file.txt"); - - let request = ApplyGitRequest { - cwd: root.to_path_buf(), - diff: "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n".to_string(), - revert: false, - preflight: true, - }; - let error = apply_git_patch(&request).expect_err("reject worktree filter"); - assert_eq!(error.kind(), io::ErrorKind::Unsupported); - assert!(!configured_filter_ran(root)); - assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n"); - } - - #[test] - fn apply_probe_rejects_command_scoped_clean_filter() { - let _g = env_lock().lock().unwrap(); - if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() { - run_isolated_test( - "apply::tests::apply_probe_rejects_command_scoped_clean_filter", - &[( - "CODEX_APPLY_GIT_CFG", - OsStr::new( - "filter.codex-test.clean=git config codex.filterran true && git hash-object --stdin", - ), - )], - ); - return; - } - - let repo = init_repo(); - let root = repo.path(); - std::fs::write(root.join("test.txt"), "orig\n").expect("write file"); - let (add_code, _, add_err) = run(root, &["git", "add", "test.txt"]); - assert_eq!(add_code, 0, "add file: {add_err}"); - let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]); - assert_eq!(commit_code, 0, "commit file: {commit_err}"); - std::fs::write(root.join(".gitattributes"), "test.txt filter=codex-test\n") - .expect("attributes"); - - let request = ApplyGitRequest { - cwd: root.to_path_buf(), - diff: "diff --git a/test.txt b/test.txt\n--- a/test.txt\n+++ b/test.txt\n@@ -1 +1 @@\n-orig\n+next\n".to_string(), - revert: false, - preflight: true, - }; - let error = apply_git_patch(&request).expect_err("reject command-scoped filter"); - assert_eq!(error.kind(), io::ErrorKind::Unsupported); - assert!(!configured_filter_ran(root)); - assert_eq!(read_file_normalized(&root.join("test.txt")), "orig\n"); - } - #[test] fn resolve_git_root_rejects_core_worktree_redirection() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 3d191735b3..8f161e12be 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -1514,6 +1514,46 @@ fn active_commondir_route_is_revalidated_before_every_child() { ); } +#[cfg(any(unix, windows))] +#[test] +fn active_worktree_identity_is_revalidated_before_every_child() { + let fixture = tempfile::tempdir().expect("fixture"); + let root = fixture.path().join("repo"); + let moved_root = fixture.path().join("repo-moved"); + std::fs::create_dir_all(&root).expect("create repository"); + run_git(&root, &["init", "-q"]); + let runner = GitRunner::for_cwd(&root).expect("runner for original repository"); + let sentinel = fixture.path().join("git-child-ran"); + let mut command = runner + .command_for_cwd(&root) + .expect("command for original repository"); + command + .args(["config", "--file"]) + .arg(&sentinel) + .args(["probe.value", "ran"]); + + std::fs::rename(&root, &moved_root).expect("move original repository"); + std::fs::create_dir_all(&root).expect("create replacement repository"); + run_git(&root, &["init", "-q"]); + assert!(moved_root.join(".git").is_dir()); + assert!(root.join(".git").is_dir()); + + let error = runner + .output(command) + .expect_err("same-path repository replacement must block Git child"); + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert!( + error + .to_string() + .contains("guarded Git repository identity changed before Git launch"), + "{error}" + ); + assert!( + !sentinel.exists(), + "Git child executed after same-path repository replacement" + ); +} + #[cfg(unix)] #[test] fn direct_external_separate_git_dir_absolute_and_relative_routes_execute_git() { diff --git a/codex-rs/git-utils/src/git_config.rs b/codex-rs/git-utils/src/git_config.rs index d476dea374..0d48eb2bce 100644 --- a/codex-rs/git-utils/src/git_config.rs +++ b/codex-rs/git-utils/src/git_config.rs @@ -5,6 +5,84 @@ use std::path::Path; use crate::git_command::GitRunner; +pub(crate) fn parse_git_boolean(value: &[u8]) -> Option { + parse_git_boolean_with_minimum(value, i128::from(i32::MIN)) +} + +/// Parse the Git boolean grammar while excluding numeric `INT_MIN`. +/// +/// Older supported Git releases reject that one signed endpoint after unit +/// expansion, so environment gates that must succeed across versions use this +/// conservative variant. The accepted spellings otherwise share one parser. +pub(crate) fn parse_git_boolean_symmetric_i32(value: &[u8]) -> Option { + parse_git_boolean_with_minimum(value, -i128::from(i32::MAX)) +} + +fn parse_git_boolean_with_minimum(value: &[u8], minimum: i128) -> Option { + if value.eq_ignore_ascii_case(b"true") + || value.eq_ignore_ascii_case(b"yes") + || value.eq_ignore_ascii_case(b"on") + { + return Some(true); + } + if value.is_empty() + || value.eq_ignore_ascii_case(b"false") + || value.eq_ignore_ascii_case(b"no") + || value.eq_ignore_ascii_case(b"off") + { + return Some(false); + } + + // Git parses the remaining boolean spellings through `git_parse_int`: C + // base-0 syntax, an optional binary-unit suffix, and signed `int` bounds. + let value = std::str::from_utf8(value) + .ok()? + .trim_start_matches(|value: char| value.is_ascii_whitespace()); + let (negative, unsigned) = match value.as_bytes().first() { + Some(b'-') => (true, &value[1..]), + Some(b'+') => (false, &value[1..]), + Some(_) => (false, value), + None => return None, + }; + let (base, unsigned) = if unsigned.starts_with("0x") || unsigned.starts_with("0X") { + (16, &unsigned[2..]) + } else if unsigned.starts_with('0') { + (8, unsigned) + } else { + (10, unsigned) + }; + let digit_count = unsigned + .bytes() + .take_while(|byte| match base { + 8 => matches!(byte, b'0'..=b'7'), + 10 => byte.is_ascii_digit(), + 16 => byte.is_ascii_hexdigit(), + _ => false, + }) + .count(); + if digit_count == 0 { + return None; + } + let (digits, suffix) = unsigned.split_at(digit_count); + let factor = if suffix.is_empty() { + 1_i128 + } else if suffix.eq_ignore_ascii_case("k") { + 1024 + } else if suffix.eq_ignore_ascii_case("m") { + 1024 * 1024 + } else if suffix.eq_ignore_ascii_case("g") { + 1024 * 1024 * 1024 + } else { + return None; + }; + let magnitude = i128::from_str_radix(digits, base).ok()?; + let signed = if negative { -magnitude } else { magnitude }; + let value = signed.checked_mul(factor)?; + (minimum..=i128::from(i32::MAX)) + .contains(&value) + .then_some(value != 0) +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub(crate) enum GitConfigScope { Unknown, @@ -77,18 +155,9 @@ pub(crate) fn parse_config_entries(output: &[u8]) -> io::Result io::Result> { - Ok(parse_config_entries_with_origins(output)? - .into_iter() - .map(|entry| (entry.key.clone(), entry)) - .collect()) -} - +/// precedence, so duplicate include directives remain ordered. Scope is +/// retained as best-effort metadata only; command-line entries remain +/// distinguishable. pub(crate) fn parse_config_entries_with_origins(output: &[u8]) -> io::Result> { if output.is_empty() { return Ok(Vec::new()); @@ -130,6 +199,16 @@ pub(crate) fn parse_config_entries_with_origins(output: &[u8]) -> io::Result io::Result> { + Ok(parse_config_entries_with_origins(output)? + .into_iter() + .map(|entry| (entry.key.clone(), entry)) + .collect()) +} + pub(crate) fn read_effective_config_entries_with_fallback( git: &GitRunner, cwd: &Path, 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 31cea3941f..4ee758ce87 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 @@ -7,6 +7,7 @@ use super::path_safety::CONFIG_PATH_KEY; use super::path_safety::git_var_path_from_bytes; use super::path_safety::invalid_config_source; use crate::git_command::GitRunner; +use crate::git_config::parse_git_boolean_symmetric_i32; pub(super) fn default_system_config_source_candidates( git: &GitRunner, @@ -259,19 +260,8 @@ fn git_env_bool(git: &GitRunner, name: &str) -> io::Result { let value = value .to_str() .ok_or_else(|| invalid_config_source("non-UTF-8 Git boolean environment value"))?; - match value.to_ascii_lowercase().as_str() { - "1" | "true" | "yes" | "on" => Ok(true), - "" | "0" | "false" | "no" | "off" => Ok(false), - value => value - .parse::() - .ok() - // Supported Git releases disagree on whether INT_MIN is a valid - // numeric boolean. Accept only their shared symmetric range so a - // value approved here cannot make the selected Git fail later. - .filter(|value| (-i64::from(i32::MAX)..=i64::from(i32::MAX)).contains(value)) - .map(|value| value != 0) - .ok_or_else(|| invalid_config_source("invalid Git boolean environment value")), - } + parse_git_boolean_symmetric_i32(value.as_bytes()) + .ok_or_else(|| invalid_config_source("invalid Git boolean environment value")) } #[cfg(windows)] diff --git a/codex-rs/git-utils/src/git_config_sources_tests.rs b/codex-rs/git-utils/src/git_config_sources_tests.rs index 49a37e267f..e44696f883 100644 --- a/codex-rs/git-utils/src/git_config_sources_tests.rs +++ b/codex-rs/git-utils/src/git_config_sources_tests.rs @@ -402,9 +402,8 @@ fn rejects_worktree_fifo_primary_source_without_opening_it() { } #[test] -fn git_config_nosystem_accepts_cross_version_boolean_values() { - const TEST_NAME: &str = - "git_config_sources::tests::git_config_nosystem_accepts_cross_version_boolean_values"; +fn git_config_nosystem_uses_the_shared_parser_with_a_cross_version_numeric_subset() { + const TEST_NAME: &str = "git_config_sources::tests::git_config_nosystem_uses_the_shared_parser_with_a_cross_version_numeric_subset"; if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { let repo = init_repo(); let unsafe_system = repo.path().join("system.gitconfig"); @@ -415,6 +414,11 @@ fn git_config_nosystem_accepts_cross_version_boolean_values() { ("-1", "ignored"), ("01", "ignored"), ("+1", "ignored"), + ("0x1", "ignored"), + ("010", "ignored"), + ("1k", "ignored"), + ("-1g", "ignored"), + (" 1", "ignored"), ("2147483647", "ignored"), ("-2147483647", "ignored"), ("true", "ignored"), @@ -427,9 +431,17 @@ fn git_config_nosystem_accepts_cross_version_boolean_values() { ("no", "rejected"), ("off", "rejected"), ("not-a-bool", "invalid"), + ("08", "invalid"), ("2147483648", "invalid"), ("-2147483648", "invalid"), + ("-0x80000000", "invalid"), + ("-020000000000", "invalid"), + ("-2097152k", "invalid"), + ("-2048m", "invalid"), + ("-2g", "invalid"), + (" -2G", "invalid"), ("-2147483649", "invalid"), + ("2g", "invalid"), ] { run_isolated_source_test( TEST_NAME, diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs index 77a055997a..310343688b 100644 --- a/codex-rs/git-utils/src/git_config_tests.rs +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -28,6 +28,105 @@ command\0command line:\0merge.Name.driver\nhelper %A %B\0"; ); } +#[test] +fn git_boolean_accepts_native_git_spellings() { + for (value, expected) in [ + (b"true".as_slice(), true), + (b"TRUE".as_slice(), true), + (b"yes".as_slice(), true), + (b"On".as_slice(), true), + (b"1".as_slice(), true), + (b"2".as_slice(), true), + (b"-1".as_slice(), true), + (b"+1".as_slice(), true), + (b"01".as_slice(), true), + (b"0x1".as_slice(), true), + (b"0Xf".as_slice(), true), + (b"010".as_slice(), true), + (b"1k".as_slice(), true), + (b"1M".as_slice(), true), + (b"1g".as_slice(), true), + (b" 1".as_slice(), true), + (b"2147483647".as_slice(), true), + (b"-2147483648".as_slice(), true), + (b"-2g".as_slice(), true), + (b"false".as_slice(), false), + (b"FALSE".as_slice(), false), + (b"no".as_slice(), false), + (b"Off".as_slice(), false), + (b"".as_slice(), false), + (b"0".as_slice(), false), + (b"-0".as_slice(), false), + (b"+0".as_slice(), false), + (b"00".as_slice(), false), + (b"0x0".as_slice(), false), + (b"0k".as_slice(), false), + ] { + assert_eq!(parse_git_boolean(value), Some(expected), "value {value:?}"); + } +} + +#[test] +fn git_boolean_rejects_values_native_git_rejects() { + for value in [ + b"08".as_slice(), + b"-08".as_slice(), + b"2147483648".as_slice(), + b"-2147483649".as_slice(), + b"9223372036854775807".as_slice(), + b"2g".as_slice(), + b"-3g".as_slice(), + b"0x".as_slice(), + b"0b1".as_slice(), + b"1kb".as_slice(), + b"1foo".as_slice(), + b"1_0".as_slice(), + b"1 ".as_slice(), + b" ".as_slice(), + b"+".as_slice(), + b"-".as_slice(), + b"not-a-bool".as_slice(), + b"\xff".as_slice(), + ] { + assert_eq!(parse_git_boolean(value), None, "value {value:?}"); + } +} + +#[test] +fn symmetric_git_boolean_parser_excludes_only_int_min_spellings() { + for value in [ + b"-2147483648".as_slice(), + b"-0x80000000".as_slice(), + b"-020000000000".as_slice(), + b"-2097152k".as_slice(), + b"-2048m".as_slice(), + b"-2g".as_slice(), + b" -2G".as_slice(), + ] { + assert_eq!(parse_git_boolean(value), Some(true), "value {value:?}"); + assert_eq!( + parse_git_boolean_symmetric_i32(value), + None, + "value {value:?}" + ); + } + + for value in [ + b"0x1".as_slice(), + b"010".as_slice(), + b"1k".as_slice(), + b"-1g".as_slice(), + b" 1".as_slice(), + b"-2147483647".as_slice(), + ] { + assert_eq!( + parse_git_boolean_symmetric_i32(value), + parse_git_boolean(value), + "value {value:?}" + ); + } +} + #[test] fn entry_parsers_preserve_duplicate_include_directives_in_order() { let scoped = b"local\0file:.git/config\0include.path\n../unsafe.gitconfig\0\ @@ -80,7 +179,7 @@ fn rejects_malformed_config_records() { b"global\0\0key\nvalue\0".as_slice(), b"global\0file:/tmp/config\0\nvalue\0".as_slice(), ] { - assert!(parse_effective_config(output).is_err(), "{output:?}"); + assert!(parse_config_entries(output).is_err(), "{output:?}"); } } diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 3bab04c74a..f09e027c75 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -4,12 +4,9 @@ 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::git_command::GitRunner; use crate::git_config::path_is_within; -use crate::git_config_sources::ensure_no_worktree_config_sources; use crate::guarded_config::GuardedGitConfig; /// Extract effective patch paths through a bound operation configuration. @@ -23,24 +20,6 @@ pub(crate) fn extract_effective_paths_from_patch_guarded( normalize_effective_patch_paths(forward_paths, reverse_paths) } -/// Extract paths with Git from a cwd whose config sources have already been -/// authorized for `git_config_args`. -pub(crate) fn extract_effective_paths_from_patch( - git: &GitRunner, - authorized_cwd: &Path, - 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)?; - normalize_effective_patch_paths(forward_paths, reverse_paths) -} - fn normalize_effective_patch_paths( forward_paths: Vec, reverse_paths: Vec, @@ -109,42 +88,14 @@ fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec 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, &[])?; - 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( @@ -287,51 +238,16 @@ 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)?; + let requested_cwd = std::fs::canonicalize(git_root)?; + let canonical_root = crate::get_git_repo_root(&requested_cwd) + .ok_or_else(|| io::Error::other("not a Git repository")) + .and_then(std::fs::canonicalize)?; + let 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) -} - -pub(crate) fn stage_effective_paths( - git: &GitRunner, - git_root: &Path, - paths: &[String], - git_config_args: &[String], -) -> io::Result<()> { - let confined = confine_patch_paths(git, git_root, paths)?; - let mut existing = Vec::new(); - for path in confined.into_exact_leaves()? { - let joined = git_root.join(&path); - if let Ok(metadata) = std::fs::symlink_metadata(&joined) { - if leaf_is_traversable_directory(metadata.file_type()) { - return Err(containment_error( - "refusing to recursively stage a directory patch path", - )); - } - existing.push(path); - } - } - if existing.is_empty() { - return Ok(()); - } - let mut args = vec![ - "--literal-pathspecs".to_string(), - "add".to_string(), - "--".to_string(), - ]; - args.extend(existing); - let (_code, _, _) = run_git(git, git_root, git_config_args, &args)?; - // We do not hard fail staging; best-effort is OK. Return Ok even on non-zero. - Ok(()) + stage_effective_paths_guarded(&config, &paths) } /// Preserve the existing reverse staging behavior while constructing every @@ -449,14 +365,14 @@ 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 metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; - confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs) + let config = GuardedGitConfig::authorize(git, git_root, Vec::new())?; + confine_patch_paths_guarded(&config, paths) } fn confine_patch_paths_guarded( @@ -604,39 +520,6 @@ 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(); - let queries = [ - vec!["rev-parse".to_string(), "--absolute-git-dir".to_string()], - vec!["rev-parse".to_string(), "--git-common-dir".to_string()], - ]; - 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 { - return Err(io::Error::other(format!( - "failed to resolve Git repository metadata (exit {code}): {}", - stderr.trim() - ))); - } - let path = stdout.trim_end_matches(['\r', '\n']); - if path.is_empty() { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - "Git returned an empty repository metadata path", - )); - } - let path = PathBuf::from(path); - let absolute = if path.is_absolute() { - path - } else { - git_root.join(path) - }; - metadata_dirs.insert(std::fs::canonicalize(absolute)?); - } - Ok(metadata_dirs.into_iter().collect()) -} - fn canonical_git_metadata_dirs_guarded(config: &GuardedGitConfig<'_>) -> io::Result> { let queries = [ ["rev-parse", "--absolute-git-dir"], diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 8f72e3841e..acff91dce9 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -68,8 +68,8 @@ fn effective_paths(diff: &str, revert: bool) -> 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) } diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index cbdd194ce9..db55a32a9e 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -2,6 +2,8 @@ use std::io; use std::path::Path; use std::path::PathBuf; +use same_file::Handle; + use crate::errors::GitReadError; use crate::git_config::path_is_within; use crate::path_authority::RepositoryRouteBoundaries; @@ -29,6 +31,7 @@ mod policy; #[derive(Debug)] pub(crate) struct RepositoryAuthority { active_worktree_root: PathBuf, + active_worktree_identity: Option, roots: Vec, worktree_roots: Vec, common_dirs: Vec, @@ -57,8 +60,11 @@ impl RepositoryAuthority { let worktree_root = crate::get_git_repo_root(&canonical_cwd) .and_then(|root| std::fs::canonicalize(root).ok()) .unwrap_or_else(|| canonical_cwd.clone()); + let active_worktree_identity = Handle::from_path(&worktree_root) + .map_err(|error| invalid_metadata(&worktree_root, error))?; let mut authority = Self { active_worktree_root: worktree_root.clone(), + active_worktree_identity: Some(active_worktree_identity), roots: Vec::new(), worktree_roots: Vec::new(), common_dirs: Vec::new(), @@ -116,6 +122,7 @@ impl RepositoryAuthority { } pub(crate) fn revalidate_active_repository_metadata(&self) -> io::Result<()> { + self.revalidate_active_worktree_identity()?; let Some(expected) = &self.active_metadata else { return Ok(()); }; @@ -150,13 +157,17 @@ 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)?; + let same_root = if let Some(expected) = &self.active_worktree_identity { + Handle::from_path(&canonical_root)? == *expected + } else { + 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 {}", + "guarded Git repository identity {} does not match runner repository {}", canonical_root.display(), self.active_worktree_root.display() ), @@ -165,6 +176,25 @@ impl RepositoryAuthority { Ok(()) } + fn revalidate_active_worktree_identity(&self) -> io::Result<()> { + let Some(expected) = &self.active_worktree_identity else { + return Ok(()); + }; + let actual = Handle::from_path(&self.active_worktree_root).map_err(|error| { + io::Error::new( + io::ErrorKind::PermissionDenied, + format!("guarded Git repository identity could not be revalidated: {error}"), + ) + })?; + if &actual != expected { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "guarded Git repository identity changed before Git launch", + )); + } + Ok(()) + } + #[cfg(test)] pub(crate) fn from_test_locations( roots: Vec, @@ -173,12 +203,15 @@ impl RepositoryAuthority { ) -> Result { let route_boundaries = repository_route_boundaries(&worktree_roots, &common_dirs) .map_err(|error| invalid_metadata(Path::new(""), error))?; + let active_worktree_root = worktree_roots + .first() + .or_else(|| roots.first()) + .cloned() + .unwrap_or_default(); + let active_worktree_identity = Handle::from_path(&active_worktree_root).ok(); Ok(Self { - active_worktree_root: worktree_roots - .first() - .or_else(|| roots.first()) - .cloned() - .unwrap_or_default(), + active_worktree_root, + active_worktree_identity, roots, worktree_roots, common_dirs, diff --git a/codex-rs/git-utils/src/repository_authority/plain_config.rs b/codex-rs/git-utils/src/repository_authority/plain_config.rs index b3a4253b77..1464ac9095 100644 --- a/codex-rs/git-utils/src/repository_authority/plain_config.rs +++ b/codex-rs/git-utils/src/repository_authority/plain_config.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use super::bytes_to_path; use super::read_bounded_file; +use crate::git_config::parse_git_boolean; #[derive(Debug, Eq, PartialEq)] pub(crate) enum CommonConfigAuthority { @@ -50,7 +51,7 @@ pub(crate) fn inspect_plain_common_config_authority( return Ok(CommonConfigAuthority::Unproven); } let bare = match unique_explicit_value(&config, "core", "bare") { - Ok(Some(value)) => match explicit_boolean(value.as_ref()) { + Ok(Some(value)) => match parse_git_boolean(value.as_ref()) { Some(value) => Some(value), None => return Ok(CommonConfigAuthority::Unproven), }, @@ -58,7 +59,7 @@ pub(crate) fn inspect_plain_common_config_authority( Err(()) => return Ok(CommonConfigAuthority::Unproven), }; match unique_explicit_value(&config, "extensions", "worktreeConfig") { - Ok(Some(value)) if explicit_boolean(value.as_ref()) == Some(false) => {} + Ok(Some(value)) if parse_git_boolean(value.as_ref()) == Some(false) => {} Ok(None) => {} Ok(Some(_)) | Err(()) => return Ok(CommonConfigAuthority::Unproven), } @@ -99,39 +100,29 @@ fn unique_explicit_value( let Some(sections) = config.sections_by_name(section_name) else { return Ok(None); }; - let mut occurrences = 0usize; - let mut explicit_values = Vec::new(); + let mut occurrence = None; for section in sections { if section.header().subsection_name().is_some() { return Err(()); } - occurrences += section - .value_names() - .filter(|name| { - let name: &str = name.as_ref(); - name.eq_ignore_ascii_case(value_name) - }) - .count(); - explicit_values.extend( - section - .values(value_name) - .into_iter() - .map(|value| value.as_ref().to_vec()), - ); + for name in section.value_names() { + let name: &str = name.as_ref(); + if !name.eq_ignore_ascii_case(value_name) { + continue; + } + if occurrence.is_some() { + return Err(()); + } + let value = section + .value_implicit(name) + .ok_or(())? + .map(|value| value.as_ref().to_vec()); + occurrence = Some(value); + } } - match (occurrences, explicit_values.len()) { - (0, 0) => Ok(None), - (1, 1) => Ok(explicit_values.pop()), - _ => Err(()), - } -} - -fn explicit_boolean(value: &[u8]) -> Option { - if value.eq_ignore_ascii_case(b"true") { - Some(true) - } else if value.eq_ignore_ascii_case(b"false") { - Some(false) - } else { - None + match occurrence { + None => Ok(None), + Some(Some(value)) => Ok(Some(value)), + Some(None) => Err(()), } } diff --git a/codex-rs/git-utils/src/repository_authority_tests.rs b/codex-rs/git-utils/src/repository_authority_tests.rs index 0447fa0d82..9f07340939 100644 --- a/codex-rs/git-utils/src/repository_authority_tests.rs +++ b/codex-rs/git-utils/src/repository_authority_tests.rs @@ -81,6 +81,36 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup CommonConfigAuthority::Bare, Some(true), ), + ( + "yes", + b"[core]\n\tbare = yes\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "on", + b"[core]\n\tbare = on\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "numeric true", + b"[core]\n\tbare = 1\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "hex true", + b"[core]\n\tbare = 0x1\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), + ( + "suffixed true", + b"[core]\n\tbare = 1k\n".as_slice(), + CommonConfigAuthority::Bare, + Some(true), + ), ( "false", b"[core]\n\tbare = false\n".as_slice(), @@ -133,13 +163,13 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup } #[test] -fn common_bare_proof_accepts_only_explicit_boolean_literals() { +fn common_bare_proof_rejects_invalid_git_boolean_values() { for (name, body) in [ - ("empty", b"[core]\n\tbare =\n".as_slice()), - ("yes", b"[core]\n\tbare = yes\n".as_slice()), - ("on", b"[core]\n\tbare = on\n".as_slice()), - ("one", b"[core]\n\tbare = 1\n".as_slice()), ("leading zero", b"[core]\n\tbare = 08\n".as_slice()), + ( + "negative leading zero", + b"[core]\n\tbare = -08\n".as_slice(), + ), ( "positive overflow", b"[core]\n\tbare = 2147483648\n".as_slice(), @@ -152,6 +182,8 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() { "i64 max", b"[core]\n\tbare = 9223372036854775807\n".as_slice(), ), + ("suffix overflow", b"[core]\n\tbare = 2g\n".as_slice()), + ("trailing junk", b"[core]\n\tbare = 1foo\n".as_slice()), ] { let common = write_common_config(body); assert_eq!( @@ -162,6 +194,34 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() { } } +#[test] +fn common_bare_proof_accepts_explicit_false_worktree_config_spellings() { + for value in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] { + let common = write_common_config( + format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(value), + CommonConfigAuthority::Bare, + "authority result for worktreeConfig={value:?}" + ); + } +} + +#[test] +fn common_bare_proof_rejects_explicit_true_worktree_config_spellings() { + for value in ["true", "yes", "on", "1", "-1", "0x1", "1k"] { + let common = write_common_config( + format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(value), + CommonConfigAuthority::Unproven, + "authority result for worktreeConfig={value:?}" + ); + } +} + #[test] fn common_bare_proof_rejects_includes_worktree_config_and_relative_worktree() { let include_target = tempfile::NamedTempFile::new().expect("included config"); @@ -226,6 +286,25 @@ fn common_config_absolute_worktree_is_returned_as_authority() { ); } +#[test] +fn common_config_false_bare_spellings_allow_absolute_worktree() { + let worktree = tempfile::tempdir().expect("worktree"); + for bare in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] { + let common = write_common_config( + format!( + "[core]\n\tbare = {bare}\n\tworktree = {}\n", + git_config_path(worktree.path()) + ) + .as_bytes(), + ); + assert_eq!( + inspect_plain_common_config_authority(common.path()).expect(bare), + CommonConfigAuthority::Worktree(worktree.path().to_path_buf()), + "authority result for core.bare={bare:?}" + ); + } +} + #[test] fn common_config_rejects_contradictory_or_malformed_bare_with_absolute_worktree() { let worktree = tempfile::tempdir().expect("worktree");