fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307) - #118
Open
mattmillerai wants to merge 8 commits into
Open
fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307)#118mattmillerai wants to merge 8 commits into
mattmillerai wants to merge 8 commits into
Conversation
…e the tool checkout (BE-6307) workflows_ref supplies the code that runs in the gate and grade jobs, and the grade job holds a GH_TOKEN carrying pull-requests: write. The "pin it to the same full SHA as uses:" contract was prose only, so a caller could pass main, a tag, or refs/pull/N/head (fork-authored, mutable) and the run succeeded. Both jobs that check out workflows_ref now reject anything but a full 40-hex commit SHA, before the checkout. Enforced in the workflow rather than in a script: the scripts are what the ref loads, so a script-side check would sit inside the blast radius it is meant to bound.
mattmillerai
added
cursor-review
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…ust to SHA shape (BE-6307) The shape check alone did not deliver the guarantee its own comment claimed. GitHub serves a fork PR's head objects from the upstream repo, so a fork-authored commit of this PUBLIC repo has a perfectly well-formed 40-hex SHA: `refs/pull/N/head` was rejected while the identical commit, spelled as a SHA, was checked out into a job holding the caller's `pull-requests: write` token. A stale pin left behind when `uses:` moved passed for the same reason. Both guard copies now also require the value to equal `github.job_workflow_sha` — the commit `uses:` actually resolved to for that job, supplied by the runner and unforgeable by any input. That is what turns "pin it to the same SHA as `uses:`" from prose into a check. An empty context warns and falls back to the shape test rather than red-checking every consumer on a platform regression; it is always populated for a `workflow_call` job and this workflow has no other trigger. Also from the review panel: - accept a mixed-case SHA (`[0-9a-fA-F]`, case-insensitive compare). Git parses object IDs case-insensitively, so rejecting one told a caller to pin the SHA it had already pinned. - never echo the raw value into a `::error::`. A multi-line value — precisely what the `[[ =~ ]]` choice exists to catch — ended the annotation at the first newline and left the rest to be re-parsed as workflow commands (`::stop-commands::`, a forged `::notice::`) in a public log. Non-ref characters become `?` and the result is truncated to one bounded line. - add scripts/pr-risk/tests/test_pin_contract.sh. The invariants "every job that checks out the ref re-asserts this itself" and "the two copies are byte-identical" were held up by a comment and hand-copying, with nothing to fail if either broke. Structural, over the workflow text, because the guard cannot live in a script: a script-side check would sit inside the blast radius it exists to bound.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 3 |
| 🟡 Medium | 4 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
…sert the guard by property (BE-6307)
Second review round, all of it fair.
FAIL CLOSED. The empty-`job_workflow_sha` path warned and exited 0, so the
lock-step axis failed open. An unrecognized `github` context property evaluates
to the empty string, which means a rename or removal upstream would have turned
the whole boundary into a silent permanent no-op with no red check anywhere —
the one failure mode a guard like this must not have. Both copies now `exit 1`
with an `::error::`. The blast radius I was protecting against is small (an
advisory shadow check with a handful of callers); the one I was creating was
not.
NARROW THE CLAIM. `github.job_workflow_sha` is whatever the CALLER's own `uses:`
resolved to, so a caller that points `uses:` itself at a fork commit is already
running that fork's copy of this file and satisfies both axes. No check inside
the called workflow can reach that; reviewing the caller on its base branch is
what bounds it. What the axis does prove is that the two halves of the tool
cannot disagree — a stale pin left behind when `uses:` moved, or a fork-authored
SHA handed to `workflows_ref` while `uses:` stayed upstream. Header, input
description, step comment and README row now say that and no more.
TEST ASSERTS PROPERTIES, NOT COUNTS. The suite pinned a literal count of two
`exit 1` lines, so it would have gone red on the fix above — a test that blocks
its own subject's hardening teaches people to delete the test. It now asserts
that no rejection path exits non-fatally (no `exit 0` at all) and that every
`::error::` is paired with a failing exit, over a comment-stripped body. Also
fixed there:
- the raw-echo check matched only the braced `${WORKFLOWS_REF}`, missing the
likelier unbraced regression it exists to catch;
- checkout detection was a byte-exact line match, so a requoted, reindented or
respaced `ref:` was invisible to both the coverage walk and the count;
- `/^jobs:$/` and the job-key anchor were exact enough that a trailing space or
comment made the primary assertion pass vacuously with zero coverage — the
awk now reports what it matched and the suite fails if that is nothing;
- the drift slicer only closed on a sibling step, so a guard that is the last
step of a job absorbed the next job and reported spurious drift;
- `mktemp -d` failure is checked before the `rm -rf` trap is installed.
Mutation-checked: reverting the fail-open, echoing the unbraced value, dropping
the lock-step compare, loosening one copy's regex, adding an unguarded checkout,
and moving a guard after the checkout it protects each fail the suite; a
respelled `${{inputs.workflows_ref}}`, a trailing comment on a job key, and a
trailing space on `jobs:` no longer do.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 6 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…rt positions not sums (BE-6307)
Third review round, on the guard and on the suite that guards it.
WORKFLOW
- The mismatch error told a caller that a local `./` call must pass its own
run's SHA. Unfollowable: the checkout below hardcodes
`repository: Comfy-Org/github-workflows`, so taking that advice swaps a guard
failure for a checkout failure on a SHA that does not exist there. Replaced
with the remedy that does apply — including the tag case, where "the same SHA
as `uses:`" is not actionable because `uses:` holds no SHA.
- Documented the corollary rather than leaving it implicit: a tag- or
branch-pinned `uses:` cannot satisfy the lock-step axis, since
`job_workflow_sha` is then whatever the tag currently points at and a
force-moved `v1` would change the tool under an already-reviewed caller. That
is intended, and it is org policy already — `uses:` must be a full commit SHA
or it fails the pin-validation consumer CI runs.
SUITE — it was checkable in ways it claimed not to be
- Nothing stopped the cheapest neuter of all: `continue-on-error: true` makes
every `exit 1` advisory, and an `if:` switches the step off. Applied to both
copies they stay byte-identical and every other assertion still passes while
the checkout proceeds on an unvalidated ref. Both are now rejected.
- Comparing TOTAL `::error::` and `exit 1` counts proved the sums match, not the
pairing the assertion's name claimed — one path could degrade to
log-and-continue while another gained a spare exit. Now positional: each
`::error::` must be followed by `exit 1` as the next non-blank line.
- `has`/`no` matched with `case` globbing, so the axis-1 needle
`^[0-9a-fA-F]{40}$` was read as a glob (bracket class, literal braces) and
could be satisfied by text that is not that regex. Now `grep -F`.
- Checkout detection was anchored to one squashed spelling, so
`ref: "${{ inputs.workflows_ref }}"` or a trailing comment was invisible —
the very spellings the comment promised were covered. Now any `ref:` key
naming `inputs.workflows_ref`, deliberately over-inclusive.
- The raw-value ban inspected only `echo` lines. The runner re-parses any step
output, so `printf`, a line continuation, and a redirect into
`$GITHUB_STEP_SUMMARY` were all open. Now every line naming the value must
consume it, never emit it, in any spelling.
- The axis-2 env assertion read the un-stripped body, so a commented-out mapping
would have satisfied it; and the drift loop had no `nullglob` guard, so an
empty slicer would have `cmp`ed a nonexistent file instead of failing cleanly.
Each of the above is mutation-verified, including the two evasions the panel
described: an unguarded checkout written in the quoted spelling, and one
rejection path degraded to log-and-continue while another gains a spare exit.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
…SHA, and close the suite's bypass holes (BE-6307)
Review follow-ups on the `workflows_ref` guard. No change to what the guard
accepts or rejects; the changes are to what it CLAIMS, what it echoes, and what
the structural suite can actually catch.
Docs, where the header overstated the guarantee:
- The COROLLARY said a tag- or branch-pinned `uses:` "cannot satisfy this". It
can: `job_workflow_sha` arrives already RESOLVED, so a `@v1` caller passing
the SHA that tag points at clears both axes. Proving `uses:` was SHA-pinned
needs the caller's original ref, which the called workflow never sees. Reworded
to say so, and to state the real consequence — the inverse of the old text —
that every sanctioned `git tag -f v1 <sha>` move takes such callers red.
- State that nesting is unsupported: in a caller -> org wrapper -> pr-risk chain
`job_workflow_sha` names one commit, so no single `workflows_ref` satisfies
both this guard and the wrapper's pin. Call the workflow directly.
- Document that the guard precedes enablement and is not subject to it (the
resolver is itself loaded from `workflows_ref`), so a drifted pin fails red
even with the `RISK_CONFIG` kill switch set. The switch stops the grading, not
a broken enrollment. README's pr-risk row carries all three.
Guard:
- Sanitize `JOB_WORKFLOW_SHA` through the same tr/truncate as the ref before it
reaches the mismatch annotation. It was only ever tested for emptiness, never
for shape, while the comment beside it argues the platform could reshape that
property out from under us — so a multi-line value could have ended the
annotation early and left the rest to be re-parsed as workflow commands in a
public log. Both copies stay byte-identical.
Suite, each hole verified by mutating the workflow and watching the named
assertion go red:
- Assert `WORKFLOWS_REF: ${{ inputs.workflows_ref }}`. Rebinding it to
`${{ github.job_workflow_sha }}` made the lock-step test a tautology that
passed while the checkout used the unvalidated input.
- Assert the PROTECTED step, not just the guard: no `if:` / `continue-on-error`
on `Load pr-risk tool`, and no job-level `continue-on-error`. `if: always()`
on the checkout ran it after the guard exited 1, guard byte-identical.
- Forbid aliasing the input out of the two shapes the scans understand, so an
`env:` binding, a composite-action `with:`, or a `git fetch` in a `run:` step
cannot route around the "every checkout is guarded" scan.
- Invert the emit scan to a whitelist: the raw value may be sanitized or tested
and nothing else. The blacklist missed one hop (`raw=$WORKFLOWS_REF` then
`echo "$raw"`) and every spelling nobody enumerated (`export`, `declare`,
`read <<<`, `case`).
- Give the `/run: |/` anchor a coverage self-check; reshaping the block scalar
emptied `$script` and both following assertions passed over nothing.
- Strip only WHOLE-LINE comments. The trailing-`#` strip truncated any line
carrying a `#` in a string (`github-workflows#NN`), which could have hidden
the `$WORKFLOWS_REF` mention the emit scan exists to inspect.
mattmillerai
added
cursor-review
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 4 |
| ⚪ Nit | 1 |
Panel: 8/8 reviewers contributed findings.
… statement, cover the runner SHA (BE-6307)
Review round 4 on the `workflows_ref` pin contract. Three holes in the
suite's own claimed guarantees, and one thing the guard cannot see that
the docs did not name.
- ALIAS SCAN, STEP OWNERSHIP. Sanctioning the env-binding SHAPE anywhere
in the file left the bypass open one step to the right: a later step
binding `WORKFLOWS_REF` of its own and running `git fetch origin
"$WORKFLOWS_REF"` adds no `ref:` key, so every count stayed put and an
unguarded fetch of an unvalidated ref shipped green. The binding is now
sanctioned only inside a guard step, and the mention count is `-eq`
rather than `-ge` (still a property — it is nrefs*2, so a third guarded
job moves both sides) so a spare mention cannot ride along unexamined.
- EMIT SCAN, PER STATEMENT. The whitelist cleared a whole LINE on its
prefix, so `if [[ -n "$WORKFLOWS_REF" ]]; then echo "$WORKFLOWS_REF" >>
"$GITHUB_STEP_SUMMARY"; fi` opened with a sanctioned `if [[ ` and
cleared in full, raw emit and all. Lines are split on `;` and each
statement judged alone.
- EMIT SCAN, BOTH VALUES. It covered only `WORKFLOWS_REF`, though the
guard's own comment insists the runner-supplied SHA gets identical
treatment because it is only ever tested for emptiness. Echoing raw
`$JOB_WORKFLOW_SHA` into an annotation would have stayed green. Now
covered, plus a coverage self-check so a renamed env key fails loudly
instead of passing vacuously, and the `${safe_job_sha}` annotation
assertion its sibling already had.
Each of the three is mutation-tested: injecting the bypass turns the
suite red.
- DOCS: `github.job_workflow_sha` also resolves in the CALLER's `with:`,
so `workflows_ref: ${{ github.job_workflow_sha }}` equals itself by
construction and passes both axes forever while `uses:` sits on `@v1`.
Worse than the tag-pinned case already documented, because the
passthrough never needs repinning and so never goes red. The guard
cannot see it — the value arrives identical either way — so it is named
in the header, the input description and the README, with the reviewer
instruction it implies: require a LITERAL 40-hex SHA in the caller's
`with:`, never an expression.
Guard body unchanged; the two copies stay byte-identical.
`\|` and `\?` in a BRE are GNU extensions; `grep -cE` says the same thing portably and reads as the ERE it already was.
mattmillerai
added
cursor-review
Contributor
Author
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟡 Medium | 5 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…not exist (BE-6307)
The lock-step half of the pin guard read `${{ github.job_workflow_sha }}`, which
is not a property of the `github` context — it exists only as an OIDC token
claim. It therefore expanded to the empty string, and the fail-closed `-z` branch
added right below it would have aborted EVERY invocation, correctly pinned
callers included, before the tool checkout. `actionlint` rejects the expression
outright ("property \"job_workflow_sha\" is not defined in object type ...") and
GitHub's context reference lists only `workflow_ref` / `workflow_sha`, neither of
which names the reusable workflow's own commit. Reported by 2 of 8 review models.
So the axis is removed rather than repaired: reading the claim would need an
`id-token: write` grant from every caller plus a token exchange in a job that
holds `permissions: {}`, which is a caller-contract change, not a fix. The header,
the input description and the README now say what is actually enforced — shape,
not provenance — and name review of the caller as the only thing that can bound
which commit the ref points at.
The surviving shape axis is tightened while it is in hand: a length test plus a
"contains a character outside [0-9a-f]" glob replaces `[[ =~ ^[0-9a-fA-F]{40}$ ]]`,
which drops the anchoring question entirely (whether ERE `$` also matches before
a trailing newline is libc-dependent) and rejects uppercase hex, which the regex
accepted and then handed to `actions/checkout` verbatim.
test_pin_contract.sh now PINS the guard's executable body verbatim instead of
characterizing it. Four findings this round were holes in the emit-scan
whitelist — it cleared a whole `;`-statement on its prefix, its trigger was
itself a blacklist, and a `has` needle proves a test is present, never that it is
the only path to the checkout. An equality closes all of them at once: verified
red against a permissive wrapper branch, an `&&`-chained raw emit, `printenv`,
`set -euxo pipefail`, a warning-instead-of-error, a dropped `exit 1`, a loosened
character class, a dropped length test, a raw value in the annotation, and an
`if: false` on the step — and green against a pure rewording of the message,
which is canonicalized out and separately checked to expand nothing but the
sanitized copy. The `nrefs * 2` mention count becomes two per-category checks.
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
2 participants
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.
ELI-5
pr-risk.ymldownloads its own grading code from this repo at whatever ref the caller names inworkflows_ref, and then runs that code holding a token that can write to pull requests. The rule "point that at the exact commit you already pinneduses:to" was written down in a comment and nowhere else — so a caller could point it atmain, at a tag, or atrefs/pull/N/head(which on a public repo resolves to code from somebody's fork) and the run would happily execute it. This PR makes the workflow check: anything that is not a full 40-character commit SHA fails the run before the download happens.What changed
.github/workflows/pr-risk.yml— a newEnforce workflows_ref pin contractstep, first in both jobs that check outworkflows_ref(gateandgrade), rejecting anything that is not^[0-9a-f]{40}$. The input's description, the header's pinned-ref paragraph, and the header's caller-pattern block now say the format is enforced rather than requested.required: trueand the absence of a default are unchanged.README.md— thepr-risk.ymlrow now says the format is ENFORCED, not just that the input is required.The check lives in the workflow, not in
grade-pr-risk.sh/apply-risk-label.sh, because those scripts are what the ref loads — a script-side check would sit inside the blast radius it is meant to bound.Judgment calls (deviations from the ticket — please sanity-check these)
1. The guard is in
gateas well asgrade. The ticket specified thegradejob only; since it was written, #115 added agatejob that checks out the same ref earlier in the run. Guarding onlygradewould have left the run's first execution of caller-named code unguarded. Both copies are byte-identical by design (asserted locally by parsing the YAML), and each job asserts the contract itself rather than inheriting it —gradeskips whengatefails today, but a job should not depend on another job's step for its own trust boundary, and that invariant survives a laterif:or graph re-ordering.gateholdspermissions: {}, so the token stakes are entirely ingrade; the earliness is the value there.2.
[[ =~ ]]instead of the ticket'sgrep -Eq.grepanchors^…$per line, so a multi-line value carrying one SHA-shaped line passes it and is then handed tocheckoutin full.[[ =~ ]]anchors against the whole value. Verified:main\n<valid sha>is rejected by the shipped form. Everything else about the step — theenv:passthrough (no inline${{ }}of the input being validated), the message, the exit code — is as specified.3. Uppercase SHAs are rejected.
^[0-9a-f]{40}$is the ticket's regex and this keeps it.git rev-parse,gh, and the GitHub UI all emit lowercase, every pin in this repo is lowercase, and a false reject here is loud and fixed by lowercasing. Flagging it as a deliberate narrowing rather than an oversight.Caller-breakage check — the ticket's premise was stale, so I re-ran it
BE-6304 concluded "zero callers pass
workflows_reftoday." That is no longer true — two callers now exist. I verified each one's actual pin rather than relying on the absence claim:workflows_refComfy-Org/cloud.github/workflows/ci-pr-risk.ymle4a8f7cd4a073da082b03950136530d4df50738fComfy-Org/comfy-cloud-mcp-server.github/workflows/ci-pr-risk.yml99288dfbf12c084f629746739e160cba882c80f3Both were run through the shipped guard body verbatim and pass. Also re-confirmed: this repo's
test-pr-risk.ymlnever invokes the reusable (it is hermetic overscripts/pr-risk/), a global GitHub code search surfaces no callers outside the org, and thev1tag (4d9cb6b) contains nopr-riskfile at all, so no@v1caller can exist. The conclusion holds — this breaks nobody — but it is now backed by evidence about the callers that exist, not by their absence.One behavior change worth naming:
gateruns regardless ofenabled, so a repo that enrolls withenabled: falseand a mutable pin now fails its check instead of silently no-opping. That is the intended shape — it surfaces a bad pin at enrollment, which is the cheapest moment to fix it — and neither live caller is in that state.Verification
actionlint .github/workflows/pr-risk.yml— clean (shellcheck 0.11 installed, so therun:bodies were linted too).main,v1,refs/pull/7/head, empty,main\n<sha>, a 39-char SHA, a 41-char SHA, and an uppercase SHA all fail with the::error::; both live callers' real SHAs pass.Load pr-risk toolcheckout in every job that has one, the two copies are byte-identical, andworkflows_refis stillrequired: true,type: string, with no default.scripts/pr-risk/suites (the path-filtered CI job this diff triggers):shellcheck -xclean; 24 + 57 + 91 + 21 = 193 tests, 0 failures.check_agents_md.py --root .passes (1 pre-existing CODEOWNERS warning, untouched by this diff).No test changes: this repo has no harness for workflow YAML, and
test-pr-risk.ymlcovers the scripts, which this diff does not touch.