From 6b40c77a0f7ff4679e34282ba5de0ac7f5a5e889 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 15:55:43 -0700 Subject: [PATCH 1/5] fix(git-utils): parse Git boolean syntax consistently --- codex-rs/git-utils/src/git_config.rs | 65 ++++++++++++++ codex-rs/git-utils/src/git_config_tests.rs | 64 +++++++++++++ .../src/repository_authority/plain_config.rs | 53 +++++------ .../src/repository_authority_tests.rs | 89 +++++++++++++++++-- 4 files changed, 235 insertions(+), 36 deletions(-) diff --git a/codex-rs/git-utils/src/git_config.rs b/codex-rs/git-utils/src/git_config.rs index 236ab89ef7..92496132a1 100644 --- a/codex-rs/git-utils/src/git_config.rs +++ b/codex-rs/git-utils/src/git_config.rs @@ -1,6 +1,71 @@ use std::path::Component; use std::path::Path; +pub(crate) fn parse_git_boolean(value: &[u8]) -> 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)?; + (i128::from(i32::MIN)..=i128::from(i32::MAX)) + .contains(&value) + .then_some(value != 0) +} + pub(crate) fn path_is_within(path: &Path, root: &Path) -> bool { let mut path_components = path.components(); for root_component in root.components() { diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs index 3795caf2f8..825d9a597f 100644 --- a/codex-rs/git-utils/src/git_config_tests.rs +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -1,5 +1,69 @@ use super::*; +#[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 path_containment_uses_component_boundaries() { let root = Path::new("/repo/root"); 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"); From 21ca43303bd71d79082d8122e9d37978b00fc03a Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 18:29:53 -0700 Subject: [PATCH 2/5] fix(git): bind configuration environment --- codex-rs/git-utils/src/errors.rs | 6 +- codex-rs/git-utils/src/git_command.rs | 9 ++ codex-rs/git-utils/src/git_command_tests.rs | 95 ++++++++++++ .../git-utils/src/git_config_environment.rs | 136 ++++++++++++++++++ .../src/git_config_environment_tests.rs | 126 ++++++++++++++++ codex-rs/git-utils/src/lib.rs | 1 + 6 files changed, 372 insertions(+), 1 deletion(-) create mode 100644 codex-rs/git-utils/src/git_config_environment.rs create mode 100644 codex-rs/git-utils/src/git_config_environment_tests.rs 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/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 924ed32385..3cac863d21 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use std::process::Command; 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; @@ -30,6 +31,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`], @@ -84,6 +86,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 } } @@ -117,12 +120,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 2679f744fc..868ad6c097 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, @@ -396,6 +485,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/lib.rs b/codex-rs/git-utils/src/lib.rs index 9309328e0b..e4dca00166 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -6,6 +6,7 @@ mod errors; mod fsmonitor; mod git_command; mod git_config; +mod git_config_environment; mod git_executable; mod info; mod local_only; From 3e1688f2cb8eff2efabb92f7e17ea46909cb7bf1 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 18:32:25 -0700 Subject: [PATCH 3/5] fix(git): authorize primary config sources --- codex-rs/git-utils/src/apply.rs | 14 +- codex-rs/git-utils/src/git_command.rs | 18 + codex-rs/git-utils/src/git_command_tests.rs | 109 ++++ codex-rs/git-utils/src/git_config.rs | 15 +- codex-rs/git-utils/src/git_config_sources.rs | 59 ++ .../src/git_config_sources/path_safety.rs | 38 ++ .../src/git_config_sources/primary_sources.rs | 205 ++++++ .../git-utils/src/git_config_sources_tests.rs | 611 ++++++++++++++++++ codex-rs/git-utils/src/git_config_tests.rs | 35 + codex-rs/git-utils/src/lib.rs | 1 + codex-rs/git-utils/src/patch_paths.rs | 56 +- codex-rs/git-utils/src/patch_paths_tests.rs | 20 +- codex-rs/git-utils/src/path_authority.rs | 2 +- .../repository_authority/authority/policy.rs | 33 + 14 files changed, 1190 insertions(+), 26 deletions(-) create mode 100644 codex-rs/git-utils/src/git_config_sources.rs create mode 100644 codex-rs/git-utils/src/git_config_sources/path_safety.rs create mode 100644 codex-rs/git-utils/src/git_config_sources/primary_sources.rs create mode 100644 codex-rs/git-utils/src/git_config_sources_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 2b9d76e959..5ba64abde2 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -13,6 +13,7 @@ use std::path::PathBuf; use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; +use crate::git_config_sources::ensure_no_worktree_primary_config_sources; use crate::patch_paths::extract_effective_paths_from_patch; use crate::patch_paths::stage_effective_paths; use crate::safe_git::DISABLED_HOOKS_PATH; @@ -47,17 +48,24 @@ pub struct ApplyGitResult { pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let git = GitRunner::for_cwd_io(&req.cwd)?; let mut cfg_parts = configured_git_config_parts(); + ensure_no_worktree_primary_config_sources(&git, &req.cwd)?; + 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_primary_config_sources(&git, &git_root)?; + } // 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, &patch_path, req.revert)?; + let patch_paths = + extract_effective_paths_from_patch(&git, &git_root, &patch_path, req.revert, &cfg_parts)?; + cfg_parts.extend(safe_git_config_parts()); if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths)?; + stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; } // Build git args @@ -66,8 +74,6 @@ pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { args.push("-R".into()); } - cfg_parts.extend(safe_git_config_parts()); - args.push(patch_path.to_string_lossy().to_string()); // Optional preflight: dry-run only; do not modify working tree diff --git a/codex-rs/git-utils/src/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 3cac863d21..9d3c6af66e 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -59,6 +59,11 @@ impl GitCommand { self.inner.env(key, value); self } + + pub(crate) fn env_remove(&mut self, key: impl AsRef) -> &mut Self { + self.inner.env_remove(key); + self + } } impl GitRunner { @@ -102,6 +107,19 @@ impl GitRunner { Ok(command) } + pub(crate) fn ensure_config_source_is_not_worktree_controlled( + &self, + path: &Path, + description: &str, + ) -> io::Result<()> { + self.authority + .ensure_config_source_is_not_worktree_controlled(path, description) + } + + 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); diff --git a/codex-rs/git-utils/src/git_command_tests.rs b/codex-rs/git-utils/src/git_command_tests.rs index 868ad6c097..3d191735b3 100644 --- a/codex-rs/git-utils/src/git_command_tests.rs +++ b/codex-rs/git-utils/src/git_command_tests.rs @@ -242,6 +242,16 @@ fn locations_for_root(root: &Path) -> RepositoryAuthority { .expect("test repository authority") } +fn config_authority_for_root(root: &Path) -> RepositoryAuthority { + let mut roots = vec![root.to_path_buf()]; + let canonical = std::fs::canonicalize(root).expect("canonical root"); + if !roots.contains(&canonical) { + roots.push(canonical); + } + RepositoryAuthority::from_test_locations(roots.clone(), roots, vec![root.join(".git")]) + .expect("config repository authority") +} + fn raw_parent_traversal(root: &Path, sibling: &str) -> PathBuf { let separator = std::path::MAIN_SEPARATOR.to_string(); let mut path = root.as_os_str().to_os_string(); @@ -381,6 +391,105 @@ fn git_metadata_marker_parser_preserves_leading_path_space() { assert!(parse_git_marker_path(b"gitdir:/missing-space\n", b"gitdir: ").is_err()); } +#[test] +fn config_source_authority_requires_an_absolute_raw_path() { + let fixture = tempdir_for_native_git(); + let root = fixture.path().join("repo"); + std::fs::create_dir_all(&root).expect("create repository"); + run_git(&root, &["init", "-q"]); + let authority = config_authority_for_root(&root); + let path = Path::new("relative/config"); + + let error = authority + .ensure_config_source_is_not_worktree_controlled(path, "test config") + .expect_err("relative config path"); + + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert!(error.to_string().contains("test config"), "{error}"); + assert!(error.to_string().contains("relative/config"), "{error}"); +} + +#[test] +fn config_source_authority_preserves_and_rejects_a_worktree_crossing_spelling() { + let fixture = tempdir_for_native_git(); + let root = fixture.path().join("repo"); + let safe = fixture.path().join("safe"); + std::fs::create_dir_all(root.join("nested")).expect("create repository descendant"); + std::fs::create_dir_all(&safe).expect("create safe directory"); + run_git(&root, &["init", "-q"]); + let authority = config_authority_for_root(&root); + let raw = root + .join("nested") + .join("..") + .join("..") + .join("safe/config"); + + let error = authority + .ensure_config_source_is_not_worktree_controlled(&raw, "crossing config") + .expect_err("worktree-crossing config path"); + + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert!(error.to_string().contains("crossing config"), "{error}"); + assert!( + error.to_string().contains(&raw.display().to_string()), + "{error}" + ); +} + +#[test] +fn config_source_authority_allows_protected_metadata_and_unrelated_external_paths() { + let fixture = tempdir_for_native_git(); + let root = fixture.path().join("repo"); + let external = fixture.path().join("external/config"); + std::fs::create_dir_all(&root).expect("create repository"); + std::fs::create_dir_all(external.parent().expect("external parent")) + .expect("create external directory"); + run_git(&root, &["init", "-q"]); + let authority = config_authority_for_root(&root); + + authority + .ensure_config_source_is_not_worktree_controlled( + &root.join(".git/config"), + "protected metadata config", + ) + .expect("protected metadata config"); + let canonical_config = + std::fs::canonicalize(root.join(".git/config")).expect("canonical metadata config"); + authority + .ensure_config_source_is_not_worktree_controlled( + &canonical_config, + "canonical protected metadata config", + ) + .expect("canonical protected metadata config"); + authority + .ensure_config_source_is_not_worktree_controlled(&external, "external config") + .expect("unrelated external config"); +} + +#[test] +fn config_source_authority_rejects_an_unregistered_related_repository_root() { + let fixture = tempdir_for_native_git(); + let root = fixture.path().join("repo"); + let alias = fixture.path().join("unregistered-related-root"); + std::fs::create_dir_all(&root).expect("create repository"); + std::fs::create_dir_all(&alias).expect("create related root"); + run_git(&root, &["init", "-q"]); + std::fs::write( + alias.join(".git"), + format!("gitdir: {}\n", root.join(".git").display()), + ) + .expect("write related metadata marker"); + let authority = config_authority_for_root(&root); + let path = alias.join("config"); + + let error = authority + .ensure_config_source_is_not_worktree_controlled(&path, "related config") + .expect_err("unregistered related repository config"); + + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert!(error.to_string().contains("related config"), "{error}"); +} + fn selected_git(locations: &RepositoryAuthority, directories: &[&Path]) -> PathBuf { let search_path = std::env::join_paths(directories).expect("PATH"); select_git_executable(locations, &search_path) diff --git a/codex-rs/git-utils/src/git_config.rs b/codex-rs/git-utils/src/git_config.rs index 92496132a1..737d3c433b 100644 --- a/codex-rs/git-utils/src/git_config.rs +++ b/codex-rs/git-utils/src/git_config.rs @@ -2,6 +2,19 @@ use std::path::Component; use std::path::Path; 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") @@ -61,7 +74,7 @@ pub(crate) fn parse_git_boolean(value: &[u8]) -> Option { let magnitude = i128::from_str_radix(digits, base).ok()?; let signed = if negative { -magnitude } else { magnitude }; let value = signed.checked_mul(factor)?; - (i128::from(i32::MIN)..=i128::from(i32::MAX)) + (minimum..=i128::from(i32::MAX)) .contains(&value) .then_some(value != 0) } diff --git a/codex-rs/git-utils/src/git_config_sources.rs b/codex-rs/git-utils/src/git_config_sources.rs new file mode 100644 index 0000000000..b539ef266c --- /dev/null +++ b/codex-rs/git-utils/src/git_config_sources.rs @@ -0,0 +1,59 @@ +use std::io; +use std::path::Path; + +use crate::git_command::GitRunner; + +mod path_safety; +mod primary_sources; + +use path_safety::normalize_absolute_path; +use path_safety::resolve_literal_path; +use primary_sources::is_disabled_primary_config_path; +use primary_sources::legacy_primary_config_source_candidates; +use primary_sources::selected_git_home_config_candidates; +use primary_sources::selected_git_prefix_system_candidate; + +/// Reject primary Git configuration paths selected from worktree-controlled +/// routes before any ordinary Git command can open them. +/// +/// This layer intentionally does not authorize includes. The complete include +/// graph is added by the next stack layer before this is promoted to a full +/// config-source capability. +pub(crate) fn ensure_no_worktree_primary_config_sources( + git: &GitRunner, + git_root: &Path, +) -> io::Result<()> { + let git_root = normalize_absolute_path(std::fs::canonicalize(git_root)?)?; + + for (description, candidate) in legacy_primary_config_source_candidates(git)? { + if !is_disabled_primary_config_path(&candidate) { + reject_source(git, &git_root, &candidate, description)?; + } + } + for candidate in selected_git_home_config_candidates(git, &git_root)? { + reject_source(git, &git_root, &candidate, "selected Git HOME config")?; + } + if let Some(candidate) = selected_git_prefix_system_candidate(git, &git_root)? { + reject_source( + git, + &git_root, + &candidate, + "selected Git prefix system config", + )?; + } + Ok(()) +} + +fn reject_source( + git: &GitRunner, + cwd: &Path, + candidate: &Path, + description: &str, +) -> io::Result<()> { + let absolute = resolve_literal_path(candidate, cwd); + git.ensure_config_source_is_not_worktree_controlled(&absolute, description) +} + +#[cfg(test)] +#[path = "git_config_sources_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/git_config_sources/path_safety.rs b/codex-rs/git-utils/src/git_config_sources/path_safety.rs new file mode 100644 index 0000000000..a2d86fd062 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_sources/path_safety.rs @@ -0,0 +1,38 @@ +use std::io; +use std::path::Path; +use std::path::PathBuf; + +use codex_utils_absolute_path::AbsolutePathBuf; + +pub(super) const CONFIG_PATH_KEY: &str = "codex.config-source.path"; + +#[cfg(unix)] +pub(super) fn git_var_path_from_bytes(path: &[u8]) -> io::Result { + use std::os::unix::ffi::OsStringExt; + + Ok(PathBuf::from(std::ffi::OsString::from_vec(path.to_vec()))) +} + +#[cfg(not(unix))] +pub(super) fn git_var_path_from_bytes(path: &[u8]) -> io::Result { + Ok(PathBuf::from(std::str::from_utf8(path).map_err(|_| { + invalid_config_source("non-UTF-8 Git config source path") + })?)) +} + +pub(super) fn resolve_literal_path(path: impl AsRef, base: &Path) -> PathBuf { + let path = path.as_ref(); + if path.is_absolute() { + path.to_path_buf() + } else { + base.join(path) + } +} + +pub(super) fn normalize_absolute_path(path: impl AsRef) -> io::Result { + Ok(AbsolutePathBuf::from_absolute_path(path)?.into_path_buf()) +} + +pub(super) fn invalid_config_source(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} 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 new file mode 100644 index 0000000000..1c5e835935 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_sources/primary_sources.rs @@ -0,0 +1,205 @@ +use std::ffi::OsStr; +use std::io; +use std::path::Path; +use std::path::PathBuf; + +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 selected_git_home_config_candidates( + git: &GitRunner, + cwd: &Path, +) -> io::Result> { + if git.config_environment_value("GIT_CONFIG_GLOBAL").is_some() { + return Ok(Vec::new()); + } + #[cfg(not(windows))] + 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 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"))?; + candidates.push(home.join(".config/git/config")); + } + Ok(candidates) +} + +pub(super) fn selected_git_prefix_system_candidate( + git: &GitRunner, + cwd: &Path, +) -> io::Result> { + 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) +} + +fn selected_git_path_candidate(git: &GitRunner, cwd: &Path, raw: &str) -> io::Result { + let tempdir = tempfile::tempdir()?; + let nonexistent_git_dir = tempdir.path().join("nonexistent-git-dir"); + let disabled_config = if cfg!(windows) { "NUL" } else { "/dev/null" }; + let mut command = git.command_for_cwd(cwd)?; + command + .env("GIT_CONFIG_GLOBAL", disabled_config) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_COUNT", "0") + .env_remove("GIT_CONFIG_PARAMETERS") + .arg("--git-dir") + .arg(&nonexistent_git_dir) + .arg("-c") + .arg(format!("{CONFIG_PATH_KEY}={raw}")) + .args([ + "config", + "--null", + "--no-includes", + "--path", + "--get", + CONFIG_PATH_KEY, + ]); + let output = git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "isolated selected Git path expansion failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let value = output + .stdout + .strip_suffix(&[0]) + .ok_or_else(|| invalid_config_source("unterminated selected Git path"))?; + if value.is_empty() || value.contains(&0) { + return Err(invalid_config_source("ambiguous selected Git path")); + } + git_var_path_from_bytes(value) +} + +pub(super) fn legacy_primary_config_source_candidates( + git: &GitRunner, +) -> io::Result> { + let mut candidates = Vec::new(); + 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(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) = git.config_environment_value("APPDATA") + && !app_data.is_empty() + { + candidates.push(( + "APPDATA Git config", + PathBuf::from(app_data).join("Git/config"), + )); + } + for home in &homes { + candidates.push(( + "HOME XDG Git config", + home_config_path(home, ".config/git/config"), + )); + } + } + } + for home in &homes { + candidates.push(("HOME Git config", home_config_path(home, ".gitconfig"))); + } + } + } + if !git_env_bool(git, "GIT_CONFIG_NOSYSTEM")? { + match git.config_environment_value("GIT_CONFIG_SYSTEM") { + Some(path) if !path.is_empty() => { + candidates.push(("GIT_CONFIG_SYSTEM", PathBuf::from(path))); + } + Some(_) => {} + None => { + #[cfg(windows)] + if let Some(program_data) = git.config_environment_value("PROGRAMDATA") + && !program_data.is_empty() + { + candidates.push(( + "PROGRAMDATA Git config", + PathBuf::from(program_data).join("Git/config"), + )); + } + } + } + } + Ok(candidates) +} + +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)) = ( + 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) = git.config_environment_value("USERPROFILE") + && !homes.iter().any(|home| home == profile) + { + homes.push(profile.to_owned()); + } + homes + } + #[cfg(not(windows))] + Vec::new() +} + +fn home_config_path(home: &std::ffi::OsStr, suffix: &str) -> PathBuf { + if home.is_empty() { + PathBuf::from(std::path::MAIN_SEPARATOR.to_string()).join(suffix) + } else { + PathBuf::from(home).join(suffix) + } +} + +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 + .to_str() + .ok_or_else(|| invalid_config_source("non-UTF-8 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)] +pub(super) fn is_disabled_primary_config_path(path: &Path) -> bool { + path.as_os_str() + .to_str() + .is_some_and(|path| path.eq_ignore_ascii_case("NUL")) +} + +#[cfg(not(windows))] +pub(super) fn is_disabled_primary_config_path(_path: &Path) -> bool { + false +} diff --git a/codex-rs/git-utils/src/git_config_sources_tests.rs b/codex-rs/git-utils/src/git_config_sources_tests.rs new file mode 100644 index 0000000000..222e9a614f --- /dev/null +++ b/codex-rs/git-utils/src/git_config_sources_tests.rs @@ -0,0 +1,611 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::ffi::OsStr; +use std::path::PathBuf; + +fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { + let mut command = std::process::Command::new("git"); + crate::safe_git::isolate_git_command_environment(&mut command); + let output = command + .args(args) + .current_dir(cwd) + .output() + .expect("run Git"); + ( + output.status.code().unwrap_or(-1), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ) +} + +fn run_success(cwd: &Path, args: &[&str]) -> String { + let (code, stdout, stderr) = run(cwd, args); + assert_eq!(code, 0, "git {args:?}: {stderr}"); + stdout.trim().to_string() +} + +fn init_repo() -> tempfile::TempDir { + let repo = tempfile::tempdir().expect("tempdir"); + init_repo_at(repo.path()); + repo +} + +fn init_repo_at(root: &Path) { + std::fs::create_dir_all(root).expect("create repository directory"); + run_success(root, &["init"]); + run_success(root, &["config", "user.email", "codex@example.com"]); + run_success(root, &["config", "user.name", "Codex"]); + std::fs::write(root.join("file.txt"), "orig\n").expect("write file"); + run_success(root, &["add", "file.txt"]); + run_success(root, &["commit", "-m", "seed"]); +} + +#[cfg(windows)] +fn create_junction(path: &Path, target: &Path) { + // Bazel's GNU Windows runner can surface temporary paths with `/` + // separators. `mklink` treats those separators as option prefixes, so + // pass native separators to the cmd.exe built-in. + let path = path.as_os_str().to_string_lossy().replace('/', "\\"); + let target = target.as_os_str().to_string_lossy().replace('/', "\\"); + let output = std::process::Command::new("cmd.exe") + .args(["/D", "/C", "mklink", "/J"]) + .arg(path) + .arg(target) + .output() + .expect("create junction"); + assert!( + output.status.success(), + "mklink failed: stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +fn guard(root: &Path) -> io::Result<()> { + let git = GitRunner::for_cwd_io(root)?; + ensure_no_worktree_primary_config_sources(&git, root) +} + +fn assert_worktree_rejection(error: io::Error) { + assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}"); + assert!(error.to_string().contains("worktree-controlled"), "{error}"); +} + +fn run_isolated_source_test(test_name: &str, env: &[(&str, &OsStr)], removed: &[&str]) { + let mut command = std::process::Command::new(std::env::current_exe().expect("test binary")); + crate::safe_git::isolate_git_command_environment(&mut command); + command + .arg(test_name) + .arg("--exact") + .arg("--nocapture") + .env("CODEX_GIT_CONFIG_SOURCE_CHILD", "1") + .env("RUST_TEST_THREADS", "1"); + for (name, value) in env { + command.env(name, value); + } + for name in removed { + command.env_remove(name); + } + let output = command.output().expect("run isolated source test"); + assert!( + output.status.success(), + "isolated test {test_name} failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn rejects_empty_primary_global_and_system_sources_in_the_worktree() { + const TEST_NAME: &str = "git_config_sources::tests::rejects_empty_primary_global_and_system_sources_in_the_worktree"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + let root = repo.path(); + let unsafe_config = root.join("empty-primary.gitconfig"); + std::fs::write(&unsafe_config, "").expect("write empty primary config"); + let safe = tempfile::NamedTempFile::new().expect("safe external config"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", root.as_os_str()), + ("GIT_CONFIG_GLOBAL", unsafe_config.as_os_str()), + ("GIT_CONFIG_SYSTEM", safe.path().as_os_str()), + ], + &[], + ); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", root.as_os_str()), + ("GIT_CONFIG_GLOBAL", safe.path().as_os_str()), + ("GIT_CONFIG_SYSTEM", unsafe_config.as_os_str()), + ], + &[], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + assert_worktree_rejection(guard(&root).expect_err("unsafe primary config source")); +} + +#[test] +fn rejects_missing_home_and_xdg_sources_in_the_worktree() { + const TEST_NAME: &str = + "git_config_sources::tests::rejects_missing_home_and_xdg_sources_in_the_worktree"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + let root = repo.path(); + let xdg = root.join("xdg"); + std::fs::create_dir(&xdg).expect("create XDG root"); + let safe_system = tempfile::NamedTempFile::new().expect("safe system config"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", root.as_os_str()), + ("HOME", root.as_os_str()), + ("XDG_CONFIG_HOME", xdg.as_os_str()), + ("GIT_CONFIG_SYSTEM", safe_system.path().as_os_str()), + ], + &["GIT_CONFIG_GLOBAL"], + ); + return; + } + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + assert_worktree_rejection(guard(&root).expect_err("unsafe HOME/XDG source")); +} + +#[test] +fn allows_explicitly_disabled_global_and_system_sources() { + const TEST_NAME: &str = + "git_config_sources::tests::allows_explicitly_disabled_global_and_system_sources"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_GLOBAL", OsStr::new("")), + ("GIT_CONFIG_SYSTEM", OsStr::new("")), + ], + &[], + ); + return; + } + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + guard(&root).expect("disabled primary config sources"); +} + +#[cfg(unix)] +#[test] +fn allows_unset_home_when_git_has_no_global_source() { + const TEST_NAME: &str = + "git_config_sources::tests::allows_unset_home_when_git_has_no_global_source"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ], + &[ + "HOME", + "XDG_CONFIG_HOME", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + ], + ); + return; + } + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + guard(&root).expect("Git without HOME or global config"); +} + +#[cfg(unix)] +#[test] +fn rejects_newline_bearing_raw_global_source_before_git_normalizes_it() { + const TEST_NAME: &str = "git_config_sources::tests::rejects_newline_bearing_raw_global_source_before_git_normalizes_it"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let parent = tempfile::tempdir().expect("fixture parent"); + let root = parent.path().join("repo"); + init_repo_at(&root); + let newline_dir = parent.path().join("external\n"); + std::fs::create_dir(&newline_dir).expect("create newline path component"); + let unsafe_config = root.join("global.gitconfig"); + std::fs::write(&unsafe_config, "").expect("write unsafe global config"); + let raw = newline_dir.join("../repo/global.gitconfig"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", root.as_os_str()), + ("GIT_CONFIG_GLOBAL", raw.as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ], + &["GIT_CONFIG_SYSTEM"], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + assert_worktree_rejection(guard(&root).expect_err("newline raw global source")); +} + +#[cfg(unix)] +#[test] +fn ignores_non_utf8_values_for_unrelated_global_keys() { + const TEST_NAME: &str = + "git_config_sources::tests::ignores_non_utf8_values_for_unrelated_global_keys"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + let global = tempfile::NamedTempFile::new().expect("external global config"); + std::fs::write(global.path(), b"[user]\nname = \xff\n") + .expect("write non-UTF-8 unrelated config value"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_GLOBAL", global.path().as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ], + &["GIT_CONFIG_SYSTEM"], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + guard(&root).expect("unrelated non-UTF-8 global value"); +} + +#[cfg(unix)] +#[test] +fn rejects_worktree_fifo_primary_source_without_opening_it() { + const TEST_NAME: &str = + "git_config_sources::tests::rejects_worktree_fifo_primary_source_without_opening_it"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + let fifo = repo.path().join("global.fifo"); + let status = std::process::Command::new("mkfifo") + .arg(&fifo) + .status() + .expect("run mkfifo"); + assert!(status.success(), "mkfifo failed: {status}"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_GLOBAL", fifo.as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ], + &["GIT_CONFIG_SYSTEM"], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + assert_worktree_rejection(guard(&root).expect_err("worktree FIFO config source")); +} + +#[test] +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"); + std::fs::write(&unsafe_system, "").expect("write unsafe system config"); + for (value, expected) in [ + ("1", "ignored"), + ("2", "ignored"), + ("-1", "ignored"), + ("01", "ignored"), + ("+1", "ignored"), + ("0x1", "ignored"), + ("010", "ignored"), + ("1k", "ignored"), + ("-1g", "ignored"), + (" 1", "ignored"), + ("2147483647", "ignored"), + ("-2147483647", "ignored"), + ("true", "ignored"), + ("yes", "ignored"), + ("on", "ignored"), + ("", "rejected"), + ("0", "rejected"), + ("-0", "rejected"), + ("false", "rejected"), + ("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, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("CODEX_GIT_CONFIG_SOURCE_EXPECTED", OsStr::new(expected)), + ("GIT_CONFIG_GLOBAL", OsStr::new("")), + ("GIT_CONFIG_SYSTEM", unsafe_system.as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new(value)), + ], + &[], + ); + } + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + match std::env::var("CODEX_GIT_CONFIG_SOURCE_EXPECTED") + .expect("expected child outcome") + .as_str() + { + "ignored" => guard(&root).expect("system source disabled"), + "rejected" => assert_worktree_rejection(guard(&root).expect_err("system source enabled")), + "invalid" => assert_eq!( + guard(&root).expect_err("invalid NOSYSTEM value").kind(), + io::ErrorKind::InvalidData + ), + expected => panic!("unexpected child outcome {expected}"), + } +} + +#[cfg(windows)] +#[test] +fn rejects_both_windows_home_candidates_when_home_is_absent() { + const TEST_NAME: &str = + "git_config_sources::tests::rejects_both_windows_home_candidates_when_home_is_absent"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + let external_profile = tempfile::tempdir().expect("external USERPROFILE"); + for home in [repo.path().to_path_buf(), repo.path().join("future-home")] { + let home = home.to_str().expect("UTF-8 Windows test path"); + assert!(home.len() >= 3 && home.as_bytes()[1] == b':', "{home}"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ("HOMEDRIVE", OsStr::new(&home[..2])), + ("HOMEPATH", OsStr::new(&home[2..])), + ("USERPROFILE", external_profile.path().as_os_str()), + ], + &[ + "HOME", + "XDG_CONFIG_HOME", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + ], + ); + } + let external_home = external_profile + .path() + .to_str() + .expect("UTF-8 external Windows home"); + assert!( + external_home.len() >= 3 && external_home.as_bytes()[1] == b':', + "{external_home}" + ); + for profile in [ + repo.path().to_path_buf(), + repo.path().join("future-profile"), + ] { + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ("HOMEDRIVE", OsStr::new(&external_home[..2])), + ("HOMEPATH", OsStr::new(&external_home[2..])), + ("USERPROFILE", profile.as_os_str()), + ], + &[ + "HOME", + "XDG_CONFIG_HOME", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + ], + ); + } + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + assert_worktree_rejection(guard(&root).expect_err("Windows synthesized HOME source")); +} + +#[cfg(windows)] +#[test] +fn allows_exact_windows_nul_for_both_primary_config_channels() { + const TEST_NAME: &str = + "git_config_sources::tests::allows_exact_windows_nul_for_both_primary_config_channels"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let repo = init_repo(); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("GIT_CONFIG_GLOBAL", OsStr::new("NUL")), + ("GIT_CONFIG_SYSTEM", OsStr::new("NUL")), + ], + &["GIT_CONFIG_NOSYSTEM"], + ); + return; + } + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + guard(&root).expect("exact NUL disables primary config channels"); +} + +#[cfg(windows)] +#[test] +fn windows_appdata_global_config_matches_native_git_selection_and_precedence() { + const TEST_NAME: &str = "git_config_sources::tests::windows_appdata_global_config_matches_native_git_selection_and_precedence"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + for xdg in [None, Some(OsStr::new(""))] { + for existing in [true, false] { + let repo = init_repo(); + let appdata = repo.path().join("appdata"); + if existing { + let config = appdata.join("Git/config"); + std::fs::create_dir_all(config.parent().expect("APPDATA config parent")) + .expect("create APPDATA config parent"); + std::fs::write(&config, "[codex]\nappdata-marker = selected\n") + .expect("write APPDATA config"); + } + let external_home = tempfile::tempdir().expect("external HOME"); + let mut env = vec![ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ( + "CODEX_GIT_CONFIG_SOURCE_EXPECTED", + OsStr::new(if existing { + "reject-existing-appdata" + } else { + "reject-missing-appdata" + }), + ), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ("HOME", external_home.path().as_os_str()), + ("APPDATA", appdata.as_os_str()), + ]; + if let Some(xdg) = xdg { + env.push(("XDG_CONFIG_HOME", xdg)); + } + let mut removed = vec![ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "HOMEDRIVE", + "HOMEPATH", + "USERPROFILE", + ]; + if xdg.is_none() { + removed.push("XDG_CONFIG_HOME"); + } + run_isolated_source_test(TEST_NAME, &env, &removed); + } + } + + let repo = init_repo(); + let appdata = repo.path().join("appdata"); + let appdata_config = appdata.join("Git/config"); + std::fs::create_dir_all(appdata_config.parent().expect("APPDATA config parent")) + .expect("create APPDATA config parent"); + std::fs::write(&appdata_config, "[codex]\nappdata-marker = appdata\n") + .expect("write APPDATA config"); + let external_home = tempfile::tempdir().expect("external HOME"); + let external_xdg = tempfile::tempdir().expect("external XDG_CONFIG_HOME"); + let xdg_config = external_xdg.path().join("git/config"); + std::fs::create_dir_all(xdg_config.parent().expect("XDG config parent")) + .expect("create XDG config parent"); + std::fs::write(&xdg_config, "[codex]\nappdata-marker = xdg\n").expect("write XDG config"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ("CODEX_GIT_CONFIG_SOURCE_EXPECTED", OsStr::new("allow-xdg")), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ("HOME", external_home.path().as_os_str()), + ("APPDATA", appdata.as_os_str()), + ("XDG_CONFIG_HOME", external_xdg.path().as_os_str()), + ], + &[ + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "HOMEDRIVE", + "HOMEPATH", + "USERPROFILE", + ], + ); + + let external_global = tempfile::NamedTempFile::new().expect("external global config"); + std::fs::write( + external_global.path(), + "[codex]\nappdata-marker = explicit\n", + ) + .expect("write explicit global config"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", repo.path().as_os_str()), + ( + "CODEX_GIT_CONFIG_SOURCE_EXPECTED", + OsStr::new("allow-explicit-global"), + ), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ("HOME", repo.path().as_os_str()), + ("APPDATA", appdata.as_os_str()), + ("XDG_CONFIG_HOME", repo.path().as_os_str()), + ("GIT_CONFIG_GLOBAL", external_global.path().as_os_str()), + ], + &["GIT_CONFIG_SYSTEM", "HOMEDRIVE", "HOMEPATH", "USERPROFILE"], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + let expected = + std::env::var("CODEX_GIT_CONFIG_SOURCE_EXPECTED").expect("expected child outcome"); + if expected != "reject-missing-appdata" { + let output = std::process::Command::new("git") + .args(["config", "--global", "--get", "codex.appdata-marker"]) + .current_dir(&root) + .output() + .expect("run native Git global config query"); + assert!( + output.status.success(), + "native Git global query failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let actual = String::from_utf8_lossy(&output.stdout); + let native_expected = match expected.as_str() { + "reject-existing-appdata" => "selected", + "allow-xdg" => "xdg", + "allow-explicit-global" => "explicit", + expected => panic!("unexpected child outcome {expected}"), + }; + assert_eq!( + actual.trim(), + native_expected, + "native Git global selection" + ); + } + match expected.as_str() { + "reject-existing-appdata" | "reject-missing-appdata" => { + assert_worktree_rejection(guard(&root).expect_err("worktree APPDATA config")); + } + "allow-xdg" => guard(&root).expect("nonempty XDG suppresses APPDATA"), + "allow-explicit-global" => { + guard(&root).expect("explicit global config suppresses HOME, XDG, and APPDATA") + } + expected => panic!("unexpected child outcome {expected}"), + } +} diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs index 825d9a597f..8f49871c03 100644 --- a/codex-rs/git-utils/src/git_config_tests.rs +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -64,6 +64,41 @@ fn git_boolean_rejects_values_native_git_rejects() { } } +#[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 path_containment_uses_component_boundaries() { let root = Path::new("/repo/root"); diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index e4dca00166..b52bf1ef19 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -7,6 +7,7 @@ mod fsmonitor; mod git_command; mod git_config; mod git_config_environment; +mod git_config_sources; mod git_executable; mod info; mod local_only; diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 168d9b448f..378a245859 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -9,16 +9,23 @@ 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_primary_config_sources; +/// 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, patch_path, revert)?; + 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, patch_path, !revert)?; + let reverse_paths = + git_apply_numstat_paths(git, authorized_cwd, patch_path, !revert, git_config_args)?; if forward_paths.len() != reverse_paths.len() { return Err(io::Error::new( io::ErrorKind::InvalidData, @@ -44,27 +51,38 @@ pub(crate) fn extract_effective_paths_from_patch( /// Security-sensitive callers must use the fallible internal extractor so an /// invalid or ambiguous patch is rejected instead of becoming an empty list. pub fn extract_paths_from_patch(diff_text: &str) -> Vec { + let Ok(cwd) = std::env::current_dir() else { + return Vec::new(); + }; + extract_paths_from_patch_from_cwd(diff_text, &cwd) +} + +fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec { let Ok((tmpdir, patch_path)) = write_temp_patch(diff_text) else { return Vec::new(); }; - let paths = std::env::current_dir() - .ok() - .and_then(|cwd| GitRunner::for_cwd(&cwd).ok()) - .and_then(|git| { - extract_effective_paths_from_patch(&git, &patch_path, /*revert*/ false).ok() - }) - .unwrap_or_default(); + let paths = (|| -> io::Result> { + let git = GitRunner::for_cwd_io(cwd)?; + 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_primary_config_sources(&git, &git_root)?; + extract_effective_paths_from_patch(&git, &git_root, &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 command_cwd = patch_path.parent().unwrap_or_else(|| Path::new(".")); - let mut cmd = git.command_for_cwd(command_cwd)?; + let mut cmd = git.command_for_cwd(authorized_cwd)?; + cmd.args(git_config_args); cmd.args(["apply", "--numstat", "-z"]); if revert { cmd.arg("-R"); @@ -226,16 +244,25 @@ 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_primary_config_sources(&git, git_root)?; let (tmpdir, patch_path) = write_temp_patch(diff)?; - let paths = extract_effective_paths_from_patch(&git, &patch_path, /*revert*/ true)?; + let paths = extract_effective_paths_from_patch( + &git, + git_root, + &patch_path, + /*revert*/ true, + &git_config_args, + )?; let _guard = tmpdir; - stage_effective_paths(&git, git_root, &paths) + 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(); @@ -259,8 +286,7 @@ pub(crate) fn stage_effective_paths( "--".to_string(), ]; args.extend(existing); - let config_parts = safe_git_config_parts(); - let (_code, _, _) = run_git(git, git_root, &config_parts, &args)?; + 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(()) } diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index e805c2d75b..4d6643a97a 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -62,13 +62,23 @@ fn read_file_normalized(path: &Path) -> String { fn effective_paths(diff: &str, revert: bool) -> io::Result> { let (tmpdir, patch_path) = write_temp_patch(diff)?; - let cwd = std::env::current_dir()?; - let git = GitRunner::for_cwd_io(&cwd)?; - let paths = extract_effective_paths_from_patch(&git, &patch_path, revert)?; + let repo = init_repo(); + let cwd = repo.path(); + let git = GitRunner::for_cwd_io(cwd)?; + 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_primary_config_sources(&git, &git_root)?; + let paths = extract_effective_paths_from_patch(&git, &git_root, &patch_path, revert, &[])?; drop(tmpdir); Ok(paths) } +fn best_effort_paths(diff: &str) -> Vec { + let repo = init_repo(); + extract_paths_from_patch_from_cwd(diff, repo.path()) +} + #[test] fn effective_paths_cover_supported_patch_headers() { let cases = [ @@ -127,7 +137,7 @@ fn effective_paths_cover_supported_patch_headers() { "{name}, revert={revert}" ); } - assert_eq!(extract_paths_from_patch(diff), expected, "{name}"); + assert_eq!(best_effort_paths(diff), expected, "{name}"); } let nul_rename_paths = parse_numstat_paths(b"0\t0\t\0old name.txt\0new name.txt\0") @@ -150,7 +160,7 @@ fn effective_paths_follow_git_for_mismatched_headers() { effective_paths(mismatch, /*revert*/ true).unwrap(), expected ); - assert_eq!(extract_paths_from_patch(mismatch), expected); + assert_eq!(best_effort_paths(mismatch), expected); } #[test] diff --git a/codex-rs/git-utils/src/path_authority.rs b/codex-rs/git-utils/src/path_authority.rs index b567df0368..1e2157c378 100644 --- a/codex-rs/git-utils/src/path_authority.rs +++ b/codex-rs/git-utils/src/path_authority.rs @@ -88,7 +88,7 @@ pub(crate) fn repository_route_boundaries( let canonical_root = std::fs::canonicalize(root)?; let canonical_marker = std::fs::canonicalize(&marker)?; if canonical_marker == canonical_root.join(".git") { - push_unique_location(&mut metadata_dirs, canonical_marker)?; + push_unique_location(&mut metadata_dirs, marker)?; } } for common in common_dirs { 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 e77afde247..e5f2379460 100644 --- a/codex-rs/git-utils/src/repository_authority/authority/policy.rs +++ b/codex-rs/git-utils/src/repository_authority/authority/policy.rs @@ -33,6 +33,29 @@ impl RepositoryAuthority { Ok(canonical) } + pub(crate) fn ensure_config_source_is_not_worktree_controlled( + &self, + path: &Path, + description: &str, + ) -> io::Result<()> { + if !path.is_absolute() { + return Err(worktree_controlled_config_source(path, description)); + } + let inspection = self.route_boundaries.inspect_route(path)?; + if inspection.crosses_worktree { + return Err(worktree_controlled_config_source(path, description)); + } + for observed in inspection.observed_paths { + if self.route_boundaries.contains_known_boundary(&observed)? { + continue; + } + if self.has_related_repository_ancestor(&observed)? { + return Err(worktree_controlled_config_source(path, description)); + } + } + Ok(()) + } + pub(super) fn path_is_untrusted_for_executable_result(&self, path: &Path) -> io::Result { let inspection = self.route_boundaries.inspect_route(path)?; if inspection.touches_worktree || inspection.crosses_metadata { @@ -145,3 +168,13 @@ impl RepositoryAuthority { Ok(()) } } + +fn worktree_controlled_config_source(path: &Path, description: &str) -> io::Error { + io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "refusing to use worktree-controlled {description}: {}", + path.display() + ), + ) +} From 9c2e1342a4a24fca84165c2c8f96cb1ac7391a99 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 18:34:27 -0700 Subject: [PATCH 4/5] fix(git): authorize included config sources --- codex-rs/git-utils/src/apply.rs | 6 +- codex-rs/git-utils/src/git_command_tests.rs | 40 + codex-rs/git-utils/src/git_config.rs | 236 +++++ codex-rs/git-utils/src/git_config_sources.rs | 95 +- .../src/git_config_sources/include_graph.rs | 120 +++ .../src/git_config_sources/path_safety.rs | 18 + .../src/git_config_sources/primary_sources.rs | 72 ++ .../git-utils/src/git_config_sources_tests.rs | 993 +++++++++++++++++- codex-rs/git-utils/src/git_config_tests.rs | 56 + codex-rs/git-utils/src/patch_paths.rs | 6 +- codex-rs/git-utils/src/patch_paths_tests.rs | 2 +- codex-rs/git-utils/src/path_authority.rs | 2 +- .../src/repository_authority/authority.rs | 39 +- 13 files changed, 1663 insertions(+), 22 deletions(-) create mode 100644 codex-rs/git-utils/src/git_config_sources/include_graph.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index 5ba64abde2..d14d0b2d4c 100644 --- a/codex-rs/git-utils/src/apply.rs +++ b/codex-rs/git-utils/src/apply.rs @@ -13,7 +13,7 @@ use std::path::PathBuf; use crate::FsmonitorOverride; use crate::apply_output::parse_git_apply_output; use crate::git_command::GitRunner; -use crate::git_config_sources::ensure_no_worktree_primary_config_sources; +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::safe_git::DISABLED_HOOKS_PATH; @@ -48,11 +48,11 @@ pub struct ApplyGitResult { pub fn apply_git_patch(req: &ApplyGitRequest) -> io::Result { let git = GitRunner::for_cwd_io(&req.cwd)?; let mut cfg_parts = configured_git_config_parts(); - ensure_no_worktree_primary_config_sources(&git, &req.cwd)?; + 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_primary_config_sources(&git, &git_root)?; + ensure_no_worktree_config_sources(&git, &git_root, &cfg_parts)?; } // Write unified diff into a temporary file 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 737d3c433b..dddab6d652 100644 --- a/codex-rs/git-utils/src/git_config.rs +++ b/codex-rs/git-utils/src/git_config.rs @@ -1,6 +1,9 @@ +use std::io; use std::path::Component; 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)) } @@ -79,6 +82,193 @@ fn parse_git_boolean_with_minimum(value: &[u8], minimum: i128) -> Option { .then_some(value != 0) } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum GitConfigScope { + Unknown, + System, + Global, + Local, + Worktree, + Command, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct GitConfigEntry { + pub(crate) scope: GitConfigScope, + pub(crate) origin: GitConfigOrigin, + pub(crate) key: String, + pub(crate) value: String, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) enum GitConfigOrigin { + CommandLine, + File(std::path::PathBuf), +} + +pub(crate) fn parse_config_entries(output: &[u8]) -> io::Result> { + if output.is_empty() { + return Ok(Vec::new()); + } + let Some(body) = output.strip_suffix(&[0]) else { + return Err(invalid_config_output("unterminated Git config output")); + }; + let fields = body.split(|byte| *byte == 0).collect::>(); + if fields.len() % 3 != 0 { + return Err(invalid_config_output("incomplete Git config record")); + } + + let mut entries = Vec::new(); + for record in fields.chunks_exact(3) { + let scope = parse_scope(record[0])?; + let origin = parse_config_origin(record[1])?; + let entry = parse_utf8_field(record[2], "config key/value")?; + let Some((key, value)) = entry.split_once('\n') else { + return Err(invalid_config_output( + "Git config record has no key/value separator", + )); + }; + if key.is_empty() { + return Err(invalid_config_output("empty Git config key")); + } + let entry = GitConfigEntry { + scope, + origin, + key: key.to_string(), + value: value.to_string(), + }; + entries.push(entry); + } + Ok(entries) +} + +/// Parse the `--show-origin` form used by Git versions that predate +/// `config --show-scope`. The record order still reflects effective config +/// 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()); + } + let Some(body) = output.strip_suffix(&[0]) else { + return Err(invalid_config_output("unterminated Git config output")); + }; + let fields = body.split(|byte| *byte == 0).collect::>(); + if fields.len() % 2 != 0 { + return Err(invalid_config_output("incomplete Git config record")); + } + + let mut entries = Vec::new(); + for record in fields.chunks_exact(2) { + let origin = parse_config_origin(record[0])?; + let entry = parse_utf8_field(record[1], "config key/value")?; + let Some((key, value)) = entry.split_once('\n') else { + return Err(invalid_config_output( + "Git config record has no key/value separator", + )); + }; + if key.is_empty() { + return Err(invalid_config_output("empty Git config key")); + } + let scope = if origin == GitConfigOrigin::CommandLine { + GitConfigScope::Command + } else { + // Older Git does not expose the scope. Selection depends on the + // emitted precedence order, not on this informational label. + GitConfigScope::Local + }; + entries.push(GitConfigEntry { + scope, + origin, + key: key.to_string(), + value: value.to_string(), + }); + } + Ok(entries) +} + +pub(crate) fn read_config_entries_without_includes( + git: &GitRunner, + cwd: &Path, + git_config_args: &[String], + pattern: &str, + probe: &str, + config_file: Option<&Path>, +) -> io::Result> { + read_config_entries_with_fallback(git, cwd, git_config_args, pattern, probe, config_file) +} + +fn read_config_entries_with_fallback( + git: &GitRunner, + cwd: &Path, + git_config_args: &[String], + pattern: &str, + probe: &str, + config_file: Option<&Path>, +) -> io::Result> { + let scoped = run_effective_config_query( + git, + cwd, + git_config_args, + pattern, + /*show_scope*/ true, + config_file, + )?; + if scoped + .status + .code() + .is_some_and(|code| code == 0 || code == 1) + { + return parse_config_entries(&scoped.stdout); + } + + let legacy = run_effective_config_query( + git, + cwd, + git_config_args, + pattern, + /*show_scope*/ false, + config_file, + )?; + if !legacy + .status + .code() + .is_some_and(|code| code == 0 || code == 1) + { + return Err(io::Error::other(format!( + "git {probe} config probe failed with status {}: {}", + legacy.status, + String::from_utf8_lossy(&legacy.stderr).trim() + ))); + } + parse_config_entries_with_origins(&legacy.stdout) +} + +fn run_effective_config_query( + git: &GitRunner, + cwd: &Path, + git_config_args: &[String], + pattern: &str, + show_scope: bool, + config_file: Option<&Path>, +) -> io::Result { + let mut command = git.command_for_cwd(cwd)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(git_config_args) + .arg("config"); + if let Some(config_file) = config_file { + command.arg("--file").arg(config_file); + } + command.arg("--null"); + if show_scope { + command.arg("--show-scope"); + } + command.args(["--show-origin", "--no-includes", "--get-regexp", pattern]); + git.output(command) +} + pub(crate) fn path_is_within(path: &Path, root: &Path) -> bool { let mut path_components = path.components(); for root_component in root.components() { @@ -104,6 +294,52 @@ fn components_equal(left: Component<'_>, right: Component<'_>) -> bool { left == right } +fn parse_scope(scope: &[u8]) -> io::Result { + match scope { + b"unknown" => Ok(GitConfigScope::Unknown), + b"system" => Ok(GitConfigScope::System), + b"global" => Ok(GitConfigScope::Global), + b"local" => Ok(GitConfigScope::Local), + b"worktree" => Ok(GitConfigScope::Worktree), + b"command" => Ok(GitConfigScope::Command), + _ => Err(invalid_config_output("unknown Git config scope")), + } +} + +fn parse_config_origin(origin: &[u8]) -> io::Result { + if origin == b"command line:" { + return Ok(GitConfigOrigin::CommandLine); + } + let path = origin + .strip_prefix(b"file:") + .ok_or_else(|| invalid_config_output("unsupported Git config origin"))?; + if path.is_empty() || path.contains(&0) { + return Err(invalid_config_output("empty Git config origin")); + } + #[cfg(unix)] + { + use std::os::unix::ffi::OsStringExt; + + Ok(GitConfigOrigin::File(std::path::PathBuf::from( + std::ffi::OsString::from_vec(path.to_vec()), + ))) + } + #[cfg(not(unix))] + { + Ok(GitConfigOrigin::File(std::path::PathBuf::from( + parse_utf8_field(path, "Git config origin path")?, + ))) + } +} + +fn parse_utf8_field<'a>(field: &'a [u8], name: &str) -> io::Result<&'a str> { + std::str::from_utf8(field).map_err(|_| invalid_config_output(&format!("non-UTF-8 {name}"))) +} + +fn invalid_config_output(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message) +} + #[cfg(test)] #[path = "git_config_tests.rs"] mod tests; diff --git a/codex-rs/git-utils/src/git_config_sources.rs b/codex-rs/git-utils/src/git_config_sources.rs index b539ef266c..bcfec7bdf1 100644 --- a/codex-rs/git-utils/src/git_config_sources.rs +++ b/codex-rs/git-utils/src/git_config_sources.rs @@ -1,30 +1,41 @@ +use std::collections::BTreeSet; use std::io; use std::path::Path; +#[cfg(test)] +use std::path::PathBuf; use crate::git_command::GitRunner; +use crate::git_config::read_config_entries_without_includes; +mod include_graph; mod path_safety; mod primary_sources; +use include_graph::validate_include_entries; use path_safety::normalize_absolute_path; use path_safety::resolve_literal_path; +use primary_sources::default_system_config_source_candidates; use primary_sources::is_disabled_primary_config_path; use primary_sources::legacy_primary_config_source_candidates; use primary_sources::selected_git_home_config_candidates; use primary_sources::selected_git_prefix_system_candidate; -/// Reject primary Git configuration paths selected from worktree-controlled -/// routes before any ordinary Git command can open them. -/// -/// This layer intentionally does not authorize includes. The complete include -/// graph is added by the next stack layer before this is promoted to a full -/// config-source capability. -pub(crate) fn ensure_no_worktree_primary_config_sources( +const INCLUDE_CONFIG_PATTERN: &str = r"^include(\.path|if\..*\.path)$"; +const MAX_CONFIG_INCLUDE_DEPTH: usize = 10; +const MAX_CONFIG_INCLUDE_FILES: usize = 1024; + +/// Reject configuration that an untrusted worktree writer can change between +/// a policy probe and the Git command it guards. +pub(crate) fn ensure_no_worktree_config_sources( git: &GitRunner, git_root: &Path, + git_config_args: &[String], ) -> io::Result<()> { let git_root = normalize_absolute_path(std::fs::canonicalize(git_root)?)?; + // 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(git)? { if !is_disabled_primary_config_path(&candidate) { reject_source(git, &git_root, &candidate, description)?; @@ -41,6 +52,65 @@ pub(crate) fn ensure_no_worktree_primary_config_sources( "selected Git prefix system config", )?; } + let entries = read_config_entries_without_includes( + git, + &git_root, + git_config_args, + INCLUDE_CONFIG_PATTERN, + "include", + /*config_file*/ None, + )?; + let mut pending = Vec::new(); + validate_include_entries(git, &git_root, entries, /*depth*/ 1, &mut pending)?; + let mut visited = BTreeSet::new(); + while let Some((config_path, depth)) = pending.pop() { + if depth > MAX_CONFIG_INCLUDE_DEPTH { + return Err(path_safety::invalid_config_source( + "Git config include depth exceeded", + )); + } + match std::fs::canonicalize(&config_path) { + Ok(_) => {} + Err(error) + if matches!( + error.kind(), + io::ErrorKind::NotFound | io::ErrorKind::NotADirectory + ) => + { + continue; + } + Err(error) => return Err(error), + } + // The same file reached through two spellings can resolve a relative + // child include differently. Deduplicate only the exact source + // spelling, not its canonical target. + if !visited.insert(config_path.clone()) { + continue; + } + if visited.len() > MAX_CONFIG_INCLUDE_FILES { + return Err(path_safety::invalid_config_source( + "too many Git config include files", + )); + } + let entries = read_config_entries_without_includes( + git, + &git_root, + &[], + INCLUDE_CONFIG_PATTERN, + "include", + Some(&config_path), + )?; + validate_include_entries(git, &git_root, entries, depth + 1, &mut pending)?; + } + // `git var` loads repository config before reporting modern default-system + // paths. Delay that supplemental probe until the complete no-includes + // source graph above has been classified, so it cannot be tricked into + // opening an untrusted include or FIFO first. + for (description, candidate) in default_system_config_source_candidates(git, &git_root)? { + if !is_disabled_primary_config_path(&candidate) { + reject_source(git, &git_root, &candidate, description)?; + } + } Ok(()) } @@ -54,6 +124,17 @@ fn reject_source( git.ensure_config_source_is_not_worktree_controlled(&absolute, description) } +#[cfg(all(test, not(windows)))] +use crate::git_config::GitConfigEntry; +#[cfg(all(test, not(windows)))] +use codex_utils_absolute_path::AbsolutePathBuf; +#[cfg(test)] +use include_graph::expand_git_config_path; +#[cfg(all(test, not(windows)))] +use include_graph::resolve_include_path; +#[cfg(test)] +use path_safety::windows_config_path_is_ambiguous; + #[cfg(test)] #[path = "git_config_sources_tests.rs"] mod tests; diff --git a/codex-rs/git-utils/src/git_config_sources/include_graph.rs b/codex-rs/git-utils/src/git_config_sources/include_graph.rs new file mode 100644 index 0000000000..da267505f5 --- /dev/null +++ b/codex-rs/git-utils/src/git_config_sources/include_graph.rs @@ -0,0 +1,120 @@ +use std::io; +use std::path::Path; +use std::path::PathBuf; + +use super::path_safety::CONFIG_PATH_KEY; +use super::path_safety::invalid_config_source; +use super::path_safety::reject_raw_ambiguous_windows_config_path; +use super::path_safety::resolve_literal_path; +use super::reject_source; +use crate::git_command::GitRunner; +use crate::git_config::GitConfigEntry; +use crate::git_config::GitConfigOrigin; + +pub(super) fn validate_include_entries( + git: &GitRunner, + git_root: &Path, + entries: Vec, + depth: usize, + pending: &mut Vec<(PathBuf, usize)>, +) -> io::Result<()> { + for entry in entries { + if !is_include_path(&entry.key) { + return Err(invalid_config_source("unexpected Git config include key")); + } + if let Some(origin) = config_file_origin(&entry, git_root)? { + reject_source(git, git_root, &origin, "Git config origin")?; + } + let include = resolve_include_path(git, git_root, &entry)?; + reject_source(git, git_root, &include, "Git config include")?; + pending.push((include, depth)); + } + Ok(()) +} + +fn config_file_origin(entry: &GitConfigEntry, cwd: &Path) -> io::Result> { + match &entry.origin { + GitConfigOrigin::CommandLine => Ok(None), + GitConfigOrigin::File(path) => Ok(Some(resolve_literal_path(path, cwd))), + } +} + +fn is_include_path(key: &str) -> bool { + key == "include.path" || key.starts_with("includeif.") && key.ends_with(".path") +} + +pub(super) fn resolve_include_path( + git: &GitRunner, + cwd: &Path, + entry: &GitConfigEntry, +) -> io::Result { + let raw = entry.value.as_str(); + if raw.is_empty() { + return Err(invalid_config_source("empty Git config include path")); + } + // Unlike generic `git config --path`, include.path treats `:(...)` as + // literal path text. Bypass the generic path expander for those spellings + // so the validated path exactly matches Git's include loader. + let expanded = if raw.starts_with(":(") { + PathBuf::from(raw) + } else { + expand_git_config_path(git, cwd, raw)? + }; + reject_raw_ambiguous_windows_config_path( + expanded + .to_str() + .ok_or_else(|| invalid_config_source("non-UTF-8 Git include path"))?, + )?; + let base = match config_file_origin(entry, cwd)? { + Some(origin) => origin + .parent() + .ok_or_else(|| invalid_config_source("Git config origin has no parent"))? + .to_path_buf(), + None if expanded.is_absolute() => cwd.to_path_buf(), + None => { + return Err(invalid_config_source( + "relative Git config include has no file origin", + )); + } + }; + Ok(resolve_literal_path(expanded, &base)) +} + +pub(super) fn expand_git_config_path( + git: &GitRunner, + cwd: &Path, + raw: &str, +) -> io::Result { + let mut command = git.command_for_cwd(cwd)?; + command + .arg("-c") + .arg(format!("{CONFIG_PATH_KEY}={raw}")) + .args([ + "config", + "--null", + "--no-includes", + "--path", + "--get", + CONFIG_PATH_KEY, + ]); + let output = git.output(command)?; + if !output.status.success() { + return Err(io::Error::other(format!( + "git include path expansion failed with status {}: {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + let value = output + .stdout + .strip_suffix(&[0]) + .ok_or_else(|| invalid_config_source("unterminated Git include path expansion"))?; + if value.is_empty() || value.contains(&0) { + return Err(invalid_config_source( + "ambiguous Git include path expansion", + )); + } + let value = std::str::from_utf8(value) + .map_err(|_| invalid_config_source("non-UTF-8 Git include path expansion"))?; + Ok(PathBuf::from(value)) +} diff --git a/codex-rs/git-utils/src/git_config_sources/path_safety.rs b/codex-rs/git-utils/src/git_config_sources/path_safety.rs index a2d86fd062..42509affdf 100644 --- a/codex-rs/git-utils/src/git_config_sources/path_safety.rs +++ b/codex-rs/git-utils/src/git_config_sources/path_safety.rs @@ -33,6 +33,24 @@ pub(super) fn normalize_absolute_path(path: impl AsRef) -> io::Result io::Result<()> { + if windows_config_path_is_ambiguous(path) { + return Err(invalid_config_source("ambiguous Windows Git config path")); + } + Ok(()) +} + +#[cfg(not(windows))] +pub(super) fn reject_raw_ambiguous_windows_config_path(_path: &str) -> io::Result<()> { + Ok(()) +} + +#[cfg(any(windows, test))] +pub(super) fn windows_config_path_is_ambiguous(path: &str) -> bool { + crate::path_authority::windows_path_is_ambiguous(path) +} + pub(super) fn invalid_config_source(message: &str) -> io::Error { io::Error::new(io::ErrorKind::InvalidData, message) } 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 1c5e835935..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 @@ -9,6 +9,30 @@ 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, + cwd: &Path, +) -> io::Result> { + 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 + // boundary treats the selected Git installation and its non-environment + // compile-time system config as host-owned trusted inputs. For older Git, + // the exact custom ETC_GITCONFIG path is therefore a documented residual; + // the derivable prefix/ProgramData paths are still checked separately and + // the no-includes graph validates every directive the system file exposes. + let Some(paths) = git_var_config_paths(git, cwd, "GIT_CONFIG_SYSTEM")? else { + return Ok(Vec::new()); + }; + Ok(paths + .into_iter() + .map(|path| ("GIT_CONFIG_SYSTEM", path)) + .collect()) +} + pub(super) fn selected_git_home_config_candidates( git: &GitRunner, cwd: &Path, @@ -86,6 +110,54 @@ fn selected_git_path_candidate(git: &GitRunner, cwd: &Path, raw: &str) -> io::Re git_var_path_from_bytes(value) } +fn git_var_config_paths( + git: &GitRunner, + cwd: &Path, + variable: &str, +) -> io::Result>> { + let mut command = git.command_for_cwd(cwd)?; + command + .env("GIT_OPTIONAL_LOCKS", "0") + .args(["var", variable]); + let output = git.output(command)?; + parse_git_var_config_paths_result( + output.status.code(), + &output.stdout, + &output.stderr, + variable, + ) +} + +pub(super) fn parse_git_var_config_paths_result( + status_code: Option, + stdout: &[u8], + stderr: &[u8], + variable: &str, +) -> io::Result>> { + match status_code { + Some(0) => parse_git_var_paths(stdout).map(Some), + Some(1) if stdout.is_empty() && stderr.is_empty() => Ok(Some(Vec::new())), + // These variables were added in Git 2.42. Older Git reports usage + // status 129; fall back to its documented environment/home sources. + Some(129) => Ok(None), + _ => Err(io::Error::other(format!( + "git {variable} source probe failed with status {status_code:?}: {}", + String::from_utf8_lossy(stderr).trim() + ))), + } +} + +fn parse_git_var_paths(output: &[u8]) -> io::Result> { + output + .split(|byte| *byte == b'\n') + .filter(|path| !path.is_empty()) + .map(|path| { + let path = path.strip_suffix(b"\r").unwrap_or(path); + git_var_path_from_bytes(path) + }) + .collect() +} + pub(super) fn legacy_primary_config_source_candidates( git: &GitRunner, ) -> io::Result> { 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 222e9a614f..e44696f883 100644 --- a/codex-rs/git-utils/src/git_config_sources_tests.rs +++ b/codex-rs/git-utils/src/git_config_sources_tests.rs @@ -1,7 +1,6 @@ use super::*; use pretty_assertions::assert_eq; use std::ffi::OsStr; -use std::path::PathBuf; fn run(cwd: &Path, args: &[&str]) -> (i32, String, String) { let mut command = std::process::Command::new("git"); @@ -63,7 +62,11 @@ fn create_junction(path: &Path, target: &Path) { fn guard(root: &Path) -> io::Result<()> { let git = GitRunner::for_cwd_io(root)?; - ensure_no_worktree_primary_config_sources(&git, root) + ensure_no_worktree_config_sources(&git, root, &[]) +} + +fn add_include(root: &Path, key: &str, value: &str) { + run_success(root, &["config", "--add", key, value]); } fn assert_worktree_rejection(error: io::Error) { @@ -95,6 +98,45 @@ fn run_isolated_source_test(test_name: &str, env: &[(&str, &OsStr)], removed: &[ ); } +#[test] +fn allows_protected_metadata_and_external_config_sources() { + let repo = init_repo(); + let root = repo.path(); + guard(root).expect("ordinary metadata config"); + + std::fs::write( + root.join(".git/protected.gitconfig"), + "[user]\nname = protected\n", + ) + .expect("write protected config"); + add_include(root, "include.path", "protected.gitconfig"); + guard(root).expect("protected metadata include"); + std::fs::create_dir(root.join(".git/subdir")).expect("create metadata subdirectory"); + add_include(root, "include.path", "subdir/../protected.gitconfig"); + guard(root).expect("metadata-local parent traversal"); + + let external = tempfile::tempdir().expect("external config directory"); + let external_config = external.path().join("safe.gitconfig"); + std::fs::write(&external_config, "[user]\nemail = safe@example.com\n") + .expect("write external config"); + add_include( + root, + "include.path", + external_config.to_str().expect("UTF-8 external path"), + ); + guard(root).expect("external include"); + add_include( + root, + "include.path", + external + .path() + .join("missing.gitconfig") + .to_str() + .expect("UTF-8 missing external path"), + ); + guard(root).expect("missing external include"); +} + #[test] fn rejects_empty_primary_global_and_system_sources_in_the_worktree() { const TEST_NAME: &str = "git_config_sources::tests::rejects_empty_primary_global_and_system_sources_in_the_worktree"; @@ -210,6 +252,20 @@ fn allows_unset_home_when_git_has_no_global_source() { guard(&root).expect("Git without HOME or global config"); } +#[test] +fn supports_pre_242_git_without_system_var_enumeration() { + assert_eq!( + primary_sources::parse_git_var_config_paths_result( + Some(129), + b"", + b"usage: git var (-l | )\n", + "GIT_CONFIG_SYSTEM", + ) + .expect("pre-2.42 fallback"), + None + ); +} + #[cfg(unix)] #[test] fn rejects_newline_bearing_raw_global_source_before_git_normalizes_it() { @@ -269,6 +325,51 @@ fn ignores_non_utf8_values_for_unrelated_global_keys() { guard(&root).expect("unrelated non-UTF-8 global value"); } +#[cfg(all(unix, not(target_os = "macos")))] +#[test] +fn preserves_non_utf8_unix_repository_and_config_origin_paths() { + use std::os::unix::ffi::OsStringExt; + + const TEST_NAME: &str = + "git_config_sources::tests::preserves_non_utf8_unix_repository_and_config_origin_paths"; + if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() { + let parent = tempfile::tempdir().expect("non-UTF-8 fixture parent"); + let root = parent + .path() + .join(std::ffi::OsString::from_vec(b"repo-\xff".to_vec())); + init_repo_at(&root); + let safe = tempfile::NamedTempFile::new().expect("safe included config"); + add_include( + &root, + "include.path", + safe.path().to_str().expect("UTF-8 safe include"), + ); + let global = parent + .path() + .join(std::ffi::OsString::from_vec(b"global-\xff".to_vec())); + std::fs::write( + &global, + format!("[include]\npath = {}\n", safe.path().display()), + ) + .expect("write non-UTF-8 global path"); + run_isolated_source_test( + TEST_NAME, + &[ + ("CODEX_GIT_CONFIG_SOURCE_ROOT", root.as_os_str()), + ("GIT_CONFIG_GLOBAL", global.as_os_str()), + ("GIT_CONFIG_NOSYSTEM", OsStr::new("1")), + ], + &["GIT_CONFIG_SYSTEM"], + ); + return; + } + + let root = PathBuf::from( + std::env::var_os("CODEX_GIT_CONFIG_SOURCE_ROOT").expect("fixture repository root"), + ); + guard(&root).expect("non-UTF-8 repository and config origins"); +} + #[cfg(unix)] #[test] fn rejects_worktree_fifo_primary_source_without_opening_it() { @@ -609,3 +710,891 @@ fn windows_appdata_global_config_matches_native_git_selection_and_precedence() { expected => panic!("unexpected child outcome {expected}"), } } + +#[test] +fn rejects_empty_nonempty_absolute_missing_and_conditional_worktree_includes() { + for (name, body, include_key, include_value) in [ + ( + "empty", + "", + "include.path".to_string(), + "../driver-config".to_string(), + ), + ( + "nonempty", + "[user]\nname = worktree\n", + "include.path".to_string(), + "../driver-config".to_string(), + ), + ( + "missing", + "", + "include.path".to_string(), + "../future.gitconfig".to_string(), + ), + ( + "inactive includeIf", + "", + "includeIf.gitdir:/definitely/not/this/repository/**.path".to_string(), + "../driver-config".to_string(), + ), + ( + "case-insensitive gitdir includeIf", + "", + "includeIf.gitdir/i:/definitely/not/this/repository/**.path".to_string(), + "../driver-config".to_string(), + ), + ( + "onbranch includeIf", + "", + "includeIf.onbranch:definitely-never/**.path".to_string(), + "../driver-config".to_string(), + ), + ( + "hasconfig includeIf", + "", + "includeIf.hasconfig:remote.*.url:https://never.example/**.path".to_string(), + "../driver-config".to_string(), + ), + ] { + let repo = init_repo(); + let root = repo.path(); + if name != "missing" { + std::fs::write(root.join("driver-config"), body).expect("write config fixture"); + } + add_include(root, &include_key, &include_value); + assert_worktree_rejection(guard(root).expect_err(name)); + } + + let repo = init_repo(); + let root = repo.path(); + let config = root.join("absolute.gitconfig"); + std::fs::write(&config, "").expect("write absolute config"); + add_include( + root, + "include.path", + config.to_str().expect("UTF-8 config path"), + ); + assert_worktree_rejection(guard(root).expect_err("absolute include")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write conditional config"); + let git_dir = run_success(root, &["rev-parse", "--absolute-git-dir"]).replace('\\', "/"); + add_include( + root, + &format!("includeIf.gitdir:{git_dir}/.path"), + "../driver-config", + ); + assert_worktree_rejection(guard(root).expect_err("active includeIf")); +} + +#[test] +fn command_scoped_include_paths_follow_the_same_boundary() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("Git runner"); + let unsafe_config = root.join("command.gitconfig"); + std::fs::write(&unsafe_config, "").expect("write worktree config"); + let unsafe_args = vec![ + "-c".to_string(), + format!("include.path={}", unsafe_config.display()), + ]; + assert_worktree_rejection( + ensure_no_worktree_config_sources(&git, root, &unsafe_args) + .expect_err("command-scoped worktree include"), + ); + + let external = tempfile::NamedTempFile::new().expect("safe external config"); + let safe_args = vec![ + "-c".to_string(), + format!("include.path={}", external.path().display()), + ]; + ensure_no_worktree_config_sources(&git, root, &safe_args) + .expect("command-scoped external include"); + + let relative_args = vec![ + "-c".to_string(), + "include.path=relative.gitconfig".to_string(), + ]; + assert!( + ensure_no_worktree_config_sources(&git, root, &relative_args).is_err(), + "relative command includes must fail closed" + ); +} + +#[test] +fn rejects_every_duplicate_and_nested_include_target() { + for unsafe_first in [true, false] { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write unsafe config"); + let external = tempfile::NamedTempFile::new().expect("safe external config"); + let safe = external.path().to_str().expect("UTF-8 safe path"); + let values = if unsafe_first { + ["../driver-config", safe] + } else { + [safe, "../driver-config"] + }; + for value in values { + add_include(root, "include.path", value); + } + assert_worktree_rejection(guard(root).expect_err("duplicate include")); + } + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write nested target"); + let external = tempfile::tempdir().expect("external config directory"); + let outer = external.path().join("outer.gitconfig"); + let nested = root.join("driver-config"); + run_success( + root, + &[ + "config", + "--file", + outer.to_str().expect("UTF-8 outer path"), + "include.path", + nested.to_str().expect("UTF-8 nested path"), + ], + ); + add_include( + root, + "include.path", + outer.to_str().expect("UTF-8 outer path"), + ); + assert_worktree_rejection(guard(root).expect_err("nested include")); +} + +#[cfg(unix)] +#[test] +fn rejects_symlink_aliases_across_the_worktree_and_metadata_boundaries() { + use std::os::unix::fs::symlink; + + let repo = init_repo(); + let root = repo.path(); + let external = tempfile::tempdir().expect("external directory"); + std::fs::write(external.path().join("safe.gitconfig"), "").expect("write safe config"); + symlink(external.path(), root.join("inside-link")).expect("inside to outside link"); + add_include(root, "include.path", "../inside-link/safe.gitconfig"); + assert_worktree_rejection(guard(root).expect_err("worktree symlink alias")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write worktree target"); + let external = tempfile::tempdir().expect("external directory"); + symlink(root, external.path().join("outside-link")).expect("outside to inside link"); + add_include( + root, + "include.path", + external + .path() + .join("outside-link/driver-config") + .to_str() + .expect("UTF-8 alias path"), + ); + assert_worktree_rejection(guard(root).expect_err("external symlink alias")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write escaped target"); + symlink(root, root.join(".git/config-link")).expect("metadata escape link"); + add_include(root, "include.path", "config-link/driver-config"); + assert_worktree_rejection(guard(root).expect_err("metadata symlink escape")); + + let repo = init_repo(); + let root = repo.path(); + let external = tempfile::tempdir().expect("external directory"); + symlink( + root.join("future-config"), + external.path().join("dangling-link"), + ) + .expect("dangling link into worktree"); + add_include( + root, + "include.path", + external + .path() + .join("dangling-link/config") + .to_str() + .expect("UTF-8 dangling alias"), + ); + assert_worktree_rejection(guard(root).expect_err("dangling symlink alias")); + + let repo = init_repo(); + let root = repo.path(); + let entry = tempfile::tempdir().expect("external entry directory"); + let exit = tempfile::tempdir().expect("external exit directory"); + std::fs::write(exit.path().join("safe.gitconfig"), "").expect("write exit config"); + symlink(root, entry.path().join("through-worktree")).expect("link into worktree"); + symlink(exit.path(), root.join("back-out")).expect("worktree-controlled exit link"); + add_include( + root, + "include.path", + entry + .path() + .join("through-worktree/back-out/safe.gitconfig") + .to_str() + .expect("UTF-8 nested alias"), + ); + assert_worktree_rejection(guard(root).expect_err("nested symlink traversal")); + + let repo = init_repo(); + let root = repo.path(); + let entry = tempfile::tempdir().expect("external entry directory"); + let exit = tempfile::tempdir().expect("external exit directory"); + std::fs::write(exit.path().join("safe.gitconfig"), "").expect("write chained config"); + symlink(exit.path(), root.join("workspace-link")).expect("workspace link out"); + symlink( + root.join("workspace-link"), + entry.path().join("direct-chain"), + ) + .expect("external link to workspace link"); + add_include( + root, + "include.path", + entry + .path() + .join("direct-chain/safe.gitconfig") + .to_str() + .expect("UTF-8 direct chain"), + ); + assert_worktree_rejection(guard(root).expect_err("direct symlink chain")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::create_dir(root.join("subdir")).expect("create symlink target"); + std::fs::write(root.join("evil.gitconfig"), "[user]\nname = worktree\n") + .expect("write worktree config"); + std::fs::write( + root.join(".git/evil.gitconfig"), + "[user]\nname = metadata\n", + ) + .expect("write metadata decoy"); + symlink(root.join("subdir"), root.join(".git/link")).expect("metadata link to worktree"); + add_include(root, "include.path", "link/../evil.gitconfig"); + assert_worktree_rejection(guard(root).expect_err("symlink parent traversal")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::create_dir(root.join("pivot")).expect("create worktree pivot"); + std::fs::write(root.join(".git/protected.gitconfig"), "") + .expect("write protected metadata config"); + add_include( + root, + "include.path", + root.join("pivot/../.git/protected.gitconfig") + .to_str() + .expect("UTF-8 pivot path"), + ); + assert_worktree_rejection(guard(root).expect_err("raw path pivots through worktree")); +} + +#[cfg(unix)] +#[test] +fn rejects_external_symlink_target_with_hidden_worktree_pivot_to_protected_config() { + use std::os::unix::fs::symlink; + + let repo = init_repo(); + let root = std::fs::canonicalize(repo.path()).expect("canonical repository"); + let external = tempfile::tempdir().expect("external alias directory"); + let protected = root.join(".git/protected.gitconfig"); + std::fs::write(&protected, "[codex]\n\tpivot = protected\n").expect("write protected config"); + std::fs::create_dir(root.join("pivot")).expect("create ordinary worktree pivot"); + let entry = external.path().join("entry"); + symlink(root.join("pivot/../.git/protected.gitconfig"), &entry) + .expect("external symlink target with raw worktree pivot"); + add_include( + &root, + "include.path", + entry.to_str().expect("UTF-8 entry path"), + ); + assert_eq!( + run_success(&root, &["config", "--includes", "--get", "codex.pivot"]), + "protected" + ); + assert_worktree_rejection( + guard(&root).expect_err("hidden pivot inside external symlink target"), + ); +} + +#[cfg(target_os = "macos")] +#[test] +fn rejects_apfs_alias_symlink_target_with_hidden_worktree_pivot() { + use std::os::unix::fs::symlink; + + let temp = std::fs::canonicalize(std::env::temp_dir()).expect("canonical temp directory"); + let parent = tempfile::tempdir_in(temp).expect("Data-volume fixture"); + let root = parent.path().join("repo"); + init_repo_at(&root); + let root = std::fs::canonicalize(root).expect("canonical repository"); + let data_alias = PathBuf::from("/System/Volumes/Data") + .join(root.strip_prefix("/").expect("absolute repository path")); + if std::fs::metadata(&data_alias).is_err() { + eprintln!("APFS Data alias unavailable; skipping native pivot assertion"); + return; + } + let protected = root.join(".git/protected.gitconfig"); + std::fs::write(&protected, "[codex]\n\tpivot = protected\n").expect("write protected config"); + std::fs::create_dir(root.join("pivot")).expect("create ordinary worktree pivot"); + let external = tempfile::tempdir().expect("external alias directory"); + let entry = external.path().join("entry"); + symlink(data_alias.join("pivot/../.git/protected.gitconfig"), &entry) + .expect("APFS alias target with raw worktree pivot"); + add_include( + &root, + "include.path", + entry.to_str().expect("UTF-8 entry path"), + ); + assert_eq!( + run_success(&root, &["config", "--includes", "--get", "codex.pivot"]), + "protected" + ); + assert_worktree_rejection(guard(&root).expect_err("APFS hidden pivot inside symlink target")); +} + +#[cfg(unix)] +#[test] +fn rejects_unregistered_same_common_worktree_used_only_as_alias_intermediate() { + use std::os::unix::fs::symlink; + + let parent = tempfile::tempdir().expect("fixture"); + let main = parent.path().join("main"); + let linked = parent.path().join("linked"); + let stale = parent.path().join("stale-unregistered"); + let external = parent.path().join("external"); + let safe = external.join("safe.gitconfig"); + let entry = external.join("entry"); + init_repo_at(&main); + run_success( + &main, + &[ + "worktree", + "add", + "-b", + "same-common-active", + linked.to_str().expect("UTF-8 linked path"), + ], + ); + std::fs::create_dir_all(&stale).expect("create stale worktree"); + std::fs::write( + stale.join(".git"), + format!("gitdir: {}\n", main.join(".git").display()), + ) + .expect("write stale same-common marker"); + std::fs::create_dir_all(&external).expect("create external directory"); + std::fs::write(&safe, "[codex]\n\tsameCommon = protected\n") + .expect("write external safe config"); + symlink(&safe, stale.join("switch")).expect("stale worktree controlled switch"); + symlink(stale.join("switch"), &entry).expect("external entry through stale worktree"); + add_include( + &linked, + "include.path", + entry.to_str().expect("UTF-8 entry path"), + ); + assert_eq!( + run_success( + &linked, + &["config", "--includes", "--get", "codex.sameCommon"] + ), + "protected" + ); + assert!( + guard(&linked).is_err(), + "unregistered same-common route intermediate must fail closed" + ); +} + +#[cfg(windows)] +#[test] +fn rejects_windows_junction_aliases_across_worktree_and_metadata_boundaries() { + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write worktree config"); + let external = tempfile::tempdir().expect("external directory"); + create_junction(&external.path().join("into-worktree"), root); + add_include( + root, + "include.path", + external + .path() + .join("into-worktree/driver-config") + .to_str() + .expect("UTF-8 junction path"), + ); + assert_worktree_rejection(guard(root).expect_err("external junction into worktree")); + + let repo = init_repo(); + let root = repo.path(); + let external = tempfile::tempdir().expect("external directory"); + std::fs::write(external.path().join("safe.gitconfig"), "").expect("write external config"); + create_junction(&root.join("out-of-worktree"), external.path()); + add_include(root, "include.path", "../out-of-worktree/safe.gitconfig"); + assert_worktree_rejection(guard(root).expect_err("worktree junction to external")); + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write worktree config"); + create_junction(&root.join(".git/metadata-escape"), root); + add_include(root, "include.path", "metadata-escape/driver-config"); + assert_worktree_rejection(guard(root).expect_err("metadata junction escape")); + + let repo = init_repo(); + let root = repo.path(); + let entry = tempfile::tempdir().expect("external entry directory"); + let exit = tempfile::tempdir().expect("external exit directory"); + std::fs::write(exit.path().join("safe.gitconfig"), "").expect("write external config"); + create_junction(&entry.path().join("through-worktree"), root); + create_junction(&root.join("back-out"), exit.path()); + add_include( + root, + "include.path", + entry + .path() + .join("through-worktree/back-out/safe.gitconfig") + .to_str() + .expect("UTF-8 multi-hop junction path"), + ); + assert_worktree_rejection(guard(root).expect_err("multi-hop junction traversal")); + + let repo = init_repo(); + let root = repo.path(); + let external = tempfile::tempdir().expect("external directory"); + create_junction(&external.path().join("into-worktree"), root); + add_include( + root, + "include.path", + external + .path() + .join("into-worktree/future.gitconfig") + .to_str() + .expect("UTF-8 missing junction target"), + ); + assert_worktree_rejection(guard(root).expect_err("junction missing suffix")); +} + +#[test] +fn linked_worktree_metadata_is_allowed_but_linked_worktree_config_is_rejected() { + let parent = tempfile::tempdir().expect("worktree parent"); + let main = parent.path().join("main"); + let linked = parent.path().join("linked"); + init_repo_at(&main); + run_success( + &main, + &[ + "worktree", + "add", + "-b", + "config-guard-linked", + linked.to_str().expect("UTF-8 linked path"), + ], + ); + + let git_dir = PathBuf::from(run_success(&linked, &["rev-parse", "--absolute-git-dir"])); + let protected = git_dir.join("protected.gitconfig"); + std::fs::write(&protected, "[user]\nname = linked\n").expect("write linked metadata config"); + add_include( + &linked, + "include.path", + protected.to_str().expect("UTF-8 protected path"), + ); + guard(&linked).expect("linked metadata include"); + run_success(&linked, &["config", "--unset-all", "include.path"]); + + std::fs::write(main.join("driver-config"), "").expect("write main-worktree config"); + add_include( + &linked, + "include.path", + main.join("driver-config") + .to_str() + .expect("UTF-8 main-worktree config"), + ); + assert_worktree_rejection(guard(&linked).expect_err("main worktree include")); + run_success(&linked, &["config", "--unset-all", "include.path"]); + add_include( + &linked, + "include.path", + main.join("future.gitconfig") + .to_str() + .expect("UTF-8 missing main-worktree config"), + ); + assert_worktree_rejection(guard(&linked).expect_err("missing main worktree include")); + run_success(&linked, &["config", "--unset-all", "include.path"]); + + let unsafe_config = linked.join("driver-config"); + std::fs::write(&unsafe_config, "").expect("write linked worktree config"); + add_include( + &linked, + "include.path", + unsafe_config.to_str().expect("UTF-8 unsafe path"), + ); + assert_worktree_rejection(guard(&linked).expect_err("linked worktree include")); +} + +#[test] +fn enclosing_repository_sibling_worktrees_are_always_untrusted() { + let parent = tempfile::tempdir().expect("fixture parent"); + let outer = parent.path().join("outer"); + let sibling = parent.path().join("sibling"); + init_repo_at(&outer); + run_success( + &outer, + &[ + "worktree", + "add", + "-b", + "config-guard-sibling", + sibling.to_str().expect("UTF-8 sibling worktree"), + ], + ); + let nested = outer.join("nested"); + init_repo_at(&nested); + + std::fs::write(sibling.join("driver-config"), "").expect("write sibling config"); + add_include( + &nested, + "include.path", + sibling + .join("driver-config") + .to_str() + .expect("UTF-8 sibling config"), + ); + assert_worktree_rejection(guard(&nested).expect_err("enclosing repo sibling config")); + + run_success(&nested, &["config", "--unset-all", "include.path"]); + std::fs::remove_dir_all(&sibling).expect("remove sibling but retain registry entry"); + add_include( + &nested, + "include.path", + sibling + .join("future.gitconfig") + .to_str() + .expect("UTF-8 missing sibling config"), + ); + assert_worktree_rejection(guard(&nested).expect_err("missing enclosing repo sibling config")); + + let nested_primary = outer.join("inner-primary"); + let inner_linked = parent.path().join("inner-linked"); + init_repo_at(&nested_primary); + run_success( + &nested_primary, + &[ + "worktree", + "add", + "-b", + "inner-linked-from-outer", + inner_linked.to_str().expect("UTF-8 inner linked worktree"), + ], + ); + for candidate in [ + outer.join("outer.txt"), + outer.join("future-outer.gitconfig"), + ] { + if candidate.file_name() == Some(OsStr::new("outer.txt")) { + std::fs::write(&candidate, "").expect("write outer config source"); + } + add_include( + &inner_linked, + "include.path", + candidate.to_str().expect("UTF-8 outer config source"), + ); + assert_worktree_rejection( + guard(&inner_linked).expect_err("enclosing primary config source"), + ); + run_success(&inner_linked, &["config", "--unset-all", "include.path"]); + } + add_include( + &inner_linked, + "include.path", + sibling + .join("future-from-inner.gitconfig") + .to_str() + .expect("UTF-8 enclosing sibling source"), + ); + assert_worktree_rejection( + guard(&inner_linked).expect_err("enclosing sibling from linked inner worktree"), + ); + run_success(&inner_linked, &["config", "--unset-all", "include.path"]); +} + +#[cfg(unix)] +#[test] +fn preserves_symlinked_caller_ancestry_and_each_include_spelling() { + use std::os::unix::fs::symlink; + + let parent = tempfile::tempdir().expect("fixture parent"); + let outer = parent.path().join("outer"); + let nested = parent.path().join("external-nested"); + init_repo_at(&outer); + init_repo_at(&nested); + std::fs::write(outer.join("driver-config"), "").expect("write outer config"); + symlink(&nested, outer.join("nested-link")).expect("link nested cwd through outer repo"); + add_include( + &nested, + "include.path", + outer + .join("driver-config") + .to_str() + .expect("UTF-8 outer config"), + ); + let git = GitRunner::for_cwd_io(&outer.join("nested-link")) + .expect("Git runner from symlinked nested cwd"); + assert_worktree_rejection( + ensure_no_worktree_config_sources(&git, &nested, &[]) + .expect_err("logical outer ancestry config"), + ); + + let repo = init_repo(); + let root = repo.path(); + std::fs::write(root.join("driver-config"), "").expect("write unsafe child config"); + let external = tempfile::tempdir().expect("external include tree"); + let shared = external.path().join("shared.gitconfig"); + std::fs::write(&shared, "[include]\npath = child.gitconfig\n") + .expect("write shared parent config"); + let unsafe_dir = external.path().join("unsafe-spelling"); + let safe_dir = external.path().join("safe-spelling"); + std::fs::create_dir(&unsafe_dir).expect("create unsafe spelling directory"); + std::fs::create_dir(&safe_dir).expect("create safe spelling directory"); + symlink(&shared, unsafe_dir.join("parent.gitconfig")) + .expect("unsafe spelling of shared parent"); + symlink(&shared, safe_dir.join("parent.gitconfig")).expect("safe spelling of shared parent"); + symlink( + root.join("driver-config"), + unsafe_dir.join("child.gitconfig"), + ) + .expect("unsafe relative child"); + std::fs::write(safe_dir.join("child.gitconfig"), "").expect("safe relative child"); + add_include( + root, + "include.path", + unsafe_dir + .join("parent.gitconfig") + .to_str() + .expect("UTF-8 unsafe spelling"), + ); + add_include( + root, + "include.path", + safe_dir + .join("parent.gitconfig") + .to_str() + .expect("UTF-8 safe spelling"), + ); + assert_worktree_rejection(guard(root).expect_err("unsafe relative alias child")); + + let parent = tempfile::tempdir().expect("linked-main dangling fixture"); + let main = parent.path().join("main"); + let linked = parent.path().join("linked"); + init_repo_at(&main); + run_success( + &main, + &[ + "worktree", + "add", + "-b", + "config-guard-dangling-main", + linked.to_str().expect("UTF-8 linked worktree"), + ], + ); + let external = tempfile::tempdir().expect("external dangling alias directory"); + symlink( + main.join("future.gitconfig"), + external.path().join("dangling-main-config"), + ) + .expect("dangling alias into main worktree"); + add_include( + &linked, + "include.path", + external + .path() + .join("dangling-main-config") + .to_str() + .expect("UTF-8 dangling main alias"), + ); + assert_worktree_rejection(guard(&linked).expect_err("dangling alias into main worktree")); +} + +#[cfg(target_os = "macos")] +#[test] +fn rejects_apfs_data_firmlink_aliases_for_existing_and_missing_worktree_configs() { + let data_root = Path::new("/System/Volumes/Data"); + if !data_root.is_dir() { + return; + } + let home = PathBuf::from(std::env::var_os("HOME").expect("HOME")); + let temp_parent = home.join(".cache/codex-git-utils-tests"); + std::fs::create_dir_all(&temp_parent).expect("create test temp parent"); + + for exists in [true, false] { + let repo = tempfile::Builder::new() + .prefix("firmlink-config-") + .tempdir_in(&temp_parent) + .expect("home tempdir"); + let root = repo.path(); + init_repo_at(root); + if exists { + std::fs::write(root.join("driver-config"), "").expect("write aliased config"); + } + let alias = data_root + .join(root.strip_prefix("/").expect("absolute repository path")) + .join("driver-config"); + add_include( + root, + "include.path", + alias.to_str().expect("UTF-8 firmlink alias"), + ); + assert_worktree_rejection(guard(root).expect_err("APFS firmlink alias")); + } + + let family = tempfile::Builder::new() + .prefix("firmlink-linked-family-") + .tempdir_in(&temp_parent) + .expect("home linked-family tempdir"); + let main = family.path().join("main"); + let linked = family.path().join("linked"); + init_repo_at(&main); + run_success( + &main, + &[ + "worktree", + "add", + "-b", + "firmlink-linked-main", + linked.to_str().expect("UTF-8 linked path"), + ], + ); + for exists in [true, false] { + let name = if exists { + "driver-config" + } else { + "future.gitconfig" + }; + if exists { + std::fs::write(main.join(name), "").expect("write main config"); + } + let alias = data_root + .join(main.strip_prefix("/").expect("absolute main path")) + .join(name); + add_include( + &linked, + "include.path", + alias.to_str().expect("UTF-8 main firmlink alias"), + ); + assert_worktree_rejection(guard(&linked).expect_err("main firmlink alias")); + run_success(&linked, &["config", "--unset-all", "include.path"]); + } + std::fs::write(main.join(".git/protected.gitconfig"), "") + .expect("write protected main metadata config"); + let protected_alias = data_root + .join(main.strip_prefix("/").expect("absolute main path")) + .join(".git/protected.gitconfig"); + add_include( + &linked, + "include.path", + protected_alias + .to_str() + .expect("UTF-8 protected metadata alias"), + ); + guard(&linked).expect("protected metadata firmlink alias"); +} + +#[test] +fn include_path_expansion_supports_git_prefix() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("Git runner"); + let prefix = + expand_git_config_path(&git, root, "%(prefix)/etc/gitconfig").expect("expand Git prefix"); + assert!(prefix.is_absolute()); +} + +#[cfg(not(windows))] +#[test] +fn include_path_expansion_preserves_colon_parentheses_as_literal_text() { + let repo = init_repo(); + let root = repo.path(); + let git = GitRunner::for_cwd_io(root).expect("Git runner"); + let optional = GitConfigEntry { + scope: crate::git_config::GitConfigScope::Local, + origin: crate::git_config::GitConfigOrigin::File(".git/config".into()), + key: "include.path".to_string(), + value: ":(optional)../future.gitconfig".to_string(), + }; + let optional_path = resolve_include_path(&git, root, &optional).expect("resolve optional path"); + assert_eq!( + AbsolutePathBuf::resolve_path_against_base(optional_path, root).as_path(), + root.join(".git/:(optional)../future.gitconfig") + ); + + let unknown = GitConfigEntry { + value: ":(unknown)../future.gitconfig".to_string(), + ..optional + }; + let unknown_path = + resolve_include_path(&git, root, &unknown).expect("resolve literal unknown path"); + assert_eq!( + AbsolutePathBuf::resolve_path_against_base(unknown_path, root).as_path(), + root.join(".git/:(unknown)../future.gitconfig") + ); + + for spelling in [":(optional)", ":(unknown)"] { + let external = tempfile::tempdir().expect("external literal config directory"); + let literal_dir = external.path().join(spelling); + std::fs::create_dir(&literal_dir).expect("create literal include directory"); + std::fs::write(literal_dir.join("child"), "[probe]\nvalue = literal\n") + .expect("write literal include child"); + let parent = external.path().join("parent.gitconfig"); + std::fs::write(&parent, format!("[include]\npath = {spelling}/child\n")) + .expect("write literal parent config"); + let value = run_success( + root, + &[ + "config", + "--file", + parent.to_str().expect("UTF-8 parent path"), + "--includes", + "--get", + "probe.value", + ], + ); + assert_eq!(value, "literal"); + add_include( + root, + "include.path", + parent.to_str().expect("UTF-8 parent path"), + ); + guard(root).expect("literal include path is external"); + run_success(root, &["config", "--unset-all", "include.path"]); + } +} + +#[test] +fn windows_path_validator_rejects_namespaces_streams_and_alias_components() { + for path in [ + r"\??\C:\repo\config", + r"\\?\GLOBALROOT\Device\config", + r"\\.\pipe\config", + r"C:\repo\config:stream", + r"C:relative\config", + r":(optional)..\future.gitconfig", + r":(unknown)..\future.gitconfig", + r"C:\repo\NUL.gitconfig", + r"C:\repo\COM¹.gitconfig", + r"C:\repo\LPT³.gitconfig", + r"C:\repo\trailing.\config", + ] { + assert!(windows_config_path_is_ambiguous(path), "{path:?}"); + } + for path in [ + r"C:\external\config", + r"\\server\share\config", + r"\\?\C:\repo\.git\config", + r"\\.\C:\repo\.git\config", + r"\\?\UNC\server\share\config", + r"\\.\UNC\server\share\config", + r"..\driver-config", + r".\driver-config", + ] { + assert!(!windows_config_path_is_ambiguous(path), "{path:?}"); + } +} diff --git a/codex-rs/git-utils/src/git_config_tests.rs b/codex-rs/git-utils/src/git_config_tests.rs index 8f49871c03..b8c167edbd 100644 --- a/codex-rs/git-utils/src/git_config_tests.rs +++ b/codex-rs/git-utils/src/git_config_tests.rs @@ -99,6 +99,62 @@ fn symmetric_git_boolean_parser_excludes_only_int_min_spellings() { } } +#[test] +fn entry_parsers_preserve_duplicate_include_directives_in_order() { + let scoped = b"local\0file:.git/config\0include.path\n../unsafe.gitconfig\0\ +local\0file:.git/config\0include.path\n/absolute/external.gitconfig\0"; + let legacy = b"file:.git/config\0include.path\n../unsafe.gitconfig\0\ +file:.git/config\0include.path\n/absolute/external.gitconfig\0"; + let expected_scoped = vec![ + GitConfigEntry { + scope: GitConfigScope::Local, + origin: GitConfigOrigin::File(".git/config".into()), + key: "include.path".to_string(), + value: "../unsafe.gitconfig".to_string(), + }, + GitConfigEntry { + scope: GitConfigScope::Local, + origin: GitConfigOrigin::File(".git/config".into()), + key: "include.path".to_string(), + value: "/absolute/external.gitconfig".to_string(), + }, + ]; + assert_eq!( + parse_config_entries(scoped).expect("scoped entries"), + expected_scoped + ); + assert_eq!( + parse_config_entries_with_origins(legacy).expect("legacy entries"), + expected_scoped + ); +} + +#[cfg(unix)] +#[test] +fn config_origin_parser_preserves_non_utf8_unix_path_bytes() { + use std::os::unix::ffi::OsStrExt; + + let entries = parse_config_entries(b"local\0file:/tmp/config-\xff\0include.path\n/tmp/safe\0") + .expect("non-UTF-8 config origin"); + let GitConfigOrigin::File(path) = &entries[0].origin else { + panic!("expected file origin"); + }; + assert_eq!(path.as_os_str().as_bytes(), b"/tmp/config-\xff"); +} + +#[test] +fn rejects_malformed_config_records() { + for output in [ + b"global\0file:/tmp/config\0key\nvalue".as_slice(), + b"global\0file:/tmp/config\0key\0".as_slice(), + b"mystery\0file:/tmp/config\0key\nvalue\0".as_slice(), + b"global\0\0key\nvalue\0".as_slice(), + b"global\0file:/tmp/config\0\nvalue\0".as_slice(), + ] { + assert!(parse_config_entries(output).is_err(), "{output:?}"); + } +} + #[test] fn path_containment_uses_component_boundaries() { let root = Path::new("/repo/root"); diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index 378a245859..b42defd914 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -9,7 +9,7 @@ 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_primary_config_sources; +use crate::git_config_sources::ensure_no_worktree_config_sources; /// Extract paths with Git from a cwd whose config sources have already been /// authorized for `git_config_args`. @@ -66,7 +66,7 @@ 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_primary_config_sources(&git, &git_root)?; + ensure_no_worktree_config_sources(&git, &git_root, &[])?; extract_effective_paths_from_patch(&git, &git_root, &patch_path, /*revert*/ false, &[]) })() .unwrap_or_default(); @@ -245,7 +245,7 @@ fn invalid_windows_patch_component(component: &str) -> bool { 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_primary_config_sources(&git, git_root)?; + ensure_no_worktree_config_sources(&git, git_root, &git_config_args)?; let (tmpdir, patch_path) = write_temp_patch(diff)?; let paths = extract_effective_paths_from_patch( &git, diff --git a/codex-rs/git-utils/src/patch_paths_tests.rs b/codex-rs/git-utils/src/patch_paths_tests.rs index 4d6643a97a..8f72e3841e 100644 --- a/codex-rs/git-utils/src/patch_paths_tests.rs +++ b/codex-rs/git-utils/src/patch_paths_tests.rs @@ -68,7 +68,7 @@ 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_primary_config_sources(&git, &git_root)?; + ensure_no_worktree_config_sources(&git, &git_root, &[])?; let paths = extract_effective_paths_from_patch(&git, &git_root, &patch_path, revert, &[])?; drop(tmpdir); Ok(paths) diff --git a/codex-rs/git-utils/src/path_authority.rs b/codex-rs/git-utils/src/path_authority.rs index 1e2157c378..c27f9e698c 100644 --- a/codex-rs/git-utils/src/path_authority.rs +++ b/codex-rs/git-utils/src/path_authority.rs @@ -18,7 +18,7 @@ use route_walker::invalid_data; use route_walker::observe_route; #[cfg(any(windows, test))] pub(crate) use windows_path::windows_authority_path_is_ambiguous; -#[cfg(windows)] +#[cfg(any(windows, test))] pub(crate) use windows_path::windows_path_is_ambiguous; #[derive(Clone, Copy, Eq, PartialEq)] diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index 05b792d357..3d014f810c 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(()); }; @@ -148,6 +155,25 @@ impl RepositoryAuthority { }) } + 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, @@ -156,12 +182,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, From 463eed4400c3986e5ffce30ffd151f0e7cc4e9fc Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Fri, 3 Jul 2026 18:36:13 -0700 Subject: [PATCH 5/5] fix(git): bind guarded patch operations --- codex-rs/git-utils/src/apply.rs | 190 +++++++-------- codex-rs/git-utils/src/git_command.rs | 4 + codex-rs/git-utils/src/guarded_config.rs | 228 ++++++++++++++++++ .../git-utils/src/guarded_config_tests.rs | 56 +++++ codex-rs/git-utils/src/lib.rs | 1 + codex-rs/git-utils/src/patch_paths.rs | 174 ++++++------- codex-rs/git-utils/src/patch_paths_tests.rs | 4 +- .../src/repository_authority/authority.rs | 21 ++ 8 files changed, 499 insertions(+), 179 deletions(-) create mode 100644 codex-rs/git-utils/src/guarded_config.rs create mode 100644 codex-rs/git-utils/src/guarded_config_tests.rs diff --git a/codex-rs/git-utils/src/apply.rs b/codex-rs/git-utils/src/apply.rs index d14d0b2d4c..d3a0787674 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::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::safe_git::DISABLED_HOOKS_PATH; +use crate::guarded_config::GuardedGitConfig; +use crate::patch_paths::extract_effective_paths_from_patch_guarded; +use crate::patch_paths::stage_effective_paths_guarded; #[cfg(test)] use crate::safe_git::isolate_git_command_environment; @@ -46,26 +44,26 @@ 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 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)?; - cfg_parts.extend(safe_git_config_parts()); + let patch_paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, req.revert)?; if req.revert && !req.preflight { // Stage WT paths first to avoid index mismatch on revert. - stage_effective_paths(&git, &git_root, &patch_paths, &cfg_parts)?; + stage_effective_paths_guarded(&config, &patch_paths)?; } // Build git args @@ -83,8 +81,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(); @@ -104,8 +102,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); @@ -127,18 +125,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!( @@ -149,18 +139,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, @@ -197,60 +176,30 @@ 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)) -} - -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(), - ] -} - -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(" ") - ) + 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(all(test, unix))] @@ -260,6 +209,8 @@ mod transport_tests; #[cfg(test)] mod tests { use super::*; + use crate::guarded_config::config_source_authorization_count; + use crate::guarded_config::reset_config_source_authorization_count; use std::ffi::OsStr; use std::path::Path; use std::sync::Mutex; @@ -353,7 +304,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()); @@ -482,6 +435,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(); @@ -540,7 +528,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"); @@ -552,7 +542,9 @@ mod tests { revert: true, preflight: false, }; + reset_config_source_authorization_count(); let res_revert = apply_git_patch(&revert_req).expect("revert ok"); + assert_eq!(config_source_authorization_count(), 1); assert_eq!(res_revert.exit_code, 0, "revert apply succeeded"); let after_revert = read_file_normalized(&root.join("file.txt")); assert_eq!(after_revert, "orig\n"); @@ -589,7 +581,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"]); @@ -619,7 +613,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(), @@ -637,7 +633,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"), @@ -664,8 +662,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/git_command.rs b/codex-rs/git-utils/src/git_command.rs index 9d3c6af66e..0cba0b465c 100644 --- a/codex-rs/git-utils/src/git_command.rs +++ b/codex-rs/git-utils/src/git_command.rs @@ -116,6 +116,10 @@ impl GitRunner { .ensure_config_source_is_not_worktree_controlled(path, description) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + self.authority.ensure_active_worktree_root(root) + } + pub(crate) fn config_environment_value(&self, name: &str) -> Option<&OsStr> { self.config_environment.value(name) } 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..4f9c008c2a --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config.rs @@ -0,0 +1,228 @@ +use std::ffi::OsStr; +use std::io; +use std::path::Path; +use std::path::PathBuf; + +use crate::FsmonitorOverride; +use crate::git_command::GitCommand; +use crate::git_command::GitRunner; +use crate::git_config_sources::ensure_no_worktree_config_sources; +use crate::safe_git::DISABLED_HOOKS_PATH; + +/// 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 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, and receive fixed library safety scalars. +pub(crate) struct GuardedGitConfig<'git> { + sources: ValidatedConfigSources<'git>, +} + +#[derive(Clone, Copy)] +enum BoundSubcommand { + AddLiteralPathspecs, + Apply, + RevParse, +} + +impl BoundSubcommand { + fn as_str(self) -> &'static str { + match self { + Self::AddLiteralPathspecs => "add", + Self::Apply => "apply", + Self::RevParse => "rev-parse", + } + } + + fn append_to(self, command: &mut GitCommand) { + if matches!(self, Self::AddLiteralPathspecs) { + command.arg("--literal-pathspecs"); + } + command.arg(self.as_str()); + } +} + +/// A command whose runner, root, config invocation, 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 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)?, + }) + } + + pub(crate) fn canonical_root(&self) -> &Path { + &self.sources.canonical_root + } + + pub(crate) fn apply_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::Apply) + } + + pub(crate) fn literal_add_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::AddLiteralPathspecs) + } + + pub(crate) fn rev_parse_command(&self) -> io::Result> { + self.guarded_command(BoundSubcommand::RevParse) + } + + fn guarded_command( + &self, + subcommand: BoundSubcommand, + ) -> io::Result> { + let mut command = self + .sources + .git + .command_for_cwd(&self.sources.canonical_root)?; + command.args(&self.sources.base_config_args); + append_safe_scalar_overrides(&mut command); + subcommand.append_to(&mut command); + Ok(GuardedGitCommand { + operation: self, + inner: command, + }) + } + + 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()); + 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..7d0eaed76b --- /dev/null +++ b/codex-rs/git-utils/src/guarded_config_tests.rs @@ -0,0 +1,56 @@ +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 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); + } +} diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index b52bf1ef19..0377521aef 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -9,6 +9,7 @@ mod git_config; mod git_config_environment; mod git_config_sources; mod git_executable; +mod guarded_config; mod info; mod local_only; mod operations; diff --git a/codex-rs/git-utils/src/patch_paths.rs b/codex-rs/git-utils/src/patch_paths.rs index b42defd914..f09e027c75 100644 --- a/codex-rs/git-utils/src/patch_paths.rs +++ b/codex-rs/git-utils/src/patch_paths.rs @@ -4,28 +4,26 @@ 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 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, @@ -46,6 +44,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 @@ -66,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( @@ -244,30 +238,28 @@ 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) + stage_effective_paths_guarded(&config, &paths) } -pub(crate) fn stage_effective_paths( - git: &GitRunner, - git_root: &Path, +/// Preserve the existing reverse staging behavior while constructing every +/// Git child from the operation-owned configuration capability. +pub(crate) fn stage_effective_paths_guarded( + config: &GuardedGitConfig<'_>, paths: &[String], - git_config_args: &[String], ) -> io::Result<()> { - let confined = confine_patch_paths(git, git_root, paths)?; + let confined = confine_patch_paths_guarded(config, paths)?; let mut existing = 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) { if leaf_is_traversable_directory(metadata.file_type()) { return Err(containment_error( @@ -280,14 +272,12 @@ pub(crate) fn stage_effective_paths( if existing.is_empty() { return Ok(()); } - let mut args = vec![ - "--literal-pathspecs".to_string(), - "add".to_string(), - "--".to_string(), - ]; + let mut args = vec!["--".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. + let mut command = config.literal_add_command()?; + command.args(&args); + let _output = command.output()?; + // Preserve the existing best-effort staging contract on non-zero status. Ok(()) } @@ -375,10 +365,29 @@ impl ConfinedPatchPaths { } } +#[cfg(test)] pub(crate) fn confine_patch_paths( git: &GitRunner, git_root: &Path, paths: &[String], +) -> io::Result { + let config = GuardedGitConfig::authorize(git, git_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 { @@ -401,8 +410,6 @@ pub(crate) fn confine_patch_paths( }); } - let canonical_root = std::fs::canonicalize(git_root)?; - let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?; let mut entries = Vec::with_capacity(paths.len()); let mut prefix_cache = std::collections::BTreeMap::new(); @@ -416,9 +423,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( @@ -513,22 +520,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, @@ -539,7 +549,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) } diff --git a/codex-rs/git-utils/src/repository_authority/authority.rs b/codex-rs/git-utils/src/repository_authority/authority.rs index 3d014f810c..db55a32a9e 100644 --- a/codex-rs/git-utils/src/repository_authority/authority.rs +++ b/codex-rs/git-utils/src/repository_authority/authority.rs @@ -155,6 +155,27 @@ impl RepositoryAuthority { }) } + pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> { + let canonical_root = std::fs::canonicalize(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 identity {} does not match runner repository {}", + canonical_root.display(), + self.active_worktree_root.display() + ), + )); + } + Ok(()) + } + fn revalidate_active_worktree_identity(&self) -> io::Result<()> { let Some(expected) = &self.active_worktree_identity else { return Ok(());