Find git installation-level configuration more robustly #1567
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
The mismatch may otherwise become cumbersome as more tests are added. + Run `cargo fmt`, reformatting whitespace from recent changes.
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.
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).
Produced on Ubuntu 24.04 LTS, running git 2.43.0-1ubuntu7.1.
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.
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.
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.
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.
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 GitoxideLabs#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 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: - dotnet/docs#41193 - python/cpython#95486 (comment) - python/cpython#95486 (comment) 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.
This is really just to make things less surprising when debugging. This also adds a comment to explain why `GIT_DIR` is being set.
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`).
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).
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.
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.
- 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`.
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.)
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.
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 GitoxideLabs#1523, and to `/tmp` or a `/tmp`-like directory since GitoxideLabs#1523 (which improved performance and security). This builds on GitoxideLabs#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. * GHSA-vw2c-22j4-2fh2 * 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.)
This was due to a mistake in my refactoring attempt in the previous commit. This refactors somewhat differently to still be readable.
This rewrites a comment and adjusts spacing.
+ 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`.
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.
EliahKagan
force-pushed
the
config-origin
branch
from
5af7e9e to
192f916
Compare
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.
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).
EliahKagan
force-pushed
the
config-origin
branch
from
192f916 to
5200184
Compare
Byron
approved these changes
Aug 31, 2024
|
Thanks. Having the new tests together in a nested module makes sense. Should the However, I have noticed that my newly introduced |
|
Thanks for the hints, they are much appreciated and I will be looking forward to a follow-up PR. |
|
I've opened #1568 for this. |
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Sep 1, 2024
It doesn't have to have any configuration variables set in the
command scope, though in practice it always should (with versions
of `git` with which `gix-testtools` is compatible) because we are
setting them with `GIT_CONFIG_{COUNT,KEY,VALUE}`, which, like `-c`,
sets configuration variables in the command scope.
But it must not have any configuration variables set in other
scopes. Of course, actual fixture scripts, when run, most often
create Git repositories, in which case there will in practice
almost always be local scope configuration for the script.
The main point here is to make sure we are omitting variables from
the global and system scopes, as we have already been doing (and
testing for), and also omitting them from the other high scopes,
such as the "unknown" scope associated with an Apple Git
installation that is higher than the system scope and unaffected
by `GIT_CONFIG_SYSTEM` but affected by `GIT_CONFIG_NOSYSTEM`.
Like the tests that preceded it, this test creates a new empty
temporary directory to set as the CWD of the test `git` command
configured by `configure_command`. As such, there should be no
local scope configuration, because this directory should be a
subdirectory of a system `/tmp`-like directory.
A `/tmp`-like directory can technically be a Git repository, and
can even contribute configuration if the repository is owned by the
current user or something is keeping `safe.directory` protections
from excluding it.
When the goal is to *get* configuration from scopes higher than the
local scope, it is worth taking steps to prevent this (which for
`gix-path` is achieved by the combination of GitoxideLabs#1523 and GitoxideLabs#1567).
But in the test suite, temporary directories should preferably be
in locations that are only `git` repositories (owned by the curent
user) in the unusual situation that this is desired, and supporting
tooling such as `git` should be at recent enough versions to really
support the usage that the test suite makes of it.
Furthermore, while it is possible to clear the local scope in this
new test -- such as by using, as the command's CWD, a subdirectory
of a directory that is, in the command's environment, prepended to
`GIT_CEILING_DIRECTORIES` -- this would tend to hide problems in
the actual `gix-testtools` code that this is testing.
If any such measure needs to be taken, it would be better done in
code that uses `configure_command` (or in `configure_command`
itself it it is widely needed and generally acceptable), rather
than in tests of `configure_command`.
For these reasons, the test is, at least for now, deliberately
written in such a way that it will fail if the directory we get
from `tempfile::TempDir::new()` is somehow a Git repository.
This commit consolidates the two preceding test cases into this
one. So now there is a single test that `configure-command` both:
- Excludes global and system scope config, as it already did.
- Also excludes other high-scoped config, which it doesn't yet do.
Thus, this new test is expected to fail on most macOS systems
(where `git` is Apple Git and the installation-associated "unknown"
scope configuration file is nonempty), until that is fixed.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
28 tasks
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 15, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 16, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 17, 2024
This creates `/etc/gitconfig` in the container and populates it with a single dummy configuration variable, so that the `system` scope will exist and tests of "GitInstallation"-related functionality will be able to pass. This is intended to let the following tests pass in the container: - gix-config-tests::config source::git_config_no_system - gix-path env::git::tests::exe_info::tolerates_broken_temp - gix-path env::git::tests::exe_info::tolerates_git_config_env_var - gix-path env::git::tests::exe_info::tolerates_oversanitized_env - gix-path env::git::tests::exe_info::with_unmodified_environment - gix-path::path env::installation_config Those tests have been failing in the container on CI, and in local container environments that are also Debian 12 and created through a similar procedure. But they are not among the tests that fail in a non-container 32-bit Debian 12 system, probably because that test system has user config in the `global` scope. When that is the highest scope, it is currently treated as "GitInstallation", even though it is not truly associated with the Git installation. The change in this commit is a workaround *specific* to one CI job: - In at least the case of `git_config_no_system`, it seems like the test should automatically handle the case of there being no scope detected as "GitInstallation". For the others, it may be reasonable for them to fail in a CI environment that does not support robustly testing the scenarios they pertain to, but there should probably be a way to suppress or weaken them on systems that have no scopes that are intended to be detected as "GitInstallation", while still testing "GitInstallation" functionality as robustly as it can be tested under those conditions. This commit does not fix that. - On macOS with Apple Git, the `unknown` scope higher than `system` is meant to be detected as "GitInstallation" in the usual case that it is nonempty. On other systems, `system` is the highest scope that is expected to exist. On some systems (besides macOS), the `system` scope is often empty, in which case the `global` scope, if nonempty, will be detected as "GitInstallation". But allowing the `global` scope, which in practice is rarely if ever associated with the installation itself (it is "global" to the user, and still separate from the `system` scope in a per-user Git installation), to be detected as a "GitInstallation" has never been guaranteed, and may be changed. Ever since the CVE-2024-45305 fix in GitoxideLabs#1523 and follow-up in GitoxideLabs#1567, `local` and `worktree` scopes are never detected as "GitInstallation", so it looks like there would be no harm, and some benefit, to excluding the `global` scope as well. This commit does not make that change. - If/when the change of excluding `global` from consideration for "GitInstallation" is made, tests that require a variable to exist in an installation-associated scope will start to fail regularly on some developers' machines, similar to how they have been failing in the container. This commit does not address that; it only adds a configuration variable in the `system` scope in the container.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request included a fix for CVE-2024-45405 (RUSTSEC-2024-0371, GHSA-m8rp-vv92-46c7). See 650a1b5 for details of the fix.
#1523 already fixed the vulnerability CVE-2024-45305 (RUSTSEC-2024-0367, GHSA-v26r-4c9c-h3j6). The changes in this PR are not required fix that vulnerability. But this does include further useful hardening related to it. For details on that, see 7280a2d and f70b904.
See also #1583.
The original PR description here is as follows: