From 1efb73cc462d0677eba824f90bda9adbbc3f6c9f Mon Sep 17 00:00:00 2001 From: Cindy Zhang <2781099+cixzhang@users.noreply.github.com> Date: Thu, 27 Aug 2026 00:01:38 -0700 Subject: [PATCH] fix(ci): authorize Repo Owners for visual acceptance --- .github/scripts/visual-gate/authorization.mjs | 29 ++++++++ .../visual-gate/authorization.test.mjs | 62 +++++++++++++++++ .../scripts/visual-gate/visual-acceptance.mjs | 27 ++++++-- .../visual-gate/visual-acceptance.test.mjs | 67 +++++++++++++++++++ .../visual-gate/workflow-concurrency.test.mjs | 26 +++++-- .github/workflows/visual-acceptance.yml | 31 ++++++--- 6 files changed, 224 insertions(+), 18 deletions(-) create mode 100644 .github/scripts/visual-gate/authorization.mjs create mode 100644 .github/scripts/visual-gate/authorization.test.mjs diff --git a/.github/scripts/visual-gate/authorization.mjs b/.github/scripts/visual-gate/authorization.mjs new file mode 100644 index 0000000000000..350f9ae07663b --- /dev/null +++ b/.github/scripts/visual-gate/authorization.mjs @@ -0,0 +1,29 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +const MAINTAINER_PERMISSIONS = new Set(['maintain', 'admin']); +const REPO_OWNER_ROLE = 'Repo Owner'; + +export function visualAcceptanceIdentity(permissionLevel = {}) { + const permission = permissionLevel.permission ?? 'none'; + const effectivePermission = permissionLevel.user?.permissions?.admin + ? 'admin' + : permissionLevel.user?.permissions?.maintain + ? 'maintain' + : permission; + return { + permission, + roleName: permissionLevel.role_name ?? null, + effectivePermission, + }; +} + +export function isVisualAcceptanceMaintainer({ + permission, + roleName, + effectivePermission = permission, +} = {}) { + return ( + MAINTAINER_PERMISSIONS.has(effectivePermission) || + roleName === REPO_OWNER_ROLE + ); +} diff --git a/.github/scripts/visual-gate/authorization.test.mjs b/.github/scripts/visual-gate/authorization.test.mjs new file mode 100644 index 0000000000000..a387d1aff7400 --- /dev/null +++ b/.github/scripts/visual-gate/authorization.test.mjs @@ -0,0 +1,62 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +import {describe, expect, it} from 'vitest'; + +import { + isVisualAcceptanceMaintainer, + visualAcceptanceIdentity, +} from './authorization.mjs'; + +const endpointIdentity = overrides => + visualAcceptanceIdentity({ + permission: 'write', + role_name: 'Custom role', + user: { + permissions: { + admin: false, + maintain: false, + pull: true, + push: true, + triage: true, + }, + }, + ...overrides, + }); + +describe('visual acceptance authorization', () => { + it.each([ + ['repository owner', endpointIdentity({role_name: 'Repo Owner'}), true], + [ + 'effective maintainer', + endpointIdentity({user: {permissions: {maintain: true}}}), + true, + ], + [ + 'effective administrator', + endpointIdentity({user: {permissions: {admin: true}}}), + true, + ], + ['ordinary writer', endpointIdentity(), false], + ['missing collaborator response', visualAcceptanceIdentity(), false], + ['unknown permission', {permission: 'unknown'}, false], + ['unknown role', {permission: 'write', roleName: 'Owner'}, false], + ['legacy maintainer record', {permission: 'maintain'}, true], + ['legacy administrator record', {permission: 'admin'}, true], + ])('%s', (_label, identity, expected) => { + expect(isVisualAcceptanceMaintainer(identity)).toBe(expected); + }); + + it('keeps the endpoint permission and role for the acceptance record', () => { + expect( + visualAcceptanceIdentity({ + permission: 'write', + role_name: 'Repo Owner', + user: {permissions: {maintain: true}}, + }), + ).toMatchObject({ + permission: 'write', + roleName: 'Repo Owner', + effectivePermission: 'maintain', + }); + }); +}); diff --git a/.github/scripts/visual-gate/visual-acceptance.mjs b/.github/scripts/visual-gate/visual-acceptance.mjs index e165024ac4edb..6340c93dca9ae 100644 --- a/.github/scripts/visual-gate/visual-acceptance.mjs +++ b/.github/scripts/visual-gate/visual-acceptance.mjs @@ -11,9 +11,10 @@ import {execFileSync} from 'node:child_process'; import {createHash} from 'node:crypto'; import fs from 'node:fs'; +import {createRequire} from 'node:module'; import path from 'node:path'; -import {PNG} from 'pngjs'; +import {isVisualAcceptanceMaintainer} from './authorization.mjs'; import {readStoryIndex, shotKey, storiesInPackages} from './lib/plan.mjs'; const args = process.argv.slice(2); @@ -276,14 +277,23 @@ function accept() { const approver = flag('approver') ?? ''; const approverId = Number(flag('approver-id')); const permission = flag('permission') ?? ''; + const effectivePermission = flag('effective-permission') ?? permission; + const roleName = flag('role-name'); const commentId = Number(flag('comment-id')); const reason = validateReason(flag('reason')); validateIdentity(pr, head); if (!approver || !Number.isSafeInteger(approverId) || approverId <= 0) fail('invalid approver'); - if (!['maintain', 'admin'].includes(permission)) - fail('approver must have maintain/admin permission'); + if ( + !isVisualAcceptanceMaintainer({ + permission, + effectivePermission, + roleName, + }) + ) { + fail('approver must have maintain/admin permission or the Repo Owner role'); + } if (!Number.isSafeInteger(commentId) || commentId <= 0) fail('invalid comment id'); @@ -351,6 +361,8 @@ function accept() { approver, approverId, permission, + effectivePermission, + roleName, reason, commentId, at: new Date().toISOString(), @@ -467,7 +479,11 @@ function validateAcceptance(value, expected = {}) { !value.decision?.approver || !Number.isSafeInteger(value.decision?.approverId) || value.decision.approverId <= 0 || - !['maintain', 'admin'].includes(value.decision?.permission) || + !isVisualAcceptanceMaintainer({ + permission: value.decision?.permission, + effectivePermission: value.decision?.effectivePermission, + roleName: value.decision?.roleName, + }) || !Number.isSafeInteger(value.decision?.commentId) || value.decision.commentId <= 0 || !value.decision?.reason @@ -584,7 +600,10 @@ function plan() { ); } +const require = createRequire(import.meta.url); + function samePixels(first, second) { + const {PNG} = require('pngjs'); const a = PNG.sync.read(fs.readFileSync(first)); const b = PNG.sync.read(fs.readFileSync(second)); return ( diff --git a/.github/scripts/visual-gate/visual-acceptance.test.mjs b/.github/scripts/visual-gate/visual-acceptance.test.mjs index fda925ff7a187..25624cdc22d6e 100644 --- a/.github/scripts/visual-gate/visual-acceptance.test.mjs +++ b/.github/scripts/visual-gate/visual-acceptance.test.mjs @@ -234,6 +234,73 @@ describe('visual acceptance', () => { ).toBe(true); }); + it('accepts without installed packages in the archive job', () => { + const isolated = path.join(root, 'isolated-visual-gate'); + fs.cpSync(path.dirname(SCRIPT), isolated, {recursive: true}); + const args = [path.join(isolated, 'visual-acceptance.mjs'), 'accept']; + for (const [name, value] of Object.entries(acceptanceFlags())) + args.push(`--${name}`, String(value)); + expect(execFileSync(process.execPath, args, {encoding: 'utf8'})).toContain( + 'Accepted 1 visual delta', + ); + }); + + it('accepts a repository owner and records the reported permission and role', () => { + run( + 'accept', + acceptanceFlags({ + approver: 'cixzhang', + permission: 'write', + 'effective-permission': 'maintain', + 'role-name': 'Repo Owner', + }), + ); + expect( + JSON.parse(fs.readFileSync(acceptanceFile(), 'utf8')).decision, + ).toMatchObject({ + approver: 'cixzhang', + permission: 'write', + effectivePermission: 'maintain', + roleName: 'Repo Owner', + }); + }); + + it.each(['maintain', 'admin'])( + 'accepts effective %s capability while preserving the reported role', + effectivePermission => { + run( + 'accept', + acceptanceFlags({ + permission: 'write', + 'effective-permission': effectivePermission, + 'role-name': 'Custom role', + }), + ); + expect( + JSON.parse(fs.readFileSync(acceptanceFile(), 'utf8')).decision, + ).toMatchObject({ + permission: 'write', + effectivePermission, + roleName: 'Custom role', + }); + }, + ); + + it.each(['maintain', 'admin'])( + 'validates a legacy %s record without role metadata', + permission => { + run('accept', acceptanceFlags({permission})); + const record = JSON.parse(fs.readFileSync(acceptanceFile(), 'utf8')); + delete record.decision.roleName; + delete record.decision.effectivePermission; + writeJSON(acceptanceFile(), record); + expect(JSON.parse(run('state', {pages, pr: 42, head: HEAD}))).toMatchObject({ + state: 'success', + reason: 'accepted', + }); + }, + ); + it('requires an explanatory reason and maintainer permission', () => { expect(fail('accept', acceptanceFlags({reason: 'intentional...'}))).toMatch( /reason must explain/, diff --git a/.github/scripts/visual-gate/workflow-concurrency.test.mjs b/.github/scripts/visual-gate/workflow-concurrency.test.mjs index 56dd491721503..a23c5f900bc58 100644 --- a/.github/scripts/visual-gate/workflow-concurrency.test.mjs +++ b/.github/scripts/visual-gate/workflow-concurrency.test.mjs @@ -56,10 +56,27 @@ describe('visual acceptance workflow concurrency', () => { ); expect(initialize).toContain("'No stable visual scope.'"); expect(authorize).not.toContain(': write'); - expect(authorize).not.toContain('actions/checkout'); + expect(authorize).toContain('actions/checkout@v7'); + expect(authorize).toContain('visualAcceptanceIdentity(response.data)'); + expect(authorize).toContain('isVisualAcceptanceMaintainer(identity)'); + expect(authorize).toContain( + "core.setOutput('effective_permission', identity.effectivePermission)", + ); + expect(authorize).toContain( + "core.setOutput('role_name', identity.roleName ?? '')", + ); + expect(authorize).not.toContain('author_association'); expect(authorize).not.toContain('issues.createComment'); expect(authorize).not.toContain('createCommitStatus'); expect(accept).toContain('needs: authorize'); + expect(accept).toContain( + 'EFFECTIVE_PERMISSION: ${{ needs.authorize.outputs.effective_permission }}', + ); + expect(accept).toContain( + 'ROLE_NAME: ${{ needs.authorize.outputs.role_name }}', + ); + expect(accept).toContain('--effective-permission "$EFFECTIVE_PERMISSION"'); + expect(accept).toContain('--role-name "$ROLE_NAME"'); expect(accept).toContain( 'group: visual-acceptance-head-${{ needs.authorize.outputs.head_repo_id }}-${{ needs.authorize.outputs.head_ref }}', ); @@ -109,16 +126,15 @@ describe('visual acceptance workflow concurrency', () => { expect(capture).not.toContain('GITHUB_RUN_ATTEMPT:'); }); - it('normalizes GitHub role flags before authorizing a maintainer', () => { + it('uses the documented collaborator permission response shape', () => { const value = workflow('visual-acceptance.yml'); const authorize = value.slice( value.indexOf(' authorize:'), value.indexOf(' accept:'), ); - expect(authorize).toContain('actor.permissions?.admin'); - expect(authorize).toContain('actor.permissions?.maintain'); - expect(authorize).toContain("? 'maintain'"); + expect(authorize).toContain('visualAcceptanceIdentity(response.data)'); + expect(authorize).not.toContain('response.data.user.permission'); }); it('uses the same head identity for post-merge promotion', () => { diff --git a/.github/workflows/visual-acceptance.yml b/.github/workflows/visual-acceptance.yml index f03b30457e3a5..a1a2f5591bc9f 100644 --- a/.github/workflows/visual-acceptance.yml +++ b/.github/workflows/visual-acceptance.yml @@ -109,17 +109,29 @@ jobs: approver: ${{ steps.authorize.outputs.approver }} approver_id: ${{ steps.authorize.outputs.approver_id }} permission: ${{ steps.authorize.outputs.permission }} + effective_permission: ${{ steps.authorize.outputs.effective_permission }} + role_name: ${{ steps.authorize.outputs.role_name }} comment_id: ${{ steps.authorize.outputs.comment_id }} reason: ${{ steps.authorize.outputs.reason }} steps: # This job is deliberately read-only. The mutation job acquires the shared # head-branch lock using these trusted API-derived outputs. + - name: Checkout trusted default-branch code + uses: actions/checkout@v7 + - name: Authorize and resolve the decision id: authorize uses: actions/github-script@v9 with: retries: 3 script: | + const {pathToFileURL} = require('node:url'); + const { + isVisualAcceptanceMaintainer, + visualAcceptanceIdentity, + } = await import( + pathToFileURL(`${process.env.GITHUB_WORKSPACE}/.github/scripts/visual-gate/authorization.mjs`).href, + ); const {owner, repo} = context.repo; const number = context.issue.number; const {data: pr} = await github.rest.pulls.get({owner, repo, pull_number: number}); @@ -165,21 +177,16 @@ jobs: refuse('that evidence is not from the latest completed CI attempt for this head.'); return; } - let permission = 'none'; + let identity = visualAcceptanceIdentity(); try { const response = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: context.actor, }); - const actor = response.data.user; - permission = actor.permissions?.admin - ? 'admin' - : actor.permissions?.maintain - ? 'maintain' - : actor.permission; + identity = visualAcceptanceIdentity(response.data); } catch (error) { if (error.status !== 404) throw error; } - if (!['maintain', 'admin'].includes(permission)) { + if (!isVisualAcceptanceMaintainer(identity)) { refuse('only a repository maintainer may accept stable visual changes.'); return; } @@ -188,7 +195,9 @@ jobs: core.setOutput('run_attempt', String(runAttempt)); core.setOutput('approver', context.actor); core.setOutput('approver_id', String(context.payload.comment.user.id)); - core.setOutput('permission', permission); + core.setOutput('permission', identity.permission); + core.setOutput('effective_permission', identity.effectivePermission); + core.setOutput('role_name', identity.roleName ?? ''); core.setOutput('comment_id', String(context.payload.comment.id)); core.setOutput('reason', reason); @@ -238,6 +247,8 @@ jobs: APPROVER: ${{ needs.authorize.outputs.approver }} APPROVER_ID: ${{ needs.authorize.outputs.approver_id }} PERMISSION: ${{ needs.authorize.outputs.permission }} + EFFECTIVE_PERMISSION: ${{ needs.authorize.outputs.effective_permission }} + ROLE_NAME: ${{ needs.authorize.outputs.role_name }} COMMENT_ID: ${{ needs.authorize.outputs.comment_id }} REASON: ${{ needs.authorize.outputs.reason }} run: | @@ -257,6 +268,8 @@ jobs: --approver "$APPROVER" \ --approver-id "$APPROVER_ID" \ --permission "$PERMISSION" \ + --effective-permission "$EFFECTIVE_PERMISSION" \ + --role-name "$ROLE_NAME" \ --comment-id "$COMMENT_ID" \ --reason "$REASON" if [ -z "$(git -C "$PAGES" status --porcelain -- "$DEST")" ]; then