security(ci): parse the DR publisher step instead of scanning workflow text - #2998
Merged
Conversation
…w text
validateDRWorkflow was the last of the three production routes still
establishing its wiring by scanning the rebuild job's TEXT. containsLine is
satisfied by the expected string appearing anywhere in the job, so five shapes
passed the gate while the disaster-recovery route published through logic the
contract never checked. Each is reproduced as an ablation and each returned nil
against the scanning version:
1. the `uses:` line surviving as a COMMENT while the step invokes a different
action;
2. the same text inside a BLOCK SCALAR, with no `uses:` step at all;
3. ghcr-token/hcloud-token named on a DIFFERENT step, so the publisher could
be invoked with no credentials;
4. the publisher step carrying `if: false`, so it never runs;
5. the publisher step carrying `continue-on-error: true`, so its failure is
discarded and the rebuild promotes the mutable tag anyway.
(4) and (5) are the requireEnforcedStep class, now closed at its sixth site:
the step is located and proven enforced in one call, so the omission is not
representable.
The three permission grants move to parsed lookups on the job's `permissions`
mapping. The scanning version compared each against a whole line INCLUDING its
trailing comment, so rewording a comment broke the gate while the grant was
untouched — a contract that fails on a documentation edit trains people to
weaken it. A permissions value that cannot be read is refused rather than
skipped, including the blanket `write-all` scalar, which grants enough today but
names none of the three and so could be narrowed silently.
extractJob and containsExactLine had no other caller here and are removed;
containsLine and lineIndexContaining stay for validatePublicationAction.
Contributor
✅
|
Contributor
Author
@coderabbitai review |
Contributor
|
|
Contributor
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
Contributor
Author
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
Contributor
Author
|
@cursor review |
Contributor
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_804a728e-56b2-4e03-9864-79d60cbd4f68) |
devantler
commented
Aug 6, 2026
devantler
left a comment
devantler
left a comment
Contributor
Author
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 0bd41dbf7ec394bcb8336a029cfee2e451953482
All three external lanes were tried at this exact head, in contract priority order, one at a time, and each returned direct per-PR evidence of being unable to deliver:
| Lane | Evidence at this head | When |
|---|---|---|
| CodeRabbit | acked ("I will review the changes"), then a Review limit reached shell 4s later; commit-status description Review rate limited; 0 review objects |
08:24:41Z → 08:24:45Z |
| Codex | comment: "You have reached your Codex usage limits for code reviews"; 0 review objects; no reaction on the trigger | 08:28:27Z |
| Cursor Bugbot | check-run Cursor Bugbot conclusion: neutral + output.title: Error, summary Bugbot run failed; cursor[bot]: "Bugbot couldn't run - usage limit reached"; 0 inline comments |
08:34:11Z → 08:34:13Z |
Bugbot's limit is a Cursor spend limit with no retry window — only an admin can raise it — so waiting would not have produced a review this run.
What I checked
- The RED half is real, not asserted. Of the 11 ablations, five return
nilagainst the pre-change scanning implementation: publisher named only in a comment, only inside a block scalar, credentials on a different step, stepif: false, stepcontinue-on-error: true. Each ablation moves the mechanism (the expected text survives somewhere inert) rather than deleting it, so it separates a scan from a parse instead of merely proving something is required. - No direction is weakened.
usesActioncompares the wholeusesvalue for equality, where the oldcontainsLinewas a substring match — so…/publish-platform-manifests-evilused to pass and is now refused. Credentials moved from "anywhere in the job text" to "on the publisher step, exact expression". The one deliberate loosening is that a permission's trailing comment is no longer part of the contract; the grant itself is still required to be exactlywrite. - Fail-closed shapes are covered: unparseable workflow, no
jobsmapping, renamed job, absentpermissions, and a blanketwrite-allscalar (refused — it grants enough today but names none of the three, so a later narrowing could drop one silently). - The consuming job actually ran, rather than counting "0 failing":
🖋️ Validate Publication Contract= pass, alongside🔍 Dead Code Analysis(confirmingextractJob/containsExactLineremoval left nothing dangling) andgolangci-lint. 30 checks, 0 failing. - End-to-end, both directions, through the real CLI rather than the unit tests alone: the shipped config passes; a comment-ablated copy placed inside the repo tree (so derived publisher/cd/ci paths resolve) is refused with the correct message. My first attempt at this control was invalid — it failed on a path error rather than the ablation — and was redone.
Considered and dismissed (recorded so a later round does not re-raise them)
- "The publisher step is enforced but not proven reached." Not a hole here: GitHub runs each step as its own process, so an early
exit 0in a precedingrunstep ends that step successfully and later steps still run. The gate-job allowlist used forvalidate-publication-contractwould be wrong for a long operational job likerebuild. - The rebuild job's own
if:is deliberately not refused — it is the supersession gate, a legitimate and necessary condition, unlike cd.yaml'sdeploy-prod. The step-level enforcement is what matters and is checked.
Verdict: no P0/P1 findings.
Contributor
Author
Readiness — all three conditions, at
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The disaster-recovery rebuild is the one route to production that runs when the cluster is gone —
the moment nobody can afford to debug a supply-chain policy by hand. Its publication wiring was the
last of the three routes still established by scanning the workflow's text rather than reading
it, and a scan cannot tell the difference between a step that runs and a string that merely appears.
Five ways the gate said "wired" about a DR route that was not — each one reproduced, and each one
accepted by the old check: the publisher named only in a comment, or inside an
echo, with itscredentials sitting on an unrelated step, or with the step switched off entirely so its failure is
skipped or discarded. In every case the workflow reads correct and the contract reports success.
Not reachable by an outside contributor — it needs a workflow edit — so this hardens a gate that
already exists rather than closing a live exposure.
What
The DR route is now parsed like the other two: the publisher is located as a real step whose action
matches exactly, proven to be neither skipped nor failure-suppressed, and its two credentials are
asserted on that step instead of anywhere in the job.
The three permission grants move to parsed lookups as well. The old check compared each against a
whole line including its trailing comment, so rewording a comment broke the gate while the grant
itself was untouched — a contract that fails on a documentation edit is one people learn to weaken.
Shipped configuration passes unchanged; no workflow or cluster config is modified.
Fixes #2941
Part of #2627