Merge pull request #1886 from EliahKagan/deprecation-warnings

Issue and test deprecation warnings

Author: Byron

.github/workflows/pythonpackage.yml

@@ -88,7 +88,7 @@ jobs:
 
     - name: Check types with mypy
       run: |
-        mypy --python-version=${{ matrix.python-version }} -p git
+        mypy --python-version=${{ matrix.python-version }}
       env:
         MYPY_FORCE_COLOR: "1"
         TERM: "xterm-256color"  # For color: https://github.com/python/mypy/issues/13817

README.md

@@ -167,7 +167,7 @@ This includes the linting and autoformatting done by Ruff, as well as some other
 To typecheck, run:
 
 ```sh
-mypy -p git
+mypy
 ```
 
 #### CI (and tox)

git/__init__.py

@@ -88,7 +88,12 @@
 
 __version__ = "git"
 
-from typing import List, Optional, Sequence, TYPE_CHECKING, Tuple, Union
+from typing import Any, List, Optional, Sequence, TYPE_CHECKING, Tuple, Union
+
+if TYPE_CHECKING:
+    from types import ModuleType
+
+import warnings
 
 from gitdb.util import to_hex_sha
 
@@ -144,11 +149,6 @@
         SymbolicReference,
         Tag,
         TagReference,
