diff --git a/.changeset/signoff-release-token.md b/.changeset/signoff-release-token.md new file mode 100644 index 00000000..067d4efe --- /dev/null +++ b/.changeset/signoff-release-token.md @@ -0,0 +1,4 @@ +--- +--- + +Pin the breaking-change signoff to the release's changesets and version rather than the PR head commit, so an ordinary push no longer voids a valid signoff. CI-only, no package changes. diff --git a/.github/scripts/major-release-signoff.test.sh b/.github/scripts/major-release-signoff.test.sh index 90cf61f7..7da8a4a4 100644 --- a/.github/scripts/major-release-signoff.test.sh +++ b/.github/scripts/major-release-signoff.test.sh @@ -454,5 +454,57 @@ else pass "an unresolved context fails the status on every event" fi +# --- signoff is pinned to the release, not the commit (YPE-6068) --------------- + +if grep -q 'TOKEN_RE="(\^|\[\^0-9A-Za-z\])\${SIGNOFF_TOKEN}' "$WORKFLOW" \ + && ! grep -q 'SHA_RE=' "$WORKFLOW"; then + pass "the signoff matches the release token, not the head SHA" +else + fail "the signoff matches the release token, not the head SHA" \ + "matching on HEAD_SHA voids a valid signoff on every push" +fi + +if grep -q 'SIGNOFF_TOKEN" =~ \^\[0-9a-f\]{16}\$' "$WORKFLOW"; then + pass "a malformed release token fails closed" +else + fail "a malformed release token fails closed" \ + "an empty token would degenerate the matcher into one that matches almost any comment" +fi + +if grep -q 'echo "v\$NEXT_VERSION \$SIGNOFF_TOKEN' "$WORKFLOW" \ + && ! grep -q 'echo "v\$NEXT_VERSION \$HEAD_SHA' "$WORKFLOW"; then + pass "the copy-paste block names the token the matcher looks for" +else + fail "the copy-paste block names the token the matcher looks for" \ + "instructions that name a different value than the matcher are unfollowable" +fi + +if grep -q 'signoff_token: \${{ steps.preview.outputs.signoff_token }}' "$WORKFLOW" \ + && grep -q 'SIGNOFF_TOKEN: \${{ needs.preview.outputs.signoff_token }}' "$WORKFLOW"; then + pass "the token reaches the signoff and comment steps from the preview job" +else + fail "the token reaches the signoff and comment steps from the preview job" \ + "an unset SIGNOFF_TOKEN is caught by the guard, but only after the job has already run" +fi + +if grep -qF 'scripts/preview-release.mjs scripts/signoff-token.mjs' "$WORKFLOW"; then + pass "the token module is restored from main with the rest of the tooling" +else + fail "the token module is restored from main with the rest of the tooling" \ + "a PR-supplied token module could mint a token matching an older signoff" +fi + +# The gate reads `.signoff_token` out of preview.json. If the preview stops emitting it the +# guard rejects an empty token and every major PR blocks, which is how this shipped the first +# time: four assertions about the workflow's text all passed while the key was never written. +PREVIEW_SRC="$ROOT/scripts/preview-release.mjs" +if grep -q "signoff_token: signoffToken(" "$PREVIEW_SRC" \ + && grep -q "jq -r '.signoff_token // empty' preview.json" "$WORKFLOW"; then + pass "the preview emits the token under the key the gate reads" +else + fail "the preview emits the token under the key the gate reads" \ + "an unemitted or renamed key blocks every major PR behind an empty-token guard" +fi + printf '\n%d passed, %d failed\n' "$passes" "$failures" [[ "$failures" -eq 0 ]] diff --git a/.github/scripts/signoff-token.test.mjs b/.github/scripts/signoff-token.test.mjs new file mode 100644 index 00000000..a4a53dd9 --- /dev/null +++ b/.github/scripts/signoff-token.test.mjs @@ -0,0 +1,90 @@ +// Drives signoffToken against real git trees. Each case mutates a scratch repo and asserts +// whether the token moved, which is the property the gate relies on. +import { execFileSync } from 'node:child_process' +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, renameSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { signoffToken } from '../../scripts/signoff-token.mjs' + +const repo = mkdtempSync(join(tmpdir(), 'signoff-token-')) +const git = (...a) => execFileSync('git', a, { cwd: repo, encoding: 'utf8' }).trim() +const commit = (msg) => { git('add', '-A'); git('-c', 'commit.gpgsign=false', 'commit', '-q', '-m', msg); return git('rev-parse', 'HEAD') } +let base +const tok = (head, v = '2.0.0') => signoffToken({ repoRoot: repo, base, head, nextVersion: v }) + +git('init', '-q', '-b', 'main'); git('config', 'user.email', 't@t'); git('config', 'user.name', 't') +mkdirSync(join(repo, '.changeset')) +writeFileSync(join(repo, '.changeset', 'a.md'), "---\n'p': major\n---\n\nbreak\n") +writeFileSync(join(repo, '.changeset', 'README.md'), 'docs\n') +writeFileSync(join(repo, 'src.txt'), 'code\n') +writeFileSync(join(repo, '.changeset', 'config.json'), '{}\n') +base = commit('base'); const baseTok = tok(base) + +const results = [] +const check = (name, actual, expectSame) => + results.push({ name, ok: (actual === baseTok) === expectSame, actual, expectSame }) + +writeFileSync(join(repo, 'src.txt'), 'code changed\n') +check('ordinary code push keeps the token', tok(commit('code')), true) + +writeFileSync(join(repo, '.changeset', 'README.md'), 'docs edited\n') +check('editing .changeset/README.md keeps the token', tok(commit('readme')), true) + +check('a version change replaces the token', tok(git('rev-parse', 'HEAD'), '2.0.1'), false) + +const afterDocs = git('rev-parse', 'HEAD') +writeFileSync(join(repo, '.changeset', 'b.md'), "---\n'p': patch\n---\n\nfix\n") +check('adding a changeset replaces the token', tok(commit('add changeset')), false) + +git('reset', '-q', '--hard', afterDocs) +writeFileSync(join(repo, '.changeset', 'a.md'), "---\n'p': major\n---\n\nbreak differently\n") +check('editing a changeset replaces the token', tok(commit('edit changeset')), false) + +git('reset', '-q', '--hard', afterDocs) +rmSync(join(repo, '.changeset', 'a.md')) +check('deleting a changeset replaces the token', tok(commit('delete changeset')), false) + +git('reset', '-q', '--hard', afterDocs) +renameSync(join(repo, '.changeset', 'a.md'), join(repo, '.changeset', 'renamed.md')) +check('renaming a changeset replaces the token', tok(commit('rename changeset')), false) + +// A tab in the path is why the blob line is split at the first tab only. Splitting on every +// tab drops the file from the digest, and a dropped file cannot be seen to change, so the +// discriminating assertion is that EDITING it moves the token. +git('reset', '-q', '--hard', afterDocs) +renameSync(join(repo, '.changeset', 'a.md'), join(repo, '.changeset', 'with\ttab.md')) +const tabBefore = tok(commit('tab-named changeset')) +writeFileSync(join(repo, '.changeset', 'with\ttab.md'), "---\n'p': major\n---\n\nbreak, reworded\n") +const tabAfter = tok(commit('edit the tab-named changeset')) +results.push({ + name: 'editing a tab-named changeset replaces the token', + ok: tabBefore !== tabAfter, +}) + +git('reset', '-q', '--hard', afterDocs) +writeFileSync(join(repo, '.changeset', 'config.json'), '{"fixed": []}\n') +check('editing .changeset/config.json replaces the token', tok(commit('edit config')), false) + +// The branch has to be current with main before it merges, and every other PR brings its own +// changeset. Merging main in must leave the signoff standing, so only this PR's changesets count. +git('reset', '-q', '--hard', afterDocs) +git('checkout', '-q', '-b', 'pr') +writeFileSync(join(repo, '.changeset', 'mine.md'), "---\n'p': major\n---\n\nmine\n") +const prTok = tok(commit('pr changeset')) +git('checkout', '-q', 'main') +git('reset', '-q', '--hard', afterDocs) +writeFileSync(join(repo, '.changeset', 'theirs.md'), "---\n'p': patch\n---\n\ntheirs\n") +commit('another PR lands on main') +git('checkout', '-q', 'pr') +git('-c', 'commit.gpgsign=false', 'merge', '-q', '--no-edit', 'main') +const savedBase = base +base = 'main' +results.push({ + name: 'merging a changeset in from main keeps the token', + ok: tok(git('rev-parse', 'HEAD')) === prTok, +}) +base = savedBase + +rmSync(repo, { recursive: true, force: true }) +for (const r of results) console.log(`${r.ok ? 'ok' : 'FAIL'}\t${r.name}`) +process.exit(results.every((r) => r.ok) ? 0 : 1) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 76560c8a..1ef77d03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -83,6 +83,9 @@ jobs: - name: Run lint run: pnpm lint + - name: Run repo script regression tests + run: pnpm test:ci-scripts + typecheck: name: Type Check runs-on: ubuntu-latest diff --git a/.github/workflows/major-release-signoff.yml b/.github/workflows/major-release-signoff.yml index bf3e70d3..52581857 100644 --- a/.github/workflows/major-release-signoff.yml +++ b/.github/workflows/major-release-signoff.yml @@ -259,6 +259,7 @@ jobs: outputs: is_major: ${{ steps.preview.outputs.is_major }} next: ${{ steps.preview.outputs.next }} + signoff_token: ${{ steps.preview.outputs.signoff_token }} release_type: ${{ steps.preview.outputs.release_type }} steps: - name: Checkout PR head @@ -279,7 +280,7 @@ jobs: # under, and the changesets config have to agree, so restoring some from main and # taking the rest from the PR gives a detector without its own dependencies. if git cat-file -e main:scripts/preview-release.mjs 2>/dev/null; then - for f in scripts/preview-release.mjs package.json pnpm-lock.yaml \ + for f in scripts/preview-release.mjs scripts/signoff-token.mjs package.json pnpm-lock.yaml \ pnpm-workspace.yaml .changeset/config.json; do git cat-file -e "main:$f" 2>/dev/null && git checkout main -- "$f" done @@ -344,6 +345,7 @@ jobs: # "null", which is not empty, so the `${VAR:-default}` fallbacks downstream never # fire and the status reads "No breaking changeset added (null)". echo "next=$(jq -r '.next // empty' preview.json)" >> "$GITHUB_OUTPUT" + echo "signoff_token=$(jq -r '.signoff_token // empty' preview.json)" >> "$GITHUB_OUTPUT" echo "release_type=$(jq -r '.release_type // empty' preview.json)" >> "$GITHUB_OUTPUT" @@ -503,6 +505,7 @@ jobs: PR_AUTHOR: ${{ needs.context.outputs.pr_author }} HEAD_SHA: ${{ needs.context.outputs.head_sha }} NEXT_VERSION: ${{ steps.decision.outputs.next }} + SIGNOFF_TOKEN: ${{ needs.preview.outputs.signoff_token }} run: | set -euo pipefail # Guard: a major changeset must also have produced a real `next` @@ -532,15 +535,21 @@ jobs: } NORM_TEMPLATE=$(normalize "$TEMPLATE_PHRASE") - # A signoff approves the code it was given on, so it names that code. Comparing - # timestamps instead would trust `committer.date`, which the committer sets, so a - # backdated commit could inherit an older approval. A SHA cannot be backdated, and - # a new push changes it, which retires the previous signoff on its own. - # The full 40 characters, not an abbreviation: a 7-character prefix is - # 28 bits, which is cheap to brute-force, so an author could craft a new - # commit sharing the approved prefix and inherit its signoff. - SHA_RE="(^|[^0-9A-Za-z])${HEAD_SHA}([^0-9A-Za-z]|$)" - echo "Signoff must name head $HEAD_SHA." + # A signoff approves a release, so it names that release: the changesets this PR + # touches, `.changeset/config.json` and the resulting version, digested by + # `signoffToken` in scripts/signoff-token.mjs. + # Naming the head SHA instead voided the signoff on every push, so a lint fix cost a + # fresh one while changing nothing about what was approved. Comparing timestamps is not + # an option either: `committer.date` is attacker-controlled, so a backdated commit could + # inherit an older approval. The digest is content-derived and cannot be backdated. + # 64 bits, not a short prefix, so a second preimage against an approved token is out of + # reach for someone wanting a different release to inherit its signoff. + if ! [[ "$SIGNOFF_TOKEN" =~ ^[0-9a-f]{16}$ ]]; then + echo "::error::Refusing to evaluate a signoff without a well-formed release token." + exit 1 + fi + TOKEN_RE="(^|[^0-9A-Za-z])${SIGNOFF_TOKEN}([^0-9A-Za-z]|$)" + echo "Signoff must name release token $SIGNOFF_TOKEN." COMMENTS=$(gh api --paginate \ "repos/${{ github.repository }}/issues/$PR_NUMBER/comments" \ @@ -563,7 +572,7 @@ jobs: NORM_BODY=$(normalize "$BODY") if [[ "$NORM_BODY" != *"$NORM_TEMPLATE"* ]]; then continue; fi if ! [[ "$BODY" =~ $VERSION_RE ]] || ! [[ "$BODY" =~ $ROCKET_RE ]]; then continue; fi - if ! [[ "$BODY" =~ $SHA_RE ]]; then continue; fi + if ! [[ "$BODY" =~ $TOKEN_RE ]]; then continue; fi # Then verify write+ permission. Cached so a chatty approver doesn't # cost one API call per comment. PERM="${PERM_CACHE[$LOGIN]:-}" @@ -609,6 +618,7 @@ jobs: PR_NUMBER: ${{ needs.context.outputs.pr_number }} HEAD_SHA: ${{ needs.context.outputs.head_sha }} NEXT_VERSION: ${{ steps.decision.outputs.next }} + SIGNOFF_TOKEN: ${{ needs.preview.outputs.signoff_token }} run: | set -euo pipefail MARKER='' @@ -622,7 +632,7 @@ jobs: echo echo "1. The verbatim acknowledgment phrase below," echo "2. The precise next version (\`v$NEXT_VERSION\` or \`$NEXT_VERSION\`)," - echo "3. The full commit hash being approved (\`$HEAD_SHA\`), and" + echo "3. The release token being approved (\`$SIGNOFF_TOKEN\`), and" echo "4. A 🚀 (\`:rocket:\`) emoji." echo echo "I confirm that this is an intentional breaking change, and I have read the release procedures. I understand and have documented its impact upon release." @@ -632,10 +642,10 @@ jobs: echo '```' echo "I confirm that this is an intentional breaking change, and I have read the release procedures. I understand and have documented its impact upon release." echo - echo "v$NEXT_VERSION $HEAD_SHA 🚀" + echo "v$NEXT_VERSION $SIGNOFF_TOKEN 🚀" echo '```' echo - echo "The check re-runs automatically when a qualifying comment is posted or edited. Pushing new commits changes the hash, so a fresh signoff is needed." + echo "The check re-runs automatically when a qualifying comment is posted or edited. The token covers this release's changesets and version, so ordinary pushes (review fixes, lint, docs) leave this signoff standing; adding or editing a changeset, or a change to the version, needs a fresh one." } > comment.md EXISTING=$(gh api --paginate \ diff --git a/package.json b/package.json index 1fb7d39b..e83d5a54 100644 --- a/package.json +++ b/package.json @@ -42,7 +42,7 @@ "analyze:select": "node scripts/analyze-select.mjs", "generate:i18n": "pnpm --filter @youversion/platform-react-ui generate:i18n", "check:i18n": "node scripts/check-i18n-parity.mjs", - "test:ci-scripts": "bash .github/scripts/check-locale-ownership.test.sh && bash .github/scripts/major-release-signoff.test.sh", + "test:ci-scripts": "bash .github/scripts/check-locale-ownership.test.sh && bash .github/scripts/major-release-signoff.test.sh && node .github/scripts/signoff-token.test.mjs", "storybook": "pnpm --filter @youversion/platform-react-ui storybook" }, "devDependencies": { diff --git a/scripts/preview-release.mjs b/scripts/preview-release.mjs index a54b7c20..c7704821 100755 --- a/scripts/preview-release.mjs +++ b/scripts/preview-release.mjs @@ -17,11 +17,12 @@ // unrelated PR until the release went out. // // Output (stdout): one line of JSON: -// { current, next, release_type, is_major, introduced_major, packages, added_changesets } +// { current, next, release_type, is_major, introduced_major, packages, added_changesets, signoff_token } // // Usage: // node scripts/preview-release.mjs --base [--head ] import { execFileSync } from 'node:child_process'; +import { isChangesetPath, signoffToken } from './signoff-token.mjs'; import parseChangeset from '@changesets/parse'; import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -135,7 +136,7 @@ function addedChangesetLevels(base) { // `[^]` rather than `.`: a filename containing a line terminator would otherwise fail // this test and drop out of the scan entirely. - const isChangeset = (f) => /^\.changeset\/[^]+\.md$/.test(f) && !/README\.md$/.test(f); + const isChangeset = isChangesetPath; const levels = []; const touched = []; @@ -188,5 +189,11 @@ console.log( introduced_major: levels.some((l) => l.level === 'major'), packages: releases.map((r) => r.name), added_changesets: added, + signoff_token: signoffToken({ + repoRoot: REPO_ROOT, + base: args.base, + head, + nextVersion: versions[0] ?? null, + }), }), ); diff --git a/scripts/signoff-token.mjs b/scripts/signoff-token.mjs new file mode 100644 index 00000000..bc28f0a3 --- /dev/null +++ b/scripts/signoff-token.mjs @@ -0,0 +1,66 @@ +#!/usr/bin/env node +/** + * The identifier a breaking-change signoff names. + * + * Pinning a signoff to the head commit meant every push voided it, so a lint fix or a review + * tweak cost a fresh signoff while changing nothing about the release. That trains people to + * rubber-stamp the gate, and it is stricter than this repo's own review policy, which keeps + * approvals across pushes (`dismiss_stale_reviews_on_push: false`). + * + * Keyed on the changesets this PR adds, edits, renames or removes since `base` (the merge-base), plus + * `.changeset/config.json` at head and the resulting version. Keying on every changeset at head + * voided the signoff whenever main was merged in, because main brings other PRs' changesets. + * Every changeset this PR touches counts, not only the ones declaring major: any changeset edit + * can move the release, and erring toward asking again is the safe direction. Blob ids come from + * git, so the hash changes if and only if the content does. Renaming a changeset re-triggers, + * which is correct: Changesets reads it by path. + * + * `config.json` is read from the head commit, not the working tree: the preview job restores + * main's copy before running, but after the merge this PR's copy is the one that governs. + * + * 16 hex characters (64 bits). The signoff names this value, so a collaborator who wanted a + * different release to inherit an existing signoff would have to grind a second preimage against + * it; 64 bits puts that out of reach, where a short prefix would not. + * + * Lives in its own module so the gate's suite can exercise it against real git trees. Testing it + * through `preview-release.mjs` would mean standing up Changesets and a workspace first, which is + * how it previously shipped covered only by assertions about the workflow's text. + */ +import { execFileSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; + +/** Mirrors @changesets/read: a `.md` under `.changeset/` that is not README. */ +export const isChangesetPath = (f) => + /^\.changeset\/[^]+\.md$/.test(f) && !/README\.md$/.test(f); + +export function signoffToken({ repoRoot, base, head, nextVersion }) { + const git = (...args) => + execFileSync('git', args, { cwd: repoRoot, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 }); + // `--raw -z`: `: `, then one path, or two + // for a rename. `-z` keeps unusual paths unquoted, so they still pass the eligibility test. + const fields = git('diff', '--raw', '-z', '-M', '--no-abbrev', base, head, '--', '.changeset') + .split('\0') + .filter((f) => f !== ''); + const changes = []; + for (let i = 0; i < fields.length; ) { + const [, newMode, , newBlob, status] = fields[i].split(' '); + const pathCount = /^[RC]/.test(status) ? 2 : 1; + const paths = fields.slice(i + 1, i + 1 + pathCount); + i += 1 + pathCount; + if (!paths.some(isChangesetPath)) continue; + changes.push([status[0], newMode, newBlob, ...paths]); + } + changes.sort((a, b) => (a.join('\0') < b.join('\0') ? -1 : 1)); + + let config = null; + try { + config = git('rev-parse', '--verify', '--quiet', `${head}:.changeset/config.json`).trim(); + } catch { + // Absent at head. Null still hashes distinctly from any blob id. + } + + return createHash('sha256') + .update(JSON.stringify({ nextVersion: nextVersion ?? null, config, changes })) + .digest('hex') + .slice(0, 16); +}