chore(#2331): trigger PR-policy workflows on pull_request_target so fork PRs get the verdict (#2333)
* 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>
This commit is contained in:
75
.github/workflows/pr-target-validator.yml
vendored
75
.github/workflows/pr-target-validator.yml
vendored
@@ -9,14 +9,29 @@ name: PR Target Validator
|
||||
#
|
||||
# See: docs/branching.md, docs/adr/230-introduce-next-integration-branch.md
|
||||
|
||||
# Trigger (#2331): pull_request_target, NOT pull_request. This job runs ONLY for
|
||||
# non-OWNER/MEMBER/COLLABORATOR authors (see the `if:` below) — i.e. exactly the
|
||||
# fork PRs whose `pull_request` GITHUB_TOKEN is downgraded to read-only
|
||||
# regardless of the `permissions:` block. On that trigger the sticky-comment
|
||||
# call below 403s, the unhandled rejection kills the step, and core.setFailed
|
||||
# never runs: the author sees an API stack trace instead of "retarget to next".
|
||||
# pull_request_target runs in the base-repo context with a write-capable token.
|
||||
# Safe here (as in pr-template-format.yml): the only checkout is the BASE branch
|
||||
# with persist-credentials: false, and the PR-controlled inputs (pr.base.ref /
|
||||
# pr.head.ref) are read as data. No head code executes, so the "PR cannot edit
|
||||
# the policy that judges it" property below is reinforced, not weakened.
|
||||
on:
|
||||
pull_request:
|
||||
pull_request_target:
|
||||
types: [opened, edited, reopened, synchronize]
|
||||
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
|
||||
cancel-in-progress: true
|
||||
|
||||
# Scope unchanged by #2331 — see the note in pr-title-validator.yml. This
|
||||
# workflow's `pull-requests: write` alone already authorizes the comment call
|
||||
# (its sticky comment has posted 8 times on that scope); the 403 was the fork
|
||||
# token downgrade, not a missing scope.
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: write
|
||||
@@ -70,10 +85,21 @@ jobs:
|
||||
}
|
||||
|
||||
// decision === 'blocked': base is main and head is not an allowed pattern.
|
||||
//
|
||||
// #2331: `head` is attacker-controlled (a fork author names their own
|
||||
// branch) and the comment below is posted by github-actions[bot] with a
|
||||
// write token. A backtick IS legal in a git ref name, so echoed raw into
|
||||
// an inline-code span it closes the span early and the remainder renders
|
||||
// as live Markdown. This is weaker than the pr-title-validator case —
|
||||
// check-ref-format forbids space, ':', '[' and '*', so no bare URL, link
|
||||
// or emphasis is expressible in a branch name — but it is the same class
|
||||
// and is stripped identically rather than left to the charset to police.
|
||||
const headForMarkdown = String(head).replace(/`/g, "'");
|
||||
|
||||
const msg = [
|
||||
`### Wrong target branch`,
|
||||
``,
|
||||
`This PR targets \`main\` but the source branch \`${head}\` is not a release, hotfix, critical-fix, or back-merge branch.`,
|
||||
`This PR targets \`main\` but the source branch \`${headForMarkdown}\` is not a release, hotfix, critical-fix, or back-merge branch.`,
|
||||
``,
|
||||
`**Most PRs should target \`next\`, not \`main\`.** See [docs/branching.md](../blob/main/docs/branching.md).`,
|
||||
``,
|
||||
@@ -91,28 +117,37 @@ jobs:
|
||||
].join('\n');
|
||||
|
||||
// Post or update a sticky comment.
|
||||
const { data: comments } = await github.rest.issues.listComments({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
});
|
||||
const marker = '<!-- pr-target-validator -->';
|
||||
const existing = comments.find(c => c.body && c.body.includes(marker));
|
||||
const body = `${marker}\n${msg}`;
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
//
|
||||
// #2331: the comment is a COURTESY, the verdict below is the GATE.
|
||||
// A failure here must never suppress the verdict. pull_request_target
|
||||
// should make the 403 impossible; this catch is defense in depth so a
|
||||
// future permission change degrades the diagnostic, not the gate.
|
||||
try {
|
||||
const { data: comments } = await github.rest.issues.listComments({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
body,
|
||||
});
|
||||
const marker = '<!-- pr-target-validator -->';
|
||||
const existing = comments.find(c => c.body && c.body.includes(marker));
|
||||
const body = `${marker}\n${msg}`;
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
body,
|
||||
});
|
||||
}
|
||||
} catch (err) {
|
||||
core.warning(`Could not post the PR-target comment (${err.status || err.message}). Guidance follows:\n${msg}`);
|
||||
}
|
||||
|
||||
if (warnOnly) {
|
||||
|
||||
89
.github/workflows/pr-title-validator.yml
vendored
89
.github/workflows/pr-title-validator.yml
vendored
@@ -23,6 +23,18 @@ name: PR Title Validator
|
||||
# matcher lands on the base branch it does not exist there — the introducing
|
||||
# PR is skipped (bootstrap); every PR after merge is fully gated.
|
||||
#
|
||||
# Trigger (#2331): pull_request_target, NOT pull_request. A `pull_request`
|
||||
# event raised from a fork hands the job a read-only GITHUB_TOKEN regardless of
|
||||
# the `permissions:` block below, so the sticky-comment call 403s, the
|
||||
# unhandled rejection kills this step, and core.setFailed never runs — the
|
||||
# contributor sees an API stack trace instead of the retitle instructions.
|
||||
# pull_request_target runs in the base-repo context with a write-capable token.
|
||||
# This is safe here for the same reason it is safe in pr-template-format.yml:
|
||||
# the only checkout is the BASE branch (persist-credentials: false) and the
|
||||
# only PR-controlled input is `pr.title`, read as data. No head code executes.
|
||||
# The trust boundary above is reinforced, not weakened — pull_request_target
|
||||
# checks out base by definition.
|
||||
#
|
||||
# Unlike pr-target-validator, this runs for ALL authors (including members):
|
||||
# the changelog drift that motivated #1549 came from member PRs.
|
||||
#
|
||||
@@ -32,13 +44,22 @@ name: PR Title Validator
|
||||
# See: scripts/release-notes/conventional-title.cjs, CONTRIBUTING.md, issue #1549.
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
pull_request_target:
|
||||
types: [opened, edited, reopened, synchronize]
|
||||
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
|
||||
cancel-in-progress: true
|
||||
|
||||
# Scope unchanged by #2331 — deliberately. `pull-requests: write` alone already
|
||||
# authorizes github.rest.issues.createComment on a PR (GitHub accepts EITHER
|
||||
# `issues` or `pull-requests` write for the issue-comments endpoint when the
|
||||
# target is a PR). Verified against this repo's own history rather than the
|
||||
# docs: this workflow has only ever declared `pull-requests: write` and its
|
||||
# sticky comment has posted 26 times; require-issue-link.yml declares only
|
||||
# `issues: write` and its comment posts too. The 403 was the fork token
|
||||
# downgrade, NOT a missing scope — so widening the scope here would add
|
||||
# privilege on a pull_request_target workflow while fixing nothing.
|
||||
permissions:
|
||||
contents: read
|
||||
pull-requests: write
|
||||
@@ -92,10 +113,24 @@ jobs:
|
||||
return;
|
||||
}
|
||||
|
||||
// #2331: `title` is attacker-controlled free text (a PR title has no
|
||||
// charset restriction) and the comment below is posted by
|
||||
// github-actions[bot] with a write token. Echoed raw into an inline-code
|
||||
// span, a single backtick in the title 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 the PR thread (phishing that borrows the bot's credibility).
|
||||
// Stripping the backtick is sufficient and complete: it is the only
|
||||
// character that can break out of an inline-code span, and everything
|
||||
// else is inert once it cannot. Only the RENDERED body needs this;
|
||||
// core.warning/core.setFailed below go to the job log, not Markdown,
|
||||
// and @actions/core already escapes workflow-command sequences there.
|
||||
const titleForMarkdown = String(title).replace(/`/g, "'");
|
||||
|
||||
const msg = [
|
||||
`### PR title needs the issue-ref convention`,
|
||||
``,
|
||||
`\`${title}\``,
|
||||
`\`${titleForMarkdown}\``,
|
||||
``,
|
||||
result.message,
|
||||
``,
|
||||
@@ -113,28 +148,42 @@ jobs:
|
||||
].join('\n');
|
||||
|
||||
// Post or update a sticky comment.
|
||||
const { data: comments } = await github.rest.issues.listComments({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
});
|
||||
const marker = '<!-- pr-title-validator -->';
|
||||
const existing = comments.find(c => c.body && c.body.includes(marker));
|
||||
const body = `${marker}\n${msg}`;
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
//
|
||||
// #2331: the comment is a COURTESY, the verdict below is the GATE.
|
||||
// Never let a failure here suppress the verdict — that inversion is
|
||||
// the bug this guard exists to prevent (a 403 on the createComment
|
||||
// call used to kill the step before core.setFailed ran, replacing
|
||||
// "retitle as type(#issue): summary" with an API stack trace).
|
||||
// pull_request_target should make the 403 impossible; this catch is
|
||||
// defense in depth so a future permission change degrades the
|
||||
// diagnostic rather than the gate.
|
||||
try {
|
||||
const { data: comments } = await github.rest.issues.listComments({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
body,
|
||||
});
|
||||
const marker = '<!-- pr-title-validator -->';
|
||||
const existing = comments.find(c => c.body && c.body.includes(marker));
|
||||
const body = `${marker}\n${msg}`;
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: pr.number,
|
||||
body,
|
||||
});
|
||||
}
|
||||
} catch (err) {
|
||||
// Surface the guidance in the job log so it is not lost entirely.
|
||||
core.warning(`Could not post the PR-title comment (${err.status || err.message}). Guidance follows:\n${msg}`);
|
||||
}
|
||||
|
||||
if (warnOnly) {
|
||||
|
||||
59
.github/workflows/require-issue-link.yml
vendored
59
.github/workflows/require-issue-link.yml
vendored
@@ -1,13 +1,28 @@
|
||||
name: Require Issue Link
|
||||
|
||||
# Trigger (#2331): pull_request_target, NOT pull_request. A `pull_request` event
|
||||
# raised from a fork hands the job a read-only GITHUB_TOKEN regardless of the
|
||||
# `permissions:` block, so the createComment call below 403s, the unhandled
|
||||
# rejection kills the step, and the core.setFailed on the last line never runs —
|
||||
# the contributor sees an API stack trace instead of "add Closes #NNN".
|
||||
# pull_request_target runs in the base-repo context with a write-capable token.
|
||||
# Safe here: this job performs NO checkout at all and reads the PR body only via
|
||||
# an env var (never interpolated into a shell), so no head code executes. The
|
||||
# #1389 fork-forgery carve-out below still holds — it is keyed on
|
||||
# head.repo.full_name == github.repository, not on the branch name alone.
|
||||
on:
|
||||
pull_request:
|
||||
pull_request_target:
|
||||
types: [opened, edited, reopened, synchronize]
|
||||
|
||||
concurrency:
|
||||
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
|
||||
cancel-in-progress: true
|
||||
|
||||
# Scope unchanged by #2331 — see the note in pr-title-validator.yml. `issues:
|
||||
# write` alone already authorizes this workflow's issues.createComment call on a
|
||||
# PR: its sticky comment has posted on same-repo PRs (#106, #164, #232, #259) on
|
||||
# exactly this scope. The 403 was the fork token downgrade, not a missing scope,
|
||||
# so no `pull-requests: write` is added — this job never calls a pulls.* API.
|
||||
permissions:
|
||||
issues: write
|
||||
|
||||
@@ -60,26 +75,34 @@ jobs:
|
||||
'',
|
||||
'Edit the PR description to add a valid `Closes #NNN`, `Fixes #NNN`, or `Resolves #NNN` line. This check will re-evaluate on the next PR update.',
|
||||
].join('\n');
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: prNumber,
|
||||
per_page: 100,
|
||||
});
|
||||
const existing = comments.find(comment => comment.body && comment.body.includes(marker));
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
// #2331: the comment is a COURTESY, the setFailed below is the GATE.
|
||||
// A failure here must never suppress the verdict. pull_request_target
|
||||
// should make the 403 impossible; this catch is defense in depth so a
|
||||
// future permission change degrades the diagnostic, not the gate.
|
||||
try {
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: prNumber,
|
||||
body,
|
||||
per_page: 100,
|
||||
});
|
||||
const existing = comments.find(comment => comment.body && comment.body.includes(marker));
|
||||
if (existing) {
|
||||
await github.rest.issues.updateComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
comment_id: existing.id,
|
||||
body,
|
||||
});
|
||||
} else {
|
||||
await github.rest.issues.createComment({
|
||||
owner: context.repo.owner,
|
||||
repo: context.repo.repo,
|
||||
issue_number: prNumber,
|
||||
body,
|
||||
});
|
||||
}
|
||||
} catch (err) {
|
||||
core.warning(`Could not post the missing-issue-link comment (${err.status || err.message}). Guidance follows:\n${body}`);
|
||||
}
|
||||
core.setFailed('PR body must contain a closing issue reference (e.g. "Closes #123").');
|
||||
|
||||
@@ -14,6 +14,34 @@ 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),
|
||||
@@ -47,6 +75,158 @@ describe('PR policy workflow maintainer carve-outs', () => {
|
||||
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');
|
||||
|
||||
|
||||
Reference in New Issue
Block a user