ci: check PR bodies for relay image URLs - #6745
hrithiksaini99 wants to merge 2 commits into
Conversation
Chessing234
left a comment
There was a problem hiding this comment.
this is carefully built — checking out github.event.pull_request.base.sha rather than the head, persist-credentials: false, contents: read, a SHA-pinned checkout action, and reading the body out of GITHUB_EVENT_PATH as data rather than interpolating ${{ github.event.pull_request.body }} into a run: block. that last one is the mistake this class of workflow usually makes, and the jq type-guard (error("pull_request.body must be a string or null")) closes the rest of it. the mktemp + trap and passing a file path rather than a string means the checker never sees contributor text on a command line either.
three things:
is this a required check? a guard that runs on pull_request_target and exits non-zero is advisory until branch protection lists it. scripts/test-pr-image-url-event.sh gets wired into ci.yml, so the contract test is gated — but the actual body check runs in its own pr-image-urls.yml workflow, and if that job name isn't in the required set, a PR with a relay media URL merges with a red check nobody has to look at. worth saying in the pr which it is; if it needs a settings change, that's the follow-up that makes the code useful.
synchronize can't change a body. the trigger list is [opened, edited, reopened, synchronize], and synchronize fires on every push to the branch — which never alters pull_request.body. on a busy PR that's the majority of the runs, all re-checking identical text. dropping it costs nothing and halves the noise; opened/edited/reopened are the three that can produce a new body.
the exit code on "no pull_request object" is a hard failure. jq -er with error("event does not contain a pull_request object") is correct for this workflow, where the trigger guarantees one. but the script is generic enough that someone will reuse it from a workflow_dispatch or a schedule and get a confusing jq stack trace instead of a skip. a named exit code or a short message before the jq call would make that reuse safe.
smaller: the test coverage listed (rejected relay URLs, accepted GitHub attachments, empty bodies, command-shaped input) is the right set — "command-shaped input" in particular is the one worth having, since it's what proves the data-not-code claim. worth naming in the pr what "command-shaped" means concretely (backticks? $(...)? a leading - that could be read as a flag?) so a reader can tell whether the argument-injection case is covered as well as the shell-injection one.
Signed-off-by: Hrithik saini <49943287+hrithiksaini99@users.noreply.github.com>
Signed-off-by: Hrithik saini <49943287+hrithiksaini99@users.noreply.github.com>
7c0c72f to
51bba27
Compare
|
Addressed in
The branch is rebased onto current |
Summary
pull_request_targetworkflow, with read-only contents permission, a SHA-pinned checkout, and persisted credentials disabled.Related issue
Fixes #6726.
Required-check status
The repository's active default-branch ruleset does not currently list
Check PR image URLsas a required status check, so the new workflow is advisory until that repository setting is updated. Maintainer follow-up after merge: addCheck PR image URLsto the required checks for the default branch.The contract test remains part of
ci.yml, but that verifies the checker integration rather than enforcing each PR body's current content.Untrusted-input coverage
The shell-injection fixture places a literal
$(touch "...")expression in the PR body and asserts that the target file is never created. The body itself is extracted byjqinto a temporary Markdown file; only that generated file path is passed to the checker, so PR text is not evaluated or supplied as command-line arguments.Testing
just ci./scripts/test-pr-image-url-event.shactionlint .github/workflows/pr-image-urls.ymlshellcheck scripts/check-pr-image-urls-event.sh scripts/test-pr-image-url-event.sh scripts/check-pr-image-urls.shNo screenshots: this is a CI-only change.