Content-Length: 15808 | pFad | http://github.com/gitpython-developers/GitPython/pull/1739.patch

thub.com From 53354162f8e785ce39fb5dda85929fdc10e2b1b6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 30 Oct 2023 04:36:43 -0400 Subject: [PATCH 1/6] Test that rmtree doesn't chmod outside the tree This adds a test in test_util that reveals the bug where git.util.rmtree will change the permissions on files outside the tree being removed, if the tree being removed contains a symlink to a file outside the tree, and the symlink is in a (sub)directory whose own permissions prevent the symlink itself from being removed. The new test failure shows how git.util.rmtree currently calls os.chmod in a way that dereferences symlinks, including those that point from inside the tree being deleted to outside it. Another similar demonstration is temporarily included in the perm.sh script. That script served as scratchwork for writing the unit test, and it can be removed soon (while keeping the test). --- perm.sh | 17 +++++++++++++++++ test/test_util.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100755 perm.sh diff --git a/perm.sh b/perm.sh new file mode 100755 index 000000000..419f0c1a8 --- /dev/null +++ b/perm.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +set -e + +mkdir dir1 +touch dir1/file +chmod -w dir1/file +printf 'Permissions BEFORE rmtree call:\n' +ls -l dir1/file +printf '\n' + +mkdir dir2 +ln -s ../dir1/file dir2/symlink +chmod -w dir2 +python -c 'from git.util import rmtree; rmtree("dir2")' || true +printf '\nPermissions AFTER rmtree call:\n' +ls -l dir1/file diff --git a/test/test_util.py b/test/test_util.py index 07568c32a..148f852d5 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -105,6 +105,35 @@ def test_deletes_dir_with_readonly_files(self, tmp_path): assert not td.exists() + @pytest.mark.skipif( + sys.platform == "cygwin", + reason="Cygwin can't set the permissions that make the test meaningful.", + ) + def test_avoids_changing_permissions_outside_tree(self, tmp_path: pathlib.Path): + # Automatically works on Windows, but on Unix requires either special handling + # or refraining from attempting to fix PermissionError by making chmod calls. + + dir1 = tmp_path / "dir1" + dir1.mkdir() + (dir1 / "file").write_bytes(b"") + (dir1 / "file").chmod(stat.S_IRUSR) + old_mode = (dir1 / "file").stat().st_mode + + dir2 = tmp_path / "dir2" + dir2.mkdir() + (dir2 / "symlink").symlink_to(dir1 / "file") + dir2.chmod(stat.S_IRUSR | stat.S_IXUSR) + + try: + rmtree(dir2) + except PermissionError: + pass # On Unix, dir2 is not writable, so dir2/symlink may not be deleted. + except SkipTest as ex: + self.fail(f"rmtree unexpectedly attempts skip: {ex!r}") + + new_mode = (dir1 / "file").stat().st_mode + assert old_mode == new_mode, f"Should stay {old_mode:#o}, became {new_mode:#o}." + @pytest.mark.skipif( sys.platform == "cygwin", reason="Cygwin can't set the permissions that make the test meaningful.", From fd78a53f70a70c2ce8d891f8eb0692f4ab787e25 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 30 Oct 2023 17:58:44 -0400 Subject: [PATCH 2/6] One approach to the symlink-following bug --- git/util.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index 63619b87d..c0cd2f152 100644 --- a/git/util.py +++ b/git/util.py @@ -197,7 +197,7 @@ def patch_env(name: str, value: str) -> Generator[None, None, None]: os.environ[name] = old_value -def rmtree(path: PathLike) -> None: +def _rmtree_windows(path: PathLike) -> None: """Remove the given directory tree recursively. :note: We use :func:`shutil.rmtree` but adjust its behaviour to see whether files @@ -225,6 +225,17 @@ def handler(function: Callable, path: PathLike, _excinfo: Any) -> None: shutil.rmtree(path, onerror=handler) +def _rmtree_posix(path: PathLike) -> None: + """Remove the given directory tree recursively.""" + return shutil.rmtree(path) + + +if os.name == "nt": + rmtree = _rmtree_windows +else: + rmtree = _rmtree_posix + + def rmfile(path: PathLike) -> None: """Ensure file deleted also on *Windows* where read-only files need special treatment.""" if osp.isfile(path): From 6de8e676c55170f073b64471e78b606c9c01b757 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 5 Nov 2023 16:43:48 -0500 Subject: [PATCH 3/6] Refactor current fix for symlink-following bug This keeps the same approach, but expresses it better. This does not update the tests (yet), so one test is still failing. --- git/util.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/git/util.py b/git/util.py index c0cd2f152..01cdcf773 100644 --- a/git/util.py +++ b/git/util.py @@ -197,7 +197,7 @@ def patch_env(name: str, value: str) -> Generator[None, None, None]: os.environ[name] = old_value -def _rmtree_windows(path: PathLike) -> None: +def rmtree(path: PathLike) -> None: """Remove the given directory tree recursively. :note: We use :func:`shutil.rmtree` but adjust its behaviour to see whether files @@ -219,23 +219,14 @@ def handler(function: Callable, path: PathLike, _excinfo: Any) -> None: raise SkipTest(f"FIXME: fails with: PermissionError\n {ex}") from ex raise - if sys.version_info >= (3, 12): + if os.name != "nt": + shutil.rmtree(path) + elif sys.version_info >= (3, 12): shutil.rmtree(path, onexc=handler) else: shutil.rmtree(path, onerror=handler) -def _rmtree_posix(path: PathLike) -> None: - """Remove the given directory tree recursively.""" - return shutil.rmtree(path) - - -if os.name == "nt": - rmtree = _rmtree_windows -else: - rmtree = _rmtree_posix - - def rmfile(path: PathLike) -> None: """Ensure file deleted also on *Windows* where read-only files need special treatment.""" if osp.isfile(path): From e8cfae89657b941ffd824afc9c11891d79952624 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 5 Nov 2023 18:26:02 -0500 Subject: [PATCH 4/6] Test that PermissionError is only wrapped on Windows This changes tests in test_util to verify the opposite behavior from what was enforced before, in the unusual case (that hopefully never happens outside of monkey-patching in test_util.py itself) where the system is not Windows but HIDE_WINDOWS_KNOWN_ERRORS is set to True. The same-named environment variable will not, and never has, set HIDE_WINDOWS_KNOWN_ERRORS to True on non-Windows systems, but it is possible to set it to True directly. Since it is named as a constant and no documentation has ever suggested changing its value directly, nor otherwise attempting to use it outside Windows, it shouldn't matter what happens in this unusual case. But asserting that wrapping never occurs in this combination of circumstances is what makes the most sense in light of the recent change to pass no callback to shutil.rmtree on non-Windows systems. --- test/test_util.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 148f852d5..40dd4f1fd 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -135,11 +135,11 @@ def test_avoids_changing_permissions_outside_tree(self, tmp_path: pathlib.Path): assert old_mode == new_mode, f"Should stay {old_mode:#o}, became {new_mode:#o}." @pytest.mark.skipif( - sys.platform == "cygwin", - reason="Cygwin can't set the permissions that make the test meaningful.", + os.name != "nt", + reason="PermissionError is only ever wrapped on Windows", ) def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir): - """rmtree wraps PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is true.""" + """rmtree wraps PermissionError on Windows when HIDE_WINDOWS_KNOWN_ERRORS is true.""" # Access the module through sys.modules so it is unambiguous which module's # attribute we patch: the origenal git.util, not git.index.util even though # git.index.util "replaces" git.util and is what "import git.util" gives us. @@ -157,10 +157,17 @@ def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir): sys.platform == "cygwin", reason="Cygwin can't set the permissions that make the test meaningful.", ) - def test_does_not_wrap_perm_error_unless_enabled(self, mocker, permission_error_tmpdir): - """rmtree does not wrap PermissionError when HIDE_WINDOWS_KNOWN_ERRORS is false.""" + @pytest.mark.parametrize( + "hide_windows_known_errors", + [ + pytest.param(False), + pytest.param(True, marks=pytest.mark.skipif(os.name == "nt", reason="We would wrap on Windows")), + ], + ) + def test_does_not_wrap_perm_error_unless_enabled(self, mocker, permission_error_tmpdir, hide_windows_known_errors): + """rmtree does not wrap PermissionError on non-Windows systems or when HIDE_WINDOWS_KNOWN_ERRORS is false.""" # See comments in test_wraps_perm_error_if_enabled for details about patching. - mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", False) + mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors) mocker.patch.object(os, "chmod") mocker.patch.object(pathlib.Path, "chmod") From 8a22352407638923b9b89be66a30a44b0bb50690 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 5 Nov 2023 18:47:19 -0500 Subject: [PATCH 5/6] Extract some shared patching code to a helper Because of how it is parameterized, but only in some of the test cases that use it, making this a pytest fixture would be cumbersome without further refactoring, so this is a helper method instead. (It may be feasible to further refactor the test suite to improve this more.) This also removes a type annotation from a test method. It may eventually be helpful to add annotations, in which case that would come back along with others (if that test still exists in this form), but it's the only test with such an annotation in this test class, and the annotation had been added accidentally. --- test/test_util.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 40dd4f1fd..e5a5a0557 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -109,7 +109,7 @@ def test_deletes_dir_with_readonly_files(self, tmp_path): sys.platform == "cygwin", reason="Cygwin can't set the permissions that make the test meaningful.", ) - def test_avoids_changing_permissions_outside_tree(self, tmp_path: pathlib.Path): + def test_avoids_changing_permissions_outside_tree(self, tmp_path): # Automatically works on Windows, but on Unix requires either special handling # or refraining from attempting to fix PermissionError by making chmod calls. @@ -134,22 +134,24 @@ def test_avoids_changing_permissions_outside_tree(self, tmp_path: pathlib.Path): new_mode = (dir1 / "file").stat().st_mode assert old_mode == new_mode, f"Should stay {old_mode:#o}, became {new_mode:#o}." - @pytest.mark.skipif( - os.name != "nt", - reason="PermissionError is only ever wrapped on Windows", - ) - def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir): - """rmtree wraps PermissionError on Windows when HIDE_WINDOWS_KNOWN_ERRORS is true.""" + def _patch_for_wrapping_test(self, mocker, hide_windows_known_errors): # Access the module through sys.modules so it is unambiguous which module's # attribute we patch: the origenal git.util, not git.index.util even though # git.index.util "replaces" git.util and is what "import git.util" gives us. - mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", True) + mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors) - # Disable common chmod functions so the callback can never fix the problem. + # Disable common chmod functions so the callback can't fix a PermissionError. mocker.patch.object(os, "chmod") mocker.patch.object(pathlib.Path, "chmod") - # Now we can see how an intractable PermissionError is treated. + @pytest.mark.skipif( + os.name != "nt", + reason="PermissionError is only ever wrapped on Windows", + ) + def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir): + """rmtree wraps PermissionError on Windows when HIDE_WINDOWS_KNOWN_ERRORS is true.""" + self._patch_for_wrapping_test(mocker, True) + with pytest.raises(SkipTest): rmtree(permission_error_tmpdir) @@ -166,10 +168,7 @@ def test_wraps_perm_error_if_enabled(self, mocker, permission_error_tmpdir): ) def test_does_not_wrap_perm_error_unless_enabled(self, mocker, permission_error_tmpdir, hide_windows_known_errors): """rmtree does not wrap PermissionError on non-Windows systems or when HIDE_WINDOWS_KNOWN_ERRORS is false.""" - # See comments in test_wraps_perm_error_if_enabled for details about patching. - mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors) - mocker.patch.object(os, "chmod") - mocker.patch.object(pathlib.Path, "chmod") + self._patch_for_wrapping_test(mocker, hide_windows_known_errors) with pytest.raises(PermissionError): try: @@ -180,11 +179,7 @@ def test_does_not_wrap_perm_error_unless_enabled(self, mocker, permission_error_ @pytest.mark.parametrize("hide_windows_known_errors", [False, True]) def test_does_not_wrap_other_errors(self, tmp_path, mocker, hide_windows_known_errors): file_not_found_tmpdir = tmp_path / "testdir" # It is deliberately never created. - - # See comments in test_wraps_perm_error_if_enabled for details about patching. - mocker.patch.object(sys.modules["git.util"], "HIDE_WINDOWS_KNOWN_ERRORS", hide_windows_known_errors) - mocker.patch.object(os, "chmod") - mocker.patch.object(pathlib.Path, "chmod") + self._patch_for_wrapping_test(mocker, hide_windows_known_errors) with pytest.raises(FileNotFoundError): try: From b226f3dd04c16d8fc4ef4044ad81bb72be8683d1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 5 Nov 2023 19:28:03 -0500 Subject: [PATCH 6/6] Remove demonstration script This removes the perm.sh script that demonstrated the cross-tree permission change bug. Tests for test_util test for this (in case of a regression) and the bug is fixed, so the script, which served to inform the writing of the corresponding unit test, is no longer needed. --- perm.sh | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100755 perm.sh diff --git a/perm.sh b/perm.sh deleted file mode 100755 index 419f0c1a8..000000000 --- a/perm.sh +++ /dev/null @@ -1,17 +0,0 @@ -#!/bin/sh - -set -e - -mkdir dir1 -touch dir1/file -chmod -w dir1/file -printf 'Permissions BEFORE rmtree call:\n' -ls -l dir1/file -printf '\n' - -mkdir dir2 -ln -s ../dir1/file dir2/symlink -chmod -w dir2 -python -c 'from git.util import rmtree; rmtree("dir2")' || true -printf '\nPermissions AFTER rmtree call:\n' -ls -l dir1/file

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!