Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 4e626bd

Browse filesBrowse files
ByronByron
authored
Merge pull request #1886 from EliahKagan/deprecation-warnings
Issue and test deprecation warnings
2 parents c7675d2 + f6060df commit 4e626bd
Copy full SHA for 4e626bd

21 files changed

+1257
-63
lines changed

‎.github/workflows/pythonpackage.yml

Copy file name to clipboardExpand all lines: .github/workflows/pythonpackage.yml
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ jobs:
8888

8989
- name: Check types with mypy
9090
run: |
91-
mypy --python-version=${{ matrix.python-version }} -p git
91+
mypy --python-version=${{ matrix.python-version }}
9292
env:
9393
MYPY_FORCE_COLOR: "1"
9494
TERM: "xterm-256color" # For color: https://github.com/python/mypy/issues/13817

‎README.md

Copy file name to clipboardExpand all lines: README.md
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ This includes the linting and autoformatting done by Ruff, as well as some other
167167
To typecheck, run:
168168

169169
```sh
170-
mypy -p git
170+
mypy
171171
```
172172

173173
#### CI (and tox)

‎git/__init__.py

Copy file name to clipboardExpand all lines: git/__init__.py
+83-26Lines changed: 83 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,12 @@
8888

8989
__version__ = "git"
9090

91-
from typing import List, Optional, Sequence, TYPE_CHECKING, Tuple, Union
91+
from typing import Any, List, Optional, Sequence, TYPE_CHECKING, Tuple, Union
92+
93+
if TYPE_CHECKING:
94+
from types import ModuleType
95+
96+
import warnings
9297

9398
from gitdb.util import to_hex_sha
9499

