Skip to content

Commit e828716

Browse files
RafaelGSSRafaelGSS
committed
fix(git-node): fix security release preparation
Signed-off-by: RafaelGSS <[email protected]>
1 parent c263c2a commit e828716

6 files changed

Lines changed: 146 additions & 31 deletions

File tree

‎lib/cherry_pick.js‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,31 @@ export default class CherryPick {
1616
lint,
1717
includeCVE,
1818
cveIds,
19-
vulnCveMap
19+
vulnCveMap,
20+
promptAmend,
21+
skipMessagePrompt
2022
} = {}) {
2123
this.prid = prid;
2224
this.cli = cli;
2325
this.dir = dir;
2426
this.upstream = upstream;
2527
this.gpgSign = gpgSign;
26-
this.options = { owner, repo, lint, includeCVE, cveIds, vulnCveMap };
28+
this.skipMessagePrompt = skipMessagePrompt ?? false;
29+
this.options = {
30+
owner, repo, lint, includeCVE, cveIds, vulnCveMap, promptAmend
31+
};
2732
}
2833

2934
get includeCVE() {
3035
return this.options.includeCVE ?? false;
3136
}
3237

38+
// Whether to ask before amending the PR to the proposal. Callers that already
39+
// asked opt out of it.
40+
get promptAmend() {
41+
return this.options.promptAmend ?? true;
42+
}
43+
3344
get cveIds() {
3445
return this.options.cveIds ?? null;
3546
}
@@ -83,13 +94,15 @@ export default class CherryPick {
8394
this.expectedCommitShas =
8495
metadata.data.commits.map(({ commit }) => commit.oid);
8596

86-
const amend = await cli.prompt(
87-
'Would you like to amend this PR to the proposal?',
88-
{ default: true }
89-
);
97+
if (this.promptAmend) {
98+
const amend = await cli.prompt(
99+
'Would you like to amend this PR to the proposal?',
100+
{ default: true }
101+
);
90102

91-
if (!amend) {
92-
return true;
103+
if (!amend) {
104+
return true;
105+
}
93106
}
94107

95108
try {
@@ -117,7 +130,7 @@ export default class CherryPick {
117130
{ captureStdout: 'lines' });
118131

119132
if (commitInfo.shas.length !== 1) {
120-
const fixupAll = await cli.prompt(
133+
const fixupAll = this.skipMessagePrompt || await cli.prompt(
121134
`${subjects.length} commits from the original PR are going to be` +
122135
'squashed into a single commit. OK to proceed?', {
123136
defaultAnswer: true

‎lib/landing_session.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -430,7 +430,10 @@ export default class LandingSession extends Session {
430430
const messageFile = this.saveMessage(rev, message);
431431
cli.separator('New Message');
432432
cli.log(message.trim());
433-
const takeMessage = await cli.prompt('Use this message?');
433+
// `skipMessagePrompt` is only set by callers that already got a blanket
434+
// confirmation; it is undefined on a regular landing session.
435+
const takeMessage = this.skipMessagePrompt ||
436+
await cli.prompt('Use this message?');
434437
if (takeMessage) {
435438
await runAsync('git', ['commit', '--amend', '-F', messageFile, ...this.gpgSign]);
436439
return true;

‎lib/prepare_release.js‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ import {
1313
} from './release/utils.js';
1414
import CherryPick from './cherry_pick.js';
1515
import Session from './session.js';
16-
import { getAffectedVersionLines } from './security-release/security-release.js';
16+
import {
17+
getAffectedVersionLines,
18+
getDependencyUpdates
19+
} from './security-release/security-release.js';
1720

1821
const isWindows = process.platform === 'win32';
1922

@@ -130,11 +133,11 @@ export default class ReleasePreparation extends Session {
130133
}
131134
}
132135

133-
for (const [name, dep] of Object.entries(vulnData.dependencies ?? {})) {
136+
for (const dep of getDependencyUpdates(vulnData.dependencies)) {
134137
const url = getPullRequestURLForLine(
135138
dep.affectedVersions, line, dep.prURL);
136139
if (url) {
137-
targets.push({ url, cveIds: null, label: `dependency: ${name}` });
140+
targets.push({ url, cveIds: null, label: `dependency: ${dep.name}` });
138141
}
139142
}
140143

@@ -169,13 +172,31 @@ export default class ReleasePreparation extends Session {
169172
cli.info(`Cherry-picking ${targets.length} PR(s) for ${line} from ` +
170173
'vulnerabilities.json');
171174

175+
let amendAll = false;
176+
172177
for (const target of targets) {
173178
const pr = parsePullRequestURL(target.url);
174179
if (!pr) {
175180
cli.warn(`Skipping invalid PR URL for ${target.label}: ${target.url}`);
176181
continue;
177182
}
178183

184+
if (!amendAll) {
185+
const answer = await cli.promptSelect(
186+
`Would you like to amend ${target.label} (${target.url}) to the proposal?`,
187+
[
188+
{ name: 'Yes', value: 'yes' },
189+
{ name: 'No, skip this PR', value: 'no' },
190+
{
191+
name: 'Yes to all remaining PRs (no further prompts)',
192+
value: 'all'
193+
}
194+
]);
195+
196+
if (answer === 'no') continue;
197+
amendAll = answer === 'all';
198+
}
199+
179200
if (!target.cveIds) {
180201
cli.warn(`No CVE-IDs found in vulnerabilities.json for ${target.url}`);
181202
}
@@ -188,7 +209,9 @@ export default class ReleasePreparation extends Session {
188209
upstream: this.upstreamForPR(pr),
189210
lint: false,
190211
includeCVE: true,
191-
cveIds: target.cveIds
212+
cveIds: target.cveIds,
213+
promptAmend: false,
214+
skipMessagePrompt: amendAll
192215
});
193216
const success = await cp.start();
194217
if (!success) {
@@ -862,6 +885,10 @@ export default class ReleasePreparation extends Session {
862885
return;
863886
}
864887
this.stagingBranch = currentBranch;
888+
// On security releases `branch` resolves to `releaseBranch`, which
889+
// tryResetBranch() needs. Derive it from the branch that is checked out:
890+
// the new version is only calculated below, and it may bump the major.
891+
this.releaseBranch = `v${match[1]}.x`;
865892
await this.tryResetBranch();
866893
this.versionComponents = await this.calculateNewVersion(await this.getLastRelease(match[1]));
867894
const { major, minor, patch } = this.versionComponents;

‎lib/security-release/security-release.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,14 @@ export function getAffectedVersionLines(affectedVersions) {
245245
return [];
246246
}
247247

248+
// `dependencies` maps a dependency name to its update, or to a list of updates
249+
// when the release lines got different ones. Flatten it into a list of updates
250+
// that each know their name.
251+
export function getDependencyUpdates(dependencies) {
252+
return Object.entries(dependencies ?? {}).flatMap(([name, update]) =>
253+
(Array.isArray(update) ? update : [update]).map((entry) => ({ ...entry, name })));
254+
}
255+
248256
export function promptDependencies(cli) {
249257
return cli.prompt('Enter the link to the dependency update PR (leave empty to exit): ', {
250258
defaultAnswer: '',

‎lib/security_blog.js‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
SecurityRelease,
99
commitAndPushVulnerabilitiesJSON,
1010
getAffectedVersionLines,
11+
getDependencyUpdates,
1112
getHighestSeverityAnnouncement,
1213
writeSecurityFile,
1314
} from './security-release/security-release.js';
@@ -293,24 +294,16 @@ export default class SecurityBlog extends SecurityRelease {
293294
}
294295

295296
getDependencyUpdatesTemplate(dependencyUpdates) {
296-
if (typeof dependencyUpdates !== 'object') return '';
297-
if (Object.keys(dependencyUpdates).length === 0) return '';
297+
const updates = getDependencyUpdates(dependencyUpdates);
298+
if (updates.length === 0) return '';
298299
let template = '\nThis security release includes the following dependency' +
299300
' updates to address public vulnerabilities:\n';
300-
if (Array.isArray(dependencyUpdates)) {
301-
for (const dependency of dependencyUpdates) {
302-
const title = this.formatDependencyUpdateTitle(dependency);
303-
const releaseLines = getAffectedVersionLines(dependency.affectedVersions);
304-
template += `- ${title} on ${releaseLines.join(', ')}\n`;
305-
}
306-
return template;
307-
}
308-
309-
for (const [dependency, { versions, affectedVersions }] of Object.entries(dependencyUpdates)) {
310-
const releaseLines = getAffectedVersionLines(affectedVersions);
311-
const formattedVersions = this.formatDependencyVersions(versions);
301+
for (const update of updates) {
302+
const title = this.formatDependencyUpdateTitle(update) || update.name;
303+
const releaseLines = getAffectedVersionLines(update.affectedVersions);
304+
const formattedVersions = this.formatDependencyVersions(update.versions);
312305
const versionText = formattedVersions ? ` (${formattedVersions})` : '';
313-
template += `- ${dependency}${versionText} on ${releaseLines.join(', ')}\n`;
306+
template += `- ${title}${versionText} on ${releaseLines.join(', ')}\n`;
314307
}
315308
return template;
316309
}
@@ -328,9 +321,11 @@ export default class SecurityBlog extends SecurityRelease {
328321

329322
formatDependencyVersions(versions) {
330323
if (!Array.isArray(versions)) return '';
331-
return versions
324+
// The current schema carries one entry per release line, so the same
325+
// version repeats when every line gets the same update.
326+
return [...new Set(versions
332327
.map((version) => this.formatDependencyUpdateVersion(version))
333-
.filter(Boolean)
328+
.filter(Boolean))]
334329
.join(', ');
335330
}
336331

‎test/unit/security_release.test.js‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import PrepareSecurityRelease, {
1212
import UpdateSecurityRelease from '../../lib/update_security_release.js';
1313
import {
1414
getAffectedVersionLines,
15+
getDependencyUpdates,
1516
getHighestSeverityAnnouncement
1617
} from '../../lib/security-release/security-release.js';
1718

@@ -909,8 +910,76 @@ describe('security_blog: getDependencyUpdatesTemplate', () => {
909910
assert.match(template, /- undici \(8\.5\.0\) on 22\.x, 24\.x/);
910911
});
911912

913+
it('does not repeat a version listed once per release line', () => {
914+
const blog = new SecurityBlog();
915+
const template = blog.getDependencyUpdatesTemplate({
916+
llhttp: {
917+
versions: [
918+
{ version: '9.4.3', affectedVersions: ['main'] },
919+
{ version: '9.4.3', affectedVersions: ['26.x'] },
920+
{ version: '9.4.3', affectedVersions: ['24.x'] }
921+
],
922+
affectedVersions: {
923+
main: 'https://github.com/nodejs-private/node-private/pull/935',
924+
'26.x': 'https://github.com/nodejs-private/node-private/pull/935',
925+
'24.x': 'https://github.com/nodejs-private/node-private/pull/935'
926+
}
927+
}
928+
});
929+
930+
assert.match(template, /- llhttp \(9\.4\.3\) on 26\.x, 24\.x/);
931+
});
932+
933+
it('renders a dependency holding a list of updates', () => {
934+
const blog = new SecurityBlog();
935+
const template = blog.getDependencyUpdatesTemplate({
936+
nghttp2: [
937+
{
938+
title: 'deps: update nghttp2 to 1.68.0',
939+
affectedVersions: ['24.x']
940+
},
941+
{
942+
name: 'nghttp2',
943+
versions: ['1.67.1'],
944+
affectedVersions: ['22.x']
945+
}
946+
]
947+
});
948+
949+
assert.match(template, /- update nghttp2 to 1\.68\.0 on 24\.x/);
950+
assert.match(template, /- nghttp2 \(1\.67\.1\) on 22\.x/);
951+
});
952+
912953
it('returns an empty string when there are no dependency updates', () => {
913954
const blog = new SecurityBlog();
914955
assert.strictEqual(blog.getDependencyUpdatesTemplate({}), '');
956+
assert.strictEqual(blog.getDependencyUpdatesTemplate(undefined), '');
957+
});
958+
});
959+
960+
describe('security_release: getDependencyUpdates', () => {
961+
it('names the updates of a map keyed by dependency', () => {
962+
assert.deepStrictEqual(
963+
getDependencyUpdates({ undici: { versions: ['8.9.0'] } }),
964+
[{ versions: ['8.9.0'], name: 'undici' }]);
965+
});
966+
967+
it('flattens a dependency holding a list of updates', () => {
968+
assert.deepStrictEqual(
969+
getDependencyUpdates({
970+
nghttp2: [
971+
{ affectedVersions: ['24.x'] },
972+
{ name: 'nghttp2', affectedVersions: ['22.x'] }
973+
]
974+
}),
975+
[
976+
{ affectedVersions: ['24.x'], name: 'nghttp2' },
977+
{ affectedVersions: ['22.x'], name: 'nghttp2' }
978+
]);
979+
});
980+
981+
it('returns an empty list when there are no dependency updates', () => {
982+
assert.deepStrictEqual(getDependencyUpdates(undefined), []);
983+
assert.deepStrictEqual(getDependencyUpdates({}), []);
915984
});
916985
});

0 commit comments

Comments
 (0)