Skip to content

Commit 7892529

Browse files
EliahKaganclaude
andcommitted
Refactor Git.execute() command docstring per gitpython-developers#2146
gitpython-developers#2146 identifies two issues with gitpython-developers#2144's :param command: rewrite, which this fixes by reworking the block as three paragraphs: 1. Description and platform handling. A sequence is recommended. For strings: on Unix-like systems the whole string is the program name (so multi-word strings raise GitCommandNotFound); on Windows the OS parses the string itself, so multi-word strings can work there but are not portable. This corrects gitpython-developers#2144's claim that with shell=False the string "is passed as a single executable name to subprocess.Popen" -- accurate on Unix-like systems, but on Windows subprocess.Popen forwards the string to CreateProcessW and Windows command-line parsing produces argv. The portability problem is the user-visible one, so the docstring leads with that framing. 2. shell=True / Git.USE_SHELL warning. The shell interprets metacharacters such as ;, |, &, and $(...) as syntax; untrusted text in the command can then execute arbitrary OS commands. Cross-references USE_SHELL for the long-form discussion (which has the attack examples and history). 3. shlex.split caveat. Far safer than shell=True (so gitpython-developers#2144's harm-reduction note is preserved), but qualified: it parses POSIX shell syntax and is still unsafe for anything but fixed, fully trusted strings. Calls out the f-string-then-shlex.split anti-pattern explicitly -- whitespace or quoting in an untrusted interpolated value can still inject arguments, so shlex.split should not be recommended as a default tokenization tool. For untrusted input, build the sequence yourself. Removes gitpython-developers#2144's hedged "possible security implications" wording and its implicit recommendation of shlex.split as a general string- to-argv splitter, in favor of named mechanisms and a narrow qualified mention. Documentation only; behavior is unchanged. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7b83f7a commit 7892529

1 file changed

Lines changed: 26 additions & 10 deletions

File tree

git/cmd.py

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,16 +1131,28 @@ def execute(
11311131
information (stdout).
11321132
11331133
:param command:
1134-
The command to execute. A sequence of program arguments is the
1135-
recommended form when `shell` is ``False`` (the default), e.g.
1136-
``["git", "log", "-n", "1"]``.
1137-
1138-
A string is accepted, but with `shell` set to ``False`` it is passed
1139-
as a single executable name to :class:`subprocess.Popen`. For example,
1140-
``"git log -n 1"`` looks for an executable literally named
1141-
``git log -n 1`` and will fail with :class:`GitCommandNotFound`. To
1142-
split a command string into argv tokens, pass ``shlex.split(...)`` as
1143-
a sequence or set `shell` to ``True`` (see the warning below).
1134+
The command to execute. A sequence of program arguments is
1135+
recommended. A string is also accepted, but handling is
1136+
platform-dependent.
1137+
1138+
On Unix-like systems, a string is the whole program name (so
1139+
``"git log -n 1"`` raises :class:`GitCommandNotFound`). On
1140+
Windows, the OS parses the string itself, so multi-word strings
1141+
can work but are not portable.
1142+
1143+
Avoid ``shell=True`` (and :attr:`Git.USE_SHELL`): the platform
1144+
shell interprets metacharacters such as ``;``, ``|``, ``&``,
1145+
and ``$(...)`` as syntax. Any untrusted text in the command
1146+
can then execute arbitrary OS commands. See :attr:`Git.USE_SHELL`.
1147+
1148+
:func:`shlex.split` is far safer than ``shell=True``. But it
1149+
parses POSIX shell syntax on all systems, and it is still
1150+
unsafe for anything but *fixed, fully trusted* strings. Do
1151+
not use it on strings built by interpolating values:
1152+
whitespace or quoting in an untrusted value can still inject
1153+
arguments. For input derived in any way from untrusted data,
1154+
build the argument sequence yourself, while ensuring each
1155+
argument is fully sanitized.
11441156
11451157
:param istream:
11461158
Standard input filehandle passed to :class:`subprocess.Popen`.
@@ -1208,6 +1220,10 @@ def execute(
12081220
needed (nor useful) to work around any known operating system specific
12091221
issues.
12101222
1223+
On Unix-like systems, when migrating away from passing string commands
1224+
with ``shell=True``, :func:`shlex.split` may serve as a transitional
1225+
step in rare cases, but this should be undertaken with extreme care.
1226+
12111227
:param env:
12121228
A dictionary of environment variables to be passed to
12131229
:class:`subprocess.Popen`.

0 commit comments

Comments
 (0)