From 795855bd416e3d0ad000f169aaac7d669b066925 Mon Sep 17 00:00:00 2001 From: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:41:56 -0700 Subject: [PATCH] Use Actions approval for artifact publication Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> --- .gitignore | 1 + docs/README.md | 5 ++-- scripts/github-pr.ts | 7 ----- scripts/publication.test.ts | 58 ------------------------------------- scripts/publish-ci.ts | 16 ++++------ scripts/site-ci.ts | 3 +- 6 files changed, 9 insertions(+), 81 deletions(-) diff --git a/.gitignore b/.gitignore index c9dd0bc..7a38072 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,7 @@ perf_*.json mem_*.json # Editor directories and files +.copilot-reviews/ .vscode/* !.vscode/extensions.json .idea diff --git a/docs/README.md b/docs/README.md index 664a417..21397d3 100644 --- a/docs/README.md +++ b/docs/README.md @@ -110,7 +110,6 @@ The preview URL then returns 404. * Each of the 36 measurement jobs runs all three strategies sequentially with a fresh server process for each. Retrying a job repeats its three strategies. * Preview builds run independently of benchmarks. Draft PRs have no preview. * Production uses published history. Previews include matching pending results when available. -* Benchmark trigger, workload, and preview workflow YAML must match `main` before publishers accept their artifacts. * Merge requires `Benchmark Policy`, `Benchmark Status`, an up-to-date branch, and squash merging. Required PR review count is zero. Preview and publication success do not block merge. Publication calls Pages after a main push or a data change. Pages also runs for @@ -122,11 +121,11 @@ Benchmark checks ignore unrelated labels and title or body edits. ## Security -* Outside contributors require **Approve and run**. Collaborators run automatically. Allowing preview execution also permits serving its JavaScript to visitors. +* GitHub Actions must use **Require approval for all external contributors**. **Approve and run** authorizes pending benchmark archival and serving preview JavaScript from that revision. Collaborators run automatically. * PR jobs use read-only repository permissions. The separate `workflow_run` publishers execute `main` code with their own tokens. * Benchmark Publication uses `contents: write` and `statuses: write`. It commits to `data` as `github-actions[bot]` and restricts write paths in code. * Pages handles PR metadata with `pull_request_target` and executes only `main` code. * Pages uses `pages: write` and `id-token: write`. Configure Pages for GitHub Actions and restrict the `github-pages` environment to `main`. -* Publishers validate PR artifacts as untrusted input. They do not execute artifact scripts or restore PR caches. Workflow matching cannot prove measurements are genuine. +* Publishers validate PR artifacts as untrusted input. Validation and successful job topology do not prove measurements are genuine. Publishers do not execute artifact scripts or restore PR caches. * PRs can change workflows and check scripts. Inspect those changes before allowing execution or merging. Self-hosted runners must be disposable and isolated from credentials and sensitive networks. * Preview JavaScript shares production's origin and browser storage. Separate-origin hosting is required for browser isolation. \ No newline at end of file diff --git a/scripts/github-pr.ts b/scripts/github-pr.ts index 47a510f..3a1a4f2 100644 --- a/scripts/github-pr.ts +++ b/scripts/github-pr.ts @@ -1,5 +1,4 @@ import { z } from 'zod' -import { readFileSync } from 'node:fs' const repository = process.env.GITHUB_REPOSITORY! if (!/^[\w.-]+\/[\w.-]+$/.test(repository)) throw new Error('GITHUB_REPOSITORY is required') @@ -28,12 +27,6 @@ export async function pages(path: string): Promise { } } -export async function workflowTrusted(path: string, head: string): Promise { - const workflow = await github(`contents/${path}?ref=${head}`) - if (workflow.encoding !== 'base64') return false - return Buffer.from(workflow.content, 'base64').toString('utf8') === readFileSync(path, 'utf8') -} - const commit = z.string().regex(/^[a-f0-9]{40}$/) export const decisionSchema = z.object({ diff --git a/scripts/publication.test.ts b/scripts/publication.test.ts index 19690e4..94979f3 100644 --- a/scripts/publication.test.ts +++ b/scripts/publication.test.ts @@ -397,35 +397,6 @@ test('label-only eligibility with simulated GitHub responses', async context => }) }) -test('workflows must match trusted main', async context => { - process.env.GITHUB_REPOSITORY = repository - process.env.GH_TOKEN = 'fixture-token' - const { workflowTrusted } = await import('./github-pr.ts') - const temporary = mkdtempSync(resolve(tmpdir(), 'benchmark-workflow-trust-')) - const previousDirectory = process.cwd() - context.after(() => { - process.chdir(previousDirectory) - rmSync(temporary, { recursive: true, force: true }) - }) - process.chdir(temporary) - mkdirSync('.github/workflows', { recursive: true }) - const head = 'a'.repeat(40) - let content = 'trusted workflow' - context.mock.method(globalThis, 'fetch', async (url: string) => { - const path = new URL(url).pathname.replace(`/repos/${repository}/`, '') - assert.ok(path.startsWith('contents/'), `Unexpected GitHub request: ${path}`) - return Response.json({ encoding: 'base64', content: Buffer.from(content).toString('base64') }) - }) - for (const workflow of ['benchmark', 'benchmark-trigger', 'preview']) { - const path = `.github/workflows/${workflow}.yml` - writeFileSync(path, 'trusted workflow') - content = 'trusted workflow' - assert.equal(await workflowTrusted(path, head), true) - content = 'changed workflow' - assert.equal(await workflowTrusted(path, head), false) - } -}) - test('Pages verifies previews with revision and build jobs and excludes draft PRs', async context => { const temporary = mkdtempSync(resolve(tmpdir(), 'benchmark-preview-policy-')) const previousDirectory = process.cwd() @@ -449,9 +420,6 @@ test('Pages verifies previews with revision and build jobs and excludes draft PR head: { sha: 'a'.repeat(40), repo: { full_name: repository } }, base: { sha: 'b'.repeat(40), ref: 'main', repo: { full_name: repository } }, } - const workflow = readFileSync(resolve(root, '.github/workflows/preview.yml'), 'utf8') - mkdirSync('.github/workflows', { recursive: true }) - writeFileSync('.github/workflows/preview.yml', workflow) const run = { id: 123, run_attempt: 1, event: 'pull_request', path: '.github/workflows/preview.yml', conclusion: 'success', head_sha: pr.head.sha, head_repository: pr.head.repo, @@ -479,7 +447,6 @@ test('Pages verifies previews with revision and build jobs and excludes draft PR 'actions/runs/123/attempts/1/jobs': { jobs: [ { name: 'revision', conclusion: 'success' }, { name: 'build', conclusion: 'success' }, ] }, - 'contents/.github/workflows/preview.yml': { encoding: 'base64', content: Buffer.from(workflow).toString('base64') }, } assert.ok(path in responses, `Unexpected GitHub request: ${path}`) return Response.json(responses[path]) @@ -513,9 +480,6 @@ test('CI archival and promotion with simulated GitHub and Git', async context => id: 12345, run_attempt: 1, event: 'pull_request', conclusion: 'success', head_sha: 'a'.repeat(40), pull_requests: [{ number: 7 }], } } writeJson(eventPath, notification, false) - const triggerWorkflow = readFileSync(resolve(root, '.github/workflows/benchmark-trigger.yml'), 'utf8') - let triggerWorkflowContent = triggerWorkflow - const workloadWorkflow = readFileSync(resolve(root, '.github/workflows/benchmark.yml'), 'utf8') const bundle = fixture() const pr = { number: 7, base: { ref: 'main', repo: { full_name: repository }, sha: bundle.run.pullRequest!.base }, @@ -558,8 +522,6 @@ test('CI archival and promotion with simulated GitHub and Git', async context => [`commits/${'d'.repeat(40)}/pulls`]: [pr], 'actions/runs/12345/attempts/1': { id: 12345, event: 'pull_request', conclusion: 'success', path: '.github/workflows/benchmark-trigger.yml', head_sha: workflowHead, created_at: bundle.run.createdAt }, 'actions/runs/12345/attempts/2': { id: 12345, event: 'pull_request', conclusion: 'success', path: '.github/workflows/benchmark-trigger.yml', head_sha: workflowHead, created_at: bundle.run.createdAt }, - 'contents/.github/workflows/benchmark-trigger.yml': { encoding: 'base64', content: Buffer.from(triggerWorkflowContent).toString('base64') }, - 'contents/.github/workflows/benchmark.yml': { encoding: 'base64', content: Buffer.from(workloadWorkflow).toString('base64') }, [`git/commits/${bundle.run.commit.sha}`]: { tree: { sha: bundle.run.commit.tree }, parents: [{ sha: bundle.run.pullRequest!.base }, { sha: bundle.run.pullRequest!.head }] }, [`git/commits/${pr.merge_commit_sha}`]: { tree: { sha: mergedTree }, message: 'Benchmark results (#7)' }, } @@ -579,9 +541,6 @@ test('CI archival and promotion with simulated GitHub and Git', async context => return { status: 0, stdout: command === 'git' && args[0] === 'diff' ? 'fixture changes' : '', stderr: '' } }) syncBuiltinESMExports() - mkdirSync(resolve(temporary, '.github/workflows'), { recursive: true }) - writeFileSync(resolve(temporary, '.github/workflows/benchmark-trigger.yml'), triggerWorkflow) - writeFileSync(resolve(temporary, '.github/workflows/benchmark.yml'), workloadWorkflow) const { main } = await import('./publish-ci.ts') const clearStore = () => { rmSync(resolve(temporary, 'data-store'), { recursive: true, force: true }) @@ -705,23 +664,6 @@ test('CI archival and promotion with simulated GitHub and Git', async context => writeJson(eventPath, notification, false) } }) - await context.test('changed workflows cannot archive before matching main', async () => { - clearStore() - triggerWorkflowContent = `${triggerWorkflow}\n` - pr.merged = false - pr.state = 'open' - try { - await assert.rejects(main(), /workflow files must match current main/) - assert.equal(downloads, 0) - assert.equal(pushes, 0) - assert.equal(statuses.at(-1).state, 'failure') - } finally { - triggerWorkflowContent = triggerWorkflow - pr.merged = true - pr.state = 'closed' - clearStore() - } - }) await context.test('merge with no archive reports failure and requires manual archival', async () => { clearStore() process.env.GITHUB_EVENT_NAME = 'push' diff --git a/scripts/publish-ci.ts b/scripts/publish-ci.ts index 0f443ca..8aed228 100644 --- a/scripts/publish-ci.ts +++ b/scripts/publish-ci.ts @@ -4,7 +4,7 @@ import { resolve } from 'node:path' import { pathToFileURL } from 'node:url' import { publicationPolicy } from '../shared/catalog.ts' import { historyRunPath, validatePublication, validatePublishableRun } from '../shared/results.ts' -import { eligibility, github, pages, workflowTrusted } from './github-pr.ts' +import { eligibility, github, pages } from './github-pr.ts' import { readJson, readOptionalJson, writeJson } from './result-store.ts' import { promoteRun, storeRun } from './publish-results.ts' @@ -45,11 +45,6 @@ async function verifiedRun(runId: number, attempt: number) { if (run.event !== 'pull_request' || run.conclusion !== 'success' || run.path !== '.github/workflows/benchmark-trigger.yml') { throw new Error('Expected a successful PR Benchmark workflow attempt') } - for (const path of ['.github/workflows/benchmark-trigger.yml', '.github/workflows/benchmark.yml']) { - if (!await workflowTrusted(path, run.head_sha)) { - throw new Error('Benchmark workflow files must match current main. After merge, rerun this publication workflow.') - } - } const latestJobs = new Map() for (let currentAttempt = attempt; currentAttempt >= 1; currentAttempt--) { let fetched = 0 @@ -167,13 +162,12 @@ export async function main() { if (notification.event !== 'pull_request' || notification.conclusion !== 'success') return if (notification.pull_requests.length !== 1) throw new Error('Workflow must identify one PR') const number = notification.pull_requests[0].number - const current = await github(`pulls/${number}`) - if (current.head.sha !== notification.head_sha) throw new Error('Workflow is stale. A fresh benchmark run is required.') - await withPublicationStatus(notification.head_sha, () => archive(notification, number)) + const eligibilityResult = await eligibility(number) + if (eligibilityResult.pr.head.sha !== notification.head_sha) throw new Error('Workflow is stale. A fresh benchmark run is required.') + await withPublicationStatus(notification.head_sha, () => archive(notification, number, eligibilityResult)) } -async function archive(notification: any, number: number) { - const { pr, decision } = await eligibility(number) +async function archive(notification: any, number: number, { pr, decision }: Awaited>) { if (decision.mode === 'skip') { console.log(`PR ${number}: approved skip. No results archived.`) return diff --git a/scripts/site-ci.ts b/scripts/site-ci.ts index 0cd9b52..23487a4 100644 --- a/scripts/site-ci.ts +++ b/scripts/site-ci.ts @@ -2,7 +2,7 @@ import { spawnSync } from 'node:child_process' import { existsSync, mkdirSync, rmSync } from 'node:fs' import { resolve } from 'node:path' import { assembleSite, previewBuildSchema } from './assemble-site.ts' -import { eligibility, github, pages, workflowTrusted } from './github-pr.ts' +import { eligibility, github, pages } from './github-pr.ts' import { canonicalJson } from '../shared/results.ts' import { readJson, writeJson } from './result-store.ts' @@ -39,7 +39,6 @@ async function verifiedBuild(run: any, pr: any, artifact: any): Promise || !run.pull_requests.some((entry: { number: number }) => entry.number === pr.number)) return false if (!artifact || artifact.expired || artifact.name !== `preview-${pr.number}-${pr.head.sha}-${run.run_attempt}`) return false if (artifact.size_in_bytes > 100 * 1024 ** 2) throw new Error(`Preview artifact is too large: ${artifact.name}`) - if (!await workflowTrusted('.github/workflows/preview.yml', run.head_sha)) return false const jobs = await github(`actions/runs/${run.id}/attempts/${run.run_attempt}/jobs?per_page=100`) for (const jobName of ['revision', 'build']) { const matches = jobs.jobs.filter((job: { name: string }) => job.name === jobName)