Fix Windows environment variable upcasing bug by EliahKagan · Pull Request #1650 · gitpython-developers/GitPython (original) (raw)

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.