GitPython (original) (raw)

That's extremely diligent of you, but even if there would have been a chance for test-failure, it's fine if CI detects it first. It's a tool and ideally, it saves some time as well.

I'm a big fan of discovering test failures through CI only. The issue, here, is that the env_case test is for a regression that only affects native Windows, as elaborated below. So current CI, which uses a Cygwin build of python, would not catch a problem. Otherwise I would not have separately tested manually. However, given that the changes to the env_case test and fixture were just comments, it still probably wasn't necessary for me to test.

#1635 (CVE-2023-40590) and #1646 only affected GitPython on native Windows builds of python.

Although the reported case of #1646 mattered because of how it broke Cygwin make, I believe the python build used must have been native even there, because #1636 only patched os.environ when is_win was true, a behavior #1650 carries forward in its more limited patching. When I wrote the description of #1650, I was not sure about git.compat.is_win on Cygwin python builds. But I have since verified that it is False, which makes sense since it's based on os.name, which is posix rather than nt on Cygwin builds.

When I found that out, I was briefly worried CVE-2023-40590 might not be fully patched. In #1636, I had committed the fix and then the test, and mistakenly assumed--based on my local verification, which was with native Windows, not Cygwin--that CI would have shown a test failure if I had committed and pushed them in the other order. The regression test introduced in #1636 (which is now verifying the modified fix as of #1650) covers both true and false cases of is_win, but it would nonetheless probably not detect the bug with a Cygwin python build even if it were present. This is because, if the current directory were automatically included in a subprocess.Popen path search in Cygwin Python, it would be an inherited behavior from the Windows API and presumably would only be triggered by .exe files. (See python/cpython#91558 (comment).) But we only test with an .exe file when is_win is true.

So if GitPython on Cygwin python builds had suffered from CVE-2023-40590, it still would. Fortunately, I am pretty sure it does not. I have manually tested the subprocess module on a Cygwin build of python, both with and without shell=True (though with shell=True on a Cygwin build, the shell is Cygwin's /bin/sh, so I was pretty sure it wouldn't happen that way). Based on this, the Windows behavior that gave rise to CVE-2023-40590 does not happen in Cygwin python builds. I've also tested this in a MSYS2 build of python--MSYS2 being a major derivative of Cygwin--with the same result.

Note that CVE-2023-40590 did affect MinGW python builds installed by MSYS2 pacman (such as MINGW64 and UCRT64 builds), because unlike MSYS2 builds, those are actually native Windows builds. But because they are native builds, os.name is nt on those, so git.compat.is_win is True, so #1636 and #1650 patch NoDefaultCurrentDirectoryInExePath in as intended.