Skip to content

Commit 73fd34f

Browse files
committed
fix: never run the checkout's pip.py, reject non-boolean inputs, keep to own comments
- Run venv and pip with python -I, so a pip.py or venv.py at the repository root no longer runs in their place with GITHUB_TOKEN. The self-test plants both files to keep it that way. - A boolean input that is neither true nor false (yes, on, a typo) now fails the step with the input's name; it used to turn the check off silently. - Only edit or delete a marked comment when a bot posted it. - Send the PR comment to GITHUB_API_URL, as the other API calls do, so it works on GitHub Enterprise Server.
1 parent dd5f113 commit 73fd34f

4 files changed

Lines changed: 141 additions & 10 deletions

File tree

‎.github/workflows/commit-check.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,14 @@ jobs:
2424
fetch-depth: 0 # Required for merge-base checks
2525
# Nothing after checkout needs authenticated git.
2626
persist-credentials: false
27+
# The install step must never run a pip.py or venv.py from the
28+
# checkout in place of pip or venv. If it did, these stop the job.
29+
- name: Plant pip.py and venv.py in the workspace
30+
shell: bash
31+
run: |
32+
for name in pip venv; do
33+
echo 'raise SystemExit("a workspace module ran in place of pip or venv")' > "$name.py"
34+
done
2735
- uses: ./ # self test
2836
with:
2937
message: true

‎action.yml‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ runs:
7373
7474
# The pinned commit-check needs Python 3.11+. Say so here: on an older
7575
# python3, pip would only report that no matching version exists.
76-
if ! $PYTHON_CMD -c 'import sys; sys.exit(sys.version_info < (3, 11))'; then
76+
if ! $PYTHON_CMD -I -c 'import sys; sys.exit(sys.version_info < (3, 11))'; then
7777
echo "::error::commit-check-action needs Python 3.11 or newer on PATH, found $($PYTHON_CMD --version 2>&1). Add actions/setup-python with python-version 3.11 or newer before this step."
7878
exit 1
7979
fi
@@ -84,7 +84,11 @@ runs:
8484
WORK="$RUNNER_TEMP/commit-check-action"
8585
rm -rf "$WORK" # a second invocation in the same job starts clean, so the wheel glob below matches exactly one file
8686
mkdir -p "$WORK/wheels"
87-
$PYTHON_CMD -m venv "$WORK/venv"
87+
# -I (isolated mode) everywhere below: `python -m` otherwise puts the
88+
# current directory, the caller's checkout, first on sys.path, so a
89+
# pip.py or venv.py at the repository root would run in place of pip
90+
# or venv, with this step's GITHUB_TOKEN.
91+
$PYTHON_CMD -I -m venv "$WORK/venv"
8892
if [[ "$RUNNER_OS" == "Windows" ]]; then
8993
source "$WORK/venv/Scripts/activate"
9094
else
@@ -94,7 +98,7 @@ runs:
9498
# One download of the pinned closure, into the scratch dir. Wheels only:
9599
# the offline install below cannot build an sdist, so a platform without
96100
# binary wheels fails here, with pip's message, rather than later.
97-
$PYTHON_CMD -m pip download -q --only-binary=:all: -d "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt"
101+
$PYTHON_CMD -I -m pip download -q --only-binary=:all: -d "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt"
98102
99103
# Verify the commit-check wheel's build provenance (PyGithub and the
100104
# transitive wheels are pinned but not attested; see README "Runner requirements").
@@ -106,7 +110,7 @@ runs:
106110
107111
# Install offline from the wheels we already downloaded and verified:
108112
# no second trip to PyPI, and the installed set is exactly the downloaded set.
109-
$PYTHON_CMD -m pip install -q --no-cache-dir --no-index --find-links "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt"
113+
$PYTHON_CMD -I -m pip install -q --no-cache-dir --no-index --find-links "$WORK/wheels" -r "$GITHUB_ACTION_PATH/requirements.txt"
110114
111115
$PYTHON_CMD "$GITHUB_ACTION_PATH/main.py"
112116
env:

‎main.py‎

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,22 @@
6666
}
6767

6868