@@ -144,11 +149,6 @@
144149
SymbolicReference,
145150
Tag,
146151
TagReference,
147-
head, # noqa: F401 # Nonpublic. May disappear! Use git.refs.head.
148-
log, # noqa: F401 # Nonpublic. May disappear! Use git.refs.log.
149-
reference, # noqa: F401 # Nonpublic. May disappear! Use git.refs.reference.
150-
symbolic, # noqa: F401 # Nonpublic. May disappear! Use git.refs.symbolic.
151-
tag, # noqa: F401 # Nonpublic. May disappear! Use git.refs.tag.
152152
)
153153
from git.diff import ( # @NoMove
154154
INDEX,
@@ -169,21 +169,9 @@
169169
IndexEntry,
170170
IndexFile,
171171
StageType,
172-
base, # noqa: F401 # Nonpublic. May disappear! Use git.index.base.
173-
fun, # noqa: F401 # Nonpublic. May disappear! Use git.index.fun.
174-
typ, # noqa: F401 # Nonpublic. May disappear! Use git.index.typ.
175-
#
176-
# NOTE: The expression `git.util` evaluates to git.index.util, and the import
177-
# `from git import util` imports git.index.util, NOT git.util. It may not be
178-
# feasible to change this until the next major version, to avoid breaking code
179-
# inadvertently relying on it. If git.index.util really is what you want, use or
180-
# import from that name, to avoid confusion. To use the "real" git.util module,
181-
# write `from git.util import ...`, or access it as `sys.modules["git.util"]`.
182-
# (This differs from other historical indirect-submodule imports that are
183-
# unambiguously nonpublic and are subject to immediate removal. Here, the public
184-
# git.util module, even though different, makes it less discoverable that the
185-
# expression `git.util` refers to a non-public attribute of the git module.)
186-
util, # noqa: F401
172+
# NOTE: This tells type checkers what util resolves to. We delete it, and it is
173+
# really resolved by __getattr__, which warns. See below on what to use instead.
174+
util,
187175
)
188176
from git.util import ( # @NoMove
189177
Actor,
@@ -196,7 +184,79 @@
196184
except GitError as _exc:
197185
raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc
198186

187+
188+
def _warned_import(message: str, fullname: str) -> "ModuleType":
189+
import importlib
190+
191+
warnings.warn(message, DeprecationWarning, stacklevel=3)
192+
return importlib.import_module(fullname)
193+
194+
195+
def _getattr(name: str) -> Any:
196+
# TODO: If __version__ is made dynamic and lazily fetched, put that case right here.
197+
198+
if name == "util":
199+
return _warned_import(
200+
"The expression `git.util` and the import `from git import util` actually "
201+
"reference git.index.util, and not the git.util module accessed in "
202+
'`from git.util import XYZ` or `sys.modules["git.util"]`. This potentially '
203+
"confusing behavior is currently preserved for compatibility, but may be "
204+
"changed in the future and should not be relied on.",
205+
fullname="git.index.util",
206+
)
207+
208+
for names, prefix in (
209+
({"head", "log", "reference", "symbolic", "tag"}, "git.refs"),
210+
({"base", "fun", "typ"}, "git.index"),
211+
):
212+
if name not in names:
213+
continue
214+
215+
fullname = f"{prefix}.{name}"
216+
217+
return _warned_import(
218+
f"{__name__}.{name} is a private alias of {fullname} and subject to "
219+
f"immediate removal. Use {fullname} instead.",
220+
fullname=fullname,
221+
)
222+
223+
raise AttributeError(f"module {__name__!r} has no attribute {name!r}")
224+
225+
226+
if not TYPE_CHECKING:
227+
# NOTE: The expression `git.util` gives git.index.util and `from git import util`
228+
# imports git.index.util, NOT git.util. It may not be feasible to change this until
229+
# the next major version, to avoid breaking code inadvertently relying on it.
230+
#
231+
# - If git.index.util *is* what you want, use (or import from) that, to avoid
232+
# confusion.
233+
#
234+
# - To use the "real" git.util module, write `from git.util import ...`, or if
235+
# necessary access it as `sys.modules["git.util"]`.
236+
#
237+
# Note also that `import git.util` technically imports the "real" git.util... but
238+
# the *expression* `git.util` after doing so is still git.index.util!
239+
#
240+
# (This situation differs from that of other indirect-submodule imports that are
241+
# unambiguously non-public and subject to immediate removal. Here, the public
242+
# git.util module, though different, makes less discoverable that the expression
243+
# `git.util` refers to a non-public attribute of the git module.)
244+
#
245+
# This had originally come about by a wildcard import. Now that all intended imports
246+
# are explicit, the intuitive but potentially incompatible binding occurs due to the
247+
# usual rules for Python submodule bindings. So for now we replace that binding with
248+
# git.index.util, delete that, and let __getattr__ handle it and issue a warning.
249+
#
250+
# For the same runtime behavior, it would be enough to forgo importing util, and
251+
# delete util as created naturally; __getattr__ would behave the same. But type
252+
# checkers would not know what util refers to when accessed as an attribute of git.
253+
del util
254+
255+
# This is "hidden" to preserve static checking for undefined/misspelled attributes.
256+
__getattr__ = _getattr
257+
199258
# { Initialize git executable path
259+
200260
GIT_OK = None
201261

202262

@@ -232,12 +292,9 @@ def refresh(path: Optional[PathLike] = None) -> None:
232292
GIT_OK = True
233293

234294

235-
# } END initialize git executable path
236-
237-
238-
#################
239295
try:
240296
refresh()
241297
except Exception as _exc:
242298
raise ImportError("Failed to initialize: {0}".format(_exc)) from _exc
243-
#################
299+
300+
# } END initialize git executable path

‎git/cmd.py

Copy file name to clipboardExpand all lines: git/cmd.py
+123-12Lines changed: 123 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
from __future__ import annotations
77

8-
__all__ = ["Git"]
8+
__all__ = ["GitMeta", "Git"]
99

1010
import contextlib
1111
import io
@@ -19,6 +19,7 @@
1919
import sys
2020
from textwrap import dedent
2121
import threading
22+
import warnings
2223

2324
from git.compat import defenc, force_bytes, safe_decode
2425
from git.exc import (
@@ -307,8 +308,79 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc
307308

308309
## -- End Utilities -- @}
309310

311+
_USE_SHELL_DEFAULT_MESSAGE = (
312+
"Git.USE_SHELL is deprecated, because only its default value of False is safe. "
313+
"It will be removed in a future release."
314+
)
315+
316+
_USE_SHELL_DANGER_MESSAGE = (
317+
"Setting Git.USE_SHELL to True is unsafe and insecure, as the effect of special "
318+
"shell syntax cannot usually be accounted for. This can result in a command "
319+
"injection vulnerability and arbitrary code execution. Git.USE_SHELL is deprecated "
320+
"and will be removed in a future release."
321+
)
322+
323+
324+
def _warn_use_shell(extra_danger: bool) -> None:
325+
warnings.warn(
326+
_USE_SHELL_DANGER_MESSAGE if extra_danger else _USE_SHELL_DEFAULT_MESSAGE,
327+
DeprecationWarning,
328+
stacklevel=3,
329+
)
330+
331+
332+
class _GitMeta(type):
333+
"""Metaclass for :class:`Git`.
334+
335+
This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used.
336+
"""
337+
338+
def __getattribute(cls, name: str) -> Any:
339+
if name == "USE_SHELL":
340+
_warn_use_shell(False)
341+
return super().__getattribute__(name)
342+
343+
def __setattr(cls, name: str, value: Any) -> Any:
344+
if name == "USE_SHELL":
345+
_warn_use_shell(value)
346+
super().__setattr__(name, value)
347+
348+
if not TYPE_CHECKING:
349+
# To preserve static checking for undefined/misspelled attributes while letting
350+
# the methods' bodies be type-checked, these are defined as non-special methods,
351+
# then bound to special names out of view of static type checkers. (The original
352+
# names invoke name mangling (leading "__") to avoid confusion in other scopes.)
353+
__getattribute__ = __getattribute
354+
__setattr__ = __setattr
355+
356+
357+
GitMeta = _GitMeta
358+
"""Alias of :class:`Git`'s metaclass, whether it is :class:`type` or a custom metaclass.
359+
360+
Whether the :class:`Git` class has the default :class:`type` as its metaclass or uses a
361+
custom metaclass is not documented and may change at any time. This statically checkable
362+
metaclass alias is equivalent at runtime to ``type(Git)``. This should almost never be
363+
used. Code that benefits from it is likely to be remain brittle even if it is used.
310364
311-
class Git:
365+
In view of the :class:`Git` class's intended use and :class:`Git` objects' dynamic
366+
callable attributes representing git subcommands, it rarely makes sense to inherit from
367+
:class:`Git` at all. Using :class:`Git` in multiple inheritance can be especially tricky
368+
to do correctly. Attempting uses of :class:`Git` where its metaclass is relevant, such
369+
as when a sibling class has an unrelated metaclass and a shared lower bound metaclass
370+
might have to be introduced to solve a metaclass conflict, is not recommended.
371+
372+
:note:
373+
The correct static type of the :class:`Git` class itself, and any subclasses, is
374+
``Type[Git]``. (This can be written as ``type[Git]`` in Python 3.9 later.)
375+
376+
:class:`GitMeta` should never be used in any annotation where ``Type[Git]`` is
377+
intended or otherwise possible to use. This alias is truly only for very rare and
378+
inherently precarious situations where it is necessary to deal with the metaclass
379+
explicitly.
380+
"""
381+
382+
383+
class Git(metaclass=_GitMeta):
312384
"""The Git class manages communication with the Git binary.
313385
314386
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:
358430
GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False)
359431
"""Enables debugging of GitPython's git commands."""
360432

361-
USE_SHELL = False
433+
USE_SHELL: bool = False
362434
"""Deprecated. If set to ``True``, a shell will be used when executing git commands.
363435
436+
Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
437+
functions should be updated to use the default value of ``False`` instead. ``True``
438+
is unsafe unless the effect of syntax treated specially by the shell is fully
439+
considered and accounted for, which is not possible under most circumstances. As
440+
detailed below, it is also no longer needed, even where it had been in the past.
441+
442+
It is in many if not most cases a command injection vulnerability for an application
443+
to set :attr:`USE_SHELL` to ``True``. Any attacker who can cause a specially crafted
444+
fragment of text to make its way into any part of any argument to any git command
445+
(including paths, branch names, etc.) can cause the shell to read and write
446+
arbitrary files and execute arbitrary commands. Innocent input may also accidentally
447+
contain special shell syntax, leading to inadvertent malfunctions.
448+
449+
In addition, how a value of ``True`` interacts with some aspects of GitPython's
450+
operation is not precisely specified and may change without warning, even before
451+
GitPython 4.0.0 when :attr:`USE_SHELL` may be removed. This includes:
452+
453+
* Whether or how GitPython automatically customizes the shell environment.
454+
455+
* Whether, outside of Windows (where :class:`subprocess.Popen` supports lists of
456+
separate arguments even when ``shell=True``), this can be used with any GitPython
457+
functionality other than direct calls to the :meth:`execute` method.
458+
459+
* Whether any GitPython feature that runs git commands ever attempts to partially
460+
sanitize data a shell may treat specially. Currently this is not done.
461+
364462
Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows
365463
in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as
366464
GitPython solves that problem more robustly and safely by using the
367465
``CREATE_NO_WINDOW`` process creation flag on Windows.
368466
369-
Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
370-
functions should be updated to use the default value of ``False`` instead. ``True``
371-
is unsafe unless the effect of shell expansions is fully considered and accounted
372-
for, which is not possible under most circumstances.
467+
Because Windows path search differs subtly based on whether a shell is used, in rare
468+
cases changing this from ``True`` to ``False`` may keep an unusual git "executable",
469+
such as a batch file, from being found. To fix this, set the command name or full
470+
path in the :envvar:`GIT_PYTHON_GIT_EXECUTABLE` environment variable or pass the
471+
full path to :func:`git.refresh` (or invoke the script using a ``.exe`` shim).
373472
374-
See:
473+
Further reading:
375474
376-
- :meth:`Git.execute` (on the ``shell`` parameter).
377-
- https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
378-
- https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
475+
* :meth:`Git.execute` (on the ``shell`` parameter).
476+
* https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
477+
* https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
478+
* https://github.com/python/cpython/issues/91558#issuecomment-1100942950
479+
* https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
379480
"""
380481

381482
_git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE"
@@ -868,6 +969,11 @@ def __init__(self, working_dir: Union[None, PathLike] = None) -> None:
868969
self.cat_file_header: Union[None, TBD] = None
869970
self.cat_file_all: Union[None, TBD] = None
870971

972+
def __getattribute__(self, name: str) -> Any:
973+
if name == "USE_SHELL":
974+
_warn_use_shell(False)
975+
return super().__getattribute__(name)
976+
871977
def __getattr__(self, name: str) -> Any:
872978
"""A convenience method as it allows to call the command as if it was an object.
873979
@@ -1138,7 +1244,12 @@ def execute(
11381244

11391245
stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
11401246
if shell is None:
1141-
shell = self.USE_SHELL
1247+
# Get the value of USE_SHELL with no deprecation warning. Do this without
1248+
# warnings.catch_warnings, to avoid a race condition with application code
1249+
# configuring warnings. The value could be looked up in type(self).__dict__
1250+
# or Git.__dict__, but those can break under some circumstances. This works
1251+
# the same as self.USE_SHELL in more situations; see Git.__getattribute__.
1252+
shell = super().__getattribute__("USE_SHELL")
11421253
_logger.debug(
11431254
"Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)",
11441255
redacted_command,

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.