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(());