Skip to content

Commit b6819c7

Browse files
authored
fix: print commit text in the step log inert (#312)
Co-authored-by: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com>
1 parent fbb0fa1 commit b6819c7

2 files changed

Lines changed: 81 additions & 4 deletions

File tree

‎main.py‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,7 @@ def _relay_cli_notices(text: str) -> None:
578578
line = line.strip()
579579
if line and line not in _RELAYED_NOTICES:
580580
_RELAYED_NOTICES.add(line)
581-
print(f"commit-check: {line}", file=sys.stderr)
581+
print(f"commit-check: {_log_safe(line)}", file=sys.stderr)
582582

583583

584584
def run_check_json(
@@ -905,6 +905,39 @@ def _annotation_escape(text: str) -> str:
905905
return text.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
906906

907907

908+
#: C0 and C1 control characters, and DEL, except the newline and tab that a
909+
#: multi-line commit message legitimately carries.
910+
_CONTROL_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f-\x9f]")
911+
912+
913+
def _log_safe(text: str) -> str:
914+
"""Make commit-controlled text inert before it is printed to the step log.
915+
916+
The tree prints values from the CLI's JSON, and those are whoever wrote
917+
the commit, branch or pull request title: a multi-line value (a whole
918+
message on a failing rule, a sign-off fix) prints each further line on a
919+
line of its own. The runner reads any log line beginning with ``::`` as a
920+
workflow command, after skipping leading whitespace, so indenting it is
921+
not enough; and it reads the older ``##[command]`` form anywhere in a
922+
line. Left as they are, such lines can forge annotations or mask log
923+
text. So such a line gets a backslash before its ``::``, ``##[`` is broken
924+
up, and control characters are shown as ``\\xNN`` escapes. This is the
925+
CLI's own ``printable()`` (commit-check#620), which covers its text
926+
output but not the JSON this action renders itself.
927+
928+
Only the step log needs it: the Markdown report and the annotation
929+
payloads are not read as commands.
930+
"""
931+
if text.isprintable() and "::" not in text and "##[" not in text:
932+
return text # nothing to escape: the common case
933+
text = _CONTROL_CHARS.sub(lambda m: f"\\x{ord(m.group()):02x}", text)
934+
text = text.replace("##[", "#\\#[")
935+
return "\n".join(
936+
line.replace("::", "\\::", 1) if line.lstrip().startswith("::") else line
937+
for line in text.split("\n")
938+
)
939+
940+
908941
def _annotation_body(scope: ScopeResult, check: dict[str, str]) -> str:
909942
"""The message of one finding's annotation, before escaping.
910943
@@ -949,7 +982,7 @@ def render_step_log(results: list[ScopeResult]) -> None:
949982
for group_name, scopes in _grouped(results):
950983
print(f"::group::{group_name}")
951984
for line in _render_scopes(scopes, include_docs=True):
952-
print(line)
985+
print(_log_safe(line))
953986
print("::endgroup::")
954987

955988
errors: list[tuple[str, str]] = []
@@ -1004,7 +1037,7 @@ def render_step_log(results: list[ScopeResult]) -> None:
10041037
if total and skipped == total:
10051038
print("\u2298 commit-check: all checks skipped, nothing was validated")
10061039
if _skip_reasons(results):
1007-
print(_skip_reasons(results))
1040+
print(_log_safe(_skip_reasons(results)))
10081041
elif warned or skipped:
10091042
passed = total - skipped - warned
10101043
tail = []
@@ -1016,7 +1049,7 @@ def render_step_log(results: list[ScopeResult]) -> None:
10161049
f"\u2714 commit-check: {passed} of {total} checks passed, {', '.join(tail)}"
10171050
)
10181051
if _skip_reasons(results):
1019-
print(_skip_reasons(results))
1052+
print(_log_safe(_skip_reasons(results)))
10201053
else:
10211054
print("\u2714 commit-check: all checks passed")
10221055

‎main_test.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1334,6 +1334,50 @@ def test_failure_prints_group_and_error_annotation(self):
13341334
output,
13351335
)
13361336

1337+
def test_commit_text_cannot_issue_workflow_commands(self):
1338+
# A failing rule prints the whole message, one line of the log per
1339+
# line of the message. The runner skips leading whitespace before
1340+
# looking for "::", and reads "##[" anywhere in a line.
1341+
scope = main.ScopeResult(
1342+
label="Commit 1/1",
1343+
checks=[
1344+
make_check(
1345+
"message",
1346+
status="fail",
1347+
rule_id="CC001",
1348+
value=(
1349+
"bad subject\n\n"
1350+
"::warning title=Forged::text\n"
1351+
" ::add-mask::secret\n"
1352+
"old ##[error]form\n"
1353+
"esc\x1b]52;c;eA==\x07"
1354+
),
1355+
error="The commit message should follow Conventional Commits.",
1356+
)
1357+
],
1358+
)
1359+
output = self._run([scope])
1360+
tree = output.split("::endgroup::")[0]
1361+
for line in tree.splitlines()[1:]: # past ::group::, our own command
1362+
self.assertFalse(line.lstrip().startswith("::"), line)
1363+
self.assertNotIn("##[", line)
1364+
self.assertNotIn("\x1b", line)
1365+
self.assertIn(" \\::warning title=Forged::text\n", tree)
1366+
self.assertIn(" \\::add-mask::secret\n", tree)
1367+
self.assertIn(" old #\\#[error]form\n", tree)
1368+
self.assertIn(" esc\\x1b]52;c;eA==\\x07\n", tree)
1369+
# The annotations are commands already; their payload is left as is.
1370+
self.assertIn("%0A::warning title=Forged::text%0A", output)
1371+
1372+
def test_log_safe_leaves_ordinary_text_alone(self):
1373+
for text in [
1374+
"feat: add login",
1375+
"value: a::b",
1376+
" ✔ Branch (feature/x)",
1377+
"tab\there",
1378+
]:
1379+
self.assertEqual(main._log_safe(text), text)
1380+
13371381
def test_annotation_names_the_commit_and_carries_the_fix(self):
13381382
output = self._run([fix_scope("Commit 2/3", sha=SHA_B)])
13391383
self.assertIn(

0 commit comments

Comments
 (0)