From 2a3213e239036bf39e345fba0737e041add20f14 Mon Sep 17 00:00:00 2001 From: Tim Hsiung <26526132+bearomorphism@users.noreply.github.com> Date: Sat, 30 May 2026 11:30:01 +0800 Subject: [PATCH] fix(check): expand env vars in --rev-range The packaged commitizen-branch pre-push hook in .pre-commit-hooks.yaml passes the literal string \..\ as an argv element and relied on shell expansion. After #1941 switched git execution to shell=False (CWE-78 hardening), git received the literal string and aborted with atal: ambiguous argument, breaking every commitizen release after v4.15.0 for users of that hook. Expand env vars explicitly on the --rev-range argument via os.path.expandvars so the hook keeps working without reintroducing shell execution. Unset variables are left literal so git surfaces a clear error instead of being silently rewritten to an empty range. Closes #2003 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- commitizen/commands/check.py | 7 ++- tests/commands/test_check_command.py | 92 +++++++++++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/commitizen/commands/check.py b/commitizen/commands/check.py index ab5e671d6..fbf707341 100644 --- a/commitizen/commands/check.py +++ b/commitizen/commands/check.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import re import sys from pathlib import Path @@ -40,7 +41,11 @@ def __init__(self, config: BaseConfig, arguments: CheckArgs, *args: object) -> N """ self.commit_msg_file = arguments.get("commit_msg_file") self.commit_msg = arguments.get("message") - self.rev_range = arguments.get("rev_range") + rev_range = arguments.get("rev_range") + # Expand env vars so the packaged ``commitizen-branch`` pre-push hook + # (which passes the literal ``$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF``) + # keeps working after the ``shell=False`` switch in #1941. See #2003. + self.rev_range = os.path.expandvars(rev_range) if rev_range else rev_range self.allow_abort = bool( arguments.get("allow_abort", config.settings["allow_abort"]) ) diff --git a/tests/commands/test_check_command.py b/tests/commands/test_check_command.py index d587fc070..227fa5fa5 100644 --- a/tests/commands/test_check_command.py +++ b/tests/commands/test_check_command.py @@ -5,7 +5,7 @@ import pytest -from commitizen import commands, git +from commitizen import cmd, commands, git from commitizen.cz import registry from commitizen.cz.base import BaseCommitizen from commitizen.exceptions import ( @@ -172,6 +172,53 @@ def test_check_a_range_of_git_commits_and_failed(config, mocker: MockFixture): commands.Check(config=config, arguments={"rev_range": "HEAD~10..master"})() +def test_check_rev_range_expands_env_vars( + config, success_mock: MockType, mocker: MockFixture, monkeypatch: pytest.MonkeyPatch +): + """The ``commitizen-branch`` pre-push hook passes the literal string + ``$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF`` and relies on env-var + expansion. Regression test for + https://github.com/commitizen-tools/commitizen/issues/2003. + """ + monkeypatch.setenv("PRE_COMMIT_FROM_REF", "abc123") + monkeypatch.setenv("PRE_COMMIT_TO_REF", "def456") + get_commits = mocker.patch( + "commitizen.git.get_commits", + return_value=_build_fake_git_commits(COMMIT_LOG), + ) + + commands.Check( + config=config, + arguments={"rev_range": "$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF"}, + )() + + success_mock.assert_called_once() + get_commits.assert_called_once_with(None, "abc123..def456") + + +def test_check_rev_range_leaves_unset_env_vars_literal( + config, mocker: MockFixture, monkeypatch: pytest.MonkeyPatch +): + """Unset env-var references should pass through unchanged so git can + surface a clear ``ambiguous argument`` error instead of being silently + rewritten to an empty range.""" + monkeypatch.delenv("PRE_COMMIT_FROM_REF", raising=False) + monkeypatch.delenv("PRE_COMMIT_TO_REF", raising=False) + get_commits = mocker.patch( + "commitizen.git.get_commits", + return_value=_build_fake_git_commits(COMMIT_LOG), + ) + + commands.Check( + config=config, + arguments={"rev_range": "$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF"}, + )() + + get_commits.assert_called_once_with( + None, "$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF" + ) + + def test_check_command_with_invalid_argument(config): with pytest.raises( InvalidCommandArgumentError, @@ -193,6 +240,49 @@ def test_check_command_with_empty_range(config: BaseConfig, util: UtilFixture): commands.Check(config=config, arguments={"rev_range": "master..master"})() +@pytest.mark.usefixtures("tmp_commitizen_project") +def test_check_rev_range_pre_commit_branch_hook_regression( + config: BaseConfig, + util: UtilFixture, + capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, +): + """End-to-end regression test for the packaged ``commitizen-branch`` + pre-push hook. + + The hook in ``.pre-commit-hooks.yaml`` runs:: + + cz check --rev-range "$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF" + + ``pre-commit`` exports those refs as environment variables but does + *not* expand them in argv, so the literal string reaches ``cz check``. + Before this fix, ``Check`` forwarded that literal to ``git log`` via + ``shell=False`` (PR #1941, CWE-78 hardening) and git aborted with + ``fatal: ambiguous argument '$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF'``. + + This test exercises the real subprocess path -- no mocks on + ``git.get_commits`` -- to guard against any future regression that + bypasses env-var expansion in the rev-range argument. + + See https://github.com/commitizen-tools/commitizen/issues/2003. + """ + util.create_file_and_commit("feat: initial") + util.create_file_and_commit("fix: second commit") + + from_ref = cmd.run(["git", "rev-parse", "HEAD~1"]).out.strip() + to_ref = cmd.run(["git", "rev-parse", "HEAD"]).out.strip() + monkeypatch.setenv("PRE_COMMIT_FROM_REF", from_ref) + monkeypatch.setenv("PRE_COMMIT_TO_REF", to_ref) + + commands.Check( + config=config, + arguments={"rev_range": "$PRE_COMMIT_FROM_REF..$PRE_COMMIT_TO_REF"}, + )() + + captured = capsys.readouterr() + assert "Commit validation: successful!" in captured.out + + def test_check_a_range_of_failed_git_commits(config, mocker: MockFixture): ill_formatted_commits_msgs = [ "First commit does not follow rule",