Skip to content

Commit a0efa57

Browse files
fix(security): prevent command injection via shell=True (CWE-78)
Replace shell=True with list-based subprocess calls for all git.py functions that interpolate user-controlled values (tag names, messages, file paths, git references). This prevents shell injection attacks where malicious values in pyproject.toml could execute arbitrary commands during CI/CD runs of 'cz bump'. Changes: - cmd.run() now accepts str | Sequence[str]; lists use shell=False - git.tag() uses list args (fixes primary attack vector) - git.add() uses list args - git.commit() uses list args + env= for GIT_COMMITTER_DATE - git.tag_exist/is_signed_tag/get_tag_message use list args - git.get_filenames_in_commit() uses list args - git.get_tags() uses list args - git._get_log_as_str_list() uses list args Closes #1918 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 35ffe03 commit a0efa57

3 files changed

Lines changed: 86 additions & 55 deletions

File tree

commitizen/cmd.py

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from commitizen.exceptions import CharacterSetDecodeError
1010

1111
if TYPE_CHECKING:
12-
from collections.abc import Mapping
12+
from collections.abc import Mapping, Sequence
1313

1414

1515
class Command(NamedTuple):
@@ -35,12 +35,23 @@ def _try_decode(bytes_: bytes) -> str:
3535
raise CharacterSetDecodeError() from e
3636

3737

