Fixups: Code fixups to get all the tests to pass

rickprice · rickprice · commit d1e140f67c23 · 2024-10-28T20:03:46.000-04:00
Fixups: Code fixups after squashing

Fixups: Fix test-requirements
diff --git a/errortext.txt b/errortext.txt
diff --git a/git/cmd.py b/git/cmd.py
@@ -5,6 +5,8 @@
 # the BSD License: http://www.opensource.org/licenses/bsd-license.php
 
 import contextlib
+import re
+
 import io
 import logging
 import os
@@ -130,6 +132,59 @@ def pump_stream(cmdline, name, stream, is_decode, handler):
         return finalizer(process)
 
 
+def _safer_popen_windows(command, shell, env=None, **kwargs):
+    """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):
     return string.replace('_', '-')
 
@@ -150,11 +205,6 @@ def dict_to_slots_and__excluded_are_none(self, d, excluded=()):
 # 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)
-
 
 class Git(LazyMixin):
 
@@ -771,21 +821,18 @@ def execute(self, command,
         log.debug("Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)",
                   command, cwd, universal_newlines, shell, istream_ok)
         try:
-            with patch_caller_env:
-                proc = Popen(
-                    command,
-                    env=env,
-                    cwd=cwd,
-                    bufsize=-1,
-                    stdin=istream,
-                    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,
+                stderr=PIPE,
+                stdout=stdout_sink,
+                shell=shell is not None and shell or self.USE_SHELL,
+                universal_newlines=universal_newlines,
+                **subprocess_kwargs
+            )
         except cmd_not_found_exception as err:
             raise GitCommandNotFound(command, err)
 
diff --git a/git/index/fun.py b/git/index/fun.py
@@ -13,7 +13,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 (
     PY3,
     defenc,
@@ -73,15 +73,12 @@ def run_commit_hook(name, index, *args):
     env["GIT_INDEX_FILE"] = safe_decode(index.path) if PY3 else safe_encode(index.path)
     env["GIT_EDITOR"] = ":"
     try:
-        cmd = subprocess.Popen(
-            [hp] + list(args),
-            env=env,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.PIPE,
-            cwd=index.repo.working_dir,
-            close_fds=is_posix,
-            creationflags=PROC_CREATIONFLAGS,
-        )
+        cmd = safer_popen([hp] + list(args),
+                               env=env,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE,
+                               cwd=index.repo.working_dir,
+                               close_fds=is_posix)
     except Exception as ex:
         raise HookExecutionError(hp, ex)
     else:
diff --git a/git/remote.py b/git/remote.py
@@ -911,7 +911,7 @@ def push(
             be null."""
         kwargs = add_progress(kwargs, self.repo.git, progress)
 
-        refspec = Git._unpack_args(refspec or [])
+        refspec = Git._Git__unpack_args(refspec or [])
         if not allow_unsafe_protocols:
             for ref in refspec:
                 Git.check_unsafe_protocols(ref)
diff --git a/git/test/lib/helper.py b/git/test/lib/helper.py
@@ -16,6 +16,7 @@
 import textwrap
 import time
 import unittest
+import virtualenv
 
 from git.compat import string_types, is_win
 from git.util import rmtree, cwd
@@ -36,7 +37,8 @@
 __all__ = (
     'fixture_path', 'fixture', 'StringProcessAdapter',
     'with_rw_directory', 'with_rw_repo', 'with_rw_and_rw_remote_repo',
-    'TestBase', 'TestCase',
+    'TestBase', 'VirtualEnvironment',
+    'TestCase',
     'SkipTest', 'skipIf',
     'GIT_REPO', 'GIT_DAEMON_PORT'
 )
@@ -83,13 +85,13 @@ 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:
             try:
-                return func(self, path)
+                return func(self, path, *args, **kwargs)
             except Exception:
                 log.info("Test %s.%s failed, output is at %r\n",
                          type(self).__name__, func.__name__, path)
