From 9df57aaec4dc5df3da4d4ee2e16db648cd735432 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 04:24:03 -0400 Subject: [PATCH 01/39] Comment Git version compatibility for EXE_INFO --- gix-path/src/env/git/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index f99c3ba871c..c28aceae44e 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -89,6 +89,7 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } + // git 2.8.0 and higher support --show-origin. cmd.args(["config", "-l", "--show-origin"]) .current_dir(env::temp_dir()) .stdin(Stdio::null()) From 650a1b5cf25e086197cc55a68525a411e1c28031 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 05:52:07 -0400 Subject: [PATCH 02/39] fix: Parse installation config path more robustly This adds the `-z`/`--null` and `--name-only` options in the `git` invocation that tries to obtain the configuration file path associated with the `git` installation itself. The benefits are: - Parsing is more reliable for paths containing unusual characters, because `-z`/`--null` causes all paths to be output literally. Previously, `"` characters were trimmed from the ends, but this would not always extract a correct path, because when a path contains characters that cause `git` to enclose it in double quotes, those characters are usually represented in a symbolic form, usually with `\` escapes. In some scenarios, such as usually on Windows when the escaped character is itself a `\` and not in the leading position, the mangled path would be usable, but more often it would not. - The volume of output is less, because `--name-only` casues values not to be included in the output. - The combination of `-z`/`--null` and `--name-only` makes the output format simpler, and the parsing logic is accordingly simpler. `git` has supported the `-z`/`--null` and `--name-only` options even before support for `--show-origin` was added in Git 2.8.0, so this change should have no effect on Git version compatibility. --- gix-path/src/env/git/mod.rs | 6 +++--- gix-path/src/env/git/tests.rs | 13 ++++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index c28aceae44e..d8947a96ff8 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -90,7 +90,7 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { cmd.creation_flags(CREATE_NO_WINDOW); } // git 2.8.0 and higher support --show-origin. - cmd.args(["config", "-l", "--show-origin"]) + cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(env::temp_dir()) .stdin(Stdio::null()) .stderr(Stdio::null()); @@ -138,8 +138,8 @@ pub(super) fn install_config_path() -> Option<&'static BStr> { fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> { let file = source.strip_prefix(b"file:")?; - let end_pos = file.find_byte(b'\t')?; - file[..end_pos].trim_with(|c| c == '"').as_bstr().into() + let end_pos = file.find_byte(b'\0')?; + file[..end_pos].as_bstr().into() } /// Given `config_path` as obtained from `install_config_path()`, return the path of the git installation base. diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 033b6f0b983..86b2b7b28f1 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -17,12 +17,15 @@ fn config_to_base_path() { #[test] fn first_file_from_config_with_origin() { - let macos = "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig credential.helper=osxkeychain\nfile:/Users/byron/.gitconfig push.default=simple\n"; + let macos = + "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig\0credential.helper\0file:/Users/byron/.gitconfig\0push.default\0"; let win_msys = - "file:C:/git-sdk-64/etc/gitconfig core.symlinks=false\r\nfile:C:/git-sdk-64/etc/gitconfig core.autocrlf=true"; - let win_cmd = "file:C:/Program Files/Git/etc/gitconfig diff.astextplain.textconv=astextplain\r\nfile:C:/Program Files/Git/etc/gitconfig filter.lfs.clean=gix-lfs clean -- %f\r\n"; - let win_msys_old = "file:\"C:\\ProgramData/Git/config\" diff.astextplain.textconv=astextplain\r\nfile:\"C:\\ProgramData/Git/config\" filter.lfs.clean=git-lfs clean -- %f\r\n"; - let linux = "file:/home/parallels/.gitconfig core.excludesfile=~/.gitignore\n"; + "file:C:/git-sdk-64/etc/gitconfig\0core.symlinks\0file:C:/git-sdk-64/etc/gitconfig\0core.autocrlf\0"; + let win_cmd = + "file:C:/Program Files/Git/etc/gitconfig\0diff.astextplain.textconv\0file:C:/Program Files/Git/etc/gitconfig\0filter.lfs.clean\0"; + let win_msys_old = + "file:C:\\ProgramData/Git/config\0diff.astextplain.textconv\0file:C:\\ProgramData/Git/config\0filter.lfs.clean\0"; + let linux = "file:/home/parallels/.gitconfig\0core.excludesfile\0"; let bogus = "something unexpected"; let empty = ""; From de2f35f5c355cacb0c076674126e8243ce24ec5c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 07:07:41 -0400 Subject: [PATCH 03/39] Extract git_cmd helper for EXE_INFO For readability, and because this code may be expande soon. + Move the first_file_from_config_with_origin helper up, so EXE_INFO and its helpers are all together. --- gix-path/src/env/git/mod.rs | 43 +++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index d8947a96ff8..57b0cc55f39 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -81,21 +81,6 @@ pub(super) static EXE_NAME: &str = "git"; /// /// The git executable is the one found in PATH or an alternative location. pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { - let git_cmd = |executable: PathBuf| { - let mut cmd = Command::new(executable); - #[cfg(windows)] - { - use std::os::windows::process::CommandExt; - const CREATE_NO_WINDOW: u32 = 0x08000000; - cmd.creation_flags(CREATE_NO_WINDOW); - } - // git 2.8.0 and higher support --show-origin. - cmd.args(["config", "-lz", "--show-origin", "--name-only"]) - .current_dir(env::temp_dir()) - .stdin(Stdio::null()) - .stderr(Stdio::null()); - cmd - }; let mut cmd = git_cmd(EXE_NAME.into()); gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path"); let cmd_output = match cmd.output() { @@ -115,6 +100,28 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned) }); +fn git_cmd(executable: PathBuf) -> Command { + let mut cmd = Command::new(executable); + #[cfg(windows)] + { + use std::os::windows::process::CommandExt; + const CREATE_NO_WINDOW: u32 = 0x08000000; + cmd.creation_flags(CREATE_NO_WINDOW); + } + // git 2.8.0 and higher support --show-origin. + cmd.args(["config", "-lz", "--show-origin", "--name-only"]) + .current_dir(env::temp_dir()) + .stdin(Stdio::null()) + .stderr(Stdio::null()); + cmd +} + +fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> { + let file = source.strip_prefix(b"file:")?; + let end_pos = file.find_byte(b'\0')?; + file[..end_pos].as_bstr().into() +} + /// Try to find the file that contains git configuration coming with the git installation. /// /// This returns the configuration associated with the `git` executable found in the current `PATH` @@ -136,12 +143,6 @@ pub(super) fn install_config_path() -> Option<&'static BStr> { PATH.as_ref().map(AsRef::as_ref) } -fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> { - let file = source.strip_prefix(b"file:")?; - let end_pos = file.find_byte(b'\0')?; - file[..end_pos].as_bstr().into() -} - /// Given `config_path` as obtained from `install_config_path()`, return the path of the git installation base. pub(super) fn config_to_base_path(config_path: &Path) -> &Path { config_path From ccd04018905bd0c00857f4ef10ce6f1156b6e835 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 08:04:57 -0400 Subject: [PATCH 04/39] Reorder gix_path::env::git tests to match order in code The mismatch may otherwise become cumbersome as more tests are added. + Run `cargo fmt`, reformatting whitespace from recent changes. --- gix-path/src/env/git/tests.rs | 99 +++++++++++++++++------------------ 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 86b2b7b28f1..d58c18eef3c 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -1,53 +1,3 @@ -use std::path::Path; - -#[test] -fn config_to_base_path() { - for (input, expected) in [ - ( - "/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig", - "/Applications/Xcode.app/Contents/Developer/usr/share/git-core", - ), - ("C:/git-sdk-64/etc/gitconfig", "C:/git-sdk-64/etc"), - ("C:\\ProgramData/Git/config", "C:\\ProgramData/Git"), - ("C:/Program Files/Git/etc/gitconfig", "C:/Program Files/Git/etc"), - ] { - assert_eq!(super::config_to_base_path(Path::new(input)), Path::new(expected)); - } -} - -#[test] -fn first_file_from_config_with_origin() { - let macos = - "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig\0credential.helper\0file:/Users/byron/.gitconfig\0push.default\0"; - let win_msys = - "file:C:/git-sdk-64/etc/gitconfig\0core.symlinks\0file:C:/git-sdk-64/etc/gitconfig\0core.autocrlf\0"; - let win_cmd = - "file:C:/Program Files/Git/etc/gitconfig\0diff.astextplain.textconv\0file:C:/Program Files/Git/etc/gitconfig\0filter.lfs.clean\0"; - let win_msys_old = - "file:C:\\ProgramData/Git/config\0diff.astextplain.textconv\0file:C:\\ProgramData/Git/config\0filter.lfs.clean\0"; - let linux = "file:/home/parallels/.gitconfig\0core.excludesfile\0"; - let bogus = "something unexpected"; - let empty = ""; - - for (source, expected) in [ - ( - macos, - Some("/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig"), - ), - (win_msys, Some("C:/git-sdk-64/etc/gitconfig")), - (win_msys_old, Some("C:\\ProgramData/Git/config")), - (win_cmd, Some("C:/Program Files/Git/etc/gitconfig")), - (linux, Some("/home/parallels/.gitconfig")), - (bogus, None), - (empty, None), - ] { - assert_eq!( - super::first_file_from_config_with_origin(source.into()), - expected.map(Into::into) - ); - } -} - #[cfg(windows)] mod locations { use std::ffi::{OsStr, OsString}; @@ -404,3 +354,52 @@ mod locations { assert!(super::super::ALTERNATIVE_LOCATIONS.is_empty()); } } + +use std::path::Path; + +#[test] +fn first_file_from_config_with_origin() { + let macos = + "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig\0credential.helper\0file:/Users/byron/.gitconfig\0push.default\0"; + let win_msys = "file:C:/git-sdk-64/etc/gitconfig\0core.symlinks\0file:C:/git-sdk-64/etc/gitconfig\0core.autocrlf\0"; + let win_cmd = + "file:C:/Program Files/Git/etc/gitconfig\0diff.astextplain.textconv\0file:C:/Program Files/Git/etc/gitconfig\0filter.lfs.clean\0"; + let win_msys_old = + "file:C:\\ProgramData/Git/config\0diff.astextplain.textconv\0file:C:\\ProgramData/Git/config\0filter.lfs.clean\0"; + let linux = "file:/home/parallels/.gitconfig\0core.excludesfile\0"; + let bogus = "something unexpected"; + let empty = ""; + + for (source, expected) in [ + ( + macos, + Some("/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig"), + ), + (win_msys, Some("C:/git-sdk-64/etc/gitconfig")), + (win_msys_old, Some("C:\\ProgramData/Git/config")), + (win_cmd, Some("C:/Program Files/Git/etc/gitconfig")), + (linux, Some("/home/parallels/.gitconfig")), + (bogus, None), + (empty, None), + ] { + assert_eq!( + super::first_file_from_config_with_origin(source.into()), + expected.map(Into::into) + ); + } +} + +#[test] +fn config_to_base_path() { + for (input, expected) in [ + ( + "/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig", + "/Applications/Xcode.app/Contents/Developer/usr/share/git-core", + ), + ("C:/git-sdk-64/etc/gitconfig", "C:/git-sdk-64/etc"), + ("C:\\ProgramData/Git/config", "C:\\ProgramData/Git"), + ("C:/Program Files/Git/etc/gitconfig", "C:/Program Files/Git/etc"), + ] { + assert_eq!(super::config_to_base_path(Path::new(input)), Path::new(expected)); + } +} From 1ee98bfa9383888e73219b47234f369930d51332 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 11:19:17 -0400 Subject: [PATCH 05/39] Make EXE_INFO testable and add a basic test for it This extracts the code used to lazily initialize `EXE_INFO` into a separate `exe_info` helper, which is an implementation detail, but can be called any number of times without caching from the tests. It also adds one test. The tests of downstream functions (those that use `EXE_INFO`) remain more important. But this will allow the test to be expanded, or another added, to test more expectations that `EXE_INFO` is supposed to satisfy. --- gix-path/src/env/git/mod.rs | 6 ++++-- gix-path/src/env/git/tests.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 57b0cc55f39..8cabb4346b8 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -80,7 +80,9 @@ pub(super) static EXE_NAME: &str = "git"; /// Invoke the git executable to obtain the origin configuration, which is cached and returned. /// /// The git executable is the one found in PATH or an alternative location. -pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { +pub(super) static EXE_INFO: Lazy> = Lazy::new(exe_info); + +fn exe_info() -> Option { let mut cmd = git_cmd(EXE_NAME.into()); gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path"); let cmd_output = match cmd.output() { @@ -98,7 +100,7 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { }; first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned) -}); +} fn git_cmd(executable: PathBuf) -> Command { let mut cmd = Command::new(executable); diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index d58c18eef3c..53aff608f4a 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -357,6 +357,19 @@ mod locations { use std::path::Path; +#[test] +fn exe_info() { + let path = super::exe_info() + .map(crate::from_bstring) + .expect("Nonempty config in the test environment"); + + assert!( + path.is_absolute(), + "Absolute, unless overridden such as with GIT_CONFIG_SYSTEM" + ); + assert!(path.exists(), "Exists, since `git config` just found an entry there"); +} + #[test] fn first_file_from_config_with_origin() { let macos = From 5a300e6c827b62a0883aeb328f828f9e898ed76b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 11:39:22 -0400 Subject: [PATCH 06/39] Test that EXE_INFO never has local scope config This tests that `EXE_INFO` never picks up a local scope configuration file when looking for the configuration associated with the installation -- not even if the system and global scopes provide no variables, such that the first row of of `git config -l` output would usually be a local scope entry. This is a regression test for a second bug that #1523 ended up fixing (in addition to the performance bug that motivatd that PR). By running `git config -l` in a directory that `git` is unlikely to treat as a local repository, that avoids reading local scope configuration even if there are no broader-scope config files for `git` to read from. This is important because local configuration variables may be unsuitable to treat as if they are the installation configuration, for reasons that are more important than distinguishing between installation (usually system scope) and global scope variables. Consider, for example, if `gix clone` is run from inside a repository. For operations on other repositories, including clones, that may be undertaken from within a first repository, the first repository's local configuration variables are not allowed to have any effect. This is important for correctness and, in some cases, for security. But if the local configuration file, usually `.git/config`, is wrongly recognized to be the configuration file associated with the `git` installation, then values from it will be used. Then, among various other possible incorrect outcomes: - Secrets from the repository `gix clone` is run from can be leaked to the remote of the repository being cloned, for example if authentication credentials are stored in `http.extraHeader`. - More rarely, the configuration in the repository `gix clone` is run from may expose information related to the repository being cloned. For example, a custom value of `core.sshCommand` with extra logging may be configured in the first repository, but where logging authentication or other information from cloning the second repository would unexpectedly expose it as well. While #1523 fixed this in nearly all practical situations, there are some situations where there could still be a problem, such that the way `git config -l` commands are run should be further hardened. (The test added here is to aid in testing such changes.) - If the system has an old version of `git` without a patch for https://github.com/git/git/security/advisories/GHSA-j342-m5hw-rr3v or other vulnerabilities where `git` would perform operations on a repository owned by another user, then `git config -l` may treat a shared temporary directory as a `git` repository, if another user on the system has created and populated a `.git` directory there. `env::temp_dir()` almost always gives a shared temporary directory on Unix-like systems, and in rare cases can give one on Windows. The other user on the system may, on some systems, even be an account representing a low-privileged service/daemon. - I think it is possible, though I do not know of cases, for a downstream distribution to patch such vulnerabilities in `git`, but do so in a way where `git config -l` commands refuse to display any configuration when run from a repository owned by another user (and not listed in `safe.directory`). If this were to happen, then we would fail to discover a path to the config file associated with the `git` installation. I expect that rarely to happen because patches are usually backported rather than being written in a different way, and `git` does not have this problem. - In the rare case that the user has made the temporary directory `env::temp_dir()` a `git` repository, this should still ideally not cause that local scope configuration to be used here. - The `TMPDIR` environment variable on a Unix-like system, or the `TMP` and/or `TEMP` environment variables on Windows, may be set to incorrect values (such as directories that do not exist or should not be used, or the empty string), or unset. Then `env::temp_dir()` may not find a suitable directory, and any of the above problems could potentially occur. These are all unlikely compared to the situation before, so even if this is hardened further, the biggest improvement was in #1523. The test introduced in this commit passes with the code as it currently stands, and fails when the `current_dir(env::temp_dir())` line #1523 added is commented out, demonstrating that it works as a regression test. However, the test is not quite done: - It does not detect the bug on macOS. This is to be expected, because on macOS there are usually "unknown"-scope values at the beginning of the output, and these should be (and are) detected as belonging to the installation. This is why it is not correct to pass `--system`. See 20ef4e9, 6b1c243, and: https://github.com/Byron/gitoxide/pull/1523#issuecomment-2291515070 This is probably acceptable. - The test wrongly fails on macOS, because it thinks correct values like /Library/Developer/CommandLineTools/usr/share/git-core/gitconfig from the "unknown" scope on macOS may be from the local scope. As currently written, the test expects that when there is nothing from the system and global scopes, `EXE_INFO` returns `None`, rather than `Some(path)` where `path` is from the macOS "unknown" scope associated with the installation. This false positive will have to be fixed, so that the test suite can pass on macOS, and so the test is at least somewhat useful on macOS (while possibly being more precise on other OSes). --- Cargo.lock | 3 ++- gix-path/Cargo.toml | 3 ++- gix-path/src/env/git/tests.rs | 16 ++++++++++++++++ gix-path/tests/fixtures/local_config.sh | 7 +++++++ gix-path/tests/realpath/mod.rs | 4 ++-- 5 files changed, 29 insertions(+), 4 deletions(-) create mode 100755 gix-path/tests/fixtures/local_config.sh diff --git a/Cargo.lock b/Cargo.lock index e72c7e2c5ba..61422f532a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2215,11 +2215,12 @@ name = "gix-path" version = "0.10.10" dependencies = [ "bstr", + "gix-testtools", "gix-trace 0.1.9", "home", "known-folders", "once_cell", - "tempfile", + "serial_test", "thiserror", "windows 0.58.0", "winreg", diff --git a/gix-path/Cargo.toml b/gix-path/Cargo.toml index ccadfacdff4..a661140b427 100644 --- a/gix-path/Cargo.toml +++ b/gix-path/Cargo.toml @@ -24,7 +24,8 @@ once_cell = "1.17.1" home = "0.5.5" [dev-dependencies] -tempfile = "3.3.0" +gix-testtools = { path = "../tests/tools" } +serial_test = { version = "3.1.0", default-features = false } [target.'cfg(windows)'.dev-dependencies] known-folders = "1.1.0" diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 53aff608f4a..4c438214a3b 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -357,7 +357,10 @@ mod locations { use std::path::Path; +use serial_test::serial; + #[test] +#[serial] fn exe_info() { let path = super::exe_info() .map(crate::from_bstring) @@ -370,6 +373,19 @@ fn exe_info() { assert!(path.exists(), "Exists, since `git config` just found an entry there"); } +#[test] +#[serial] +fn exe_info_never_from_local_scope() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); + let null_device = if cfg!(windows) { "NUL" } else { "/dev/null" }; + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_SYSTEM", null_device) + .set("GIT_CONFIG_GLOBAL", null_device); + let info = super::exe_info(); + assert!(info.is_none()); +} + #[test] fn first_file_from_config_with_origin() { let macos = diff --git a/gix-path/tests/fixtures/local_config.sh b/gix-path/tests/fixtures/local_config.sh new file mode 100755 index 00000000000..26dda271a0d --- /dev/null +++ b/gix-path/tests/fixtures/local_config.sh @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +set -eu -o pipefail + +git init -q + +# Shouldn't be necessary, as a repo starts with some config vars, but this removes any doubt. +git config --local foo.bar baz diff --git a/gix-path/tests/realpath/mod.rs b/gix-path/tests/realpath/mod.rs index 331f86dff3f..321ca67fcb7 100644 --- a/gix-path/tests/realpath/mod.rs +++ b/gix-path/tests/realpath/mod.rs @@ -2,7 +2,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use bstr::ByteVec; -use tempfile::tempdir; +use gix_testtools::tempfile; use gix_path::{realpath::Error, realpath_opts}; @@ -28,7 +28,7 @@ fn fuzzed_timeout() -> crate::Result { #[test] fn assorted() -> crate::Result { - let cwd = tempdir()?; + let cwd = tempfile::tempdir()?; let cwd = cwd.path(); let symlinks_disabled = 0; From fd065ac281f64c4bee18e0ce68930b1c8444a4c7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 17:09:00 +0000 Subject: [PATCH 07/39] Add generated archive for local_config.sh Produced on Ubuntu 24.04 LTS, running git 2.43.0-1ubuntu7.1. --- .../generated-archives/local_config.tar | Bin 0 -> 49664 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 gix-path/tests/fixtures/generated-archives/local_config.tar diff --git a/gix-path/tests/fixtures/generated-archives/local_config.tar b/gix-path/tests/fixtures/generated-archives/local_config.tar new file mode 100644 index 0000000000000000000000000000000000000000..7a915f5d054f52d997bde7e3c88364226b571902 GIT binary patch literal 49664 zcmeHwZF|#Jwsyb9zhYPTOcEN|u@hjP(9ofQPIC?=To4X(E?!*4mJ=0QGLlWmQ0BLv z`(A5rNtTm12}#e)Gv;c+miO!0Yp?gU)M$Ql#nYGE6YZ#zAV#S5GBdD{bzp2mE6OXdFi4o1TfFx$ODhyVG8r|4knZ z#^?Drdtug1qtQ4@;+vR~=YMx|vwH8{_w`n*_09DCZ?)U^KnHGOG1W2u&F}xqI1Ywk z&!pif$)a(Rp4>NKFB+TiQIwf}GziUd2|bq0I5F&j-@{U;My(3h#rJ;+2*~~KCUHMH zys`Dpz5kF6D)%1*puM>NH+CY$z^}VW8osS975BfNq{CqRF-$YC1G8<`YfB^p!=xAD z+c=$swWWg~<(Gai$nb5D90r3y8ullnUN8wfHwSI4Xq5t3Dk>o??=idxJbqAAPnE!HpALh~jjl1hr`rqEzxVNDH z*LMVGj&#ew0AL20PZ=*WzMnv`OZma#gWpe+;r0xg)NYw4QH&nrqhM@kLmQjpXnbUr zZ!H^2huvg24C8UuK+B(!)C6XfCclN`{`u&niD$ z84tSerwoP$76T!;35*69q&FEJz)ldv*(gbc0pQ_&V!}8$fH_eNiyVCodWSV7I#VMi z@=9pvHEOl+Q`i+ArI_hwVTx&dimVG_{`GiJzW-_H2cyy81WdMjbd4ln-u=IOXGZ?J zv(bXT;OnQAghP z>MN9?)9|AY5(&Z6_t_G+=ty>$HPP1B#;aZp2GX(dKN+u{58;_V{RxB@gl_6#{d4O( zul^VBOKt1MNL}UrV^d-DF}%jhj~h7W{)6?;-2crxcNhGBKJohc78kw$!{B|WOulbM zlkCVO;w~2Tj#t@<4Os-i@x+APqXfz!+yYaraUm<(A{!n+qS#*MwT|hCq>@;4#VE%| zVJuhK(;vO^n50I~_Ig*j;1=6?yFoqlV#c_Gf8E{hJbU@#=g!`<{oTE%ot?dxPxoUelG@Tz_%Rq54}VRp0*!V_cN}caOsE`;t6m;PlN!uQ8ZI4B1a5Gpj)SQ%QfOkT;`@%cR8- zS}X)b8|~uc*nCqoh+dYdOkbf>q(+qjalI}eHELVl#VM&VAEGQZmL6VCYAoIVV!ms< z`$WGv_rG-)VUtz+&*sAZGw0@B&(GI!|8Yyq4sI0g6ZAii%8=rcrwScJ3Ah9fC-euX z#b3-m2}CYPFT7_MKqSMXfDVP~Rcw@QZ+4cph#}6<7cZkNVia9^c~0uPH$yLqD@R&! z%e*W*HQ)yUks^r-dOaB4UPOm+*z=Qq-v^in@V@Xjf~!!h&7*`$ve?+*UhX@fe(7vJKbH-?_Wbwt`j}RGo z+8+_&Lz(&E%l)7Co_BU1J$|}xeuwv_XZ+YKXU%tSRvNb-yjgABt~aN)v1Q)0nxo}a z%ytM1qwzmv#(P)C&>rOAmS=wdU33Y2k!KGc$Ya84p8<+a?dZ~E1y!Lp84V(Q2~U9* zbQM*;JrD6n)Y>`Y^1I4{mhHr~C6mE^iG57s=!5aQZbyiSY`%MKmfqfO@$Y7Dc~#c= zxu()NcGva~Qh9rw$*I*gKl^LO_kVpG&yKQNz>t6b5)rJ=?aKVHVQ#SCL_}e z$FK`vB^W0TkHuDin8c_b;dgOfTVV~<%n`0pkdX!j#zix(bSEjC4s<`aS7nGn0BfAO z7BC&eSPZWbkfe5CM}a^?439knqvvSi?2A2`Km%D!c>oTJL1_9=pStC2A43pyh{^e) zvncM0mwIachIt0VA&4#v(-;Q!)ifLgV;JQewNI0TUU8w@$4UBL2!0xZ$HIJ)(MwgPs6|9i%rCxY2IXu+Uhus%W{uld?9zTB$TD@-E zpr?%aCiY~som_oU+yHi_L_&udwEyIb!;um_K+k&-%rP(HY%-!N3qaLf6&se(9Cj_c zg)BEPuW5jT;E(qcP=RMeGnb%rH;8$q8VxqP4<vy#5^rCcS z6&qo@G#sZV_`PMn|CS|jXY~H?eoZV)SalkHm_%UN|M_C?>DJcYUcm5Ecdf|z+;hym zgi*d20yyD*r+Yqoc6!fe&$wz#KL`+oanx~FSC*Jf4ou}aN9M{COzzrt1d{BrM9@cB zv3DIW?5)(3RXK$vy>^{6%qD{|9{UeIcLz!L{g*ZKm(TK5ZrYbG9PSGN%2iDDvTInW zOUI~(p3+Bd&r<_=GVL)nc8hO1YW|OP) z6Wl(yHK60bX4^Od8HG?d-6g*n82 z9-3abf*Yf2$2nNk$oD8%==M0m(zfMj+beHw@BIAaQAYr5Kkq(WGs~{0@qdG#Cr*4A zU&rGUNU%%1MWh$RjrPhaqT{)F7-PS+)L_om6bX0L)U68(KVkSx0`XNDI1ZQTgwKTN z6HBQ{W#k40uOUu#kAnCRkXIfvOKU~&kHJU8MMn*Q*bB}#=!Gc=*DVgR56z_CblhdMS1KzCbmchxNs19uaIF|3 z4kDdp3rbd?9SD&svnA27a9enLNTqN+&~IfHoNH!%4R&)AelT9wQ=&E7N-mx^`}~be zur&39o%+Lv*FYYHF)*pTIN_OVZ?Uv5x^hLCZS(#2-qPczkGSZi*Pc8jd78d(c=CB_ zI8KNTOYRW1%xjOx<-J{#$0d-;PstcVy!ODEq?M(A-diMmEPcUWSi!kOl{lAQCRwDM z^jx|sO&erqmaZ1$vo613+ZBSTE#yA$J~eqOC93u@dMR_w$s9SNC7I3p=6gpD9+2^2 zhBM?7!6?}mP+!L?_H`z+a81wHm-~5kYeH~dSqXbfY!|#qN)~bdmnWlPu&O?BYaS>5qp{y-OTns3~UCU!AGu4;!l$(ETM-4Aq#c9 zUrs(uQxLChNGb9gL=1U?)0V4m#jXctT}V}3n-kXdBbGdDPHc!%!oyDnkl968Z`=lF z03~F4rGX`6l}<}iUqO_x2c73Lu8q#&`t7% zD6mleg28cclG&?RQU@XYkoNVX6q=_{EZCW34OWT{3|*Yw!bK=;j;;3Ssg0@fQqle_ z<*HkKp9&$?AWG}IJ(A1iVRn)&(_C7&54tCZh^b$;q@~mmyM7hL8||&FzlP(TB<)qBGFT5l+zH=j{qqz2;<`E)>hXJNeJ0}m78&k=i8(fzy5I$ zC|PV6*5i*8UtGPoQP*Vso$LR*!(5+=|L@M7dyDwbKLQI1{~s)SXPa}afir+&i8ugy zq3AfGp$hB_$vR*bL?35@1-EiGSjdJjtBN@it@1b}Mv<>TW0Ke|^iIKG)Ub+HSc=4$ zk-h26R85^o4xR|rgg1;RgFdeK+jIvl} zVGy=EqYx{Dk*TWw%7DmT7@zE@@%Tp_L5n6#U~FZcJX~Orgtuc%W-USCyk)1|g|dm< zT7}JzC$MGq1K6?F;8Mdv5Kzx>I&@SV;09xIdy2|g4x8Y5ftZ;G^hsL%x* z3_Ykpj7g%tg;$~C=X=Ef<=v_m^1hzM0TRIG%x_R1peyjDyr4i4)gw~(E@FUTq)?cP z@kiRrps>!s_Rbw+qvL2)Y(>w73VjKAaC9($3smrt0!#`Qri^5vhyf15qu^tdOw#*6 zx?{>r(tx%f{6-gQ8Zs&|lzg;u=b|fyv9^9BvQgd|!H-INh!_h#hH(V1fi>O3y@(Tc zsBXOtHvVqPeJ#<Xf^J2wk7E?$uddp=apt?pO~DfG5#dNkYh7QHRIVm5)# z6Zb{NhV!8kD-9?q8p$>VvsT;{>mec&qh!e%r9t`^L2HENimjlj8vpt}3`foosHPHk#&#Q1G)%UMP+0iyp!(85n36>4zIZMr znspG4kLfJqV>hWpNbK{0TxwjIKtdCIc&x>KfMbGXQ8JI{F{7dd2jWo(Wz5pc9h`fQ zWStjVrc_#75CZuHFj&rnN|ogyWoe*0u#|yH^GG-ZFBu$IonAN^Bqy}{!YLxN0|*C` zXaIWBbm6&VdyTGQ)>D#h0Sbo^v=YS;6py(86Z)HS^K6O4=ca@;3(CAv8Qfqm2-TR* ziiL+)u>>CkK(HLUZA|x-Tv}?eAg$tf40^$6%oJM^L=aXQhSMGne{iy9!p0%CB*`B= zK7$doXBrKU_@t)d+=t1m2nJ$cW#0;}>D@#=7wyA7;w*fGxxlOF!`KSaG|{YN5H(m- zj%*0FsE#j3JRC703%*)=yT_>IVtC?Ip4$w!7+CY^Gn9vZWxc97q=l_cz{iok(sP7N zXR?7Pf{kf1ImC5l>YGKtq>lt-fW{b>*XV#dcfj9h2Lht}U_25D&up2;zdYOjpU(dC z$B#QNety{M{QUTD+=E=kU@RLjeyB91{h-+kKQ`mZU|<)`v9~!?je1&bNxo}n4rV;^ zlLJY-_N~a~``H-BWWLYGSv>`4_C$OKv@R?F*!*eGt8+s9G9QMgK`=*!Dg}i*Oj3(n zmGj${jwf>@!hjJUk;r0xRNMfmy!6o``k2ky`dnW*K9H5WYUnl!-ko+9jLep9W8wGzMeBaxCnC01b#`CpbF+LP+B2`C^&+_GfXF!s?}~td#EJLwIkqv16|J0W}<=K3C0n#R7B-`;HJl#6VLc*r_Nv z-TZk7CvV<^Oa$%G<2gh-1`F|o_V*s`8Too7&T`1YEPH|_<{7CXj$n;WMB)}GIboz2 z(QFid+`OGGmD&z~ibJ#d6;10%4}jtTp8}`b$`^3fR4bIg7%K<-li7jk(Ak!xS|9AN z`E+{Hs>e;u^B6%+ki~OxR2wRq>K3Qa4`Wj{0fPD)P;xkw#K5Y(_u>mzIUK^1%*TGZ z71@M5Ym5e`?nDvQg6yJja$^56&JJOYg6qYkkaQ5tLXjQeL6CBVT#Qf9Qco0u?zr11 zuLe|zQ3U>V{Sm>{W)(7^`USk`kAHc%`=s;J?vuxwhxqt8!i`;5xW z7m*W-SC)8|$G4)iTTK-{}Q#K292u>9Y8=72nv1Nk_OnFo!~;qsu{HvWZvJ~fB3|S_M+%W6`p>{I}hGL5qA>_V)jq=eU;Sf1sZ~4AVoG1#(T&|6KnMYJ<<@f8BvoXp#T<_2T{9 z5eoUAg3j3vAV?{5>wiEhtmc6UKG`%d#3H#Yu&kV3rHTOh7IxUGS-V;SWJ=s}ih#IW zVA%j&ttth1F%Ve~csnbWTviLH-j7q#JB1obZ4XPNn^bkm1we#1G1P{CY{`!^+umT^ z4~H0~`>*ztYrFMshPn6u9^U&=)&Do-eF4P~H}`2#|K~#WzYZkO8}V&1v`3hRpqB-Q zctHRw{6M1ybwEPH?ENp>y)7BYX@JxnmQ2v>C}{c6reLhc2N>`~>)OAtey*5nb@Ck% zBd`<|%JD(&U<0CGa$MQ~eqdfHbPr@A4MI9}Lg>}-DM8JH-MYF}rvl43*gqT?&S7Ps zy*f`cC|h9_pvFkuoIzFx%qB0dMRjR_an4Gl9d!|b`0O$`i41X-MX2|0{vra-4sGXz z)(h!G^M%soE-V$|5-sgq79`UAIeS;n&PBKH{6M)eqBx1Gf`DFqS$%k+rKpZU^N#Eo zM>4Y}j0nK!?p=8KS*QWjquW4Ji^+}94M=Ntz@F&vmym4eGrS{e5WpRQc?+N0UJcho zKW})h%#RO30~6c$k={Y>@W8wkxRN|z?UOs zWTfznhkTvFv9ucj@3E~1=6l@_KImp1Adyz5(b_cD zlVi7GA%S8$iSA2#7vFV|nQcy3bPPesXU;l1sXGVexYeqJqpAvyPe$Q3X-N5Um&nJe zj-c0F5#~Zelsl}}s;D||h-&+(Rp2hq>o2>`NtErKQ;Q4r&Ree%K}Crd_bLRz(ko?9 zNloek^AU!Xq;aHJw>~ft+lfY{_TFmEmPsmglwwK9)ZYr0KS?Sjxl5`Ug*bZ#Rp36<75#Xn>F_{yAOB&j1$@Z} zoD!LnYQ6#$kATpP8Uqe2G^c!ZGd?b?6#wxK?`tq1J~SyGO8A;!f#oV9IAPHP4^0*!=5*Z z{VACN4T>a55& zothQLq7nF(`qG>(M6lqY_V~*qzeGfam>b9oL6riWiJuvqg%}no5S5Ei(;^H*HT-DB zSgz<6NZXQ>*J5g~^mxl@DMaa9?&^P-h%3oe6rVHuniW^{fPC zmPhl53%@W!{NxfaC=GHt5a{Zz;Q#~~*YzAk-f>a}ig)B; zV)T4p1ZmG-{VAAM-9(82=ucB8=_HBr>6Gy8y4LH=yVEoUcE7GTCBS_EB_>+wrDIqWTUh>K$+;E`v}R|aQf0>XQxqc|6f&+fSYO2Q z-<)mY4;Z;*|Dk;=4G&O)@|tZxl!MNG{|h2LHfQWVo11tiURnx|6JJxA*@m5<_N5nGDJW_nrS3Ky#)oFw6+Mq;1}y_K1Q%C1U+Uma7i@h z)Z4Cu<#-xoqPy*x6+~OW2MV2BLY3t)=jEU-a9%sYtE@o9diCWCFoq?^%%hMJU^!fp zQPeHW4tc1A6@{=QF>y=S#Bv;d8ZSH9L7)h?rGU;Z5Cy=(Zh-iD6)Cth$-dC{upr_R zc?uMb^MyN!Ycx4vQ2;=~hBG3j!9LsOtTmDm-I-c0z-X|51PYmF~UlWO$Gu?gjLzFTs7k!nc(0# z{4M|k+6b`d`>or3mK@ToQRY2GbTm^?Eb7{3cF!zMhMBMioddB$n4&-~UF8ooz`PU1 z9sRm3r$y-=*uh9!pe?FnY@-686f}6J&GIGvO8r%xCNxw{jpba!BH_q-b%7)?r!VF{ zfGTk`)(H$3PoAG(Pf*QNBH99$dHp+_6-I?!Lef4O5)E70yX2Gi#F`49 zdawljU>(izjxHufgLT-U)2aX`hs8U0KK0M`3VmUzIq1m0mb$mk=q`>V`RkPK4!T~w zv<|fr6c2FWfJ(3mTJXzxS7t*^K8r+pg1<`g>CDM#8#v}=yrLYQML`1DQ4r)5(-y)^ z2qDZmE{kBxm1|w@z?PI`*y~!u8%v#$L(!8bP}$Nfa+Uz-?hN)hRUeH(*vnWWYene2 zK=M0lR=r+lB@TIx;)$SeusqooFzZkbKOAP|KkRXE>8w;RM63hi zNtTwH?^c`<=(|-TdH+i*;9T1eP=bVu_<~5VK{Q@zzKNSKe8}=zZ{;W2DY$l628O6# zgEK7AY~9th2FSS zCUS`Hzkl;+Z>J+wOvFiKnX-ni!p?p;3DTY;j!Hp=CL?rFawb^o&9Ou)Fyu5J>@`LV zWo;i6oWVYQCaIxT9V(=Pp;FO{Adu9|ZL`M`aTU%%5yX1lJbPdmij)$rg8joFMSv`j6?_EMr4f;;mG<<~S&oGn)O#c8 zrGyK4Y9Iq(kL8Rzw>SwrKtKG4)0f(Q+;)+sCOpCJfJtn^gTo->Vz5M}JV`L86MQrc z48A>r?G@;rFbi@8w^yN(0J0_S+S?^cVMy5$XLMM!reGz5$BXUgX7m(ewSh$(eK!*& zIN5am=B72^U)p1mj*19wS_?B8+iDmRj7mpUCaC9ygjs}Z54cfD;KNf5p+lN0-`#{C zYq?F{2GXjh+5a^OIrpGQ< zS!8G6g#-lywQU4A*Pw^#c2Q1zlPj24{gRq!pd<=eD=Q*uh=)ny!3$>*Y6Tf`N5RMj zck{VqOY-udZm8swa8`BaC+D` zGY#E<{P#eXxFc;`OFLk>*=a*I1?uQO7l5u!&LV5I+QT($f&6Pn^t3Wl2^pRbQvOhK z{#d%aAAPcQ^nf_D0oaSL60D_3(&tC%gZxlYjd5jmDGVUMgsH4kx6bx;Oq)xw)R_C? zSI|&(ZYZsGoK)u7kx@wj>{1FA!OV#PQf+FlC|dAss*Xsg(z8R_?vxZ#6==TH{{a!s ziUJF`PwgnPt#%S+`(lrAL}2Pr2t|7$0H729Zr%p> ze>uan{kKvB{+jH-=i7hR+xObD`G4#87WKdX2)vlK|DGNH3qy|NCXf}|YMou)}B<^tGM9Z!?guF6F3w zbfL0_XQN4q5G*nA;EGq?_fquMnr5P^a6ZxWSi{u{0W-xmn10WOcHy8dcq<~SL|aqt zg~JyuY)da+(6>SCQ;=38b}a8^C#YC+$I20iqUJ8Ok{mge+Pxe=y<6rP1YB%f0yN8O zweW(R2(x`S7&!nsBQ#Ck5bqh}m`@BpqEtqtJ5k>8VKYj2t_W6Iu$n&NaE}}mKVH|ZfW_Bl~`X1keAt0FlkO-AWi~>-7P!Um3mV0(}*3|B1m31U>Z|?Mb9NFe^_mkfg#8uL`lT&zBAh> z?tI$f4Xo^#4dpZCgL6vHzK2A&w*UCbuOb1yNqrKUuTfovg`Cq2=Iki;H4_v`ZTb{2 zwl6E00kuSh6(-Utc=MV7(TG zY5s3X&^7UWw~jgdf1`D0qZ;mGG7wTG@2$Evb8y+cRKv zQID~dlG4g8wS1hji@M~|a~iRUS&9&C!a~(Q`pO@^*xB6`VTT0SmaC(vk=L*z7estz zLj#x5C&;=H-w#RvShF7};AwHsuCavtJ_oC$LDxwRawN>pYUpd-TZ}ZqneC(r%0H9g zzzj1)41fd#;|R&#x%Q6AFD6WM2N8Tg5SNESozBB2PqufsX(WsAB)*tv-CGN|FeB( z_Wi$`twsL#Rgd&i{R;UHtnX~|&pG0s7ITFCa5DeC`Z&=Yv2aNijV4K2u0J>Mlw+O-y;c4}R@ ze2Cyv(sOHtviks?FKWGVBC-J$MdW4bE?Ds8^Cx6a=eNa%u}_Xd=Pq~0@+VqL5!=M< z%yN?AzxWA)L!MQO#@0*$ZfkN`&)^D_McW)k`{`oqi`HX zVL!yy>VU{IJ8i&Xb&baK=#0N%>e!B?*ioRx`Pm`xgPy$AMsoKPUNp<#UHeW~bfA4i z7SHUz94cs5l$ETc7)g-fPa<>|6B4!U@^FZ2;*_%YlEG?DxLRV!8UL5N{;!i8A^cxY zEX*#&IWFFKj{S!}SM5JA3ElhV&c&y=ddq+F`VVscKZasHQr^3@s_=a01B8zKT%Isdzx3;KVx6TMt7Ie)b> z-_Yq_iT<}X*EbgT|8hrprPk&1Z${7)uQbZ}ovv{I+Z(V6E%^WWaJ`OSr_P^mr?`$m zD`Q;g{M!rpuL8y!`P%8ggr9su&`Jl1utBGD1G8M^{O`0E{Qm}yVrIyh^KSwl=-B2< z$7fn!?E%kU>N)ZQbbb9N|95@8JsbaV?_PTm|8X^NU#b@!c=ohSyRo^^SQPZQ6#V|x z8_(WFjok TUltHpKwtrZ1qA;8g24X*s$x21 literal 0 HcmV?d00001 From 49e0715f67d6fa6b8d009dbf684f7ce013b4104c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 13:28:50 -0400 Subject: [PATCH 08/39] Fix EXE_INFO no local scope test for macOS By setting `GIT_CONFIG_NOSYSTEM` (to `1`), rather than setting `GIT_CONFIG_SYSTEM` (to `/dev/null`). The test still sets `GIT_CONFIG_GLOBAL` to `/dev/null. Although setting `GIT_CONFIG_SYSTEM` to `/dev/null` (or even to a file with actual configuration in it) does not take precedence over the "unknown" scope (usually) under `/Library`, and documentation in git(1) and git-config(1) seems to suggest `GIT_CONFIG_SYSTEM` and `GIT_CONFIG_NOSYSTEM` would affect the same scope(s), it turns out that setting `GIT_CONFIG_NOSYSTEM` to `1` is sufficient to cause both the system scope and the installation-associated "unknown" scope to be omitted from `git config -l` output. This fixes both the false positive and false negative in that test case, which is now as precise on macOS as on other systems. In addition, because `GIT_CONFIG_NOSYSTEM` can be set in real-world scenarios (to make a configuration reproducible or to suppress broken or wanted systemwide configuration, as documented), the bug where a local configuration file path could be used affected macOS more fully than I had anticipated. This commit also improves a variable name and adds an assertion message to clarify the test. --- gix-path/src/env/git/tests.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 4c438214a3b..8374cdef410 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -378,12 +378,11 @@ fn exe_info() { fn exe_info_never_from_local_scope() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); - let null_device = if cfg!(windows) { "NUL" } else { "/dev/null" }; let _env = gix_testtools::Env::new() - .set("GIT_CONFIG_SYSTEM", null_device) - .set("GIT_CONFIG_GLOBAL", null_device); - let info = super::exe_info(); - assert!(info.is_none()); + .set("GIT_CONFIG_NOSYSTEM", "1") + .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }); + let maybe_path = super::exe_info(); + assert!(maybe_path.is_none(), "Finds no config path if the config would be local"); } #[test] From 65d51518e7fcb7fd69f5121e76d2abd2a01446d4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 24 Aug 2024 18:31:27 -0400 Subject: [PATCH 09/39] Code formatting --- gix-path/src/env/git/tests.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 8374cdef410..2f6822dbed9 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -382,7 +382,10 @@ fn exe_info_never_from_local_scope() { .set("GIT_CONFIG_NOSYSTEM", "1") .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }); let maybe_path = super::exe_info(); - assert!(maybe_path.is_none(), "Finds no config path if the config would be local"); + assert!( + maybe_path.is_none(), + "Finds no config path if the config would be local" + ); } #[test] From 287f2676b7c4603d3eb412f9c2478db9f7ec2647 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 25 Aug 2024 21:16:07 -0400 Subject: [PATCH 10/39] Test EXE_INFO no local config even if temp dir is a repo This adds a test that `EXE_INFO` does not return the path to a local scope configuration file even if no other configuration file is available and even if the temp dir (from `env::temp_dir()`) is itself a Git repository. This test is not quite done, because it is strangely passing, even though the implementation is not yet hardened to the degree that it should be able to avoid doing this. Specifically, although Git will refuse to use (and, in `git config -l`, will omit) the local scope configuration from a repository that is owned by another user (and not allowed via `safe.directory`), this test is attempting to cause a local repository owned by the current user to be used as the directory `env::temp_dir()` will return. Once that test bug is fixed, the test should start failing. Then, when further hardening against unusual temporary directories (and vulnerable `git` versions) is implemented, it should pass again. --- gix-path/src/env/git/tests.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 2f6822dbed9..70cb2fe2acf 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -388,6 +388,25 @@ fn exe_info_never_from_local_scope() { ); } +#[test] +#[serial] +fn exe_info_never_from_local_scope_even_if_temp_is_here() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let repo_str = repo.to_str().expect("valid Unicode"); + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_NOSYSTEM", "1") + .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }) + .set("TMPDIR", repo_str) // Mainly for Unix. + .set("TMP", repo_str) // Mainly for Windows. + .set("TEMP", repo_str); // Mainly for Windows, too. + let maybe_path = super::exe_info(); + assert!( + maybe_path.is_none(), + "Finds no config path if the config would be local even in a `/tmp`-like dir" + ); +} + #[test] fn first_file_from_config_with_origin() { let macos = From 744bb38726309cd2d97003ca721fc7850866eefa Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 25 Aug 2024 21:52:20 -0400 Subject: [PATCH 11/39] Fix bug in new test where temp dir should be a repo The problem was that the path used for environment variables that inform `env::temp_dir()` was a relative path. Although `temp_dir()` itself will return an absolute path (resolved from it), the test changes the current directory. This change occurred between when the relative path was obtained by the test code and when it was resolved by the code under test. The path was therefore resolved to a different location (the last few path components were repeated) that never exists. Since it didn't exist, the `git config -l` command failed, and no installation config file path was returned, causing the test to wrongly pass (since `None` is the correct result when only local scope configuration files are available). The fix is to use an absolute path. In addition, this canonicalizes the path rather than merely resolving it, and does so even before changing directory to it, so that if `temp_dir()` "fixes up" the path in ways beyond resolving it, such as resolving symlinks, the test is less likely to wrongly pass. The test now rightly fails, since the hardening it tests for has not yet been implemented in the code under test. Further changes to the test are still warranted, at least to clean it up, and possibly also to make it slightly more robust. --- gix-path/src/env/git/tests.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 70cb2fe2acf..a18d365791b 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -391,7 +391,10 @@ fn exe_info_never_from_local_scope() { #[test] #[serial] fn exe_info_never_from_local_scope_even_if_temp_is_here() { - let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") + .expect("script succeeds") + .canonicalize() + .expect("path is valid and exists"); let repo_str = repo.to_str().expect("valid Unicode"); let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); let _env = gix_testtools::Env::new() @@ -400,6 +403,9 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { .set("TMPDIR", repo_str) // Mainly for Unix. .set("TMP", repo_str) // Mainly for Windows. .set("TEMP", repo_str); // Mainly for Windows, too. + //assert_eq!(std::env::temp_dir(), repo); + //assert_eq!(std::env::temp_dir(), Path::new("/a/b/c"), "Bogus assertion to debug test"); + //std::thread::sleep(std::time::Duration::from_secs(3600)); let maybe_path = super::exe_info(); assert!( maybe_path.is_none(), From 15cec4ed114b2cd8acc333bb847ff18a9b59589c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 25 Aug 2024 22:36:55 -0400 Subject: [PATCH 12/39] Check that the test affects `env::temp_dir()` as desired This check is probably overzealous in that a path that identifies the same directory in a different way that is non-equivalent under path equality would also be acceptable. But a more restrictive check is simpler, and since we have canonicalized the path and used it after that for both changing the directory and setting the environment variables we intend that `env::temp_dir()` will use, that is unlikely to be a problem. That it not to say that this cannot break in practice. Rather, it can break, but if it does, there is a substantial likelihood that the test is not ensuring the behavior it wishes to check. So to preserve it as a regression test, failures of this new assertion should be examined. This commit also removes some old cruft (commented out test code I had used while investigating a test bug) and rewords some custom assertion messages so it is clearer what the expectation is. --- gix-path/src/env/git/tests.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index a18d365791b..1eff6406a06 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -384,7 +384,7 @@ fn exe_info_never_from_local_scope() { let maybe_path = super::exe_info(); assert!( maybe_path.is_none(), - "Finds no config path if the config would be local" + "Should find no config path if the config would be local" ); } @@ -403,13 +403,11 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { .set("TMPDIR", repo_str) // Mainly for Unix. .set("TMP", repo_str) // Mainly for Windows. .set("TEMP", repo_str); // Mainly for Windows, too. - //assert_eq!(std::env::temp_dir(), repo); - //assert_eq!(std::env::temp_dir(), Path::new("/a/b/c"), "Bogus assertion to debug test"); - //std::thread::sleep(std::time::Duration::from_secs(3600)); + assert_eq!(std::env::temp_dir(), repo, "It is likely that setting up the test failed"); let maybe_path = super::exe_info(); assert!( maybe_path.is_none(), - "Finds no config path if the config would be local even in a `/tmp`-like dir" + "Should find no config path if the config would be local even in a `/tmp`-like dir" ); } From 7280a2d2f8b55a594ae134dd9a0a7a1668b7b56c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 00:02:13 -0400 Subject: [PATCH 13/39] fix: More robustly ensure "installation" config is not local When invoking `git` to find the configuration file path associated with the `git` installation itself, this sets `GIT_DIR` to a path that cannot be a `.git` directory for any repository, to keep `git config -l` from including any local scope entries in the output of the `git config -l ...` command that is used to find the origin for the first Git configuration variable. Specifically, a path to the null device is used. This is `/dev/null` on Unix and `NUL` on Windows. This is not a directory, and when treated as a file it is always treated as empty: reading from it, if successful, reaches end-of-file immediately. This problem is unlikely since #1523, which caused this `git` invocation to use a `/tmp`-like location (varying by system and environment) as its current working directory. Although the goal of that change was just to improve performance, it pretty much fixed the bug where local-scope configuration could be treated as installation-level configuration when no configuration variables are available from higher scopes. This change further hardens against two edge cases: - If the `git` command is an old and unpatched vulnerable version in which `safe.directory` is not yet implemented, or in which https://github.com/git/git/security/advisories/GHSA-j342-m5hw-rr3v or other vulnerabilities where `git` would perform operations on untrusted local repositories owned by other users are unpatched, then a `.git` subdirectory of a shared `/tmp` or `/tmp`-like directory could be created by another account, and its local configuration would still have been used. (This is not a bug in gitoxide per se; having vulnerable software installed that other software may use is inherently insecure. But it is nice to offer a small amount of protection against this when readily feasible.) - If the `/tmp`-like location is a Git repository owned by the current user, then its local configuration would have been used. Any path guaranteed to point to a nonexistent entry or one that is guaranteed to be (or to be treated as) an empty file or directory should be sufficient here. Using the null device, even though it is not directory-like, seems like a reasonably intuitive way to do it. A note for Windows: There is more than one reasonable path to the null device. One is DOS-style relative path `NUL`, as used here. One of the others, which `NUL` in effect resolves to when opened, is the fully qualified Windows device namespace path `\\.\NUL`. I used the former here to ensure we avoid any situation where `git` would misinterpret a `\` in `\\.\NUL` in a POSIX-like fashion. This seems unlikely, and it could be looked into further if reasons surface to prefer `\\.\NUL`. One possible reason to prefer `\\.\NUL` is that which names are treated as reserved legacy DOS device names changes from version to version of Windows, with Windows 11 treating some of them as ordinary filenames. However, while this affects names such as `CON`, it does not affect `NUL`, at least written unsuffixed. I'm not sure if any Microsoft documentation has yet been updated to explain this in detail, but see: - https://github.com/dotnet/docs/issues/41193 - https://github.com/python/cpython/pull/95486#issuecomment-1881469554 - https://github.com/python/cpython/pull/95486#issuecomment-1882134234 At least historically, it has been possible on Windows, though rare, for the null device to be absent. This was the case on Windows Fundamentals for Legacy PCs (WinFPE). Even if that somehow were ever to happen today, this usage should be okay, because attempting to open the device would still fail rather than open some other file (as may even be happening in Git for Windows already), the name `NUL` would still presumably be reserved (much as the names `COM?` where `?` is replaced with a Unicode superscript 1, 2, or 3 are reserved even though those devices don't really exist), and I think `git config -l` commands should still shrug off the error opening the file and give non-local-scope configuration, as it does when `GIT_DIR` is set to a nonexistent location. --- gix-path/src/env/git/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 8cabb4346b8..d4012dfc161 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -82,6 +82,11 @@ pub(super) static EXE_NAME: &str = "git"; /// The git executable is the one found in PATH or an alternative location. pub(super) static EXE_INFO: Lazy> = Lazy::new(exe_info); +#[cfg(windows)] +static NULL_DEVICE: &str = "NUL"; +#[cfg(not(windows))] +static NULL_DEVICE: &str = "/dev/null"; + fn exe_info() -> Option { let mut cmd = git_cmd(EXE_NAME.into()); gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path"); @@ -113,6 +118,7 @@ fn git_cmd(executable: PathBuf) -> Command { // git 2.8.0 and higher support --show-origin. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(env::temp_dir()) + .env("GIT_DIR", NULL_DEVICE) .stdin(Stdio::null()) .stderr(Stdio::null()); cmd From 572307708e52f57624b5ef5dabfbb4b4eb68982d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 03:54:12 -0400 Subject: [PATCH 14/39] Set GIT_WORK_TREE along with GIT_DIR, to avoid confusion This is really just to make things less surprising when debugging. This also adds a comment to explain why `GIT_DIR` is being set. --- gix-path/src/env/git/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index d4012dfc161..95c5929b0f3 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -118,7 +118,8 @@ fn git_cmd(executable: PathBuf) -> Command { // git 2.8.0 and higher support --show-origin. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(env::temp_dir()) - .env("GIT_DIR", NULL_DEVICE) + .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. + .env("GIT_WORK_TREE", NULL_DEVICE) // Not needed, but clarifies intent. .stdin(Stdio::null()) .stderr(Stdio::null()); cmd From 9641660ebf0f330b5f02db8d7bda8dd3266e890b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 04:28:50 -0400 Subject: [PATCH 15/39] Slightly improve quality of test failure messages This uses `assert_eq!` with `None`, instead of `assert!` on an `is_none()` call, so that we get `left` and `right` expressions in test failure output. It also includes some other changes that are just for code readability: running `cargo fmt`, and replacing an expression in the tests that evaluates to the same string as `crate::NULL_DEVICE` with that (as the goal is not to test the value of `NULL_DEVICE`). --- gix-path/src/env/git/tests.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 1eff6406a06..ba9c90d1af4 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -382,8 +382,8 @@ fn exe_info_never_from_local_scope() { .set("GIT_CONFIG_NOSYSTEM", "1") .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }); let maybe_path = super::exe_info(); - assert!( - maybe_path.is_none(), + assert_eq!( + maybe_path, None, "Should find no config path if the config would be local" ); } @@ -399,14 +399,18 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); let _env = gix_testtools::Env::new() .set("GIT_CONFIG_NOSYSTEM", "1") - .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }) + .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE) .set("TMPDIR", repo_str) // Mainly for Unix. .set("TMP", repo_str) // Mainly for Windows. .set("TEMP", repo_str); // Mainly for Windows, too. - assert_eq!(std::env::temp_dir(), repo, "It is likely that setting up the test failed"); + assert_eq!( + std::env::temp_dir(), + repo, + "It is likely that setting up the test failed" + ); let maybe_path = super::exe_info(); - assert!( - maybe_path.is_none(), + assert_eq!( + maybe_path, None, "Should find no config path if the config would be local even in a `/tmp`-like dir" ); } From 60465a5b5900916d3e7ab80e025e01f237c21e7b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 05:38:15 -0400 Subject: [PATCH 16/39] Test EXE_INFO no local config even if temp dir doesn't exist This doesn't pass yet, because the `git config -l` command is currently being run from `env::temp_dir()` (even when some better directory may be discoverable). --- gix-path/src/env/git/tests.rs | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index ba9c90d1af4..8a10db242cf 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -359,9 +359,7 @@ use std::path::Path; use serial_test::serial; -#[test] -#[serial] -fn exe_info() { +fn check_exe_info() { let path = super::exe_info() .map(crate::from_bstring) .expect("Nonempty config in the test environment"); @@ -373,6 +371,28 @@ fn exe_info() { assert!(path.exists(), "Exists, since `git config` just found an entry there"); } +#[test] +#[serial] +fn exe_info() { + check_exe_info(); +} + +#[test] +#[serial] +fn exe_info_tolerates_broken_tmp() { + let empty = gix_testtools::tempfile::tempdir().expect("We can create a new temporary subdirectory"); + let nonexistent = empty.path().join("nonexistent"); + assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); + let nonexistent_str = nonexistent.to_str().expect("valid Unicode"); + + let _env = gix_testtools::Env::new() + .set("TMPDIR", nonexistent_str) // Mainly for Unix. + .set("TMP", nonexistent_str) // Mainly for Windows. + .set("TEMP", nonexistent_str); // Mainly for Windows, too. + + check_exe_info(); +} + #[test] #[serial] fn exe_info_never_from_local_scope() { From 703f882f1887b3c69553c79a8d6e4d3214be904f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 05:51:52 -0400 Subject: [PATCH 17/39] Clarify assert and expect messages This rewords some messages in recently added tests to use "It ..." in a way that people reading test run output are likely to find intuitive, using it in places where it will be clear, and rewording to remove it in one place where it would be misleading. --- gix-path/src/env/git/tests.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 8a10db242cf..65c74927033 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -362,13 +362,16 @@ use serial_test::serial; fn check_exe_info() { let path = super::exe_info() .map(crate::from_bstring) - .expect("Nonempty config in the test environment"); + .expect("It is present in the test environment (nonempty config)"); assert!( path.is_absolute(), - "Absolute, unless overridden such as with GIT_CONFIG_SYSTEM" + "It is absolute (unless overridden such as with GIT_CONFIG_SYSTEM)" + ); + assert!( + path.exists(), + "It should exist on disk, since `git config` just found an entry there" ); - assert!(path.exists(), "Exists, since `git config` just found an entry there"); } #[test] @@ -380,7 +383,7 @@ fn exe_info() { #[test] #[serial] fn exe_info_tolerates_broken_tmp() { - let empty = gix_testtools::tempfile::tempdir().expect("We can create a new temporary subdirectory"); + let empty = gix_testtools::tempfile::tempdir().expect("can create new temporary subdirectory"); let nonexistent = empty.path().join("nonexistent"); assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); let nonexistent_str = nonexistent.to_str().expect("valid Unicode"); @@ -426,7 +429,7 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { assert_eq!( std::env::temp_dir(), repo, - "It is likely that setting up the test failed" + "Possible test bug: Setting up the test may have failed" ); let maybe_path = super::exe_info(); assert_eq!( From 79af25969e148efd456dacb05100fb2a90ebc054 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 06:18:14 -0400 Subject: [PATCH 18/39] Check `env::temp_dir()` in both tests that set temp vars Before, `exe_info_never_from_local_scope_even_if_temp_is_here` had checked it, but the newer test `exe_info_tolerates_broken_tmp` had not. Now they both check that it returns the value that was set as the `TMPDIR`, `TMP`, and `TEMP` environment variables. This also extracts duplicate code (while avoiding making more of it by adding duplicating that check) and adjusts spacing for readability. --- gix-path/src/env/git/tests.rs | 41 ++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 65c74927033..e4f92f12ade 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -359,6 +359,23 @@ use std::path::Path; use serial_test::serial; +fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { + let path_str = path.to_str().expect("valid Unicode"); + + let env = gix_testtools::Env::new() + .set("TMPDIR", path_str) // Mainly for Unix. + .set("TMP", path_str) // Mainly for Windows. + .set("TEMP", path_str); // Mainly for Windows, too. + + assert_eq!( + std::env::temp_dir(), + path, + "Possible test bug: Setting up the test may have failed" + ); + + env +} + fn check_exe_info() { let path = super::exe_info() .map(crate::from_bstring) @@ -386,13 +403,8 @@ fn exe_info_tolerates_broken_tmp() { let empty = gix_testtools::tempfile::tempdir().expect("can create new temporary subdirectory"); let nonexistent = empty.path().join("nonexistent"); assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); - let nonexistent_str = nonexistent.to_str().expect("valid Unicode"); - - let _env = gix_testtools::Env::new() - .set("TMPDIR", nonexistent_str) // Mainly for Unix. - .set("TMP", nonexistent_str) // Mainly for Windows. - .set("TEMP", nonexistent_str); // Mainly for Windows, too. + let _env = set_temp_env_vars(&nonexistent); check_exe_info(); } @@ -400,10 +412,12 @@ fn exe_info_tolerates_broken_tmp() { #[serial] fn exe_info_never_from_local_scope() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); let _env = gix_testtools::Env::new() .set("GIT_CONFIG_NOSYSTEM", "1") .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }); + let maybe_path = super::exe_info(); assert_eq!( maybe_path, None, @@ -418,19 +432,12 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { .expect("script succeeds") .canonicalize() .expect("path is valid and exists"); - let repo_str = repo.to_str().expect("valid Unicode"); + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); - let _env = gix_testtools::Env::new() + let _env = set_temp_env_vars(&repo) .set("GIT_CONFIG_NOSYSTEM", "1") - .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE) - .set("TMPDIR", repo_str) // Mainly for Unix. - .set("TMP", repo_str) // Mainly for Windows. - .set("TEMP", repo_str); // Mainly for Windows, too. - assert_eq!( - std::env::temp_dir(), - repo, - "Possible test bug: Setting up the test may have failed" - ); + .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); + let maybe_path = super::exe_info(); assert_eq!( maybe_path, None, From 5c1b4c0a912847551803c3f29da21778e736b23c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 06:26:01 -0400 Subject: [PATCH 19/39] Adjust some test code for clarity - Make the message in `set_temp_env_vars` more specific, so it's clear what kind of thing appears to have gone wrong. - Replace another occurrence of the `if`-`else` logic for the null device path, that I had missed earlier, with `super::NULL_DEVICE`. --- gix-path/src/env/git/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index e4f92f12ade..c8ce82a5d8d 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -370,7 +370,7 @@ fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { assert_eq!( std::env::temp_dir(), path, - "Possible test bug: Setting up the test may have failed" + "Possible test bug: Temp dir path may not have been customized successfully" ); env @@ -416,7 +416,7 @@ fn exe_info_never_from_local_scope() { let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); let _env = gix_testtools::Env::new() .set("GIT_CONFIG_NOSYSTEM", "1") - .set("GIT_CONFIG_GLOBAL", if cfg!(windows) { "NUL" } else { "/dev/null" }); + .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); let maybe_path = super::exe_info(); assert_eq!( From 56dab133353c5605121bfd9ab362bf05f8848945 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 06:32:59 -0400 Subject: [PATCH 20/39] Maybe slightly decrease risk of test precondition check failure This canonicalizes the existing parent directory prefix of the nonexistent-directory path used in `exe_info_tolerates_broken_tmp`, so it may be less likely to be changed when obtained as the temp dir path via `env::temp_dir()`. That way, the test precondition check in the `set_temp_env_vars` test helper is less likely to fail when setting up the test succeeded. (The other caller already canonicalizes its path.) --- gix-path/src/env/git/tests.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index c8ce82a5d8d..1de7368cb60 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -401,7 +401,11 @@ fn exe_info() { #[serial] fn exe_info_tolerates_broken_tmp() { let empty = gix_testtools::tempfile::tempdir().expect("can create new temporary subdirectory"); - let nonexistent = empty.path().join("nonexistent"); + let nonexistent = empty + .path() + .canonicalize() + .expect("path to the new directory works") + .join("nonexistent"); assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); let _env = set_temp_env_vars(&nonexistent); @@ -431,7 +435,7 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") .expect("script succeeds") .canonicalize() - .expect("path is valid and exists"); + .expect("repo path is valid and exists"); let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); let _env = set_temp_env_vars(&repo) From e60540fe4592b26b9b5858750312b9b5cbe92f6a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 11:32:47 -0400 Subject: [PATCH 21/39] Extract nonexistent directory logic to a test helper struct --- gix-path/src/env/git/tests.rs | 44 +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 1de7368cb60..c682bde6be5 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -355,10 +355,41 @@ mod locations { } } -use std::path::Path; +use std::path::{Path, PathBuf}; +use gix_testtools::tempfile; use serial_test::serial; +/// Wrapper for a path to a location kept from accidentally existing, until drop. +#[derive(Debug)] +struct NonexistentLocation { + _empty: tempfile::TempDir, + nonexistent: PathBuf, +} + +impl NonexistentLocation { + fn new() -> Self { + let empty = tempfile::tempdir().expect("can create new temporary subdirectory"); + + let nonexistent = empty + .path() + .canonicalize() + .expect("path to the new directory works") + .join("nonexistent"); + + assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); + + Self { + _empty: empty, + nonexistent, + } + } + + fn path(&self) -> &Path { + &self.nonexistent + } +} + fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { let path_str = path.to_str().expect("valid Unicode"); @@ -400,15 +431,8 @@ fn exe_info() { #[test] #[serial] fn exe_info_tolerates_broken_tmp() { - let empty = gix_testtools::tempfile::tempdir().expect("can create new temporary subdirectory"); - let nonexistent = empty - .path() - .canonicalize() - .expect("path to the new directory works") - .join("nonexistent"); - assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); - - let _env = set_temp_env_vars(&nonexistent); + let non = NonexistentLocation::new(); + let _env = set_temp_env_vars(non.path()); check_exe_info(); } From c80d562fc0b0dd6298c01e8ba48f6fde211a732c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 11:40:26 -0400 Subject: [PATCH 22/39] Add another broken temp test The new test, `exe_info_same_result_with_broken_temp`, is separate from the `exe_info` and `exe_info_tolerates_broken_tmp` tests, rather than pushing all three of them into a single test, so that more granular failure informaiton is available. This is useful for three reasons: 1. There's a fair bit of subtlety here. 2. As the tests are currently written, even with all identified shortcomings in the code under test fixed, some of the tests would not always pass if the test environment "naturally" has no configuration variables available from either its system or global scope. (On macOS, this would also require that the other "unknown" installation scope have no variables, which is rare.) This is uncommon enough that, at least for now, I think it may be acceptable, given that it affects only the tests. But this condition should be kept easy to diagnose. 3. Due to (2), if our `EXE_INFO` implementation is later modified so it also excludes *global* scope variables (as could be done by setting `GIT_CONFIG_GLOBAL` to the null device path in the environment for the `git config -l ...` subprocess), then some of the tests would be caused no longer to pass in a test environment where the highest scope of any variable is the global scope. This condition is somewhat uncommon but, other than on macOS, not really rare. But it is not at all clear that we really should accept the values of global variables, for `GitInstallation`, since these are not associated with the installation itself. There seem to be three possible reasons to keep doing so: - Maybe some platform or installation method introduces such installation-specific configuration. A possible reason a custom Git might carry installation-level configuration in a file that Git is made to regard to have the global scope is that it would ensure it goes below *other* configuration specific to the installation. (While possible, I am not sure this is a good idea, nor if it's really important to support even if something is doing it.) - Allowing global scope configuration makes it so if other measures taken to exclude local scope configuration somehow fail, it will still be excluded in most cases. - Some users of gitoxide (including applications that use its crates) may be relying on this, such that some global scope configuration may not be used when it should be, if it is changed. Although this is not a reason to avoid changing it, it may be a reason to avoid doing so immediately, along with these other changes. Taken together, I think those arguments for continuing sometimes to treat global configuration as belonging to the installation are fairly weak, though I find the third point temporarily persuasive. So maybe this will eventually change. If so, having the tests give clearer diagnostics for whatever may go wrong in the code under test, and in the tests themselves, seems like it may be a substantial benefit. Like the preexisting broken temp test, the new one does not pass yet, because the code under test has not yet been hardened for that situation. --- gix-path/src/env/git/tests.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index c682bde6be5..2ce4e4d0c24 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -436,6 +436,20 @@ fn exe_info_tolerates_broken_tmp() { check_exe_info(); } +#[test] +#[serial] +fn exe_info_same_result_with_broken_temp() { + let with_unmodified_temp = super::exe_info(); + + let with_nonexistent_temp = { + let non = NonexistentLocation::new(); + let _env = set_temp_env_vars(non.path()); + super::exe_info() + }; + + assert_eq!(with_unmodified_temp, with_nonexistent_temp); +} + #[test] #[serial] fn exe_info_never_from_local_scope() { From 15e7b67ba7447a7a630550d14543d721ee05b696 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 12:23:35 -0400 Subject: [PATCH 23/39] Fix a test name for consistency --- gix-path/src/env/git/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 2ce4e4d0c24..3b11a050ba8 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -430,7 +430,7 @@ fn exe_info() { #[test] #[serial] -fn exe_info_tolerates_broken_tmp() { +fn exe_info_tolerates_broken_temp() { let non = NonexistentLocation::new(); let _env = set_temp_env_vars(non.path()); check_exe_info(); From f35e44c41cc2ae4216c8c76075cdeedd5c4076c7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 12:52:16 -0400 Subject: [PATCH 24/39] Explain why we don't just use `--show-scope` --- gix-path/src/env/git/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 95c5929b0f3..4e7e7809674 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -115,7 +115,11 @@ fn git_cmd(executable: PathBuf) -> Command { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } - // git 2.8.0 and higher support --show-origin. + // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were + // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. + // Low versions of git are still sometimes used, and this is sometimes reasonable because + // downstream distributions often backport security patches without adding most new features. + // So for now, we forgo the convenience of --show-scope for greater backward compatibility. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(env::temp_dir()) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. From 29c6ccabf99c3cd694d5f503fb45fceac986ebb6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 12:58:50 -0400 Subject: [PATCH 25/39] Explain why we don't just use `--system` --- gix-path/src/env/git/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 4e7e7809674..5090f448fd3 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -120,6 +120,11 @@ fn git_cmd(executable: PathBuf) -> Command { // Low versions of git are still sometimes used, and this is sometimes reasonable because // downstream distributions often backport security patches without adding most new features. // So for now, we forgo the convenience of --show-scope for greater backward compatibility. + // + // Separately from that, we can't use --system here, because scopes treated higher than the + // system scope are possible. This commonly happens on macOS with Apple Git, where the config + // file under /Library is shown as an "unknown" scope but takes precedence over the system + // scope. Although GIT_CONFIG_NOSYSTEM will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(env::temp_dir()) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. From f70b904bc520b2962dd2a77c035b6d9c47bb1cb8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 14:03:12 -0400 Subject: [PATCH 26/39] fix: Don't require usable temp dir to get installation config When running `git config -l ...` to find the configuration file path associated with the `git` installation itself, the current working directory for the subprocess was set to the current directory prior to #1523, and to `/tmp` or a `/tmp`-like directory since #1523 (which improved performance and security). This builds on #1523, as well as on subsequent changes to run `git` in a way that its behavior depends less on its CWD, by making an even more robust choice of CWD for the subprocess, so that the CWD is less likely to be deeply nested or on network storage; more likely to exist; and, on Unix-like systems, less likely to contain a `.git` entry (though a `git` with security updates should refuse to take any configuration from such a repository unless it is owned by the user). Due to a combination of other measures that harden against malicious or unusual contents (especially setting `GIT_DIR`), the most significant benefit of this change is to fix the problem that a nonexistent temp dir would prevent the command from succeeding. The main way that could happen is if `TMPDIR` on Unix-like systems, or `TMP` or `TEMP` on Windows, is set to an incorrect value. Because these variables are sometimes reasonable to customize for specific purposes, it is plausible for them to be set to incorrect values by accident. Except on Windows, this always uses `/` as the CWD for the subprocess. On Windows, we use the Windows directory (usually `C:\Windows`) rather than the root of the system drive (usually `C:\`), because: - We are currently obtaining this information from environment variables, and it is possible for our own parent process to pass down an overly sanitized environment. Although this can be so sanitized we cannot find the Windows directory, this is less likely to occur than being unable to find the root of the system drive. This due to moderately broad awareness that the `SystemRoot` environment variable (which, somewhat confusingly, holds the path of the Windows directory, not the root of the system drive) should be preserved even when clearing most other variables. Some libraries will even automatically preserve `SystemRoot` when clearing others or restore it. For example: * https://go-review.googlesource.com/c/go/+/174318 As far as I know, such treatment of `SystemDrive` is less common. And also these two considerations, which are minor by comparison: - Under the current behavior of `env::temp_dir()`, which is now a fallback if we cannot determine the Windows directory, we already fall back to the Windows directory evenutally, if temp dir related environment variables are also unset. This is because `env::temp_dir()` usually calls `GetTempDir2` in the Windows API, which implements that fallback behavior (after first trying the user's user profile directory). Avoiding adding yet another place to fall back to that would not otherwise be attempted slightly decreases behavioral complexity, and there is no reason to think a directory like `C:\` would work when a directory like `C:\Windows` doesn't. - The root of the system drive on a Windows system usually permits limited user accounts to create new directories there, so a directory like `C:\` on Windows actually has most of the disadvantages of a location like `/tmp` on a Unix-like system. * https://github.com/git-for-windows/git/security/advisories/GHSA-vw2c-22j4-2fh2 * https://github.com/Byron/gitoxide/security/advisories/GHSA-mgvv-9p9g-3jv4 This is actually a much less significant reason to prefer a directory like `C:\Windows` to a directory like `C:\` than it might seem. After all, if `C:\.git` exists and and `git` uses it when run from `C:\`, then `git` would usually also use it when run from `C:\Windows` (and from numerous other locations)! However, the reason there is still a small reason to prefer a location like `C:\Windows` to a location like `C:\` is that, if a system has a vulnerable `git` but a user or system administrator has sought to work around it by listing `C:\` in `GIT_CEILING_DIRECTORIES`, then that may keep `git` from traversing upward into `C:\`, but it would not keep `C:\` from being used if that is where we already are. An even more significant reason this motivation is a minor one is that the other measures we are taking, including setting `GIT_DIR`, should be sufficient to avoid at least the security dimension of the problem, which arises from actually using the configuration from a repo that is discovered. The reason we do not prefer the user's user profile directory is: - The user profile directory may be more deeply nested. - The user profile directory may sometimes be on slow network storage when the discovered Windows directory is not. - In some situations, the user profile directory does not actually exist, or does not exist yet. - Overly sanitized environments are more likely to lack the `USERPROFILE` vairable than the `SystemRoot` variable. - Users may occasionally choose to have their entire user profile directory be a Git repository. - It's no easier to avoid the problem of using `C:\.git` in a user profile directory than in `C:\Windows`: they're usually both under `C:\`, and are both not the same as `C:\`. (If the user profile directory is a repository, then that will avoid that problem, yet be its own problem, if not for other measures that prevent both.) --- gix-path/src/env/git/mod.rs | 15 ++++++++++++++- gix-path/src/env/git/tests.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 5090f448fd3..7a82691c90d 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -109,12 +109,24 @@ fn exe_info() -> Option { fn git_cmd(executable: PathBuf) -> Command { let mut cmd = Command::new(executable); + #[cfg(windows)] { use std::os::windows::process::CommandExt; const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); + + cmd.current_dir( + env::var_os("SystemRoot") // Most reliable env var for path to Windows directory. + .or_else(|| env::var_os("windir")) // Less reliable, but some callers are unusual. + .map(PathBuf::from) + .filter(|p| p.is_absolute()) + .unwrap_or_else(env::temp_dir), + ); } + #[cfg(not(windows))] + cmd.current_dir("/"); + // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. // Low versions of git are still sometimes used, and this is sometimes reasonable because @@ -125,12 +137,13 @@ fn git_cmd(executable: PathBuf) -> Command { // system scope are possible. This commonly happens on macOS with Apple Git, where the config // file under /Library is shown as an "unknown" scope but takes precedence over the system // scope. Although GIT_CONFIG_NOSYSTEM will suppress this as well, passing --system omits it. + // cmd.args(["config", "-lz", "--show-origin", "--name-only"]) - .current_dir(env::temp_dir()) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. .env("GIT_WORK_TREE", NULL_DEVICE) // Not needed, but clarifies intent. .stdin(Stdio::null()) .stderr(Stdio::null()); + cmd } diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 3b11a050ba8..410f4286e2f 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -407,6 +407,10 @@ fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { env } +fn unset_windows_directory_vars<'a>() -> gix_testtools::Env<'a> { + gix_testtools::Env::new().unset("windir").unset("SystemRoot") +} + fn check_exe_info() { let path = super::exe_info() .map(crate::from_bstring) @@ -436,6 +440,15 @@ fn exe_info_tolerates_broken_temp() { check_exe_info(); } +#[test] +#[serial] +fn exe_info_tolerates_oversanitized_env() { + // This test runs on all systems, but it is a regression test for Windows. + // Also, having both a broken temp dir and an over-sanitized environment is not supported. + let _env = unset_windows_directory_vars(); + check_exe_info(); +} + #[test] #[serial] fn exe_info_same_result_with_broken_temp() { @@ -450,6 +463,20 @@ fn exe_info_same_result_with_broken_temp() { assert_eq!(with_unmodified_temp, with_nonexistent_temp); } +#[test] +#[serial] +fn exe_info_same_result_with_oversanitized_env() { + // See `exe_info_tolerates_oversanitized_env`. + let with_unmodified_env = super::exe_info(); + + let with_oversanitized_env = { + let _env = unset_windows_directory_vars(); + super::exe_info() + }; + + assert_eq!(with_unmodified_env, with_oversanitized_env); +} + #[test] #[serial] fn exe_info_never_from_local_scope() { From 847244728e9464cf2924509bd09b8b698e83fc09 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 27 Aug 2024 22:50:37 -0400 Subject: [PATCH 27/39] Fix unused import on non-Windows systems --- gix-path/src/env/git/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 7a82691c90d..5896a27e0da 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -1,4 +1,3 @@ -use std::env; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; @@ -112,6 +111,7 @@ fn git_cmd(executable: PathBuf) -> Command { #[cfg(windows)] { + use std::env; use std::os::windows::process::CommandExt; const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); From ab0dcc168a8ecd2caedffc63f9e00e468e2628c2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 27 Aug 2024 22:59:56 -0400 Subject: [PATCH 28/39] Refactor for readability; clarify comments --- gix-path/src/env/git/mod.rs | 24 +++++++++++------------- gix-path/src/env/git/tests.rs | 7 +++---- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 5896a27e0da..37c03c00ddc 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -109,23 +109,21 @@ fn exe_info() -> Option { fn git_cmd(executable: PathBuf) -> Command { let mut cmd = Command::new(executable); - #[cfg(windows)] - { - use std::env; + if cfg!(windows) { use std::os::windows::process::CommandExt; const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); - cmd.current_dir( - env::var_os("SystemRoot") // Most reliable env var for path to Windows directory. - .or_else(|| env::var_os("windir")) // Less reliable, but some callers are unusual. - .map(PathBuf::from) - .filter(|p| p.is_absolute()) - .unwrap_or_else(env::temp_dir), - ); + use std::env; + let cwd = env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. + .or_else(|| env::var_os("windir")) // Same. Less reliable, but some callers are unusual. + .map(PathBuf::from) + .filter(|p| p.is_absolute()) + .unwrap_or_else(env::temp_dir); + cmd.current_dir(cwd); + } else { + cmd.current_dir("/"); } - #[cfg(not(windows))] - cmd.current_dir("/"); // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. @@ -140,7 +138,7 @@ fn git_cmd(executable: PathBuf) -> Command { // cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. - .env("GIT_WORK_TREE", NULL_DEVICE) // Not needed, but clarifies intent. + .env("GIT_WORK_TREE", NULL_DEVICE) // Just to avoid confusion when debugging. .stdin(Stdio::null()) .stderr(Stdio::null()); diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 410f4286e2f..0226db48305 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -360,7 +360,7 @@ use std::path::{Path, PathBuf}; use gix_testtools::tempfile; use serial_test::serial; -/// Wrapper for a path to a location kept from accidentally existing, until drop. +/// Wrapper for a valid path to a plausible location, kept from accidentally existing (until drop). #[derive(Debug)] struct NonexistentLocation { _empty: tempfile::TempDir, @@ -443,8 +443,8 @@ fn exe_info_tolerates_broken_temp() { #[test] #[serial] fn exe_info_tolerates_oversanitized_env() { - // This test runs on all systems, but it is a regression test for Windows. - // Also, having both a broken temp dir and an over-sanitized environment is not supported. + // This test runs on all systems, but it is only checking for a Windows regression. Also, on + // Windows, having both a broken temp dir and an over-sanitized environment is not supported. let _env = unset_windows_directory_vars(); check_exe_info(); } @@ -466,7 +466,6 @@ fn exe_info_same_result_with_broken_temp() { #[test] #[serial] fn exe_info_same_result_with_oversanitized_env() { - // See `exe_info_tolerates_oversanitized_env`. let with_unmodified_env = super::exe_info(); let with_oversanitized_env = { From 130511452bc5605d5af48c10c53ba2f6e9ddce3d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 27 Aug 2024 23:39:14 -0400 Subject: [PATCH 29/39] Fix `os::windows` error on non-Windows This was due to a mistake in my refactoring attempt in the previous commit. This refactors somewhat differently to still be readable. --- gix-path/src/env/git/mod.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 37c03c00ddc..52942d2c0b7 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -1,3 +1,4 @@ +use std::env; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; @@ -108,23 +109,21 @@ fn exe_info() -> Option { fn git_cmd(executable: PathBuf) -> Command { let mut cmd = Command::new(executable); - - if cfg!(windows) { + #[cfg(windows)] + { use std::os::windows::process::CommandExt; const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); - - use std::env; - let cwd = env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. + } + let cwd = if cfg!(windows) { + env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. .or_else(|| env::var_os("windir")) // Same. Less reliable, but some callers are unusual. .map(PathBuf::from) .filter(|p| p.is_absolute()) - .unwrap_or_else(env::temp_dir); - cmd.current_dir(cwd); + .unwrap_or_else(env::temp_dir) } else { - cmd.current_dir("/"); - } - + "/".into() + }; // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. // Low versions of git are still sometimes used, and this is sometimes reasonable because @@ -137,6 +136,7 @@ fn git_cmd(executable: PathBuf) -> Command { // scope. Although GIT_CONFIG_NOSYSTEM will suppress this as well, passing --system omits it. // cmd.args(["config", "-lz", "--show-origin", "--name-only"]) + .current_dir(cwd) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. .env("GIT_WORK_TREE", NULL_DEVICE) // Just to avoid confusion when debugging. .stdin(Stdio::null()) From 598c487e25c5463a90130c8e77884f272b3ef1c3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 27 Aug 2024 23:53:55 -0400 Subject: [PATCH 30/39] Small clarity tweaks This rewrites a comment and adjusts spacing. --- gix-path/src/env/git/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 52942d2c0b7..30732fc2770 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -117,7 +117,7 @@ fn git_cmd(executable: PathBuf) -> Command { } let cwd = if cfg!(windows) { env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. - .or_else(|| env::var_os("windir")) // Same. Less reliable, but some callers are unusual. + .or_else(|| env::var_os("windir")) // Same, in case our parent filtered out SystemRoot. .map(PathBuf::from) .filter(|p| p.is_absolute()) .unwrap_or_else(env::temp_dir) @@ -141,7 +141,6 @@ fn git_cmd(executable: PathBuf) -> Command { .env("GIT_WORK_TREE", NULL_DEVICE) // Just to avoid confusion when debugging. .stdin(Stdio::null()) .stderr(Stdio::null()); - cmd } From 7fa5e3530620e2540d615dd3195c645d0c9446e9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 28 Aug 2024 23:33:52 -0400 Subject: [PATCH 31/39] Explain why we run `git` from a different directory + Copyedit other comments for readability. --- gix-path/src/env/git/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 30732fc2770..df08922b6ec 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -115,9 +115,14 @@ fn git_cmd(executable: PathBuf) -> Command { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } + // We will try to run `git` from a location fairly high in the filesystem. This can be faster + // than running it from our CWD, if we are deeply nested or on network storage. We try to pick + // a place that exists, is unlikely to be a repo, and forbids unprivileged users from putting a + // `.git` dir or other entries inside (so not `C:\` on Windows). But we will also be setting + // `GIT_DIR` to a location `git` can't read config from, so this is mostly for performance. let cwd = if cfg!(windows) { env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. - .or_else(|| env::var_os("windir")) // Same, in case our parent filtered out SystemRoot. + .or_else(|| env::var_os("windir")) // Same. In case our parent filtered out SystemRoot. .map(PathBuf::from) .filter(|p| p.is_absolute()) .unwrap_or_else(env::temp_dir) @@ -126,19 +131,18 @@ fn git_cmd(executable: PathBuf) -> Command { }; // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. - // Low versions of git are still sometimes used, and this is sometimes reasonable because + // Low versions of Git are still sometimes used, and this is sometimes reasonable because // downstream distributions often backport security patches without adding most new features. // So for now, we forgo the convenience of --show-scope for greater backward compatibility. // // Separately from that, we can't use --system here, because scopes treated higher than the // system scope are possible. This commonly happens on macOS with Apple Git, where the config - // file under /Library is shown as an "unknown" scope but takes precedence over the system - // scope. Although GIT_CONFIG_NOSYSTEM will suppress this as well, passing --system omits it. - // + // file under `/Library` is shown as an "unknown" scope but takes precedence over the system + // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(cwd) .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. - .env("GIT_WORK_TREE", NULL_DEVICE) // Just to avoid confusion when debugging. + .env("GIT_WORK_TREE", NULL_DEVICE) // This is just to avoid confusion when debugging. .stdin(Stdio::null()) .stderr(Stdio::null()); cmd From b8278138271835ace4e520ccdc1e5c7e4556992a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 28 Aug 2024 23:48:15 -0400 Subject: [PATCH 32/39] Simplify the new comments + Get rid of some nearby comments, moving the essential information into the new ones. --- gix-path/src/env/git/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index df08922b6ec..5d2bb5c98b8 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -116,13 +116,12 @@ fn git_cmd(executable: PathBuf) -> Command { cmd.creation_flags(CREATE_NO_WINDOW); } // We will try to run `git` from a location fairly high in the filesystem. This can be faster - // than running it from our CWD, if we are deeply nested or on network storage. We try to pick - // a place that exists, is unlikely to be a repo, and forbids unprivileged users from putting a - // `.git` dir or other entries inside (so not `C:\` on Windows). But we will also be setting - // `GIT_DIR` to a location `git` can't read config from, so this is mostly for performance. + // than running it from our own CWD, if we are deeply nested or on network storage. let cwd = if cfg!(windows) { - env::var_os("SystemRoot") // Usually `C:\Windows`. Not to be confused with `C:\`. - .or_else(|| env::var_os("windir")) // Same. In case our parent filtered out SystemRoot. + // Use the Windows directory (usually `C:\Windows`) if we can tell what it is. Don't use + // `C:\`, as limited users can put a `.git` dir there (though up-to-date Git won't use it). + env::var_os("SystemRoot") + .or_else(|| env::var_os("windir")) .map(PathBuf::from) .filter(|p| p.is_absolute()) .unwrap_or_else(env::temp_dir) From 8f6d39d2a6988e2e6c69a4f15f5d8aa6c917c6a6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 29 Aug 2024 18:08:46 -0400 Subject: [PATCH 33/39] Unset a couple env vars just in case I think that, when `GIT_DIR` is set to a location `git` cannot read any repository information from, then `GIT_COMMON_DIR` is not used for "common" parts like `config` either. But I haven't found this documented anywhere and I'm not sure it's guaranteed. So this unsets `GIT_COMMON_DIR`, to avoid reading local-scope configuration for any repository. This also unsets `GIT_DISCOVERY_ACROSS_FILESYSTEM`, which shouldn't be necessary to avoid reading local configuration, but I think at least some versions of Git on some systems will traverse upward from the current directory even when `GIT_DIR` is specified (even though they won't consider us to be in a repository found in the traversal). Unsetting it causes the default of false to be used. This still does not clear all or most of the environment, nor does it attempt to unset all variables that affect `git` behavior, because some variables should be honored to change what we regard to be the configuration associated with the installation, e.g., `GIT_CONFIG_SYSTEM`, while others should be honored for debugging related effects. --- gix-path/src/env/git/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 5d2bb5c98b8..dae4af08947 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -140,8 +140,10 @@ fn git_cmd(executable: PathBuf) -> Command { // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(cwd) + .env_remove("GIT_COMMON_DIR") // We are setting `GIT_DIR`. + .env_remove("GIT_DISCOVERY_ACROSS_FILESYSTEM") .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. - .env("GIT_WORK_TREE", NULL_DEVICE) // This is just to avoid confusion when debugging. + .env("GIT_WORK_TREE", NULL_DEVICE) // Avoid confusion when debugging. .stdin(Stdio::null()) .stderr(Stdio::null()); cmd From 4e936bc99c781c9829b6529de32e2bb0bb1debbd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 29 Aug 2024 20:21:23 -0400 Subject: [PATCH 34/39] Fix misstatement of Windows directory rationale + Make clear that `SystemRoot` really is intended to give this, as readers unfamiliar with that variable may erroneously confuse it with `SystemDrive` or otherwise assume it would give a path like `C:\` rather than `C:\Windows`. --- gix-path/src/env/git/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index dae4af08947..82dac126419 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -118,8 +118,8 @@ fn git_cmd(executable: PathBuf) -> Command { // We will try to run `git` from a location fairly high in the filesystem. This can be faster // than running it from our own CWD, if we are deeply nested or on network storage. let cwd = if cfg!(windows) { - // Use the Windows directory (usually `C:\Windows`) if we can tell what it is. Don't use - // `C:\`, as limited users can put a `.git` dir there (though up-to-date Git won't use it). + // Try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, + // except in rare cases where our own parent has not passed down that environment variable. env::var_os("SystemRoot") .or_else(|| env::var_os("windir")) .map(PathBuf::from) From 073e27783c752901211fb18acb6b4a917ad91d0e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 29 Aug 2024 21:51:08 -0400 Subject: [PATCH 35/39] Explore also setting a ceiling directory On Windows, where in theory it could matter if some version of Git performs upward traversal (even while not using configuration, due to `GIT_DIR` being set). The added complexity of doing this may not be justified. --- gix-path/src/env/git/mod.rs | 42 +++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 82dac126419..4cc5e2c73bd 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -115,19 +115,6 @@ fn git_cmd(executable: PathBuf) -> Command { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } - // We will try to run `git` from a location fairly high in the filesystem. This can be faster - // than running it from our own CWD, if we are deeply nested or on network storage. - let cwd = if cfg!(windows) { - // Try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, - // except in rare cases where our own parent has not passed down that environment variable. - env::var_os("SystemRoot") - .or_else(|| env::var_os("windir")) - .map(PathBuf::from) - .filter(|p| p.is_absolute()) - .unwrap_or_else(env::temp_dir) - } else { - "/".into() - }; // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. // Low versions of Git are still sometimes used, and this is sometimes reasonable because @@ -139,13 +126,32 @@ fn git_cmd(executable: PathBuf) -> Command { // file under `/Library` is shown as an "unknown" scope but takes precedence over the system // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) - .current_dir(cwd) - .env_remove("GIT_COMMON_DIR") // We are setting `GIT_DIR`. + .stdin(Stdio::null()) + .stderr(Stdio::null()) + .env_remove("GIT_COMMON_DIR") // Because we are setting `GIT_DIR`. .env_remove("GIT_DISCOVERY_ACROSS_FILESYSTEM") .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. - .env("GIT_WORK_TREE", NULL_DEVICE) // Avoid confusion when debugging. - .stdin(Stdio::null()) - .stderr(Stdio::null()); + .env("GIT_WORK_TREE", NULL_DEVICE); // Just to avoid confusion when debugging. + + // We will run `git` from a location fairly high in the filesystem if we can. This can be + // faster than running it from our own CWD, if we are deeply nested or on network storage. + if cfg!(windows) { + // We try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, + // except in rare cases where our own parent has not passed down that environment variable. + let cwd = env::var_os("SystemRoot") + .or_else(|| env::var_os("windir")) + .map(PathBuf::from) + .filter(|p| p.is_absolute()) + .unwrap_or_else(env::temp_dir); + if let Some(parent) = cwd.parent().filter(|p| p.is_absolute() && *p != cwd) { + cmd.env("GIT_CEILING_DIRECTORIES", parent); + } + cmd.current_dir(cwd); + } else { + // The root should always be available, with nothing above it to worry about traversing to. + cmd.current_dir("/"); + } + cmd } From 2bce0d20c876f076e899e9e0ea03e571487309d9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 29 Aug 2024 21:58:56 -0400 Subject: [PATCH 36/39] Don't set/change ceiling directories The tiny chance of a performance or robustness benefit is not worth the complexity, because: - Git, at least versions I've looked at, will avoid doing the particular traversals that this stands to prevent, just from `GIT_DIR`. - This shouldn't be needed for correctness or security, since the effect of `GIT_DIR` follows from the documentation (as well as being tested on various versions). - This amount of redundancy is hard to justify. Even if `GIT_DIR` failed to have the desired effect, the protection in any recent or otherwise properly patched versions of Git should prevent a malicious repository at a location like `C:\` from affecting the configuration gleaned (or any `git` behavior). - Besides the complexity of the code, there is also the complexity of satisfying oneself that it really is acceptable to *clobber* existing configuration of ceiling directories, in the particular situation we are in. Of course, this could be avoided by prepending it and a `;` (which is the separator on Windows). But that would potentially worsen a situation where, if used, the entries take time for Git to canonicalize due to a slow fileystem. This commit is mostly just a revert of the previous commit, but it does also adjust some comments in light of further insights, to avoid suggesting benefits that are not known to pertain, and to mention the case of a nonexistent current directory. --- gix-path/src/env/git/mod.rs | 42 ++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index 4cc5e2c73bd..f591f546386 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -115,6 +115,19 @@ fn git_cmd(executable: PathBuf) -> Command { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } + // We will try to run `git` from a location fairly high in the filesystem, in the hope that it + // may help if we are deeply nested, on network store, or in a directory that has been deleted. + let cwd = if cfg!(windows) { + // We try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, + // except in rare cases where our own parent has not passed down that environment variable. + env::var_os("SystemRoot") + .or_else(|| env::var_os("windir")) + .map(PathBuf::from) + .filter(|p| p.is_absolute()) + .unwrap_or_else(env::temp_dir) + } else { + "/".into() + }; // Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were // supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0. // Low versions of Git are still sometimes used, and this is sometimes reasonable because @@ -126,32 +139,13 @@ fn git_cmd(executable: PathBuf) -> Command { // file under `/Library` is shown as an "unknown" scope but takes precedence over the system // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) - .stdin(Stdio::null()) - .stderr(Stdio::null()) - .env_remove("GIT_COMMON_DIR") // Because we are setting `GIT_DIR`. + .current_dir(cwd) + .env_remove("GIT_COMMON_DIR") // We are setting `GIT_DIR`. .env_remove("GIT_DISCOVERY_ACROSS_FILESYSTEM") .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. - .env("GIT_WORK_TREE", NULL_DEVICE); // Just to avoid confusion when debugging. - - // We will run `git` from a location fairly high in the filesystem if we can. This can be - // faster than running it from our own CWD, if we are deeply nested or on network storage. - if cfg!(windows) { - // We try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, - // except in rare cases where our own parent has not passed down that environment variable. - let cwd = env::var_os("SystemRoot") - .or_else(|| env::var_os("windir")) - .map(PathBuf::from) - .filter(|p| p.is_absolute()) - .unwrap_or_else(env::temp_dir); - if let Some(parent) = cwd.parent().filter(|p| p.is_absolute() && *p != cwd) { - cmd.env("GIT_CEILING_DIRECTORIES", parent); - } - cmd.current_dir(cwd); - } else { - // The root should always be available, with nothing above it to worry about traversing to. - cmd.current_dir("/"); - } - + .env("GIT_WORK_TREE", NULL_DEVICE) // Avoid confusion when debugging. + .stdin(Stdio::null()) + .stderr(Stdio::null()); cmd } From 6160a8308038cee1e604b6fb86c9bfb779b3b835 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 29 Aug 2024 23:26:00 -0400 Subject: [PATCH 37/39] Test no local scope with empty system config This is in addition to testing it with suppressed system config, which was already being done. The `exe_info_never_from_local_scope*` tests are now back to the approach they originally used, setting `GIT_CONFIG_SYSTEM` rather than `GIT_CONFIG_NOSYSTEM`, and new tests (or, rather, the previous ones, renamed) set `GIT_CONFIG_NOSYSTEM`. The reason setting `GIT_CONFIG_SYSTEM` to paths like `/dev/null` had been replaced by setting `GIT_CONFIG_NOSYSTEM` to `1` was to make the tests work on macOS, where it is harder for an end-to-end test to allow a system-scope configuration to be used but arrange for it to be empty. This is harder on macOS because of the "unknown" scope of configuration variables from a file belonging to the installation, often in `/Library`, which is higher than the system scope and can be suppressed via `GIT_CONFIG_NOSYSTEM`, but which is not suppressed by setting `GIT_CONFIG_SYSTEM` to an empty file. The tests, brought back here, that set `GIT_CONFIG_SYSTEM` while leaving `GIT_CONFIG_NOSYSTEM` unset, are thus skipped on macOS. In part to allow the important functionality in play to be *mostly* tested even on macOS, and in part because it is an important case unto itself, this also preserves (renamed) tests that set `GIT_CONFIG_NOSYSTEM`. The reason to restore testing of the `GIT_CONFIG_SYSTEM` approach is threefold: - It is not obvious by reading the tests themselves that setting `GIT_CONFIG_NOSYSTEM` covers cases where system scope config is allowed but happens to provide no configuration variables. - Although the implementation does not currently special-case `GIT_CONFIG_NOSYSTEM` as a shortcut to elide the `git config -l` subprocess invocation, this may be done in the future. (I believe the main reason it is not done now is that we currently allow global scope configuration to be treated as belonging to the installation if no higher scope config is available, and it is not obvious that this behavior will be preserved.) - Even if we get configuration we consider to be associated with the `git` installation that does not come from the system scope, suppressing the system scope with `GIT_CONFIG_NOSYSTEM` often causes it not to be used. That environment variable is checked in `gix_config::types::Source::storage_location()`, suppressing both the `GitInstallation` and `System` cases. Therefore, while local scope configuration leaking through would carry some risk even when `GIT_CONFIG_NOSYSTEM` is turned on, the significance of this is unclear, and it is not the main concern. What most needs to be avoided is misattribuitng local scope configuration as being associated with the `git` installation when configuration associated with the `git` installation is actually intended to be used. So there should be test cases that cover that, so it is clear that it is covered, and so it is clear from reading the test suite that these tests really are related to a realistic problem. The assertion messages are edited to distinguish between system scope configuration being suppressed versus being empty (no vars). --- gix-path/src/env/git/tests.rs | 43 +++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 0226db48305..3c50939bf88 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -478,9 +478,27 @@ fn exe_info_same_result_with_oversanitized_env() { #[test] #[serial] +#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. fn exe_info_never_from_local_scope() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE) + .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); + + let maybe_path = super::exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path if the config would be local (empty system config)" + ); +} + +#[test] +#[serial] +fn exe_info_never_from_local_scope_nosystem() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); let _env = gix_testtools::Env::new() .set("GIT_CONFIG_NOSYSTEM", "1") @@ -489,18 +507,39 @@ fn exe_info_never_from_local_scope() { let maybe_path = super::exe_info(); assert_eq!( maybe_path, None, - "Should find no config path if the config would be local" + "Should find no config path if the config would be local (suppressed system config)" ); } #[test] #[serial] +#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. fn exe_info_never_from_local_scope_even_if_temp_is_here() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") .expect("script succeeds") .canonicalize() .expect("repo path is valid and exists"); + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); + let _env = set_temp_env_vars(&repo) + .set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE) + .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); + + let maybe_path = super::exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path if the config would be local even in a `/tmp`-like dir (empty system config)" + ); +} + +#[test] +#[serial] +fn exe_info_never_from_local_scope_even_if_temp_is_here_nosystem() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") + .expect("script succeeds") + .canonicalize() + .expect("repo path is valid and exists"); + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); let _env = set_temp_env_vars(&repo) .set("GIT_CONFIG_NOSYSTEM", "1") @@ -509,7 +548,7 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() { let maybe_path = super::exe_info(); assert_eq!( maybe_path, None, - "Should find no config path if the config would be local even in a `/tmp`-like dir" + "Should find no config path if the config would be local even in a `/tmp`-like dir (suppressed system config)" ); } From 5200184ecbe888d5c18617f7b3b9c4c6fdef2c5f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Aug 2024 04:48:11 -0400 Subject: [PATCH 38/39] Clarify comment about where we run `git` from --- gix-path/src/env/git/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index f591f546386..04cc376f8a9 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -115,8 +115,8 @@ fn git_cmd(executable: PathBuf) -> Command { const CREATE_NO_WINDOW: u32 = 0x08000000; cmd.creation_flags(CREATE_NO_WINDOW); } - // We will try to run `git` from a location fairly high in the filesystem, in the hope that it - // may help if we are deeply nested, on network store, or in a directory that has been deleted. + // We will try to run `git` from a location fairly high in the filesystem, in the hope it may + // be faster if we are deeply nested, on a slow disk, or in a directory that has been deleted. let cwd = if cfg!(windows) { // We try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`, // except in rare cases where our own parent has not passed down that environment variable. From 5ac5f741f8fdae5b7425f89cfc73d7ebecd5b92b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 31 Aug 2024 11:17:05 +0200 Subject: [PATCH 39/39] improve structure of `exe_info` tests --- gix-path/src/env/git/tests.rs | 325 +++++++++++++++++----------------- 1 file changed, 165 insertions(+), 160 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 3c50939bf88..e7cf48310bc 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -1,3 +1,5 @@ +use std::path::Path; + #[cfg(windows)] mod locations { use std::ffi::{OsStr, OsString}; @@ -355,201 +357,204 @@ mod locations { } } -use std::path::{Path, PathBuf}; +mod exe_info { + use std::path::{Path, PathBuf}; -use gix_testtools::tempfile; -use serial_test::serial; + use crate::env::git::{exe_info, NULL_DEVICE}; + use gix_testtools::tempfile; + use serial_test::serial; -/// Wrapper for a valid path to a plausible location, kept from accidentally existing (until drop). -#[derive(Debug)] -struct NonexistentLocation { - _empty: tempfile::TempDir, - nonexistent: PathBuf, -} + /// Wrapper for a valid path to a plausible location, kept from accidentally existing (until drop). + #[derive(Debug)] + struct NonexistentLocation { + _empty: tempfile::TempDir, + nonexistent: PathBuf, + } -impl NonexistentLocation { - fn new() -> Self { - let empty = tempfile::tempdir().expect("can create new temporary subdirectory"); + impl NonexistentLocation { + fn new() -> Self { + let empty = tempfile::tempdir().expect("can create new temporary subdirectory"); - let nonexistent = empty - .path() - .canonicalize() - .expect("path to the new directory works") - .join("nonexistent"); + let nonexistent = empty + .path() + .canonicalize() + .expect("path to the new directory works") + .join("nonexistent"); - assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); + assert!(!nonexistent.exists(), "Test bug: Need nonexistent directory"); - Self { - _empty: empty, - nonexistent, + Self { + _empty: empty, + nonexistent, + } + } + + fn path(&self) -> &Path { + &self.nonexistent } } - fn path(&self) -> &Path { - &self.nonexistent + fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { + let path_str = path.to_str().expect("valid Unicode"); + + let env = gix_testtools::Env::new() + .set("TMPDIR", path_str) // Mainly for Unix. + .set("TMP", path_str) // Mainly for Windows. + .set("TEMP", path_str); // Mainly for Windows, too. + + assert_eq!( + std::env::temp_dir(), + path, + "Possible test bug: Temp dir path may not have been customized successfully" + ); + + env } -} -fn set_temp_env_vars<'a>(path: &Path) -> gix_testtools::Env<'a> { - let path_str = path.to_str().expect("valid Unicode"); + fn unset_windows_directory_vars<'a>() -> gix_testtools::Env<'a> { + gix_testtools::Env::new().unset("windir").unset("SystemRoot") + } - let env = gix_testtools::Env::new() - .set("TMPDIR", path_str) // Mainly for Unix. - .set("TMP", path_str) // Mainly for Windows. - .set("TEMP", path_str); // Mainly for Windows, too. + fn check_exe_info() { + let path = exe_info() + .map(crate::from_bstring) + .expect("It is present in the test environment (nonempty config)"); - assert_eq!( - std::env::temp_dir(), - path, - "Possible test bug: Temp dir path may not have been customized successfully" - ); + assert!( + path.is_absolute(), + "It is absolute (unless overridden such as with GIT_CONFIG_SYSTEM)" + ); + assert!( + path.exists(), + "It should exist on disk, since `git config` just found an entry there" + ); + } - env -} + #[test] + #[serial] + fn with_unmodified_environment() { + check_exe_info(); + } -fn unset_windows_directory_vars<'a>() -> gix_testtools::Env<'a> { - gix_testtools::Env::new().unset("windir").unset("SystemRoot") -} + #[test] + #[serial] + fn tolerates_broken_temp() { + let non = NonexistentLocation::new(); + let _env = set_temp_env_vars(non.path()); + check_exe_info(); + } -fn check_exe_info() { - let path = super::exe_info() - .map(crate::from_bstring) - .expect("It is present in the test environment (nonempty config)"); + #[test] + #[serial] + fn tolerates_oversanitized_env() { + // This test runs on all systems, but it is only checking for a Windows regression. Also, on + // Windows, having both a broken temp dir and an over-sanitized environment is not supported. + let _env = unset_windows_directory_vars(); + check_exe_info(); + } - assert!( - path.is_absolute(), - "It is absolute (unless overridden such as with GIT_CONFIG_SYSTEM)" - ); - assert!( - path.exists(), - "It should exist on disk, since `git config` just found an entry there" - ); -} + #[test] + #[serial] + fn same_result_with_broken_temp() { + let with_unmodified_temp = exe_info(); -#[test] -#[serial] -fn exe_info() { - check_exe_info(); -} + let with_nonexistent_temp = { + let non = NonexistentLocation::new(); + let _env = set_temp_env_vars(non.path()); + exe_info() + }; -#[test] -#[serial] -fn exe_info_tolerates_broken_temp() { - let non = NonexistentLocation::new(); - let _env = set_temp_env_vars(non.path()); - check_exe_info(); -} + assert_eq!(with_unmodified_temp, with_nonexistent_temp); + } -#[test] -#[serial] -fn exe_info_tolerates_oversanitized_env() { - // This test runs on all systems, but it is only checking for a Windows regression. Also, on - // Windows, having both a broken temp dir and an over-sanitized environment is not supported. - let _env = unset_windows_directory_vars(); - check_exe_info(); -} + #[test] + #[serial] + fn same_result_with_oversanitized_env() { + let with_unmodified_env = exe_info(); -#[test] -#[serial] -fn exe_info_same_result_with_broken_temp() { - let with_unmodified_temp = super::exe_info(); + let with_oversanitized_env = { + let _env = unset_windows_directory_vars(); + exe_info() + }; - let with_nonexistent_temp = { - let non = NonexistentLocation::new(); - let _env = set_temp_env_vars(non.path()); - super::exe_info() - }; + assert_eq!(with_unmodified_env, with_oversanitized_env); + } - assert_eq!(with_unmodified_temp, with_nonexistent_temp); -} + #[test] + #[serial] + #[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. + fn never_from_local_scope() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); -#[test] -#[serial] -fn exe_info_same_result_with_oversanitized_env() { - let with_unmodified_env = super::exe_info(); + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_SYSTEM", NULL_DEVICE) + .set("GIT_CONFIG_GLOBAL", NULL_DEVICE); - let with_oversanitized_env = { - let _env = unset_windows_directory_vars(); - super::exe_info() - }; + let maybe_path = exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path if the config would be local (empty system config)" + ); + } - assert_eq!(with_unmodified_env, with_oversanitized_env); -} + #[test] + #[serial] + fn never_from_local_scope_nosystem() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); -#[test] -#[serial] -#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. -fn exe_info_never_from_local_scope() { - let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); - - let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); - let _env = gix_testtools::Env::new() - .set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE) - .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); - - let maybe_path = super::exe_info(); - assert_eq!( - maybe_path, None, - "Should find no config path if the config would be local (empty system config)" - ); -} + let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_NOSYSTEM", "1") + .set("GIT_CONFIG_GLOBAL", NULL_DEVICE); -#[test] -#[serial] -fn exe_info_never_from_local_scope_nosystem() { - let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + let maybe_path = exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path if the config would be local (suppressed system config)" + ); + } - let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir"); - let _env = gix_testtools::Env::new() - .set("GIT_CONFIG_NOSYSTEM", "1") - .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); + #[test] + #[serial] + #[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. + fn never_from_local_scope_even_if_temp_is_here() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") + .expect("script succeeds") + .canonicalize() + .expect("repo path is valid and exists"); - let maybe_path = super::exe_info(); - assert_eq!( - maybe_path, None, - "Should find no config path if the config would be local (suppressed system config)" - ); -} + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); + let _env = set_temp_env_vars(&repo) + .set("GIT_CONFIG_SYSTEM", NULL_DEVICE) + .set("GIT_CONFIG_GLOBAL", NULL_DEVICE); -#[test] -#[serial] -#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. -fn exe_info_never_from_local_scope_even_if_temp_is_here() { - let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") - .expect("script succeeds") - .canonicalize() - .expect("repo path is valid and exists"); - - let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); - let _env = set_temp_env_vars(&repo) - .set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE) - .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); - - let maybe_path = super::exe_info(); - assert_eq!( - maybe_path, None, - "Should find no config path if the config would be local even in a `/tmp`-like dir (empty system config)" - ); -} + let maybe_path = exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path if the config would be local even in a `/tmp`-like dir (empty system config)" + ); + } -#[test] -#[serial] -fn exe_info_never_from_local_scope_even_if_temp_is_here_nosystem() { - let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") - .expect("script succeeds") - .canonicalize() - .expect("repo path is valid and exists"); - - let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); - let _env = set_temp_env_vars(&repo) - .set("GIT_CONFIG_NOSYSTEM", "1") - .set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE); - - let maybe_path = super::exe_info(); - assert_eq!( + #[test] + #[serial] + fn never_from_local_scope_even_if_temp_is_here_nosystem() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh") + .expect("script succeeds") + .canonicalize() + .expect("repo path is valid and exists"); + + let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir"); + let _env = set_temp_env_vars(&repo) + .set("GIT_CONFIG_NOSYSTEM", "1") + .set("GIT_CONFIG_GLOBAL", NULL_DEVICE); + + let maybe_path = exe_info(); + assert_eq!( maybe_path, None, "Should find no config path if the config would be local even in a `/tmp`-like dir (suppressed system config)" ); + } } #[test]