diff --git a/components/git/release.js b/components/git/release.js index 3678dd54..c5b98c0e 100644 --- a/components/git/release.js +++ b/components/git/release.js @@ -14,10 +14,6 @@ const PROMOTE = 'promote'; const RELEASERS = 'releasers'; const releaseOptions = { - filterLabel: { - describe: 'Labels separated by "," to filter security PRs', - type: 'string' - }, 'gpg-sign': { describe: 'GPG-sign commits, will be passed to the git process', alias: 'S' @@ -51,7 +47,9 @@ const releaseOptions = { }, security: { describe: 'Demarcate the new security release as a security release. ' + - 'Optionally provide path to security-release repository for CVE auto-population', + 'Optionally provide the path to vulnerabilities.json (or the ' + + 'security-release repository) to cherry-pick the reports and ' + + 'dependency updates for this release line and auto-populate CVEs', type: 'string', coerce: (arg) => { // If --security=path is used, return the path diff --git a/components/git/security.js b/components/git/security.js index 769fdce3..62ae73c6 100644 --- a/components/git/security.js +++ b/components/git/security.js @@ -12,6 +12,7 @@ import SecurityBlog from '../../lib/security_blog.js'; import SecurityAnnouncement from '../../lib/security-announcement.js'; import { forceRunAsync } from '../../lib/run.js'; import { getAffectedVersionLines } from '../../lib/security-release/security-release.js'; +import { getPullRequestURLForLine } from '../../lib/prepare_release.js'; export const command = 'security [options]'; export const describe = 'Manage an in-progress security release or start a new one.'; @@ -305,9 +306,12 @@ async function applySecurityPatches(cli) { const { releaseDate, reports, dependencies } = await fetchVulnerabilitiesDotJSON(cli, req); let patchedVersion; - for (const { affectedVersions, prURL, title } of Object.values(dependencies).flat()) { - if (!getAffectedVersionLines(affectedVersions).includes(`${nodeMajorVersion}.x`)) continue; - cli.separator(`Taking care of ${title}...`); + const releaseLine = `${nodeMajorVersion}.x`; + for (const [name, dep] of Object.entries(dependencies ?? {})) { + const prURL = getPullRequestURLForLine( + dep.affectedVersions, releaseLine, dep.prURL); + if (!prURL) continue; + cli.separator(`Taking care of ${name}...`); if (await skipIfExisting(cli, prURL)) continue; const argv = parsePRFromURL(prURL); diff --git a/lib/prepare_release.js b/lib/prepare_release.js index e0d989e8..7089924a 100644 --- a/lib/prepare_release.js +++ b/lib/prepare_release.js @@ -6,8 +6,6 @@ import { replaceInFile } from 'replace-in-file'; import { forceRunAsync, runAsync, runSync } from './run.js'; import { writeJson, readJson } from './file.js'; -import Request from './request.js'; -import auth from './auth.js'; import { getEOLDate, getStartLTSBlurb, @@ -15,9 +13,41 @@ import { } from './release/utils.js'; import CherryPick from './cherry_pick.js'; import Session from './session.js'; +import { getAffectedVersionLines } from './security-release/security-release.js'; const isWindows = process.platform === 'win32'; +// Parse a GitHub Pull Request URL into its owner, repo and number. +// Returns null when the URL does not look like a PR URL. +export function parsePullRequestURL(url) { + const match = /github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/.exec(url || ''); + if (!match) return null; + return { owner: match[1], repo: match[2], number: Number(match[3]) }; +} + +// Resolve the PR URL that fixes a given release line from a report or +// dependency `affectedVersions` entry. +// Supports the current schema, where `affectedVersions` maps a release line to +// its PR URL (e.g. `{ '22.x': 'https://.../pull/1' }`), and the legacy schema, +// where `affectedVersions` is an array of lines paired with a single `prURL`. +export function getPullRequestURLForLine(affectedVersions, line, legacyPrURL) { + if ( + affectedVersions && + typeof affectedVersions === 'object' && + !Array.isArray(affectedVersions) + ) { + return affectedVersions[line] || null; + } + + if (Array.isArray(affectedVersions) && legacyPrURL) { + return getAffectedVersionLines(affectedVersions).includes(line) + ? legacyPrURL + : null; + } + + return null; +} + export default class ReleasePreparation extends Session { constructor(argv, cli, dir) { super(cli, dir); @@ -32,7 +62,6 @@ export default class ReleasePreparation extends Session { this.defaultReleaseDate = argv.releaseDate ?? new Date().toISOString().slice(0, 10); this.ltsCodename = ''; this.date = ''; - this.filterLabels = argv.filterLabel && argv.filterLabel.split(','); this.newVersion = argv.newVersion; } @@ -40,93 +69,130 @@ export default class ReleasePreparation extends Session { return this.isSecurityRelease ? this.releaseBranch : this.stagingBranch; } - warnForNonMergeablePR(pr) { + // Resolve the path to the security release vulnerabilities.json. + // `--security` may point either directly at a vulnerabilities.json file or at + // the root of the security-release repository, in which case the well-known + // sub-path is appended. + getVulnerabilitiesJSONPath() { + if (!this.securityReleaseRepo) return null; + if (this.securityReleaseRepo.endsWith('.json')) { + return this.securityReleaseRepo; + } + return path.join( + this.securityReleaseRepo, + 'security-release', + 'next-security-release', + 'vulnerabilities.json' + ); + } + + loadVulnerabilitiesJSON() { const { cli } = this; + const vulnPath = this.getVulnerabilitiesJSONPath(); - cli.warn(`PR#${pr.number} - ${pr.title} is not 'MERGEABLE'. - Status: ${pr.mergeable}`); + if (!vulnPath) { + cli.error('No vulnerabilities.json path provided. Pass the path to ' + + 'vulnerabilities.json (or the security-release repository) via ' + + '`--security=`.'); + return null; + } + + if (!existsSync(vulnPath)) { + cli.error(`vulnerabilities.json not found at ${vulnPath}.`); + return null; + } + + try { + cli.startSpinner(`Reading vulnerabilities.json from ${vulnPath}..`); + const vulnData = JSON.parse(readFileSync(vulnPath, 'utf-8')); + cli.stopSpinner(`Done reading vulnerabilities.json from ${vulnPath}`); + return vulnData; + } catch (err) { + cli.error(`Failed to read vulnerabilities.json: ${err.message}`); + return null; + } } - async getOpenPRs(filterLabels) { - const credentials = await auth({ github: true }); - const request = new Request(credentials); - const data = await request.gql('PRs', { - owner: this.owner, - repo: this.repo, - labels: filterLabels - }); - return data.repository.pullRequests.nodes; + // Build the list of PRs to cherry-pick for a given release line from both the + // reports and the dependency updates, using their `affectedVersions` mapping. + getSecurityCherryPickTargets(vulnData, line) { + const targets = []; + + for (const report of vulnData.reports ?? []) { + const url = getPullRequestURLForLine( + report.affectedVersions, line, report.prURL); + if (url) { + targets.push({ + url, + cveIds: report.cveIds?.length ? report.cveIds : null, + label: report.title || report.id + }); + } + } + + for (const [name, dep] of Object.entries(vulnData.dependencies ?? {})) { + const url = getPullRequestURLForLine( + dep.affectedVersions, line, dep.prURL); + if (url) { + targets.push({ url, cveIds: null, label: `dependency: ${name}` }); + } + } + + return targets; + } + + upstreamForPR({ owner, repo }) { + if (owner === 'nodejs-private') { + return `https://${this.username}:${this.config.token}` + + `@github.com/${owner}/${repo}.git`; + } + return `https://github.com/${owner}/${repo}.git`; } - async cherryPickSecurityPRs(filterLabels) { + async cherryPickSecurityPRs() { const { cli } = this; + const line = `${this.versionComponents.major}.x`; - const prs = await this.getOpenPRs(filterLabels); - if (prs.length === 0) { - cli.warn(`There are no PRs available in ${this.owner}/${this.repo}`); + const vulnData = this.loadVulnerabilitiesJSON(); + if (!vulnData) { + cli.error('Cannot proceed with a security release without a valid ' + + 'vulnerabilities.json.'); return false; } - const vulnCveMap = new Map(); - if (this.isSecurityRelease && this.securityReleaseRepo) { - const vulnPath = path.join( - this.securityReleaseRepo, - 'security-release', - 'next-security-release', - 'vulnerabilities.json' - ); - - if (!existsSync(vulnPath)) { - cli.error(`vulnerabilities.json not found at ${vulnPath}. ` + - 'Skipping CVE auto-population.'); - cli.warn('PRs will require manual CVE-ID entry.'); - } else { - try { - cli.startSpinner(`Reading vulnerabilities.json from ${vulnPath}..`); - const vulnData = JSON.parse(readFileSync(vulnPath, 'utf-8')); - cli.stopSpinner(`Done reading vulnerabilities.json from ${vulnPath}`); - - if (vulnData.reports && Array.isArray(vulnData.reports)) { - vulnData.reports.forEach(report => { - if (report.prURL && report.cveIds && report.cveIds.length > 0) { - vulnCveMap.set(report.prURL, report.cveIds); - } - }); - } - cli.ok(`Loaded ${vulnCveMap.size} CVE mappings from vulnerabilities.json`); - } catch (err) { - cli.error(`Failed to read vulnerabilities.json: ${err.message}`); - cli.warn('Continuing without CVE auto-population.'); - } - } + const targets = this.getSecurityCherryPickTargets(vulnData, line); + if (targets.length === 0) { + cli.warn(`No PRs found in vulnerabilities.json for release line ${line}.`); + return false; } - for (const pr of prs) { - if (pr.mergeable !== 'MERGEABLE') { - this.warnForNonMergeablePR(pr); - } + cli.info(`Cherry-picking ${targets.length} PR(s) for ${line} from ` + + 'vulnerabilities.json'); - // Look up CVE-IDs from vulnerabilities.json - const prUrl = `https://github.com/${this.owner}/${this.repo}/pull/${pr.number}`; - const cveIds = vulnCveMap.get(prUrl); + for (const target of targets) { + const pr = parsePullRequestURL(target.url); + if (!pr) { + cli.warn(`Skipping invalid PR URL for ${target.label}: ${target.url}`); + continue; + } - if (!cveIds || cveIds.length === 0) { - cli.warn(`No CVE-IDs found in vulnerabilities.json for ${prUrl}`); + if (!target.cveIds) { + cli.warn(`No CVE-IDs found in vulnerabilities.json for ${target.url}`); } + cli.info(`Cherry-picking ${target.label} (${target.url})`); const cp = new CherryPick(pr.number, this.dir, cli, { - owner: this.owner, - repo: this.repo, + owner: pr.owner, + repo: pr.repo, gpgSign: this.gpgSign, - upstream: this.isSecurityRelease ? `https://${this.username}:${this.config.token}@github.com/${this.owner}/${this.repo}.git` : this.upstream, + upstream: this.upstreamForPR(pr), lint: false, includeCVE: true, - cveIds: cveIds || null, - vulnCveMap + cveIds: target.cveIds }); const success = await cp.start(); if (!success) { - cli.warn('The cherry-pick has failed. PR-ID: ' + pr.number); + cli.warn(`The cherry-pick has failed. PR: ${target.url}`); return false; } } @@ -147,7 +213,7 @@ export default class ReleasePreparation extends Session { { defaultAnswer: this.runBranchDiff }); if (runBranchDiff) { if (isSecurityRelease) { - const success = await this.cherryPickSecurityPRs(this.filterLabels); + const success = await this.cherryPickSecurityPRs(); if (!success) { cli.error('Aborting security release preparation.'); return; diff --git a/lib/prepare_security.js b/lib/prepare_security.js index 94344dc8..0917a86c 100644 --- a/lib/prepare_security.js +++ b/lib/prepare_security.js @@ -17,7 +17,6 @@ import { writeSecurityFile, SecurityRelease } from './security-release/security-release.js'; -import _ from 'lodash'; function relativeDate(date) { const days = Math.floor((Date.now() - date) / (1000 * 60 * 60 * 24)); @@ -261,8 +260,7 @@ export default class PrepareSecurityRelease extends SecurityRelease { const reports = reportSelectionMode === 'include-all' ? await this.includeAllTriagedReports(excludedReports) : await this.chooseReports(excludedReports); - const depUpdates = await this.getDependencyUpdates(); - const deps = _.groupBy(depUpdates, 'name'); + const deps = await this.getDependencyUpdates(); // create the vulnerabilities.json file in the security-release repo const filePath = await this.createVulnerabilitiesJSON(reports, deps, releaseDate); @@ -558,8 +556,11 @@ export default class PrepareSecurityRelease extends SecurityRelease { process.exit(1); } + // Collect dependency updates using the same schema as the reports: each + // dependency maps every affected release line to the PR that lands it, e.g. + // { undici: { affectedVersions: { '22.x': '', '24.x': '' } } } async getDependencyUpdates() { - const deps = []; + const deps = {}; const updates = await this.cli.prompt('Are there dependency updates in this security release?', { defaultAnswer: true, @@ -583,20 +584,24 @@ export default class PrepareSecurityRelease extends SecurityRelease { }); const versions = await this.cli.prompt( - 'Which release line does this dependency update affect?', { + 'Which release line(s) does this dependency update PR affect? ' + + '(comma-separated)', { defaultAnswer: supportedVersions, questionType: 'input' }); try { const res = await this.req.getPullRequest(dep); - const { html_url, title } = res; - deps.push({ - name, - url: html_url, - title, - affectedVersions: versions.split(',').map((v) => v.replace('v', '').trim()) - }); + const { html_url: prURL } = res; + const affectedVersions = versions + .split(',') + .map((v) => v.replace('v', '').trim()) + .filter(Boolean); + + deps[name] ??= { affectedVersions: {} }; + for (const line of affectedVersions) { + deps[name].affectedVersions[line] = prURL; + } this.cli.separator(); } catch (error) { this.cli.error('Invalid PR url. Please provide a valid PR url.'); diff --git a/lib/security_blog.js b/lib/security_blog.js index 15e4a724..d6bbcda9 100644 --- a/lib/security_blog.js +++ b/lib/security_blog.js @@ -287,9 +287,13 @@ export default class SecurityBlog extends SecurityRelease { if (Object.keys(dependencyUpdates).length === 0) return ''; let template = '\nThis security release includes the following dependency' + ' updates to address public vulnerabilities:\n'; - for (const [dependency, { versions, affectedVersions }] of Object.entries(dependencyUpdates)) { - const releaseLines = getAffectedVersionLines(affectedVersions); - template += `- ${dependency} (${versions.join(', ')}) on ${releaseLines.join(', ')}\n`; + for (const [dependency, entry] of Object.entries(dependencyUpdates)) { + const releaseLines = getAffectedVersionLines(entry.affectedVersions); + const versions = Array.isArray(entry.versions) + ? entry.versions.map((v) => (typeof v === 'string' ? v : v.version)) + : []; + const versionSuffix = versions.length ? ` (${versions.join(', ')})` : ''; + template += `- ${dependency}${versionSuffix} on ${releaseLines.join(', ')}\n`; } return template; } diff --git a/test/unit/prepare_release.test.js b/test/unit/prepare_release.test.js index 1e14b889..594a60b7 100644 --- a/test/unit/prepare_release.test.js +++ b/test/unit/prepare_release.test.js @@ -3,6 +3,10 @@ import assert from 'node:assert'; import { readFileSync } from 'node:fs'; import * as utils from '../../lib/release/utils.js'; +import { + parsePullRequestURL, + getPullRequestURLForLine +} from '../../lib/prepare_release.js'; describe('prepare_release: utils.getEOLDate', () => { it('calculates the correct EOL date', () => { @@ -68,3 +72,67 @@ describe('prepare_release: utils.updateTestProcessRelease', () => { assert.strictEqual(updated, expected); }); }); + +describe('prepare_release: parsePullRequestURL', () => { + it('parses a private node-private PR URL', () => { + assert.deepStrictEqual( + parsePullRequestURL('https://github.com/nodejs-private/node-private/pull/927'), + { owner: 'nodejs-private', repo: 'node-private', number: 927 } + ); + }); + + it('parses a public nodejs/node PR URL', () => { + assert.deepStrictEqual( + parsePullRequestURL('https://github.com/nodejs/node/pull/63752'), + { owner: 'nodejs', repo: 'node', number: 63752 } + ); + }); + + it('returns null for empty or invalid URLs', () => { + assert.strictEqual(parsePullRequestURL(''), null); + assert.strictEqual(parsePullRequestURL(undefined), null); + assert.strictEqual(parsePullRequestURL('https://github.com/nodejs/node'), null); + }); +}); + +describe('prepare_release: getPullRequestURLForLine', () => { + const affectedVersions = { + main: 'https://github.com/nodejs-private/node-private/pull/924', + '26.x': 'https://github.com/nodejs-private/node-private/pull/924', + '24.x': 'https://github.com/nodejs-private/node-private/pull/927', + '22.x': 'https://github.com/nodejs-private/node-private/pull/927' + }; + + it('resolves the PR URL for the requested line (object schema)', () => { + assert.strictEqual( + getPullRequestURLForLine(affectedVersions, '22.x'), + 'https://github.com/nodejs-private/node-private/pull/927' + ); + assert.strictEqual( + getPullRequestURLForLine(affectedVersions, '26.x'), + 'https://github.com/nodejs-private/node-private/pull/924' + ); + }); + + it('returns null when the line is absent or empty', () => { + assert.strictEqual(getPullRequestURLForLine(affectedVersions, '20.x'), null); + assert.strictEqual(getPullRequestURLForLine({ '22.x': '' }, '22.x'), null); + }); + + it('falls back to the legacy array schema paired with prURL', () => { + const legacyPrURL = 'https://github.com/nodejs-private/node-private/pull/900'; + assert.strictEqual( + getPullRequestURLForLine(['22.x', '24.x'], '22.x', legacyPrURL), + legacyPrURL + ); + assert.strictEqual( + getPullRequestURLForLine(['24.x'], '22.x', legacyPrURL), + null + ); + }); + + it('returns null for missing affectedVersions', () => { + assert.strictEqual(getPullRequestURLForLine(undefined, '22.x'), null); + assert.strictEqual(getPullRequestURLForLine(null, '22.x'), null); + }); +}); diff --git a/test/unit/security_release.test.js b/test/unit/security_release.test.js index 686d9cfd..23f7748d 100644 --- a/test/unit/security_release.test.js +++ b/test/unit/security_release.test.js @@ -817,3 +817,36 @@ describe('security_blog: post-release severity wording', () => { assert.throws(() => blog.getVulnerabilities(content), /severity\.rating not found for report 1/); }); }); + +describe('security_blog: getDependencyUpdatesTemplate', () => { + it('renders release lines from the reports-style affectedVersions object', () => { + const blog = new SecurityBlog(); + const template = blog.getDependencyUpdatesTemplate({ + undici: { + affectedVersions: { + '24.x': 'https://github.com/nodejs-private/node-private/pull/922', + '22.x': 'https://github.com/nodejs-private/node-private/pull/932' + } + } + }); + + assert.match(template, /- undici on 24\.x, 22\.x/); + }); + + it('keeps rendering versions from the legacy schema', () => { + const blog = new SecurityBlog(); + const template = blog.getDependencyUpdatesTemplate({ + undici: { + versions: ['8.5.0'], + affectedVersions: ['22.x', '24.x'] + } + }); + + assert.match(template, /- undici \(8\.5\.0\) on 22\.x, 24\.x/); + }); + + it('returns an empty string when there are no dependency updates', () => { + const blog = new SecurityBlog(); + assert.strictEqual(blog.getDependencyUpdatesTemplate({}), ''); + }); +});