Fix Windows environment variable upcasing bug

[EliahKagan]

Fixes #1646

This fixes the bug I inadvertently introduced in #1636 where all environment variables' names are set upper-case on Windows.

It uses a simple hand-rolled context manager to patch the NoDefaultCurrentDirectoryInExePath variable, instead of unittest.mock.patch.dict. The latter was setting unrelated environment variables to the original (same) values via os.environ, and as a result, their names were all converted to upper-case on Windows. This is the intended behavior of os.environ on Windows, at least in CPython, per python/cpython#73010 and python/cpython#101754, but I had not been thinking of this, and more importantly I was not aware that unittest.mock.patch.dict effectively writes all items to the mapping when it unpatches, including items that had not been listed for patching.

Because only environment variables that are actually set through os.environ have their names upcased, the only variable whose name should be upcased now is NoDefaultCurrentDirectoryInExePath, which should be fine. (It has a single established use/meaning in Windows, where it's treated case-insensitively as environment variables in Windows usually are.)

---

The actual fix here is pretty simple, but robustly testing it is tricky. One pitfall in testing it remains unaddressed at this point: Either the bug doesn't affect Cygwin builds of Python (linking cygwin1.dll), or is_win evaluates to false in the test-cygwin.yml workflows, so the project's current CI cannot verify the fix. Notice, for example, that the first two commits (d88372a and 7296e5c) show as passing all tests, even though they should show as failing on Windows. I am unsure if that should block this PR or not, since it seems like this sort of thing might actually be common.

I could make the test run on more platforms than Windows, but I don't think that would be more robust, because on other systems it would effectively just be testing that the Python implementation of os.environ works as expected on non-Windows systems. Furthermore, the important test logic would differ depending on the value of is_win, so if the Cygwin workflow has is_win as false, then it would only create the false impression that CI was verifying the fix.

It might be possible to improve the situation in the CI workflows themselves. But I think that would take longer than it might be good to wait on fixing this bug, since this bug currently prevents users who rely on case-sensitive environment variables from upgrading far enough to receive fixes for either CVE-2023-40590 (3.1.33) or CVE-2023-41040 (3.1.35, forthcoming).

I have verified the fix locally. That test failure is from before the fix in c7fad20. It is taken from 7296e5c.

Both tests shown there pass starting in the third commit (c7fad20), as expected, and continue to pass in the fourth commit (eebdb25). Due to the new test's use of the new "fixture" script env_case.py, together with how the official installation instructions do not create a true editable install, if python setup.py install was how installation was done, and it was done prior to checking out the fix, then python setup.py install has to be run again for the "fixture" script to get the current code when it runs import git. I considered going back to the previous way that doesn't execute a separate script file, but I think that code may be harder to understand for someone who doesn't already know how the test works.

[irwand]

This works for single thread, but because it temporarily change os.environ, this could cause unexpected race-condition with other threads that are doing something relying on os.environ. This bug will be even harder to detect and catch.

One of the best way to fix this is to re-implement shutil.which() that does not prepend current cwd into the PATH when looking for git, and then just use the git path we found directly. This way we don't cause unexpected side effect by temporarily changing os.environ

[Byron]

Thanks a lot for contributing a fix for a fix for the CVE! It's much appreciated, particularly because this is a strange issue and you even found a way to add a regression test.

Regarding CI, if you have interest in adding a way to improve CI coverage so that this kind of bug would have been caught, I'd definitely appreciate another PR.

PS: I would assign 'legendary' status to your PRs - they make the impression of being a work-log of sorts, maybe one that is part of your workflow or organize work even. Amazing.

[EliahKagan]

@irwand Yes, in #1636 I had not really thought carefully enough about the situations in which a data race on os.environ might occur. Per #1052 (comment), GitPython is not thread-safe, with some concurrent operations such as chdir being unsafe while GitPython code is executing. However, a project that uses GitPython and separately mutates os.environ for a reason unrelated to GitPython should ideally be free of data races, and likely was free of them (unless also doing chdir) prior to #1636.

Although reimplementing shutil.which is one option, I would lean toward a variation of the approach I took in #1636: instead of patching os.environ or otherwise changing environment variables for the current process, use another layer of subprocess indirection (on Windows only), so GitPython creates a subprocess that sets NoDefaultCurrentDirectoryInExePath and then itself creates the git subprocess. I'm not really sure what is better, though.

I think it's worthwhile to merge this PR in order to fix the immediate problem in #1646. But a new issue should probably be opened for the race condition that you have rightly identified. I don't know if you'd prefer to open it or if I should.

[irwand]

I really appreciate the fix and have no objection to merging this in 👍 .. this will allow us to continue upgrading to newer gitpython :-) . The os.environ side effect is there, but not sure practically how bad that would be. Really if the industry already identifies that including cwd when looking for executables is a bad idea, maybe NoDefaultCurrentDirectoryInExePath should just be default. Feel free to open the race condition issue. From my point of view this is good enough and the race condition issue is somewhat low priority. Thank you again!

[Byron]

The new release with various fixes is now available: https://pypi.org/project/GitPython/3.1.35/