-        head,  # noqa: F401  # Nonpublic. May disappear! Use git.refs.head.
-        log,  # noqa: F401  # Nonpublic. May disappear! Use git.refs.log.
-        reference,  # noqa: F401  # Nonpublic. May disappear! Use git.refs.reference.
-        symbolic,  # noqa: F401  # Nonpublic. May disappear! Use git.refs.symbolic.
-        tag,  # noqa: F401  # Nonpublic. May disappear! Use git.refs.tag.
     )
     from git.diff import (  # @NoMove
         INDEX,
@@ -169,21 +169,9 @@
         IndexEntry,
         IndexFile,
         StageType,
-        base,  # noqa: F401  # Nonpublic. May disappear! Use git.index.base.
-        fun,  # noqa: F401  # Nonpublic. May disappear! Use git.index.fun.
-        typ,  # noqa: F401  # Nonpublic. May disappear! Use git.index.typ.
-        #
-        # NOTE: The expression `git.util` evaluates to git.index.util, and the import
-        # `from git import util` imports git.index.util, NOT git.util. It may not be
-        # feasible to change this until the next major version, to avoid breaking code
-        # inadvertently relying on it. If git.index.util really is what you want, use or
-        # import from that name, to avoid confusion. To use the "real" git.util module,
-        # write `from git.util import ...`, or access it as `sys.modules["git.util"]`.
-        # (This differs from other historical indirect-submodule imports that are
-        # unambiguously nonpublic and are subject to immediate removal. Here, the public
-        # git.util module, even though different, makes it less discoverable that the
-        # expression `git.util` refers to a non-public attribute of the git module.)
-        util,  # noqa: F401
+        # NOTE: This tells type checkers what util resolves to. We delete it, and it is
+        # really resolved by __getattr__, which warns. See below on what to use instead.
+        util,
     )
     from git.util import (  # @NoMove
         Actor,
@@ -196,7 +184,79 @@
 except GitError as _exc:
     raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc
 
+
+def _warned_import(message: str, fullname: str) -> "ModuleType":
+    import importlib
+
+    warnings.warn(message, DeprecationWarning, stacklevel=3)
+    return importlib.import_module(fullname)
+
+
+def _getattr(name: str) -> Any:
+    # TODO: If __version__ is made dynamic and lazily fetched, put that case right here.
+
+    if name == "util":
+        return _warned_import(
+            "The expression `git.util` and the import `from git import util` actually "
+            "reference git.index.util, and not the git.util module accessed in "
+            '`from git.util import XYZ` or `sys.modules["git.util"]`. This potentially '
+            "confusing behavior is currently preserved for compatibility, but may be "
+            "changed in the future and should not be relied on.",
+            fullname="git.index.util",
+        )
+
+    for names, prefix in (
+        ({"head", "log", "reference", "symbolic", "tag"}, "git.refs"),
+        ({"base", "fun", "typ"}, "git.index"),
+    ):
+        if name not in names:
+            continue
+
+        fullname = f"{prefix}.{name}"
+
+        return _warned_import(
+            f"{__name__}.{name} is a private alias of {fullname} and subject to "
+            f"immediate removal. Use {fullname} instead.",
+            fullname=fullname,
+        )
+
+    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
+
+
+if not TYPE_CHECKING:
+    # NOTE: The expression `git.util` gives git.index.util and `from git import util`
+    # imports git.index.util, NOT git.util. It may not be feasible to change this until
+    # the next major version, to avoid breaking code inadvertently relying on it.
+    #
+    # - If git.index.util *is* what you want, use (or import from) that, to avoid
+    #   confusion.
+    #
+    # - To use the "real" git.util module, write `from git.util import ...`, or if
+    #   necessary access it as `sys.modules["git.util"]`.
+    #
+    # Note also that `import git.util` technically imports the "real" git.util... but
+    # the *expression* `git.util` after doing so is still git.index.util!
+    #
+    # (This situation differs from that of other indirect-submodule imports that are
+    # unambiguously non-public and subject to immediate removal. Here, the public
+    # git.util module, though different, makes less discoverable that the expression
+    # `git.util` refers to a non-public attribute of the git module.)
+    #
+    # This had originally come about by a wildcard import. Now that all intended imports
+    # are explicit, the intuitive but potentially incompatible binding occurs due to the
+    # usual rules for Python submodule bindings. So for now we replace that binding with
+    # git.index.util, delete that, and let __getattr__ handle it and issue a warning.
+    #
+    # For the same runtime behavior, it would be enough to forgo importing util, and
+    # delete util as created naturally; __getattr__ would behave the same. But type
+    # checkers would not know what util refers to when accessed as an attribute of git.
+    del util
+
+    # This is "hidden" to preserve static checking for undefined/misspelled attributes.
+    __getattr__ = _getattr
+
 # { Initialize git executable path
+
 GIT_OK = None
 
 
@@ -232,12 +292,9 @@ def refresh(path: Optional[PathLike] = None) -> None:
     GIT_OK = True
 
 
-# } END initialize git executable path
-
-
-#################
 try:
     refresh()
 except Exception as _exc:
     raise ImportError("Failed to initialize: {0}".format(_exc)) from _exc
-#################
+
+# } END initialize git executable path

git/cmd.py

@@ -5,7 +5,7 @@
 
 from __future__ import annotations
 
-__all__ = ["Git"]
+__all__ = ["GitMeta", "Git"]
 
 import contextlib
 import io
@@ -19,6 +19,7 @@
 import sys
 from textwrap import dedent
 import threading
+import warnings
 
 from git.compat import defenc, force_bytes, safe_decode
 from git.exc import (
@@ -307,8 +308,79 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
 
 ## -- End Utilities -- @}
 
+_USE_SHELL_DEFAULT_MESSAGE = (
+    "Git.USE_SHELL is deprecated, because only its default value of False is safe. "
+    "It will be removed in a future release."
+)
+
+_USE_SHELL_DANGER_MESSAGE = (
+    "Setting Git.USE_SHELL to True is unsafe and insecure, as the effect of special "
+    "shell syntax cannot usually be accounted for. This can result in a command "
+    "injection vulnerability and arbitrary code execution. Git.USE_SHELL is deprecated "
+    "and will be removed in a future release."
+)
+
+
+def _warn_use_shell(extra_danger: bool) -> None:
+    warnings.warn(
+        _USE_SHELL_DANGER_MESSAGE if extra_danger else _USE_SHELL_DEFAULT_MESSAGE,
+        DeprecationWarning,
+        stacklevel=3,
+    )
+
+
+class _GitMeta(type):
+    """Metaclass for :class:`Git`.
+
+    This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used.
+    """
+
+    def __getattribute(cls, name: str) -> Any:
+        if name == "USE_SHELL":
+            _warn_use_shell(False)
+        return super().__getattribute__(name)
+
+    def __setattr(cls, name: str, value: Any) -> Any:
+        if name == "USE_SHELL":
+            _warn_use_shell(value)
+        super().__setattr__(name, value)
+
+    if not TYPE_CHECKING:
+        # To preserve static checking for undefined/misspelled attributes while letting
+        # the methods' bodies be type-checked, these are defined as non-special methods,
+        # then bound to special names out of view of static type checkers. (The original
+        # names invoke name mangling (leading "__") to avoid confusion in other scopes.)
+        __getattribute__ = __getattribute
+        __setattr__ = __setattr
+
+
+GitMeta = _GitMeta
+"""Alias of :class:`Git`'s metaclass, whether it is :class:`type` or a custom metaclass.
+
+Whether the :class:`Git` class has the default :class:`type` as its metaclass or uses a
+custom metaclass is not documented and may change at any time. This statically checkable
+metaclass alias is equivalent at runtime to ``type(Git)``. This should almost never be
+used. Code that benefits from it is likely to be remain brittle even if it is used.
 
-class Git:
+In view of the :class:`Git` class's intended use and :class:`Git` objects' dynamic
+callable attributes representing git subcommands, it rarely makes sense to inherit from
+:class:`Git` at all. Using :class:`Git` in multiple inheritance can be especially tricky
+to do correctly. Attempting uses of :class:`Git` where its metaclass is relevant, such
+as when a sibling class has an unrelated metaclass and a shared lower bound metaclass
+might have to be introduced to solve a metaclass conflict, is not recommended.
+
+:note:
+    The correct static type of the :class:`Git` class itself, and any subclasses, is
+    ``Type[Git]``. (This can be written as ``type[Git]`` in Python 3.9 later.)
+
+    :class:`GitMeta` should never be used in any annotation where ``Type[Git]`` is
+    intended or otherwise possible to use. This alias is truly only for very rare and
+    inherently precarious situations where it is necessary to deal with the metaclass
+    explicitly.
+"""
+
+
+class Git(metaclass=_GitMeta):
     """The Git class manages communication with the Git binary.
 
     It provides a convenient interface to calling the Git binary, such as in::
@@ -358,24 +430,53 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
     GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
     """Enables debugging of GitPython's git commands."""
 
-    USE_SHELL = False
+    USE_SHELL: bool = False
     """Deprecated. If set to ``True``, a shell will be used when executing git commands.
 
+    Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
+    functions should be updated to use the default value of ``False`` instead. ``True``
+    is unsafe unless the effect of syntax treated specially by the shell is fully
+    considered and accounted for, which is not possible under most circumstances. As
+    detailed below, it is also no longer needed, even where it had been in the past.
+
+    It is in many if not most cases a command injection vulnerability for an application
+    to set :attr:`USE_SHELL` to ``True``. Any attacker who can cause a specially crafted
+    fragment of text to make its way into any part of any argument to any git command
+    (including paths, branch names, etc.) can cause the shell to read and write
+    arbitrary files and execute arbitrary commands. Innocent input may also accidentally
+    contain special shell syntax, leading to inadvertent malfunctions.
+
+    In addition, how a value of ``True`` interacts with some aspects of GitPython's
+    operation is not precisely specified and may change without warning, even before
+    GitPython 4.0.0 when :attr:`USE_SHELL` may be removed. This includes:
+
+    * Whether or how GitPython automatically customizes the shell environment.
+
+    * Whether, outside of Windows (where :class:`subprocess.Popen` supports lists of
+      separate arguments even when ``shell=True``), this can be used with any GitPython
+      functionality other than direct calls to the :meth:`execute` method.
+
+    * Whether any GitPython feature that runs git commands ever attempts to partially
+      sanitize data a shell may treat specially. Currently this is not done.
+
     Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows
     in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as
     GitPython solves that problem more robustly and safely by using the
     ``CREATE_NO_WINDOW`` process creation flag on Windows.
 
-    Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
-    functions should be updated to use the default value of ``False`` instead. ``True``
-    is unsafe unless the effect of shell expansions is fully considered and accounted
-    for, which is not possible under most circumstances.
+    Because Windows path search differs subtly based on whether a shell is used, in rare
+    cases changing this from ``True`` to ``False`` may keep an unusual git "executable",
+    such as a batch file, from being found. To fix this, set the command name or full
+    path in the :envvar:`GIT_PYTHON_GIT_EXECUTABLE` environment variable or pass the
+    full path to :func:`git.refresh` (or invoke the script using a ``.exe`` shim).
 
-    See:
+    Further reading:
 
-    - :meth:`Git.execute` (on the ``shell`` parameter).
-    - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
-    - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
+    * :meth:`Git.execute` (on the ``shell`` parameter).
+    * https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
+    * https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
+    * https://github.com/python/cpython/issues/91558#issuecomment-1100942950
+    * https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
     """
 
     _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE"
@@ -868,6 +969,11 @@ def __init__(self, working_dir: Union[None, PathLike] = None) -> None:
         self.cat_file_header: Union[None, TBD] = None
         self.cat_file_all: Union[None, TBD] = None
 
+    def __getattribute__(self, name: str) -> Any:
+        if name == "USE_SHELL":
+            _warn_use_shell(False)
+        return super().__getattribute__(name)
+
     def __getattr__(self, name: str) -> Any:
         """A convenience method as it allows to call the command as if it was an object.
 
@@ -1138,7 +1244,12 @@ def execute(
 
         stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
         if shell is None:
-            shell = self.USE_SHELL
+            # Get the value of USE_SHELL with no deprecation warning. Do this without
+            # warnings.catch_warnings, to avoid a race condition with application code
+            # configuring warnings. The value could be looked up in type(self).__dict__
+            # or Git.__dict__, but those can break under some circumstances. This works
+            # the same as self.USE_SHELL in more situations; see Git.__getattribute__.
+            shell = super().__getattribute__("USE_SHELL")
         _logger.debug(
             "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)",
             redacted_command,