diff --git a/commitizen/cli.py b/commitizen/cli.py index 85674e8c4..177099480 100644 --- a/commitizen/cli.py +++ b/commitizen/cli.py @@ -704,7 +704,7 @@ def main() -> None: logger.warning( "\nWARN: Incomplete commit command: received -- separator without any following git arguments\n" ) - extra_args = " ".join(unknown_args[1:]) + extra_args = unknown_args[1:] arguments["extra_cli_args"] = extra_args conf = config.read_cfg(args.config) diff --git a/commitizen/cmd.py b/commitizen/cmd.py index fe70da9c9..effe1ff37 100644 --- a/commitizen/cmd.py +++ b/commitizen/cmd.py @@ -2,14 +2,15 @@ import os import subprocess -from typing import TYPE_CHECKING, NamedTuple +import warnings +from typing import TYPE_CHECKING, NamedTuple, overload from charset_normalizer import from_bytes from commitizen.exceptions import CharacterSetDecodeError if TYPE_CHECKING: - from collections.abc import Mapping + from collections.abc import Mapping, Sequence class Command(NamedTuple): @@ -35,12 +36,18 @@ def _try_decode(bytes_: bytes) -> str: raise CharacterSetDecodeError() from e -def run(cmd: str, env: Mapping[str, str] | None = None) -> Command: +def _popen( + cmd: str | Sequence[str], + *, + shell: bool, + env: Mapping[str, str] | None = None, +) -> Command: if env is not None: env = {**os.environ, **env} + process = subprocess.Popen( cmd, - shell=True, + shell=shell, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, @@ -55,3 +62,44 @@ def run(cmd: str, env: Mapping[str, str] | None = None) -> Command: stderr, return_code, ) + + +@overload +def run(cmd: str, env: Mapping[str, str] | None = None) -> Command: ... + + +@overload +def run(cmd: Sequence[str], env: Mapping[str, str] | None = None) -> Command: ... + + +def run(cmd: str | Sequence[str], env: Mapping[str, str] | None = None) -> Command: + """Run a command safely without shell interpretation (shell=False). + + Arguments are passed directly to the OS, preventing shell-injection + vulnerabilities (CWE-78). + + Passing a string is deprecated and will be removed in a future version. + Use a list of arguments instead, or use run_shell() for shell features. + """ + if isinstance(cmd, str): + warnings.warn( + "Passing a string to cmd.run() is deprecated and will be removed in v5. " + "Use a list of arguments instead, or use cmd.run_shell() explicitly.", + DeprecationWarning, + stacklevel=2, + ) + return _popen(cmd, shell=True, env=env) + return _popen(cmd, shell=False, env=env) + + +def run_shell(cmd: str, env: Mapping[str, str] | None = None) -> Command: + """Run a command string via the system shell (shell=True). + + Only use this for cases that intentionally require shell features + (e.g., user-defined hooks with pipes/redirects). Never pass + untrusted/user-controlled values into *cmd*. + + Related: CWE-78 (OS Command Injection), + https://github.com/commitizen-tools/commitizen/issues/1918 + """ + return _popen(cmd, shell=True, env=env) diff --git a/commitizen/commands/bump.py b/commitizen/commands/bump.py index 9e2ae81b1..0b6e0ffa3 100644 --- a/commitizen/commands/bump.py +++ b/commitizen/commands/bump.py @@ -439,8 +439,8 @@ def __call__(self) -> None: else: out.success("Done!") - def _get_commit_args(self) -> str: + def _get_commit_args(self) -> list[str]: commit_args = ["-a"] if self.no_verify: commit_args.append("--no-verify") - return " ".join(commit_args) + return commit_args diff --git a/commitizen/commands/changelog.py b/commitizen/commands/changelog.py index 8cef5d39f..5521da373 100644 --- a/commitizen/commands/changelog.py +++ b/commitizen/commands/changelog.py @@ -257,7 +257,7 @@ def __call__(self) -> None: changelog_meta.latest_version_position = None changelog_meta.unreleased_end = latest_full_release_info.index + 1 - commits = git.get_commits(start=start_rev, end=end_rev, args="--topo-order") + commits = git.get_commits(start=start_rev, end=end_rev, args=["--topo-order"]) if ( not self.allow_no_commit and not commits diff --git a/commitizen/commands/commit.py b/commitizen/commands/commit.py index 6668c0d65..178df2c82 100644 --- a/commitizen/commands/commit.py +++ b/commitizen/commands/commit.py @@ -34,7 +34,7 @@ class CommitArgs(TypedDict, total=False): all: bool dry_run: bool edit: bool - extra_cli_args: str + extra_cli_args: list[str] message_length_limit: int no_retry: bool signoff: bool @@ -132,7 +132,7 @@ def _get_message(self) -> str: return self._get_message_by_prompt_commit_questions() def __call__(self) -> None: - extra_args = self.arguments.get("extra_cli_args", "") + extra_args: list[str] = self.arguments.get("extra_cli_args", []) dry_run = bool(self.arguments.get("dry_run")) write_message_to_file = self.arguments.get("write_message_to_file") signoff = bool(self.arguments.get("signoff")) @@ -167,7 +167,7 @@ def __call__(self) -> None: raise DryRunExit() if self.config.settings["always_signoff"] or signoff: - extra_args = f"{extra_args} -s".strip() + extra_args = [*extra_args, "-s"] c = git.commit(commit_message, args=extra_args) if c.return_code != 0: diff --git a/commitizen/commands/init.py b/commitizen/commands/init.py index ad33d884b..f05bc23c1 100644 --- a/commitizen/commands/init.py +++ b/commitizen/commands/init.py @@ -134,11 +134,12 @@ def __call__(self) -> None: "pre-commit is not installed in current environment." ) - cmd_str = "pre-commit install " + " ".join( - f"--hook-type {ty}" for ty in hook_types - ) - c = cmd.run(cmd_str) + cmd_args = ["pre-commit", "install"] + for ty in hook_types: + cmd_args.extend(["--hook-type", ty]) + c = cmd.run(cmd_args) if c.return_code != 0: + cmd_str = " ".join(cmd_args) raise InitFailedError( "Failed to install pre-commit hook.\n" f"Error running {cmd_str}." diff --git a/commitizen/git.py b/commitizen/git.py index dc2603898..ce9f440c9 100644 --- a/commitizen/git.py +++ b/commitizen/git.py @@ -5,10 +5,14 @@ from functools import lru_cache from pathlib import Path from tempfile import NamedTemporaryFile +from typing import TYPE_CHECKING from commitizen import cmd, out from commitizen.exceptions import GitCommandError +if TYPE_CHECKING: + from collections.abc import Sequence + class EOLType(Enum): """The EOL type from `git config core.eol`.""" @@ -19,7 +23,7 @@ class EOLType(Enum): @classmethod def for_open(cls) -> str: - c = cmd.run("git config core.eol") + c = cmd.run(["git", "config", "core.eol"]) eol = c.out.strip().upper() return cls._char_for_open()[cls._safe_cast(eol)] @@ -164,49 +168,47 @@ def tag( tag: str, annotated: bool = False, signed: bool = False, msg: str | None = None ) -> cmd.Command: if not annotated and not signed: - return cmd.run(f"git tag {tag}") + return cmd.run(["git", "tag", tag]) # according to https://git-scm.com/book/en/v2/Git-Basics-Tagging, # we're not able to create lightweight tag with message. # by adding message, we make it a annotated tags option = "-s" if signed else "-a" # The else case is for annotated tags - return cmd.run(f'git tag {option} {tag} -m "{msg or tag}"') + return cmd.run(["git", "tag", option, tag, "-m", msg or tag]) def add(*args: str) -> cmd.Command: - return cmd.run(f"git add {' '.join(args)}") + return cmd.run(["git", "add", *args]) def commit( message: str, - args: str = "", + args: Sequence[str] = (), committer_date: str | None = None, ) -> cmd.Command: f = NamedTemporaryFile("wb", delete=False) f.write(message.encode("utf-8")) f.close() - command = _create_commit_cmd_string(args, committer_date, f.name) - c = cmd.run(command) - os.unlink(f.name) - return c + cmd_args = ["git", "commit"] + if args: + cmd_args.extend(args) + cmd_args.extend(["-F", f.name]) + env: dict[str, str] | None = None + if committer_date: + env = {"GIT_COMMITTER_DATE": committer_date} -def _create_commit_cmd_string(args: str, committer_date: str | None, name: str) -> str: - command = f'git commit {args} -F "{name}"' - if not committer_date: - return command - if os.name != "nt": - return f"GIT_COMMITTER_DATE={committer_date} {command}" - # Using `cmd /v /c "{command}"` sets environment variables only for that command - return f'cmd /v /c "set GIT_COMMITTER_DATE={committer_date}&& {command}"' + c = cmd.run(cmd_args, env=env) + os.unlink(f.name) + return c def get_commits( start: str | None = None, end: str | None = None, *, - args: str = "", + args: Sequence[str] = (), ) -> list[GitCommit]: """Get the commits between start and end.""" if end is None: @@ -226,7 +228,10 @@ def get_filenames_in_commit(git_reference: str = "") -> list[str]: :returns: file names committed in the last commit by default or inside the passed git reference """ - c = cmd.run(f"git show --name-only --pretty=format: {git_reference}") + cmd_args = ["git", "show", "--name-only", "--pretty=format:"] + if git_reference: + cmd_args.append(git_reference) + c = cmd.run(cmd_args) if c.return_code == 0: return c.out.strip().split("\n") raise GitCommandError(c.err) @@ -237,15 +242,17 @@ def get_tags( ) -> list[GitTag]: inner_delimiter = "---inner_delimiter---" formatter = ( - f'"%(refname:strip=2){inner_delimiter}' + f"%(refname:strip=2){inner_delimiter}" f"%(objectname){inner_delimiter}" f"%(creatordate:format:{dateformat}){inner_delimiter}" - f'%(object)"' + f"%(object)" ) - extra = "--merged" if reachable_only else "" + cmd_args = ["git", "tag", f"--format={formatter}", "--sort=-creatordate"] + if reachable_only: + cmd_args.append("--merged") # Force the default language for parsing env = {"LC_ALL": "C", "LANG": "C", "LANGUAGE": "C"} - c = cmd.run(f"git tag --format={formatter} --sort=-creatordate {extra}", env=env) + c = cmd.run(cmd_args, env=env) if c.return_code != 0: if reachable_only and c.err == "fatal: malformed object name HEAD\n": # this can happen if there are no commits in the repo yet @@ -262,37 +269,37 @@ def get_tags( def tag_exist(tag: str) -> bool: - c = cmd.run(f"git tag --list {tag}") + c = cmd.run(["git", "tag", "--list", tag]) return tag in c.out def is_signed_tag(tag: str) -> bool: - return cmd.run(f"git tag -v {tag}").return_code == 0 + return cmd.run(["git", "tag", "-v", tag]).return_code == 0 def get_latest_tag_name() -> str | None: - c = cmd.run("git describe --abbrev=0 --tags") + c = cmd.run(["git", "describe", "--abbrev=0", "--tags"]) if c.err: return None return c.out.strip() def get_tag_message(tag: str) -> str | None: - c = cmd.run(f"git tag -l --format='%(contents:subject)' {tag}") + c = cmd.run(["git", "tag", "-l", "--format=%(contents:subject)", tag]) if c.err: return None return c.out.strip() def get_tag_names() -> list[str]: - c = cmd.run("git tag --list") + c = cmd.run(["git", "tag", "--list"]) if c.err: return [] return [tag for raw in c.out.split("\n") if (tag := raw.strip())] def find_git_project_root() -> Path | None: - c = cmd.run("git rev-parse --show-toplevel") + c = cmd.run(["git", "rev-parse", "--show-toplevel"]) if c.err: return None return Path(c.out.strip()) @@ -300,17 +307,17 @@ def find_git_project_root() -> Path | None: def is_staging_clean() -> bool: """Check if staging is clean.""" - c = cmd.run("git diff --no-ext-diff --cached --name-only") + c = cmd.run(["git", "diff", "--no-ext-diff", "--cached", "--name-only"]) return not bool(c.out) def is_git_project() -> bool: - c = cmd.run("git rev-parse --is-inside-work-tree") + c = cmd.run(["git", "rev-parse", "--is-inside-work-tree"]) return c.out.strip() == "true" def get_core_editor() -> str | None: - c = cmd.run("git var GIT_EDITOR") + c = cmd.run(["git", "var", "GIT_EDITOR"]) if c.out: return c.out.strip() return None @@ -321,21 +328,31 @@ def smart_open(*args, **kwargs): # type: ignore[no-untyped-def,unused-ignore] # return open(*args, newline=EOLType.for_open(), **kwargs) -def _get_log_as_str_list(start: str | None, end: str, args: str) -> list[str]: +def _get_log_as_str_list(start: str | None, end: str, args: Sequence[str]) -> list[str]: """Get string representation of each log entry""" delimiter = "----------commit-delimiter----------" log_format: str = "%H%n%P%n%s%n%an%n%ae%n%b" command_range = f"{start}..{end}" if start else end - command = f"git -c log.showSignature=False log --pretty={log_format}{delimiter} {args} {command_range}" + cmd_args = [ + "git", + "-c", + "log.showSignature=False", + "log", + f"--pretty={log_format}{delimiter}", + ] + if args: + cmd_args.extend(args) + if command_range: + cmd_args.append(command_range) - c = cmd.run(command) + c = cmd.run(cmd_args) if c.return_code != 0: raise GitCommandError(c.err) return c.out.split(f"{delimiter}\n") def get_default_branch() -> str: - c = cmd.run("git symbolic-ref refs/remotes/origin/HEAD") + c = cmd.run(["git", "symbolic-ref", "refs/remotes/origin/HEAD"]) if c.return_code != 0: raise GitCommandError(c.err) return c.out.strip() diff --git a/commitizen/hooks.py b/commitizen/hooks.py index 10560d5ea..ccb966672 100644 --- a/commitizen/hooks.py +++ b/commitizen/hooks.py @@ -17,7 +17,7 @@ def run(hooks: str | list[str], _env_prefix: str = "CZ_", **env: object) -> None for hook in hooks: out.info(f"Running hook '{hook}'") - c = cmd.run(hook, env=_format_env(_env_prefix, env)) + c = cmd.run_shell(hook, env=_format_env(_env_prefix, env)) if c.out: out.write(c.out) diff --git a/tests/commands/test_bump_command.py b/tests/commands/test_bump_command.py index 021e24291..1e5a162ba 100644 --- a/tests/commands/test_bump_command.py +++ b/tests/commands/test_bump_command.py @@ -2,6 +2,7 @@ import inspect import re +from pathlib import Path from textwrap import dedent from typing import TYPE_CHECKING from unittest.mock import call @@ -28,8 +29,6 @@ ) if TYPE_CHECKING: - from pathlib import Path - from pytest_mock import MockFixture from pytest_regressions.file_regression import FileRegressionFixture @@ -64,7 +63,9 @@ def test_bump_minor_increment(commit_msg: str, util: UtilFixture): assert git.tag_exist("0.2.0") is True assert ( "commit:refs/tags/0.2.0" - in cmd.run('git for-each-ref refs/tags --format "%(objecttype):%(refname)"').out + in cmd.run( + ["git", "for-each-ref", "refs/tags", "--format", "%(objecttype):%(refname)"] + ).out ) @@ -76,7 +77,9 @@ def test_bump_minor_increment_annotated(commit_msg: str, util: UtilFixture): assert git.tag_exist("0.2.0") is True assert ( "tag:refs/tags/0.2.0" - in cmd.run('git for-each-ref refs/tags --format "%(objecttype):%(refname)"').out + in cmd.run( + ["git", "for-each-ref", "refs/tags", "--format", "%(objecttype):%(refname)"] + ).out ) assert git.is_signed_tag("0.2.0") is False @@ -90,7 +93,9 @@ def test_bump_minor_increment_signed(commit_msg: str, util: UtilFixture): assert git.tag_exist("0.2.0") is True assert ( "tag:refs/tags/0.2.0" - in cmd.run('git for-each-ref refs/tags --format "%(objecttype):%(refname)"').out + in cmd.run( + ["git", "for-each-ref", "refs/tags", "--format", "%(objecttype):%(refname)"] + ).out ) assert git.is_signed_tag("0.2.0") is True @@ -107,7 +112,9 @@ def test_bump_minor_increment_annotated_config_file( assert git.tag_exist("0.2.0") is True assert ( "tag:refs/tags/0.2.0" - in cmd.run('git for-each-ref refs/tags --format "%(objecttype):%(refname)"').out + in cmd.run( + ["git", "for-each-ref", "refs/tags", "--format", "%(objecttype):%(refname)"] + ).out ) assert git.is_signed_tag("0.2.0") is False @@ -125,7 +132,9 @@ def test_bump_minor_increment_signed_config_file( assert git.tag_exist("0.2.0") is True assert ( "tag:refs/tags/0.2.0" - in cmd.run('git for-each-ref refs/tags --format "%(objecttype):%(refname)"').out + in cmd.run( + ["git", "for-each-ref", "refs/tags", "--format", "%(objecttype):%(refname)"] + ).out ) assert git.is_signed_tag("0.2.0") is True @@ -283,10 +292,10 @@ def test_bump_command_prerelease_exact_mode(util: UtilFixture): @pytest.mark.usefixtures("tmp_commitizen_project") def test_bump_on_git_with_hooks_no_verify_disabled(util: UtilFixture): """Bump commit without --no-verify""" - cmd.run("mkdir .git/hooks") + Path(".git/hooks").mkdir(parents=True, exist_ok=True) with open(".git/hooks/pre-commit", "w", encoding="utf-8") as f: f.write('#!/usr/bin/env bash\necho "0.1.0"') - cmd.run("chmod +x .git/hooks/pre-commit") + Path(".git/hooks/pre-commit").chmod(0o755) # MINOR util.create_file_and_commit("feat: new file") @@ -296,10 +305,10 @@ def test_bump_on_git_with_hooks_no_verify_disabled(util: UtilFixture): @pytest.mark.usefixtures("tmp_commitizen_project") def test_bump_tag_exists_raises_exception(util: UtilFixture): - cmd.run("mkdir .git/hooks") + Path(".git/hooks").mkdir(parents=True, exist_ok=True) with open(".git/hooks/post-commit", "w", encoding="utf-8") as f: f.write("#!/usr/bin/env bash\nexit 9") - cmd.run("chmod +x .git/hooks/post-commit") + Path(".git/hooks/post-commit").chmod(0o755) # MINOR util.create_file_and_commit("feat: new file") @@ -312,10 +321,10 @@ def test_bump_tag_exists_raises_exception(util: UtilFixture): @pytest.mark.usefixtures("tmp_commitizen_project") def test_bump_on_git_with_hooks_no_verify_enabled(util: UtilFixture): - cmd.run("mkdir .git/hooks") + Path(".git/hooks").mkdir(parents=True, exist_ok=True) with open(".git/hooks/pre-commit", "w", encoding="utf-8") as f: f.write('#!/usr/bin/env bash\necho "0.1.0"') - cmd.run("chmod +x .git/hooks/pre-commit") + Path(".git/hooks/pre-commit").chmod(0o755) # MINOR util.create_file_and_commit("feat: new file") diff --git a/tests/commands/test_commit_command.py b/tests/commands/test_commit_command.py index 2322cb3cb..292530a8d 100644 --- a/tests/commands/test_commit_command.py +++ b/tests/commands/test_commit_command.py @@ -1,10 +1,11 @@ +import sys from pathlib import Path from unittest.mock import ANY import pytest from pytest_mock import MockFixture, MockType -from commitizen import cmd, commands +from commitizen import cli, cmd, commands from commitizen.cz.exceptions import CzException from commitizen.cz.utils import get_backup_file_path from commitizen.exceptions import ( @@ -99,7 +100,7 @@ def test_commit_retry_works( temp_file = commit_cmd.backup_file_path commit_cmd() - commit_mock.assert_called_with("backup commit", args="") + commit_mock.assert_called_with("backup commit", args=[]) prompt_mock.assert_not_called() success_mock.assert_called_once() assert not Path(temp_file).exists() @@ -112,7 +113,7 @@ def test_commit_retry_after_failure_no_backup( config.settings["retry_after_failure"] = True commands.Commit(config, {})() - commit_mock.assert_called_with("feat: user created\n\ncloses #21", args="") + commit_mock.assert_called_with("feat: user created\n\ncloses #21", args=[]) prompt_mock_feat.assert_called_once() success_mock.assert_called_once() @@ -128,7 +129,7 @@ def test_commit_retry_after_failure_works( temp_file = commit_cmd.backup_file_path commit_cmd() - commit_mock.assert_called_with("backup commit", args="") + commit_mock.assert_called_with("backup commit", args=[]) prompt_mock.assert_not_called() success_mock.assert_called_once() assert not Path(temp_file).exists() @@ -143,7 +144,7 @@ def test_commit_retry_after_failure_with_no_retry_works( temp_file = commit_cmd.backup_file_path commit_cmd() - commit_mock.assert_called_with("feat: user created\n\ncloses #21", args="") + commit_mock.assert_called_with("feat: user created\n\ncloses #21", args=[]) prompt_mock_feat.assert_called_once() success_mock.assert_called_once() assert not Path(temp_file).exists() @@ -178,7 +179,7 @@ def test_commit_command_with_signoff_option( ): commands.Commit(config, {"signoff": True})() - commit_mock.assert_called_once_with(ANY, args="-s") + commit_mock.assert_called_once_with(ANY, args=["-s"]) success_mock.assert_called_once() @@ -189,7 +190,7 @@ def test_commit_command_with_always_signoff_enabled( config.settings["always_signoff"] = True commands.Commit(config, {})() - commit_mock.assert_called_once_with(ANY, args="-s") + commit_mock.assert_called_once_with(ANY, args=["-s"]) success_mock.assert_called_once() @@ -198,9 +199,9 @@ def test_commit_command_with_gpgsign_and_always_signoff_enabled( config, success_mock: MockType, commit_mock: MockType ): config.settings["always_signoff"] = True - commands.Commit(config, {"extra_cli_args": "-S"})() + commands.Commit(config, {"extra_cli_args": ["-S"]})() - commit_mock.assert_called_once_with(ANY, args="-S -s") + commit_mock.assert_called_once_with(ANY, args=["-S", "-s"]) success_mock.assert_called_once() @@ -216,9 +217,9 @@ def test_commit_when_nothing_to_commit(config, mocker: MockFixture): @pytest.mark.usefixtures("staging_is_clean", "prompt_mock_feat") def test_commit_with_allow_empty(config, success_mock: MockType, commit_mock: MockType): - commands.Commit(config, {"extra_cli_args": "--allow-empty"})() + commands.Commit(config, {"extra_cli_args": ["--allow-empty"]})() commit_mock.assert_called_with( - "feat: user created\n\ncloses #21", args="--allow-empty" + "feat: user created\n\ncloses #21", args=["--allow-empty"] ) success_mock.assert_called_once() @@ -228,10 +229,10 @@ def test_commit_with_signoff_and_allow_empty( config, success_mock: MockType, commit_mock: MockType ): config.settings["always_signoff"] = True - commands.Commit(config, {"extra_cli_args": "--allow-empty"})() + commands.Commit(config, {"extra_cli_args": ["--allow-empty"]})() commit_mock.assert_called_with( - "feat: user created\n\ncloses #21", args="--allow-empty -s" + "feat: user created\n\ncloses #21", args=["--allow-empty", "-s"] ) success_mock.assert_called_once() @@ -282,11 +283,21 @@ def test_commit_command_with_all_option( def test_commit_command_with_extra_args( config, success_mock: MockType, commit_mock: MockType ): - commands.Commit(config, {"extra_cli_args": "-- -extra-args1 -extra-arg2"})() - commit_mock.assert_called_once_with(ANY, args="-- -extra-args1 -extra-arg2") + commands.Commit(config, {"extra_cli_args": ["--", "-extra-args1", "-extra-arg2"]})() + commit_mock.assert_called_once_with(ANY, args=["--", "-extra-args1", "-extra-arg2"]) success_mock.assert_called_once() +@pytest.mark.usefixtures( + "tmp_commitizen_project", "staging_is_clean", "prompt_mock_feat" +) +def test_commit_cli_extra_args_parsing(mocker: MockFixture, commit_mock: MockType): + """Test that extra args after -- are passed as a list through the CLI.""" + mocker.patch.object(sys, "argv", ["cz", "commit", "--", "--no-verify"]) + cli.main() + commit_mock.assert_called_once_with(ANY, args=["--no-verify"]) + + @pytest.mark.usefixtures("staging_is_clean") @pytest.mark.parametrize("editor", ["vim", None]) def test_manual_edit(editor, config, mocker: MockFixture, tmp_path): diff --git a/tests/conftest.py b/tests/conftest.py index 178b92200..9b29d2a19 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -76,7 +76,7 @@ def chdir(tmp_path: Path) -> Iterator[Path]: @pytest.fixture def tmp_git_project(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): monkeypatch.chdir(tmp_path) - cmd.run("git init") + cmd.run(["git", "init"]) return tmp_path @@ -123,7 +123,7 @@ def _initial( def _get_gpg_keyid(signer_mail): - _new_key = cmd.run(f"gpg --list-secret-keys {signer_mail}") + _new_key = cmd.run(["gpg", "--list-secret-keys", signer_mail]) _m = re.search( r"[a-zA-Z0-9 \[\]-_]*\n[ ]*([0-9A-Za-z]*)\n[\na-zA-Z0-9 \[\]-_<>@]*", _new_key.out, @@ -159,8 +159,8 @@ def tmp_commitizen_project_with_gpg(tmp_commitizen_project): assert key_id # configure git to use gpg signing - cmd.run("git config commit.gpgsign true") - cmd.run(f"git config user.signingkey {key_id}") + cmd.run(["git", "config", "commit.gpgsign", "true"]) + cmd.run(["git", "config", "user.signingkey", key_id]) yield tmp_commitizen_project finally: diff --git a/tests/test_bump_create_commit_message.py b/tests/test_bump_create_commit_message.py index 54b375335..55aa76879 100644 --- a/tests/test_bump_create_commit_message.py +++ b/tests/test_bump_create_commit_message.py @@ -68,9 +68,9 @@ def test_bump_pre_commit_changelog(util: UtilFixture, retry, hook_runner): """ ) ) - cmd.run("git add -A") - cmd.run('git commit -m "fix: _test"') - cmd.run(f"{hook_runner} install") + cmd.run(["git", "add", "-A"]) + cmd.run(["git", "commit", "-m", "fix: _test"]) + cmd.run_shell(f"{hook_runner} install") util.run_cli(*bump_args) # Pre-commit fixed last line adding extra indent and "\" char assert Path("CHANGELOG.md").read_text() == dedent( @@ -106,9 +106,9 @@ def test_bump_pre_commit_changelog_fails_always(util: UtilFixture, retry, hook_r """ ) ) - cmd.run("git add -A") - cmd.run('git commit -m "feat: forbid changelogs"') - cmd.run(f"{hook_runner} install") + cmd.run(["git", "add", "-A"]) + cmd.run(["git", "commit", "-m", "feat: forbid changelogs"]) + cmd.run_shell(f"{hook_runner} install") with pytest.raises(exceptions.BumpCommitFailedError): util.run_cli(*bump_args) @@ -117,8 +117,8 @@ def test_bump_pre_commit_changelog_fails_always(util: UtilFixture, retry, hook_r def test_bump_with_build_metadata(util: UtilFixture): def _add_entry(test_str: str, args: list): Path(test_str).write_text("") - cmd.run("git add -A") - cmd.run(f'git commit -m "fix: test-{test_str}"') + cmd.run(["git", "add", "-A"]) + cmd.run(["git", "commit", "-m", f"fix: test-{test_str}"]) cz_args = ["bump", "--changelog", "--yes"] + args util.run_cli(*cz_args) diff --git a/tests/test_bump_hooks.py b/tests/test_bump_hooks.py index 739d1ce6a..28f0db944 100644 --- a/tests/test_bump_hooks.py +++ b/tests/test_bump_hooks.py @@ -13,7 +13,7 @@ def test_run(mocker: MockFixture): cmd_run_mock = mocker.Mock() cmd_run_mock.return_value.return_code = 0 - mocker.patch.object(cmd, "run", cmd_run_mock) + mocker.patch.object(cmd, "run_shell", cmd_run_mock) hooks.run(bump_hooks) @@ -30,7 +30,7 @@ def test_run_error(mocker: MockFixture): cmd_run_mock = mocker.Mock() cmd_run_mock.return_value.return_code = 1 - mocker.patch.object(cmd, "run", cmd_run_mock) + mocker.patch.object(cmd, "run_shell", cmd_run_mock) with pytest.raises(RunHookError): hooks.run(bump_hooks) @@ -40,3 +40,14 @@ def test_format_env(): result = hooks._format_env("TEST_", {"foo": "bar", "bar": "baz"}) assert result["TEST_FOO"] == "bar" assert result["TEST_BAR"] == "baz" + + +def test_run_integration(): + """Integration test that actually executes hooks.run() without mocking.""" + hooks.run("python -c \"print('hook ran')\"") + + +def test_run_integration_error(): + """Integration test that a failing hook raises RunHookError.""" + with pytest.raises(RunHookError): + hooks.run('python -c "import sys; sys.exit(1)"') diff --git a/tests/test_cmd.py b/tests/test_cmd.py index 77a67cf4e..734c90091 100644 --- a/tests/test_cmd.py +++ b/tests/test_cmd.py @@ -55,3 +55,41 @@ def decode(self, encoding="utf-8", errors="strict"): with pytest.raises(CharacterSetDecodeError): cmd._try_decode(_bytes()) + + +def test_run_returns_command_with_shell_false(): + """Test that cmd.run executes a list-based command without shell.""" + c = cmd.run(["python", "-c", "print('hello')"]) + assert c.return_code == 0 + assert "hello" in c.out + + +def test_run_shell_returns_command_with_shell_true(): + """Test that cmd.run_shell executes a string command via the shell.""" + c = cmd.run_shell("python -c \"print('hello')\"") + assert c.return_code == 0 + assert "hello" in c.out + + +def test_run_with_env(): + """Test that cmd.run passes extra environment variables.""" + c = cmd.run( + ["python", "-c", "import os; print(os.environ['CZ_TEST_VAR'])"], + env={"CZ_TEST_VAR": "test_value"}, + ) + assert c.return_code == 0 + assert "test_value" in c.out + + +def test_run_with_string_emits_deprecation_warning(): + """Test that passing a string to cmd.run() emits a DeprecationWarning.""" + import warnings + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + c = cmd.run("python -c \"print('deprecated')\"") + assert c.return_code == 0 + assert "deprecated" in c.out + assert len(w) == 1 + assert issubclass(w[0].category, DeprecationWarning) + assert "cmd.run()" in str(w[0].message) diff --git a/tests/test_conf.py b/tests/test_conf.py index 94bca4e77..c004e96e1 100644 --- a/tests/test_conf.py +++ b/tests/test_conf.py @@ -320,7 +320,7 @@ def test_warn_same_filename_different_directories_with_git( ): """Test warning when same config filename exists in the current directory and in the git root.""" monkeypatch.chdir(tmp_path) - cmd.run("git init") + cmd.run(["git", "init"]) # Create config in git root (tmp_path / config_file).write_text(content) @@ -378,7 +378,7 @@ def test_no_warn_with_single_config_file( """Test that no warning is issued when user explicitly specifies config.""" monkeypatch.chdir(tmp_path) if with_git: - cmd.run("git init") + cmd.run(["git", "init"]) (tmp_path / config_file).write_text(content) diff --git a/tests/test_git.py b/tests/test_git.py index 59098528a..51586eb22 100644 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -2,7 +2,7 @@ import inspect import os -import platform +from pathlib import Path from typing import TYPE_CHECKING import pytest @@ -108,7 +108,7 @@ def test_get_log_as_str_list_empty(): The behavior is different depending on the version of git. """ try: - gitlog = git._get_log_as_str_list(start=None, end="HEAD", args="") + gitlog = git._get_log_as_str_list(start=None, end="HEAD", args=[]) except GitCommandError: return assert len(gitlog) == 0, "list should be empty if no assert" @@ -284,11 +284,11 @@ def test_get_latest_tag_name(util: UtilFixture): def test_is_staging_clean_when_adding_file(): assert git.is_staging_clean() is True - cmd.run("touch test_file") + Path("test_file").touch() assert git.is_staging_clean() is True - cmd.run("git add test_file") + cmd.run(["git", "add", "test_file"]) assert git.is_staging_clean() is False @@ -297,17 +297,14 @@ def test_is_staging_clean_when_adding_file(): def test_is_staging_clean_when_updating_file(): assert git.is_staging_clean() is True - cmd.run("touch test_file") - cmd.run("git add test_file") - if os.name == "nt": - cmd.run('git commit -m "add test_file"') - else: - cmd.run("git commit -m 'add test_file'") - cmd.run("echo 'test' > test_file") + Path("test_file").touch() + cmd.run(["git", "add", "test_file"]) + cmd.run(["git", "commit", "-m", "add test_file"]) + Path("test_file").write_text("test") assert git.is_staging_clean() is True - cmd.run("git add test_file") + cmd.run(["git", "add", "test_file"]) assert git.is_staging_clean() is False @@ -316,13 +313,13 @@ def test_is_staging_clean_when_updating_file(): def test_get_eol_for_open(): assert git.EOLType.for_open() == os.linesep - cmd.run("git config core.eol lf") + cmd.run(["git", "config", "core.eol", "lf"]) assert git.EOLType.for_open() == "\n" - cmd.run("git config core.eol crlf") + cmd.run(["git", "config", "core.eol", "crlf"]) assert git.EOLType.for_open() == "\r\n" - cmd.run("git config core.eol native") + cmd.run(["git", "config", "core.eol", "native"]) assert git.EOLType.for_open() == os.linesep @@ -346,9 +343,7 @@ def test_create_tag_with_message(util: UtilFixture): tag_message = "test message" util.create_tag(tag_name, tag_message) assert git.get_latest_tag_name() == tag_name - assert git.get_tag_message(tag_name) == ( - tag_message if platform.system() != "Windows" else f"'{tag_message}'" - ) + assert git.get_tag_message(tag_name) == tag_message @pytest.mark.parametrize( @@ -356,15 +351,15 @@ def test_create_tag_with_message(util: UtilFixture): [ ( "/tmp/temp file", - 'git commit --signoff -F "/tmp/temp file"', + ["git", "commit", "--signoff", "-F", "/tmp/temp file"], ), ( "/tmp dir/temp file", - 'git commit --signoff -F "/tmp dir/temp file"', + ["git", "commit", "--signoff", "-F", "/tmp dir/temp file"], ), ( "/tmp/tempfile", - 'git commit --signoff -F "/tmp/tempfile"', + ["git", "commit", "--signoff", "-F", "/tmp/tempfile"], ), ], ids=[ @@ -374,16 +369,16 @@ def test_create_tag_with_message(util: UtilFixture): ], ) def test_commit_with_spaces_in_path( - mocker: MockFixture, file_path: str, expected_cmd: str, util: UtilFixture + mocker: MockFixture, file_path: str, expected_cmd: list[str], util: UtilFixture ): mock_run = util.mock_cmd() mock_unlink = mocker.patch("os.unlink") mock_temp_file = mocker.patch("commitizen.git.NamedTemporaryFile") mock_temp_file.return_value.name = file_path - git.commit("feat: new feature", "--signoff") + git.commit("feat: new feature", ["--signoff"]) - mock_run.assert_called_once_with(expected_cmd) + mock_run.assert_called_once_with(expected_cmd, env=None) mock_unlink.assert_called_once_with(file_path) @@ -404,7 +399,7 @@ def test_get_filenames_in_commit_with_git_reference(util: UtilFixture): """Test get_filenames_in_commit with a specific git reference (commit SHA).""" first_filename = "first_feature.txt" util.create_file_and_commit("feat: first feature", filename=first_filename) - first_commit_rev = cmd.run("git rev-parse HEAD").out.strip() + first_commit_rev = cmd.run(["git", "rev-parse", "HEAD"]).out.strip() second_filename = "second_feature.txt" util.create_file_and_commit("feat: second feature", filename=second_filename) @@ -474,29 +469,41 @@ def test_git_commit_from_rev_and_commit(linebreak): @pytest.mark.parametrize( - ("os_name", "committer_date", "expected_cmd"), + "committer_date", [ - ( - "nt", - "2024-03-20", - 'cmd /v /c "set GIT_COMMITTER_DATE=2024-03-20&& git commit -F "temp.txt""', - ), - ( - "posix", - "2024-03-20", - 'GIT_COMMITTER_DATE=2024-03-20 git commit -F "temp.txt"', - ), - ("nt", None, 'git commit -F "temp.txt"'), - ("posix", None, 'git commit -F "temp.txt"'), + "2024-03-20", + None, ], ) -def test_create_commit_cmd_string( - mocker: MockFixture, os_name: str, committer_date: str, expected_cmd: str -): - """Test the OS-specific behavior of _create_commit_cmd_string""" - mocker.patch("os.name", os_name) - result = git._create_commit_cmd_string("", committer_date, "temp.txt") - assert result == expected_cmd +def test_commit_uses_list_args_and_env(mocker: MockFixture, committer_date: str | None): + """Test that git.commit uses list-based subprocess (no shell injection).""" + mock_run = mocker.patch("commitizen.cmd.run") + mock_run.return_value = cmd.Command("", "", b"", b"", 0) + + # We need to mock NamedTemporaryFile to control the file name + mock_file = mocker.MagicMock() + mock_file.name = "temp.txt" + mock_file.__enter__ = mocker.MagicMock(return_value=mock_file) + mock_file.__exit__ = mocker.MagicMock(return_value=False) + mocker.patch("commitizen.git.NamedTemporaryFile", return_value=mock_file) + mocker.patch("os.unlink") + + git.commit( + "test message", args=["-a", "--no-verify"], committer_date=committer_date + ) + + call_args = mock_run.call_args + # First positional arg should be a list (no shell injection) + cmd_list = call_args[0][0] + assert isinstance(cmd_list, list) + assert cmd_list == ["git", "commit", "-a", "--no-verify", "-F", "temp.txt"] + + if committer_date: + env = call_args[1]["env"] + assert env == {"GIT_COMMITTER_DATE": committer_date} + else: + env = call_args[1]["env"] + assert env is None def test_get_default_branch_success(util: UtilFixture): diff --git a/tests/utils.py b/tests/utils.py index bca565f78..77554c797 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -55,7 +55,7 @@ def create_file_and_commit( filename = str(uuid.uuid4()) Path(filename).touch() - c = cmd.run("git add .") + c = cmd.run(["git", "add", "."]) if c.return_code != 0: raise exceptions.CommitError(c.err) c = git.commit(message, committer_date=committer_date) @@ -65,29 +65,29 @@ def create_file_and_commit( def create_branch(self, name: str) -> None: """Create a new branch.""" - c = cmd.run(f"git branch {name}") + c = cmd.run(["git", "branch", name]) if c.return_code != 0: raise exceptions.GitCommandError(c.err) def switch_branch(self, branch: str) -> None: """Switch to given branch.""" - c = cmd.run(f"git switch {branch}") + c = cmd.run(["git", "switch", branch]) if c.return_code != 0: raise exceptions.GitCommandError(c.err) def merge_branch(self, branch: str) -> None: """Merge given branch into current branch.""" - c = cmd.run(f"git merge {branch}") + c = cmd.run(["git", "merge", branch]) if c.return_code != 0: raise exceptions.GitCommandError(c.err) self.tick() def get_current_branch(self) -> str: """Get current git branch name.""" - c = cmd.run("git rev-parse --abbrev-ref HEAD") + c = cmd.run(["git", "rev-parse", "--abbrev-ref", "HEAD"]) if c.return_code != 0: raise exceptions.GitCommandError(c.err) - return c.out + return c.out.strip() def create_tag( self, tag: str, message: str | None = None, annotated: bool | None = None