From 4106192887a08fbe46b3bdf5491f682e9e7b6bf5 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:58:10 -0300 Subject: [PATCH 1/3] fix(pr-validation): only a pull_request run can supersede an approval 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. --- .../workflows/breaking-change-approval.yml | 32 +++++-- .../breaking-change-guard/test-workflow.py | 87 ++++++++++++++++--- 2 files changed, 101 insertions(+), 18 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index f00749e5..c009097c 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -801,11 +801,25 @@ jobs: # # One head branch can carry more than one pull request — same # branch, different bases — so the workflow scope alone still lets - # another pull request's run stand in for this one's. `.pull_requests` - # carries the run's own pull request and settles it. It comes back - # empty for runs from forks, and there the branch scope is the best - # evidence left: a run of this workflow, on this head branch, newer - # than this one. + # another pull request's run stand in for this one's. + # `.pull_requests` carries the run's own pull request and settles + # it. + # + # And only a `pull_request` run counts. Without that filter any + # newer run of this workflow on the branch qualified, including one + # somebody started by hand: dispatch a run, cancel the pending + # approval, and the verdict reads `superseded`, which exits 0 — a + # green gate with no approval, available to anybody with write + # access. Reported by @fredcamaral and reproduced; the `event` + # filter is applied twice, once as a query parameter and once in + # the selection, because the parameter alone is the server's + # promise and this decides whether a merge is allowed. + # + # `.pull_requests` comes back empty for runs from forks. Those are + # matched on the head instead, which still ties the run to this + # revision rather than to the branch at large — the empty arm used + # to accept any run with no pull request attached, which is what + # let a dispatched run through. workflow_id=$( gh api "repos/${REPO}/actions/runs/${RUN_ID}" --jq '.workflow_id' 2>/dev/null || true ) @@ -813,11 +827,13 @@ jobs: newer=0 if [ -n "$workflow_id" ]; then newer=$( - gh api "repos/${REPO}/actions/workflows/${workflow_id}/runs?branch=${HEAD_REF}&per_page=50" \ + gh api "repos/${REPO}/actions/workflows/${workflow_id}/runs?branch=${HEAD_REF}&event=pull_request&per_page=50" \ --jq "[.workflow_runs[] | select(.id > ${RUN_ID}) - | select((.pull_requests | length) == 0 - or any(.pull_requests[]; .number == ${PR_NUMBER}))] + | select(.event == \"pull_request\") + | select(any(.pull_requests[]?; .number == ${PR_NUMBER}) + or ((.pull_requests | length) == 0 + and .head_sha == \"${HEAD_SHA}\"))] | length" 2>/dev/null || echo 0 ) fi diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 9f011c94..0b562fff 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -347,7 +347,12 @@ def run_verdict(**overrides): env["MOCK_RUNS_JSON"] = json.dumps( { "workflow_runs": [ - {"id": run_id + 1 + i, "pull_requests": [{"number": pr_number}]} + { + "id": run_id + 1 + i, + "event": "pull_request", + "head_sha": env["HEAD_SHA"], + "pull_requests": [{"number": pr_number}], + } for i in range(int(env["MOCK_NEWER_RUNS"])) ] } @@ -1510,8 +1515,8 @@ def test_a_newer_run_of_another_pull_request_is_not_supersession(self): # pull request it knows nothing about. runs = { "workflow_runs": [ - {"id": 101, "pull_requests": [{"number": 77}]}, - {"id": 102, "pull_requests": [{"number": 77}]}, + {"id": 101, "event": "pull_request", "pull_requests": [{"number": 77}]}, + {"id": 102, "event": "pull_request", "pull_requests": [{"number": 77}]}, ] } _, other_pr = run_verdict( @@ -1520,22 +1525,84 @@ def test_a_newer_run_of_another_pull_request_is_not_supersession(self): self.assertEqual("unresolved", other_pr["state"]) # Mixed: the same two, plus one that is actually ours. - runs["workflow_runs"].append({"id": 103, "pull_requests": [{"number": 42}]}) + runs["workflow_runs"].append( + {"id": 103, "event": "pull_request", "pull_requests": [{"number": 42}]} + ) _, ours = run_verdict( APPROVAL_RESULT="cancelled", MOCK_RUNS_JSON=json.dumps(runs) ) self.assertEqual("superseded", ours["state"]) - def test_a_run_without_pull_requests_falls_back_to_the_branch_scope(self): - # Runs from forks come back with `pull_requests: []`. Dropping those - # would make every cancellation on a fork unresolved; keeping them - # leaves the branch scope as the evidence, which is what there is. - runs = {"workflow_runs": [{"id": 101, "pull_requests": []}]} + def test_a_run_from_a_fork_is_matched_on_the_head_not_the_branch(self): + # Runs from forks come back with `pull_requests: []`. Dropping them + # would make every cancellation on a fork unresolved; accepting any run + # that has none is how a dispatched run passed for a pull request it + # had nothing to do with. They are matched on the head instead, which + # ties the run to this revision. + same_head = { + "workflow_runs": [ + { + "id": 101, + "event": "pull_request", + "head_sha": "abc123", + "pull_requests": [], + } + ] + } _, fork = run_verdict( - APPROVAL_RESULT="cancelled", MOCK_RUNS_JSON=json.dumps(runs) + APPROVAL_RESULT="cancelled", MOCK_RUNS_JSON=json.dumps(same_head) ) self.assertEqual("superseded", fork["state"]) + other_head = json.loads(json.dumps(same_head)) + other_head["workflow_runs"][0]["head_sha"] = "def456" + _, unrelated = run_verdict( + APPROVAL_RESULT="cancelled", MOCK_RUNS_JSON=json.dumps(other_head) + ) + self.assertEqual("unresolved", unrelated["state"]) + + def test_a_dispatched_run_is_not_supersession(self): + # Reported by @fredcamaral and reproduced: with no event filter, any + # newer run of this workflow on the branch counted. Dispatch one by + # hand, cancel the pending approval, and the verdict reads + # `superseded` — which exits 0. A green gate with no approval, for + # 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. + for description, run in ( + ( + "dispatched, no pull request attached", + {"id": 101, "event": "workflow_dispatch", "head_sha": "abc123", + "pull_requests": []}, + ), + ( + "dispatched, naming this pull request", + {"id": 101, "event": "workflow_dispatch", "head_sha": "abc123", + "pull_requests": [{"number": 42}]}, + ), + ( + "a scheduled run on the same branch", + {"id": 101, "event": "schedule", "head_sha": "abc123", + "pull_requests": []}, + ), + ): + with self.subTest(description): + _, outputs = run_verdict( + APPROVAL_RESULT="cancelled", + MOCK_RUNS_JSON=json.dumps({"workflow_runs": [run]}), + ) + self.assertEqual("unresolved", outputs["state"], description) + + # And `unresolved` must not pass, which is the half that makes the + # filter matter. + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Record the verdict" + ) + self.assertIn('[ "$STATE" = "superseded" ]', body) + self.assertNotIn('[ "$STATE" = "unresolved" ]', body) + def test_a_reused_approval_must_belong_to_this_pull_request(self): step = extract_step( extract_job(APPROVAL_WORKFLOW, "request"), From da3bf82ce9240b2e88dfb71f1ed7acbb51757a9b Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:04:47 -0300 Subject: [PATCH 2/3] fix(pr-validation): resolve a reused approval from the run, not the record 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. --- .../workflows/breaking-change-approval.yml | 102 ++++++++++++++++-- docs/breaking-change-approval.md | 20 +++- .../breaking-change-commits/README.md | 20 ++++ .../breaking-change-commits/action.yml | 14 +++ .../breaking-change-commits/resolve.sh | 43 +++++++- src/validate/breaking-change-commits/test.sh | 94 ++++++++++++++++ .../breaking-change-guard/test-workflow.py | 88 ++++++++++++++- 7 files changed, 365 insertions(+), 16 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index c009097c..c299af5b 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -405,7 +405,7 @@ jobs: contents: read pull-requests: write outputs: - already-approved: ${{ steps.prior.outputs.already-approved }} + already-approved: ${{ steps.reuse.outputs.already-approved }} approved-by: ${{ steps.prior.outputs.approved-by }} steps: # Has this exact breaking change already been approved on this pull @@ -435,7 +435,8 @@ jobs: run: | set -euo pipefail - already=false + candidate_run="" + candidate_head="" approved_by="" approving_run="" unverified="" @@ -498,10 +499,17 @@ jobs: # Matched on the branch, because `pull_requests` on a run comes # back empty in practice — verified against run 38967637198, which # really did carry an approval for this gate. - run_branch=$( + # + # The run's own head is read here and nowhere else. It is the only + # unforgeable statement of *what* that approval covered: the + # fingerprint in the record says what the writer claims it covered, + # and anybody who can read the repository can write a record. + run_facts=$( gh api "repos/${REPO}/actions/runs/${approving_run}" \ - --jq '.head_branch // ""' 2>/dev/null || true + --jq '[(.head_branch // ""), (.head_sha // "")] | @tsv' 2>/dev/null || true ) + run_branch=$(printf '%s' "$run_facts" | cut -f1) + run_head=$(printf '%s' "$run_facts" | cut -f2) if [ -z "$run_branch" ] || [ "$run_branch" != "$HEAD_REF" ]; then unverified="${unverified:+$unverified }${approving_run}" @@ -516,8 +524,12 @@ jobs: | .user.login' 2>/dev/null || true ) - if [ -n "$approvers" ]; then - already=true + if [ -n "$approvers" ] && [ -n "$run_head" ]; then + # A candidate, not a verdict. What this run approved is still + # unknown — the next step resolves the breaking set as it stood + # at `run_head` and only then is the record believed. + candidate_run="$approving_run" + candidate_head="$run_head" approved_by="$approvers" break fi @@ -534,9 +546,10 @@ jobs: echo " fingerprint : ${FINGERPRINT}" echo " records found : $(grep -c . <<< "$records" || true)" echo " unverified runs : [${unverified:-}]" - echo " already approved : ${already}${approved_by:+ (by ${approved_by})}" - elif [ "$already" = "true" ]; then - echo "::notice::This breaking change was approved on run ${approving_run} by ${approved_by} — not asking again" + echo " candidate run : ${candidate_run:-none}${approved_by:+ (by ${approved_by})}" + echo " candidate head : ${candidate_head:-none}" + elif [ -n "$candidate_run" ]; then + echo "::notice::Run ${candidate_run} carried an approval for this branch — checking what it approved" elif [ -n "$records" ]; then echo "::warning::A recorded approval of ${FINGERPRINT} exists but could not be verified against the Environment log (runs: ${unverified:-none}). Asking again." fi @@ -558,10 +571,77 @@ jobs: approved_by=$(printf '%s' "$approved_by" | awk 'NF' | paste -sd, -) { - echo "already-approved=$already" + echo "candidate-run=${candidate_run}" + echo "candidate-head=${candidate_head}" echo "approved-by=${approved_by}" } >> "$GITHUB_OUTPUT" + # What did that run actually approve? Resolved from the run's own head, + # which nobody can write, rather than from the fingerprint in the record, + # which anybody who can read the repository can write. + # + # @fredcamaral demonstrated the gap on #926. The fingerprint is derived + # from public commit SHAs, so it is computable in advance: a member posts + # a record carrying fp(A,B) and pointing at the run that approved only A, + # then pushes the breaking commit B. Every check up to here passes — the + # record is from a MEMBER, the run is on this branch, the run really did + # carry an approval — and nothing asked what it approved. The verdict + # came back `approved`, exit 0, no reviewer involved. + - name: Resolve what the approving run approved + id: approved-set + if: steps.prior.outputs.candidate-head != '' + continue-on-error: true + uses: LerianStudio/github-actions-shared-workflows/src/validate/breaking-change-commits@develop + with: + github-token: ${{ secrets.MANAGE_TOKEN || github.token }} + pr-number: ${{ needs.detect.outputs.pr-number }} + at-sha: ${{ steps.prior.outputs.candidate-head }} + base-ref: ${{ needs.detect.outputs.base-ref }} + + - name: Confirm the approval covers this change + id: reuse + env: + CANDIDATE_RUN: ${{ steps.prior.outputs.candidate-run }} + CANDIDATE_HEAD: ${{ steps.prior.outputs.candidate-head }} + APPROVED_BY: ${{ steps.prior.outputs.approved-by }} + APPROVED_FINGERPRINT: ${{ steps.approved-set.outputs.fingerprint }} + RESOLVE_OUTCOME: ${{ steps.approved-set.outcome }} + FINGERPRINT: ${{ needs.detect.outputs.fingerprint }} + PR_NUMBER: ${{ needs.detect.outputs.pr-number }} + DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} + run: | + set -euo pipefail + + already=false + + # Three ways to not reuse, and all of them ask the reviewer again. + # Asking twice costs attention; reusing an approval that covered + # something else costs the gate. + if [ -z "$CANDIDATE_RUN" ]; then + reason="no verified approval on record" + elif [ "$RESOLVE_OUTCOME" != "success" ] || [ -z "$APPROVED_FINGERPRINT" ]; then + reason="could not resolve what run ${CANDIDATE_RUN} approved" + elif [ "$APPROVED_FINGERPRINT" != "$FINGERPRINT" ]; then + reason="run ${CANDIDATE_RUN} approved ${APPROVED_FINGERPRINT}, this is ${FINGERPRINT}" + else + already=true + reason="run ${CANDIDATE_RUN} approved ${APPROVED_FINGERPRINT}, unchanged" + fi + + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::DRY RUN — approval reuse" + echo " this change : ${FINGERPRINT:-none}" + echo " approved at : ${CANDIDATE_HEAD:-none}" + echo " that change was : ${APPROVED_FINGERPRINT:-unresolved}" + echo " reusing : ${already} — ${reason}" + elif [ "$already" = "true" ]; then + echo "::notice::PR #${PR_NUMBER} — ${reason}; not asking ${APPROVED_BY} again" + elif [ -n "$CANDIDATE_RUN" ]; then + echo "::warning::Not reusing the approval on PR #${PR_NUMBER} — ${reason}." + fi + + echo "already-approved=$already" >> "$GITHUB_OUTPUT" + # Read from the BASE ref, never the head: a CODEOWNERS taken from the pull # request branch would let an author edit who is allowed to approve their # own breaking change. @@ -628,7 +708,7 @@ jobs: needs.detect.result == 'success' && needs.detect.outputs.in-scope == 'true' && needs.detect.outputs.has-breaking-changes == 'true' - && steps.prior.outputs.already-approved != 'true' + && steps.reuse.outputs.already-approved != 'true' && !inputs.dry_run uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop with: diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 790f362e..77e81466 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -300,19 +300,35 @@ pull request, and the fingerprint is computable from public data: the commit SHAs are on the pull request and the digest is defined in this repository. A record on its own is therefore something a stranger can write. -So two things must hold before an approval is reused: +So three things must hold before an approval is reused: 1. the record sits in a comment from an **owner or member** of the organisation, or from `github-actions[bot]` — the identity the reporter writes under when no `MANAGE_TOKEN` is passed, and one nobody but Actions can author as 2. the **run it names really was approved** for the `breaking-approval` - Environment, read from the Environment's own approval log + Environment, read from the Environment's own approval log, and that run is + on this head branch +3. the **set that run approved** is this set — resolved from the run's own + `head_sha`, never from the fingerprint written in the record Every record for the fingerprint is tried, not only the first: runs are deleted on the repository's retention schedule, so the oldest record goes unverifiable while a newer one is still good. +The third was missing until @fredcamaral demonstrated what it cost. The first +two establish that *an* approval happened on this branch; neither says what it +approved. Since the fingerprint is derived from public commit SHAs it is +computable before the commit exists, so a member could post a record carrying +`fp(A,B)` pointing at the run that approved only `A`, then push the breaking +commit `B` — and the gate would read "already approved" and return success with +no reviewer involved. Resolving the approved set from the run's own head closes +it: the record now says only *which run to ask*, and the run answers. + +When that resolution fails — a deleted run, an unreadable comparison, a pull +request past the commit cap — the approval is not reused. Not knowing is not +evidence that it still applies. + Only a run whose Environment approval actually happened writes a record. A reused approval reports `approved` with the gate job skipped, and a record naming *that* run would assert something no log can confirm — then outlive the diff --git a/src/validate/breaking-change-commits/README.md b/src/validate/breaking-change-commits/README.md index 5e8e7122..3d16ac25 100644 --- a/src/validate/breaking-change-commits/README.md +++ b/src/validate/breaking-change-commits/README.md @@ -87,6 +87,24 @@ and decides; this reads the API because the jobs that need it have no checkout. They must agree, which is why the patterns are kept identical — and the tests here cover the same negative cases. +## Resolving at a past revision + +`at-sha` (with `base-ref`) resolves the set as it stood at that revision +instead of at the pull request's current head, by comparing `base-ref...at-sha` +rather than reading the pull request's commits — which always report the +current state. + +The gate uses it for one question: *what did that approval approve?* The +fingerprint written in the approval record answers a different one — what the +writer claims it covered — and anybody who can read the repository can write a +record. Since the fingerprint is derived from public commit SHAs it is also +computable in advance, so a record naming a real approved run could be posted +before the breaking commit it claims to cover even exists. Resolving from the +approving run's own head closes that: the head is not writable. + +The 250 cap applies here too, measured against `total_commits`, and is refused +the same way. + ## Inputs | Input | Description | Required | Default | @@ -94,6 +112,8 @@ here cover the same negative cases. | `github-token` | Token used to read the commits. Needs `pull-requests: read`. | Yes | — | | `pr-number` | Pull request whose commits are listed. | Yes | — | | `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | +| `at-sha` | Resolve the set as it stood at this revision instead of at the current head. Requires `base-ref`. | No | `''` | +| `base-ref` | Base of the comparison when `at-sha` is given. | No | `''` | ## Outputs diff --git a/src/validate/breaking-change-commits/action.yml b/src/validate/breaking-change-commits/action.yml index 089b4122..0be76574 100644 --- a/src/validate/breaking-change-commits/action.yml +++ b/src/validate/breaking-change-commits/action.yml @@ -12,6 +12,18 @@ inputs: description: 'Repository owning the pull request, as `owner/name`.' required: false default: ${{ github.repository }} + at-sha: + description: >- + Resolve the set as it stood at this revision instead of at the pull + request's current head. Requires `base-ref`. The gate uses it to ask what + an approval actually approved, rather than believing a fingerprint + written in a comment. + required: false + default: '' + base-ref: + description: 'Base of the comparison when `at-sha` is given — the pull request''s base branch.' + required: false + default: '' outputs: count: @@ -37,6 +49,8 @@ runs: GH_TOKEN: ${{ inputs.github-token }} REPO: ${{ inputs.repository }} PR_NUMBER: ${{ inputs.pr-number }} + AT_SHA: ${{ inputs.at-sha }} + BASE_REF: ${{ inputs.base-ref }} run: | set -euo pipefail bash "${GITHUB_ACTION_PATH}/resolve.sh" >> "${GITHUB_OUTPUT}" diff --git a/src/validate/breaking-change-commits/resolve.sh b/src/validate/breaking-change-commits/resolve.sh index 1bf07ed9..7d332825 100755 --- a/src/validate/breaking-change-commits/resolve.sh +++ b/src/validate/breaking-change-commits/resolve.sh @@ -23,6 +23,13 @@ set -euo pipefail GH_TOKEN=${GH_TOKEN:-} REPO=${REPO:-} PR_NUMBER=${PR_NUMBER:-} +# Optional. Resolves the set as it stood at a given revision instead of at the +# pull request's current head — which is what lets the gate ask the question it +# actually needs answered: what did that approval approve? Reading the +# fingerprint out of a comment answers a different one, and a comment is +# writable by anybody who can read the repository. +BASE_REF=${BASE_REF:-} +AT_SHA=${AT_SHA:-} export GH_TOKEN @@ -38,7 +45,34 @@ for binary in gh jq; do fi done -if ! commits=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \ +if [[ -n "${AT_SHA}" ]]; then + if [[ -z "${BASE_REF}" ]]; then + printf 'Breaking Change Commits: BASE_REF is required with AT_SHA.\n' >&2 + exit 1 + fi + + # `compare` answers for a revision that is no longer the head. The pull + # request's own commits endpoint cannot: it always reports the current state. + if ! comparison=$(gh api "repos/${REPO}/compare/${BASE_REF}...${AT_SHA}" 2>/dev/null); then + printf 'Breaking Change Commits: could not compare %s...%s in %s.\n' \ + "${BASE_REF}" "${AT_SHA}" "${REPO}" >&2 + exit 1 + fi + + # Same cap, same refusal. `compare` returns at most 250 commits and declares + # the real total separately, so a truncated answer is detectable — and + # answering from one would be worse here than anywhere else, since this + # decides whether an approval is reused. + declared_total=$(jq -r '.total_commits // 0' <<< "${comparison}") + returned_total=$(jq -r '.commits | length' <<< "${comparison}") + if [[ "${declared_total}" -ne "${returned_total}" ]]; then + printf 'Breaking Change Commits: %s...%s has %s commits but the comparison returned %s.\n' \ + "${BASE_REF}" "${AT_SHA}" "${declared_total}" "${returned_total}" >&2 + exit 1 + fi + + commits=$(jq -r '.commits[] | [.sha, .commit.message] | @base64' <<< "${comparison}") +elif ! commits=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate \ --jq '.[] | [.sha, .commit.message] | @base64' 2>/dev/null); then printf 'Breaking Change Commits: could not read the commits of PR #%s in %s.\n' \ "${PR_NUMBER}" "${REPO}" >&2 @@ -53,10 +87,15 @@ fi # covered. # # Refuse instead. A gate that cannot see the whole change says so. +# +# Only for the pull-request path: the comparison above does its own version of +# this check, against `total_commits`, because it has no pull request to ask. returned=$(grep -c . <<< "${commits}" || true) [[ -z "${commits}" ]] && returned=0 -if ! declared=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.commits' 2>/dev/null); then +if [[ -n "${AT_SHA}" ]]; then + declared="${returned}" +elif ! declared=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.commits' 2>/dev/null); then printf 'Breaking Change Commits: could not read the commit count of PR #%s in %s.\n' \ "${PR_NUMBER}" "${REPO}" >&2 exit 1 diff --git a/src/validate/breaking-change-commits/test.sh b/src/validate/breaking-change-commits/test.sh index 82ed7a79..529b56a6 100755 --- a/src/validate/breaking-change-commits/test.sh +++ b/src/validate/breaking-change-commits/test.sh @@ -18,6 +18,12 @@ mkdir -p "${TEST_ROOT}/bin" cat > "${TEST_ROOT}/bin/gh" <<'STUB' #!/usr/bin/env bash case "$*" in + # The historical path: the breaking set as it stood at a given revision, + # which is how the gate asks what an approval actually approved. COMPARE_JSON + # is the canned payload; COMPARE_TOTAL overrides `total_commits` to simulate + # the same 250 cap the pull-request path has. + *"/compare/"*) + printf '%s' "${COMPARE_JSON:-{\"total_commits\":0,\"commits\":[]\}}" ;; *"/commits"*) printf '%s\n' ${COMMITS_B64} ;; # The declared total. DECLARED_COMMITS overrides it to simulate the 250 cap, # where the endpoint returns fewer commits than the pull request has. @@ -63,6 +69,40 @@ check() { fi } +# compare_payload [ ...] +compare_payload() { + local total=$1; shift + local entries='' + while [[ $# -gt 0 ]]; do + entries="${entries:+${entries},}$(jq -cn --arg s "$1" --arg m "$2" \ + '{sha: $s, commit: {message: $m}}')" + shift 2 + done + printf '{"total_commits":%s,"commits":[%s]}' "${total}" "${entries}" +} + +run_at_sha() { + env GH_TOKEN=stub REPO=LerianStudio/example PR_NUMBER=42 \ + BASE_REF=develop AT_SHA="$1" COMPARE_JSON="$2" COMMITS_B64='' \ + bash "${RESOLVER}" 2>/dev/null || true +} + +check_at_sha() { + local description=$1 payload=$2 expected_count=$3 + local output count + output=$(run_at_sha aaaa111 "${payload}") + count=$(sed -n 's/^count=//p' <<< "${output}") + + if [[ "${count}" == "${expected_count}" ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - %s\n' "${description}" + else + FAILED=$((FAILED + 1)) + printf 'not ok - %s (count=%s; wanted %s)\n' \ + "${description}" "${count}" "${expected_count}" + fi +} + BANG=$(encode aaa1111 'feat(scope)!: breaks the contract') PLAIN=$(encode bbb2222 'fix(scope): ordinary work') FOOTER=$(encode ccc3333 'feat(scope): looks ordinary @@ -221,5 +261,59 @@ else printf 'not ok - a complete enumeration was refused (count=%s)\n' "${complete}" fi +# --- resolving at a past revision ------------------------------------------- +# The gate asks "what did that approval approve?" and the only honest answer +# comes from the approving run's own head. Believing the fingerprint written in +# a comment is what let a member reuse somebody else's approval. +check_at_sha 'a past revision is resolved from the comparison' \ + "$(compare_payload 2 aaa1111 'feat(scope)!: breaks the contract' \ + bbb2222 'fix(scope): ordinary work')" 1 +check_at_sha 'a past revision with nothing breaking' \ + "$(compare_payload 1 bbb2222 'fix(scope): ordinary work')" 0 +check_at_sha 'the footer form counts there too' \ + "$(compare_payload 1 ccc3333 'feat(scope): looks ordinary + +BREAKING CHANGE: the field was removed')" 1 + +# The same cap, and the same refusal. Answering from a truncated comparison +# would let an omitted breaking commit leave the fingerprint unchanged. +truncated=$(run_at_sha aaaa111 "$(compare_payload 300 aaa1111 'feat(scope)!: breaks')") +if [[ -z "$(sed -n 's/^count=//p' <<< "${truncated}")" ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - a truncated comparison is refused\n' +else + FAILED=$((FAILED + 1)) + printf 'not ok - a truncated comparison produced a verdict (%s)\n' "${truncated}" +fi + +# Two revisions of the same pull request: the fingerprint has to differ once a +# breaking commit is added, or reuse cannot tell them apart. +fp_before=$( + run_at_sha aaaa111 "$(compare_payload 1 aaa1111 'feat(scope)!: breaks')" \ + | sed -n 's/^fingerprint=//p' +) +fp_after=$( + run_at_sha bbbb222 "$(compare_payload 2 aaa1111 'feat(scope)!: breaks' \ + ddd4444 'feat(other)!: breaks too')" \ + | sed -n 's/^fingerprint=//p' +) +if [[ -n "${fp_before}" && -n "${fp_after}" && "${fp_before}" != "${fp_after}" ]]; then + PASSED=$((PASSED + 1)) + printf 'ok - adding a breaking commit changes the fingerprint\n' +else + FAILED=$((FAILED + 1)) + printf 'not ok - the fingerprint did not change (%s vs %s)\n' "${fp_before}" "${fp_after}" +fi + +# And AT_SHA without a base cannot silently fall back to the current head. +if ! env GH_TOKEN=stub REPO=LerianStudio/example PR_NUMBER=42 \ + AT_SHA=aaaa111 COMMITS_B64='' bash "${RESOLVER}" >/dev/null 2>&1; then + PASSED=$((PASSED + 1)) + printf 'ok - AT_SHA without BASE_REF is refused\n' +else + FAILED=$((FAILED + 1)) + printf 'not ok - AT_SHA without BASE_REF was accepted\n' +fi + printf '\n%s passed, %s failed\n' "${PASSED}" "${FAILED}" [[ ${FAILED} -eq 0 ]] diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 0b562fff..3a8b094a 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1359,7 +1359,7 @@ def test_a_standing_approval_skips_the_request_entirely(self): # announced "needs a decision" and added the pending label, and the # verdict took both back seconds later. A flicker in the timeline for a # decision that was never in question. - self.assertIn("steps.prior.outputs.already-approved != 'true'", step) + self.assertIn("steps.reuse.outputs.already-approved != 'true'", step) def test_the_decider_is_linked_not_mentioned(self): for state, env, who in ( @@ -1772,6 +1772,92 @@ def test_a_reused_approval_does_not_skip_the_other_guards(self): "say whether this run may speak at all", ) + def run_reuse(self, **overrides): + """Run the step that decides whether an approval is reused.""" + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["request"]["steps"] + if s.get("name") == "Confirm the approval covers this change" + ) + env = { + "CANDIDATE_RUN": "4242", + "CANDIDATE_HEAD": "aaaa111", + "APPROVED_BY": "carol", + "APPROVED_FINGERPRINT": "8b5ad0f390f484b1", + "RESOLVE_OUTCOME": "success", + "FINGERPRINT": "8b5ad0f390f484b1", + "PR_NUMBER": "42", + "DRY_RUN": "false", + } + env.update(overrides) + _, outputs = execute_body(step["run"], env) + return outputs["already-approved"] + + def test_a_forged_record_cannot_reuse_someone_elses_approval(self): + # @fredcamaral's report on #926, reproduced as a test. + # + # The fingerprint is derived from public commit SHAs, so it is + # computable before the commit exists. The attack: 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 earlier check passes — the + # record is from a MEMBER, the run is on this branch, the run really + # did carry an Environment approval — and none of them asks what it + # approved. The answer used to be taken from the record itself. + # + # It is now resolved from the approving run's own head, which nobody + # can write. + self.assertEqual( + "false", + self.run_reuse( + APPROVED_FINGERPRINT="8b5ad0f390f484b1", # what run 4242 approved: A + FINGERPRINT="50306b18a137bc7b", # what is here now: A+B + ), + "an approval of a different set was reused", + ) + + # The legitimate case it exists for: a push that did not touch the + # breaking commits — a typo fix — resolves to the same set. + self.assertEqual("true", self.run_reuse()) + + # Unresolvable is not reusable. A deleted run, an unreadable + # comparison, a pull request past the commit cap: none of them is + # evidence that the approval still applies. + self.assertEqual("false", self.run_reuse(RESOLVE_OUTCOME="failure")) + self.assertEqual("false", self.run_reuse(APPROVED_FINGERPRINT="")) + self.assertEqual("false", self.run_reuse(CANDIDATE_RUN="")) + + def test_the_approved_set_is_read_from_the_run_not_the_record(self): + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + steps = workflow["jobs"]["request"]["steps"] + + # The lookup hands over a candidate and the run's own head. It must not + # decide reuse by itself any more. + lookup = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Check for a prior approval of the same change", + ) + self.assertIn("candidate-head=", lookup) + self.assertNotIn("already-approved=", lookup) + # `.head_sha` comes from the run, beside the branch it is already + # verified against. + self.assertIn(".head_sha", lookup) + + # The resolver is pointed at that head, never at the current one. + resolve = next( + s for s in steps if s.get("name") == "Resolve what the approving run approved" + ) + self.assertEqual( + "${{ steps.prior.outputs.candidate-head }}", resolve["with"]["at-sha"] + ) + self.assertIn("breaking-change-commits@", resolve["uses"]) + + # And the job publishes the confirmed answer, not the candidate. + self.assertEqual( + "${{ steps.reuse.outputs.already-approved }}", + workflow["jobs"]["request"]["outputs"]["already-approved"], + ) + def test_two_approvers_serialise_into_one_output_value(self): # An Environment can require more than one reviewer, and the approvals # API answers one login per line. Written straight out, the second From 2ee4cdbb9d839c30a46ff618c263dbfdbe07fcf5 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Tue, 20 Oct 2026 10:21:57 -0300 Subject: [PATCH 3/3] docs(validate): state the contents scope the at-sha path needs 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. --- .../breaking-change-commits/README.md | 10 +++++++++- .../breaking-change-commits/action.yml | 14 ++++++++++---- .../breaking-change-guard/test-workflow.py | 19 +++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/validate/breaking-change-commits/README.md b/src/validate/breaking-change-commits/README.md index 3d16ac25..996f04b0 100644 --- a/src/validate/breaking-change-commits/README.md +++ b/src/validate/breaking-change-commits/README.md @@ -105,11 +105,19 @@ approving run's own head closes that: the head is not writable. The 250 cap applies here too, measured against `total_commits`, and is refused the same way. +**This path needs `contents: read`.** The comparison API is repository +contents, not pull request data, so a token carrying only `pull-requests: read` +fails every comparison — and a failed comparison reads as "cannot confirm what +that approval covered", which means the approval is not reused and the reviewer +is asked again on every push. Nothing says why, so grant the scope. +`breaking-change-approval.yml` already does for its own `request` job; this +applies to anything calling the composite directly. + ## Inputs | Input | Description | Required | Default | |-------|-------------|----------|---------| -| `github-token` | Token used to read the commits. Needs `pull-requests: read`. | Yes | — | +| `github-token` | Token used to read the commits. Needs `pull-requests: read` — and `contents: read` as well when `at-sha` is given. | Yes | — | | `pr-number` | Pull request whose commits are listed. | Yes | — | | `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | | `at-sha` | Resolve the set as it stood at this revision instead of at the current head. Requires `base-ref`. | No | `''` | diff --git a/src/validate/breaking-change-commits/action.yml b/src/validate/breaking-change-commits/action.yml index 0be76574..1967029f 100644 --- a/src/validate/breaking-change-commits/action.yml +++ b/src/validate/breaking-change-commits/action.yml @@ -3,7 +3,13 @@ description: "Lists the commits in a pull request that carry a breaking change, inputs: github-token: - description: 'Token used to read the pull request commits. Needs `pull-requests: read`.' + description: >- + Token used to read the commits. Needs `pull-requests: read`, and + `contents: read` as well when `at-sha` is given — that path reads the + comparison API, which is repository contents rather than pull request + data. Granting only the first makes every comparison fail, and the gate + reads a failed comparison as "cannot confirm", so it asks the reviewer + again on every push without saying why. required: true pr-number: description: Pull request whose commits are listed. @@ -15,9 +21,9 @@ inputs: at-sha: description: >- Resolve the set as it stood at this revision instead of at the pull - request's current head. Requires `base-ref`. The gate uses it to ask what - an approval actually approved, rather than believing a fingerprint - written in a comment. + request's current head. Requires `base-ref`, and a token carrying + `contents: read`. The gate uses it to ask what an approval actually + approved, rather than believing a fingerprint written in a comment. required: false default: '' base-ref: diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 3a8b094a..b61c0af2 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1827,6 +1827,25 @@ def test_a_forged_record_cannot_reuse_someone_elses_approval(self): self.assertEqual("false", self.run_reuse(APPROVED_FINGERPRINT="")) self.assertEqual("false", self.run_reuse(CANDIDATE_RUN="")) + def test_the_reuse_lookup_can_read_the_comparison_it_depends_on(self): + # The `at-sha` path reads the comparison API, which is repository + # contents rather than pull request data. Without `contents: read` the + # comparison fails, the gate reads that as "cannot confirm what that + # approval covered", and the reviewer is asked again on every push — + # silently, since the step is `continue-on-error`. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + self.assertEqual("read", workflow["jobs"]["request"]["permissions"]["contents"]) + + # And the composite says so, for anyone calling it directly. + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "validate" / "breaking-change-commits" + / "action.yml" + ).read_text(encoding="utf-8") + ) + self.assertIn("contents: read", action["inputs"]["github-token"]["description"]) + self.assertIn("contents: read", action["inputs"]["at-sha"]["description"]) + def test_the_approved_set_is_read_from_the_run_not_the_record(self): workflow = yaml.safe_load(APPROVAL_WORKFLOW) steps = workflow["jobs"]["request"]["steps"]