69+
#: Boolean inputs whose value was neither true nor false, as ``(name, value)``.
70+
#: ``main`` fails the step on them before any check runs.
71+
INVALID_FLAGS: list[tuple[str, str]] = []
72+
73+
6974
def env_flag(name: str, default: str = "false") -> bool:
70-
"""Read a GitHub Action boolean-style environment variable."""
71-
return os.getenv(name, default).lower() == "true"
75+
"""Read a GitHub Action boolean input: ``true`` or ``false``, any case.
76+
77+
Anything else (``yes``, ``on``, a typo) used to read as false and turn
78+
the check off without a word. It is now recorded in ``INVALID_FLAGS``,
79+
and the step fails with the input's name.
80+
"""
81+
value = os.getenv(name, default).strip().lower()
82+
if value not in ("true", "false"):
83+
INVALID_FLAGS.append((name, os.getenv(name, default)))
84+
return value == "true"
7285

7386

7487
def _reconfigure_io() -> None:
@@ -1809,14 +1822,15 @@ def _is_bot(comment: Any) -> bool:
18091822
def _find_own_comments(comments: list[Any]) -> tuple[Any | None, list[Any]]:
18101823
"""Pick the comment to update and the ones to delete.
18111824
1812-
Returns ``(target, stale)``. Only comments carrying ``COMMENT_MARKER`` are
1813-
ever deleted — those are unambiguously ours. A comment from an earlier
1825+
Returns ``(target, stale)``. Only comments a bot posted with
1826+
``COMMENT_MARKER`` in them are ever edited or deleted: a person can paste
1827+
the marker too (quoting a report, say), and their comment is theirs. A comment from an earlier
18141828
version has no marker, so it is adopted (edited, which adds the marker)
18151829
when there is no marked comment yet, and only if a bot posted it: the
18161830
legacy signal is a title prefix, which a person can type by accident, and
18171831
editing someone's comment out from under them is not recoverable.
18181832
"""
1819-
marked = [c for c in comments if COMMENT_MARKER in c.body]
1833+
marked = [c for c in comments if COMMENT_MARKER in c.body and _is_bot(c)]
18201834
if marked:
18211835
return marked[-1], marked[:-1]
18221836

@@ -1887,7 +1901,8 @@ def add_pr_comments(results: list[ScopeResult]) -> int:
18871901
if not repo_name:
18881902
raise ValueError("GITHUB_REPOSITORY is not set")
18891903

1890-
g = Github(auth=Auth.Token(token))
1904+
api_url = os.getenv("GITHUB_API_URL", "https://api.github.com")
1905+
g = Github(auth=Auth.Token(token), base_url=api_url)
18911906
repo = g.get_repo(repo_name)
18921907
pull_request = repo.get_issue(pr_number)
18931908

@@ -2033,9 +2048,23 @@ def log_error_and_exit(ret_code: int) -> None:
20332048
sys.exit(ret_code)
20342049

20352050

2051+
def fail_on_invalid_flags() -> None:
2052+
"""Stop with an error when a boolean input is neither true nor false."""
2053+
if not INVALID_FLAGS:
2054+
return
2055+
for name, value in INVALID_FLAGS:
2056+
input_name = name.lower().replace("_", "-")
2057+
print(
2058+
f"::error::Input {input_name} must be true or false, got "
2059+
f"{_log_safe(repr(value))}"
2060+
)
2061+
sys.exit(1)
2062+
2063+
20362064
def main():
20372065
"""Main function to run commit-check and render all output surfaces."""
20382066
_reconfigure_io()
2067+
fail_on_invalid_flags()
20392068
log_env_vars()
20402069

20412070
ret_code, results = run_commit_check()

‎main_test.py‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2455,6 +2455,7 @@ def test_updates_existing_comment_when_changed(self):
24552455
def test_skips_when_comment_is_up_to_date(self):
24562456
body = main.render_pr_comment([fail_scope()])
24572457
existing = MagicMock(body=body)
2458+
existing.user.type = "Bot"
24582459
mock_pull_request = MagicMock()
24592460
mock_pull_request.get_comments.return_value = [existing]
24602461
mock_repo = MagicMock()
@@ -2904,6 +2905,14 @@ def test_bot_comment_from_an_older_version_is_adopted(self):
29042905
self.assertIs(target, legacy)
29052906
self.assertEqual(stale, [])
29062907

