Skip to content

fix(security): prevent command injection via shell=True (CWE-78)#1941

Open
bearomorphism wants to merge 1 commit intomasterfrom
fix/command-injection-shell-true
Open

fix(security): prevent command injection via shell=True (CWE-78)#1941
bearomorphism wants to merge 1 commit intomasterfrom
fix/command-injection-shell-true

Conversation

@bearomorphism
Copy link
Copy Markdown
Collaborator

@bearomorphism bearomorphism commented May 3, 2026

Description

Fix command injection vulnerability (CWE-78) where shell=True in cmd.run() allowed arbitrary command execution via user-controlled values interpolated into git commands (e.g., annotated_tag_message in pyproject.toml).

Closes #1918

Changes

Split cmd.run() into two explicit APIs:

  • cmd.run(Sequence[str]) -- executes commands with shell=False (safe, no injection)
  • cmd.run(str) -- deprecated, emits DeprecationWarning, still uses shell=True for backward compat
  • cmd.run_shell(str) -- explicit opt-in for shell=True (only used by user-defined hooks)

Converted all git operations to list-based execution (shell=False):

  • git.tag(), git.add(), git.commit(), git.get_commits(), git.get_tags()
  • git.get_filenames_in_commit(), git.tag_exist(), git.is_signed_tag()
  • git.get_tag_message(), git.get_tag_names(), git.find_git_project_root()
  • git.is_staging_clean(), git.is_git_project(), git.get_core_editor()
  • git._get_log_as_str_list(), git.get_default_branch()

Eliminated args string splitting (list->string->list round-trip):

  • git.commit(args=...) and git.get_commits(args=...) now accept Sequence[str] instead of str
  • cli.py passes extra_cli_args as a list directly from argparse (no " ".join())
  • bump._get_commit_args() returns list[str] instead of joined string
  • This preserves arguments with spaces that were previously broken by naive .split()

Other improvements:

  • git.commit() uses env={"GIT_COMMITTER_DATE": ...} instead of OS-specific shell tricks (cmd /v /c on Windows, env prefix on Unix)
  • Removed _create_commit_cmd_string() helper (no longer needed)
  • Removed shell quoting artifacts from get_tags() and get_tag_message() format strings (fixes a Windows bug where literal quotes appeared in output)
  • hooks.py uses cmd.run_shell() -- intentionally shell-based for user-defined hooks with pipes/redirects

Checklist

Was generative AI tooling used to co-author this PR?

  • Yes (please specify the tool below)

Generated-by: GitHub Copilot CLI following the guidelines

Code Changes

  • Add test cases to all the changes you introduce
  • Run uv run poe all locally to ensure this change passes linter check and tests
  • Manually test the changes:
    • Verify the feature/bug fix works as expected in real-world scenarios
    • Test edge cases and error conditions
    • Ensure backward compatibility is maintained
    • Document any manual testing steps performed
  • Update the documentation for the changes

Documentation Changes

N/A - Internal implementation change only, no user-facing behavior or CLI changes.

Expected Behavior

User-controlled values (tag names, messages, file paths) are passed as literal arguments to git commands without shell interpretation, preventing command injection attacks in CI/CD pipelines.

Steps to Test This Pull Request

  1. Create a pyproject.toml with a malicious annotated_tag_message:
    [tool.commitizen]
    name = "cz_conventional_commits"
    version = "0.1.0"
    tag_format = "$version"
    annotated_tag = true
    annotated_tag_message = 'Release" && echo INJECTED && echo "'
  2. Run cz bump --yes
  3. Verify that INJECTED is NOT printed (command not executed)
  4. Verify the tag is created with the literal message string

Verification: All Attack Vectors from #1918 Addressed

Affected code (from issue) Fix applied
cmd.run(f'git tag {option} {tag} -m "{msg or tag}"') cmd.run(["git", "tag", option, tag, "-m", msg or tag]) (line 177)
cmd.run(f"git tag {tag}") cmd.run(["git", "tag", tag]) (line 171)
cmd.run(f"git add {' '.join(args)}") cmd.run(["git", "add", *args]) (line 181)
subprocess.Popen(cmd, shell=True) shell=False for all list-based cmd.run() calls

