Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Fix Windows environment variable upcasing bug#1650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This uses a simple hand-rolled context manager to patch theNoDefaultCurrentDirectoryInExePath variable, instead ofunittest.mock.patch.dict. The latter set unrelated environmentvariables to the original (same) values via os.environ, and as aresult, their names were all converted to upper-case on Windows.Because only environment variables that are actually set throughos.environ have their names upcased, the only variable whose nameshould be upcased now is NoDefaultCurrentDirectoryInExePath, whichshould be fine (it has a single established use/meaning in Windows,where it's treated case-insensitively as environment variables inWindows *usually* are).
irwand commentedSep 7, 2023
This works for single thread, but because it temporarily change One of the best way to fix this is to re-implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@irwand Yes, in#1636 I had not really thought carefully enough about the situations in which a data race on Although reimplementing 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 commentedSep 7, 2023
I really appreciate the fix and have no objection to merging this in 👍 .. this will allow us to continue upgrading to newer gitpython :-) . The |
The new release with various fixes is now available:https://pypi.org/project/GitPython/3.1.35/ |
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.34` -> `==3.1.35` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>gitpython-developers/GitPython (GitPython)</summary>###[`v3.1.35`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.35):- a fix forCVE-2023-41040[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.34...3.1.35)#### What's Changed- Bump actions/checkout from 3 to 4 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1643](https://togithub.com/gitpython-developers/GitPython/pull/1643)- Fix 'Tree' object has no attribute '\_name' when submodule path isnormal path by [@​CosmosAtlas](https://togithub.com/CosmosAtlas)in[https://github.com/gitpython-developers/GitPython/pull/1645](https://togithub.com/gitpython-developers/GitPython/pull/1645)- FixCVE-2023-41040 by[@​facutuesca](https://togithub.com/facutuesca) in[https://github.com/gitpython-developers/GitPython/pull/1644](https://togithub.com/gitpython-developers/GitPython/pull/1644)- Only make config more permissive in tests that need it by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1648](https://togithub.com/gitpython-developers/GitPython/pull/1648)- Added test for PR[#​1645](https://togithub.com/gitpython-developers/GitPython/issues/1645)submodule path by[@​CosmosAtlas](https://togithub.com/CosmosAtlas) in[https://github.com/gitpython-developers/GitPython/pull/1647](https://togithub.com/gitpython-developers/GitPython/pull/1647)- Fix Windows environment variable upcasing bug by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1650](https://togithub.com/gitpython-developers/GitPython/pull/1650)#### New Contributors- [@​CosmosAtlas](https://togithub.com/CosmosAtlas) made theirfirst contribution in[https://github.com/gitpython-developers/GitPython/pull/1645](https://togithub.com/gitpython-developers/GitPython/pull/1645)- [@​facutuesca](https://togithub.com/facutuesca) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1644](https://togithub.com/gitpython-developers/GitPython/pull/1644)**Full Changelog**:gitpython-developers/GitPython@3.1.34...3.1.35</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/allenporter/flux-local).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44My4wIiwidXBkYXRlZEluVmVyIjoiMzYuODMuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
%5d(https%3a%2f%2frenovatebot.com){kind=link}
Bump gitpython from 3.1.32 to 3.1.35Bumps gitpython from 3.1.32 to 3.1.35.Release notesSourced from gitpython's releases.3.1.35 - a fix forCVE-2023-41040What's ChangedBump actions/checkout from 3 to 4 by @dependabot ingitpython-developers/GitPython#1643Fix 'Tree' object has no attribute '_name' when submodule path is normal path by @CosmosAtlas ingitpython-developers/GitPython#1645FixCVE-2023-41040 by @facutuesca ingitpython-developers/GitPython#1644Only make config more permissive in tests that need it by @EliahKagan ingitpython-developers/GitPython#1648Added test for PR #1645 submodule path by @CosmosAtlas ingitpython-developers/GitPython#1647Fix Windows environment variable upcasing bug by @EliahKagan ingitpython-developers/GitPython#1650New Contributors@CosmosAtlas made their first contribution ingitpython-developers/GitPython#1645@facutuesca made their first contribution ingitpython-developers/GitPython#1644Full Changelog: gitpython-developers/GitPython@3.1.34...3.1.353.1.34 - fix resource leakingWhat's Changedutil: close lockfile after opening successfully by @skshetry ingitpython-developers/GitPython#1639New Contributors@skshetry made their first contribution ingitpython-developers/GitPython#1639Full Changelog: gitpython-developers/GitPython@3.1.33...3.1.34v3.1.33 - with security fixWhat's ChangedWIP Quick doc by @LeoDaCoda ingitpython-developers/GitPython#1608Partial clean up wrt mypy and black by @bodograumann ingitpython-developers/GitPython#1617Disable merge_includes in config writers by @bodograumann ingitpython-developers/GitPython#1618feat: full typing for "progress" parameter in Repo class by @madebylydia ingitpython-developers/GitPython#1634FixCVE-2023-40590 by @EliahKagan ingitpython-developers/GitPython#1636#1566 Creating a lock now uses python built-in "open()" method to work arou… by @HageMaster3108 ingitpython-developers/GitPython#1619New Contributors@LeoDaCoda made their first contribution ingitpython-developers/GitPython#1608@bodograumann made their first contribution ingitpython-developers/GitPython#1617@EliahKagan made their first contribution ingitpython-developers/GitPython#1636@HageMaster3108 made their first contribution ingitpython-developers/GitPython#1619Full Changelog: gitpython-developers/GitPython@3.1.32...3.1.33Commitsc8e303f prepare next release09e1b3d Merge pull request #1650 from EliahKagan/envcase8017421 Merge pull request #1647 from CosmosAtlas/masterfafb4f6 updated docs to better describe testing procedure with new repo9da24d4 add test for submodule path not owned by submodule caseeebdb25 Eliminate duplication of git.util.cwd logicc7fad20 Fix Windows env var upcasing regression7296e5c Make test helper script a file, for readabilityd88372a Add test for Windows env var upcasing regression11839ab Merge pull request #1648 from EliahKagan/file-protocolAdditional commits viewable in compare viewDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting@dependabot rebase.Dependabot commands and optionsYou can trigger Dependabot actions by commenting on this PR:@dependabot rebase will rebase this PR@dependabot recreate will recreate this PR, overwriting any edits that have been made to it@dependabot merge will merge this PR after your CI passes on it@dependabot squash and merge will squash and merge this PR after your CI passes on it@dependabot cancel merge will cancel a previously requested merge and block automerging@dependabot reopen will reopen this PR if it is closed@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)You can disable automated security fix PRs for this repo from the Security Alerts page.Reviewed-by: Vladimir Vshivkov
Uh oh!
There was an error while loading.Please reload this page.
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
NoDefaultCurrentDirectoryInExePathvariable, instead ofunittest.mock.patch.dict. The latter was setting unrelated environment variables to the original (same) values viaos.environ, and as a result, their names were all converted to upper-case on Windows. This is the intended behavior ofos.environon Windows, at least in CPython, perpython/cpython#73010 andpython/cpython#101754, but I had not been thinking of this, and more importantly I was not aware thatunittest.mock.patch.dicteffectively 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.environhave their names upcased, the only variable whose name should be upcased now isNoDefaultCurrentDirectoryInExePath, which should be fine. (It has a single established use/meaning in Windows, where it's treated case-insensitively as environment variables in Windowsusually 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), oris_winevaluates to false in thetest-cygwin.ymlworkflows,so the project's current CI cannot verify the fix. Notice, for example, that the first two commits (d88372a and7296e5c) 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.environworks as expected on non-Windows systems. Furthermore, the important test logic would differ depending on the value ofis_win, so if the Cygwin workflow hasis_winas 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 eitherCVE-2023-40590 (3.1.33) orCVE-2023-41040 (3.1.35, forthcoming).
I have verified the fix locally. That test failure is frombefore the fix inc7fad20. It is taken from7296e5c.
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, ifpython setup.py installwas how installation was done, and it was done prior to checking out the fix, thenpython setup.py installhas to be run again for the "fixture" script to get the current code when it runsimport 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.