From 010a7bbfd237d2ab0627c20a542933abf978b690 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 8 May 2026 16:34:31 -0400 Subject: [PATCH 1/2] Rewrite Git.execute() command parameter docstring per #2146 #2146 identifies two factual problems in #2144's `:param command:` rewrite, both of which this fixes: 1. The claim that with `shell=False` a string "is passed as a single executable name to `subprocess.Popen`" is accurate on Unix-like systems, but on Windows `subprocess.Popen` forwards the string to `CreateProcessW` and Windows command-line parsing produces argv. So multi-word strings happen to work on Windows -- which makes the docstring misleading for Windows readers and unhelpful for anyone whose actual problem is portability. 2. The blanket recommendation to use `shlex.split` read as a general string-to-argv splitter. `shlex.split` parses POSIX shell syntax on all systems, and the resulting tokens are still unsafe for anything but fixed, fully trusted strings -- in particular, strings built by interpolating values can still inject extra arguments via embedded whitespace or quoting. Restructure `:param command:` into four paragraphs (description, platform-specific string handling, `shell=True` / `Git.USE_SHELL` warning, qualified `shlex.split` note) and cross-reference `USE_SHELL` for the long-form security discussion rather than reproducing it. Add a related paragraph to `:param shell:` noting `shlex.split`'s narrow value as a transitional tool when migrating existing string-command `shell=True` calls on Unix-like systems, with extreme care, and cross-referencing `:param command:` for the risks. This fixes #2146. Only documentation is changed. Co-authored-by: Claude Opus 4.7 (1M context) --- git/cmd.py | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 7f2564d45..861009099 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1131,16 +1131,28 @@ def execute( information (stdout). :param command: - The command to execute. A sequence of program arguments is the - recommended form when `shell` is ``False`` (the default), e.g. - ``["git", "log", "-n", "1"]``. - - A string is accepted, but with `shell` set to ``False`` it is passed - as a single executable name to :class:`subprocess.Popen`. For example, - ``"git log -n 1"`` looks for an executable literally named - ``git log -n 1`` and will fail with :class:`GitCommandNotFound`. To - split a command string into argv tokens, pass ``shlex.split(...)`` as - a sequence or set `shell` to ``True`` (see the warning below). + The command to execute. A sequence of program arguments is recommended. + A string is also accepted, but its meaning is strongly platform-dependent. + + By default, a shell is not used. On Unix-like systems, a string is the whole + program name (so ``"git log -n 1"`` raises :class:`GitCommandNotFound`). On + Windows, the program parses the arguments itself, so multi-word strings can + work but are not portable. + + Avoid ``shell=True`` (and :attr:`Git.USE_SHELL`): this runs the command in + a shell, which is generally unsafe. The shell interprets metacharacters + such as ``;``, ``|``, ``&``, ``$(...)``, ``$VAR``, ``%VAR%``, and ``^`` + (depending on the platform) as syntax. Any untrusted text in the command + can then execute arbitrary OS commands. See :attr:`Git.USE_SHELL`. + + Producing a sequence automatically by :func:`shlex.split` and passing it + as the command is far safer than ``shell=True``. But :func:`shlex.split` + parses POSIX shell syntax on all systems, and the result is still unsafe + for anything but *fixed, fully trusted* strings. Do not use it on strings + built by interpolating values: whitespace or quoting in an untrusted value + can still inject arguments. For input derived in any way from untrusted + data, build the argument sequence yourself, while ensuring each argument + is fully sanitized. :param istream: Standard input filehandle passed to :class:`subprocess.Popen`. @@ -1208,6 +1220,11 @@ def execute( needed (nor useful) to work around any known operating system specific issues. + On Unix-like systems, when migrating away from passing string commands with + ``shell=True``, :func:`shlex.split` may serve as a transitional step in rare + cases, but this should be undertaken with extreme care. See the `command` + parameter above on the risks. + :param env: A dictionary of environment variables to be passed to :class:`subprocess.Popen`. From 38d62c4b60b5cd2ed743c00f2cac1875c76449e0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 9 May 2026 21:19:10 -0400 Subject: [PATCH 2/2] Word `shell` mention of `shlex.split` more carefully To avert the mistake of not removing `shell=True` when using it. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) --- git/cmd.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 861009099..98bb62775 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1222,8 +1222,8 @@ def execute( On Unix-like systems, when migrating away from passing string commands with ``shell=True``, :func:`shlex.split` may serve as a transitional step in rare - cases, but this should be undertaken with extreme care. See the `command` - parameter above on the risks. + cases, with extreme care. (Drop ``shell=True`` and pass the resulting + sequence as the command.) See the `command` parameter above on the risks. :param env: A dictionary of environment variables to be passed to