Move safer_popen from git.util to git.cmd

I had originally written it in git.util because it is used from two
separate modules (git.cmd and git.index.fun) and is arguably the
same sort of thing as remove_password_if_present, in that both
relate to running processes (and to security) and both are used
from multiple modules yet are not meant for use outside GitPython.

However, all of this is also true of git.cmd.handle_process_output,
which this really has more in common with: it's a utility related
*only* to the use of subprocesses, while remove_password_if_present
can be used for other sanitization. In addition, it also replaces
git.cmd.PROC_CREATIONFLAGS (also imported in git.index.fun) by
passing that to Popen on Windows (since the situations where a
custom value of creationflags should be used are the same as those
where safer_popen should be called for its primary benefit of
avoiding an untrusted path search when creating the subprocess).

safer_popen and its Windows implementation _safer_popen_windows are
thus moved from git/util.py to git/cmd.py, with related changes,
such as to imports, done everywhere they are needed.

Author: EliahKagan

git/cmd.py

@@ -29,8 +29,8 @@
     cygpath,
     expand_path,
     is_cygwin_git,
+    patch_env,
     remove_password_if_present,
-    safer_popen,
     stream_copy,
 )
 
@@ -46,6 +46,7 @@
     Iterator,
     List,
     Mapping,
+    Optional,
     Sequence,
     TYPE_CHECKING,
     TextIO,
@@ -102,7 +103,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 +208,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("_", "-")
 

git/index/fun.py

@@ -18,15 +18,15 @@
 )
 import subprocess
 
-from git.cmd import handle_process_output
+from git.cmd import handle_process_output, safer_popen
 from git.compat import defenc, force_bytes, force_text, safe_decode
 from git.exc import HookExecutionError, UnmergedEntriesError
 from git.objects.fun import (
     traverse_tree_recursive,
     traverse_trees_recursive,
     tree_to_stream,
 )
-from git.util import IndexFileSHA1Writer, finalize_process, safer_popen
+from git.util import IndexFileSHA1Writer, finalize_process
 from gitdb.base import IStream
 from gitdb.typ import str_tree_type
 

git/util.py

@@ -33,7 +33,6 @@
     IO,
     Iterator,
     List,
-    Mapping,
     Optional,
     Pattern,
     Sequence,
@@ -536,67 +535,6 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]:
     return new_cmdline
 
 
-def _safer_popen_windows(
-    command: Union[str, Sequence[Any]],
-    *,
-    shell: bool = False,
-    env: Optional[Mapping[str, str]] = None,
-    **kwargs: Any,
-) -> subprocess.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 subprocess.Popen(
-            command,
-            shell=shell,
-            env=safer_env,
-            creationflags=creationflags,
-            **kwargs,
-        )
-
-
-if os.name == "nt":
-    safer_popen = _safer_popen_windows
-else:
-    safer_popen = subprocess.Popen
-
 # } END utilities
 
 # { Classes

test/test_git.py

@@ -25,7 +25,7 @@
 import ddt
 
 from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
-from git.util import cwd, finalize_process, safer_popen
+from git.util import cwd, finalize_process
 from test.lib import TestBase, fixture_path, with_rw_directory
 
 
@@ -114,7 +114,6 @@ 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, "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.
@@ -389,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]