diff --git a/.github/actions/release-approval-guard/README.md b/.github/actions/release-approval-guard/README.md new file mode 100644 index 0000000..35ccf50 --- /dev/null +++ b/.github/actions/release-approval-guard/README.md @@ -0,0 +1,36 @@ +# Release approval guard + +Fails a release when someone who approved its environment authored or merged a PR in the push that triggered the run. + +This restores what GitHub's "Prevent self-review" environment setting did before merge queues. That setting excludes the run's triggering actor. When a person merged a PR, the actor was that person, so they couldn't approve the release. With a merge queue (GitHub or Trunk), the actor is a bot, so nobody is excluded. + +## Usage + +Add the step to the job that uses the approval environment, before the job uses its secrets: + +```yaml +jobs: + version-bump: + environment: 'NPM Release' + permissions: + contents: read + actions: read # approvals of this run + pull-requests: read # PRs in the push + steps: + - uses: PostHog/.github/.github/actions/release-approval-guard@ + with: + environment: 'NPM Release' +``` + +It needs no checkout. It needs Node, which GitHub-hosted runners include. + +## Behavior + +- It finds every PR in the push that triggered the run (`before...after`). A merge queue can merge several PRs in one push. +- It blocks approval by each PR's author and by whoever merged it. With GitHub's merge queue, `merged_by` is the person who added the PR to the queue. With Trunk, it's `trunk-io[bot]`, so only the author is checked. +- On events other than `push`, it does nothing, because GitHub's own check already excludes whoever triggered the run. +- It fails closed if the run has no approval for the environment, if a commit in the push isn't part of a merged PR, or if the push range can't be listed in full. +- The approvals API doesn't say which run attempt an approval belongs to, so every approval in the run counts. After a block, a different approver must approve a **new run**. A re-run can still see the blocked approval. +- Like the setting it replaces, it only covers the PRs in the triggering push. If a release also publishes earlier unreleased changes, their authors aren't excluded. + +Run the tests with `node --test .github/actions/release-approval-guard/guard.test.mjs`. diff --git a/.github/actions/release-approval-guard/action.yaml b/.github/actions/release-approval-guard/action.yaml new file mode 100644 index 0000000..e624f34 --- /dev/null +++ b/.github/actions/release-approval-guard/action.yaml @@ -0,0 +1,22 @@ +name: 'Release approval guard' +description: 'Fail a release when an approver of its environment authored or merged a PR in the push that triggered it' + +inputs: + environment: + description: 'Name of the environment whose approval gates the release, e.g. "NPM Release"' + required: true + github-token: + description: 'Token with `actions: read`, `contents: read` and `pull-requests: read` on the repository' + required: false + default: ${{ github.token }} + +runs: + using: 'composite' + steps: + - name: Check release approvers against the PRs in the push + shell: bash + env: + GH_TOKEN: ${{ inputs.github-token }} + ENVIRONMENT: ${{ inputs.environment }} + ACTION_PATH: ${{ github.action_path }} + run: node "$ACTION_PATH/guard.mjs" diff --git a/.github/actions/release-approval-guard/guard.mjs b/.github/actions/release-approval-guard/guard.mjs new file mode 100644 index 0000000..edd6232 --- /dev/null +++ b/.github/actions/release-approval-guard/guard.mjs @@ -0,0 +1,132 @@ +#!/usr/bin/env node +import { appendFileSync, readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + +export class GuardError extends Error {} + +export function createApi({ token, apiUrl = 'https://api.github.com', fetchImpl = fetch, retryDelayMs = 2000 }) { + return { + async get(path) { + for (let attempt = 1; ; attempt++) { + const res = await fetchImpl(`${apiUrl}${path}`, { + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'X-GitHub-Api-Version': '2022-11-28', + }, + }); + if (res.ok) return res.json(); + if (res.status >= 500 && attempt < 3) { + await new Promise((resolve) => setTimeout(resolve, retryDelayMs * attempt)); + continue; + } + throw new GuardError(`GitHub API ${res.status} for ${path}: ${await res.text()}`); + } + }, + }; +} + +// Approvals aren't tied to a run attempt, so every approval in the run counts. +export async function approversFor(api, repo, runId, environment) { + const approvals = await api.get(`/repos/${repo}/actions/runs/${runId}/approvals`); + const logins = approvals + .filter((a) => a.state === 'approved' && a.environments.some((e) => e.name === environment)) + .map((a) => a.user.login); + return [...new Set(logins)].sort(); +} + +export async function pushedPrs(api, repo, before, after) { + if (!before || /^0+$/.test(before)) { + throw new GuardError(`The push has no previous commit (before=${before}), so its PRs cannot be found.`); + } + const compare = await api.get(`/repos/${repo}/compare/${before}...${after}`); + if (compare.commits.length === 0 || compare.commits.length < compare.total_commits) { + throw new GuardError(`Cannot list all ${compare.total_commits} commits between ${before} and ${after}.`); + } + const numbers = new Set(); + for (const commit of compare.commits) { + const merged = (await api.get(`/repos/${repo}/commits/${commit.sha}/pulls`)).filter((pr) => pr.merged_at); + if (merged.length === 0) { + throw new GuardError(`Commit ${commit.sha} in the push does not belong to a merged PR.`); + } + for (const pr of merged) numbers.add(pr.number); + } + return Promise.all([...numbers].sort((a, b) => a - b).map((n) => api.get(`/repos/${repo}/pulls/${n}`))); +} + +export async function checkReleaseApproval({ api, repo, runId, environment, before, after }) { + const approvers = await approversFor(api, repo, runId, environment); + if (approvers.length === 0) { + throw new GuardError( + `No approval for the "${environment}" environment in this run. ` + + 'Check the environment input and that the environment has required reviewers.', + ); + } + const prs = await pushedPrs(api, repo, before, after); + const blocked = new Map(); + const block = (login, reason) => { + if (!login) return; + const key = login.toLowerCase(); + if (!blocked.has(key)) blocked.set(key, []); + blocked.get(key).push(reason); + }; + for (const pr of prs) { + block(pr.user?.login, `author of #${pr.number}`); + block(pr.merged_by?.login, `merged #${pr.number}`); + } + const violations = approvers + .filter((login) => blocked.has(login.toLowerCase())) + .map((login) => ({ login, reasons: blocked.get(login.toLowerCase()) })); + return { approvers, prs: prs.map((pr) => pr.number), violations }; +} + +async function main() { + const env = process.env; + if (env.GITHUB_EVENT_NAME !== 'push') { + console.log( + `Not a push (${env.GITHUB_EVENT_NAME}), so GitHub's "Prevent self-review" already excludes whoever triggered the run.`, + ); + return; + } + const environment = env.ENVIRONMENT?.trim(); + if (!environment) throw new GuardError('The environment input is required.'); + const event = JSON.parse(readFileSync(env.GITHUB_EVENT_PATH, 'utf8')); + + const result = await checkReleaseApproval({ + api: createApi({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL }), + repo: env.GITHUB_REPOSITORY, + runId: env.GITHUB_RUN_ID, + environment, + before: event.before, + after: event.after, + }); + + console.log(`PRs in the push: ${result.prs.map((n) => `#${n}`).join(', ')}`); + console.log(`Approvers of "${environment}": ${result.approvers.join(', ')}`); + + if (result.violations.length > 0) { + const lines = result.violations.map((v) => `${v.login} (${v.reasons.join(', ')})`); + console.log( + `::error title=Release approved by its author::${lines.join('; ')} approved a release ` + + 'triggered by their own PR. Another approver must approve a new run of this ' + + 'workflow. A re-run of this run can still see the earlier approval and fail again.', + ); + if (env.GITHUB_STEP_SUMMARY) { + appendFileSync( + env.GITHUB_STEP_SUMMARY, + `### Release blocked\n\nApproved by the author or merger of a PR in this push:\n\n${lines + .map((l) => `- ${l}`) + .join('\n')}\n`, + ); + } + process.exit(1); + } + console.log('✓ No approver authored or merged a PR in this push.'); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + main().catch((e) => { + console.log(`::error title=Release approval guard::${e.message}`); + process.exit(1); + }); +} diff --git a/.github/actions/release-approval-guard/guard.test.mjs b/.github/actions/release-approval-guard/guard.test.mjs new file mode 100644 index 0000000..aad616a --- /dev/null +++ b/.github/actions/release-approval-guard/guard.test.mjs @@ -0,0 +1,147 @@ +import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; + +import { checkReleaseApproval, createApi, GuardError } from './guard.mjs'; + +const repo = 'PostHog/posthog-js'; +const API = 'https://api.github.test'; +const before = 'b'.repeat(40); +const after = 'a'.repeat(40); + +function fakeApi(routes, failures = {}) { + const fetchImpl = async (url) => { + const path = url.slice(API.length); + if (failures[path] > 0) { + failures[path]--; + return new Response('unavailable', { status: 502 }); + } + if (!(path in routes)) return new Response(`no route ${path}`, { status: 404 }); + return Response.json(routes[path]); + }; + return createApi({ token: 't', apiUrl: API, fetchImpl, retryDelayMs: 0 }); +} + +const approval = (login, environment = 'NPM Release', state = 'approved') => ({ + state, + user: { login }, + environments: [{ name: environment }], +}); + +// Each entry is [commit sha, PR number, author, merged_by]. +function push(approvals, prs) { + const routes = { + [`/repos/${repo}/actions/runs/7/approvals`]: approvals, + [`/repos/${repo}/compare/${before}...${after}`]: { + total_commits: prs.length, + commits: prs.map(([sha]) => ({ sha })), + }, + }; + for (const [sha, number, author, mergedBy] of prs) { + routes[`/repos/${repo}/commits/${sha}/pulls`] = [{ number, merged_at: '2026-10-01T00:00:00Z' }]; + routes[`/repos/${repo}/pulls/${number}`] = { + number, + user: { login: author }, + merged_by: mergedBy && { login: mergedBy }, + }; + } + return routes; +} + +const check = (routes, environment = 'NPM Release') => + checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment, before, after }); + +test('passes when no approver authored or merged a PR in the push', async () => { + const result = await check(push([approval('bob')], [['c1', 1, 'alice', 'alice']])); + assert.deepEqual(result.violations, []); + assert.deepEqual(result.prs, [1]); +}); + +test('blocks the PR author', async () => { + const result = await check(push([approval('alice')], [['c1', 1, 'alice', 'github-merge-queue[bot]']])); + assert.deepEqual(result.violations, [{ login: 'alice', reasons: ['author of #1'] }]); +}); + +test('blocks whoever merged the PR', async () => { + const result = await check(push([approval('bob')], [['c1', 1, 'external', 'bob']])); + assert.deepEqual(result.violations, [{ login: 'bob', reasons: ['merged #1'] }]); +}); + +test('checks every PR in a batched merge queue push', async () => { + const prs = [ + ['c1', 1, 'alice', 'alice'], + ['c2', 2, 'carol', 'carol'], + ]; + const result = await check(push([approval('carol')], prs)); + assert.deepEqual(result.prs, [1, 2]); + assert.deepEqual(result.violations.map((v) => v.login), ['carol']); +}); + +test('matches logins case-insensitively', async () => { + const result = await check(push([approval('Alice')], [['c1', 1, 'alice', 'alice']])); + assert.equal(result.violations[0].login, 'Alice'); +}); + +test('blocks when any approval in the run came from an author', async () => { + const result = await check(push([approval('bob'), approval('alice')], [['c1', 1, 'alice', 'alice']])); + assert.deepEqual(result.violations.map((v) => v.login), ['alice']); +}); + +test('ignores approvals that are rejected or for other environments', async () => { + const approvals = [approval('alice', 'S3 Upload'), approval('alice', 'NPM Release', 'rejected'), approval('bob')]; + const result = await check(push(approvals, [['c1', 1, 'alice', 'alice']])); + assert.deepEqual(result.approvers, ['bob']); + assert.deepEqual(result.violations, []); +}); + +test('fails closed when the run has no approval for the environment', async () => { + await assert.rejects(check(push([approval('bob', 'Other')], [['c1', 1, 'alice', 'alice']])), GuardError); +}); + +test('fails closed when a commit in the push is not from a merged PR', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + routes[`/repos/${repo}/commits/c1/pulls`] = [{ number: 1, merged_at: null }]; + await assert.rejects(check(routes), /c1 in the push does not belong to a merged PR/); +}); + +test('fails closed when the comparison does not list every commit', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + routes[`/repos/${repo}/compare/${before}...${after}`].total_commits = 300; + await assert.rejects(check(routes), /Cannot list all 300 commits/); +}); + +test('fails closed when the push has no previous commit', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + await assert.rejects( + checkReleaseApproval({ api: fakeApi(routes), repo, runId: 7, environment: 'NPM Release', before: '0'.repeat(40), after }), + /has no previous commit/, + ); +}); + +test('surfaces GitHub API errors', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + delete routes[`/repos/${repo}/pulls/1`]; + await assert.rejects(check(routes), /GitHub API 404/); +}); + +test('retries GitHub server errors', async () => { + const routes = push([approval('bob')], [['c1', 1, 'alice', 'alice']]); + const compare = `/repos/${repo}/compare/${before}...${after}`; + const api = fakeApi(routes, { [compare]: 2 }); + const result = await checkReleaseApproval({ api, repo, runId: 7, environment: 'NPM Release', before, after }); + assert.deepEqual(result.prs, [1]); + await assert.rejects( + checkReleaseApproval({ api: fakeApi(routes, { [compare]: 3 }), repo, runId: 7, environment: 'NPM Release', before, after }), + /GitHub API 502/, + ); +}); + +test('leaves runs that are not pushes to GitHub\'s own check', () => { + const script = fileURLToPath(new URL('./guard.mjs', import.meta.url)); + const out = execFileSync('node', [script], { + env: { ...process.env, GITHUB_EVENT_NAME: 'workflow_dispatch' }, + encoding: 'utf8', + }); + assert.match(out, /Not a push \(workflow_dispatch\)/); +}); diff --git a/.github/workflows/release-approval-guard-tests.yml b/.github/workflows/release-approval-guard-tests.yml new file mode 100644 index 0000000..2b95e32 --- /dev/null +++ b/.github/workflows/release-approval-guard-tests.yml @@ -0,0 +1,24 @@ +name: Release approval guard tests + +on: + pull_request: + merge_group: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Setup Node + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: '24' + + - name: Test release approval guard + run: node --test .github/actions/release-approval-guard/guard.test.mjs diff --git a/AGENTS.md b/AGENTS.md index 4e1cfe6..cbfff82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,12 @@ No general test suite. Run the changeset hygiene script tests with: node --test .github/scripts/check-changeset-coverage.test.mjs ``` +Run the release approval guard tests with: + +```bash +node --test .github/actions/release-approval-guard/guard.test.mjs +``` + The semgrep rule tests run with: ```bash