From 2c039fddff110feb55090b3a76c2c8880da4d616 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 25 Sep 2026 16:58:02 -0700 Subject: [PATCH 1/2] Keep test git from following a hook's GIT_DIR into this repo Git hooks, `git rebase -x` and `git bisect run` export GIT_DIR and GIT_INDEX_FILE, and in a linked worktree those are absolute paths to the real repository. Several tests run `git init`, `git add --all` and `git config` in temp dirs with only `current_dir` set, so under those variables they act on the enclosing repo instead. Committing from a worktree ran the pre-commit hook's `cargo test`, which flipped the shared .git/config to `core.bare = true` (breaking the primary clone), replaced the worktree's index with test fixtures, and, with --no-fail-fast, wrote `user.email=test@example.com` into the shared config and made fixture commits on the real branch. A regular clone isn't fully safe either: `git commit -a` and `git commit ` export an absolute GIT_INDEX_FILE there too. Two test-only layers: - A cargo runner (.cargo/config.toml -> dev/without-git-repo-env) clears every variable `git rev-parse --local-env-vars` lists, then execs the test binary. It runs before the test process starts, so it also covers what per-command scrubbing can't: in-process production `ls-files`/`git add` reached from unit tests, and the CLI children tests launch. It fails closed if git can't produce the list. - A tripwire, `assert_git_env_isolated()`, runs before every test git write in tests/common, the tracked_files unit tests, and runner_api's in-process stage. It refuses if any of those variables survive, covering runs that bypass the runner (a test binary run directly, `--manifest-path` from outside the repo, non-unix hosts, an overriding runner). Production code is unchanged and keeps honoring GIT_DIR/GIT_INDEX_FILE: users run `codeownership validate` from their own hooks, where `git_stage` must stage into the index being committed. The runner only affects what cargo launches inside this repo. Five regression tests in tests/git_env_isolation_test.rs cover the runner stripping the variables, failing closed without git, and preserving the binary's exit status, and the tripwire refusing both the worktree shape and a lone GIT_INDEX_FILE. Each fails when the behavior it guards is removed. --- .cargo/config.toml | 3 + dev/without-git-repo-env | 5 ++ src/tracked_files.rs | 20 ++++++ tests/common/mod.rs | 28 ++++++++ tests/git_env_isolation_test.rs | 113 ++++++++++++++++++++++++++++++++ tests/runner_api.rs | 4 ++ 6 files changed, 173 insertions(+) create mode 100644 .cargo/config.toml create mode 100755 dev/without-git-repo-env create mode 100644 tests/git_env_isolation_test.rs diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 0000000..a208c5f --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,3 @@ +# Hooks, `rebase -x` and `bisect run` export GIT_DIR/GIT_INDEX_FILE (absolute in a worktree); tests' temp-repo git would follow them here. +[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..131634d --- /dev/null +++ b/dev/without-git-repo-env @@ -0,0 +1,5 @@ +#!/bin/sh +# Cargo runner (.cargo/config.toml): exec the binary without git's repo-local env; fail if git can't list it. +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..05be776 100644 --- a/src/tracked_files.rs +++ b/src/tracked_files.rs @@ -29,8 +29,27 @@ pub(crate) fn find_tracked_files(base_path: &Path) -> Option = String::from_utf8_lossy(&output.stdout) + .lines() + .filter(|var| std::env::var_os(var).is_some()) + .map(str::to_owned) + .collect(); + assert!( + leaked.is_empty(), + "refusing to run git with inherited {leaked:?}; run tests through cargo, whose runner clears them (.cargo/config.toml)" + ); + } + #[test] fn test_untracked_files() { + assert_git_env_isolated(); let tmp_dir = tempfile::tempdir().unwrap(); assert!(find_tracked_files(tmp_dir.path()).is_none()); @@ -58,6 +77,7 @@ mod tests { #[test] fn test_tracked_files_from_subdirectory() { + assert_git_env_isolated(); let tmp_dir = tempfile::tempdir().unwrap(); let backend_dir = tmp_dir.path().join("backend"); let tracked_file = backend_dir.join("app/models/foo.rb"); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8c55df0..f7599b4 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -12,6 +12,30 @@ pub enum OutputStream { Stderr, } +// The variables that pin git to one repository, per git itself. +#[allow(dead_code)] +pub fn repo_local_git_env_vars() -> Vec { + let output = 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. +#[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:?}; run tests through cargo, whose runner clears them (.cargo/config.toml)" + ); +} + #[allow(dead_code)] pub fn run_codeowners( relative_fixture_path: &str, @@ -75,6 +99,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 +114,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 +130,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 +158,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") diff --git a/tests/git_env_isolation_test.rs b/tests/git_env_isolation_test.rs new file mode 100644 index 0000000..eec0da7 --- /dev/null +++ b/tests/git_env_isolation_test.rs @@ -0,0 +1,113 @@ +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"); +} + +// Re-runs the child half of `test_git_helpers_refuse_inherited_repo_env` directly, bypassing the cargo runner. +fn run_git_helper_child(envs: &[(&str, std::path::PathBuf)]) -> (bool, String) { + let mut cmd = Command::new(std::env::current_exe().unwrap()); + cmd.args(["test_git_helpers_refuse_inherited_repo_env", "--exact", "--test-threads=1"]) + .env(CHILD_ENV, "1"); + 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) +} + +#[test] +fn test_git_helpers_refuse_inherited_repo_env() { + if std::env::var_os(CHILD_ENV).is_some() { + let temp_dir = tempfile::tempdir().unwrap(); + common::init_git_repo(temp_dir.path()); + return; + } + + // 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(&[("GIT_DIR", outer_git_dir.clone()), ("GIT_INDEX_FILE", outer_git_dir.join("index"))]); + assert!(!succeeded, "child should have refused: {log}"); + assert!(log.contains("refusing to run git with inherited"), "unexpected failure: {log}"); + assert!(!outer_git_dir.exists(), "test git 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(&[("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); From 5f8c76b38f0c849bcaca5e57cca7ac6cf3359dfc Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Fri, 25 Sep 2026 18:02:55 -0700 Subject: [PATCH 2/2] Share the git env tripwire and test it at every helper - Move the tripwire to tests/support/git_env.rs, spliced with include!() into tests/common and the tracked_files unit tests instead of duplicated. - Guard build_run_config too, since its callers stage in-process. - Re-run each tests/common helper in a child under an inherited GIT_DIR, so dropping any one helper's tripwire call fails a test. - Make the child an ignored test, so an ambient marker variable can't turn the parent test into a silent no-op. - Fix the tripwire message for runs that overrode the runner, and document the conflict with a user's own cfg(...) runner in .cargo/config.toml. --- .cargo/config.toml | 5 ++- dev/without-git-repo-env | 3 +- src/tracked_files.rs | 18 +-------- tests/common/mod.rs | 26 ++----------- tests/git_env_isolation_test.rs | 65 +++++++++++++++++++++++++-------- tests/support/git_env.rs | 28 ++++++++++++++ 6 files changed, 87 insertions(+), 58 deletions(-) create mode 100644 tests/support/git_env.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index a208c5f..828120c 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -1,3 +1,6 @@ -# Hooks, `rebase -x` and `bisect run` export GIT_DIR/GIT_INDEX_FILE (absolute in a worktree); tests' temp-repo git would follow them here. +# 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 index 131634d..e93fa46 100755 --- a/dev/without-git-repo-env +++ b/dev/without-git-repo-env @@ -1,5 +1,6 @@ #!/bin/sh -# Cargo runner (.cargo/config.toml): exec the binary without git's repo-local env; fail if git can't list it. +# 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 05be776..9d6717c 100644 --- a/src/tracked_files.rs +++ b/src/tracked_files.rs @@ -29,23 +29,7 @@ pub(crate) fn find_tracked_files(base_path: &Path) -> Option = String::from_utf8_lossy(&output.stdout) - .lines() - .filter(|var| std::env::var_os(var).is_some()) - .map(str::to_owned) - .collect(); - assert!( - leaked.is_empty(), - "refusing to run git with inherited {leaked:?}; run tests through cargo, whose runner clears them (.cargo/config.toml)" - ); - } + include!("../tests/support/git_env.rs"); #[test] fn test_untracked_files() { diff --git a/tests/common/mod.rs b/tests/common/mod.rs index f7599b4..25c544a 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -12,29 +12,7 @@ pub enum OutputStream { Stderr, } -// The variables that pin git to one repository, per git itself. -#[allow(dead_code)] -pub fn repo_local_git_env_vars() -> Vec { - let output = 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. -#[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:?}; run tests through cargo, whose runner clears them (.cargo/config.toml)" - ); -} +include!("../support/git_env.rs"); #[allow(dead_code)] pub fn run_codeowners( @@ -177,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 index eec0da7..702257c 100644 --- a/tests/git_env_isolation_test.rs +++ b/tests/git_env_isolation_test.rs @@ -64,11 +64,19 @@ fn test_cargo_runner_preserves_the_exit_status() { assert_eq!(status.code(), Some(3), "runner must not mask a failing test binary"); } -// Re-runs the child half of `test_git_helpers_refuse_inherited_repo_env` directly, bypassing the cargo runner. -fn run_git_helper_child(envs: &[(&str, std::path::PathBuf)]) -> (bool, String) { +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(["test_git_helpers_refuse_inherited_repo_env", "--exact", "--test-threads=1"]) - .env(CHILD_ENV, "1"); + cmd.args(["git_helper_child", "--exact", "--ignored", "--test-threads=1"]) + .env(CHILD_ENV, helper); for (var, value) in envs { cmd.env(var, value); } @@ -81,21 +89,46 @@ fn run_git_helper_child(envs: &[(&str, std::path::PathBuf)]) -> (bool, String) { (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] -fn test_git_helpers_refuse_inherited_repo_env() { - if std::env::var_os(CHILD_ENV).is_some() { - let temp_dir = tempfile::tempdir().unwrap(); - common::init_git_repo(temp_dir.path()); +#[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}"), } +} - // 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(&[("GIT_DIR", outer_git_dir.clone()), ("GIT_INDEX_FILE", outer_git_dir.join("index"))]); - assert!(!succeeded, "child should have refused: {log}"); - assert!(log.contains("refusing to run git with inherited"), "unexpected failure: {log}"); - assert!(!outer_git_dir.exists(), "test git wrote to the repo named by the inherited GIT_DIR"); +#[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] @@ -103,7 +136,7 @@ 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(&[("GIT_INDEX_FILE", outer_index.clone())]); + 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!( 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)" + ); +}