38-
def run(cmd: str, env: Mapping[str, str] | None = None) -> Command:
38+
def run(
39+
cmd: str | Sequence[str], env: Mapping[str, str] | None = None
40+
) -> Command:
41+
"""Run a command and return its output.
42+
43+
When *cmd* is a list/sequence of strings the command is executed directly
44+
(shell=False) which avoids shell-injection vulnerabilities (CWE-78).
45+
When *cmd* is a plain string it is executed via the shell for backward
46+
compatibility with callers that rely on shell features.
47+
"""
3948
if env is not None:
4049
env = {**os.environ, **env}
50+
51+
use_shell = isinstance(cmd, str)
4152
process = subprocess.Popen(
4253
cmd,
43-
shell=True,
54+
shell=use_shell,
4455
stdout=subprocess.PIPE,
4556
stderr=subprocess.PIPE,
4657
stdin=subprocess.PIPE,

commitizen/git.py

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -164,17 +164,17 @@ def tag(
164164
tag: str, annotated: bool = False, signed: bool = False, msg: str | None = None
165165
) -> cmd.Command:
166166
if not annotated and not signed:
167-
return cmd.run(f"git tag {tag}")
167+
return cmd.run(["git", "tag", tag])
168168

169169
# according to https://git-scm.com/book/en/v2/Git-Basics-Tagging,
170170
# we're not able to create lightweight tag with message.
171171
# by adding message, we make it a annotated tags
172172
option = "-s" if signed else "-a" # The else case is for annotated tags
173-
return cmd.run(f'git tag {option} {tag} -m "{msg or tag}"')
173+
return cmd.run(["git", "tag", option, tag, "-m", msg or tag])
174174

175175

176176
def add(*args: str) -> cmd.Command:
177-
return cmd.run(f"git add {' '.join(args)}")
177+
return cmd.run(["git", "add", *args])
178178

179179

180180
def commit(
@@ -186,20 +186,18 @@ def commit(
186186
f.write(message.encode("utf-8"))
187187
f.close()
188188

189-
command = _create_commit_cmd_string(args, committer_date, f.name)
190-
c = cmd.run(command)
191-
os.unlink(f.name)
192-
return c
189+
cmd_args = ["git", "commit"]
190+
if args:
191+
cmd_args.extend(args.split())
192+
cmd_args.extend(["-F", f.name])
193193

194+
env: dict[str, str] | None = None
195+
if committer_date:
196+
env = {"GIT_COMMITTER_DATE": committer_date}
194197

195-
def _create_commit_cmd_string(args: str, committer_date: str | None, name: str) -> str:
196-
command = f'git commit {args} -F "{name}"'
197-
if not committer_date:
198-
return command
199-
if os.name != "nt":
200-
return f"GIT_COMMITTER_DATE={committer_date} {command}"
201-
# Using `cmd /v /c "{command}"` sets environment variables only for that command
202-
return f'cmd /v /c "set GIT_COMMITTER_DATE={committer_date}&& {command}"'
198+
c = cmd.run(cmd_args, env=env)
199+
os.unlink(f.name)
200+
return c
203201

204202

205203
def get_commits(
@@ -226,7 +224,10 @@ def get_filenames_in_commit(git_reference: str = "") -> list[str]:
226224
227225
:returns: file names committed in the last commit by default or inside the passed git reference
228226
"""
229-
c = cmd.run(f"git show --name-only --pretty=format: {git_reference}")
227+
cmd_args = ["git", "show", "--name-only", "--pretty=format:"]
228+
if git_reference:
229+
cmd_args.append(git_reference)
230+
c = cmd.run(cmd_args)
230231
if c.return_code == 0:
231232
return c.out.strip().split("\n")
232233
raise GitCommandError(c.err)
@@ -237,15 +238,17 @@ def get_tags(
237238
) -> list[GitTag]:
238239
inner_delimiter = "---inner_delimiter---"
239240
formatter = (
240-
f'"%(refname:strip=2){inner_delimiter}'
241+
f"%(refname:strip=2){inner_delimiter}"
241242
f"%(objectname){inner_delimiter}"
242243
f"%(creatordate:format:{dateformat}){inner_delimiter}"
243-
f'%(object)"'
244+
f"%(object)"
244245
)
245-
extra = "--merged" if reachable_only else ""
246+
cmd_args = ["git", "tag", f"--format={formatter}", "--sort=-creatordate"]
247+
if reachable_only:
248+
cmd_args.append("--merged")
246249
# Force the default language for parsing
247250
env = {"LC_ALL": "C", "LANG": "C", "LANGUAGE": "C"}
248-
c = cmd.run(f"git tag --format={formatter} --sort=-creatordate {extra}", env=env)
251+
c = cmd.run(cmd_args, env=env)
249252
if c.return_code != 0:
250253
if reachable_only and c.err == "fatal: malformed object name HEAD\n":
251254
# this can happen if there are no commits in the repo yet
@@ -262,12 +265,12 @@ def get_tags(
262265

263266

264267
def tag_exist(tag: str) -> bool:
265-
c = cmd.run(f"git tag --list {tag}")
268+
c = cmd.run(["git", "tag", "--list", tag])
266269
return tag in c.out
267270

268271

269272
def is_signed_tag(tag: str) -> bool:
270-
return cmd.run(f"git tag -v {tag}").return_code == 0
273+
return cmd.run(["git", "tag", "-v", tag]).return_code == 0
271274

272275

273276
def get_latest_tag_name() -> str | None:
@@ -278,7 +281,7 @@ def get_latest_tag_name() -> str | None:
278281

279282

280283
def get_tag_message(tag: str) -> str | None:
281-
c = cmd.run(f"git tag -l --format='%(contents:subject)' {tag}")
284+
c = cmd.run(["git", "tag", "-l", "--format=%(contents:subject)", tag])
282285
if c.err:
283286
return None
284287
return c.out.strip()
@@ -326,9 +329,16 @@ def _get_log_as_str_list(start: str | None, end: str, args: str) -> list[str]:
326329
delimiter = "----------commit-delimiter----------"
327330
log_format: str = "%H%n%P%n%s%n%an%n%ae%n%b"
328331
command_range = f"{start}..{end}" if start else end
329-
command = f"git -c log.showSignature=False log --pretty={log_format}{delimiter} {args} {command_range}"
332+
cmd_args = [
333+
"git", "-c", "log.showSignature=False", "log",
334+
f"--pretty={log_format}{delimiter}",
335+
]
336+
if args:
337+
cmd_args.extend(args.split())
338+
if command_range:
339+
cmd_args.append(command_range)
330340

331-
c = cmd.run(command)
341+
c = cmd.run(cmd_args)
332342
if c.return_code != 0:
333343
raise GitCommandError(c.err)
334344
return c.out.split(f"{delimiter}\n")

tests/test_git.py

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -346,25 +346,23 @@ def test_create_tag_with_message(util: UtilFixture):
346346
tag_message = "test message"
347347
util.create_tag(tag_name, tag_message)
348348
assert git.get_latest_tag_name() == tag_name
349-
assert git.get_tag_message(tag_name) == (
350-
tag_message if platform.system() != "Windows" else f"'{tag_message}'"
351-
)
349+
assert git.get_tag_message(tag_name) == tag_message
352350

353351

354352
@pytest.mark.parametrize(
355353
("file_path", "expected_cmd"),
356354
[
357355
(
358356
"/tmp/temp file",
359-
'git commit --signoff -F "/tmp/temp file"',
357+
["git", "commit", "--signoff", "-F", "/tmp/temp file"],
360358
),
361359
(
362360
"/tmp dir/temp file",
363-
'git commit --signoff -F "/tmp dir/temp file"',
361+
["git", "commit", "--signoff", "-F", "/tmp dir/temp file"],
364362
),
365363
(
366364
"/tmp/tempfile",
367-
'git commit --signoff -F "/tmp/tempfile"',
365+
["git", "commit", "--signoff", "-F", "/tmp/tempfile"],
368366
),
369367
],
370368
ids=[
@@ -374,7 +372,7 @@ def test_create_tag_with_message(util: UtilFixture):
374372
],
375373
)
376374
def test_commit_with_spaces_in_path(
377-
mocker: MockFixture, file_path: str, expected_cmd: str, util: UtilFixture
375+
mocker: MockFixture, file_path: str, expected_cmd: list[str], util: UtilFixture
378376
):
379377
mock_run = util.mock_cmd()
380378
mock_unlink = mocker.patch("os.unlink")
@@ -383,7 +381,7 @@ def test_commit_with_spaces_in_path(
383381

384382
git.commit("feat: new feature", "--signoff")
385383

386-
mock_run.assert_called_once_with(expected_cmd)
384+
mock_run.assert_called_once_with(expected_cmd, env=None)
387385
mock_unlink.assert_called_once_with(file_path)
388386

389387

@@ -474,29 +472,41 @@ def test_git_commit_from_rev_and_commit(linebreak):
474472

475473

476474
@pytest.mark.parametrize(
477-
("os_name", "committer_date", "expected_cmd"),
475+
("committer_date",),
478476
[
479-
(
480-
"nt",
481-
"2024-03-20",
482-
'cmd /v /c "set GIT_COMMITTER_DATE=2024-03-20&& git commit -F "temp.txt""',
483-
),
484-
(
485-
"posix",
486-
"2024-03-20",
487-
'GIT_COMMITTER_DATE=2024-03-20 git commit -F "temp.txt"',
488-
),
489-
("nt", None, 'git commit -F "temp.txt"'),
490-
("posix", None, 'git commit -F "temp.txt"'),
477+
("2024-03-20",),
478+
(None,),
491479
],
492480
)
493-
def test_create_commit_cmd_string(
494-
mocker: MockFixture, os_name: str, committer_date: str, expected_cmd: str
481+
def test_commit_uses_list_args_and_env(
482+
mocker: MockFixture, committer_date: str | None
495483
):
496-
"""Test the OS-specific behavior of _create_commit_cmd_string"""
497-
mocker.patch("os.name", os_name)
498-
result = git._create_commit_cmd_string("", committer_date, "temp.txt")
499-
assert result == expected_cmd
484+
"""Test that git.commit uses list-based subprocess (no shell injection)."""
485+
mock_run = mocker.patch("commitizen.cmd.run")
486+
mock_run.return_value = cmd.Command("", "", b"", b"", 0)
487+
488+
# We need to mock NamedTemporaryFile to control the file name
489+
mock_file = mocker.MagicMock()
490+
mock_file.name = "temp.txt"
491+
mock_file.__enter__ = mocker.MagicMock(return_value=mock_file)
492+
mock_file.__exit__ = mocker.MagicMock(return_value=False)
493+
mocker.patch("commitizen.git.NamedTemporaryFile", return_value=mock_file)
494+
mocker.patch("os.unlink")
495+
496+
git.commit("test message", args="-a --no-verify", committer_date=committer_date)
497+
498+
call_args = mock_run.call_args
499+
# First positional arg should be a list (no shell injection)
500+
cmd_list = call_args[0][0]
501+
assert isinstance(cmd_list, list)
502+
assert cmd_list == ["git", "commit", "-a", "--no-verify", "-F", "temp.txt"]
503+
504+
if committer_date:
505+
env = call_args[1]["env"]
506+
assert env == {"GIT_COMMITTER_DATE": committer_date}
507+
else:
508+
env = call_args[1]["env"]
509+
assert env is None
500510

501511

502512
def test_get_default_branch_success(util: UtilFixture):

0 commit comments

Comments
 (0)