* chore(#2331): trigger PR-policy workflows on pull_request_target Three PR-policy workflows (pr-title-validator, pr-target-validator, require-issue-link) triggered on plain `pull_request`, so a fork PR's GITHUB_TOKEN was downgraded to read-only regardless of the declared `permissions:`. Each one comments on the PR and THEN emits its verdict, so the createComment 403 killed the github-script step before core.setFailed ran: the contributor saw an API stack trace instead of the instructions the comment exists to deliver. Confirmed on PR #2084 (job 86573823878), whose title has been non-compliant since 2026-07-08 while the explanatory comment 403'd on every run. Switches all three to pull_request_target (base-repo context, write-capable token), matching the three siblings that already do this correctly (pr-template-format, close-draft-prs, auto-close-unsolicited-prs). Safe: the only checkouts are BASE-branch with persist-credentials: false, and every PR-controlled input is read as data — no head code executes. Also wraps each comment in try/catch so a comment failure can never again suppress the verdict. Extends tests/workflow-maintainer-skip.test.cjs with the trigger lock already applied to close-draft-prs.yml (:32-42) for this same defect class. Closes #2331 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2331): strip backticks before echoing untrusted text into bot comments Found by the orthogonal security review of this change. pr-title-validator and pr-target-validator echo attacker-controlled text (the PR title; the fork's branch name) into an inline-code span in a comment posted by github-actions[bot]. A single backtick closes the span early and the remainder renders as live Markdown — GFM autolinks a bare URL — so a fork author could make our own bot post an arbitrary clickable link into a PR thread, borrowing the bot's credibility for phishing. This interpolation is unchanged from next, but it was NOT previously reachable from forks: the createComment call 403'd and the comment was never posted. The trigger switch in the parent commit is what makes it reachable by untrusted authors for the first time, using the write token it grants — so it is in scope here and fixed here rather than deferred. A PR title has no charset restriction, so that vector is fully exploitable. The branch-name vector is weaker (check-ref-format forbids space, ':', '[' and '*', so no bare URL, link or emphasis is expressible) but is the same class and is stripped identically rather than left to the charset to police. Stripping the backtick is complete: it is the only character that can break out of an inline-code span. Only the rendered body needs this — core.warning/setFailed go to the job log, where @actions/core already escapes workflow commands. Also strengthens the try/catch test to assert core.setFailed sits AFTER the catch block rather than merely existing, so moving the verdict inside the try (the exact inversion #2331 fixes) fails the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2331): assert verdict ordering on code, not on comment prose The first cut of the verdict-ordering guard failed against correct code. It used indexOf('core.setFailed') on raw source, and these workflows name core.setFailed in their own comments while explaining the bug — at lines 29/118/147, 16 and 6, all BEFORE the catch block. So the assertion compared a comment to the call and reported the inversion it was written to catch. gsd-test caught it: 4 unique failures across linux-node22/24. The code was right; the test was measuring the wrong text. Fixes: - readWorkflowCode() strips whole-line YAML/JS comments so positional assertions see only executable text. - The ordering check is extracted to verdictSurvivesCommentFailure() and exercised against BOTH a good and an inverted sample, so the guard is proven non-vacuous rather than merely passing. - A test pins the trap itself: raw source really does mention core.setFailed before the catch, while the stripped view puts the real call after it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2331): drop the unnecessary permission widening; the trigger was the whole bug Both orthogonal review passes flagged the permissions block, from opposite directions — one said issues:write was dead surface on require-issue-link, the other said it was the load-bearing scope the two validators lacked. Neither is right, and the repo's own history settles it: - pr-title-validator declares pull-requests:write ONLY, and its sticky comment has posted 26 times. - pr-target-validator declares pull-requests:write ONLY — posted 8 times. - require-issue-link declares issues:write ONLY — posted on same-repo PRs #106, #164, #232, #259. So GitHub accepts EITHER scope for issues.createComment when the target is a PR, and all three files already declared a sufficient one. The 403 was purely the fork token downgrade. My added scopes fixed nothing and widened privilege on precisely the workflows now running as pull_request_target — the context where surplus scope matters most. Reverted: permissions are byte-identical to next, and the diff is now trigger + try/catch + sanitizer only. The permission test previously used an (issues|pull-requests) alternation, so it passed on the pre-fix tree and would not have caught removal of the scope that matters. It now asserts each file's SPECIFIC scope and, more usefully, asserts the absence of the other — locking the least-privilege property against a future 'add it to be safe' regression. It is a forward lock, not a #2331 fails-first test; the trigger assertion is the fails-first one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
318 lines
16 KiB
JavaScript
318 lines
16 KiB
JavaScript
// allow-test-rule: source-text-is-the-product
|
|
// These workflow files are deployed policy; the tests lock the maintainer
|
|
// carve-out so future edits do not accidentally re-enable enforcement.
|
|
'use strict';
|
|
|
|
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
|
|
const MAINTAINER_SKIP_EXPR = 'contains(fromJSON(\'["OWNER","MEMBER","COLLABORATOR"]\'), github.event.pull_request.author_association) == false';
|
|
|
|
function readWorkflow(relativePath) {
|
|
return fs.readFileSync(path.join(process.cwd(), relativePath), 'utf8');
|
|
}
|
|
|
|
// Comment-stripped view of a workflow, for assertions about CODE STRUCTURE.
|
|
// These files document their own rationale, so prose routinely names the very
|
|
// symbols a positional assertion looks for (e.g. "...and core.setFailed never
|
|
// runs"). Matching raw source makes such an assertion measure the comment
|
|
// rather than the call — the #2331 ordering test did exactly that and failed
|
|
// against correct code. Drop whole-line YAML (`#`) and JS (`//`) comments so
|
|
// positional checks see only executable text.
|
|
function readWorkflowCode(relativePath) {
|
|
return readWorkflow(relativePath)
|
|
.split('\n')
|
|
.filter((line) => {
|
|
const t = line.trim();
|
|
return t !== '' && !t.startsWith('#') && !t.startsWith('//');
|
|
})
|
|
.join('\n');
|
|
}
|
|
|
|
// True when the verdict call sits AFTER the comment-posting catch block — i.e.
|
|
// a thrown/403'd comment cannot skip the gate (#2331). Takes comment-stripped
|
|
// code. Extracted so the predicate itself can be exercised against a known-bad
|
|
// sample below; a presence-only check would pass on the inverted arrangement.
|
|
function verdictSurvivesCommentFailure(code) {
|
|
const catchWarning = code.indexOf('Could not post');
|
|
const verdict = code.indexOf('core.setFailed(');
|
|
if (catchWarning === -1 || verdict === -1) return false;
|
|
return verdict > catchWarning;
|
|
}
|
|
|
|
function assertMaintainerSkip(source) {
|
|
assert.ok(
|
|
source.includes(MAINTAINER_SKIP_EXPR),
|
|
`Expected workflow to include maintainer skip expression: ${MAINTAINER_SKIP_EXPR}`
|
|
);
|
|
}
|
|
|
|
describe('PR policy workflow maintainer carve-outs', () => {
|
|
test('draft PR auto-close does not run for maintainer-authored PRs', () => {
|
|
const workflow = readWorkflow('.github/workflows/close-draft-prs.yml');
|
|
|
|
assert.match(workflow, /github\.event\.pull_request\.draft == true/);
|
|
assertMaintainerSkip(workflow);
|
|
});
|
|
|
|
test('draft PR auto-close triggers on pull_request_target so fork PRs cannot bypass it', () => {
|
|
const workflow = readWorkflow('.github/workflows/close-draft-prs.yml');
|
|
|
|
// A bare `pull_request` trigger hands fork PRs (how first-time/external
|
|
// contributors contribute) a read-only GITHUB_TOKEN, so the close/comment
|
|
// API calls 403 and the draft PR survives — bypassing the auto-close.
|
|
// `pull_request_target` runs in the base-repo context with a write-capable
|
|
// token. Guard against a regression back to the bypassable trigger.
|
|
assert.match(workflow, /^\s*pull_request_target:/m);
|
|
assert.doesNotMatch(workflow, /^\s*pull_request:\s*$/m);
|
|
});
|
|
|
|
test('PR target validator does not run for maintainer-authored PRs', () => {
|
|
const workflow = readWorkflow('.github/workflows/pr-target-validator.yml');
|
|
|
|
assertMaintainerSkip(workflow);
|
|
});
|
|
|
|
// #2331: same defect class as the close-draft-prs.yml trigger lock above,
|
|
// swept across the three PR-policy workflows it had never covered. Each one
|
|
// comments on the PR and THEN emits its verdict; on a bare `pull_request`
|
|
// trigger a fork PR's read-only GITHUB_TOKEN 403s the comment call, the
|
|
// unhandled rejection kills the github-script step, and the verdict
|
|
// (core.setFailed) never runs — the contributor gets an API stack trace
|
|
// instead of the instructions the comment exists to deliver.
|
|
for (const { file, name, scope, otherScope } of [
|
|
{ file: '.github/workflows/pr-title-validator.yml', name: 'PR title validator', scope: 'pull-requests', otherScope: 'issues' },
|
|
{ file: '.github/workflows/pr-target-validator.yml', name: 'PR target validator', scope: 'pull-requests', otherScope: 'issues' },
|
|
{ file: '.github/workflows/require-issue-link.yml', name: 'Require issue link', scope: 'issues', otherScope: 'pull-requests' },
|
|
]) {
|
|
test(`${name} triggers on pull_request_target so fork PRs get the verdict, not a 403`, () => {
|
|
const workflow = readWorkflow(file);
|
|
|
|
assert.match(workflow, /^\s*pull_request_target:/m);
|
|
assert.doesNotMatch(workflow, /^\s*pull_request:\s*$/m);
|
|
});
|
|
|
|
test(`${name} keeps exactly the one write scope its comment call needs`, () => {
|
|
const workflow = readWorkflow(file);
|
|
|
|
// pull_request_target only grants what `permissions:` declares, so the
|
|
// write scope must be present or the trigger change alone would not fix
|
|
// the 403. Assert the SPECIFIC scope, not an `(issues|pull-requests)`
|
|
// alternation — an alternation is satisfied by whichever scope happens to
|
|
// be there and would not catch its removal.
|
|
//
|
|
// Each file needs only ONE: GitHub accepts either `issues: write` or
|
|
// `pull-requests: write` for issues.createComment when the target is a
|
|
// PR. Verified from this repo's history, not the docs — pr-title-validator
|
|
// has posted on `pull-requests: write` alone, require-issue-link on
|
|
// `issues: write` alone. The 403 was the fork downgrade, not the scope.
|
|
//
|
|
// The negative half is the point of this test: a pull_request_target
|
|
// workflow must not carry privilege it never exercises, so adding the
|
|
// other scope "to be safe" is a regression this catches.
|
|
assert.match(workflow, new RegExp(`^\\s*${scope}:\\s*write\\s*$`, 'm'));
|
|
assert.doesNotMatch(
|
|
workflow,
|
|
new RegExp(`^\\s*${otherScope}:\\s*write\\s*$`, 'm'),
|
|
`${file} does not call a ${otherScope}.* API — do not grant it write on a pull_request_target workflow`
|
|
);
|
|
});
|
|
|
|
test(`${name} cannot let a failed comment suppress its verdict`, () => {
|
|
const workflow = readWorkflow(file);
|
|
|
|
// Defense in depth: the comment is a courtesy, the verdict is the gate.
|
|
// Guard the inversion (comment throws -> setFailed skipped) that #2331
|
|
// fixed, so a future permission change degrades the diagnostic only.
|
|
assert.match(workflow, /\btry\s*\{/);
|
|
assert.match(workflow, /catch\s*\(err\)\s*\{[\s\S]*?core\.warning/);
|
|
|
|
// Assert the ORDER, not just the presence: the verdict must appear AFTER
|
|
// the catch block's core.warning. If it were moved inside the try, it
|
|
// would textually precede the catch — exactly the regression this locks.
|
|
// Presence-only assertions pass either way.
|
|
//
|
|
// Read the comment-stripped view: these workflows' own prose names
|
|
// `core.setFailed` while explaining the bug, and matching raw source made
|
|
// this assertion compare a comment instead of the call.
|
|
assert.equal(
|
|
verdictSurvivesCommentFailure(readWorkflowCode(file)),
|
|
true,
|
|
'core.setFailed must sit AFTER the catch block, not inside the try — ' +
|
|
'otherwise a thrown comment error skips the verdict (#2331)'
|
|
);
|
|
});
|
|
}
|
|
|
|
// #2331: these two workflows echo attacker-controlled text (PR title / fork
|
|
// branch name) into a bot-authored comment posted with a write token. Raw
|
|
// interpolation into an inline-code span lets a single backtick close the span
|
|
// so the remainder renders as live Markdown — on a PR title (no charset limit)
|
|
// that is enough to autolink an arbitrary URL from github-actions[bot].
|
|
for (const { file, name, varName } of [
|
|
{ file: '.github/workflows/pr-title-validator.yml', name: 'PR title validator', varName: 'titleForMarkdown' },
|
|
{ file: '.github/workflows/pr-target-validator.yml', name: 'PR target validator', varName: 'headForMarkdown' },
|
|
]) {
|
|
test(`${name} strips backticks before echoing untrusted text into the comment`, () => {
|
|
const workflow = readWorkflow(file);
|
|
|
|
// The sanitizer exists and removes the one character that can break out
|
|
// of an inline-code span.
|
|
assert.match(workflow, new RegExp(`const ${varName} = String\\(\\w+\\)\\.replace\\(/\`/g, "'"\\)`));
|
|
// The rendered comment interpolates the SANITIZED value, never the raw one.
|
|
assert.match(workflow, new RegExp(`\\\\\`\\$\\{${varName}\\}`));
|
|
});
|
|
}
|
|
|
|
test('the verdict-ordering predicate rejects the inversion it exists to catch', () => {
|
|
// Non-vacuity: prove the guard fails on the bad arrangement, not just that
|
|
// it passes on the current (good) one.
|
|
const good = [
|
|
'try {',
|
|
' await github.rest.issues.createComment({});',
|
|
'} catch (err) {',
|
|
' core.warning(`Could not post the comment (${err.status}).`);',
|
|
'}',
|
|
"core.setFailed('nope');",
|
|
].join('\n');
|
|
const inverted = [
|
|
'try {',
|
|
' await github.rest.issues.createComment({});',
|
|
" core.setFailed('nope');", // <-- swallowed by the catch
|
|
'} catch (err) {',
|
|
' core.warning(`Could not post the comment (${err.status}).`);',
|
|
'}',
|
|
].join('\n');
|
|
|
|
assert.equal(verdictSurvivesCommentFailure(good), true);
|
|
assert.equal(verdictSurvivesCommentFailure(inverted), false);
|
|
// Missing either half is not a pass.
|
|
assert.equal(verdictSurvivesCommentFailure("core.setFailed('x');"), false);
|
|
assert.equal(verdictSurvivesCommentFailure('core.warning(`Could not post`);'), false);
|
|
});
|
|
|
|
test('readWorkflowCode strips prose that would confuse a positional assertion', () => {
|
|
// The exact trap that made the first cut of the ordering test fail against
|
|
// correct code: these workflows name `core.setFailed` in their own comments,
|
|
// before the call, so a raw-source indexOf compares the comment.
|
|
const raw = readWorkflow('.github/workflows/pr-title-validator.yml');
|
|
const code = readWorkflowCode('.github/workflows/pr-title-validator.yml');
|
|
|
|
assert.ok(
|
|
raw.indexOf('core.setFailed') < raw.indexOf('Could not post'),
|
|
'precondition: raw source mentions core.setFailed in prose before the catch'
|
|
);
|
|
assert.ok(
|
|
code.indexOf('core.setFailed(') > code.indexOf('Could not post'),
|
|
'comment-stripped code puts the real call after the catch'
|
|
);
|
|
assert.doesNotMatch(code, /^\s*#/m, 'no YAML comment lines survive');
|
|
assert.doesNotMatch(code, /^\s*\/\//m, 'no JS comment lines survive');
|
|
});
|
|
|
|
test('the backtick sanitizer actually neutralizes the inline-code breakout', () => {
|
|
// Behavioral check of the transform the workflows apply, rather than only
|
|
// asserting the source text contains it.
|
|
const sanitize = (v) => String(v).replace(/`/g, "'");
|
|
const hostile = 'bad`See https://evil.example/ci-status for details x';
|
|
|
|
assert.match('`' + hostile + '`', /`bad`See/, 'pre-fix: the span breaks out');
|
|
assert.doesNotMatch('`' + sanitize(hostile) + '`', /`bad`/, 'post-fix: it cannot');
|
|
assert.equal(sanitize(hostile).includes('`'), false, 'no backtick survives');
|
|
// Boundary: no backtick, one backtick, many backticks.
|
|
assert.equal(sanitize('plain'), 'plain');
|
|
assert.equal(sanitize('`'), "'");
|
|
assert.equal(sanitize('``a``'), "''a''");
|
|
});
|
|
|
|
test('draft PR sweep enforces the same policy as the event-driven close', () => {
|
|
const workflow = readWorkflow('.github/workflows/close-draft-prs-sweep.yml');
|
|
|
|
// Timer-driven in base-repo context, plus a manual dispatch for testing.
|
|
// It must NOT be a fork-triggered event (no pull_request / pull_request_target trigger).
|
|
assert.match(workflow, /schedule:/);
|
|
assert.match(workflow, /cron:\s*'0 \*\/6 \* \* \*'/);
|
|
assert.match(workflow, /workflow_dispatch:/);
|
|
assert.doesNotMatch(workflow, /^\s*pull_request(_target)?:/m);
|
|
|
|
// Write-capable token (needed to close PRs from base context). Tolerant of
|
|
// intervening blank lines or additional permission keys.
|
|
assert.match(workflow, /permissions:\s+pull-requests:\s*write/);
|
|
|
|
// Identical maintainer carve-out to close-draft-prs.yml — a Set membership
|
|
// test over author_association, negated (no github.event.pull_request in a
|
|
// scheduled run).
|
|
assert.match(workflow, /new Set\(\['OWNER', 'MEMBER', 'COLLABORATOR'\]\)/);
|
|
assert.match(workflow, /!MAINTAINER_ASSOCIATIONS\.has\([^)]*\.author_association\)/);
|
|
|
|
// Paginates over open PRs and filters to drafts.
|
|
assert.match(workflow, /github\.paginate\(github\.rest\.pulls\.list/);
|
|
assert.match(workflow, /state:\s*'open'/);
|
|
assert.match(workflow, /pr\.draft === true/);
|
|
|
|
// Same user-facing policy message as close-draft-prs.yml (locks the core
|
|
// content so the sweep cannot silently drift to a weaker message).
|
|
assert.match(workflow, /## Draft PRs are not accepted/);
|
|
assert.match(workflow, /npm run test:coverage/);
|
|
assert.match(workflow, /CONTRIBUTING\.md#pull-request-guidelines/);
|
|
});
|
|
});
|
|
|
|
describe('Require Issue Link back-merge automation carve-out', () => {
|
|
test('the fail step is skipped for same-repo auto-backmerge PRs', () => {
|
|
const workflow = readWorkflow('.github/workflows/require-issue-link.yml');
|
|
|
|
// Auto-backmerge PRs (chore/backmerge-main-to-next-*) map to no issue, and a
|
|
// `Closes #N` would pollute the released CHANGELOG. The fail step must carve
|
|
// them out — keyed on the workflow-authored branch name AND same-repo
|
|
// identity so a fork PR cannot forge the exemption (#1389).
|
|
assert.match(
|
|
workflow,
|
|
/startsWith\(github\.head_ref, 'chore\/backmerge-main-to-next-'\)/
|
|
);
|
|
assert.match(
|
|
workflow,
|
|
/github\.event\.pull_request\.head\.repo\.full_name == github\.repository/
|
|
);
|
|
|
|
// The carve-out must live on the failing step's `if:` alongside the
|
|
// found=='false' check (step-level, so the required check still reports
|
|
// SUCCESS rather than a branch-protection-blocking "skipped").
|
|
assert.match(workflow, /steps\.check\.outputs\.found == 'false'/);
|
|
});
|
|
});
|
|
|
|
describe('Auto-backmerge needs_review version-manifest carve-out (#1404)', () => {
|
|
const workflow = readWorkflow('.github/workflows/auto-backmerge.yml');
|
|
|
|
test('all version-bearing manifests are filtered via version-only detection', () => {
|
|
// package.json / package-lock.json / plugin.json / marketplace.json
|
|
// diverge every release; a drop that is ONLY "version" lines must not park
|
|
// (parking is what lets the back-merge go stale). A substantive change
|
|
// still parks. The grep matches the indented "version": line for
|
|
// marketplace.json's plugins[0].version too. (#1404 / #1855)
|
|
// #1928: gemini-extension.json was removed with the sunset gemini runtime.
|
|
assert.ok(
|
|
workflow.includes("VERSION_STAMP_MANIFESTS='package.json package-lock.json .claude-plugin/plugin.json .claude-plugin/marketplace.json'"),
|
|
'auto-backmerge.yml must version-only-filter all version-bearing manifests (incl. marketplace.json #1855)'
|
|
);
|
|
assert.ok(
|
|
workflow.includes(`grep -vE '^[+-][[:space:]]*"version":'`),
|
|
'auto-backmerge.yml must filter version-only diffs via the "version" grep'
|
|
);
|
|
});
|
|
|
|
test('package-lock.json is NOT blindly excluded (lockfile-only changes still park)', () => {
|
|
// A lockfile-only substantive change (e.g. npm audit fix) rewrites
|
|
// resolved/integrity lines, so version-only filtering lets it through to
|
|
// review rather than dropping it silently. Guard against regression to a
|
|
// blanket exclude. (#1404)
|
|
assert.ok(
|
|
!workflow.includes(":(exclude)package-lock.json"),
|
|
'package-lock.json must not be globally excluded; rely on version-only filtering'
|
|
);
|
|
});
|
|
});
|