2908+
def test_human_comment_with_the_marker_is_never_touched(self):
2909+
"""A person can paste the marker too, e.g. quoting a report."""
2910+
quoted = self._comment(f"{main.COMMENT_MARKER}\nquoted", user_type="User")
2911+
mine = self._comment(f"{main.COMMENT_MARKER}\nreport")
2912+
self.assertEqual(main._find_own_comments([quoted, mine]), (mine, []))
2913+
self.assertEqual(main._find_own_comments([mine, quoted]), (mine, []))
2914+
self.assertEqual(main._find_own_comments([quoted]), (None, []))
2915+
29072916
def test_no_comments_yields_nothing_to_update(self):
29082917
target, stale = main._find_own_comments([])
29092918
self.assertIsNone(target)
@@ -4096,3 +4105,84 @@ def test_a_missing_pygithub_warns(self):
40964105

40974106
if __name__ == "__main__":
40984107
unittest.main()
4108+
4109+
4110+
class TestBooleanInputs(unittest.TestCase):
4111+
"""A boolean input is true or false; anything else fails the step."""
4112+
4113+
def setUp(self):
4114+
main.INVALID_FLAGS.clear()
4115+
self.addCleanup(main.INVALID_FLAGS.clear)
4116+
4117+
def test_true_and_false_in_any_case_and_padding(self):
4118+
for value, expected in (
4119+
("true", True),
4120+
(" True ", True),
4121+
("FALSE", False),
4122+
("false", False),
4123+
):
4124+
with patch.dict(os.environ, {"MESSAGE": value}):
4125+
self.assertIs(main.env_flag("MESSAGE"), expected, value)
4126+
self.assertEqual(main.INVALID_FLAGS, [])
4127+
4128+
def test_unset_input_takes_the_default(self):
4129+
with patch.dict(os.environ, {}, clear=True):
4130+
self.assertFalse(main.env_flag("MESSAGE"))
4131+
self.assertEqual(main.INVALID_FLAGS, [])
4132+
4133+
def test_other_values_are_recorded(self):
4134+
for value in ("yes", "on", "1", "ture"):
4135+
with patch.dict(os.environ, {"MESSAGE": value}):
4136+
self.assertFalse(main.env_flag("MESSAGE"))
4137+
self.assertEqual(
4138+
main.INVALID_FLAGS,
4139+
[
4140+
("MESSAGE", "yes"),
4141+
("MESSAGE", "on"),
4142+
("MESSAGE", "1"),
4143+
("MESSAGE", "ture"),
4144+
],
4145+
)
4146+
4147+
def test_invalid_input_fails_before_any_check(self):
4148+
main.INVALID_FLAGS.append(("PR_TITLE", "yes"))
4149+
out = io.StringIO()
4150+
with (
4151+
patch("sys.stdout", out),
4152+
patch("main.run_commit_check") as run,
4153+
self.assertRaises(SystemExit) as cm,
4154+
):
4155+
main.main()
4156+
self.assertEqual(cm.exception.code, 1)
4157+
run.assert_not_called()
4158+
self.assertIn(
4159+
"::error::Input pr-title must be true or false, got 'yes'", out.getvalue()
4160+
)
4161+
4162+
4163+
class TestPrCommentApiUrl(unittest.TestCase):
4164+
"""The PR comment goes to the same API host as every other call (GHES)."""
4165+
4166+
def test_comment_client_uses_github_api_url(self):
4167+
github_module = MagicMock()
4168+
pull_request = MagicMock()
4169+
pull_request.get_comments.return_value = []
4170+
github_module.Github.return_value.get_repo.return_value.get_issue.return_value = pull_request
4171+
with (
4172+
patch("main.PR_COMMENTS_ENABLED", True),
4173+
patch("main.is_fork_pr_with_readonly_token", return_value=False),
4174+
patch("main.get_pr_number", return_value=7),
4175+
patch.dict(
4176+
os.environ,
4177+
{
4178+
"GITHUB_EVENT_NAME": "pull_request",
4179+
"GITHUB_TOKEN": "t",
4180+
"GITHUB_REPOSITORY": "o/r",
4181+
"GITHUB_API_URL": "https://ghe.example.com/api/v3",
4182+
},
4183+
),
4184+
patch.dict(sys.modules, {"github": github_module}),
4185+
):
4186+
main.add_pr_comments([fail_scope()])
4187+
_, kwargs = github_module.Github.call_args
4188+
self.assertEqual(kwargs["base_url"], "https://ghe.example.com/api/v3")

0 commit comments

Comments
 (0)