ci(cursor-review): post the review from a job that never checks out PR code - #236
Merged
Merged
Conversation
…R code The `consolidate` job checked out the PR repo, ran `cursor-agent --print --trust` (no `--sandbox`) as the judge over that checkout, and then — in the same job — minted the review bot's App token and ran post-review.py with it. Cursor's print mode can use write and shell tools, so a judge steered by a malicious diff could rewrite the assets checkout or a downloaded action before those later steps ran, with a write-scoped token arriving in the same runner. Split it the way pr-size.yml already splits its comment job: * `consolidate` keeps the PR checkout and the judge, drops `pull-requests: write`, and hands off `consolidated.json`, `judge-findings.json` and a small meta JSON (judge_status / consolidated_count / degraded / triggered_by / ledger download outcome) as the `cursor-review-consolidated` artifact. * `post-review` is new: no PR checkout, only this repo's assets at the pinned `workflows_ref` plus artifacts. It mints the App token, renders the ledger note and posts the review, capped at 10 minutes. The reviewed diff comes from `diff-size`'s `pr-diff` artifact rather than the copy the judge job had shell access to. The invariant is now: no job both checks out PR code and holds a write-scoped credential. `tests/test_workflow_job_isolation.py` pins it structurally so a regression fails CI instead of passing review. Also: * notify-complete's "one consolidated review is on the PR" DM keys on the post job's result, not consolidate's, and names which half broke. * The guarded-ref-checkout census in the workflow-pins suite gains the new job's assets checkout (17, was 16); the lint confirms it carries its guard. * SECURITY.md drops the known-gap carve-out for the judge, which this closes. Consumers pick this up on their next SHA bump; no caller is bumped here.
mattmillerai
added
agent-coded
mattmillerai
marked this pull request as ready for review
August 27, 2026 10:23
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe ChangesCursor review credential isolation
Sequence Diagram(s)sequenceDiagram
participant PRCode
participant consolidate
participant ArtifactStorage
participant post-review
participant GitHub
PRCode->>consolidate: provide checked-out code
consolidate->>ArtifactStorage: upload findings and metadata
post-review->>ArtifactStorage: download payload
post-review->>GitHub: publish review without PR checkout
Suggested reviewers: Merge Risk: 🔵 Low · up to A degraded permission failure can still produce a success Slack message even when no review appears on the pull request. The change is otherwise mergeable, but the notification condition should be tightened so owners are not told that a review landed when it did not. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ 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 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 4 |
| 🟢 Low | 4 |
Panel: 8/8 reviewers contributed findings.
…on test (BE-9569)
Review-panel findings on the credential split, in severity order.
`post-review`'s `if:` was `always() && result != 'skipped' && != 'cancelled'`.
Two holes:
- `always()` runs a job even when the RUN is cancelled. In the common supersede
ordering the documented caller's `cancel-in-progress` fires AFTER consolidate
has finished and uploaded, so its result is still `success` and the
`!= 'cancelled'` test sails past — the poster would then post a review pinned
to the superseded head SHA alongside the new run's. Splitting the post into
its own job widened that window. Now `!cancelled()`, as `over-cap-comment`
already documents for itself.
- Accepting a FAILED consolidate made the artifact spoofable end-to-end.
Artifact names are run-global and the header concedes a prompt-injected panel
cell has shell access; a cell claiming `cursor-review-consolidated` first
makes the real upload fail (v4+ artifacts are immutable), reds consolidate,
and the spoofed payload was what got posted under the bot's write token. Now
gated on `consolidate.result == 'success'`. The degraded-judge path is
unaffected — that step is `continue-on-error`, so consolidate still succeeds.
Also:
- Assert each of the three payload files is present after the download.
`if-no-files-found: error` fires only on ZERO matches, so a partial bundle
uploads clean; the poster then died with an unhandled FileNotFoundError
reading as "the poster is broken" when the cause was upstream.
- Validate `triggered_by` against a GitHub-login shape when reading the meta
file. It is the one header field post-review.py interpolates raw, and since
the split it arrives out of the prompt-injectable job rather than from
`github.actor` in-job — so `@org/team`, `[text](url)` and arbitrary markdown
reached a review posted under the bot's identity.
- Report the two halves of the ledger banner separately. If only the POSTER's
download failed the panel DID have prior-round context, and telling the author
"findings may repeat earlier rounds" there is false.
test_workflow_job_isolation.py, which is what actually pins the invariant:
- Classify a checkout by the VALUE of `repository:`, not the key's presence.
`${{ github.repository }}`, `head.repo.full_name` and `''` all resolve to the
PR under review, and a `repository:`-shaped line in the step's own `run:`
block exempted it too — both assertions then passed vacuously for a job that
checks out PR code AND holds the bot key. Unrecognised values now count as PR
code (stricter, never quieter), and `run:` bodies are stripped first.
- Parse the `permissions:` block instead of matching one exact string, so
`write-all`, `pull-requests: 'write'` and a trailing `# comment` all count.
- Assert PR-checkout jobs declare an EXPLICIT read-only block. The likeliest
regression was deleting the block outright: a `workflow_call` reusable then
inherits the CALLER job's permissions and the documented caller grants
`pull-requests: write`, silently restoring write scope with every test green.
- Assert no workflow-level grant, which every job would inherit.
- Use `code_lines` for the judge premise and the artifact handover, and require
the judge line to START the invocation — the job's header comment and the
step's own `echo "cursor-agent (judge) exit code"` kept both green with the
call deleted.
Doc sync: CONTRIBUTING.md and build-ledger.py's two docstrings still asserted
the pre-split boundary, contradicting SECURITY.md on the same claim.
Verified: seven regressions injected one at a time, each firing the matching
assertion (drop the permissions block; `pull-requests: 'write' # reason` back
on consolidate; `permissions: write-all`; a workflow-level grant; the assets
checkout switched to `${{ github.repository }}`; a run-block forgery of the
exemption; the judge invocation removed). All 10 LEDGER_NOTE branches and 10
triggered_by inputs exercised against the real step bodies extracted from the
YAML. cursor-review 314 tests OK (307 before, +2 new isolation tests);
workflow-pins 308, groom 365, public-repo-hygiene 196, refresh-reviewers 48,
agents-md-integrity 46: all OK. check_workflow_pins.py OK, 0 exempt.
actionlint: the one pre-existing `job.workflow_sha` finding, unchanged.
shellcheck + `bash -n` clean on every run block.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
|
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
The judge model that writes our PR reviews used to run in the same job that held the review bot's private key. That judge reads the PR's diff — attacker-authored text — and Cursor's
--printmode can use write and shell tools, so a diff that talked the judge into rewriting a script on disk would have been rewriting a script that ran moments later with a GitHub App token in the environment. This moves the token mint and the review post into their own job on a fresh runner that never checks out the PR. Nothing about the review a user sees changes.What changed
consolidateandpost-revieware now two jobs, the same shapepr-size.ymlalready uses for itscommentjob andcursor-review-catalog-drift.ymluses for itsissues: writejob.consolidatekeeps thefetch-depth: 0PR checkout and the judge, and dropspull-requests: write— it iscontents: readonly now. Two new steps at the end write a small meta JSON (judge_status,consolidated_count,degraded,triggered_by, and the ledger download's outcome) and upload it alongsideconsolidated.json+judge-findings.jsonas thecursor-review-consolidatedartifact.post-review(new) does noactions/checkoutof the PR repo — onlyComfy-Org/github-workflows@${{ inputs.workflows_ref }}withpersist-credentials: false, behind the sameRequire a pinned workflows_refguard every other checkout site carries. It downloads the payload, mints the App token, renders the ledger note and runspost-review.pyexactly as before.permissions: contents: read+pull-requests: write,timeout-minutes: 10.post-review.pyanchors inline comments against is downloaded fromdiff-size's ownpr-diffartifact, not re-uploaded from the judge's job — so the copy the poster reads is the one the panel reviewed, not one the judge had shell access to.consolidate.Verified:
post-review.pyshells out toghand nothing else — nogit, no dependence on a PR checkout. Every value it needs arrives as a CLI arg or via the artifact.The invariant, stated once: no job in
cursor-review.ymlboth checks out PR code and holds a write-scoped credential..github/cursor-review/tests/test_workflow_job_isolation.py(new, stdlib-only, 9 tests) pins that structurally — it parses the workflow, classifies each job by whether anyactions/checkoutstep resolves to something other than the pinned assets repo, and asserts no such job carries a write-scopedpermissions:grant or referencesBOT_APP_PRIVATE_KEY. Every assertion was confirmed non-vacuous by injecting the matching regression and watching it fire.Carried along, because the split makes them wrong otherwise:
notify-complete's "One consolidated review is on the PR" Slack DM now keys on the post job's result rather thanconsolidate's, and distinguishes the two failures — a green consolidate no longer justifies claiming the review landed..github/workflow-pins/tests/moves 16 → 17 for the new job's assets checkout.check_workflow_pins.pyconfirms it carries its guard (0 exempt).SECURITY.md,CONTRIBUTING.mdandbuild-ledger.py's two docstrings each carried an explicit statement of the pre-split boundary ("It is not absolute for cursor-review's judge…", "a workflow whoseconsolidatejob holdspull-requests: write", "consolidate renders a banner"). All four now describe the split.README.mdcatalog row,.github/cursor-review/README.md,docs/callers/cursor-review.md(the "don't useConsolidate panelas a required check" gotcha now says the same ofPost review, whichneeds:it and skips in all the same cases plus the failure cases).Judgment calls
if: !cancelled() && needs.consolidate.result == 'success'. Both halves are load-bearing.!cancelled()rather thanalways()because the documented caller setscancel-in-progress: trueandalways()runs a job even when the run is cancelled — in the common supersede orderingconsolidatehas already finished and uploaded, so a needs-level!= 'cancelled'test passes and the poster would post against a superseded head SHA alongside the new run's review. Splitting the post into its own job widened that window, since cancellation used to kill the post step mid-job. Same reasoningover-cap-commentalready documents for its own!cancelled(). And== 'success'rather than "ran at all" because this job's entire input is a downloaded artifact: artifact names are run-global, a prompt-injected panel cell can claimcursor-review-consolidatedfirst, v4+ immutability then fails the real upload and redsconsolidate— so a gate that accepted a failed consolidate would post the spoofed payload under the bot's write token.consolidateskips this job rather than producing a second red one. Consequence of the above, and a reversal of this PR's first draft. The failure stays visible asconsolidate's own red status and innotify-complete's DM, which names which half broke; there was never anything worth posting on that path.Require the consolidated payloadis still there as the diagnostic for a transient download failure, and now also asserts each of the three expected files landed —if-no-files-found: errorfires only when ZERO paths match, so a partial bundle uploads clean. A judge that never submitted is not this case: its step iscontinue-on-error, soconsolidatestill succeeds and the degraded panel-union review still posts.ledger-note.txtout of the judge's job,post-reviewdownloads the pristinecursor-review-ledgerartifact itself. The banner's delivery check then covers both questions instead of one, and reports them separately, because they mean different things to the author: if the judge's download failed the panel really did review without prior-round context ("findings may repeat earlier rounds"); if only the poster's download failed — a case that exists only because this job fetches the note itself — the panel and judge had the context and only the header could not be rendered, so saying "findings may repeat" there would be false.triggered_byis validated on the way out of the meta file (^[A-Za-z0-9-]{1,39}(\[bot\])?$,[bot]becauseResolve triggererlegitimately emits it). It is the one review-header fieldpost-review.pyinterpolates raw rather than throughneutralize_mentions, which was safe while the value came fromgithub.actorin-job and stopped being safe once it started arriving out of the prompt-injectable job. A rejected value degrades to no attribution line, not to a wrong one.needs: [consolidate, ledger]rather than alsogate. Nothing in the job readsgate's outputs, and a skippedgatealready skipsconsolidate, which skips this.ledgeris needed forneeds.ledger.outputs.status.post-reviewinstead of a red one) fires only where the old code already produced no review.Residual
grep -nshowsecrets.BOT_APP_PRIVATE_KEYonly inpost-review. It also appears inover-cap-comment, a job that did not exist when the ticket was written. That job checks out nothing at all, so the boundary the criterion protects holds — but if you grep expecting one job, you will find two. The test asserts the accurate form (holders == {over-cap-comment, post-review}, neither with a PR checkout) rather than the literal one.notify-complete's success DM can still overstate what landed, and this is deferred to a follow-up rather than fixed here.post-review.pydeliberately exits 0 on a read-only-token 403 after writing the review only to$GITHUB_STEP_SUMMARY(and does the same on several otherPOST_FAILED_SUMMARY_NOTEpaths), so a greenpost-reviewjob does not strictly mean the review is on the PR. This PR did not introduce the gap — keying the DM onconsolidatebeforehand was wrong in exactly the same way — it only moved which job the DM reads. Closing it means changingpost-review.py's exit contract to emit a posted-versus-degraded value, surfacing it as a job output, and extendingtest_post_review.py; that is a separate concern from the isolation boundary.ci-cursor-review.ymldeliberately pins a merged-mainSHA rather than a local path (AGENTS.md: "do not 'simplify' them to a path"), so the cursor-review run on this PR executes the old, unsplit workflow; no consumer can reach the new shape until this lands and their SHA bump merges. What has been verified instead: the workflow parses,actionlintreports no new finding, everyrun:block passesbash -nandshellcheck, all three new/changed inline step bodies were executed locally against the real text extracted from the YAML (the payload-completeness guard, all 10LEDGER_NOTEbranches, and 10triggered_byinputs including markdown-link and team-mention attempts), and every isolation assertion was fired by injecting its regression. The first real consumer run after the bump is the acceptance test, and the thing to watch is thePost reviewjob: an artifact-plumbing mistake surfaces there as a red job with no review posted, not as a silently wrong review.@676fbb6L1503–1900,--approve-mcps, and aledger-note.txtupload). Two plan items turned out to be no-ops against the current tree and were not done:post-review.pyneeds no change (it never shells out togit), andtests/test_wire_bot_identity.pyasserts on the caller-wiring helper, not on job names or steps, so there was nothing in it to update. The new isolation suite is what covers the job structure instead..github/coderabbit-config's suite was not run: it needsjsonschemafrom its pinnedrequirements.txt, which is not installed in this environment. That directory is untouched by this diff, so this is a not-run, not a known failure.cursor-review.ymlalone.groom.ymlis documented as already credential-split andpr-size.ymlis the pattern this copies, but neither was re-verified here, andpr-area-label.yml,pr-derisk.ymlandlinear-ticket.ymlwere not looked at.Provenance
python3 -m unittest discover -s .github/cursor-review/tests: 314 passed, 0 failed (305 on base, +9 isolation tests);.github/workflow-pins/tests: 308 passed;.github/groom: 365;.github/public-repo-hygiene: 196;.github/refresh-reviewers: 48;.github/agents-md-integrity: 46 — all OK.python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 exempt;python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (1 pre-existing CODEOWNERS warning);actionlint .github/workflows/cursor-review.yml: 1 finding, byte-identical to the one onorigin/main(job.workflow_sha, in the untouched ledger job);shellcheck+bash -nclean on everyrun:block in the file. Isolation suite proven non-vacuous against 7 injected regressions (permissions block deleted;pull-requests: 'write' # commentrestored onconsolidate;permissions: write-all; a workflow-level grant; the assets checkout switched to${{ github.repository }}; arun:-block forgery of therepository:exemption; the judge invocation removed).consolidatenow skips the poster rather than reddening it; that thenotify-completeDM's posted-versus-degraded gap is deferred to a follow-up; and that the end-to-end consumer-run criterion cannot be exercised before merge.