diff --git a/main.py b/main.py index 5961d2f..73c6b29 100755 --- a/main.py +++ b/main.py @@ -578,7 +578,7 @@ def _relay_cli_notices(text: str) -> None: line = line.strip() if line and line not in _RELAYED_NOTICES: _RELAYED_NOTICES.add(line) - print(f"commit-check: {line}", file=sys.stderr) + print(f"commit-check: {_log_safe(line)}", file=sys.stderr) def run_check_json( @@ -905,6 +905,39 @@ def _annotation_escape(text: str) -> str: return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") +#: C0 and C1 control characters, and DEL, except the newline and tab that a +#: multi-line commit message legitimately carries. +_CONTROL_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f]") + + +def _log_safe(text: str) -> str: + """Make commit-controlled text inert before it is printed to the step log. + + The tree prints values from the CLI's JSON, and those are whoever wrote + the commit, branch or pull request title: a multi-line value (a whole + message on a failing rule, a sign-off fix) prints each further line on a + line of its own. The runner reads any log line beginning with ``::`` as a + workflow command, after skipping leading whitespace, so indenting it is + not enough; and it reads the older ``##[command]`` form anywhere in a + line. Left as they are, such lines can forge annotations or mask log + text. So such a line gets a backslash before its ``::``, ``##[`` is broken + up, and control characters are shown as ``\\xNN`` escapes. This is the + CLI's own ``printable()`` (commit-check#620), which covers its text + output but not the JSON this action renders itself. + + Only the step log needs it: the Markdown report and the annotation + payloads are not read as commands. + """ + if text.isprintable() and "::" not in text and "##[" not in text: + return text # nothing to escape: the common case + text = _CONTROL_CHARS.sub(lambda m: f"\\x{ord(m.group()):02x}", text) + text = text.replace("##[", "#\\#[") + return "\n".join( + line.replace("::", "\\::", 1) if line.lstrip().startswith("::") else line + for line in text.split("\n") + ) + + def _annotation_body(scope: ScopeResult, check: dict[str, str]) -> str: """The message of one finding's annotation, before escaping. @@ -949,7 +982,7 @@ def render_step_log(results: list[ScopeResult]) -> None: for group_name, scopes in _grouped(results): print(f"::group::{group_name}") for line in _render_scopes(scopes, include_docs=True): - print(line) + print(_log_safe(line)) print("::endgroup::") errors: list[tuple[str, str]] = [] @@ -1004,7 +1037,7 @@ def render_step_log(results: list[ScopeResult]) -> None: if total and skipped == total: print("\u2298 commit-check: all checks skipped, nothing was validated") if _skip_reasons(results): - print(_skip_reasons(results)) + print(_log_safe(_skip_reasons(results))) elif warned or skipped: passed = total - skipped - warned tail = [] @@ -1016,7 +1049,7 @@ def render_step_log(results: list[ScopeResult]) -> None: f"\u2714 commit-check: {passed} of {total} checks passed, {', '.join(tail)}" ) if _skip_reasons(results): - print(_skip_reasons(results)) + print(_log_safe(_skip_reasons(results))) else: print("\u2714 commit-check: all checks passed") diff --git a/main_test.py b/main_test.py index c43307f..8a5c507 100644 --- a/main_test.py +++ b/main_test.py @@ -1334,6 +1334,50 @@ def test_failure_prints_group_and_error_annotation(self): output, ) + def test_commit_text_cannot_issue_workflow_commands(self): + # A failing rule prints the whole message, one line of the log per + # line of the message. The runner skips leading whitespace before + # looking for "::", and reads "##[" anywhere in a line. + scope = main.ScopeResult( + label="Commit 1/1", + checks=[ + make_check( + "message", + status="fail", + rule_id="CC001", + value=( + "bad subject\n\n" + "::warning title=Forged::text\n" + " ::add-mask::secret\n" + "old ##[error]form\n" + "esc\x1b]52;c;eA==\x07" + ), + error="The commit message should follow Conventional Commits.", + ) + ], + ) + output = self._run([scope]) + tree = output.split("::endgroup::")[0] + for line in tree.splitlines()[1:]: # past ::group::, our own command + self.assertFalse(line.lstrip().startswith("::"), line) + self.assertNotIn("##[", line) + self.assertNotIn("\x1b", line) + self.assertIn(" \\::warning title=Forged::text\n", tree) + self.assertIn(" \\::add-mask::secret\n", tree) + self.assertIn(" old #\\#[error]form\n", tree) + self.assertIn(" esc\\x1b]52;c;eA==\\x07\n", tree) + # The annotations are commands already; their payload is left as is. + self.assertIn("%0A::warning title=Forged::text%0A", output) + + def test_log_safe_leaves_ordinary_text_alone(self): + for text in [ + "feat: add login", + "value: a::b", + " ✔ Branch (feature/x)", + "tab\there", + ]: + self.assertEqual(main._log_safe(text), text) + def test_annotation_names_the_commit_and_carries_the_fix(self): output = self._run([fix_scope("Commit 2/3", sha=SHA_B)]) self.assertIn(