chore(sec): pr-labels.yml + qa-gate.yml → pull_request_target#409
Conversation
Closes the same self-bypass hole that drove the v0.18.4 CLA-bypass migration (#387 retrospective): with `pull_request`, a same-repo PR runs the workflow file from the PR branch with a write-capable GITHUB_TOKEN, letting a contributor with push access edit the label-discipline rules or QA Gate logic inside their own PR. `pull_request_target` always evaluates the base-branch copy of the workflow. Both workflows are safe under `pull_request_target` because neither checks out the PR branch or executes PR code — they only read PR metadata from `github.event` and write labels/statuses via the GitHub API. `qa-gate.yml` additionally hardened: inline `'${{ toJSON(github.event.pull_request.labels.*.name) }}'` shell grep replaced with a `PR_LABELS` env var, and `${{ github.repository }}` / `${{ github.event.pull_request.head.sha }}` interpolated into the `gh api` URL line lifted into `REPO` / `HEAD_SHA` env vars — same cascade-consistency hardening already in pr-labels.yml / pr-labels-ci.yml. Caveat: introduction PR will not auto-label or auto-status itself — base branch still has `pull_request`, PR has `pull_request_target`, neither matches the other event. Manual label progression and a manual `QA Gate` status set required this once. Future PRs pick up the new behavior after merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
[QA] Round 1 — Ready for QA Signoff. Zero findings.
CI-only change closing the same self-bypass class as #387. 3 files, +42/-4. All 17 status checks green; merge state BLOCKED only because qa-gate.yml doesn't fire on its own introduction PR (documented bootstrap caveat — QA Gate status will be posted manually by maintainer at signoff time).
Code review
| Area | Verdict |
|---|---|
Trigger swap (pull_request → pull_request_target) on pr-labels.yml + qa-gate.yml |
Correct. Neither workflow checks out the PR branch or executes PR code; both only read github.event and write via the GitHub API, so the classic pull_request_target hazard does not apply. |
qa-gate.yml env-var hardening (PR_LABELS / REPO / HEAD_SHA) |
Matches the cascade pattern already in pr-labels.yml / pr-labels-ci.yml. Contributor-controlled values now flow through env, never inlined into the shell command line. |
| Repo-wide trigger audit | Verified by grep: PR-body table matches .github/workflows/*.yml exactly. The five execute-PR-code workflows (ci, gitleaks, semgrep, pip-audit, docker-smoke) correctly stay on pull_request. cla-bot-bypass.yml and dependabot-changelog.yml already on pull_request_target. pr-labels-ci.yml on workflow_run. No other gating workflow needs migration. |
| CHANGELOG entry placement | ## [Unreleased] → ### Security after ### Fixed. KaC v1.1.0 ordering correct. Wording is rich and accurate (covers both the trigger swap and the env-var hardening). |
Manual tests (PR-body checkboxes 1–3 verified, 4 deferred)
- Workflow YAML parses cleanly. All CI workflows on the PR head succeeded; no parse-error annotations on either changed workflow file. ✓
pr-labels.ymlandqa-gate.ymldid not fire on this PR. Verified viarepos/cmeans/mcp-awareness/actions/runs?head_sha=b68df74...: onlycla-bot-bypass(success) andDependabot CHANGELOG(skipped) ran onpull_request_target; the fivepull_requestruns are ci/gitleaks/semgrep/pip-audit. Nopr-labels.ymlorqa-gate.ymlruns exist on this SHA. ✓pr-labels-ci.ymlstill fires viaworkflow_run. Run25072746466(2026-04-28 19:16:40 UTC) found PR #409 via thehead_branchfallback, observedDev Activeon the PR, and exited cleanly with "Dev Active present — skipping promotion". Theworkflow_runpath is unaffected. ✓- Post-merge spot check on the next PR. Deferred to next PR's QA review — please verify there that
pr-labels.ymlauto-appliesAwaiting CIon opening, andqa-gate.ymlposts theQA Gatestatus under the new trigger.
Live-bootstrap evidence on this PR
Current PR labels: Ready for QA, QA Active. Under the base-branch (still pull_request) pr-labels.yml, adding QA Active would trigger the on-label cleanup and remove Ready for QA. The fact that Ready for QA lingered after I added QA Active is direct visible confirmation that pr-labels.yml is not firing on this PR — exactly the bootstrap behavior the PR body documents.
Manual interventions for this PR
Per the PR body, after maintainer applies QA Approved, post the QA Gate status manually:
gh api repos/cmeans/mcp-awareness/statuses/b68df7423085d134d159536652a3e71055600bb0 --method POST -f state=success -f description="QA testing completed (manual post — qa-gate.yml does not fire on its own introduction PR)" -f context="QA Gate"
Applying Ready for QA Signoff as the final act.
cmeans
left a comment
There was a problem hiding this comment.
[QA] Round 1 — Ready for QA Signoff. Zero findings.
CI-only change closing the same self-bypass class as #387. 3 files, +42/-4. All 17 status checks green; merge state BLOCKED only because qa-gate.yml doesn't fire on its own introduction PR (documented bootstrap caveat — QA Gate status will be posted manually by maintainer at signoff time).
Code review
| Area | Verdict |
|---|---|
Trigger swap (pull_request → pull_request_target) on pr-labels.yml + qa-gate.yml |
Correct. Neither workflow checks out the PR branch or executes PR code; both only read github.event and write via the GitHub API, so the classic pull_request_target hazard does not apply. |
qa-gate.yml env-var hardening (PR_LABELS / REPO / HEAD_SHA) |
Matches the cascade pattern already in pr-labels.yml / pr-labels-ci.yml. Contributor-controlled values now flow through env, never inlined into the shell command line. |
| Repo-wide trigger audit | Verified by grep: PR-body table matches .github/workflows/*.yml exactly. The five execute-PR-code workflows (ci, gitleaks, semgrep, pip-audit, docker-smoke) correctly stay on pull_request. cla-bot-bypass.yml and dependabot-changelog.yml already on pull_request_target. pr-labels-ci.yml on workflow_run. No other gating workflow needs migration. |
| CHANGELOG entry placement | ## [Unreleased] → ### Security after ### Fixed. KaC v1.1.0 ordering correct. Wording is rich and accurate (covers both the trigger swap and the env-var hardening). |
Manual tests (PR-body checkboxes 1–3 verified, 4 deferred)
- Workflow YAML parses cleanly. All CI workflows on the PR head succeeded; no parse-error annotations on either changed workflow file. ✓
pr-labels.ymlandqa-gate.ymldid not fire on this PR. Verified viarepos/cmeans/mcp-awareness/actions/runs?head_sha=b68df74...: onlycla-bot-bypass(success) andDependabot CHANGELOG(skipped) ran onpull_request_target; the fivepull_requestruns are ci/gitleaks/semgrep/pip-audit. Nopr-labels.ymlorqa-gate.ymlruns exist on this SHA. ✓pr-labels-ci.ymlstill fires viaworkflow_run. Run25072746466(2026-04-28 19:16:40 UTC) found PR #409 via thehead_branchfallback, observedDev Activeon the PR, and exited cleanly with "Dev Active present — skipping promotion". Theworkflow_runpath is unaffected. ✓- Post-merge spot check on the next PR. Deferred to next PR's QA review — please verify there that
pr-labels.ymlauto-appliesAwaiting CIon opening, andqa-gate.ymlposts theQA Gatestatus under the new trigger.
Live-bootstrap evidence on this PR
Current PR labels: Ready for QA, QA Active. Under the base-branch (still pull_request) pr-labels.yml, adding QA Active would trigger the on-label cleanup and remove Ready for QA. The fact that Ready for QA lingered after I added QA Active is direct visible confirmation that pr-labels.yml is not firing on this PR — exactly the bootstrap behavior the PR body documents.
Manual interventions for this PR
Per the PR body, after maintainer applies QA Approved, post the QA Gate status manually:
gh api repos/cmeans/mcp-awareness/statuses/b68df7423085d134d159536652a3e71055600bb0 --method POST -f state=success -f description="QA testing completed (manual post — qa-gate.yml does not fire on its own introduction PR)" -f context="QA Gate"
Applying Ready for QA Signoff as the final act.
|
[QA] Applying Ready for QA Signoff — all 17 checks pass, zero findings on Round 1. CI-only workflow YAML change with verified bootstrap behavior (neither |
…#378) (#410) ## Summary Closes #378. Two stale-label traps in `pr-labels-ci.yml` fixed symmetrically; both rooted in narrow outer guards that only fired on `Awaiting CI`, missing the post-`CI Failed` recovery arc and the `Ready for QA → CI Failed` regression arc. | Job | Today | Now | | --- | --- | --- | | `on-ci-pass` | Promotes only when `Awaiting CI` is present | Promotes when `Awaiting CI` OR `CI Failed` is present | | `on-ci-fail` | Adds `CI Failed` only when `Awaiting CI` is present | Adds `CI Failed` when `Awaiting CI` OR `Ready for QA` is present | ### Bug 1 — `CI Failed → CI pass` silently no-ops (issue #378) Reproduction trail in #377 (2026-04-22): a lint-failing push moved labels to `CI Failed`; the fix-up push made CI go green; `on-ci-pass` fired and ran, but its outer `if echo "$LABELS" | grep -q "^Awaiting CI$"` was false (only `CI Failed` was present), so it silently no-op'd. PR sat at `CI Failed` while CI was actually green. Required a manual `gh pr edit --remove-label "CI Failed" --add-label "Ready for QA"` to unstick. ### Bug 2 — `Ready for QA → CI re-fail` keeps the green label (symmetric) Mirror trap on `on-ci-fail`: a CI re-run on a PR sitting at `Ready for QA` (e.g., manual re-trigger after a flake, or a workflow change forcing a re-run) that turns red leaves the PR labelled `Ready for QA` because the outer `if echo "$LABELS" | grep -q "^Awaiting CI$"` is false. The status check goes red but the label still says ready — QA might pick it up assuming CI is green. ### Review-state preservation Broadening the triggers introduces a new risk: if a `QA Active` / `Ready for QA Signoff` / `QA Approved` label coexists with a CI label (race, or manual mistake), the broader trigger could overwrite review-machine state with `Ready for QA` (on pass) or `CI Failed` (on fail). To prevent that, both jobs now short-circuit explicitly when any of those three labels is present: ```bash for QA_STATE in "QA Active" "Ready for QA Signoff" "QA Approved"; do if echo "$LABELS" | grep -q "^$QA_STATE$"; then echo "$QA_STATE present — skipping (review in progress)" exit 0 fi done ``` Rationale: review state advances independently of CI re-runs. A passing or failing CI re-run on a PR that's already in QA review is visible via the check itself; the label transition would be redundant on success and destructive on failure. `Dev Active` short-circuit preserved unchanged. ### Safety - Trigger remains `workflow_run` — base-branch context, immune to PR-branch edit attacks (same protection class as the `pull_request_target` migration in #409). - No new contributor-controlled inputs. Label list still read via `gh pr view --json labels` (repo-owned strings, not fork-controlled). - All grep patterns remain anchored (`^Label$`) so labels like `Awaiting CI Failed` (if one ever existed) cannot accidentally satisfy a `^Awaiting CI$` check. - Existing env-routing of `HEAD_BRANCH` / `RUN_ID` / `PR` / `REPO` (hardened in #332/#333) is unchanged. Nothing I add interpolates new contributor-controlled values into shell. ### State-machine trace (full) Pre-state → CI conclusion → resulting transition (✓ = covered, ✗ = no-op, * = new): | Pre-state | CI = success | CI = failure | | --- | --- | --- | | `Awaiting CI` | → `Ready for QA` ✓ | → `CI Failed` ✓ | | `CI Failed` | → `Ready for QA` ✓* | stays `CI Failed` ✓ | | `Ready for QA` | stays `Ready for QA` ✓ | → `CI Failed` ✓* | | `Dev Active` | no-op (skip) ✓ | no-op (skip) ✓ | | `QA Active` | no-op (skip) ✓* | no-op (skip) ✓* | | `Ready for QA Signoff` | no-op (skip) ✓* | no-op (skip) ✓* | | `QA Approved` | no-op (skip) ✓* | no-op (skip) ✓* | The * entries are new in this PR. The `Dev Active` and "no pre-state" cases were already correct. ## Test plan Workflow YAML only. No tests to add. ## QA ### Prerequisites - None — pure workflow YAML change. ### Manual tests 1. - [x] **Workflow YAML parses cleanly.** Confirm the Actions tab on this PR shows no parse-error annotations on `pr-labels-ci.yml`. 2. - [x] **Diff matches the state-machine trace table above.** Read `.github/workflows/pr-labels-ci.yml` head-to-toe; for each row of the trace, confirm the corresponding code path emits the expected transition (or skip). 3. - [x] **#409 migration live-validation (deferred from #409 QA test plan #4).** This is the first PR opened against `main` since the `pr-labels.yml` / `qa-gate.yml` migration to `pull_request_target`. Confirm: - `pr-labels.yml` `on-push` fired on opening: `Awaiting CI` was applied automatically (no manual addition required this time). - `qa-gate.yml` posted a `QA Gate` status on this PR's head SHA from app `15368` (GitHub Actions). Visible in the status-check rollup. - These two observations together confirm #409's migration works end-to-end on a real PR — not just on the introduction PR's bootstrap-skipped path. 4. - [ ] **Verification of the bug-fix itself is post-merge.** `workflow_run` triggers always run from the default branch (per the `LIMITATION` comment at the top of `pr-labels-ci.yml`), so this PR's changes do not run on this PR. The natural validation is the next CI-fail-then-pass PR after this lands — when that happens, the PR should auto-promote `CI Failed → Ready for QA` without manual intervention. Reviewer should add a follow-up note here (or in the awareness milestone for this PR) once that natural validation occurs. ### Out-of-scope follow-ups (not for this PR) - The `dismiss_stale_reviews_on_push` setting interacts with these transitions in subtle ways (review approvals get auto-dismissed on push, then CI re-runs). No change proposed; just flagging for awareness. - A future enhancement could add a `QA Invalidated` style label for the case where CI re-fails on a PR in QA review, but doing so requires designing the QA recovery path. Out of scope for #378. Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Migrates the two remaining self-gating workflows (
pr-labels.yml,qa-gate.yml) frompull_requesttopull_request_targetso the gating logic a PR is evaluated under is always the base-branch (main) copy — closing the same self-bypass hole that drove the v0.18.4 CLA-bypass migration (#387 retrospective).With
pull_request, a same-repo PR runs the workflow file from the PR branch with a write-capableGITHUB_TOKEN. A contributor with push access could:pr-labels.ymlto applyQA Approvedautomatically inside their own PR, orqa-gate.ymlto postQA Gate=successregardless of label state,and have the modified version run on that same PR.
pull_request_targetalways evaluates the workflow file from the base branch, so a PR cannot alter the gate that applies to it.qa-gate.ymladditionally hardened: the inline'${{ toJSON(github.event.pull_request.labels.*.name) }}'shell grep replaced with aPR_LABELSenv var, and${{ github.repository }}/${{ github.event.pull_request.head.sha }}interpolated into thegh apiURL line lifted intoREPO/HEAD_SHAenv vars — same cascade-consistency pattern already inpr-labels.yml/pr-labels-ci.yml.Why this is safe under
pull_request_targetThe classic
pull_request_targethazard is checking out and executing untrusted PR code with elevated permissions. Neither workflow checks out the PR branch or executes PR code — both only read PR metadata fromgithub.eventand write labels/statuses via the GitHub API. The base-branch checkout (whichpull_request_targetdefaults to) is never invoked.Other workflows audited, intentionally left on
pull_requestci.ymlpull_requestpull_request_targetwith secrets access would be unsafe.gitleaks.ymlpull_requestsemgrep.ymlpull_requestpip-audit.ymlpull_requestdocker-smoke.ymlpull_requestcla-bot-bypass.ymlpull_request_targetdependabot-changelog.ymlpull_request_targetpr-labels-ci.ymlworkflow_runBootstrap caveat — read this before reviewing
This introduction PR will not auto-label or auto-status itself:
on: pull_request:for both workflows; the PR branch hason: pull_request_target:.pull_requestevents look at the PR-branch workflow file → trigger doesn't match → workflow doesn't fire.pull_request_targetevents look at the base-branch workflow file → trigger doesn't match → workflow doesn't fire.So on this PR specifically:
pr-labels.ymlwon't addAwaiting CIon push or transition labels.qa-gate.ymlwon't post theQA Gatestatus check.pr-labels-ci.yml(workflow_run) is unaffected and will still promoteAwaiting CI→Ready for QAonce CI completes — but only ifAwaiting CIis added first. Manual label progression on this PR; future PRs pick up the new behavior automatically once this lands.This is the same one-time bootstrap that the v0.18.4 CLA-bypass migration required, and it's correct: introducing a self-gating mechanism should require a manual review + merge, otherwise the mechanism bootstraps itself, which is the same circularity that makes the hole exploitable.
Test plan
CI-only change. No code or test count changes.
QA
Prerequisites
Manual tests
pr-labels.ymlandqa-gate.ymldid not fire on this PR. This is the documented bootstrap behavior. Expected: in the Actions tab for this PR, neither workflow has a run associated with this PR's events (onlypr-labels-ci.ymlruns may appear, fired byworkflow_runfromci.yml).pr-labels-ci.ymlstill promotes correctly viaworkflow_run. Once CI passes on this PR andAwaiting CIis manually added, expected:pr-labels-ci.ymlremovesAwaiting CIand addsReady for QA. (This validates theworkflow_runpath is unaffected by this change.)Awaiting CIadded automatically bypr-labels.ymlon opening (now firing underpull_request_targetfrom main)QA Gatecommit status posted byqa-gate.yml(now firing underpull_request_targetfrom main)This step happens on a future PR, not this one — note it as a verification to perform when reviewing the next merged PR.
Manual interventions required for this PR
Because the gating workflows don't fire on this PR, QA must take the following steps manually instead of waiting for automation:
Awaiting CIlabel after pushing (sopr-labels-ci.ymlcan promote on CI green).QA Approvedlabel as usual; then post theQA Gatestatus manually: