From c6fa50f5678ace85e67b3966c71280efb80064eb Mon Sep 17 00:00:00 2001 From: cryppadotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:57:53 +0000 Subject: [PATCH] fix(ci): verify trusted workflow provenance before review notifications Co-Authored-By: Paperclip --- .github/scripts/notify-pr-merge-review.mjs | 45 +++++++++--- .../tests/notify-pr-merge-review.test.mjs | 68 ++++++++++++++++--- doc/PR-MERGE-REVIEW-WEBHOOK.md | 12 +++- 3 files changed, 106 insertions(+), 19 deletions(-) diff --git a/.github/scripts/notify-pr-merge-review.mjs b/.github/scripts/notify-pr-merge-review.mjs index 63ca5cc4895..d8151919ba0 100644 --- a/.github/scripts/notify-pr-merge-review.mjs +++ b/.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; diff --git a/.github/scripts/tests/notify-pr-merge-review.test.mjs b/.github/scripts/tests/notify-pr-merge-review.test.mjs index baff48c312b..56dca3294ce 100644 --- a/.github/scripts/tests/notify-pr-merge-review.test.mjs +++ b/.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]); }); diff --git a/doc/PR-MERGE-REVIEW-WEBHOOK.md b/doc/PR-MERGE-REVIEW-WEBHOOK.md index a1eab6bd0eb..b9e63a50672 100644 --- a/doc/PR-MERGE-REVIEW-WEBHOOK.md +++ b/doc/PR-MERGE-REVIEW-WEBHOOK.md @@ -40,8 +40,16 @@ Other checks must succeed or finish as skipped or neutral. Optional skipped or neutral checks permit an assessment; they do not prove successful verification. For example, a neutral scanner can mean coverage is unavailable. The advisory routine evaluates these evidence gaps and reviews before recommending merge. -The notification workflow itself and the optional CI profile exporter are -excluded from verification checks to avoid feedback loops. +Workflow names do not exclude checks. The notifier and CI profile exporter run +on the default branch, so their own checks do not belong to the PR revision. + +Check names alone are not trusted. Both core aggregates must belong to the same +GitHub Actions suite and a successful run of the repository's actual +`.github/workflows/pr.yml`. Its run metadata must reference +`pr-trusted.yml@master`. The complete PR file list must prove that the caller +`pr.yml` was not changed or renamed. A missing or truncated list stops delivery. +PRs that change this caller need a manual advisory review until their caller +change reaches the trusted branch. Other workflow changes can still be reviewed. A final refresh rejects head or base changes during evaluation. Paperclip's idempotency receipt deduplicates events for the same PR, head, and base. A new