diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 0000000..828120c --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,6 @@ +# Hooks, `rebase -x` and `bisect run` export GIT_DIR/GIT_INDEX_FILE (absolute in a worktree), and the +# tests' temp-repo git would follow them into this repo. The runner clears them; see +# tests/git_env_isolation_test.rs. If your own cargo config also sets a `cfg(...)` runner, cargo refuses +# to pick one: override with CARGO_TARGET__RUNNER, and the tests' tripwire still guards git. +[target.'cfg(unix)'] +runner = "dev/without-git-repo-env" diff --git a/dev/without-git-repo-env b/dev/without-git-repo-env new file mode 100755 index 0000000..e93fa46 --- /dev/null +++ b/dev/without-git-repo-env @@ -0,0 +1,6 @@ +#!/bin/sh +# Cargo runner (.cargo/config.toml): exec the binary without git's repo-local env. +# Fails closed: if git can't list the variables, nothing runs. +vars=$(git rev-parse --local-env-vars) || exit +unset $vars +exec "$@" diff --git a/src/tracked_files.rs b/src/tracked_files.rs index ee83e24..9d6717c 100644 --- a/src/tracked_files.rs +++ b/src/tracked_files.rs @@ -29,8 +29,11 @@ pub(crate) fn find_tracked_files(base_path: &Path) -> Option( relative_fixture_path: &str, @@ -75,6 +77,7 @@ pub fn copy_dir_recursive(from: &Path, to: &Path) { #[allow(dead_code)] pub fn git_reset_all(path: &Path) { + assert_git_env_isolated(); let status = Command::new("git") .arg("reset") .current_dir(path) @@ -89,6 +92,7 @@ pub fn git_reset_all(path: &Path) { #[allow(dead_code)] pub fn git_add_all_files(path: &Path) { + assert_git_env_isolated(); let status = Command::new("git") .arg("add") .arg("--all") @@ -104,6 +108,7 @@ pub fn git_add_all_files(path: &Path) { #[allow(dead_code)] pub fn init_git_repo(path: &Path) { + assert_git_env_isolated(); let status = Command::new("git") .arg("init") .current_dir(path) @@ -131,6 +136,7 @@ pub fn init_git_repo(path: &Path) { #[allow(dead_code)] pub fn is_file_staged(repo_root: &Path, rel_path: &str) -> bool { + assert_git_env_isolated(); let output = Command::new("git") .arg("diff") .arg("--name-only") @@ -149,6 +155,8 @@ pub fn is_file_staged(repo_root: &Path, rel_path: &str) -> bool { #[allow(dead_code)] pub fn build_run_config(project_root: &Path, codeowners_rel_path: &str) -> RunConfig { + // Callers pass the config to in-process runs that may stage. + assert_git_env_isolated(); let project_root = project_root.canonicalize().expect("failed to canonicalize project root"); let codeowners_file_path = project_root.join(codeowners_rel_path); let config_path = project_root.join("config/code_ownership.yml"); diff --git a/tests/git_env_isolation_test.rs b/tests/git_env_isolation_test.rs new file mode 100644 index 0000000..702257c --- /dev/null +++ b/tests/git_env_isolation_test.rs @@ -0,0 +1,146 @@ +use std::process::Command; + +mod common; + +const CHILD_ENV: &str = "CODEOWNERS_GIT_ENV_TRIPWIRE_CHILD"; + +#[cfg(unix)] +fn cargo_runner() -> Command { + Command::new(std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("dev/without-git-repo-env")) +} + +#[cfg(unix)] +#[test] +fn test_cargo_runner_strips_repo_local_git_env() { + let temp_dir = tempfile::tempdir().unwrap(); + let vars = common::repo_local_git_env_vars(); + let mut cmd = cargo_runner(); + for var in &vars { + cmd.env(var, temp_dir.path().join("outer.git")); + } + let output = cmd + .env("GIT_EDITOR", ":") + .arg("env") + .output() + .expect("failed to run the cargo runner"); + assert!( + output.status.success(), + "runner failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let env = String::from_utf8(output.stdout).unwrap(); + let leaked: Vec<&String> = vars + .iter() + .filter(|var| env.lines().any(|line| line.starts_with(&format!("{var}=")))) + .collect(); + assert!(leaked.is_empty(), "runner passed {leaked:?} through to the test binary"); + assert!( + env.lines().any(|line| line == "GIT_EDITOR=:"), + "runner dropped a git var that isn't repo-local" + ); +} + +#[cfg(unix)] +#[test] +fn test_cargo_runner_refuses_to_run_without_git() { + let empty_path = tempfile::tempdir().unwrap(); + let output = cargo_runner() + .env("PATH", empty_path.path()) + .args(["/bin/echo", "ran"]) + .output() + .expect("failed to run the cargo runner"); + assert!(!output.status.success(), "runner ran the binary without clearing git's env"); + assert!(!String::from_utf8_lossy(&output.stdout).contains("ran")); +} + +#[cfg(unix)] +#[test] +fn test_cargo_runner_preserves_the_exit_status() { + let status = cargo_runner() + .args(["/bin/sh", "-c", "exit 3"]) + .status() + .expect("failed to run the cargo runner"); + assert_eq!(status.code(), Some(3), "runner must not mask a failing test binary"); +} + +const HELPERS: &[&str] = &[ + "init_git_repo", + "git_add_all_files", + "git_reset_all", + "is_file_staged", + "build_run_config", +]; + +// Runs one tests/common helper in a fresh copy of this binary, bypassing the cargo runner. +fn run_git_helper_child(helper: &str, envs: &[(&str, std::path::PathBuf)]) -> (bool, String) { + let mut cmd = Command::new(std::env::current_exe().unwrap()); + cmd.args(["git_helper_child", "--exact", "--ignored", "--test-threads=1"]) + .env(CHILD_ENV, helper); + for (var, value) in envs { + cmd.env(var, value); + } + let output = cmd.output().expect("failed to re-run the test binary"); + let log = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + (output.status.success(), log) +} + +// Ignored so an ambient CHILD_ENV can't turn a parent test into a no-op; a plain `--include-ignored` run does nothing. +#[test] +#[ignore = "run by the tests below"] +fn git_helper_child() { + let Some(helper) = std::env::var_os(CHILD_ENV) else { + return; + }; + let temp_dir = tempfile::tempdir().unwrap(); + let path = temp_dir.path(); + match helper.to_str().unwrap() { + "init_git_repo" => common::init_git_repo(path), + "git_add_all_files" => common::git_add_all_files(path), + "git_reset_all" => common::git_reset_all(path), + "is_file_staged" => { + common::is_file_staged(path, "CODEOWNERS"); + } + "build_run_config" => { + common::build_run_config(path, "CODEOWNERS"); + } + other => panic!("unknown helper {other}"), + } +} + +#[test] +fn test_git_helpers_refuse_inherited_repo_env() { + for helper in HELPERS { + // A pre-commit hook in a worktree exports both, as absolute paths. + let outer = tempfile::tempdir().unwrap(); + let outer_git_dir = outer.path().join("outer.git"); + let (succeeded, log) = run_git_helper_child( + helper, + &[("GIT_DIR", outer_git_dir.clone()), ("GIT_INDEX_FILE", outer_git_dir.join("index"))], + ); + assert!(!succeeded, "{helper} should have refused: {log}"); + assert!( + log.contains("refusing to run git with inherited"), + "{helper} failed unexpectedly: {log}" + ); + assert!(!outer_git_dir.exists(), "{helper} wrote to the repo named by the inherited GIT_DIR"); + } +} + +#[test] +fn test_git_helpers_refuse_an_inherited_index_file_alone() { + // `git commit -a` or `git commit ` in a regular clone exports only an absolute GIT_INDEX_FILE. + let outer = tempfile::tempdir().unwrap(); + let outer_index = outer.path().join("index.lock"); + let (succeeded, log) = run_git_helper_child("init_git_repo", &[("GIT_INDEX_FILE", outer_index.clone())]); + assert!(!succeeded, "child should have refused: {log}"); + assert!(log.contains("\"GIT_INDEX_FILE\""), "tripwire didn't name GIT_INDEX_FILE: {log}"); + assert!( + !outer_index.exists(), + "test git wrote to the index named by the inherited GIT_INDEX_FILE" + ); +} diff --git a/tests/runner_api.rs b/tests/runner_api.rs index 53a1cc3..8c9d4b1 100644 --- a/tests/runner_api.rs +++ b/tests/runner_api.rs @@ -2,6 +2,8 @@ use std::path::Path; use codeowners::runner::{self, RunConfig}; +mod common; + fn write_file(temp_dir: &Path, file_path: &str, content: &str) { let file_path = temp_dir.join(file_path); let _ = std::fs::create_dir_all(file_path.parent().unwrap()); @@ -176,6 +178,8 @@ javascript_package_paths: executable_name: None, }; + // Stages in-process, so this process's own git env decides which index it writes. + common::assert_git_env_isolated(); let gv = runner::generate_and_validate(&rc, vec![], true); assert!(gv.io_errors.is_empty(), "io: {:?}", gv.io_errors); assert!(gv.validation_errors.is_empty(), "val: {:?}", gv.validation_errors); diff --git a/tests/support/git_env.rs b/tests/support/git_env.rs new file mode 100644 index 0000000..9c98d55 --- /dev/null +++ b/tests/support/git_env.rs @@ -0,0 +1,28 @@ +// Spliced with include!() into tests/common and src/tracked_files.rs's unit tests, which can't share a module. + +// The variables that pin git to one repository, per git itself. +#[allow(dead_code)] +pub fn repo_local_git_env_vars() -> Vec { + let output = std::process::Command::new("git") + .args(["rev-parse", "--local-env-vars"]) + .output() + .expect("failed to run git rev-parse --local-env-vars"); + assert!(output.status.success(), "git rev-parse --local-env-vars failed"); + String::from_utf8_lossy(&output.stdout).lines().map(str::to_owned).collect() +} + +// Inherited GIT_DIR/GIT_INDEX_FILE (hooks, worktrees) override current_dir and aim test git at the enclosing repo. +// Call before any test git write, including in-process `runner::generate(_, true)` and a CLI `generate`; +// the shared repo helpers in tests/common already do. Deliberately checks git's whole list, so a bypassed +// runner fails closed even on benign vars like GIT_CONFIG_PARAMETERS. +#[allow(dead_code)] +pub fn assert_git_env_isolated() { + let leaked: Vec = repo_local_git_env_vars() + .into_iter() + .filter(|var| std::env::var_os(var).is_some()) + .collect(); + assert!( + leaked.is_empty(), + "refusing to run git with inherited {leaked:?}; unset them, or run tests through this repo's cargo runner (.cargo/config.toml)" + ); +}