diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 962791ae7..cc2bee3a6 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -4,7 +4,7 @@ on: [push, pull_request, workflow_dispatch] jobs: build: - runs-on: windows-latest + runs-on: windows-2022 strategy: fail-fast: false env: @@ -61,4 +61,5 @@ jobs: - name: Test with pytest run: | set +x + git config --global --add safe.directory '*' /usr/bin/python -m pytest diff --git a/.github/workflows/empty.yml b/.github/workflows/empty.yml new file mode 100644 index 000000000..838d7cc39 --- /dev/null +++ b/.github/workflows/empty.yml @@ -0,0 +1,10 @@ +name: Empty Workflow +on: + workflow_dispatch: + +jobs: + empty_job: + runs-on: ubuntu-latest + steps: + - name: Echo + run: echo diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 5e79664a8..ec16745b1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -4,11 +4,11 @@ on: [push, pull_request, workflow_dispatch] jobs: lint: - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 - - uses: actions/setup-python@v4 + - uses: MatteoH2O1999/setup-python@v4 with: python-version: "3.x" - uses: pre-commit/action@v3.0.0 diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index a5467ef94..f5d3b20d9 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -11,7 +11,7 @@ permissions: jobs: build: - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 strategy: fail-fast: false matrix: @@ -31,7 +31,7 @@ jobs: submodules: recursive - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v4 + uses: MatteoH2O1999/setup-python@v4 with: python-version: ${{ matrix.python-version }} allow-prereleases: ${{ matrix.experimental }} diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 5a34b8af0..bff915c40 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -9,7 +9,7 @@ repos: flake8-comprehensions==3.14.0, flake8-typing-imports==1.14.0, ] - exclude: ^doc|^git/ext/ + exclude: ^test/fixtures/polyglot$|^doc|^git/ext/ - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 diff --git a/VERSION b/VERSION index 1f1a39706..9defec169 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.1.37 +3.1.37+sp1 diff --git a/git/cmd.py b/git/cmd.py index 9921dd6c9..e9cbb191a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -19,7 +19,6 @@ defenc, force_bytes, safe_decode, - is_posix, is_win, ) from git.exc import CommandError @@ -43,6 +42,7 @@ Iterator, List, Mapping, + Optional, Sequence, TYPE_CHECKING, TextIO, @@ -99,7 +99,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: @@ -207,6 +207,68 @@ def pump_stream( return None +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("_", "-") @@ -225,16 +287,6 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc ## -- End Utilities -- @} -# value of Windows process creation flag taken from MSDN -CREATE_NO_WINDOW = 0x08000000 - -## 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 = ( - CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP if is_win else 0 # type: ignore[attr-defined] -) # mypy error if not windows - - class Git(LazyMixin): """ @@ -963,11 +1015,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 # NOQA # exists, flake8 unknown @UndefinedVariable - maybe_patch_caller_env = contextlib.nullcontext() # end handle stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") @@ -983,21 +1032,18 @@ def execute( istream_ok, ) 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 is not None and shell or self.USE_SHELL, - close_fds=is_posix, # unsupported on windows - 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: @@ -1010,11 +1056,7 @@ def execute( def _kill_process(pid: int) -> None: """Callback method to kill a process.""" - p = Popen( - ["ps", "--ppid", str(pid)], - stdout=PIPE, - creationflags=PROC_CREATIONFLAGS, - ) + p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE) child_pids = [] if p.stdout is not None: for line in p.stdout: diff --git a/git/index/fun.py b/git/index/fun.py index b50f1f465..612f404d1 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -16,7 +16,7 @@ ) import subprocess -from git.cmd import PROC_CREATIONFLAGS, handle_process_output +from git.cmd import handle_process_output, safer_popen from git.compat import ( defenc, force_text, @@ -102,14 +102,14 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: relative_hp = Path(hp).relative_to(index.repo.working_dir).as_posix() cmd = ["bash.exe", relative_hp] - process = subprocess.Popen( + # process = subprocess.Popen( + process = safer_popen( cmd + list(args), env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=index.repo.working_dir, close_fds=is_posix, - creationflags=PROC_CREATIONFLAGS, ) except Exception as ex: raise HookExecutionError(hp, ex) from ex diff --git a/git/util.py b/git/util.py index 48901ba0c..66a1dd058 100644 --- a/git/util.py +++ b/git/util.py @@ -291,6 +291,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() diff --git a/test/fixtures/polyglot b/test/fixtures/polyglot new file mode 100755 index 000000000..f1dd56b26 --- /dev/null +++ b/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') diff --git a/test/lib/helper.py b/test/lib/helper.py index e8464b7d4..8219601df 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -13,6 +13,7 @@ import textwrap import time import unittest +import venv from git.compat import is_win from git.util import rmtree, cwd @@ -38,6 +39,7 @@ "with_rw_repo", "with_rw_and_rw_remote_repo", "TestBase", + "VirtualEnvironment", "TestCase", "SkipTest", "skipIf", @@ -89,12 +91,12 @@ 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.mktemp(prefix=func.__name__) os.mkdir(path) 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", @@ -401,3 +403,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}") diff --git a/test/test_git.py b/test/test_git.py index 481309538..df036acf0 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -4,23 +4,42 @@ # # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ +import contextlib import os import shutil import subprocess import sys -from tempfile import TemporaryDirectory, TemporaryFile +import ddt +from tempfile import TemporaryFile from unittest import mock, skipUnless -from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd +from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo from test.lib import TestBase, fixture_path from test.lib import with_rw_directory from git.util import cwd, finalize_process import os.path as osp +from pathlib import Path from git.compat import is_win +@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 def setUpClass(cls): @@ -76,22 +95,49 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): def test_it_executes_git_to_shell_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 is_win: - # 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(is_win, "The regression only affected Windows, and this test logic is OS-specific.") def test_it_avoids_upcasing_unrelated_environment_variable_names(self): @@ -284,7 +330,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] @@ -300,13 +346,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) diff --git a/test/test_index.py b/test/test_index.py index fba9c78ec..28325e940 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -5,12 +5,17 @@ # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ +import contextlib from io import BytesIO +import logging import os +import re from stat import S_ISLNK, ST_MODE +import subprocess import tempfile from unittest import skipIf import shutil +import ddt from git import ( IndexFile, @@ -25,12 +30,12 @@ ) from git.compat import is_win from git.exc import HookExecutionError, InvalidGitRepositoryError -from git.index.fun import hook_path +from git.index.fun import hook_path, run_commit_hook from git.index.typ import BaseIndexEntry, IndexEntry from git.objects import Blob -from test.lib import TestBase, fixture_path, fixture, with_rw_repo +from test.lib import TestBase, VirtualEnvironment, fixture_path, fixture, with_rw_repo from test.lib import with_rw_directory -from git.util import Actor, rmtree +from git.util import Actor, cwd, rmtree from git.util import HIDE_WINDOWS_KNOWN_ERRORS, hex_to_bin from gitdb.base import IStream @@ -41,9 +46,22 @@ HOOKS_SHEBANG = "#!/usr/bin/env sh\n" +log = logging.getLogger(__name__) + is_win_without_bash = is_win and not shutil.which("bash.exe") +def _get_windows_ansi_encoding(): + """Get the encoding specified by the Windows system-wide ANSI active code page.""" + # locale.getencoding may work but is only in Python 3.11+. Use the registry instead. + import winreg + + hklm_path = R"SYSTEM\CurrentControlSet\Control\Nls\CodePage" + with winreg.OpenKey(winreg.HKEY_LOCAL_MACHINE, hklm_path) as key: + value, _ = winreg.QueryValueEx(key, "ACP") + return f"cp{value}" + + def _make_hook(git_dir, name, content, make_exec=True): """A helper to create a hook""" hp = hook_path(name, git_dir) @@ -57,6 +75,7 @@ def _make_hook(git_dir, name, content, make_exec=True): return hp +@ddt.ddt class TestIndex(TestBase): def __init__(self, *args): super(TestIndex, self).__init__(*args) diff --git a/test/test_installation.py b/test/test_installation.py index 0cb0c71fa..da128af45 100644 --- a/test/test_installation.py +++ b/test/test_installation.py @@ -4,47 +4,34 @@ import ast import os import subprocess -import sys -from git.compat import is_win -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 is_win 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, result.returncode, msg=result.stderr or result.stdout or "Can't install project", ) - result = subprocess.run([self.python, "-c", "import git"], stdout=subprocess.PIPE, cwd=self.sources) + result = subprocess.run([venv.python, "-c", "import git"], stdout=subprocess.PIPE, cwd=venv.sources) self.assertEqual( 0, result.returncode, msg=result.stderr or result.stdout or "Selftest failed", ) 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) @@ -54,3 +41,13 @@ def test_installation(self, rw_dir): msg="Failed to follow the conventions for https://docs.python.org/3/library/sys.html#sys.path", ) self.assertTrue(syspath[1].endswith("gitdb"), msg="Failed to add gitdb to 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