fix(ci): document why gate-failure-watch's workflow_run trigger is safe - #10386
proggeramlug wants to merge 2 commits into
Conversation
zizmor has been red on main since 2026-09-06, the day gate-failure-watch.yml landed, on one high finding: dangerous-triggers against its `workflow_run`. The audit flags that trigger categorically -- "almost always used insecurely", at Medium confidence -- and this workflow is one of the cases where it is not. Both insecure uses are already closed in the file. The `observe` job's `if:` admits only schedule, or dispatch/push on main or a v* tag, so a fork PR's run can never reach the write-capable token; and the checkout pins `ref: main` with `persist-credentials: false` and runs only the default-branch scripts/gate_failure_watch.py, so the triggering run's code is never executed and none of its artifacts are downloaded. Permissions are actions:read, contents:read, issues:write. Suppressed inline rather than in .github/zizmor.yml so the justification sits next to the trigger it excuses, and carries its own ratchet: an artifact download, a head_sha checkout or a looser `if:` means deleting the marker. Removing the trigger is not an option -- observing another workflow's completion is the feature, and #9830 measured a correctly-failing scheduled workflow staying red for nineteen days unnoticed. Verified with the pinned zizmor 1.28.0 (external-tools.json) using the workflow's own invocation: `zizmor .github/ --min-severity high` goes from exit 14 with one high finding to exit 0, "No findings to report". The marker has to trail the `on:` key -- the same text as a comment block above it does not suppress anything, which is why this was checked against the real tool rather than reasoned about.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe workflow documents why its ChangesWorkflow audit annotation
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to The PR only documents and suppresses an existing workflow audit finding; no production or security regression is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Landed via merge train #10397 (v0.5.1584). All source commits preserve authorship; merged main matches the validated train exactly. |
zizmorhas been red onmainsince 2026-09-06 — the daygate-failure-watch.ymllanded — on a single high finding:It was not in the CI run's job list, which is why it is easy to miss:
zizmoris its own workflow, so a triage that reads only theCIrun's failures never sees it.This is a suppression of a high-severity finding, so the case for it should be checkable rather than taken on trust. zizmor flags
workflow_runcategorically — "almost always used insecurely", at Medium audit confidence. The two ways it is used insecurely are both already closed in this file, and each is one line to verify:observejob'sif:admits onlyschedule, orworkflow_dispatch/repository_dispatch/pushwhosehead_branchismainor av*tag. A fork PR's run satisfies none of those, so the write-capable token is never reachable from a contributor-controlled trigger.ref: mainwithpersist-credentials: false, and the only thing executed isscripts/gate_failure_watch.pyout of that trusted default-branch checkout. Nothing is taken from the triggering run — no artifact download, nohead_shacheckout.Permissions are
actions: read,contents: read,issues: write— enough to read a run's conclusion and file one issue, and nothing else.Removing the trigger is not an alternative: observing another workflow's completion is the feature. #9830 measured a correctly-failing scheduled workflow that stayed red for nineteen days with nobody noticing, which is what this exists to prevent.
Suppressed inline, not in
.github/zizmor.yml. The justification then sits next to the trigger it excuses rather than in a file three directories away, and it carries its own ratchet: if this workflow ever gains an artifact download, ahead_shacheckout, or a looserif:, the marker has to go and the gate fails again. That matches.github/zizmor.yml's own stated rule — "never add without a dated reason" — and the one inline precedent already in the tree (soak-autofix.yml'sartipacked).Validation — against the real tool, not by reasoning:
zizmor 1.28.0(external-tools.jsontools.zizmor) and ran the workflow's own invocation,zizmor .github/ --min-severity high: exit 14 with one high finding before, exit 0 and "No findings to report" after.on:suppresses nothing — exit 14, unchanged. The marker must trail theon:key. I would have shipped the non-working form otherwise.check_gate_freshness.py,gate_failure_watch.py --self-testand--check-configall pass, and the file still parses as YAML with both triggers intact.If you would rather not carry a suppressed high finding at all, the honest alternative is to drop
workflow_runand lose the observer — I did not take that decision unilaterally.Summary by CodeRabbit
Chores
Documentation