Manual validation result: Running cz bump with a malicious annotated_tag_message = 'Release" && echo INJECTED && echo "' -- the injected command was NOT executed, and the tag was created with the literal string as its message.

Backward Compatibility

cmd.run() accepts both str (deprecated, emits DeprecationWarning, uses shell=True) and Sequence[str] (safe, shell=False). External plugins calling cmd.run("git ...") will continue to work but receive a deprecation warning guiding them to migrate.

Additional Context

Related: CWE-78 (OS Command Injection)

The attack vector requires a poisoned pyproject.toml (e.g., via malicious PR) that gets executed in CI/CD running cz bump. With shell=True, values like annotated_tag_message were interpolated into shell command strings, allowing arbitrary command execution. With shell=False + list args, each value is a literal argv entry -- shell metacharacters (&&, ;, |, backticks) have no special meaning.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 3, 2026

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.23%. Comparing base (b4f4209) to head (90b055b).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
commitizen/commands/init.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
+ Coverage   98.10%   98.23%   +0.12%     
==========================================
  Files          61       61              
  Lines        2748     2772      +24     
==========================================
+ Hits         2696     2723      +27     
+ Misses         52       49       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bearomorphism bearomorphism force-pushed the fix/command-injection-shell-true branch from a0efa57 to 11d6890 Compare May 3, 2026 05:46
@bearomorphism bearomorphism marked this pull request as draft May 3, 2026 05:48
@bearomorphism bearomorphism force-pushed the fix/command-injection-shell-true branch 10 times, most recently from 28f982f to 713e7ed Compare May 3, 2026 08:38
@bearomorphism bearomorphism marked this pull request as ready for review May 3, 2026 08:41
@bearomorphism bearomorphism requested a review from Copilot May 3, 2026 08:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens command execution throughout Commitizen by separating safe subprocess usage from explicit shell execution, addressing the reported shell=True command-injection path in git operations and related CLI argument handling.

Changes:

  • Split command execution into cmd.run() for argv-based safe execution and cmd.run_shell() for intentional shell use in hooks.
  • Converted git-related commands and commit/changelog argument plumbing from shell strings to list-based subprocess arguments.
  • Updated tests to cover the new command APIs, env passing, hook execution, and list-based CLI/git argument handling.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/utils.py Updates test helpers to use argv-based git commands and trims branch-name output.
tests/test_git.py Revises git tests for list-based command execution and new commit/env behavior.
tests/test_conf.py Switches test repo setup to argv-based git init.
tests/test_cmd.py Adds direct tests for cmd.run, cmd.run_shell, and env propagation.
tests/test_bump_hooks.py Updates hook tests to patch run_shell and adds integration coverage.
tests/test_bump_create_commit_message.py Replaces shell-string git commands in bump/changelog tests and keeps hook install via shell.
tests/conftest.py Converts fixture setup and GPG/git config helpers to argv-based commands.
tests/commands/test_commit_command.py Updates commit-command expectations to list args and adds CLI extra-args parsing coverage.
tests/commands/test_bump_command.py Adjusts git command assertions and replaces shell filesystem setup with Path APIs.
commitizen/hooks.py Routes user-defined hooks through explicit run_shell().
commitizen/git.py Converts git operations to safe argv execution, passes commit env explicitly, and removes shell-string commit construction.
commitizen/commands/init.py Builds pre-commit install as argv instead of a shell string.
commitizen/commands/commit.py Changes extra git args from string to list[str] and forwards them directly to git.commit().
commitizen/commands/changelog.py Passes topo-order git-log args as a list.
commitizen/commands/bump.py Returns commit args as list[str] for safe downstream execution.
commitizen/cmd.py Introduces shared _popen() plus explicit safe run() and shell-based run_shell() APIs.
commitizen/cli.py Preserves extra CLI args as a list instead of joining/splitting a string.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_git.py Outdated
Comment thread tests/test_git.py Outdated
@bearomorphism bearomorphism force-pushed the fix/command-injection-shell-true branch 3 times, most recently from 153d8f6 to e7aa5f8 Compare May 3, 2026 10:28
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 <[email protected]>
@bearomorphism bearomorphism force-pushed the fix/command-injection-shell-true branch from e7aa5f8 to 90b055b Compare May 3, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Command injection via shell=True in git command execution (CWE-78)

2 participants