Skip to content

ci: fix commitlint workflow and post lint failures as PR comments#735

Merged
bnusunny merged 4 commits into
mainfrom
fix-commitlint-exit-code
May 28, 2026
Merged

ci: fix commitlint workflow and post lint failures as PR comments#735
bnusunny merged 4 commits into
mainfrom
fix-commitlint-exit-code

Conversation

@bnusunny
Copy link
Copy Markdown
Contributor

@bnusunny bnusunny commented May 27, 2026

Summary

  • Commitlint workflow silently passed even when commit messages were invalid: tee masked commitlint's exit code (no pipefail) and continue-on-error: true then turned the step's outcome into success, so the gating Show help on failure step never ran. Fixed with set -o pipefail.
  • Add inline PR-comment feedback for lint failures, including for fork PRs, by splitting the workflow into two: an unprivileged pull_request workflow (commitlint.yaml) that runs the lint and uploads its output as an artifact, and a privileged workflow_run workflow (commitlint-comment.yaml) that downloads the artifact and posts the comment. The privileged workflow never checks out PR code and never runs npm/build steps, addressing the "pwn request" pattern that CodeQL flags on pull_request_target + checkout designs.
  • Validate the PR number from the artifact against ^[0-9]+$ before passing it to gh pr comment, and render commitlint output inside a fenced code block to neutralize markdown injection from commit messages.
  • Add --ignore-scripts to npm install and persist-credentials: false to checkout as defense in depth.
  • Remove unused .gitallowed — no workflow or tooling references it, and the only file containing the allowlisted BEGIN PRIVATE KEY string was .gitallowed itself.

Notes

  • workflow_run workflows only execute from files on the default branch, so end-to-end validation of the comment-posting step has to happen on a follow-up PR after this one merges.
  • The old (broken) commitlint workflow is still gating this PR until merge.

Test plan

  • CI passes on this PR (commits in this PR follow the convention).
  • CodeQL alert If possible to support wss protocol's web socket #11 clears after this PR's analysis run.
  • After merge: open a follow-up PR with an intentionally non-conformant commit message, confirm the check fails and a PR comment is posted with the lint output.
  • After merge: do the same from a fork to confirm fork-PR coverage.

bnusunny added 2 commits May 27, 2026 19:47
The commitlint step piped to tee without pipefail, so commitlint's
non-zero exit code was masked by tee's success and the step's outcome
was always success. Combined with continue-on-error: true, the
Show help on failure step never ran and the job passed even when
commits violated the convention.

Fix the exit-code propagation and add inline PR-comment feedback for
lint failures, including for fork PRs, using the split-workflow pattern
recommended by GitHub Security Lab to avoid the pwn-request risks of
pull_request_target + checkout:

- commitlint.yaml stays on: pull_request (unprivileged). Adds
  set -o pipefail so commitlint's exit code propagates. On failure,
  uploads the lint output and PR number as an artifact, then exits 1
  to fail the check.

- commitlint-comment.yaml is a new on: workflow_run workflow
  (privileged) with pull-requests: write. It never checks out PR code
  and never runs npm or any build step. It downloads the artifact,
  validates the PR number against ^[0-9]+$, and posts the lint output
  as a PR comment inside a fenced code block to neutralize markdown
  injection from commit messages.

Defense in depth:
- npm install --ignore-scripts disables preinstall and postinstall
  scripts on the unprivileged side.
- persist-credentials: false strips the writable git credential helper
  from the checkout.

Note: workflow_run workflows only execute from files on the default
branch, so end-to-end validation requires a follow-up test PR after
this merges.
This was an allowlist for a secret-scanning tool, but no workflow,
Makefile, or tooling in the repo references it. The only file that
contained the allowlisted "BEGIN PRIVATE KEY" string was .gitallowed
itself, so removing it changes nothing about scan results.
@bnusunny bnusunny force-pushed the fix-commitlint-exit-code branch from aeac541 to ead2306 Compare May 27, 2026 19:48
Comment thread .github/workflows/commitlint-comment.yaml Outdated
Comment thread .github/workflows/commitlint-comment.yaml Outdated
bnusunny added 2 commits May 28, 2026 02:35
Resolve PR number from the trusted workflow_run head_sha via the GitHub
API instead of reading it from the artifact. The artifact is produced by
the unprivileged pull_request workflow whose file fork authors control,
so an attacker could otherwise direct the bot comment at a different PR.

Size the markdown code fence one backtick longer than the longest
backtick run in the lint output, so a commit message containing ``` can
no longer escape the fence and inject markdown into the bot comment.
@bnusunny bnusunny merged commit 2335750 into main May 28, 2026
6 checks passed
@bnusunny bnusunny deleted the fix-commitlint-exit-code branch May 28, 2026 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants