diff --git a/.github/workflows/pr-target-validator.yml b/.github/workflows/pr-target-validator.yml index a1bdddd2d..d22cef1f0 100644 --- a/.github/workflows/pr-target-validator.yml +++ b/.github/workflows/pr-target-validator.yml @@ -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 = ''; - 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 = ''; + 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) { diff --git a/.github/workflows/pr-title-validator.yml b/.github/workflows/pr-title-validator.yml index 008fded5b..d89cb2045 100644 --- a/.github/workflows/pr-title-validator.yml +++ b/.github/workflows/pr-title-validator.yml @@ -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 = ''; - 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 = ''; + 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) { diff --git a/.github/workflows/require-issue-link.yml b/.github/workflows/require-issue-link.yml index 146f5e9f1..5f6d0ec1c 100644 --- a/.github/workflows/require-issue-link.yml +++ b/.github/workflows/require-issue-link.yml @@ -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").'); diff --git a/tests/workflow-maintainer-skip.test.cjs b/tests/workflow-maintainer-skip.test.cjs index fef6b6e00..c75f0eafd 100644 --- a/tests/workflow-maintainer-skip.test.cjs +++ b/tests/workflow-maintainer-skip.test.cjs @@ -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');