Repository navigation
fix(pr-validation): close two paths past the breaking-change gate - #940
Merged
bedatty merged 3 commits intoOct 20, 2026
Merged
Conversation
Reported by @fredcamaral on #926 and reproduced. The newer-run check counted any newer run of this workflow on the branch, so: dispatch a run by hand, cancel the pending approval, and the verdict reads `superseded` - which exits 0. A green gate with no approval, available to anybody with write access. `self-pr-validation.yml` enables `workflow_dispatch` and skips validation on it, so the dispatched run does not even have to do anything. The `(.pull_requests | length) == 0` arm is mine, from #932. CodeRabbit proposed the strict filter there and I widened it so cancellations on forks would not read as unresolved. That is the hole: it accepted any run with no pull request attached, which is exactly what a dispatched run looks like. Runs must now be `pull_request`, filtered both as a query parameter and in the selection - the parameter is the server's promise, and this decides whether a merge is allowed. Forks still have no `pull_requests` and are matched on the head instead, which ties the run to this revision rather than to the branch at large. Verified by reverting the filter: the three dispatched and scheduled cases pass as superseded again, and the fork case stops distinguishing heads.
…ecord Reported by @fredcamaral on #926. The reuse lookup took the fingerprint from the record itself, so it only ever established that *an* approval happened on this branch - never what that approval covered. The fingerprint is derived from public commit SHAs, so it is computable before the commit exists. A member posts a record carrying fp(A,B) that points at the run which approved only A, then pushes the breaking commit B. Every check passed: the record is from a MEMBER, the run is on this branch, the run really did carry an Environment approval. The verdict came back approved, exit 0, no reviewer involved. Commenting needs no write access. The record now says only which run to ask, and the run answers. `breaking-change-commits` gains an `at-sha` mode that resolves the set from a comparison against a past revision - the pull request's commits endpoint cannot, it always reports the current state - and the lookup points it at the approving run's own `head_sha`, which nobody can write. Reuse requires that resolved fingerprint to equal this one. Three ways to not reuse, all of which ask the reviewer again: no verified record, an unresolvable approved set (deleted run, unreadable comparison, past the commit cap), or a set that differs. Not knowing is not evidence that an approval still applies. The 250-commit refusal applies to the comparison too, measured against `total_commits`. Answering from a truncated one matters more here than anywhere else: an omitted breaking commit would leave the fingerprint unchanged and the approval would be honoured for a change it never covered. Verified by removing the comparison: the forged case passes again.
|
Warning Review limit reachedThe included review limit has been reached and this organization has disabled usage-based review continuation. Wait for reviews to reset or ask a billing admin to change After included review limits.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Next included review available in 43 minutes. View limit details
WalkthroughThe workflow now verifies a candidate approval against the breaking-commit fingerprint resolved at the approving run’s head. The resolver supports revision-specific comparisons. Supersession checks now filter newer runs by workflow, event type, and pull request. ChangesBreaking-change approval
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. (4 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change: closing two approval-gate paths in PR validation. Description check The description is complete and directly addresses the two security fixes, behavior changes, breaking-change impact, testing, affected workflows, and related issue. It also records that a real caller … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and directly addresses the two security fixes, behavior changes, breaking-change impact, testing, affected workflows, and related issue. It also records that a real caller workflow run was not performed.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
lerian-studio
added
size/L
Contributor
🔍 Lint Analysis
|
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
Contributor
🛡️ CodeQL Analysis ResultsLanguages analyzed: ✅ No security issues found. 🔍 View full scan logs | 🛡️ Security tab |
This comment has been minimized.
This comment has been minimized.
lerian-studio
added
the
review-ready
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CodeRabbit on #940. The `at-sha` path reads the comparison API, which is repository contents rather than pull request data, while the token description and the README still said `pull-requests: read` alone. The failure mode is the quiet kind: a caller granting exactly the documented scope gets a failed comparison, the gate reads that as "cannot confirm what that approval covered", and the reviewer is asked again on every push with nothing saying why. The step is `continue-on-error`, so there is no red check to follow either. `breaking-change-approval.yml` already grants `contents: read` to its `request` job, so the gate itself was never affected - direct callers of the composite were. A test now pins that grant, since the reuse path depends on it and losing it would fail the same silent way.
Contributor
|
@coderabbitai review |
|
github-actions
Bot
deleted the
fix/gate-forged-reuse-and-dispatch-supersession
branch
October 20, 2026 13:39
This was referenced Oct 20, 2026
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
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.
Description
The two findings @fredcamaral reproduced at
7c0c8b5on #926. Both are real, both reproduce, and both let someone inside the organisation pass the gate with no approval. Neither reachedmain— #926 was closed rather than merged — but both are indevelopand in thev1.86.0-betapre-releases.🔴 A forged record could reuse someone else's approval
breaking-change-approval.ymltook the fingerprint from the record itself. Everything it verified established that an approval happened on this branch; nothing established what that approval covered.The fingerprint is derived from public commit SHAs, so it is computable before the commit exists:
fp(A,B)for the breaking commitBthey are about to push.A.B.Every check passed — the record is from a
MEMBER, the run is on this branch, the run really did carry an Environment approval forbreaking-approval— and the verdict came backapproved,exit 0, with no reviewer involved. Commenting needs no write access.The record now says only which run to ask, and the run answers.
breaking-change-commitsgains anat-shamode that resolves the breaking set by comparingbase-ref...at-sha. The pull request's commits endpoint cannot do this — it always reports the current state.head_sha, which nobody can write, and hands over a candidate rather than a verdict.Three ways to not reuse, all of which ask the reviewer again: no verified record, an unresolvable approved set (deleted run, unreadable comparison, past the commit cap), or a set that differs. Not knowing is not evidence that an approval still applies.
The 250-commit refusal applies to the comparison too, measured against
total_commits. It matters more here than anywhere else: an omitted breaking commit would leave the fingerprint unchanged, and the approval would be honoured for a change it never covered.🟠 A dispatched run could pass a cancelled approval as superseded
The newer-run check counted any newer run of this workflow on the branch. Dispatch one by hand, cancel the pending approval, and the verdict reads
superseded— which exits 0.pr-validation.ymlreads only the exit code, so theresult=failuresitting in the outputs blocks nothing. A green gate with no approval, for anybody with write access.self-pr-validation.ymlenablesworkflow_dispatchand skips validation on it, so the dispatched run does not even have to do anything.The
(.pull_requests | length) == 0arm is mine, from #932. CodeRabbit proposed the strict filter there and I widened it so cancellations on forks would not read as unresolved. That is the hole: it accepted any run with no pull request attached, which is exactly what a dispatched run looks like.Runs must now be
pull_request— filtered both as a query parameter and in the selection, since the parameter is the server's promise and this decides whether a merge is allowed. Forks still have nopull_requestsand are matched on the head instead, which ties the run to this revision rather than to the branch at large.Type of Change
feat: New workflow or new input/output/step in an existing workflowfix: Bug fix in a workflow (incorrect behavior, broken step, wrong condition)perf: Performance improvement (e.g. caching, parallelism, reduced steps)refactor: Internal restructuring with no behavior changedocs: Documentation only (README, docs/, inline comments)ci: Changes to self-CI (workflows under.github/workflows/that run on this repo)chore: Dependency bumps, config updates, maintenancetest: Adding or updating testsBreaking Changes
None.
at-shaandbase-refare new optional inputs onbreaking-change-commits, both defaulting to empty, and empty keeps the existing path exactly. No reusable-workflow input or output changes.An approval granted before this merges is still reused, provided it covers the current set — which is the whole point. What stops being reused is one that never covered it.
Testing
@this-branchor the beta tagactionlintclean,shellcheckclean,typosclean. Suites: 112 workflow tests, 60 detector, 32 gate-scope, 28 commit-list (6 new), 22 CODEOWNERS.Both attacks are encoded as tests, and both were verified by mutation rather than by passing:
test_a_forged_record_cannot_reuse_someone_elses_approval— runs the confirmation step withAPPROVED_FINGERPRINT=fp(A)andFINGERPRINT=fp(A+B), which is the forged case, and requiresfalse. Removing the comparison makes it pass again.test_a_dispatched_run_is_not_supersession—workflow_dispatchwith and without a pull request attached, plusschedule. Removing the event filter makes all three pass as superseded again.Plus
test_the_approved_set_is_read_from_the_run_not_the_record(the lookup no longer decides reuse by itself),test_a_run_from_a_fork_is_matched_on_the_head_not_the_branch, and six cases in the composite suite covering theat-shapath, its own cap refusal, and that adding a breaking commit changes the fingerprint.Related Issues
Addresses #926 (comment).
Two things @fredcamaral listed as unchecked are still unchecked here: the
outcomejob after a whole run is cancelled by hand (it relies onif: always()), and whether GitHub reportsMEMBERfor private organisation memberships — which would widen who can write an accepted record. Worth a follow-up; the fix above does not depend on either, since the record's author no longer determines what gets reused.