ci: allow only the reviewed GitHub Actions (#650)

.github/agent/action-allowlist.json

@@ -0,0 +1,14 @@
+{
+  "allowed_actions": [
+    "actions/attest-build-provenance",
+    "actions/checkout",
+    "actions/dependency-review-action",
+    "actions/download-artifact",
+    "actions/setup-node",
+    "actions/upload-artifact",
+    "anchore/sbom-action",
+    "aquasecurity/trivy-action",
+    "github/codeql-action",
+    "raven-actions/actionlint"
+  ]
+}

docs/security.md

@@ -787,6 +787,62 @@ script gets a narrow, documented exception — never a blanket re-enable.
 Playwright's browser install stays an explicit step
 (`npx playwright install --with-deps chromium`), so it is unaffected.
 
+### Allowed GitHub Actions
+
+A pin and an allowlist answer two different questions, so both are required. The
+pin (`action-pins` gate 1) says "this action cannot change under us"; it says
+nothing about *who wrote it*. A pull request can add a brand-new third-party
+action pinned to a malicious commit, and that commit runs on our runner with our
+token — reviewed, pinned, and still an exfiltration. The allowlist says "only the
+actions this project has read are allowed".
+
+The list is data in the tree, `.github/agent/action-allowlist.json`, as exact
+`owner/repo` names — no wildcards, because `actions/*` re-opens the hole the list
+exists to close. A sub-path belongs to its repository: `github/codeql-action/init`
+is covered by `github/codeql-action`, since a fork of one sub-action is not a
+different action. `readAllowlist` refuses to load a malformed file rather than
+returning an empty list, and a name in the list that no workflow uses is a test
+failure too — drift in either direction is how a list stops describing the tree.
+
+CI enforces the list in the `action-pins` required job. The checker is read from
+the base branch, so the list it reads is the one already reviewed there; a pull
+request therefore cannot grant itself an action in the same commit that uses it.
+That is deliberate ordering, not an inconvenience: **the list entry lands first,
+in its own reviewed change, and the workflow that uses the action comes after.**
+
+The GitHub setting behind it is the other half, and it is a **repository setting,
+so it is the owner's action, never an agent's**:
+
+| Setting | Where | At the time of writing |
+|---|---|---|
+| `allowed_actions` | Settings → Actions → General → Actions permissions | `all` |
+| `sha_pinning_required` | same page, "Require actions to be pinned to a full-length commit SHA" | `true` |
+
+`GET /repos/{owner}/{repo}/actions/permissions` reports both, and `PATCH` on the
+same path sets them — `{"allowed_actions":"selected","sha_pinning_required":true}`,
+then `PUT /repos/{owner}/{repo}/actions/permissions/selected-actions` with
+`{"github_owned_allowed":false,"verified_allowed":false,"patterns_allowed":[...]}`
+carrying the same `owner/repo` names as the JSON file. The settings page is the
+same switch. Until they are flipped, the list is enforced by CI alone: it stops a
+pull request from merging with an unreviewed action, but it does not stop the
+workflow from starting if one is already on the branch.
+
+Procedure for adding an action:
+
+1. Open a T3 issue naming the action, its 40-hex pin, the tag it came from and
+   why no existing action does the job. An action without that reason is the
+   thing this control exists to refuse.
+2. Add one line to `.github/agent/action-allowlist.json` in its own reviewed
+   change, with the justification in the pull request body. The suite's
+   both-directions test must still pass, so a list entry that no workflow uses
+   cannot ride along.
+3. Land it, then open the workflow change that uses the action. The gate reads the
+   list from the base branch, so reversing the order reds the newer pull request
+   until the list is on `main`.
+4. Only the owner flips `allowed_actions` to `selected` and publishes the same
+   patterns through `patterns_allowed`; an agent may read the settings, never write
+   them.
+
 ### Secret scanning and push protection
 
 Two boundaries, deliberately redundant: GitHub's push protection refuses a

scripts/action-pins.mjs

@@ -1,7 +1,8 @@
 #!/usr/bin/env node
 // Supply-chain gate 1, wired into CI: scan the real workflow tree and fail the
-// job on any action reference that is not a 40-hex pin with its tag comment.
-import { scanActorGates, scanWorkflows } from './lib/action-pins.mjs'
+// job on any action reference that is not a 40-hex pin with its tag comment, or
+// that names an action outside the reviewed allowlist.
+import { ALLOWLIST_FILE, defaultAllowlistPath, readAllowlist, scanActorGates, scanWorkflows } from './lib/action-pins.mjs'
 
 // #640: the checker runs from the base branch, but the workflows it reads are
 // the pull request's own — they are the data under test, so the directory is an
@@ -13,18 +14,36 @@ function argument(name, fallback) {
 }
 
 const directory = argument('--workflows', '.github/workflows')
-const report = scanWorkflows(directory)
+// #650: the list is read relative to this script, which in CI is the base
+// checkout, so a pull request cannot add its own action to the list.
+const allowlistPath = argument('--allowlist', defaultAllowlistPath())
+let allowlist
+try {
+  allowlist = readAllowlist(allowlistPath)
+} catch (error) {
+  console.error(`action-pins: ${error.message}`)
+  process.exit(1)
+}
+
+const report = scanWorkflows(directory, allowlist)
 
 for (const finding of report.findings) {
   const hint =
     finding.problem === 'not-pinned'
       ? 'pin it to a 40-character commit sha and keep the tag as a trailing comment'
-      : 'add the tag it was pinned from as a trailing comment'
+      : finding.problem === 'not-allowed'
+        ? `this action is not on the reviewed list; adding it needs a separate reviewed change to ${ALLOWLIST_FILE} before a workflow may use it`
+        : 'add the tag it was pinned from as a trailing comment'
   console.error(`${finding.file}:${finding.line}: ${finding.value}: ${hint}`)
 }
 
 if (report.findings.length > 0) {
-  console.error(`action-pins: ${report.findings.length} action reference(s) are not pinned`)
+  const notAllowed = report.findings.filter((finding) => finding.problem === 'not-allowed').length
+  const notPinned = report.findings.length - notAllowed
+  const parts = []
+  if (notPinned > 0) parts.push(`${notPinned} action reference(s) are not pinned`)
+  if (notAllowed > 0) parts.push(`${notAllowed} action reference(s) are not on the allowlist`)
+  console.error(`action-pins: ${parts.join('; ')}`)
   process.exit(1)
 }
 
@@ -54,4 +73,4 @@ if (gates.findings.length > 0) {
   process.exit(1)
 }
 
-console.log(`action-pins: ok (${report.checked} pinned references across ${report.files.length} workflow files; ${gates.checked} actions: write job(s) gated on the actor)`)
+console.log(`action-pins: ok (${report.checked} pinned references across ${report.files.length} workflow files, all on the ${allowlist.length}-action allowlist; ${gates.checked} actions: write job(s) gated on the actor)`)

scripts/lib/action-pins.d.mts

@@ -1,7 +1,7 @@
 // Types for the action-pin checker, so the TypeScript suite can call the real
 // implementation and build its fixtures in a temporary directory instead of
 // restating the rules.
-export type ActionPinProblem = 'not-pinned' | 'missing-tag-comment'
+export type ActionPinProblem = 'not-pinned' | 'missing-tag-comment' | 'not-allowed'
 
 export interface ActionPinFinding {
   file: string
@@ -16,9 +16,23 @@ export interface ActionPinReport {
   files: string[]
 }
 
-export declare function checkWorkflow(text: string, file: string): ActionPinFinding[]
+// `allowlist` is the reviewed owner/repo list; omitting it skips the allowlist
+// half of the gate, which the CLI never does.
+export declare function checkWorkflow(text: string, file: string, allowlist?: string[]): ActionPinFinding[]
 
-export declare function scanWorkflows(dir: string): ActionPinReport
+export declare function scanWorkflows(dir: string, allowlist?: string[]): ActionPinReport
+
+/** Where the reviewed list lives, relative to the repository root. */
+export declare const ALLOWLIST_FILE: string
+
+/** Resolves the list next to this module, so a base-branch copy reads its own. */
+export declare function defaultAllowlistPath(): string
+
+/** Throws on a malformed file: an unreadable list must not allow everything. */
+export declare function readAllowlist(path?: string): string[]
+
+/** `github/codeql-action/init@sha` -> `github/codeql-action`. */
+export declare function repositoryOf(value: string): string
 
 // One per reason the gate refuses: an ungated privileged job, an event it cannot
 // classify, a privileged job whose triggers it could not read, and every shape it

scripts/lib/action-pins.mjs

@@ -7,13 +7,71 @@
 // secrets without a commit here, a review, or a diff anyone can see. The trailing
 // comment is required because it is what makes a re-pin a one-line, reviewable
 // edit (`@<sha> # v4.4.0`) instead of an opaque sha swap nobody can check.
+//
+// Gate 1a, #650: a pin stops a tag from moving; it says nothing about who wrote
+// the action. A pull request can add any third-party action pinned to any commit
+// it likes, and that commit runs with this repository's token. So the second
+// half of the rule is a list: only the actions this project has reviewed are
+// allowed, and the list is data in the tree (`ALLOWLIST_FILE`) so widening it is
+// a reviewed one-line diff instead of something a pull request grants itself.
 import { readdirSync, readFileSync } from 'node:fs'
-import { join } from 'node:path'
+import { dirname, join, resolve } from 'node:path'
+import { fileURLToPath } from 'node:url'
 
 const USES_LINE = /^(\s*(?:-\s*)?uses:\s*)(\S+)(.*)$/
 const PINNED = /^[^\s@]+@[0-9a-f]{40}$/
 const TAG_COMMENT = /(^|\s)#\s*\S+/
 
+// The list lives with the other machine-read policy files. `owner/repo` only:
+// a wildcard would re-open exactly the hole the list closes, so `actions/*` is
+// rejected as malformed rather than read as "any action under that owner".
+export const ALLOWLIST_FILE = '.github/agent/action-allowlist.json'
+const ACTION_REPOSITORY = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/
+
+// Resolved from this file, never from the working directory: the checker runs
+// from the base checkout, so the list it reads is the one already reviewed on
+// the base branch. A pull request therefore cannot widen the list for itself in
+// the same commit that uses a new action — the entry has to land first.
+export function defaultAllowlistPath() {
+  return resolve(dirname(fileURLToPath(import.meta.url)), '..', '..', ALLOWLIST_FILE)
+}
+
+// Throws on a malformed file instead of returning an empty list: a gate that
+// silently allows everything when its data is unreadable is worse than no gate.
+export function readAllowlist(path = defaultAllowlistPath()) {
+  let parsed
+  try {
+    parsed = JSON.parse(readFileSync(path, 'utf8'))
+  } catch (error) {
+    throw new Error(`${path}: the allowlist must be a JSON file (${error.message})`)
+  }
+  const list = parsed?.allowed_actions
+  if (!Array.isArray(list) || list.length === 0) {
+    throw new Error(`${path}: allowed_actions must be a non-empty array of owner/repo names`)
+  }
+  for (const entry of list) {
+    if (typeof entry !== 'string' || !ACTION_REPOSITORY.test(entry)) {
+      throw new Error(`${path}: ${JSON.stringify(entry)} is not an exact owner/repo name (wildcards are not allowed)`)
+    }
+  }
+  const duplicates = list.filter((entry, index) => list.indexOf(entry) !== index)
+  if (duplicates.length > 0) {
+    throw new Error(`${path}: ${duplicates.join(', ')} appears more than once`)
+  }
+  return list
+}
+
+// `github/codeql-action/init@<sha>` belongs to `github/codeql-action`: the
+// allowlist names repositories, and a sub-path is part of the repository it
+// lives in rather than a separate entry someone could forget to classify.
+export function repositoryOf(value) {
+  const at = value.indexOf('@')
+  const path = at === -1 ? value : value.slice(0, at)
+  const segments = path.split('/')
+  if (segments.length < 2) return path
+  return `${segments[0]}/${segments[1]}`
+}
+
 // A local action (`./path`) is a file in this repository and is reviewed like
 // any other file; a `docker://` reference is pinned by digest at the call site
 // and is not an action at all. Both are out of scope for this gate, and the
@@ -35,7 +93,11 @@ function referenceOf(line) {
 
 // Returns one finding per `uses:` that is not a full-sha pin with a tag comment.
 // `file` is carried through so a caller scanning a tree can name the file.
-export function checkWorkflow(text, file) {
+// `allowlist` is the reviewed `owner/repo` list; omitting it skips gate 1a, which
+// the CLI never does — it always passes what it read, so a caller that forgets
+// the list cannot make the gate look green.
+export function checkWorkflow(text, file, allowlist) {
+  const allow = allowlist === undefined ? null : new Set(allowlist.map((entry) => entry.toLowerCase()))
   const findings = []
   const lines = text.split(/\r?\n/)
   for (let index = 0; index < lines.length; index += 1) {
@@ -48,14 +110,19 @@ export function checkWorkflow(text, file) {
     if (!TAG_COMMENT.test(` ${reference.rest}`)) {
       findings.push({ file, line: index + 1, value: reference.value, problem: 'missing-tag-comment' })
     }
+    // Checked even when the reference is otherwise malformed, so a workflow that
+    // adds an unreviewed action is told about the action as well as the pin.
+    if (allow !== null && !allow.has(repositoryOf(reference.value).toLowerCase())) {
+      findings.push({ file, line: index + 1, value: reference.value, problem: 'not-allowed' })
+    }
   }
   return findings
 }
 
 // Scans every *.yml / *.yaml directly under `dir`. `checked` counts the action
 // references in scope, so a run that found nothing to check cannot look the same
 // as a run where everything passed.
-export function scanWorkflows(dir) {
+export function scanWorkflows(dir, allowlist) {
   const findings = []
   const files = []
   let checked = 0
@@ -64,7 +131,7 @@ export function scanWorkflows(dir) {
     const file = join(dir, entry.name)
     files.push(file)
     const text = readFileSync(file, 'utf8')
-    findings.push(...checkWorkflow(text, file))
+    findings.push(...checkWorkflow(text, file, allowlist))
     for (const line of text.split(/\r?\n/)) {
       const reference = referenceOf(line)
       if (reference !== null && !outOfScope(reference.value)) checked += 1

test/actionPins.test.ts

@@ -1,9 +1,9 @@
 import test from 'node:test'
 import assert from 'node:assert/strict'
-import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'
+import { mkdtempSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
-import { checkActorGates, checkWorkflow, scanActorGates, scanWorkflows } from '../scripts/lib/action-pins.mjs'
+import { ALLOWLIST_FILE, checkActorGates, checkWorkflow, readAllowlist, repositoryOf, scanActorGates, scanWorkflows } from '../scripts/lib/action-pins.mjs'
 
 // A real pin, the one this repository uses for actions/checkout.
 const PIN = '11d5960a326750d5838078e36cf38b85af677262'
@@ -338,3 +338,78 @@ test('an event the gate cannot classify is a finding until it is classified', ()
     assert.deepEqual(checkActorGates(workflow(`  ${safe}:`, privilegedJob('repair', null)), 'fixture.yml'), [], `${safe} cannot be started by an outside account`)
   }
 })
+
+// #650: a pin stops a tag from moving; it says nothing about who wrote the
+// action. The policy also allows only the actions this project has reviewed, and
+// the list is data in the tree so a new action is a reviewable one-line addition
+// rather than something a pull request grants itself.
+test('an action outside the allowlist is refused even when it is fully pinned', () => {
+  const text = `      - uses: stranger/exfiltrate@${PIN} # v1.0.0\n`
+  assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout']), [
+    { file: 'fixture.yml', line: 1, value: `stranger/exfiltrate@${PIN}`, problem: 'not-allowed' },
+  ])
+})
+
+test('an allowlisted action passes, and a sub-path belongs to its repository', () => {
+  const text = [
+    `      - uses: actions/checkout@${PIN} # v4.4.0`,
+    `      - uses: github/codeql-action/init@${PIN} # v3.28.1`,
+  ].join('\n')
+  assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout', 'github/codeql-action']), [])
+})
+
+test('the list is what admits an action, so widening it is a visible one-line change', () => {
+  const text = `      - uses: newvendor/tool@${PIN} # v2.0.0\n`
+  assert.equal(checkWorkflow(text, 'fixture.yml', ['actions/checkout']).length, 1)
+  assert.deepEqual(checkWorkflow(text, 'fixture.yml', ['actions/checkout', 'newvendor/tool']), [])
+})
+
+test('the workflow tree in this repository passes the allowlist, not just the pin check', () => {
+  const allowlist = readAllowlist()
+  assert.ok(allowlist.length >= 10, `the reviewed list has shrunk to ${allowlist.length} entries`)
+  assert.deepEqual(scanWorkflows('.github/workflows', allowlist).findings, [])
+})
+
+// The list is the policy, so drift in either direction is a defect: an action in
+// the tree that nobody added to the list (the gate would red the night after the
+// merge) and an entry nobody uses (the next reader cannot tell whether it is a
+// leftover or a mistake). Both are cheap to assert here and invisible otherwise.
+test('the allowlist and the workflow tree name exactly the same repositories', () => {
+  const directory = '.github/workflows'
+  const used = new Set<string>()
+  for (const name of readdirSync(directory)) {
+    if (!/\.ya?ml$/i.test(name)) continue
+    for (const line of readFileSync(join(directory, name), 'utf8').split(/\r?\n/)) {
+      const match = /^\s*(?:-\s*)?uses:\s*(\S+)/.exec(line)
+      if (match === null) continue
+      const value = match[1]?.replace(/^(['"])(.*)\1$/, '$2') ?? ''
+      if (value.startsWith('./') || value.startsWith('docker://')) continue
+      used.add(repositoryOf(value))
+    }
+  }
+  const listed = new Set<string>(readAllowlist())
+  assert.deepEqual([...used].filter((repo) => !listed.has(repo)), [], 'a workflow uses an action the list does not name')
+  assert.deepEqual([...listed].filter((repo) => !used.has(repo)), [], 'the list names an action no workflow uses')
+})
+
+test('a malformed allowlist refuses to load instead of allowing everything', (t) => {
+  const dir = mkdtempSync(join(tmpdir(), 'action-allow-'))
+  t.after(() => rmSync(dir, { recursive: true, force: true }))
+  const write = (name: string, body: string): string => {
+    const path = join(dir, name)
+    writeFileSync(path, body)
+    return path
+  }
+
+  assert.deepEqual(readAllowlist(write('ok.json', JSON.stringify({ allowed_actions: ['actions/checkout'] }))), ['actions/checkout'])
+  // A wildcard is the hole this list exists to close, so it is not "any action".
+  assert.throws(() => readAllowlist(write('wildcard.json', JSON.stringify({ allowed_actions: ['actions/*'] }))), /wildcards are not allowed/)
+  assert.throws(() => readAllowlist(write('empty.json', JSON.stringify({ allowed_actions: [] }))), /non-empty array/)
+  assert.throws(() => readAllowlist(write('dupe.json', JSON.stringify({ allowed_actions: ['actions/checkout', 'actions/checkout'] }))), /appears more than once/)
+  assert.throws(() => readAllowlist(write('missing.json', '')), /must be a JSON file/)
+})
+
+test('the shipped allowlist file is where the checker looks by default', () => {
+  assert.match(ALLOWLIST_FILE, /^\.github\/agent\/[a-z-]+\.json$/)
+  assert.ok(readAllowlist().includes('actions/checkout'), 'the default path must resolve to the reviewed list in this tree')
+})