From 851ac68a0fae4abacc3b37f04b4924fb7f298f52 Mon Sep 17 00:00:00 2001 From: Leo Date: Sun, 27 Sep 2026 01:08:26 -0400 Subject: [PATCH 1/2] feat: exempt merge commits vouched for by a trusted commit status A workflow that merges the base branch into open pull requests authors its merge commits as its own bot account (github-actions[bot] for the Actions token). That account cannot sign, so every caught-up pull request failed the check. Two new inputs, trusted-merge-status-context and trusted-merge-status-creator-ids, let such a workflow vouch for the exact merge commits it verified: a commit with two or more parents contributes no identities when this repository's newest status with that context is a success created by a configured account ID. Nothing in git metadata is trusted. Statuses are read from this repository by REST and need statuses write access here to create, so a fork cannot vouch for its own commits (GitHub-signed github-actions[bot] commits, by contrast, can be minted in any fork). A status names one exact commit, only the newest status for the context counts, lookups are bounded, and a partial or malformed configuration fails the run. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 + README.md | 6 + .../integration/trustedMergeStatus.test.ts | 216 ++++++++++++++++++ __tests__/testHelpers/fakeGithubCore.ts | 40 +++- action.yml | 14 ++ dist/index.js | 94 +++++++- src/graphql.ts | 20 +- src/shared/getInputs.ts | 11 + src/shared/limits.ts | 4 + src/trustedMergeStatus.ts | 93 ++++++++ 10 files changed, 503 insertions(+), 3 deletions(-) create mode 100644 __tests__/integration/trustedMergeStatus.test.ts create mode 100644 src/trustedMergeStatus.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 720b3c58..81e8f446 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,14 @@ commit that introduced it. ## Unreleased +### Added + +- `trusted-merge-status-context` and `trusted-merge-status-creator-ids` + exempt a merge commit from signing when this repository's newest commit + status with that context is a success created by a configured account. + Automated base-branch merges authored by a bot no longer fail the check, + and no git name, email, or signature is trusted. + ### Security - The write-capable signer now emits `cla_passed=true` only after final diff --git a/README.md b/README.md index 49769f0c..eee70e1f 100644 --- a/README.md +++ b/README.md @@ -264,6 +264,10 @@ and `remote-repository-name`: `` in your CLA workflow file. Use `allowlist-ids` for maintainers and documented automation accounts that never need to sign. Values are comma-separated numeric GitHub database IDs. A configured ID is exempt as the Pull Request opener and as a primary commit author, and an allowlisted opener also bypasses the opener-authorship hard-fail. Unlinked identities have no ID and can never match. The deprecated `allowlist` name, email, and glob input is ignored because commit metadata can spoof those values. +##### Trusted merge status + +A workflow that merges the base branch into open Pull Requests (a "catch-up" bot) authors merge commits as its own bot account, which cannot sign. To exempt those merges without trusting any git metadata, have that workflow post a commit status on each merge commit it verified, then set `trusted-merge-status-context` to the status context and `trusted-merge-status-creator-ids` to the numeric ID of the account that posts it (41898282 for `github-actions[bot]` using the workflow's `GITHUB_TOKEN`). A commit is exempt only when it has two or more parents and this repository's newest status with that context is a success created by a configured account. Statuses are stored per repository and need statuses write access to create, so a fork cannot vouch for its own commits. Post the status before the merge reaches the Pull Request branch, for example by pushing the commit to a scratch ref first, so the check that the push starts already sees it. + ##### Demo for step 5 ![allowlist](https://github.com/cla-assistant/github-action/blob/master/images/allowlist.gif?raw=true) @@ -299,6 +303,8 @@ Do not configure `PERSONAL_ACCESS_TOKEN` when signatures stay in the current rep | `expected-comment-created-at` | _optional_ | Exact creation timestamp emitted by `signer-preflight` for the authenticated signing comment. Required when `expected-comment-id` is set. | `${{ steps.preflight.outputs.comment_created_at }}` | | `expected-comment-author-id` | _optional_ | Numeric GitHub account ID emitted by `signer-preflight` for the authenticated signing comment. Required when `expected-comment-id` is set. | `${{ steps.preflight.outputs.comment_author_id }}` | | `allowlist-ids` | _optional_ | Comma-separated numeric GitHub user IDs that never need to sign, as the opener or as a primary commit author. Unlinked identities cannot match. | Maintainer and reviewed automation account IDs. | +| `trusted-merge-status-context` | _optional_ | Commit status context that exempts a merge commit's primary author when this repository's newest status with that context is a success from a configured creator. Set together with `trusted-merge-status-creator-ids`. | `cmux/catch-up` | +| `trusted-merge-status-creator-ids` | _optional_ | Comma-separated numeric GitHub account IDs whose trusted merge status counts. | 41898282 | | `allowlist` | _deprecated_ | Ignored. Raw names, emails, and globs are unsafe identity evidence. | | | `remote-repository-name` | _optional_ | provide the remote repository name where all the signatures should be stored . | remote repository name | | `remote-organization-name` | _optional_ | provide the remote organization name where all the signatures should be stored. | remote organization name | diff --git a/__tests__/integration/trustedMergeStatus.test.ts b/__tests__/integration/trustedMergeStatus.test.ts new file mode 100644 index 00000000..d1486a71 --- /dev/null +++ b/__tests__/integration/trustedMergeStatus.test.ts @@ -0,0 +1,216 @@ +import * as core from '@actions/core' +import { installFakeGitHub, FakeGitHub } from '../testHelpers/fakeGithub' +import { CommitStatusFixture } from '../testHelpers/fakeGithubCore' +import { resetEnv, setDefaultInputs } from '../testHelpers/env' +import { reloadOctokit, setContext } from '../testHelpers/context' + +const MERGE_SHA = 'a'.repeat(40) +const ACTIONS_BOT = { login: 'github-actions[bot]', id: 41898282 } +const CATCH_UP = 'cmux/catch-up' + +async function runAction() { + reloadOctokit() + for (const path of Object.keys(require.cache)) { + if (path.includes('/src/')) delete require.cache[path] + } + const { run } = require('../../src/main') as typeof import('../../src/main') + await run() +} + +function watchCore() { + const failed = jest.spyOn(core, 'setFailed').mockImplementation(() => {}) + const info = jest.spyOn(core, 'info').mockImplementation(() => {}) + const output = jest.spyOn(core, 'setOutput').mockImplementation(() => {}) + return { + get failures() { + return failed.mock.calls.map(c => String(c[0])) + }, + get outputs() { + return output.mock.calls.map(c => [c[0], c[1]]) + }, + restore() { + failed.mockRestore() + info.mockRestore() + output.mockRestore() + } + } +} + +describe('trusted merge status', () => { + let fake: FakeGitHub + + afterEach(async () => { + await fake.close() + resetEnv() + }) + + function setUp(options: { + inputs?: Record + parentCount?: number + statuses?: CommitStatusFixture[] + forkStatuses?: CommitStatusFixture[] + }) { + setDefaultInputs({ + 'path-to-signatures': 'signatures/cla.json', + 'trusted-merge-status-context': CATCH_UP, + 'trusted-merge-status-creator-ids': String(ACTIONS_BOT.id), + ...(options.inputs || {}) + }) + fake = installFakeGitHub() + const repository = fake.repo('acme', 'widgets') + repository.addPullRequest({ + number: 12, + head: { sha: 'headsha', ref: 'feature/cla' }, + user: { login: 'alice', id: 1001 }, + commits: [ + { author: { login: 'alice', id: 1001 } }, + { + oid: MERGE_SHA, + parentCount: options.parentCount ?? 2, + author: ACTIONS_BOT + } + ] + }) + repository.setFile('signatures/cla.json', { + signedContributors: [{ name: 'alice', id: 1001 }] + }) + // Oldest first here; addCommitStatus lists the newest first. + for (const status of options.statuses || []) { + repository.addCommitStatus(MERGE_SHA, status) + } + for (const status of options.forkStatuses || []) { + fake.repo('mallory', 'widgets').addCommitStatus(MERGE_SHA, status) + } + setContext({ + owner: 'acme', + repo: 'widgets', + issueNumber: 12, + actor: 'alice', + eventName: 'pull_request_target', + payload: { + pull_request: { number: 12, state: 'open' }, + repository: { id: repository.state.id }, + action: 'synchronize' + } + }) + } + + const success: CommitStatusFixture = { + context: CATCH_UP, + state: 'success', + creator: ACTIONS_BOT + } + + it('skips a merge commit vouched for by a trusted status in this repository', async () => { + setUp({ statuses: [success] }) + const watch = watchCore() + await runAction() + expect(watch.failures).toEqual([]) + expect(watch.outputs).toContainEqual(['cla_passed', true]) + expect( + fake.requestLog.some( + r => + r.method === 'GET' && + r.path.startsWith(`/repos/acme/widgets/commits/${MERGE_SHA}/statuses`) + ) + ).toBe(true) + watch.restore() + }) + + it('requires the merge author to sign without a status', async () => { + setUp({}) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + watch.restore() + }) + + it('ignores a status created by an account that is not configured', async () => { + setUp({ + statuses: [ + { context: CATCH_UP, state: 'success', creator: { login: 'x', id: 7 } } + ] + }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + watch.restore() + }) + + it('honors only the newest status for the context', async () => { + setUp({ statuses: [success, { ...success, state: 'failure' }] }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + watch.restore() + }) + + it('never exempts a single-parent commit', async () => { + setUp({ parentCount: 1, statuses: [success] }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + watch.restore() + }) + + it('ignores a status that exists only in another repository', async () => { + setUp({ forkStatuses: [success] }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + watch.restore() + }) + + it('reads no statuses when the feature is not configured', async () => { + setUp({ + inputs: { + 'trusted-merge-status-context': '', + 'trusted-merge-status-creator-ids': '' + }, + statuses: [success] + }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + expect(fake.requestLog.some(r => r.path.includes('/statuses'))).toBe(false) + watch.restore() + }) + + it('fails closed on a partial configuration', async () => { + setUp({ + inputs: { 'trusted-merge-status-creator-ids': '' }, + statuses: [success] + }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /trusted-merge-status-context and trusted-merge-status-creator-ids must be provided together/ + ) + watch.restore() + }) + + it('fails closed on malformed creator IDs', async () => { + setUp({ + inputs: { 'trusted-merge-status-creator-ids': 'github-actions[bot]' }, + statuses: [success] + }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /must be comma-separated numeric GitHub account IDs/ + ) + watch.restore() + }) +}) diff --git a/__tests__/testHelpers/fakeGithubCore.ts b/__tests__/testHelpers/fakeGithubCore.ts index fab11e3e..62eb3edc 100644 --- a/__tests__/testHelpers/fakeGithubCore.ts +++ b/__tests__/testHelpers/fakeGithubCore.ts @@ -53,6 +53,10 @@ export interface PullRequest { merged?: boolean state?: 'open' | 'closed' commits: Array<{ + /** Commit SHA; defaults to a unique 40-hex value per commit. */ + oid?: string + /** Number of parents; 2 or more is a merge commit. Defaults to 1. */ + parentCount?: number author: GitActorFixture committer?: GitActorFixture coAuthors?: GitActorFixture[] @@ -83,8 +87,16 @@ export interface Workflow { runs: WorkflowRun[] } +export interface CommitStatusFixture { + context: string + state: 'success' | 'failure' | 'pending' | 'error' + creator: { login: string; id: number } | null +} + export interface RepoState { id: number + /** Commit statuses per SHA, newest first like the REST API. */ + statuses: Map files: Map comments: Map pulls: Map @@ -104,6 +116,8 @@ export interface FakeRepoHandle { listComments(issueNumber: number): Comment[] isLocked(issueNumber: number): boolean addWorkflow(name: string, runs?: WorkflowRun[]): Workflow + /** Record a commit status; the newest is listed first. */ + addCommitStatus(sha: string, status: CommitStatusFixture): FakeRepoHandle state: RepoState } @@ -155,6 +169,7 @@ export function createFakeGitHubCore(): FakeGitHubCore { r = { id: nextRepoId++, files: new Map(), + statuses: new Map(), comments: new Map(), pulls: new Map(), workflows: [], @@ -328,6 +343,21 @@ export function createFakeGitHubCore(): FakeGitHubCore { type: 'Bot' }) }) + addRoute( + getRoutes, + '/repos/:owner/:repo/commits/:sha/statuses', + (m, _body, query) => { + const owner = decodeURIComponent(m[1]!) + const name = decodeURIComponent(m[2]!) + const s = decodeURIComponent(m[3]!) + const all = getRepo(owner, name).statuses.get(s) || [] + return paginate( + all, + query, + `/repos/${owner}/${name}/commits/${s}/statuses` + ) + } + ) addRoute(getRoutes, '/repos/:owner/:repo/git/commits/:sha', m => { const s = decodeURIComponent(m[3]!) return json(200, { sha: s, tree: { sha: `tree-${s}` } }) @@ -501,9 +531,11 @@ export function createFakeGitHubCore(): FakeGitHubCore { } : null }) - const edges = pr.commits.map(c => ({ + const edges = pr.commits.map((c, index) => ({ node: { commit: { + oid: c.oid ?? (index + 1).toString(16).padStart(40, 'c'), + parents: { totalCount: c.parentCount ?? 1 }, message: c.message || '', author: actor(c.author), committer: actor(c.committer || c.author), @@ -631,6 +663,12 @@ export function createFakeGitHubCore(): FakeGitHubCore { l => l.owner === owner && l.repo === name && l.issue === issueNumber ) }, + addCommitStatus(sha_, status) { + const list = repo.statuses.get(sha_) || [] + list.unshift({ ...status }) + repo.statuses.set(sha_, list) + return this + }, addWorkflow(name_, runs = []) { const wf: Workflow = { id: 10000 + repo.workflows.length, diff --git a/action.yml b/action.yml index 9f2f8139..fe43ca84 100644 --- a/action.yml +++ b/action.yml @@ -58,6 +58,20 @@ inputs: allowlist-ids: description: "Comma-separated numeric GitHub user IDs that never need to sign, as the Pull Request opener or as a commit author. Unlinked identities cannot match." default: "" + trusted-merge-status-context: + description: > + Optional commit status context that vouches for an automated merge of + the base branch into a Pull Request. A merge commit (two or more + parents) needs no signature from its primary author when this + repository's newest status with this context is a success created by an + account in trusted-merge-status-creator-ids. Set both inputs or neither. + default: "" + trusted-merge-status-creator-ids: + description: > + Comma-separated numeric GitHub account IDs whose trusted merge status + counts, for example 41898282 for github-actions[bot] when a workflow in + this repository posts the status with its GITHUB_TOKEN. + default: "" remote-repository-name: description: "provide the remote repository name where all the signatures should be stored" remote-organization-name: diff --git a/dist/index.js b/dist/index.js index 1fcd1807..1512b14d 100644 --- a/dist/index.js +++ b/dist/index.js @@ -48131,6 +48131,13 @@ const getExpectedSigningComment = () => { const getRequiredBaseRef = () => getInput('required-base-ref', { required: false }) || 'main'; const getAllowListItem = () => getInput('allowlist', { required: false }); const getAllowListIds = () => getInput('allowlist-ids', { required: false }); +/** + * Commit status context that vouches for an automated merge of the base + * branch into a Pull Request (see trustedMergeStatus.ts). Empty disables it. + */ +const getTrustedMergeStatusContext = () => getInput('trusted-merge-status-context', { required: false }).trim(); +/** Numeric account IDs whose trusted merge status counts. */ +const getTrustedMergeStatusCreatorIds = () => getInput('trusted-merge-status-creator-ids', { required: false }).trim(); const getSignedCommitMessage = () => getInput('signed-commit-message', { required: false }); const getCreateFileCommitMessage = () => getInput('create-file-commit-message', { required: false }); const getCustomNotSignedPrComment = () => getInput('custom-notsigned-prcomment', { required: false }); @@ -48322,12 +48329,88 @@ const MAX_LEDGER_WRITE_ATTEMPTS = 3; // A first-file create can lose a race with another Pull Request. Retry only // the safe read that confirms the other run created a valid ledger. const MAX_LEDGER_CREATE_RECOVERY_ATTEMPTS = 3; +// Commit status lookups for merge commits that may be vouched for by a +// trusted merge status. Merge commits past this bound are treated as +// unvouched and their authors must sign as usual. +const MAX_TRUSTED_MERGE_STATUS_LOOKUPS = 20; + +;// CONCATENATED MODULE: ./src/trustedMergeStatus.ts + + + + +const COMMIT_SHA = /^[0-9a-f]{40}$/; +/** + * Reads the optional trusted merge vouching configuration. Both inputs are + * required together; a partial or malformed configuration fails the run + * instead of silently widening or narrowing who must sign. + */ +function getTrustedMergeStatus() { + const statusContext = getTrustedMergeStatusContext(); + const rawIds = getTrustedMergeStatusCreatorIds(); + if (!statusContext && !rawIds) + return undefined; + if (!statusContext || !rawIds) { + throw new Error('trusted-merge-status-context and trusted-merge-status-creator-ids must be provided together'); + } + if (/[\r\n]/.test(statusContext) || statusContext.length > 255) { + throw new Error('trusted-merge-status-context is malformed'); + } + const creatorIds = new Set(); + for (const raw of rawIds.split(',')) { + const entry = raw.trim(); + const id = Number(entry); + if (!/^[1-9][0-9]*$/.test(entry) || !Number.isSafeInteger(id)) { + throw new Error('trusted-merge-status-creator-ids must be comma-separated numeric GitHub account IDs'); + } + creatorIds.add(id); + } + return { + context: statusContext, + creatorIds, + remainingLookups: MAX_TRUSTED_MERGE_STATUS_LOOKUPS + }; +} +/** + * A merge commit is exempt from signing when this repository's newest commit + * status with the configured context is a success created by a configured + * account. Commit statuses are stored per repository and only a token with + * statuses write access to this repository can create one, so a fork cannot + * vouch for its own commits, and git author, committer, and signature + * metadata play no part. The status names one exact commit, so it cannot be + * moved to a different tree. Single-parent commits are never exempt. + */ +async function isTrustedMergeCommit(commit, trust) { + if (!commit.parents || commit.parents.totalCount < 2) + return false; + if (typeof commit.oid !== 'string' || !COMMIT_SHA.test(commit.oid)) { + return false; + } + if (trust.remainingLookups <= 0) + return false; + trust.remainingLookups -= 1; + // Newest first. Only the newest status for the context counts, so a later + // failure or a later status from another account revokes the exemption. + const response = await octokit.rest.repos.listCommitStatusesForRef({ + owner: github_context.repo.owner, + repo: github_context.repo.repo, + ref: commit.oid, + per_page: 100 + }); + const newest = response.data.find(status => status.context === trust.context); + const creatorId = newest?.creator?.id; + return Boolean(newest && + newest.state === 'success' && + typeof creatorId === 'number' && + trust.creatorIds.has(creatorId)); +} ;// CONCATENATED MODULE: ./src/graphql.ts + // Bound work on untrusted Pull Request data. These limits are well above a // normal contribution but stop a single PR from consuming an unbounded number // of GraphQL pages or creating an unbounded signature set. @@ -48341,6 +48424,8 @@ query($owner:String! $name:String! $number:Int! $cursor:String){ edges { node { commit { + oid + parents { totalCount } author { email name @@ -48370,7 +48455,9 @@ query($owner:String! $name:String! $number:Int! $cursor:String){ * collected so the opener authorship guard can accept a Co-authored-by * trailer. The git committer field is ignored: it names whoever applied the * commit (a maintainer, GitHub's web-flow merge, a rebase tool), not a - * copyright holder, so it never creates a signing obligation. + * copyright holder, so it never creates a signing obligation. A merge commit + * vouched for by a trusted merge status (trustedMergeStatus.ts) contributes + * no identities at all. */ async function getCommitters(expectedHeadSha) { try { @@ -48378,6 +48465,7 @@ async function getCommitters(expectedHeadSha) { throw new Error('The live Pull Request head commit is missing; refusing to query commit identities'); } const committers = new Map(); + const trustedMergeStatus = getTrustedMergeStatus(); const addActor = (actor, role) => { const roles = { isPrimaryAuthor: role === 'primaryAuthor', @@ -48453,6 +48541,10 @@ async function getCommitters(expectedHeadSha) { if (identityAssertionCount > MAX_GIT_IDENTITY_ASSERTIONS) { throw new Error(`A Pull Request reports more than ${MAX_GIT_IDENTITY_ASSERTIONS} git identity assertions. The action will fail closed.`); } + if (trustedMergeStatus && + (await isTrustedMergeCommit(commit, trustedMergeStatus))) { + continue; + } addActor(commit.author, 'primaryAuthor'); commit.authors.nodes .slice(1) diff --git a/src/graphql.ts b/src/graphql.ts index a4a6815f..ca77c199 100644 --- a/src/graphql.ts +++ b/src/graphql.ts @@ -2,6 +2,10 @@ import { context } from '@actions/github' import { Committer } from './interfaces' import { octokit } from './octokit' import { errorMessage } from './shared/errors' +import { + getTrustedMergeStatus, + isTrustedMergeCommit +} from './trustedMergeStatus' import { MAX_AUTHORS_PER_COMMIT, MAX_GIT_IDENTITY_ASSERTIONS, @@ -25,6 +29,8 @@ interface GraphQLAuthorsConnection { } interface GraphQLCommit { + oid?: string | null + parents?: { totalCount: number } | null author?: GraphQLActor | null authors: GraphQLAuthorsConnection } @@ -62,6 +68,8 @@ query($owner:String! $name:String! $number:Int! $cursor:String){ edges { node { commit { + oid + parents { totalCount } author { email name @@ -92,7 +100,9 @@ query($owner:String! $name:String! $number:Int! $cursor:String){ * collected so the opener authorship guard can accept a Co-authored-by * trailer. The git committer field is ignored: it names whoever applied the * commit (a maintainer, GitHub's web-flow merge, a rebase tool), not a - * copyright holder, so it never creates a signing obligation. + * copyright holder, so it never creates a signing obligation. A merge commit + * vouched for by a trusted merge status (trustedMergeStatus.ts) contributes + * no identities at all. */ export default async function getCommitters( expectedHeadSha: string @@ -104,6 +114,7 @@ export default async function getCommitters( ) } const committers = new Map() + const trustedMergeStatus = getTrustedMergeStatus() const addActor = ( actor: GraphQLActor | null | undefined, @@ -210,6 +221,13 @@ export default async function getCommitters( ) } + if ( + trustedMergeStatus && + (await isTrustedMergeCommit(commit, trustedMergeStatus)) + ) { + continue + } + addActor(commit.author, 'primaryAuthor') commit.authors.nodes .slice(1) diff --git a/src/shared/getInputs.ts b/src/shared/getInputs.ts index 29507f4d..dd17ce21 100644 --- a/src/shared/getInputs.ts +++ b/src/shared/getInputs.ts @@ -89,6 +89,17 @@ export const getAllowListItem = (): string => export const getAllowListIds = (): string => core.getInput('allowlist-ids', { required: false }) +/** + * Commit status context that vouches for an automated merge of the base + * branch into a Pull Request (see trustedMergeStatus.ts). Empty disables it. + */ +export const getTrustedMergeStatusContext = (): string => + core.getInput('trusted-merge-status-context', { required: false }).trim() + +/** Numeric account IDs whose trusted merge status counts. */ +export const getTrustedMergeStatusCreatorIds = (): string => + core.getInput('trusted-merge-status-creator-ids', { required: false }).trim() + export const getSignedCommitMessage = (): string => core.getInput('signed-commit-message', { required: false }) diff --git a/src/shared/limits.ts b/src/shared/limits.ts index 74ec3819..0cee33fd 100644 --- a/src/shared/limits.ts +++ b/src/shared/limits.ts @@ -24,3 +24,7 @@ export const MAX_LEDGER_WRITE_ATTEMPTS = 3 // A first-file create can lose a race with another Pull Request. Retry only // the safe read that confirms the other run created a valid ledger. export const MAX_LEDGER_CREATE_RECOVERY_ATTEMPTS = 3 +// Commit status lookups for merge commits that may be vouched for by a +// trusted merge status. Merge commits past this bound are treated as +// unvouched and their authors must sign as usual. +export const MAX_TRUSTED_MERGE_STATUS_LOOKUPS = 20 diff --git a/src/trustedMergeStatus.ts b/src/trustedMergeStatus.ts new file mode 100644 index 00000000..72018185 --- /dev/null +++ b/src/trustedMergeStatus.ts @@ -0,0 +1,93 @@ +import { context } from '@actions/github' +import { octokit } from './octokit' +import { + getTrustedMergeStatusContext, + getTrustedMergeStatusCreatorIds +} from './shared/getInputs' +import { MAX_TRUSTED_MERGE_STATUS_LOOKUPS } from './shared/limits' + +interface TrustedMergeStatus { + context: string + creatorIds: Set + remainingLookups: number +} + +interface CommitShape { + oid?: string | null + parents?: { totalCount: number } | null +} + +const COMMIT_SHA = /^[0-9a-f]{40}$/ + +/** + * Reads the optional trusted merge vouching configuration. Both inputs are + * required together; a partial or malformed configuration fails the run + * instead of silently widening or narrowing who must sign. + */ +export function getTrustedMergeStatus(): TrustedMergeStatus | undefined { + const statusContext = getTrustedMergeStatusContext() + const rawIds = getTrustedMergeStatusCreatorIds() + if (!statusContext && !rawIds) return undefined + if (!statusContext || !rawIds) { + throw new Error( + 'trusted-merge-status-context and trusted-merge-status-creator-ids must be provided together' + ) + } + if (/[\r\n]/.test(statusContext) || statusContext.length > 255) { + throw new Error('trusted-merge-status-context is malformed') + } + const creatorIds = new Set() + for (const raw of rawIds.split(',')) { + const entry = raw.trim() + const id = Number(entry) + if (!/^[1-9][0-9]*$/.test(entry) || !Number.isSafeInteger(id)) { + throw new Error( + 'trusted-merge-status-creator-ids must be comma-separated numeric GitHub account IDs' + ) + } + creatorIds.add(id) + } + return { + context: statusContext, + creatorIds, + remainingLookups: MAX_TRUSTED_MERGE_STATUS_LOOKUPS + } +} + +/** + * A merge commit is exempt from signing when this repository's newest commit + * status with the configured context is a success created by a configured + * account. Commit statuses are stored per repository and only a token with + * statuses write access to this repository can create one, so a fork cannot + * vouch for its own commits, and git author, committer, and signature + * metadata play no part. The status names one exact commit, so it cannot be + * moved to a different tree. Single-parent commits are never exempt. + */ +export async function isTrustedMergeCommit( + commit: CommitShape, + trust: TrustedMergeStatus +): Promise { + if (!commit.parents || commit.parents.totalCount < 2) return false + if (typeof commit.oid !== 'string' || !COMMIT_SHA.test(commit.oid)) { + return false + } + if (trust.remainingLookups <= 0) return false + trust.remainingLookups -= 1 + + // Newest first. Only the newest status for the context counts, so a later + // failure or a later status from another account revokes the exemption. + const response = await octokit.rest.repos.listCommitStatusesForRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: commit.oid, + per_page: 100 + }) + const newest = response.data.find(status => status.context === trust.context) + const creatorId = newest?.creator?.id + return Boolean( + newest && + newest.state === 'success' && + typeof creatorId === 'number' && + trust.creatorIds.has(creatorId) + ) +} From 3bdedfb05157fd9c1c879dfea58455e32a770f96 Mon Sep 17 00:00:00 2001 From: Leo Date: Sun, 27 Sep 2026 02:26:32 -0400 Subject: [PATCH 2/2] fix: page through merge commit statuses and document the permissions The newest status for the trusted context can sit behind 100 newer statuses from other contexts. Search up to 5 pages, newest first, and treat a context not found by then as unvouched. The README now says to grant statuses: read when the inputs are set, and that the creator ID must belong to an account whose status-write credential only trusted code holds (github-actions[bot] only when no workflow hands Pull Request code a GITHUB_TOKEN with statuses: write). Adds doc comments to the functions this change touches. Co-Authored-By: Claude Opus 5.5 --- README.md | 10 ++-- .../integration/trustedMergeStatus.test.ts | 38 +++++++++++++++ __tests__/testHelpers/fakeGithubCore.ts | 2 + dist/index.js | 47 +++++++++++++++---- src/graphql.ts | 10 ++++ src/shared/limits.ts | 3 ++ src/trustedMergeStatus.ts | 43 +++++++++++++---- 7 files changed, 130 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index eee70e1f..7e7b4d4d 100644 --- a/README.md +++ b/README.md @@ -89,9 +89,9 @@ jobs: contents: write # this can be read if signatures are in a remote repository issues: write pull-requests: write - # No statuses permission is needed. The action fails or succeeds this - # GitHub Actions job through @actions/core and never calls the commit - # status or check-run APIs. + # The action fails or succeeds this job through @actions/core and never + # writes commit statuses or check runs. Add `statuses: read` only when + # trusted-merge-status-context is set; it reads merge commit statuses. # Advisory signer only. A separate trusted exact-head worker is required # when this check is required by branch protection. Serialize signer runs # for one Pull Request. A separate lock job uses the @@ -266,7 +266,9 @@ Use `allowlist-ids` for maintainers and documented automation accounts that neve ##### Trusted merge status -A workflow that merges the base branch into open Pull Requests (a "catch-up" bot) authors merge commits as its own bot account, which cannot sign. To exempt those merges without trusting any git metadata, have that workflow post a commit status on each merge commit it verified, then set `trusted-merge-status-context` to the status context and `trusted-merge-status-creator-ids` to the numeric ID of the account that posts it (41898282 for `github-actions[bot]` using the workflow's `GITHUB_TOKEN`). A commit is exempt only when it has two or more parents and this repository's newest status with that context is a success created by a configured account. Statuses are stored per repository and need statuses write access to create, so a fork cannot vouch for its own commits. Post the status before the merge reaches the Pull Request branch, for example by pushing the commit to a scratch ref first, so the check that the push starts already sees it. +A workflow that merges the base branch into open Pull Requests (a "catch-up" bot) authors merge commits as its own bot account, which cannot sign. To exempt those merges without trusting any git metadata, have that workflow post a commit status on each merge commit it verified, then set `trusted-merge-status-context` to the status context and `trusted-merge-status-creator-ids` to the numeric ID of the account that posts it (41898282 for `github-actions[bot]` using the workflow's `GITHUB_TOKEN`). A commit is exempt only when it has two or more parents and this repository's newest status with that context is a success created by a configured account. Statuses are stored per repository and need statuses write access to create, so a fork cannot vouch for its own commits. Post the status before the merge reaches the Pull Request branch, for example by pushing the commit to a scratch ref first, so the check that the push starts already sees it. Grant `statuses: read` to the signer job (and to a `signer-preflight` gate) when these inputs are set: an explicit permissions list leaves it at `none`, and the status request then fails the run. + +The creator ID is the whole trust anchor, so configure only an account whose status-write credential is held by trusted code. `github-actions[bot]` qualifies only if no workflow in the repository gives Pull Request-controlled code a `GITHUB_TOKEN` with `statuses: write` (for example a `pull_request_target` or `workflow_run` job that checks out and runs the head). If one does, use a dedicated GitHub App whose key only the merge workflow can read. Statuses are paged newest first; the action reads up to 5 pages of 100, and a context not found in them counts as unvouched. ##### Demo for step 5 diff --git a/__tests__/integration/trustedMergeStatus.test.ts b/__tests__/integration/trustedMergeStatus.test.ts index d1486a71..1337ad64 100644 --- a/__tests__/integration/trustedMergeStatus.test.ts +++ b/__tests__/integration/trustedMergeStatus.test.ts @@ -8,6 +8,7 @@ const MERGE_SHA = 'a'.repeat(40) const ACTIONS_BOT = { login: 'github-actions[bot]', id: 41898282 } const CATCH_UP = 'cmux/catch-up' +/** Runs the action's main entry with a fresh module cache and Octokit. */ async function runAction() { reloadOctokit() for (const path of Object.keys(require.cache)) { @@ -17,6 +18,7 @@ async function runAction() { await run() } +/** Spies on setFailed, info, and setOutput; restore() undoes the spies. */ function watchCore() { const failed = jest.spyOn(core, 'setFailed').mockImplementation(() => {}) const info = jest.spyOn(core, 'info').mockImplementation(() => {}) @@ -44,6 +46,11 @@ describe('trusted merge status', () => { resetEnv() }) + /** + * Builds PR 12 in acme/widgets: a signed commit by alice and a merge commit + * by github-actions[bot], with the given merge statuses (oldest first) in + * this repository and optionally in a fork, then sets the event context. + */ function setUp(options: { inputs?: Record parentCount?: number @@ -213,4 +220,35 @@ describe('trusted merge status', () => { ) watch.restore() }) + + /** `count` statuses for an unrelated context, all newer than `success`. */ + function otherStatuses(count: number): CommitStatusFixture[] { + return Array.from({ length: count }, () => ({ + context: 'ci/other', + state: 'success' as const, + creator: ACTIONS_BOT + })) + } + + it('finds the trusted status past the first page', async () => { + setUp({ statuses: [success, ...otherStatuses(150)] }) + const watch = watchCore() + await runAction() + expect(watch.failures).toEqual([]) + expect(watch.outputs).toContainEqual(['cla_passed', true]) + watch.restore() + }) + + it('treats a context beyond the page bound as unvouched', async () => { + setUp({ statuses: [success, ...otherStatuses(500)] }) + const watch = watchCore() + await runAction() + expect(watch.failures.join('\n')).toMatch( + /Committers of Pull Request number 12/ + ) + expect( + fake.requestLog.filter(r => r.path.includes('/statuses')).length + ).toBe(5) + watch.restore() + }) }) diff --git a/__tests__/testHelpers/fakeGithubCore.ts b/__tests__/testHelpers/fakeGithubCore.ts index 62eb3edc..262e6313 100644 --- a/__tests__/testHelpers/fakeGithubCore.ts +++ b/__tests__/testHelpers/fakeGithubCore.ts @@ -343,6 +343,7 @@ export function createFakeGitHubCore(): FakeGitHubCore { type: 'Bot' }) }) + // Commit statuses for one SHA in one repository, newest first, paginated. addRoute( getRoutes, '/repos/:owner/:repo/commits/:sha/statuses', @@ -663,6 +664,7 @@ export function createFakeGitHubCore(): FakeGitHubCore { l => l.owner === owner && l.repo === name && l.issue === issueNumber ) }, + /** Prepends a status so the list stays newest first, like the API. */ addCommitStatus(sha_, status) { const list = repo.statuses.get(sha_) || [] list.unshift({ ...status }) diff --git a/dist/index.js b/dist/index.js index 1512b14d..00f76e99 100644 --- a/dist/index.js +++ b/dist/index.js @@ -48333,6 +48333,9 @@ const MAX_LEDGER_CREATE_RECOVERY_ATTEMPTS = 3; // trusted merge status. Merge commits past this bound are treated as // unvouched and their authors must sign as usual. const MAX_TRUSTED_MERGE_STATUS_LOOKUPS = 20; +// Status pages (100 each, newest first) searched for the trusted context on +// one merge commit. A context not found within them counts as unvouched. +const MAX_TRUSTED_MERGE_STATUS_PAGES = 5; ;// CONCATENATED MODULE: ./src/trustedMergeStatus.ts @@ -48389,21 +48392,37 @@ async function isTrustedMergeCommit(commit, trust) { if (trust.remainingLookups <= 0) return false; trust.remainingLookups -= 1; - // Newest first. Only the newest status for the context counts, so a later - // failure or a later status from another account revokes the exemption. - const response = await octokit.rest.repos.listCommitStatusesForRef({ - owner: github_context.repo.owner, - repo: github_context.repo.repo, - ref: commit.oid, - per_page: 100 - }); - const newest = response.data.find(status => status.context === trust.context); + const newest = await findNewestStatus(commit.oid, trust.context); const creatorId = newest?.creator?.id; return Boolean(newest && newest.state === 'success' && typeof creatorId === 'number' && trust.creatorIds.has(creatorId)); } +/** + * Returns this repository's newest status for `statusContext` on `sha`, or + * undefined. GitHub lists statuses newest first, 100 per page, so the first + * match is the newest. Only the newest status counts: a later failure or a + * later status from another account revokes the exemption. Paging stops at + * MAX_TRUSTED_MERGE_STATUS_PAGES; a context not found by then is unvouched. + */ +async function findNewestStatus(sha, statusContext) { + for (let page = 1; page <= MAX_TRUSTED_MERGE_STATUS_PAGES; page++) { + const response = await octokit.rest.repos.listCommitStatusesForRef({ + owner: github_context.repo.owner, + repo: github_context.repo.repo, + ref: sha, + per_page: 100, + page + }); + const match = response.data.find(status => status.context === statusContext); + if (match) + return match; + if (response.data.length < 100) + return undefined; + } + return undefined; +} ;// CONCATENATED MODULE: ./src/graphql.ts @@ -48466,6 +48485,7 @@ async function getCommitters(expectedHeadSha) { } const committers = new Map(); const trustedMergeStatus = getTrustedMergeStatus(); + /** Records one GitHub actor under the given role, merging duplicates. */ const addActor = (actor, role) => { const roles = { isPrimaryAuthor: role === 'primaryAuthor', @@ -48571,6 +48591,10 @@ async function getCommitters(expectedHeadSha) { throw new Error(`GraphQL call to get commit identities failed: ${errorMessage(e)}`); } } +/** + * Adds an identity to the map, or merges its roles and email into the + * existing entry for the same account ID, email, or name. + */ function addCommitter(committers, incoming) { const key = identityKey(incoming); const current = committers.get(key); @@ -48583,6 +48607,7 @@ function addCommitter(committers, incoming) { if (!current.email && incoming.email) current.email = incoming.email; } +/** Map key for an identity: account ID, else email, else lowercased name. */ function identityKey(committer) { if (committer.id > 0) return `id:${committer.id}`; @@ -48590,6 +48615,10 @@ function identityKey(committer) { return `email:${committer.email.toLowerCase()}`; return `unknown:${committer.name.toLowerCase()}`; } +/** + * Whether two GraphQL actors are the same identity: by account ID when both + * have one, otherwise by case-insensitive email. + */ function actorsMatch(left, right) { if (!right) return false; diff --git a/src/graphql.ts b/src/graphql.ts index ca77c199..e95586c6 100644 --- a/src/graphql.ts +++ b/src/graphql.ts @@ -116,6 +116,7 @@ export default async function getCommitters( const committers = new Map() const trustedMergeStatus = getTrustedMergeStatus() + /** Records one GitHub actor under the given role, merging duplicates. */ const addActor = ( actor: GraphQLActor | null | undefined, role: CommitIdentityRole @@ -265,6 +266,10 @@ export default async function getCommitters( } } +/** + * Adds an identity to the map, or merges its roles and email into the + * existing entry for the same account ID, email, or name. + */ function addCommitter( committers: Map, incoming: Committer @@ -283,12 +288,17 @@ function addCommitter( if (!current.email && incoming.email) current.email = incoming.email } +/** Map key for an identity: account ID, else email, else lowercased name. */ function identityKey(committer: Committer): string { if (committer.id > 0) return `id:${committer.id}` if (committer.email) return `email:${committer.email.toLowerCase()}` return `unknown:${committer.name.toLowerCase()}` } +/** + * Whether two GraphQL actors are the same identity: by account ID when both + * have one, otherwise by case-insensitive email. + */ function actorsMatch( left: GraphQLActor, right: GraphQLActor | null | undefined diff --git a/src/shared/limits.ts b/src/shared/limits.ts index 0cee33fd..ef1e4673 100644 --- a/src/shared/limits.ts +++ b/src/shared/limits.ts @@ -28,3 +28,6 @@ export const MAX_LEDGER_CREATE_RECOVERY_ATTEMPTS = 3 // trusted merge status. Merge commits past this bound are treated as // unvouched and their authors must sign as usual. export const MAX_TRUSTED_MERGE_STATUS_LOOKUPS = 20 +// Status pages (100 each, newest first) searched for the trusted context on +// one merge commit. A context not found within them counts as unvouched. +export const MAX_TRUSTED_MERGE_STATUS_PAGES = 5 diff --git a/src/trustedMergeStatus.ts b/src/trustedMergeStatus.ts index 72018185..4a159b1c 100644 --- a/src/trustedMergeStatus.ts +++ b/src/trustedMergeStatus.ts @@ -4,14 +4,19 @@ import { getTrustedMergeStatusContext, getTrustedMergeStatusCreatorIds } from './shared/getInputs' -import { MAX_TRUSTED_MERGE_STATUS_LOOKUPS } from './shared/limits' +import { + MAX_TRUSTED_MERGE_STATUS_LOOKUPS, + MAX_TRUSTED_MERGE_STATUS_PAGES +} from './shared/limits' +/** Parsed trusted merge status configuration plus the remaining lookup budget. */ interface TrustedMergeStatus { context: string creatorIds: Set remainingLookups: number } +/** The GraphQL commit fields the exemption needs. */ interface CommitShape { oid?: string | null parents?: { totalCount: number } | null @@ -74,15 +79,7 @@ export async function isTrustedMergeCommit( if (trust.remainingLookups <= 0) return false trust.remainingLookups -= 1 - // Newest first. Only the newest status for the context counts, so a later - // failure or a later status from another account revokes the exemption. - const response = await octokit.rest.repos.listCommitStatusesForRef({ - owner: context.repo.owner, - repo: context.repo.repo, - ref: commit.oid, - per_page: 100 - }) - const newest = response.data.find(status => status.context === trust.context) + const newest = await findNewestStatus(commit.oid, trust.context) const creatorId = newest?.creator?.id return Boolean( newest && @@ -91,3 +88,29 @@ export async function isTrustedMergeCommit( trust.creatorIds.has(creatorId) ) } + +/** + * Returns this repository's newest status for `statusContext` on `sha`, or + * undefined. GitHub lists statuses newest first, 100 per page, so the first + * match is the newest. Only the newest status counts: a later failure or a + * later status from another account revokes the exemption. Paging stops at + * MAX_TRUSTED_MERGE_STATUS_PAGES; a context not found by then is unvouched. + */ +async function findNewestStatus( + sha: string, + statusContext: string +): Promise<{ state: string; creator?: { id: number } | null } | undefined> { + for (let page = 1; page <= MAX_TRUSTED_MERGE_STATUS_PAGES; page++) { + const response = await octokit.rest.repos.listCommitStatusesForRef({ + owner: context.repo.owner, + repo: context.repo.repo, + ref: sha, + per_page: 100, + page + }) + const match = response.data.find(status => status.context === statusContext) + if (match) return match + if (response.data.length < 100) return undefined + } + return undefined +}