diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index bc6c8ed57c..0a73626164 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -14,14 +14,13 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use tokio::process::Command; -use tokio::time::Duration as TokioDuration; use tokio::time::timeout; use ts_rs::TS; use crate::GitSha; use crate::safe_git::DISABLED_HOOKS_PATH; -use crate::safe_git::EXECUTABLE_FILTER_CONFIG_PATTERN; -use crate::safe_git::config_output_has_untrusted_executable_helpers; +use crate::safe_git::GIT_COMMAND_TIMEOUT; +use crate::safe_git::has_configured_executable_filters_from; use crate::safe_git::isolate_tokio_git_command_environment; /// Return `true` if the project folder specified by the `Config` is inside a @@ -69,9 +68,6 @@ pub async fn get_git_repo_root_with_fs( .ok()? } -/// Timeout for git commands to prevent freezing on large repositories -const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5); - #[derive(Serialize, Deserialize, Clone, Debug, JsonSchema, TS)] pub struct GitInfo { /// Current commit hash (SHA) @@ -475,37 +471,6 @@ async fn run_git_command_with_timeout_from( } } -async fn has_configured_executable_filters_from(git: &Path, cwd: &Path) -> Option { - let mut command = Command::new(git); - isolate_tokio_git_command_environment(&mut command); - command - .args([ - "config", - "--null", - "--show-scope", - "--includes", - "--get-regexp", - EXECUTABLE_FILTER_CONFIG_PATTERN, - ]) - .current_dir(cwd) - .kill_on_drop(true); - let output = match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { - Ok(Ok(output)) => output, - _ => return None, - }; - if !output - .status - .code() - .is_some_and(|code| code == 0 || code == 1) - { - return None; - } - - Some(config_output_has_untrusted_executable_helpers( - &output.stdout, - )) -} - async fn get_git_remotes(cwd: &Path) -> Option> { let output = run_git_command_with_timeout(&["remote"], cwd).await?; if !output.status.success() { @@ -962,109 +927,6 @@ mod tests { use pretty_assertions::assert_eq; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; - use std::path::Path; - - async fn run_git(repo_path: &Path, args: &[&str]) { - let output = Command::new("git") - .args(args) - .current_dir(repo_path) - .output() - .await - .expect("run git command"); - assert!( - output.status.success(), - "git command failed: {args:?}\nstdout:\n{}\nstderr:\n{}", - String::from_utf8_lossy(&output.stdout), - String::from_utf8_lossy(&output.stderr) - ); - } - - async fn create_test_git_repo(temp_dir: &tempfile::TempDir) -> std::path::PathBuf { - let repo_path = temp_dir.path().join("repo"); - std::fs::create_dir(&repo_path).expect("create repo dir"); - run_git(&repo_path, &["init"]).await; - run_git(&repo_path, &["config", "user.name", "Test User"]).await; - run_git(&repo_path, &["config", "user.email", "test@example.com"]).await; - std::fs::write(repo_path.join("test.txt"), "test content").expect("write test file"); - run_git(&repo_path, &["add", "."]).await; - run_git(&repo_path, &["commit", "-m", "initial"]).await; - repo_path - } - - async fn configure_clean_filter(repo_path: &Path, tracked_path: &str) { - std::fs::write( - repo_path.join(".gitattributes"), - format!("{tracked_path} filter=x=y\n"), - ) - .expect("write attributes"); - run_git(repo_path, &["add", ".gitattributes"]).await; - run_git(repo_path, &["commit", "-m", "attributes"]).await; - run_git( - repo_path, - &[ - "config", - "filter.x=y.clean", - "git config codex.filterran true && git hash-object --stdin", - ], - ) - .await; - - let tracked_file = repo_path.join(tracked_path); - let contents = std::fs::read_to_string(&tracked_file).expect("read tracked file"); - std::thread::sleep(std::time::Duration::from_secs(1)); - std::fs::write(tracked_file, contents).expect("refresh tracked file"); - } - - async fn configured_filter_ran(repo_path: &Path) -> bool { - let output = Command::new("git") - .args(["config", "--get", "codex.filterran"]) - .current_dir(repo_path) - .output() - .await - .expect("read filter marker"); - output.status.success() - } - - async fn add_submodule_with_clean_filter(parent: &Path) { - let source = tempfile::tempdir().expect("submodule source"); - let source_path = source.path(); - run_git(source_path, &["init"]).await; - run_git(source_path, &["config", "user.name", "Test User"]).await; - run_git(source_path, &["config", "user.email", "test@example.com"]).await; - std::fs::write(source_path.join("nested.txt"), "original\n").expect("nested file"); - std::fs::write( - source_path.join(".gitattributes"), - "nested.txt filter=codex-test\n", - ) - .expect("nested attributes"); - run_git(source_path, &["add", "."]).await; - run_git(source_path, &["commit", "-m", "seed"]).await; - - run_git( - parent, - &[ - "-c", - "protocol.file.allow=always", - "submodule", - "add", - source_path.to_str().expect("source path"), - "nested", - ], - ) - .await; - run_git(parent, &["commit", "-m", "add submodule"]).await; - let nested = parent.join("nested"); - run_git( - &nested, - &[ - "config", - "filter.codex-test.clean", - "git config codex.filterran true && git hash-object --stdin", - ], - ) - .await; - std::fs::write(nested.join("nested.txt"), "modified\n").expect("dirty nested file"); - } #[test] fn canonicalize_git_remote_url_normalizes_github_variants() { @@ -1103,84 +965,6 @@ mod tests { } } - #[tokio::test] - async fn get_has_changes_rejects_configured_clean_filter_without_running_it() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let repo_path = create_test_git_repo(&temp_dir).await; - configure_clean_filter(&repo_path, "test.txt").await; - - assert_eq!(get_has_changes(&repo_path).await, None); - assert!(!configured_filter_ran(&repo_path).await); - } - - #[tokio::test] - async fn get_has_changes_does_not_enter_dirty_submodules() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let repo_path = create_test_git_repo(&temp_dir).await; - add_submodule_with_clean_filter(&repo_path).await; - - assert_eq!(get_has_changes(&repo_path).await, Some(false)); - assert!(!configured_filter_ran(&repo_path.join("nested")).await); - } - - #[tokio::test] - async fn diff_against_sha_does_not_enter_dirty_submodules() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let repo_path = create_test_git_repo(&temp_dir).await; - add_submodule_with_clean_filter(&repo_path).await; - run_git(&repo_path, &["config", "diff.submodule", "diff"]).await; - let head = Command::new("git") - .args(["rev-parse", "HEAD"]) - .current_dir(&repo_path) - .output() - .await - .expect("read HEAD"); - assert!(head.status.success()); - let head = String::from_utf8(head.stdout).expect("utf8 HEAD"); - - assert!( - diff_against_sha(&repo_path, &GitSha::new(head.trim())) - .await - .is_some() - ); - assert!(!configured_filter_ran(&repo_path.join("nested")).await); - } - - #[tokio::test] - async fn git_diff_to_remote_rejects_configured_clean_filter_without_running_it() { - let temp_dir = tempfile::tempdir().expect("create temp dir"); - let repo_path = create_test_git_repo(&temp_dir).await; - let remote_path = temp_dir.path().join("remote.git"); - run_git( - temp_dir.path(), - &["init", "--bare", remote_path.to_str().unwrap()], - ) - .await; - run_git( - &repo_path, - &["remote", "add", "origin", remote_path.to_str().unwrap()], - ) - .await; - let branch_output = Command::new("git") - .args(["rev-parse", "--abbrev-ref", "HEAD"]) - .current_dir(&repo_path) - .output() - .await - .expect("read branch"); - assert!(branch_output.status.success(), "read branch"); - let branch = String::from_utf8(branch_output.stdout) - .expect("branch utf8") - .trim() - .to_string(); - run_git(&repo_path, &["push", "-u", "origin", &branch]).await; - - configure_clean_filter(&repo_path, "test.txt").await; - run_git(&repo_path, &["push", "origin", &branch]).await; - - assert!(git_diff_to_remote(&repo_path).await.is_none()); - assert!(!configured_filter_ran(&repo_path).await); - } - #[cfg(unix)] #[tokio::test] async fn fsmonitor_override_rejects_configured_helper() { diff --git a/codex-rs/git-utils/src/safe_git.rs b/codex-rs/git-utils/src/safe_git.rs index 5095a9d9d3..7c3aec6921 100644 --- a/codex-rs/git-utils/src/safe_git.rs +++ b/codex-rs/git-utils/src/safe_git.rs @@ -1,11 +1,16 @@ use std::io; use std::path::Path; use std::process::Command; +use tokio::process::Command as TokioCommand; +use tokio::time::Duration; +use tokio::time::timeout; pub(crate) const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; pub(crate) const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|smudge|process)$"; pub(crate) const EXECUTABLE_PATCH_CONFIG_PATTERN: &str = r"^(filter\..*\.(clean|smudge|process)|merge\..*\.driver)$"; +/// Timeout for internal Git commands to prevent freezing on large repositories. +pub(crate) const GIT_COMMAND_TIMEOUT: Duration = Duration::from_secs(5); const ISOLATED_GIT_ENVIRONMENT: [&str; 9] = [ "GIT_DIR", @@ -35,6 +40,37 @@ pub(crate) fn isolate_tokio_git_command_environment(command: &mut tokio::process } } +pub(crate) async fn has_configured_executable_filters_from(git: &Path, cwd: &Path) -> Option { + let mut command = TokioCommand::new(git); + isolate_tokio_git_command_environment(&mut command); + command + .args([ + "config", + "--null", + "--show-scope", + "--includes", + "--get-regexp", + EXECUTABLE_FILTER_CONFIG_PATTERN, + ]) + .current_dir(cwd) + .kill_on_drop(true); + let output = match timeout(GIT_COMMAND_TIMEOUT, command.output()).await { + Ok(Ok(output)) => output, + _ => return None, + }; + if !output + .status + .code() + .is_some_and(|code| code == 0 || code == 1) + { + return None; + } + + Some(config_output_has_untrusted_executable_helpers( + &output.stdout, + )) +} + pub(crate) fn ensure_no_executable_git_config( cwd: &Path, pattern: &str, @@ -105,48 +141,5 @@ pub(crate) fn config_output_has_untrusted_executable_helpers(stdout: &[u8]) -> b } #[cfg(test)] -mod tests { - use super::*; - - #[test] - fn rejects_untrusted_nonempty_helper_values() { - assert!(!config_output_has_untrusted_executable_helpers(b"")); - assert!(!config_output_has_untrusted_executable_helpers(b"\0")); - assert!(config_output_has_untrusted_executable_helpers( - b"local\0filter.example.clean\nhelper\0" - )); - assert!(config_output_has_untrusted_executable_helpers( - b"command\0merge.example.driver\nhelper\0" - )); - assert!(config_output_has_untrusted_executable_helpers( - b"global\0merge.example.driver\nhelper\0" - )); - assert!(config_output_has_untrusted_executable_helpers( - b"system\0merge.example.driver\nhelper\0" - )); - assert!(config_output_has_untrusted_executable_helpers( - b"worktree\0filter.example.process\nhelper\0" - )); - } - - #[test] - fn allows_trusted_or_disabled_helper_values() { - assert!(!config_output_has_untrusted_executable_helpers( - b"global\0filter.lfs.process\ngit-lfs filter-process\0" - )); - assert!(!config_output_has_untrusted_executable_helpers( - b"local\0filter.example.clean\n\0" - )); - assert!(!config_output_has_untrusted_executable_helpers( - b"global\0merge.disabled.driver\n\0" - )); - } - - #[test] - fn rejects_malformed_probe_output() { - assert!(config_output_has_untrusted_executable_helpers(b"local\0")); - assert!(config_output_has_untrusted_executable_helpers( - b"local\0filter.example.clean\0" - )); - } -} +#[path = "safe_git_tests.rs"] +mod tests; diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs new file mode 100644 index 0000000000..ad0cd7ad7b --- /dev/null +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -0,0 +1,228 @@ +use super::*; +use crate::get_has_changes; +use crate::git_diff_to_remote; +use pretty_assertions::assert_eq; +use std::path::Path; +use tokio::process::Command as TokioCommand; + +#[test] +fn rejects_untrusted_nonempty_helper_values() { + assert!(!config_output_has_untrusted_executable_helpers(b"")); + assert!(!config_output_has_untrusted_executable_helpers(b"\0")); + assert!(config_output_has_untrusted_executable_helpers( + b"local\0filter.example.clean\nhelper\0" + )); + assert!(config_output_has_untrusted_executable_helpers( + b"command\0merge.example.driver\nhelper\0" + )); + assert!(config_output_has_untrusted_executable_helpers( + b"global\0merge.example.driver\nhelper\0" + )); + assert!(config_output_has_untrusted_executable_helpers( + b"system\0merge.example.driver\nhelper\0" + )); + assert!(config_output_has_untrusted_executable_helpers( + b"worktree\0filter.example.process\nhelper\0" + )); +} + +#[test] +fn allows_trusted_or_disabled_helper_values() { + assert!(!config_output_has_untrusted_executable_helpers( + b"global\0filter.lfs.process\ngit-lfs filter-process\0" + )); + assert!(!config_output_has_untrusted_executable_helpers( + b"local\0filter.example.clean\n\0" + )); + assert!(!config_output_has_untrusted_executable_helpers( + b"global\0merge.disabled.driver\n\0" + )); +} + +#[test] +fn rejects_malformed_probe_output() { + assert!(config_output_has_untrusted_executable_helpers(b"local\0")); + assert!(config_output_has_untrusted_executable_helpers( + b"local\0filter.example.clean\0" + )); +} + +async fn run_git(repo_path: &Path, args: &[&str]) { + let output = TokioCommand::new("git") + .args(args) + .current_dir(repo_path) + .output() + .await + .expect("run git command"); + assert!( + output.status.success(), + "git command failed: {args:?}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +async fn create_test_git_repo(temp_dir: &tempfile::TempDir) -> std::path::PathBuf { + let repo_path = temp_dir.path().join("repo"); + std::fs::create_dir(&repo_path).expect("create repo dir"); + run_git(&repo_path, &["init"]).await; + run_git(&repo_path, &["config", "user.name", "Test User"]).await; + run_git(&repo_path, &["config", "user.email", "test@example.com"]).await; + std::fs::write(repo_path.join("test.txt"), "test content").expect("write test file"); + run_git(&repo_path, &["add", "."]).await; + run_git(&repo_path, &["commit", "-m", "initial"]).await; + repo_path +} + +async fn configure_clean_filter(repo_path: &Path, tracked_path: &str) { + std::fs::write( + repo_path.join(".gitattributes"), + format!("{tracked_path} filter=x=y\n"), + ) + .expect("write attributes"); + run_git(repo_path, &["add", ".gitattributes"]).await; + run_git(repo_path, &["commit", "-m", "attributes"]).await; + run_git( + repo_path, + &[ + "config", + "filter.x=y.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + + let tracked_file = repo_path.join(tracked_path); + let contents = std::fs::read_to_string(&tracked_file).expect("read tracked file"); + std::thread::sleep(std::time::Duration::from_secs(1)); + std::fs::write(tracked_file, contents).expect("refresh tracked file"); +} + +async fn configured_filter_ran(repo_path: &Path) -> bool { + let output = TokioCommand::new("git") + .args(["config", "--get", "codex.filterran"]) + .current_dir(repo_path) + .output() + .await + .expect("read filter marker"); + output.status.success() +} + +async fn add_submodule_with_clean_filter(parent: &Path) { + let source = tempfile::tempdir().expect("submodule source"); + let source_path = source.path(); + run_git(source_path, &["init"]).await; + run_git(source_path, &["config", "user.name", "Test User"]).await; + run_git(source_path, &["config", "user.email", "test@example.com"]).await; + std::fs::write(source_path.join("nested.txt"), "original\n").expect("nested file"); + std::fs::write( + source_path.join(".gitattributes"), + "nested.txt filter=codex-test\n", + ) + .expect("nested attributes"); + run_git(source_path, &["add", "."]).await; + run_git(source_path, &["commit", "-m", "seed"]).await; + + run_git( + parent, + &[ + "-c", + "protocol.file.allow=always", + "submodule", + "add", + source_path.to_str().expect("source path"), + "nested", + ], + ) + .await; + run_git(parent, &["commit", "-m", "add submodule"]).await; + let nested = parent.join("nested"); + run_git( + &nested, + &[ + "config", + "filter.codex-test.clean", + "git config codex.filterran true && git hash-object --stdin", + ], + ) + .await; + std::fs::write(nested.join("nested.txt"), "modified\n").expect("dirty nested file"); +} + +async fn add_origin_and_push(repo_path: &Path, remote_path: &Path) -> String { + run_git( + remote_path.parent().expect("remote parent"), + &["init", "--bare", remote_path.to_str().expect("remote path")], + ) + .await; + run_git( + repo_path, + &[ + "remote", + "add", + "origin", + remote_path.to_str().expect("remote path"), + ], + ) + .await; + let branch_output = TokioCommand::new("git") + .args(["rev-parse", "--abbrev-ref", "HEAD"]) + .current_dir(repo_path) + .output() + .await + .expect("read branch"); + assert!(branch_output.status.success(), "read branch"); + let branch = String::from_utf8(branch_output.stdout) + .expect("branch utf8") + .trim() + .to_string(); + run_git(repo_path, &["push", "-u", "origin", &branch]).await; + branch +} + +#[tokio::test] +async fn get_has_changes_rejects_configured_clean_filter_without_running_it() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + configure_clean_filter(&repo_path, "test.txt").await; + + assert_eq!(get_has_changes(&repo_path).await, None); + assert!(!configured_filter_ran(&repo_path).await); +} + +#[tokio::test] +async fn get_has_changes_does_not_enter_dirty_submodules() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + add_submodule_with_clean_filter(&repo_path).await; + + assert_eq!(get_has_changes(&repo_path).await, Some(false)); + assert!(!configured_filter_ran(&repo_path.join("nested")).await); +} + +#[tokio::test] +async fn git_diff_to_remote_does_not_enter_dirty_submodules() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + add_submodule_with_clean_filter(&repo_path).await; + run_git(&repo_path, &["config", "diff.submodule", "diff"]).await; + let remote_path = temp_dir.path().join("remote.git"); + add_origin_and_push(&repo_path, &remote_path).await; + + assert!(git_diff_to_remote(&repo_path).await.is_some()); + assert!(!configured_filter_ran(&repo_path.join("nested")).await); +} + +#[tokio::test] +async fn git_diff_to_remote_rejects_configured_clean_filter_without_running_it() { + let temp_dir = tempfile::tempdir().expect("create temp dir"); + let repo_path = create_test_git_repo(&temp_dir).await; + let remote_path = temp_dir.path().join("remote.git"); + let branch = add_origin_and_push(&repo_path, &remote_path).await; + + configure_clean_filter(&repo_path, "test.txt").await; + run_git(&repo_path, &["push", "origin", &branch]).await; + + assert!(git_diff_to_remote(&repo_path).await.is_none()); + assert!(!configured_filter_ran(&repo_path).await); +}