diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index f00749e5..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: @@ -801,11 +881,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 +907,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/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..996f04b0 100644 --- a/src/validate/breaking-change-commits/README.md +++ b/src/validate/breaking-change-commits/README.md @@ -87,13 +87,41 @@ 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. + +**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 | `''` | +| `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..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. @@ -12,6 +18,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`, 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: + description: 'Base of the comparison when `at-sha` is given — the pull request''s base branch.' + required: false + default: '' outputs: count: @@ -37,6 +55,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 9f011c94..b61c0af2 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"])) ] } @@ -1354,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 ( @@ -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"), @@ -1705,6 +1772,111 @@ 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_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"] + + # 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