diff --git a/.github/scripts/lockfile-trust.mjs b/.github/scripts/lockfile-trust.mjs index c710f58ebbe..3ac0701ea34 100644 --- a/.github/scripts/lockfile-trust.mjs +++ b/.github/scripts/lockfile-trust.mjs @@ -11,8 +11,8 @@ * workflow writes it. After a verified Dependabot lockfile merges, the refresh workflow renews * the proof for it. * - * Keep this file free of imports outside node:*. The PR policy job runs the copy from the base - * commit, so a pull request cannot rewrite the check that judges it. + * Keep this file free of imports outside node:*. The PR policy job runs the copy from the current + * base-branch tip, so a pull request cannot rewrite the check that judges it. * * Usage: * node .github/scripts/lockfile-trust.mjs pr # PR policy job @@ -20,7 +20,7 @@ */ import { createHash } from 'node:crypto'; import { execFileSync } from 'node:child_process'; -import { appendFileSync } from 'node:fs'; +import { appendFileSync, realpathSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -211,7 +211,8 @@ function checkMerged() { writeOutputs({ trusted_lock_sha256: lockSha256, dependabot_pr: String(pull.number) }); } -if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { +// Compare real paths: the runner's temporary directory can sit behind a symlink. +if (process.argv[1] && realpathSync(path.resolve(process.argv[1])) === realpathSync(fileURLToPath(import.meta.url))) { const mode = process.argv[2]; if (mode === 'pr') checkPullRequest(); else if (mode === 'merged') checkMerged(); diff --git a/.github/scripts/tests/lockfile-trust.test.mjs b/.github/scripts/tests/lockfile-trust.test.mjs index a669ce9c880..026c6a7ffc8 100644 --- a/.github/scripts/tests/lockfile-trust.test.mjs +++ b/.github/scripts/tests/lockfile-trust.test.mjs @@ -1,7 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { execFileSync, spawnSync } from 'node:child_process'; -import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { @@ -201,8 +201,8 @@ test('pr command blocks a contributor lockfile change', () => { test('workflows run the trust policy from trusted code and pass the verified hash to the proof refresh', () => { const pr = readFileSync('.github/workflows/pr-trusted.yml', 'utf8'); - assert.match(pr, /git show "\$BASE_SHA:\.github\/scripts\/lockfile-trust\.mjs" > "\$policy"/); - assert.match(pr, /node "\$policy" pr/); + assert.match(pr, /git show "\$trusted_base:\.github\/scripts\/lockfile-trust\.mjs" > "\$policy"/); + assert.match(pr, /BASE_SHA="\$trusted_base" node "\$policy" pr/); assert.match(pr, /PAPERCLIP_CI_PR_AUTHOR: \$\{\{ github\.event\.pull_request\.user\.login \}\}/); assert.doesNotMatch(pr, /github\.head_ref != 'chore\/refresh-lockfile'/); @@ -211,3 +211,99 @@ test('workflows run the trust policy from trusted code and pass the verified has assert.match(refresh, /--refresh --trusted-lock-sha256 "\$TRUSTED_LOCK_SHA256"/); assert.ok(refresh.indexOf('lockfile-trust.mjs merged') < refresh.indexOf('--trusted-lock-sha256')); }); + +// Run the policy job's step script, as GitHub would, against a merge commit. +// RUNNER_TEMP is a symlink, so the policy must still recognize itself as the entry point. +function runPolicyStep({ staleBase, githubSha, headSha, author, headRef, root }) { + const workflow = readFileSync('.github/workflows/pr-trusted.yml', 'utf8'); + const step = workflow.split(' - name: Block untrusted lockfile edits\n')[1]; + const lines = step.split('\n'); + const start = lines.findIndex((line) => line === ' run: |') + 1; + const end = lines.findIndex((line, index) => index >= start && line.trim() !== '' && !line.startsWith(' ')); + const script = lines.slice(start, end).map((line) => line.slice(10)).join('\n'); + const output = path.join(root, '.output'); + writeFileSync(output, ''); + const runnerTemp = path.join(mkdtempSync(path.join(tmpdir(), 'policy-runner-')), 'temp'); + symlinkSync(mkdtempSync(path.join(tmpdir(), 'policy-runner-real-')), runnerTemp); + const result = spawnSync('bash', ['-e', '-c', script], { + cwd: root, encoding: 'utf8', + env: { ...process.env, RUNNER_TEMP: runnerTemp, GITHUB_OUTPUT: output, + GITHUB_REPOSITORY: repo, GITHUB_SHA: githubSha, BASE_SHA: staleBase, HEAD_SHA: headSha, PR_NUMBER: '1', + PR_AUTHOR: author, HEAD_REPO: repo, HEAD_REF: headRef }, + }); + return { status: result.status, stdout: result.stdout + result.stderr, output: readFileSync(output, 'utf8') }; +} + +test('the policy step loads the policy from the merge commit when the recorded base is stale', () => { + const root = mkdtempSync(path.join(tmpdir(), 'lockfile-policy-step-')); + try { + const git = (...args) => execFileSync('git', args, { cwd: root, encoding: 'utf8' }).trim(); + const commit = (message) => { + git('add', '.'); + git('-c', 'user.name=Fixture', '-c', 'user.email=fixture@example.invalid', 'commit', '-qm', message); + return git('rev-parse', 'HEAD'); + }; + git('init', '-q', '-b', 'master'); + writeFileSync(path.join(root, 'pnpm-lock.yaml'), 'lock: base\n'); + const staleBase = commit('base before the policy'); + // A contributor branch named like the refresh branch, from the stale base. The earlier rule exempts it. + git('checkout', '-q', '-b', 'chore/refresh-lockfile'); + writeFileSync(path.join(root, 'pnpm-lock.yaml'), 'lock: contributor\n'); + const head = commit('contributor lockfile'); + git('checkout', '-q', 'master'); + execFileSync('mkdir', ['-p', path.join(root, '.github/scripts')]); + writeFileSync(path.join(root, '.github/scripts/lockfile-trust.mjs'), readFileSync('.github/scripts/lockfile-trust.mjs')); + commit('add the policy'); + git('-c', 'user.name=GitHub', '-c', 'user.email=noreply@github.com', 'merge', '-q', '--no-ff', '-X', 'theirs', '-m', 'merge', head); + const merge = git('rev-parse', 'HEAD'); + + const blocked = runPolicyStep({ root, staleBase, githubSha: merge, headSha: head, author: 'someuser', headRef: 'chore/refresh-lockfile' }); + assert.equal(blocked.status, 1, blocked.stdout); + assert.match(blocked.stdout, /Untrusted lockfile change/); + + // A contributor pull request that does not touch lock content passes through the loaded policy. + git('checkout', '-q', '-b', 'docs', staleBase); + writeFileSync(path.join(root, 'notes.md'), 'docs\n'); + const docs = commit('docs only'); + git('checkout', '-q', 'master'); + git('-c', 'user.name=GitHub', '-c', 'user.email=noreply@github.com', 'merge', '-q', '--no-ff', '-m', 'merge docs', docs); + const allowed = runPolicyStep({ root, staleBase, githubSha: git('rev-parse', 'HEAD'), headSha: docs, author: 'someuser', headRef: 'docs' }); + assert.equal(allowed.status, 0, allowed.stdout); + assert.match(allowed.stdout, /does not change lock content/); + assert.match(allowed.output, /^writer=none$/m); + + // Without GitHub's merge commit, the step keeps the recorded base, which predates the policy. + const fallback = runPolicyStep({ root, staleBase, githubSha: head, headSha: head, author: 'someuser', headRef: 'chore/refresh-lockfile' }); + assert.equal(fallback.status, 0, fallback.stdout); + assert.match(fallback.output, /^writer=none$/m); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test('the policy step fails when the loaded policy exits without reporting a writer', () => { + const root = mkdtempSync(path.join(tmpdir(), 'lockfile-policy-silent-')); + try { + const git = (...args) => execFileSync('git', args, { cwd: root, encoding: 'utf8' }).trim(); + const commit = (message) => { + git('add', '.'); + git('-c', 'user.name=Fixture', '-c', 'user.email=fixture@example.invalid', 'commit', '-qm', message); + return git('rev-parse', 'HEAD'); + }; + git('init', '-q', '-b', 'master'); + execFileSync('mkdir', ['-p', path.join(root, '.github/scripts')]); + writeFileSync(path.join(root, '.github/scripts/lockfile-trust.mjs'), 'process.exit(0);\n'); + const baseSha = commit('a policy that skips its checks'); + git('checkout', '-q', '-b', 'feature'); + writeFileSync(path.join(root, 'pnpm-lock.yaml'), 'lock: contributor\n'); + const head = commit('contributor lockfile'); + git('checkout', '-q', 'master'); + git('-c', 'user.name=GitHub', '-c', 'user.email=noreply@github.com', 'merge', '-q', '--no-ff', '-m', 'merge', head); + + const result = runPolicyStep({ root, staleBase: baseSha, githubSha: git('rev-parse', 'HEAD'), headSha: head, author: 'someuser', headRef: 'feature' }); + assert.equal(result.status, 1, result.stdout); + assert.match(result.stdout, /did not report a lockfile writer/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/.github/workflows/pr-trusted.yml b/.github/workflows/pr-trusted.yml index 7a151a783c7..1231280351f 100644 --- a/.github/workflows/pr-trusted.yml +++ b/.github/workflows/pr-trusted.yml @@ -133,8 +133,8 @@ jobs: # Only trusted environments write lock content: the lockfile refresh # workflow, and Dependabot when the lockfile bytes come from a verified - # Dependabot commit. Run the policy from the base commit so a pull request - # cannot rewrite the check that judges it. + # Dependabot commit. Run the policy from the current base-branch tip so a + # pull request cannot rewrite the check that judges it. - name: Block untrusted lockfile edits id: lockfile_writer env: @@ -146,9 +146,22 @@ jobs: HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} HEAD_REF: ${{ github.head_ref }} run: | + # GitHub does not move pull_request.base.sha forward on every push, so + # it can name an old base-branch commit. The checkout is GitHub's merge + # commit: its first parent is the current base-branch tip, and its + # second parent is the PR head. Load the policy from that tip. + trusted_base="$BASE_SHA" + if [ "$(git rev-parse --verify --quiet "$GITHUB_SHA^2")" = "$HEAD_SHA" ]; then + trusted_base="$(git rev-parse "$GITHUB_SHA^1")" + fi policy="$RUNNER_TEMP/lockfile-trust.mjs" - if git show "$BASE_SHA:.github/scripts/lockfile-trust.mjs" > "$policy" 2>/dev/null; then - node "$policy" pr + if git show "$trusted_base:.github/scripts/lockfile-trust.mjs" > "$policy" 2>/dev/null; then + BASE_SHA="$trusted_base" node "$policy" pr + # Fail closed if the policy did not run. + if ! grep -q '^writer=' "$GITHUB_OUTPUT"; then + echo "::error title=Lockfile policy::lockfile-trust.mjs did not report a lockfile writer." + exit 1 + fi exit 0 fi # The base predates lockfile-trust.mjs: keep the earlier rule. @@ -156,7 +169,7 @@ jobs: if [ "$HEAD_REF" = 'chore/refresh-lockfile' ] || [ "$PR_AUTHOR" = 'dependabot[bot]' ]; then exit 0 fi - changed="$(git diff --name-only "$BASE_SHA...$HEAD_SHA")" + changed="$(git diff --name-only "$trusted_base...$HEAD_SHA")" if printf '%s\n' "$changed" | grep -qx 'pnpm-lock.yaml'; then echo "Do not commit pnpm-lock.yaml in pull requests. CI owns lockfile updates." exit 1 diff --git a/doc/RELEASE-AUTOMATION-SETUP.md b/doc/RELEASE-AUTOMATION-SETUP.md index d5541c23e6a..45607c30e85 100644 --- a/doc/RELEASE-AUTOMATION-SETUP.md +++ b/doc/RELEASE-AUTOMATION-SETUP.md @@ -228,7 +228,7 @@ If `@cryppadotta` is not the right reviewer identity in the public repo, change The reporting-lock proof (`tests/runner-e2e/report-lock.json` and `report-lock.patch`) controls the lock that paid evaluation jobs install, so it counts as lock content too. Only this workflow writes it. -`.github/scripts/lockfile-trust.mjs` enforces these rules in the `pr-trusted.yml` policy job. The job runs the copy of the script from the base commit, so a pull request cannot change the check that judges it. To update a Dependabot pull request that is behind `master`, comment `@dependabot recreate`. Do not merge `master` into its branch, because a merged lockfile no longer matches Dependabot's commit. +`.github/scripts/lockfile-trust.mjs` enforces these rules in the `pr-trusted.yml` policy job. The job runs the copy of the script from the current tip of the base branch, which is the first parent of GitHub's merge commit. A pull request therefore cannot change the check that judges it. The job does not use `pull_request.base.sha`, because GitHub does not update it on every push. To update a Dependabot pull request that is behind `master`, comment `@dependabot recreate`. Do not merge `master` into its branch, because a merged lockfile no longer matches Dependabot's commit. After a Dependabot lockfile merges, this workflow renews the proof. It first confirms three things: GitHub signed the merge commit, Dependabot opened the pull request, and the lockfile at the pull request head matches a verified Dependabot commit. It then records the merged lockfile in the proof and merges the refresh like any other. Paid evaluation jobs refuse the new lockfile until that refresh merges.