Merge branch 'main' into fix-bash-exe

Author: emanspeaks

.pre-commit-config.yaml

@@ -29,7 +29,7 @@ repos:
   hooks:
   - id: shellcheck
     args: [--color]
-    exclude: ^git/ext/
+    exclude: ^test/fixtures/polyglot$|^git/ext/
 
 - repo: https://github.com/pre-commit/pre-commit-hooks
   rev: v4.4.0

.readthedocs.yaml

@@ -0,0 +1,35 @@
+# Read the Docs configuration file for Sphinx projects
+# See https://docs.readthedocs.io/en/stable/config-file/v2.html for details
+
+# Required
+version: 2
+
+# Set the OS, Python version and other tools you might need
+build:
+  os: ubuntu-22.04
+  tools:
+    python: "3.12"
+    # You can also specify other tool versions:
+    # nodejs: "20"
+    # rust: "1.70"
+    # golang: "1.20"
+
+# Build documentation in the "docs/" directory with Sphinx
+sphinx:
+  configuration: doc/source/conf.py
+  # You can configure Sphinx to use a different builder, for instance use the dirhtml builder for simpler URLs
+  # builder: "dirhtml"
+  # Fail on all warnings to avoid broken references
+  # fail_on_warning: true
+
+# Optionally build your docs in additional formats such as PDF and ePub
+# formats:
+#   - pdf
+#   - epub
+
+# Optional but recommended, declare the Python requirements required
+# to build your documentation
+# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html
+# python:
+#   install:
+#     - requirements: docs/requirements.txt

AUTHORS

@@ -53,6 +53,7 @@ Contributors are:
 -Santos Gallegos <stsewd _at_ proton.me>
 -Wenhan Zhu <wzhu.cosmos _at_ gmail.com>
 -Eliah Kagan <eliah.kagan _at_ gmail.com>
+-Ethan Lin <et.repositories _at_ gmail.com>
 -Randy Eckman <emanspeaks _at_ gmail.com>
 
 Portions derived from other open source works and are clearly marked.

VERSION

@@ -1 +1 @@
-3.1.40
+3.1.41

doc/requirements.txt

@@ -1,3 +1,8 @@
-sphinx==4.3.0
+sphinx == 4.3.2
 sphinx_rtd_theme
+sphinxcontrib-applehelp >= 1.0.2, <= 1.0.4
+sphinxcontrib-devhelp == 1.0.2
+sphinxcontrib-htmlhelp >= 2.0.0, <= 2.0.1
+sphinxcontrib-qthelp == 1.0.3
+sphinxcontrib-serializinghtml == 1.1.5
 sphinx-autodoc-typehints

doc/source/changes.rst

@@ -2,6 +2,18 @@
 Changelog
 =========
 
+3.1.41
+======
+
+This release is relevant for security as it fixes a possible arbitary
+code execution on Windows.
+
+See this PR for details: https://github.com/gitpython-developers/GitPython/pull/1792
+An advisory is available soon at: https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-2mqj-m65w-jghx
+
+See the following for all changes.
+https://github.com/gitpython-developers/GitPython/releases/tag/3.1.41
+
 3.1.40
 ======
 

doc/source/conf.py

@@ -93,7 +93,7 @@
 # Options for HTML output
 # -----------------------
 
-html_theme = "sphinx_rtd_theme"
+# html_theme = "sphinx_rtd_theme"
 html_theme_options = {}
 
 # The name for this set of Sphinx documents.  If None, it defaults to

git/cmd.py

@@ -47,6 +47,7 @@
     Iterator,
     List,
     Mapping,
+    Optional,
     Sequence,
     TYPE_CHECKING,
     TextIO,
@@ -103,7 +104,7 @@ def handle_process_output(
         Callable[[bytes, "Repo", "DiffIndex"], None],
     ],
     stderr_handler: Union[None, Callable[[AnyStr], None], Callable[[List[AnyStr]], None]],
-    finalizer: Union[None, Callable[[Union[subprocess.Popen, "Git.AutoInterrupt"]], None]] = None,
+    finalizer: Union[None, Callable[[Union[Popen, "Git.AutoInterrupt"]], None]] = None,
     decode_streams: bool = True,
     kill_after_timeout: Union[None, float] = None,
 ) -> None:
