Skip to content

Commit 5a300e6

Browse files
committed
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 GitoxideLabs#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 GitoxideLabs#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 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 GitoxideLabs#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 GitoxideLabs#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: GitoxideLabs#1523 (comment) 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).
1 parent 1ee98bf commit 5a300e6

File tree

5 files changed

+29
-4
lines changed

5 files changed

+29
-4
lines changed

Cargo.lock

Lines changed: 2 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

gix-path/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@ once_cell = "1.17.1"
2424
home = "0.5.5"
2525

2626
[dev-dependencies]
27-
tempfile = "3.3.0"
27+
gix-testtools = { path = "../tests/tools" }
28+
serial_test = { version = "3.1.0", default-features = false }
2829

2930
[target.'cfg(windows)'.dev-dependencies]
3031
known-folders = "1.1.0"

gix-path/src/env/git/tests.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,7 +357,10 @@ mod locations {
357357

358358
use std::path::Path;
359359

360+
use serial_test::serial;
361+
360362
#[test]
363+
#[serial]
361364
fn exe_info() {
362365
let path = super::exe_info()
363366
.map(crate::from_bstring)
@@ -370,6 +373,19 @@ fn exe_info() {
370373
assert!(path.exists(), "Exists, since `git config` just found an entry there");
371374
}
372375

376+
#[test]
377+
#[serial]
378+
fn exe_info_never_from_local_scope() {
379+
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds");
380+
let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir");
381+
let null_device = if cfg!(windows) { "NUL" } else { "/dev/null" };
382+
let _env = gix_testtools::Env::new()
383+
.set("GIT_CONFIG_SYSTEM", null_device)
384+
.set("GIT_CONFIG_GLOBAL", null_device);
385+
let info = super::exe_info();
386+
assert!(info.is_none());
387+
}
388+
373389
#[test]
374390
fn first_file_from_config_with_origin() {
375391
let macos =
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
#!/usr/bin/env bash
2+
set -eu -o pipefail
3+
4+
git init -q
5+
6+
# Shouldn't be necessary, as a repo starts with some config vars, but this removes any doubt.
7+
git config --local foo.bar baz

gix-path/tests/realpath/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::path::{Path, PathBuf};
22
use std::time::Duration;
33

44
use bstr::ByteVec;
5-
use tempfile::tempdir;
5+
use gix_testtools::tempfile;
66

77
use gix_path::{realpath::Error, realpath_opts};
88

@@ -28,7 +28,7 @@ fn fuzzed_timeout() -> crate::Result {
2828

2929
#[test]
3030
fn assorted() -> crate::Result {
31-
let cwd = tempdir()?;
31+
let cwd = tempfile::tempdir()?;
3232
let cwd = cwd.path();
3333
let symlinks_disabled = 0;
3434

0 commit comments

Comments
 (0)