@@ -379,3 +381,49 @@ 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):
+        # On Python2 virtualenv the pip option and symlinks options aren't available
+        if os.name == "nt":
+            self._env_dir = osp.realpath(env_dir)
+            # venv.create(self.env_dir, symlinks=False, with_pip=with_pip)
+            virtualenv.cli_run([self.env_dir])
+        else:
+            self._env_dir = env_dir
+            # venv.create(self.env_dir, symlinks=True, with_pip=with_pip)
+            virtualenv.cli_run([self.env_dir])
+
+    @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("no regular file or symlink " + str(path))
diff --git a/git/test/test_git.py b/git/test/test_git.py
@@ -44,6 +44,11 @@
 except ImportError:
     import mock
 
+try:
+    from pathlib import Path
+except ImportError:
+    from pathlib2 import Path
+
 from git.compat import is_win
 
 @contextlib.contextmanager
@@ -57,6 +62,22 @@ def _chdir(new_dir):
         os.chdir(old_dir)
 
 
+@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
+
+
+
 class TestGit(TestBase):
 
     @classmethod
@@ -321,7 +342,7 @@ def counter_stderr(line):
                                 stdout=subprocess.PIPE,
                                 stderr=subprocess.PIPE,
                                 shell=False,
-                                creationflags=cmd.PROC_CREATIONFLAGS,
+                                # creationflags=cmd.PROC_CREATIONFLAGS,
                                 )
 
         handle_process_output(proc, counter_stdout, counter_stderr, finalize_process)
diff --git a/git/test/test_repo.py b/git/test/test_repo.py
@@ -262,6 +262,9 @@ def test_clone_from_pathlib_withConfig(self, rw_dir):
 
     @with_rw_repo("HEAD")
     def test_clone_unsafe_options(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         tmp_file = tmp_dir / "pwn"
         unsafe_options = [
@@ -321,6 +324,9 @@ def test_clone_unsafe_options(self, rw_repo):
 
     @with_rw_repo("HEAD")
     def test_clone_safe_options(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         options = [
             "--depth=1",
@@ -335,6 +341,9 @@ def test_clone_safe_options(self, rw_repo):
 
     @with_rw_repo("HEAD")
     def test_clone_from_unsafe_options(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         tmp_file = tmp_dir / "pwn"
         unsafe_options = [
@@ -400,6 +409,9 @@ def test_clone_from_unsafe_options(self, rw_repo):
 
     @with_rw_repo("HEAD")
     def test_clone_from_safe_options(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         options = [
             "--depth=1",
@@ -413,6 +425,9 @@ def test_clone_from_safe_options(self, rw_repo):
             assert destination.exists()
 
     def test_clone_from_unsafe_procol(self):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         tmp_file = tmp_dir / "pwn"
         urls = [
@@ -425,6 +440,9 @@ def test_clone_from_unsafe_procol(self):
             assert not tmp_file.exists()
 
     def test_clone_from_unsafe_procol_allowed(self):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         tmp_file = tmp_dir / "pwn"
         urls = [
@@ -1286,6 +1304,9 @@ def test_git_work_tree_env(self, rw_dir):
 
     @with_rw_repo("HEAD")
     def test_clone_command_injection(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         unexpected_file = tmp_dir / "pwn"
         assert not unexpected_file.exists()
@@ -1297,6 +1318,9 @@ def test_clone_command_injection(self, rw_repo):
 
     @with_rw_repo("HEAD")
     def test_clone_from_command_injection(self, rw_repo):
+        if pathlib is None:  # pythons bellow 3.4 don't have pathlib
+            raise SkipTest("pathlib was introduced in 3.4")
+
         tmp_dir = pathlib.Path(tempfile.mkdtemp())
         temp_repo = Repo.init(tmp_dir / "repo")
         unexpected_file = tmp_dir / "pwn"
diff --git a/git/util.py b/git/util.py
@@ -192,6 +192,17 @@ def _get_exe_extensions():
 
 
 def py_where(program, path=None):
+    """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-requirements.txt b/test-requirements.txt
@@ -4,5 +4,9 @@ coverage
 flake8
 nose
 tox
+mock; python_version=='2.7'
+pathlib2; python_version=='2.7'
 backports.tempfile
 six
+virtualenv
+
