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:

This improves the robustness of how gix-path runs git and parses its output to obtain a path to a file to treat as holding installation-level (GitInstallation) configuration.

1523, which had performance as its goal, actually made things significantly more robust. This continues in that direction, and should preserve and extend its benefits in terms of how we run git. This also simplifies and fixes a bug in how we parse its output.

A couple of the tests are marked not to run on macOS, because the "unknown" configuration noted in https://github.com/Byron/gitoxide/pull/1523#issuecomment-2291515070, which actually improves the situation significantly, nonetheless gets in the way of their assertions (it is harder to produce one of the main problems being tested for on macOS).

However, because I think this is specific to Apple Git, I suspect those tests should actually be made to run on macOS, just only on CI.

In addition, because I have only limited and sporadic access to a macOS system other than CI, I have not run the tests in their final form on such a system, though I was able to do so earlier.

EliahKagan commented 1 month ago

Thanks. Having the new tests together in a nested module makes sense.

Should the first_file_from_config_with_origin test also go inside the exe_info module? That function (both before and after this PR) is a helper for EXE_INFO that is separate so that it can easily have a test. This PR breaks up the implementation of EXE_INFO into more such functions, in part to be able to test them.

However, first_file_from_config_with_origin is different in that it is in no sense an end-to-end test: it does not need to modify and restore global state (and thus does not use #[serial]. So maybe that is a reason to keep it out of the exe_info nested module.

I have noticed that my newly introduced NULL_DEVICE static item can, and seems like it should, be a const instead. I can open a small follow-up PR that makes either or both of these changes, if warranted.

Byron commented 1 month ago

Thanks for the hints, they are much appreciated and I will be looking forward to a follow-up PR.

EliahKagan commented 1 month ago

I've opened #1568 for this.

EliahKagan commented 1 month ago

I've edited this PR description to note the vulnerability it fixes as well as its relationship to #1523 and the vulnerabilities it fixes, and to mention #1583. This comment is itself unnecessary and I am actually posting it to investigate a possible regression in GitHub's cross-linking behavior, where edited-in references seem not to induce cross-linking. Accordingly, I'll probably hide this comment shortly. Sorry for any inconvenience!

EliahKagan commented 1 month ago

GitoxideLabs / gitoxide

Find git installation-level configuration more robustly #1567

1523, which had performance as its goal, actually made things significantly more robust. This continues in that direction, and should preserve and extend its benefits in terms of how we run `git`. This also simplifies and fixes a bug in how we parse its output.

1583

GitoxideLabs / gitoxide

Find git installation-level configuration more robustly #1567

1523, which had performance as its goal, actually made things significantly more robust. This continues in that direction, and should preserve and extend its benefits in terms of how we run git. This also simplifies and fixes a bug in how we parse its output.

1583

1523, which had performance as its goal, actually made things significantly more robust. This continues in that direction, and should preserve and extend its benefits in terms of how we run `git`. This also simplifies and fixes a bug in how we parse its output.