fix(ci): verify trusted workflow provenance before review notifications

Co-Authored-By: Paperclip <noreply@paperclip.ing>

.github/scripts/notify-pr-merge-review.mjs

@@ -3,7 +3,6 @@ import { readFileSync } from "node:fs";
 import { fileURLToPath } from "node:url";
 
 const repository = "paperclipai/paperclip";
-const workflowName = "PR merge review notification";
 const shaPattern = /^[a-f0-9]{40}$/;
 const minimumChecks = ["ci / verify", "ci / e2e"];
 const gh = (args) => JSON.parse(execFileSync("gh", args, {
@@ -16,12 +15,13 @@ const pages = (endpoint) => gh(["api", "--paginate", "--slurp", endpoint]).flat(
 const query = `query($number:Int!, $endCursor:String) {
   repository(owner:"paperclipai", name:"paperclip") {
     pullRequest(number:$number) {
-      number url state isDraft headRefOid baseRefOid baseRefName
+      number url state isDraft headRefOid baseRefOid baseRefName changedFiles
       statusCheckRollup { contexts(first:100, after:$endCursor) {
         totalCount pageInfo { hasNextPage endCursor }
         nodes {
           __typename
-          ... on CheckRun { name status conclusion checkSuite { workflowRun { workflow { name } } } }
+          ... on CheckRun { name status conclusion checkSuite { databaseId app { databaseId }
+            workflowRun { databaseId } } }
           ... on StatusContext { context state }
         }
       } }
@@ -44,9 +44,7 @@ export function snapshot(number, request = gh) {
     }
     candidate = pr;
     const contexts = pr.statusCheckRollup.contexts;
-    checks.push(...contexts.nodes.map((check) => ({ ...check,
-      workflowName: check.checkSuite?.workflowRun?.workflow?.name,
-    })));
+    checks.push(...contexts.nodes);
     if (!contexts.pageInfo.hasNextPage) {
       if (checks.length !== contexts.totalCount) throw new Error("Incomplete check listing");
       break;
@@ -64,7 +62,7 @@ export function green(pr, requiredNames = minimumChecks) {
       !shaPattern.test(pr.headRefOid) || !shaPattern.test(pr.baseRefOid)) return false;
   const checks = pr.statusCheckRollup;
   if (!Array.isArray(checks) || checks.length === 0) return false;
-  const relevant = checks.filter((check) => ![workflowName, "CI profile"].includes(check.workflowName));
+  const relevant = checks;
   const passed = (check) => check.__typename === "StatusContext"
     ? check.state === "SUCCESS"
     : check.__typename === "CheckRun" && check.status === "COMPLETED" && check.conclusion === "SUCCESS";
@@ -76,6 +74,36 @@ export function green(pr, requiredNames = minimumChecks) {
       relevant.some((check) => (check.name ?? check.context) === name && passed(check)));
 }
 
+export function trustedVerification(pr, { read = api, list = pages } = {}) {
+  // Names alone are forgeable in pull_request YAML. Require the real Actions
+  // suite for the unmodified PR caller and its master-owned reusable workflow.
+  const actionsChecks = pr.statusCheckRollup.filter((check) =>
+    check.__typename === "CheckRun" && check.status === "COMPLETED" && check.conclusion === "SUCCESS" &&
+    check.checkSuite?.app?.databaseId === 15368 &&
+    Number.isSafeInteger(check.checkSuite?.workflowRun?.databaseId));
+  const runIds = new Set(actionsChecks.filter((check) => minimumChecks.includes(check.name))
+    .map((check) => check.checkSuite.workflowRun.databaseId));
+  let trusted = false;
+  for (const id of runIds) {
+    const run = read(`/repos/${repository}/actions/runs/${id}`);
+    if (run.repository?.full_name !== repository || run.path !== ".github/workflows/pr.yml" ||
+        run.event !== "pull_request" || run.head_sha !== pr.headRefOid ||
+        run.status !== "completed" || run.conclusion !== "success") continue;
+    if (!minimumChecks.every((name) => actionsChecks.some((check) => check.name === name &&
+        check.checkSuite.workflowRun.databaseId === id && check.checkSuite.databaseId === run.check_suite_id))) continue;
+    const reusable = run.referenced_workflows?.find((entry) =>
+      entry.path === `${repository}/.github/workflows/pr-trusted.yml@master` &&
+      entry.ref === "refs/heads/master" && shaPattern.test(entry.sha));
+    if (reusable) { trusted = true; break; }
+  }
+  if (!trusted || !Number.isSafeInteger(pr.changedFiles) || pr.changedFiles < 0) return false;
+  const files = list(`/repos/${repository}/pulls/${pr.number}/files?per_page=100`);
+  // GitHub caps this API at 3,000 files. A partial listing cannot prove that
+  // the caller is unmodified. Do not execute or download any of these files.
+  return files.length === pr.changedFiles && files.every((file) =>
+    typeof file.filename === "string" && ![file.filename, file.previous_filename].includes(".github/workflows/pr.yml"));
+}
+
 export async function deliver(pr, { url, token, fetcher = fetch } = {}) {
   if (!url || !token) return "unconfigured";
   try {
@@ -123,7 +151,8 @@ export async function notify(event, { read = api, list = pages, view = snapshot,
     // describe this PR's current head; an old successful run cannot trigger it.
     if ((run?.event !== "pull_request_target" && pr.headRefOid !== sha) || !green(pr, required)) continue;
     const fresh = view(number);
-    if (fresh.headRefOid !== pr.headRefOid || fresh.baseRefOid !== pr.baseRefOid || !green(fresh, required)) continue;
+    if (fresh.headRefOid !== pr.headRefOid || fresh.baseRefOid !== pr.baseRefOid ||
+        !green(fresh, required) || !trustedVerification(fresh, { read, list })) continue;
     outcomes.push(await send(fresh));
   }
   return outcomes;

.github/scripts/tests/notify-pr-merge-review.test.mjs

@@ -4,20 +4,31 @@ import { readFileSync, mkdtempSync, writeFileSync, rmSync } from "node:fs";
 import { spawnSync } from "node:child_process";
 import { tmpdir } from "node:os";
 import path from "node:path";
-import { green, deliver, notify, snapshot } from "../notify-pr-merge-review.mjs";
+import { green, deliver, notify, snapshot, trustedVerification } from "../notify-pr-merge-review.mjs";
 
 const head = "a".repeat(40);
 const base = "b".repeat(40);
 const check = (name, conclusion = "SUCCESS", extra = {}) => ({
-  __typename: "CheckRun", name, status: "COMPLETED", conclusion, workflowName: "PR", ...extra,
+  __typename: "CheckRun", name, status: "COMPLETED", conclusion, workflowName: "PR",
+  checkSuite: { databaseId: 8, app: { databaseId: 15368 }, workflowRun: { databaseId: 7 } },
+  ...extra,
 });
 const candidate = (extra = {}) => ({
   number: 42, state: "OPEN", isDraft: false, headRefOid: head, baseRefOid: base,
-  baseRefName: "master", statusCheckRollup: [check("ci / verify"), check("ci / e2e")], ...extra,
+  baseRefName: "master", changedFiles: 1, statusCheckRollup: [check("ci / verify"), check("ci / e2e")], ...extra,
 });
 const event = (extra = {}) => ({ repository: { full_name: "paperclipai/paperclip" }, sha: head, ...extra });
+const trustedRun = (extra = {}) => ({
+  id: 7, repository: { full_name: "paperclipai/paperclip" }, path: ".github/workflows/pr.yml",
+  event: "pull_request", head_sha: head, status: "completed", conclusion: "success", check_suite_id: 8,
+  referenced_workflows: [{ path: "paperclipai/paperclip/.github/workflows/pr-trusted.yml@master",
+    ref: "refs/heads/master", sha: base }], ...extra,
+});
+const files = [{ filename: "README.md" }];
+const read = (endpoint) => endpoint.includes("/rules/") ? [] : trustedRun();
+const list = (endpoint) => endpoint.includes("/files?") ? files : [{ number: 42 }];
 const deps = (extra = {}) => ({
-  read: () => [], list: () => [{ number: 42 }], view: () => candidate(),
+  read, list, view: () => candidate(),
   send: async () => "delivered", ...extra,
 });
 
@@ -39,7 +50,7 @@ test("reads every check page and fails closed on incomplete or changing listings
     statusCheckRollup: { contexts: { totalCount: 3, nodes: [...candidate().statusCheckRollup, own],
       pageInfo: { hasNextPage: false } } },
   }) } } }));
-  assert.equal(green(withOwn), true);
+  assert.equal(green(withOwn), false);
   assert.throws(() => snapshot(42, () => page([check("ci / verify")], false)), /Incomplete/);
   calls = 0;
   assert.throws(() => snapshot(42, () => ++calls === 1 ? page([check("ci / verify")], true)
@@ -70,10 +81,10 @@ test("waits for pending checks and statuses, permits skipped optional routing la
     check("optional scanner coverage", "NEUTRAL")] })), true);
 });
 
-test("excludes its own notification and profiling exporter, but not other workflows", () => {
+test("workflow names cannot hide pending or failing checks", () => {
   const extras = ["PR merge review notification", "CI profile"].map((workflowName) =>
     check("notify", "", { status: "IN_PROGRESS", workflowName }));
-  assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, ...extras] })), true);
+  assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup, ...extras] })), false);
   assert.equal(green(candidate({ statusCheckRollup: [...candidate().statusCheckRollup,
     check("another", "", { status: "QUEUED", workflowName: "Other verification" })] })), false);
 });
@@ -87,7 +98,7 @@ test("resolves fork PRs from the head commit when workflow payload has no PRs",
   const seen = [];
   const outcomes = await notify(event({ workflow_run: {
     status: "completed", event: "pull_request", head_sha: head, pull_requests: [],
-  } }), deps({ list: (endpoint) => { seen.push(endpoint); return [{ number: 42 }]; } }));
+  } }), deps({ list: (endpoint) => { seen.push(endpoint); return list(endpoint); } }));
   assert.deepEqual(outcomes, ["delivered"]);
   assert.match(seen[0], new RegExp(`/commits/${head}/pulls\\?per_page=100$`));
 });
@@ -98,6 +109,44 @@ test("reads additional required rules and holds until their check succeeds", asy
   assert.deepEqual(outcomes, []);
 });
 
+test("forged successful check names from a PR-controlled workflow cannot notify", async () => {
+  const input = deps({ read: (endpoint) => endpoint.includes("/rules/") ? [] : {
+    id: 7, path: ".github/workflows/attacker.yml", event: "pull_request",
+    head_sha: head, status: "completed", conclusion: "success",
+  } });
+  assert.deepEqual(await notify(event(), input), []);
+});
+
+test("requires genuine suite, caller, revision, and master reusable workflow provenance", () => {
+  assert.equal(trustedVerification(candidate(), deps()), true);
+  for (const changes of [
+    { path: ".github/workflows/attacker.yml" }, { event: "pull_request_target" },
+    { head_sha: "c".repeat(40) }, { status: "in_progress" }, { conclusion: "failure" },
+    { repository: { full_name: "attacker/repo" } }, { check_suite_id: 99 },
+    { referenced_workflows: [] },
+    { referenced_workflows: [{ path: "paperclipai/paperclip/.github/workflows/pr-trusted.yml@master",
+      ref: "refs/pull/42/merge", sha: base }] },
+  ]) {
+    assert.equal(trustedVerification(candidate(), deps({ read: () => trustedRun(changes) })), false);
+  }
+  const wrongSuite = check("ci / verify", "SUCCESS", {
+    checkSuite: { databaseId: 99, app: { databaseId: 1 }, workflowRun: { databaseId: 7 } },
+  });
+  assert.equal(trustedVerification(candidate({
+    statusCheckRollup: [wrongSuite, check("ci / e2e")],
+  }), deps()), false);
+});
+
+test("modified or renamed caller and truncated file listings cannot notify", async () => {
+  for (const fileList of [[], [{ filename: ".github/workflows/pr.yml" }],
+    [{ filename: "moved.yml", previous_filename: ".github/workflows/pr.yml" }]]) {
+    const dependencies = deps({ list: (endpoint) => endpoint.includes("/files?")
+      ? fileList : [{ number: 42 }] });
+    assert.deepEqual(await notify(event(), dependencies), []);
+  }
+  assert.equal(trustedVerification(candidate({ changedFiles: undefined }), deps()), false);
+});
+
 test("rejects stale head events and a head, base, or check change during refresh", async () => {
   assert.deepEqual(await notify(event({ sha: "c".repeat(40) }), deps()), []);
   for (const extra of [{ headRefOid: "c".repeat(40) }, { baseRefOid: "d".repeat(40) },
@@ -129,7 +178,8 @@ test("does not query foreign repositories, malformed events, or non-PR workflows
 
 test("does not coalesce different PRs, and deduplicates association entries", async () => {
   const sent = [];
-  await notify(event(), deps({ list: () => [{ number: 42 }, { number: 42 }, { number: 43 }],
+  await notify(event(), deps({ list: (endpoint) => endpoint.includes("/files?") ? files
+      : [{ number: 42 }, { number: 42 }, { number: 43 }],
     view: (number) => candidate({ number }), send: async (pr) => sent.push(pr.number) }));
   assert.deepEqual(sent, [42, 43]);
 });