diff --git a/lib/prepare_security.js b/lib/prepare_security.js index 34ff3dac..ae9f6fe2 100644 --- a/lib/prepare_security.js +++ b/lib/prepare_security.js @@ -2,6 +2,7 @@ import fs from 'node:fs'; import path from 'node:path'; import auth from './auth.js'; import Request from './request.js'; +import { parsePRFromURL } from './links.js'; import { NEXT_SECURITY_RELEASE_BRANCH, NEXT_SECURITY_RELEASE_FOLDER, @@ -196,8 +197,7 @@ export default class PrepareSecurityRelease extends SecurityRelease { await this.closeAndRequestDisclosure(vulnerabilityJSON.reports); this.cli.info('Closing pull requests'); - // For now, close the ones with Security Release label - await this.closePRWithLabel('Security Release'); + await this.closePullRequests(vulnerabilityJSON); if (vulnerabilityJSON.buildIssue) { this.cli.info('Commenting on nodejs/build issue'); @@ -637,26 +637,25 @@ export default class PrepareSecurityRelease extends SecurityRelease { this.cli.stopSpinner('Done closing H1 Reports and requesting disclosure'); } - async closePRWithLabel(labels) { - if (typeof labels === 'string') { - labels = [labels]; - } - - const url = 'https://github.com/nodejs-private/node-private/pull'; + async closePullRequests(vulnerabilityJSON) { this.cli.startSpinner('Closing GitHub Pull Requests...'); - // At this point, GitHub does not provide filters through their REST API - const prs = await this.req.getPullRequest(url); - for (const pr of prs) { - if (pr.labels.some((l) => labels.includes(l.name))) { - this.cli.updateSpinner(`Closing Pull Request: ${pr.number}`); - await confirmSecurityStep( - this.cli, - `close GitHub pull request \`nodejs-private/node-private#${pr.number}\``, - 'This closes a pull request labeled for the security release.' - ); - await this.req.closePullRequest(pr.number, - { owner: 'nodejs-private', repo: 'node-private' }); - } + const entries = [ + ...vulnerabilityJSON.reports, + ...Object.values(vulnerabilityJSON.dependencies ?? {}) + ]; + const urls = new Set(entries.flatMap( + ({ affectedVersions }) => Object.values(affectedVersions))); + + for (const url of urls) { + const { owner, repo, prid } = parsePRFromURL(url); + const name = `${owner}/${repo}#${prid}`; + this.cli.updateSpinner(`Closing Pull Request: ${name}`); + await confirmSecurityStep( + this.cli, + `close GitHub pull request \`${name}\``, + 'This closes a pull request listed in vulnerabilities.json.' + ); + await this.req.closePullRequest(prid, { owner, repo }); } this.cli.stopSpinner('Closed GitHub Pull Requests.'); } diff --git a/lib/request.js b/lib/request.js index f7396690..0bafd73f 100644 --- a/lib/request.js +++ b/lib/request.js @@ -158,7 +158,7 @@ export default class Request { async closePullRequest(id, { owner, repo }) { const url = `/repos/${owner}/${repo}/pulls/${id}`; const options = { - method: 'POST', + method: 'PATCH', headers: { 'Content-Type': 'application/json' }, diff --git a/test/unit/request.test.js b/test/unit/request.test.js index 349bb61c..d4d637c2 100644 --- a/test/unit/request.test.js +++ b/test/unit/request.test.js @@ -12,6 +12,25 @@ function createRequest(response) { } describe('Request', () => { + describe('closePullRequest', () => { + it('updates the pull request state with PATCH', async() => { + const request = createRequest({}); + let requestOptions; + request.json = async(url, options) => { + assert.strictEqual(url, '/repos/nodejs/node/pulls/123'); + requestOptions = options; + return {}; + }; + + await request.closePullRequest(123, { owner: 'nodejs', repo: 'node' }); + + assert.strictEqual(requestOptions.method, 'PATCH'); + assert.deepStrictEqual(JSON.parse(requestOptions.body), { + state: 'closed' + }); + }); + }); + describe('query', () => { it('preserves detailed GraphQL errors', async() => { const variables = { owner: 'nodejs', repo: 'node', prid: 65130 }; diff --git a/test/unit/security_release.test.js b/test/unit/security_release.test.js index cd34fede..9947d9c8 100644 --- a/test/unit/security_release.test.js +++ b/test/unit/security_release.test.js @@ -16,6 +16,55 @@ import { getHighestSeverityAnnouncement } from '../../lib/security-release/security-release.js'; +describe('security_release: cleanup pull requests', () => { + const vulnerabilities = { + reports: [ + { + affectedVersions: { + main: 'https://github.com/nodejs-private/node-private/pull/2', + '24.x': 'https://github.com/nodejs-private/node-private/pull/3', + '22.x': 'https://github.com/nodejs-private/node-private/pull/3' + } + } + ], + dependencies: { + undici: { + affectedVersions: { + main: 'https://github.com/nodejs/node/pull/5' + } + } + } + }; + + it('closes each PR from affectedVersions once', async() => { + const prompts = []; + const closed = []; + const release = new PrepareSecurityRelease({ + startSpinner() {}, + updateSpinner() {}, + stopSpinner() {}, + prompt(message) { + prompts.push(message); + return true; + } + }); + release.req = { + closePullRequest(prid, repository) { + closed.push({ prid, ...repository }); + } + }; + + await release.closePullRequests(vulnerabilities); + + assert.deepStrictEqual(closed, [ + { owner: 'nodejs-private', repo: 'node-private', prid: 2 }, + { owner: 'nodejs-private', repo: 'node-private', prid: 3 }, + { owner: 'nodejs', repo: 'node', prid: 5 } + ]); + assert.strictEqual(prompts.length, closed.length); + }); +}); + function report(id, rating, affectedVersions = ['24.x']) { return { id,