diff --git a/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.mjs b/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.mjs index 3a819bb..b4be55d 100644 --- a/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.mjs +++ b/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.mjs @@ -8,6 +8,15 @@ const DEFAULT_ALLOWED_SIGNERS_RELATIVE_PATH = '../../signing/allowed_signers'; const FULL_SHA_PATTERN = /^[0-9a-fA-F]{40}$/; const INTEGER_PATTERN = /^[0-9]+$/; const LINE_SPLIT_PATTERN = /\r?\n/; +// A merge commit in the PR branch (e.g. from merging the base branch in) +// can put the true merge-base much further back on the merged-in side than +// the initial shallow fetch's depth budget reaches, since that budget is +// sized for a linear history of prCommitCount commits. When that happens, +// rev-list can't prove shared ancestry across the shallow boundary and +// overcounts. Retry with a deepened fetch a few times before paying for a +// full unshallow fetch, so the common (linear-history) case stays cheap. +const MAX_DEEPEN_ATTEMPTS = 4; +const DEEPEN_MULTIPLIER = 4; const enforce = process.env.SIGNED_COMMIT_ENFORCE !== 'false'; @@ -63,6 +72,7 @@ function main() { expectedCommitCount: prCommitCount, prBaseSha, prHeadSha, + prNumber, workspace, }); return; @@ -140,18 +150,22 @@ function ensureGitRepository(workspace) { } } -function fetchFromOrigin(ref, workspace) { - const token = process.env.GITHUB_TOKEN; - if (!token) { - return; - } +function authConfigArgs(token) { const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString( 'base64', ); - const configArgs = [ + return [ '-c', `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`, ]; +} + +function fetchFromOrigin(ref, workspace) { + const token = process.env.GITHUB_TOKEN; + if (!token) { + return; + } + const configArgs = authConfigArgs(token); if (FULL_SHA_PATTERN.test(ref)) { git([...configArgs, 'fetch', '--no-tags', 'origin', ref], workspace); @@ -184,18 +198,8 @@ function unshallowFromOrigin(workspace) { if (!token) { return; } - const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString( - 'base64', - ); git( - [ - '-c', - `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`, - 'fetch', - '--unshallow', - '--no-tags', - 'origin', - ], + [...authConfigArgs(token), 'fetch', '--unshallow', '--no-tags', 'origin'], workspace, ); } @@ -239,21 +243,11 @@ function fetchPullRequestCommits({ workspace, }) { const token = requiredEnv('GITHUB_TOKEN'); - const authHeader = Buffer.from(`x-access-token:${token}`, 'utf8').toString( - 'base64', - ); ensureGitRepository(workspace); const fetchBase = git( - [ - '-c', - `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`, - 'fetch', - '--no-tags', - 'origin', - prBaseSha, - ], + [...authConfigArgs(token), 'fetch', '--no-tags', 'origin', prBaseSha], workspace, ); if (!fetchBase.ok) { @@ -262,8 +256,7 @@ function fetchPullRequestCommits({ const fetch = git( [ - '-c', - `http.https://github.com/.extraheader=AUTHORIZATION: basic ${authHeader}`, + ...authConfigArgs(token), 'fetch', '--no-tags', `--depth=${prCommitCount + 1}`, @@ -290,32 +283,92 @@ function verifyPullRequestCommits({ expectedCommitCount, prBaseSha, prHeadSha, + prNumber, workspace, }) { + const commits = resolvePullRequestCommits({ + expectedCommitCount, + prBaseSha, + prHeadSha, + prNumber, + workspace, + }); + + verifyCommits(commits, allowedSignersPath, workspace); + + workflowCommand( + 'notice', + `All ${commits.length} PR commit(s) are signed by allowed SSH keys.`, + ); +} + +function listCommitsInRange(baseSha, headSha, workspace) { const revList = git( - ['rev-list', '--reverse', `${prBaseSha}..${prHeadSha}`], + ['rev-list', '--reverse', `${baseSha}..${headSha}`], workspace, ); if (!revList.ok) { fail(`Could not list PR commits locally: ${commandDetails(revList)}`); } + return revList.stdout.trim().split(LINE_SPLIT_PATTERN).filter(Boolean); +} + +function deepenPullRequestFetch({deepenBy, prNumber, workspace}) { + const token = requiredEnv('GITHUB_TOKEN'); + const deepen = git( + [ + ...authConfigArgs(token), + 'fetch', + '--no-tags', + `--deepen=${deepenBy}`, + 'origin', + `+refs/pull/${prNumber}/head:refs/remotes/pull/${prNumber}/head`, + ], + workspace, + ); + if (!deepen.ok) { + fail(`Could not deepen PR history: ${commandDetails(deepen)}`); + } +} + +function resolvePullRequestCommits({ + expectedCommitCount, + prBaseSha, + prHeadSha, + prNumber, + workspace, +}) { + let commits = listCommitsInRange(prBaseSha, prHeadSha, workspace); + + let deepenBy = Math.max(expectedCommitCount, 1) * DEEPEN_MULTIPLIER; + for ( + let attempt = 0; + commits.length !== expectedCommitCount && attempt < MAX_DEEPEN_ATTEMPTS; + attempt++ + ) { + deepenPullRequestFetch({deepenBy, prNumber, workspace}); + commits = listCommitsInRange(prBaseSha, prHeadSha, workspace); + deepenBy *= DEEPEN_MULTIPLIER; + } + + // Guaranteed correct (no shallow boundary left to hide shared ancestry), + // but expensive, so it's only a last resort after deepening fails to + // converge. + if ( + commits.length !== expectedCommitCount && + isShallowRepository(workspace) + ) { + unshallowFromOrigin(workspace); + commits = listCommitsInRange(prBaseSha, prHeadSha, workspace); + } - const commits = revList.stdout - .trim() - .split(LINE_SPLIT_PATTERN) - .filter(Boolean); if (commits.length !== expectedCommitCount) { fail( `Expected ${expectedCommitCount} PR commit(s) from the pull_request payload, but git rev-list found ${commits.length}.`, ); } - verifyCommits(commits, allowedSignersPath, workspace); - - workflowCommand( - 'notice', - `All ${commits.length} PR commit(s) are signed by allowed SSH keys.`, - ); + return commits; } function verifyCommitRange({ diff --git a/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.test.mjs b/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.test.mjs index c29b0ff..1dbee83 100644 --- a/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.test.mjs +++ b/.github/actions/verify-signed-commit-authors/verify-signed-commit-authors.test.mjs @@ -18,6 +18,7 @@ const scriptPath = fileURLToPath( const baseSha = '1111111111111111111111111111111111111111'; const firstCommit = '2222222222222222222222222222222222222222'; const secondCommit = '3333333333333333333333333333333333333333'; +const phantomCommit = '4444444444444444444444444444444444444444'; const headSha = secondCommit; const activeAllowedSigners = 'alice@example.com namespaces="git" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAITestKey\n'; @@ -73,6 +74,78 @@ test('accepts pull request commits signed by allowed SSH keys', () => { ); }); +test('deepens the shallow fetch and retries when a merge commit undercounts the initial depth', () => { + const result = runVerifier({ + env: { + FAKE_GIT_REQUIRE_DEEPENS: '1', + }, + }); + + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.match( + result.stdout, + /::notice::All 2 PR commit\(s\) are signed by allowed SSH keys\./, + ); + + const calls = readToolCalls(result.dir); + const deepenCalls = calls.filter( + call => + call.command === 'git' && + call.args.some(arg => arg.startsWith('--deepen=')), + ); + assert.equal( + deepenCalls.length, + 1, + 'expected exactly one deepen fetch before the count converged', + ); +}); + +test('falls back to a full unshallow fetch when deepening does not converge, and still succeeds if that fixes it', () => { + const result = runVerifier({ + env: { + FAKE_GIT_REQUIRE_DEEPENS: '100', + FAKE_GIT_SHALLOW: '1', + }, + }); + + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.match( + result.stdout, + /::notice::All 2 PR commit\(s\) are signed by allowed SSH keys\./, + ); + + const calls = readToolCalls(result.dir); + const deepenCalls = calls.filter( + call => + call.command === 'git' && + call.args.some(arg => arg.startsWith('--deepen=')), + ); + assert.equal( + deepenCalls.length, + 4, + 'expected all deepen attempts to be exhausted before falling back', + ); + const unshallowCall = calls.find( + call => call.command === 'git' && call.args.includes('--unshallow'), + ); + assert.ok(unshallowCall, 'expected git fetch --unshallow as a last resort'); +}); + +test('fails closed when the commit count still does not match after deepening and unshallowing', () => { + const result = runVerifier({ + env: { + FAKE_GIT_ALWAYS_MISMATCH: '1', + FAKE_GIT_SHALLOW: '1', + }, + }); + + assert.equal(result.status, 1); + assert.match( + result.stdout, + /::error::Expected 2 PR commit\(s\) from the pull_request payload, but git rev-list found 3\./, + ); +}); + test('rejects a commit whose signature is not made by an allowed key', () => { const result = runVerifier({ env: { @@ -432,7 +505,7 @@ function writeToolStubs(binDir, dir) { writeExecutable( join(binDir, 'git'), `#!/usr/bin/env node -import {appendFileSync, existsSync} from 'node:fs'; +import {appendFileSync, existsSync, readFileSync} from 'node:fs'; const allArgs = process.argv.slice(2); appendFileSync( @@ -459,6 +532,9 @@ if (command === 'fetch') { } appendFileSync(${JSON.stringify(join(dir, 'unshallow-called'))}, '1'); } + if (args.some(arg => arg.startsWith('--deepen='))) { + appendFileSync(${JSON.stringify(join(dir, 'deepen-called'))}, '1'); + } process.exit(0); } @@ -507,7 +583,25 @@ if (command === 'rev-list') { if (process.env.FAKE_GIT_EMPTY_REV_LIST) { process.exit(0); } - process.stdout.write(${JSON.stringify(`${firstCommit}\n${secondCommit}\n`)}); + const normal = ${JSON.stringify(`${firstCommit}\n${secondCommit}\n`)}; + const withPhantom = ${JSON.stringify(`${phantomCommit}\n${firstCommit}\n${secondCommit}\n`)}; + if (process.env.FAKE_GIT_ALWAYS_MISMATCH) { + process.stdout.write(withPhantom); + process.exit(0); + } + const requiredDeepens = Number(process.env.FAKE_GIT_REQUIRE_DEEPENS || 0); + if (requiredDeepens > 0) { + const deepenCallsPath = ${JSON.stringify(join(dir, 'deepen-called'))}; + const deepenCalls = existsSync(deepenCallsPath) + ? readFileSync(deepenCallsPath, 'utf8').length + : 0; + const unshallowed = existsSync(${JSON.stringify(join(dir, 'unshallow-called'))}); + if (!unshallowed && deepenCalls < requiredDeepens) { + process.stdout.write(withPhantom); + process.exit(0); + } + } + process.stdout.write(normal); process.exit(0); }