@@ -208,6 +209,68 @@ def pump_stream(
         finalizer(process)
 
 
+def _safer_popen_windows(
+    command: Union[str, Sequence[Any]],
+    *,
+    shell: bool = False,
+    env: Optional[Mapping[str, str]] = None,
+    **kwargs: Any,
+) -> Popen:
+    """Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
+
+    This avoids an untrusted search path condition where a file like ``git.exe`` in a
+    malicious repository would be run when GitPython operates on the repository. The
+    process using GitPython may have an untrusted repository's working tree as its
+    current working directory. Some operations may temporarily change to that directory
+    before running a subprocess. In addition, while by default GitPython does not run
+    external commands with a shell, it can be made to do so, in which case the CWD of
+    the subprocess, which GitPython usually sets to a repository working tree, can
+    itself be searched automatically by the shell. This wrapper covers all those cases.
+
+    :note: This currently works by setting the ``NoDefaultCurrentDirectoryInExePath``
+        environment variable during subprocess creation. It also takes care of passing
+        Windows-specific process creation flags, but that is unrelated to path search.
+
+    :note: The current implementation contains a race condition on :attr:`os.environ`.
+        GitPython isn't thread-safe, but a program using it on one thread should ideally
+        be able to mutate :attr:`os.environ` on another, without unpredictable results.
+        See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
+    """
+    # CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
+    # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
+    # https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
+    creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
+
+    # When using a shell, the shell is the direct subprocess, so the variable must be
+    # set in its environment, to affect its search behavior. (The "1" can be any value.)
+    if shell:
+        safer_env = {} if env is None else dict(env)
+        safer_env["NoDefaultCurrentDirectoryInExePath"] = "1"
+    else:
+        safer_env = env
+
+    # When not using a shell, the current process does the search in a CreateProcessW
+    # API call, so the variable must be set in our environment. With a shell, this is
+    # unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
+    # patched. If not, in the rare case the ComSpec environment variable is unset, the
+    # shell is searched for unsafely. Setting NoDefaultCurrentDirectoryInExePath in all
+    # cases, as here, is simpler and protects against that. (The "1" can be any value.)
+    with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
+        return Popen(
+            command,
+            shell=shell,
+            env=safer_env,
+            creationflags=creationflags,
+            **kwargs,
+        )
+
+
+if os.name == "nt":
+    safer_popen = _safer_popen_windows
+else:
+    safer_popen = Popen
+
+
 def dashify(string: str) -> str:
     return string.replace("_", "-")
 
@@ -226,14 +289,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
 ## -- End Utilities -- @}
 
 
-if os.name == "nt":
-    # CREATE_NEW_PROCESS_GROUP is needed to allow killing it afterwards. See:
-    # https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
-    PROC_CREATIONFLAGS = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP
-else:
-    PROC_CREATIONFLAGS = 0
-
-
 class Git(LazyMixin):
     """The Git class manages communication with the Git binary.
 
@@ -1160,11 +1215,8 @@ def execute(
                     redacted_command,
                     '"kill_after_timeout" feature is not supported on Windows.',
                 )
-            # Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value.
-            maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1")
         else:
             cmd_not_found_exception = FileNotFoundError
-            maybe_patch_caller_env = contextlib.nullcontext()
         # END handle
 
         stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
@@ -1179,20 +1231,18 @@ def execute(
             universal_newlines,
         )
         try:
-            with maybe_patch_caller_env:
-                proc = Popen(
-                    command,
-                    env=env,
-                    cwd=cwd,
-                    bufsize=-1,
-                    stdin=(istream or DEVNULL),
-                    stderr=PIPE,
-                    stdout=stdout_sink,
-                    shell=shell,
-                    universal_newlines=universal_newlines,
-                    creationflags=PROC_CREATIONFLAGS,
-                    **subprocess_kwargs,
-                )
+            proc = safer_popen(
+                command,
+                env=env,
+                cwd=cwd,
+                bufsize=-1,
+                stdin=(istream or DEVNULL),
+                stderr=PIPE,
+                stdout=stdout_sink,
+                shell=shell,
+                universal_newlines=universal_newlines,
+                **subprocess_kwargs,
+            )
         except cmd_not_found_exception as err:
             raise GitCommandNotFound(redacted_command, err) from err
         else:

git/index/fun.py

@@ -18,7 +18,7 @@
 )
 import subprocess
 
-from git.cmd import PROC_CREATIONFLAGS, handle_process_output, Git
+from git.cmd import handle_process_output, Git, safer_popen
 from git.compat import defenc, force_bytes, force_text, safe_decode
 from git.exc import HookExecutionError, UnmergedEntriesError
 from git.objects.fun import (
@@ -98,13 +98,12 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
             relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix()
             cmd = [Git.GIT_PYTHON_BASH_EXECUTABLE, relative_hp]
 
-        process = subprocess.Popen(
+        process = safer_popen(
             cmd + list(args),
             env=env,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             cwd=index.repo.working_dir,
-            creationflags=PROC_CREATIONFLAGS,
         )
     except Exception as ex:
         raise HookExecutionError(hp, ex) from ex

git/objects/tree.py

@@ -53,54 +53,6 @@
 __all__ = ("TreeModifier", "Tree")
 
 
-def git_cmp(t1: TreeCacheTup, t2: TreeCacheTup) -> int:
-    a, b = t1[2], t2[2]
-    # assert isinstance(a, str) and isinstance(b, str)
-    len_a, len_b = len(a), len(b)
-    min_len = min(len_a, len_b)
-    min_cmp = cmp(a[:min_len], b[:min_len])
-
-    if min_cmp:
-        return min_cmp
-
-    return len_a - len_b
-
-
-def merge_sort(a: List[TreeCacheTup], cmp: Callable[[TreeCacheTup, TreeCacheTup], int]) -> None:
-    if len(a) < 2:
-        return
-
-    mid = len(a) // 2
-    lefthalf = a[:mid]
-    righthalf = a[mid:]
-
-    merge_sort(lefthalf, cmp)
-    merge_sort(righthalf, cmp)
-
-    i = 0
-    j = 0
-    k = 0
-
-    while i < len(lefthalf) and j < len(righthalf):
-        if cmp(lefthalf[i], righthalf[j]) <= 0:
-            a[k] = lefthalf[i]
-            i = i + 1
-        else:
-            a[k] = righthalf[j]
-            j = j + 1
-        k = k + 1
-
-    while i < len(lefthalf):
-        a[k] = lefthalf[i]
-        i = i + 1
-        k = k + 1
-
-    while j < len(righthalf):
-        a[k] = righthalf[j]
-        j = j + 1
-        k = k + 1
-
-
 class TreeModifier:
     """A utility class providing methods to alter the underlying cache in a list-like fashion.
 
@@ -131,7 +83,7 @@ def set_done(self) -> "TreeModifier":
 
         :return self:
         """
-        merge_sort(self._cache, git_cmp)
+        self._cache.sort(key=lambda x: (x[2] + "/") if x[1] == Tree.tree_id << 12 else x[2])
         return self
 
     # } END interface

git/util.py

@@ -327,6 +327,17 @@ def _get_exe_extensions() -> Sequence[str]:
 
 
 def py_where(program: str, path: Optional[PathLike] = None) -> List[str]:
+    """Perform a path search to assist :func:`is_cygwin_git`.
+
+    This is not robust for general use. It is an implementation detail of
+    :func:`is_cygwin_git`. When a search following all shell rules is needed,
+    :func:`shutil.which` can be used instead.
+
+    :note: Neither this function nor :func:`shutil.which` will predict the effect of an
+        executable search on a native Windows system due to a :class:`subprocess.Popen`
+        call without ``shell=True``, because shell and non-shell executable search on
+        Windows differ considerably.
+    """
     # From: http://stackoverflow.com/a/377028/548792
     winprog_exts = _get_exe_extensions()
 

test-requirements.txt

@@ -9,4 +9,3 @@ pytest-cov
 pytest-instafail
 pytest-mock
 pytest-sugar
-sumtypes

test/fixtures/polyglot

@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+# Valid script in both Bash and Python, but with different behavior.
+""":"
+echo 'Ran intended hook.' >output.txt
+exit
+" """
+from pathlib import Path
+Path('payload.txt').write_text('Ran impostor hook!', encoding='utf-8')

test/lib/helper.py

@@ -14,6 +14,7 @@
 import textwrap
 import time
 import unittest
+import venv
 
 import gitdb
 
@@ -36,6 +37,7 @@
     "with_rw_repo",
     "with_rw_and_rw_remote_repo",
     "TestBase",
+    "VirtualEnvironment",
     "TestCase",
     "SkipTest",
     "skipIf",
@@ -88,11 +90,11 @@ def with_rw_directory(func):
     test succeeds, but leave it otherwise to aid additional debugging."""
 
     @wraps(func)
-    def wrapper(self):
+    def wrapper(self, *args, **kwargs):
         path = tempfile.mkdtemp(prefix=func.__name__)
         keep = False
         try:
-            return func(self, path)
+            return func(self, path, *args, **kwargs)
         except Exception:
             log.info(
                 "Test %s.%s failed, output is at %r\n",
@@ -390,3 +392,46 @@ def _make_file(self, rela_path, data, repo=None):
         with open(abs_path, "w") as fp:
             fp.write(data)
         return abs_path
+
+
+class VirtualEnvironment:
+    """A newly created Python virtual environment for use in a test."""
+
+    __slots__ = ("_env_dir",)
+
+    def __init__(self, env_dir, *, with_pip):
+        if os.name == "nt":
+            self._env_dir = osp.realpath(env_dir)
+            venv.create(self.env_dir, symlinks=False, with_pip=with_pip)
+        else:
+            self._env_dir = env_dir
+            venv.create(self.env_dir, symlinks=True, with_pip=with_pip)
+
+    @property
+    def env_dir(self):
+        """The top-level directory of the environment."""
+        return self._env_dir
+
+    @property
+    def python(self):
+        """Path to the Python executable in the environment."""
+        return self._executable("python")
+
+    @property
+    def pip(self):
+        """Path to the pip executable in the environment, or RuntimeError if absent."""
+        return self._executable("pip")
+
+    @property
+    def sources(self):
+        """Path to a src directory in the environment, which may not exist yet."""
+        return os.path.join(self.env_dir, "src")
+
+    def _executable(self, basename):
+        if os.name == "nt":
+            path = osp.join(self.env_dir, "Scripts", basename + ".exe")
+        else:
+            path = osp.join(self.env_dir, "bin", basename)
+        if osp.isfile(path) or osp.islink(path):
+            return path
+        raise RuntimeError(f"no regular file or symlink {path!r}")

test/test_git.py

@@ -3,16 +3,18 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
+import contextlib
 import gc
 import inspect
 import logging
 import os
 import os.path as osp
+from pathlib import Path
 import re
 import shutil
 import subprocess
 import sys
-from tempfile import TemporaryDirectory, TemporaryFile
+from tempfile import TemporaryFile
 from unittest import skipUnless
 
 if sys.version_info >= (3, 8):
@@ -27,6 +29,21 @@
 from test.lib import TestBase, fixture_path, with_rw_directory
 
 
+@contextlib.contextmanager
+def _patch_out_env(name):
+    try:
+        old_value = os.environ[name]
+    except KeyError:
+        old_value = None
+    else:
+        del os.environ[name]
+    try:
+        yield
+    finally:
+        if old_value is not None:
+            os.environ[name] = old_value
+
+
 @ddt.ddt
 class TestGit(TestBase):
     @classmethod
@@ -97,29 +114,28 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
 
     def _do_shell_combo(self, value_in_call, value_from_class):
         with mock.patch.object(Git, "USE_SHELL", value_from_class):
-            # git.cmd gets Popen via a "from" import, so patch it there.
-            with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen:
+            with mock.patch.object(cmd, "safer_popen", wraps=cmd.safer_popen) as mock_safer_popen:
                 # Use a command with no arguments (besides the program name), so it runs
                 # with or without a shell, on all OSes, with the same effect.
                 self.git.execute(["git"], with_exceptions=False, shell=value_in_call)
 
-        return mock_popen
+        return mock_safer_popen
 
     @ddt.idata(_shell_cases)
     def test_it_uses_shell_or_not_as_specified(self, case):
         """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`."""
         value_in_call, value_from_class, expected_popen_arg = case
-        mock_popen = self._do_shell_combo(value_in_call, value_from_class)
-        mock_popen.assert_called_once()
-        self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg)
+        mock_safer_popen = self._do_shell_combo(value_in_call, value_from_class)
+        mock_safer_popen.assert_called_once()
+        self.assertIs(mock_safer_popen.call_args.kwargs["shell"], expected_popen_arg)
 
     @ddt.idata(full_case[:2] for full_case in _shell_cases)
     def test_it_logs_if_it_uses_a_shell(self, case):
         """``shell=`` in the log message agrees with what is passed to `Popen`."""
         value_in_call, value_from_class = case
         with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher:
-            mock_popen = self._do_shell_combo(value_in_call, value_from_class)
-        self._assert_logged_for_popen(log_watcher, "shell", mock_popen.call_args.kwargs["shell"])
+            mock_safer_popen = self._do_shell_combo(value_in_call, value_from_class)
+        self._assert_logged_for_popen(log_watcher, "shell", mock_safer_popen.call_args.kwargs["shell"])
 
     @ddt.data(
         ("None", None),
@@ -134,22 +150,49 @@ def test_it_logs_istream_summary_for_stdin(self, case):
     def test_it_executes_git_and_returns_result(self):
         self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")
 
-    def test_it_executes_git_not_from_cwd(self):
-        with TemporaryDirectory() as tmpdir:
-            if os.name == "nt":
-                # Copy an actual binary executable that is not git.
-                other_exe_path = os.path.join(os.getenv("WINDIR"), "system32", "hostname.exe")
-                impostor_path = os.path.join(tmpdir, "git.exe")
-                shutil.copy(other_exe_path, impostor_path)
-            else:
-                # Create a shell script that doesn't do anything.
-                impostor_path = os.path.join(tmpdir, "git")
-                with open(impostor_path, mode="w", encoding="utf-8") as file:
-                    print("#!/bin/sh", file=file)
-                os.chmod(impostor_path, 0o755)
-
-            with cwd(tmpdir):
-                self.assertRegex(self.git.execute(["git", "version"]), r"^git version\b")
+    @ddt.data(
+        # chdir_to_repo, shell, command, use_shell_impostor
+        (False, False, ["git", "version"], False),
+        (False, True, "git version", False),
+        (False, True, "git version", True),
+        (True, False, ["git", "version"], False),
+        (True, True, "git version", False),
+        (True, True, "git version", True),
+    )
+    @with_rw_directory
+    def test_it_executes_git_not_from_cwd(self, rw_dir, case):
+        chdir_to_repo, shell, command, use_shell_impostor = case
+
+        repo = Repo.init(rw_dir)
+
+        if os.name == "nt":
+            # Copy an actual binary executable that is not git. (On Windows, running
+            # "hostname" only displays the hostname, it never tries to change it.)
+            other_exe_path = Path(os.environ["SystemRoot"], "system32", "hostname.exe")
+            impostor_path = Path(rw_dir, "git.exe")
+            shutil.copy(other_exe_path, impostor_path)
+        else:
+            # Create a shell script that doesn't do anything.
+            impostor_path = Path(rw_dir, "git")
+            impostor_path.write_text("#!/bin/sh\n", encoding="utf-8")
+            os.chmod(impostor_path, 0o755)
+
+        if use_shell_impostor:
+            shell_name = "cmd.exe" if os.name == "nt" else "sh"
+            shutil.copy(impostor_path, Path(rw_dir, shell_name))
+
+        with contextlib.ExitStack() as stack:
+            if chdir_to_repo:
+                stack.enter_context(cwd(rw_dir))
+            if use_shell_impostor:
+                stack.enter_context(_patch_out_env("ComSpec"))
+
+            # Run the command without raising an exception on failure, as the exception
+            # message is currently misleading when the command is a string rather than a
+            # sequence of strings (it really runs "git", but then wrongly reports "g").
+            output = repo.git.execute(command, with_exceptions=False, shell=shell)
+
+        self.assertRegex(output, r"^git version\b")
 
     @skipUnless(
         os.name == "nt",
@@ -345,7 +388,7 @@ def test_environment(self, rw_dir):
                 self.assertIn("FOO", str(err))
 
     def test_handle_process_output(self):
-        from git.cmd import handle_process_output
+        from git.cmd import handle_process_output, safer_popen
 
         line_count = 5002
         count = [None, 0, 0]
@@ -361,13 +404,12 @@ def counter_stderr(line):
             fixture_path("cat_file.py"),
             str(fixture_path("issue-301_stderr")),
         ]
-        proc = subprocess.Popen(
+        proc = safer_popen(
             cmdline,
             stdin=None,
             stdout=subprocess.PIPE,
             stderr=subprocess.PIPE,
             shell=False,
-            creationflags=cmd.PROC_CREATIONFLAGS,
         )
 
         handle_process_output(proc, counter_stdout, counter_stderr, finalize_process)

test/test_index.py

@@ -3,18 +3,21 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
+import contextlib
+from dataclasses import dataclass
 from io import BytesIO
 import logging
 import os
 import os.path as osp
 from pathlib import Path
 import re
+import shutil
 from stat import S_ISLNK, ST_MODE
 import subprocess
 import tempfile
 
+import ddt
 import pytest
-from sumtypes import constructor, sumtype
 
 from git import (
     BlobFilter,
@@ -36,9 +39,16 @@
 from git.index.typ import BaseIndexEntry, IndexEntry
 from git.index.util import TemporaryFileSwap
 from git.objects import Blob
-from git.util import Actor, hex_to_bin, rmtree
+from git.util import Actor, cwd, hex_to_bin, rmtree
 from gitdb.base import IStream
-from test.lib import TestBase, fixture, fixture_path, with_rw_directory, with_rw_repo
+from test.lib import (
+    TestBase,
+    VirtualEnvironment,
+    fixture,
+    fixture_path,
+    with_rw_directory,
+    with_rw_repo,
+)
 
 HOOKS_SHEBANG = "#!/usr/bin/env sh\n"
 
@@ -56,34 +66,48 @@ def _get_windows_ansi_encoding():
     return f"cp{value}"
 
 
-@sumtype
 class WinBashStatus:
-    """Status of bash.exe for native Windows. Affects which commit hook tests can pass.
+    """Namespace of native-Windows bash.exe statuses. Affects what hook tests can pass.
 
     Call check() to check the status. (CheckError and WinError should not typically be
     used to trigger skip or xfail, because they represent unexpected situations.)
     """
 
-    Inapplicable = constructor()
-    """This system is not native Windows: either not Windows at all, or Cygwin."""
+    @dataclass
+    class Inapplicable:
+        """This system is not native Windows: either not Windows at all, or Cygwin."""
+
+    @dataclass
+    class Absent:
+        """No command for bash.exe is found on the system."""
+
+    @dataclass
+    class Native:
+        """Running bash.exe operates outside any WSL distribution (as with Git Bash)."""
+
+    @dataclass
+    class Wsl:
+        """Running bash.exe calls bash in a WSL distribution."""
 
-    Absent = constructor()
-    """No command for bash.exe is found on the system."""
+    @dataclass
+    class WslNoDistro:
+        """Running bash.exe tries to run bash on a WSL distribution, but none exists."""
 
-    Native = constructor()
-    """Running bash.exe operates outside any WSL distribution (as with Git Bash)."""
+        process: "subprocess.CompletedProcess[bytes]"
+        message: str
 
-    Wsl = constructor()
-    """Running bash.exe calls bash in a WSL distribution."""
+    @dataclass
+    class CheckError:
+        """Running bash.exe fails in an unexpected error or gives unexpected output."""
 
-    WslNoDistro = constructor("process", "message")
-    """Running bash.exe tries to run bash on a WSL distribution, but none exists."""
+        process: "subprocess.CompletedProcess[bytes]"
+        message: str
 
-    CheckError = constructor("process", "message")
-    """Running bash.exe fails in an unexpected error or gives unexpected output."""
+    @dataclass
+    class WinError:
+        """bash.exe may exist but can't run. CreateProcessW fails unexpectedly."""
 
-    WinError = constructor("exception")
-    """bash.exe may exist but can't run. CreateProcessW fails unexpectedly."""
+        exception: OSError
 
     @classmethod
     def check(cls):
@@ -172,6 +196,7 @@ def _make_hook(git_dir, name, content, make_exec=True):
     return hp
 
 
+@ddt.ddt
 class TestIndex(TestBase):
     def __init__(self, *args):
         super().__init__(*args)
@@ -1012,6 +1037,47 @@ def test_run_commit_hook(self, rw_repo):
         output = Path(rw_repo.git_dir, "output.txt").read_text(encoding="utf-8")
         self.assertEqual(output, "ran fake hook\n")
 
+    @ddt.data((False,), (True,))
+    @with_rw_directory
+    def test_hook_uses_shell_not_from_cwd(self, rw_dir, case):
+        (chdir_to_repo,) = case
+
+        shell_name = "bash.exe" if os.name == "nt" else "sh"
+        maybe_chdir = cwd(rw_dir) if chdir_to_repo else contextlib.nullcontext()
+        repo = Repo.init(rw_dir)
+
+        # We need an impostor shell that works on Windows and that the test can
+        # distinguish from the real bash.exe. But even if the real bash.exe is absent or
+        # unusable, we should verify the impostor is not run. So the impostor needs a
+        # clear side effect (unlike in TestGit.test_it_executes_git_not_from_cwd). Popen
+        # on Windows uses CreateProcessW, which disregards PATHEXT; the impostor may
+        # need to be a binary executable to ensure the vulnerability is found if
+        # present. No compiler need exist, shipping a binary in the test suite may
+        # target the wrong architecture, and generating one in a bespoke way may trigger
+        # false positive virus scans. So we use a Bash/Python polyglot for the hook and
+        # use the Python interpreter itself as the bash.exe impostor. But an interpreter
+        # from a venv may not run when copied outside of it, and a global interpreter
+        # won't run when copied to a different location if it was installed from the
+        # Microsoft Store. So we make a new venv in rw_dir and use its interpreter.
+        venv = VirtualEnvironment(rw_dir, with_pip=False)
+        shutil.copy(venv.python, Path(rw_dir, shell_name))
+        shutil.copy(fixture_path("polyglot"), hook_path("polyglot", repo.git_dir))
+        payload = Path(rw_dir, "payload.txt")
+
+        if type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.WslNoDistro}:
+            # The real shell can't run, but the impostor should still not be used.
+            with self.assertRaises(HookExecutionError):
+                with maybe_chdir:
+                    run_commit_hook("polyglot", repo.index)
+            self.assertFalse(payload.exists())
+        else:
+            # The real shell should run, and not the impostor.
+            with maybe_chdir:
+                run_commit_hook("polyglot", repo.index)
+            self.assertFalse(payload.exists())
+            output = Path(rw_dir, "output.txt").read_text(encoding="utf-8")
+            self.assertEqual(output, "Ran intended hook.\n")
+
     @pytest.mark.xfail(
         type(_win_bash_status) is WinBashStatus.Absent,
         reason="Can't run a hook on Windows without bash.exe.",

test/test_installation.py

@@ -4,31 +4,19 @@
 import ast
 import os
 import subprocess
-import sys
 
-from test.lib import TestBase
-from test.lib.helper import with_rw_directory
+from test.lib import TestBase, VirtualEnvironment, with_rw_directory
 
 
 class TestInstallation(TestBase):
-    def setUp_venv(self, rw_dir):
-        self.venv = rw_dir
-        subprocess.run([sys.executable, "-m", "venv", self.venv], stdout=subprocess.PIPE)
-        bin_name = "Scripts" if os.name == "nt" else "bin"
-        self.python = os.path.join(self.venv, bin_name, "python")
-        self.pip = os.path.join(self.venv, bin_name, "pip")
-        self.sources = os.path.join(self.venv, "src")
-        self.cwd = os.path.dirname(os.path.dirname(__file__))
-        os.symlink(self.cwd, self.sources, target_is_directory=True)
-
     @with_rw_directory
     def test_installation(self, rw_dir):
-        self.setUp_venv(rw_dir)
+        venv = self._set_up_venv(rw_dir)
 
         result = subprocess.run(
-            [self.pip, "install", "."],
+            [venv.pip, "install", "."],
             stdout=subprocess.PIPE,
-            cwd=self.sources,
+            cwd=venv.sources,
         )
         self.assertEqual(
             0,
@@ -37,9 +25,9 @@ def test_installation(self, rw_dir):
         )
 
         result = subprocess.run(
-            [self.python, "-c", "import git"],
+            [venv.python, "-c", "import git"],
             stdout=subprocess.PIPE,
-            cwd=self.sources,
+            cwd=venv.sources,
         )
         self.assertEqual(
             0,
@@ -48,9 +36,9 @@ def test_installation(self, rw_dir):
         )
 
         result = subprocess.run(
-            [self.python, "-c", "import gitdb; import smmap"],
+            [venv.python, "-c", "import gitdb; import smmap"],
             stdout=subprocess.PIPE,
-            cwd=self.sources,
+            cwd=venv.sources,
         )
         self.assertEqual(
             0,
@@ -62,9 +50,9 @@ def test_installation(self, rw_dir):
         # by inserting its location into PYTHONPATH or otherwise patched into
         # sys.path, make sure it is not wrongly inserted as the *first* entry.
         result = subprocess.run(
-            [self.python, "-c", "import sys; import git; print(sys.path)"],
+            [venv.python, "-c", "import sys; import git; print(sys.path)"],
             stdout=subprocess.PIPE,
-            cwd=self.sources,
+            cwd=venv.sources,
         )
         syspath = result.stdout.decode("utf-8").splitlines()[0]
         syspath = ast.literal_eval(syspath)
@@ -73,3 +61,13 @@ def test_installation(self, rw_dir):
             syspath[0],
             msg="Failed to follow the conventions for https://docs.python.org/3/library/sys.html#sys.path",
         )
+
+    @staticmethod
+    def _set_up_venv(rw_dir):
+        venv = VirtualEnvironment(rw_dir, with_pip=True)
+        os.symlink(
+            os.path.dirname(os.path.dirname(__file__)),
+            venv.sources,
+            target_is_directory=True,
+        )
+        return venv

test/test_tree.py

@@ -8,7 +8,9 @@
 from git.objects import Tree, Blob
 from test.lib import TestBase
 
+import os
 import os.path as osp
+import subprocess
 
 
 class TestTree(TestBase):
@@ -40,6 +42,62 @@ def test_serializable(self):
             testtree._deserialize(stream)
         # END for each item in tree
 
+    def test_tree_modifier_ordering(self):
+        def setup_git_repository_and_get_ordered_files():
+            os.mkdir("tmp")
+            os.chdir("tmp")
+            subprocess.run(["git", "init", "-q"], check=True)
+            os.mkdir("file")
+            for filename in [
+                "bin",
+                "bin.d",
+                "file.to",
+                "file.toml",
+                "file.toml.bin",
+                "file0",
+                "file/a",
+            ]:
+                open(filename, "a").close()
+
+            subprocess.run(["git", "add", "."], check=True)
+            subprocess.run(["git", "commit", "-m", "c1"], check=True)
+            tree_hash = subprocess.check_output(["git", "rev-parse", "HEAD^{tree}"]).decode().strip()
+            cat_file_output = subprocess.check_output(["git", "cat-file", "-p", tree_hash]).decode()
+            return [line.split()[-1] for line in cat_file_output.split("\n") if line]
+
+        hexsha = "6c1faef799095f3990e9970bc2cb10aa0221cf9c"
+        roottree = self.rorepo.tree(hexsha)
+        blob_mode = Tree.blob_id << 12
+        tree_mode = Tree.tree_id << 12
+
+        files_in_desired_order = [
+            (blob_mode, "bin"),
+            (blob_mode, "bin.d"),
+            (blob_mode, "file.to"),
+            (blob_mode, "file.toml"),
+            (blob_mode, "file.toml.bin"),
+            (blob_mode, "file0"),
+            (tree_mode, "file"),
+        ]
+        mod = roottree.cache
+        for file_mode, file_name in files_in_desired_order:
+            mod.add(hexsha, file_mode, file_name)
+        # end for each file
+
+        def file_names_in_order():
+            return [t[1] for t in files_in_desired_order]
+
+        def names_in_mod_cache():
+            a = [t[2] for t in mod._cache]
+            here = file_names_in_order()
+            return [e for e in a if e in here]
+
+        git_file_names_in_order = setup_git_repository_and_get_ordered_files()
+        os.chdir("..")
+
+        mod.set_done()
+        assert names_in_mod_cache() == git_file_names_in_order, "set_done() performs git-sorting"
+
     def test_traverse(self):
         root = self.rorepo.tree("0.1.6")
         num_recursive = 0