Skip to content

Commit 3545e28

Browse files
committed
feat: add confirmation steps to any write operation
1 parent 4ea22b8 commit 3545e28

6 files changed

Lines changed: 234 additions & 54 deletions

File tree

docs/git-node.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,12 @@ $ git node vote \
440440

441441
Manage or starts a security release process.
442442

443+
Every `git node security` action asks for permission before each mutating step.
444+
Read-only operations, such as reading files or fetching report data, run without
445+
confirmation. Each confirmation names the command or service action that will
446+
write state and explains the side effect, such as writing files, committing and
447+
pushing changes, creating issues, or updating HackerOne reports.
448+
443449
<a id="git-node-security-prerequisites"></a>
444450

445451
### Prerequisites

lib/prepare_security.js

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import {
1212
getSupportedVersions,
1313
getReportSeverity,
1414
pickReport,
15+
confirmSecurityStep,
16+
writeSecurityFile,
1517
SecurityRelease
1618
} from './security-release/security-release.js';
1719
import _ from 'lodash';
@@ -72,6 +74,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
7274

7375
if (vulnerabilityJSON.buildIssue) {
7476
this.cli.info('Commenting on nodejs/build issue');
77+
await confirmSecurityStep(
78+
this.cli,
79+
`comment on GitHub issue \`${vulnerabilityJSON.buildIssue}\``,
80+
'This posts that the security release is out on the build tracking issue.'
81+
);
7582
await this.req.commentIssue(
7683
vulnerabilityJSON.buildIssue,
7784
'Security release is out'
@@ -80,6 +87,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
8087

8188
if (vulnerabilityJSON.dockerIssue) {
8289
this.cli.info('Commenting on nodejs/docker-node issue');
90+
await confirmSecurityStep(
91+
this.cli,
92+
`comment on GitHub issue \`${vulnerabilityJSON.dockerIssue}\``,
93+
'This posts that the security release is out on the docker-node tracking issue.'
94+
);
8395
await this.req.commentIssue(
8496
vulnerabilityJSON.dockerIssue,
8597
'Security release is out'
@@ -91,11 +103,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
91103
vulnerabilityJSON.releaseDate}?`,
92104
{ defaultAnswer: true });
93105
if (updateFolder) {
94-
this.updateReleaseFolder(
106+
await this.updateReleaseFolder(
95107
vulnerabilityJSON.releaseDate.replaceAll('/', '-')
96108
);
97109
const securityReleaseFolder = path.join(process.cwd(), 'security-release');
98-
commitAndPushVulnerabilitiesJSON(
110+
await commitAndPushVulnerabilitiesJSON(
99111
securityReleaseFolder,
100112
'chore: change next-security-release folder',
101113
{ cli: this.cli, repository: this.repository }
@@ -110,7 +122,7 @@ export default class PrepareSecurityRelease extends SecurityRelease {
110122

111123
async startVulnerabilitiesJSONCreation(releaseDate, content, excludedReports = []) {
112124
// checkout on the next-security-release branch
113-
checkoutOnSecurityReleaseBranch(this.cli, this.repository);
125+
await checkoutOnSecurityReleaseBranch(this.cli, this.repository);
114126

115127
// choose the reports to include in the security release
116128
const reports = await this.chooseReports(excludedReports);
@@ -134,7 +146,7 @@ export default class PrepareSecurityRelease extends SecurityRelease {
134146

135147
// commit and push the vulnerabilities.json file
136148
const commitMessage = 'chore: create vulnerabilities.json for next security release';
137-
commitAndPushVulnerabilitiesJSON(filePath,
149+
await commitAndPushVulnerabilitiesJSON(filePath,
138150
commitMessage,
139151
{ cli: this.cli, repository: this.repository });
140152

@@ -270,17 +282,31 @@ export default class PrepareSecurityRelease extends SecurityRelease {
270282
}, null, 2) + '\n';
271283

272284
const folderPath = path.resolve(NEXT_SECURITY_RELEASE_FOLDER);
273-
await fs.promises.mkdir(folderPath, { recursive: true });
274-
275285
const fullPath = path.join(folderPath, 'vulnerabilities.json');
276-
await fs.promises.writeFile(fullPath, fileContent);
286+
await confirmSecurityStep(
287+
this.cli,
288+
`create directory \`${folderPath}\``,
289+
'This creates the security release folder if it does not already exist.'
290+
);
291+
await fs.promises.mkdir(folderPath, { recursive: true });
292+
await writeSecurityFile(
293+
this.cli,
294+
fullPath,
295+
fileContent,
296+
'This creates vulnerabilities.json for the next security release.'
297+
);
277298
this.cli.stopSpinner(`Created ${fullPath}`);
278299

279300
return fullPath;
280301
}
281302

282303
async createPullRequest(content) {
283304
const { owner, repo } = this.repository;
305+
await confirmSecurityStep(
306+
this.cli,
307+
`create GitHub pull request \`${owner}/${repo}: ${this.title}\``,
308+
`This opens a pull request from ${NEXT_SECURITY_RELEASE_BRANCH} to main.`
309+
);
284310
const response = await this.req.createPullRequest(
285311
this.title,
286312
content ?? 'List of vulnerabilities to be included in the next security release',
@@ -361,13 +387,23 @@ export default class PrepareSecurityRelease extends SecurityRelease {
361387
this.cli.startSpinner('Closing HackerOne reports');
362388
for (const report of jsonReports) {
363389
this.cli.updateSpinner(`Closing report ${report.id}...`);
390+
await confirmSecurityStep(
391+
this.cli,
392+
`resolve HackerOne report \`${report.id}\``,
393+
'This marks the HackerOne report as resolved.'
394+
);
364395
await this.req.updateReportState(
365396
report.id,
366397
'resolved',
367398
'Closing as resolved'
368399
);
369400

370401
this.cli.updateSpinner(`Requesting disclosure to report ${report.id}...`);
402+
await confirmSecurityStep(
403+
this.cli,
404+
`request disclosure for HackerOne report \`${report.id}\``,
405+
'This asks HackerOne to disclose the resolved report.'
406+
);
371407
await this.req.requestDisclosure(report.id);
372408
}
373409
this.cli.stopSpinner('Done closing H1 Reports and requesting disclosure');
@@ -385,6 +421,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
385421
for (const pr of prs) {
386422
if (pr.labels.some((l) => labels.includes(l.name))) {
387423
this.cli.updateSpinner(`Closing Pull Request: ${pr.number}`);
424+
await confirmSecurityStep(
425+
this.cli,
426+
`close GitHub pull request \`nodejs-private/node-private#${pr.number}\``,
427+
'This closes a pull request labeled for the security release.'
428+
);
388429
await this.req.closePullRequest(pr.number,
389430
{ owner: 'nodejs-private', repo: 'node-private' });
390431
}

lib/security-announcement.js

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import fs from 'node:fs';
21
import {
32
NEXT_SECURITY_RELEASE_REPOSITORY,
43
checkoutOnSecurityReleaseBranch,
@@ -7,7 +6,8 @@ import {
76
validateDate,
87
formatDateToYYYYMMDD,
98
commitAndPushVulnerabilitiesJSON,
10-
createIssue
9+
createIssue,
10+
writeSecurityFile
1111
} from './security-release/security-release.js';
1212
import auth from './auth.js';
1313
import Request from './request.js';
@@ -30,7 +30,7 @@ export default class SecurityAnnouncement {
3030
this.req = new Request(credentials);
3131

3232
// checkout on security release branch
33-
checkoutOnSecurityReleaseBranch(cli, this.repository);
33+
await checkoutOnSecurityReleaseBranch(cli, this.repository);
3434
// read vulnerabilities JSON file
3535
const content = getVulnerabilitiesJSON(cli);
3636
// validate the release date read from vulnerabilities JSON
@@ -52,9 +52,14 @@ export default class SecurityAnnouncement {
5252
content.dockerIssue = dockerIssue;
5353

5454
const vulnerabilitiesJSONPath = getVulnerabilitiesJSONPath();
55-
fs.writeFileSync(vulnerabilitiesJSONPath, JSON.stringify(content, null, 2));
55+
await writeSecurityFile(
56+
cli,
57+
vulnerabilitiesJSONPath,
58+
JSON.stringify(content, null, 2),
59+
'This records the build and docker tracking issue links in vulnerabilities.json.'
60+
);
5661
const commitMessage = 'chore: add build and docker issue link';
57-
commitAndPushVulnerabilitiesJSON([vulnerabilitiesJSONPath],
62+
await commitAndPushVulnerabilitiesJSON([vulnerabilitiesJSONPath],
5863
commitMessage, { cli: this.cli, repository: this.repository });
5964

6065
this.cli.ok('Added docker and build issue in vulnerabilities.json');

lib/security-release/security-release.js

Lines changed: 93 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,36 @@ export const PLACEHOLDERS = {
3030
downloads: '%DOWNLOADS%'
3131
};
3232

33-
export function checkRemote(cli, repository) {
33+
function formatCommand(command, args) {
34+
return [command, ...args].join(' ');
35+
}
36+
37+
export async function confirmSecurityStep(cli, action, detail) {
38+
const lines = [`Allow action: ${action}?`];
39+
if (detail) {
40+
lines.push('', detail);
41+
}
42+
const message = lines.join('\n');
43+
44+
const allowed = await cli.prompt(message, { defaultAnswer: false });
45+
if (!allowed) {
46+
cli.info(`Aborted: ${action}.`);
47+
process.exit(0);
48+
}
49+
}
50+
51+
export async function runSecurityGitCommand(cli, args, detail) {
52+
const command = formatCommand('git', args);
53+
await confirmSecurityStep(cli, `run \`${command}\``, detail);
54+
return runSync('git', args);
55+
}
56+
57+
export async function writeSecurityFile(cli, filePath, content, detail) {
58+
await confirmSecurityStep(cli, `write \`${filePath}\``, detail);
59+
return fs.writeFileSync(filePath, content);
60+
}
61+
62+
export async function checkRemote(cli, repository) {
3463
const remote = runSync('git', ['ls-remote', '--get-url', 'origin']).trim();
3564
const { owner, repo } = repository;
3665
const securityReleaseOrigin = [
@@ -44,26 +73,42 @@ export function checkRemote(cli, repository) {
4473
}
4574
}
4675

47-
export function checkoutOnSecurityReleaseBranch(cli, repository) {
48-
checkRemote(cli, repository);
76+
export async function checkoutOnSecurityReleaseBranch(cli, repository) {
77+
await checkRemote(cli, repository);
4978
const currentBranch = runSync('git', ['branch', '--show-current']).trim();
5079
cli.info(`Current branch: ${currentBranch} `);
5180

5281
if (currentBranch !== NEXT_SECURITY_RELEASE_BRANCH) {
53-
runSync('git', ['checkout', '-B', NEXT_SECURITY_RELEASE_BRANCH]);
82+
await runSecurityGitCommand(
83+
cli,
84+
['checkout', '-B', NEXT_SECURITY_RELEASE_BRANCH],
85+
`This checks out or recreates the ${NEXT_SECURITY_RELEASE_BRANCH} branch locally.`
86+
);
5487
cli.ok(`Checkout on branch: ${NEXT_SECURITY_RELEASE_BRANCH} `);
5588
};
5689
}
5790

58-
export function commitAndPushVulnerabilitiesJSON(filePath, commitMessage, { cli, repository }) {
59-
checkRemote(cli, repository);
91+
export async function commitAndPushVulnerabilitiesJSON(
92+
filePath,
93+
commitMessage,
94+
{ cli, repository }
95+
) {
96+
await checkRemote(cli, repository);
6097

6198
if (Array.isArray(filePath)) {
62-
for (const path of filePath) {
63-
runSync('git', ['add', path]);
99+
for (const currentPath of filePath) {
100+
await runSecurityGitCommand(
101+
cli,
102+
['add', currentPath],
103+
`This stages ${currentPath} for the security release commit.`
104+
);
64105
}
65106
} else {
66-
runSync('git', ['add', filePath]);
107+
await runSecurityGitCommand(
108+
cli,
109+
['add', filePath],
110+
`This stages ${filePath} for the security release commit.`
111+
);
67112
}
68113

69114
const staged = runSync('git', ['diff', '--name-only', '--cached']).trim();
@@ -72,15 +117,31 @@ export function commitAndPushVulnerabilitiesJSON(filePath, commitMessage, { cli,
72117
return;
73118
}
74119

75-
runSync('git', ['commit', '-m', commitMessage]);
120+
await runSecurityGitCommand(
121+
cli,
122+
['commit', '-m', commitMessage],
123+
`This creates a local commit with message: ${commitMessage}`
124+
);
76125

77126
try {
78-
runSync('git', ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH]);
127+
await runSecurityGitCommand(
128+
cli,
129+
['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH],
130+
`This pushes the security release branch to origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
131+
);
79132
} catch (error) {
80133
cli.warn('Rebasing...');
81134
// try to pull rebase and push again
82-
runSync('git', ['pull', 'origin', NEXT_SECURITY_RELEASE_BRANCH, '--rebase']);
83-
runSync('git', ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH]);
135+
await runSecurityGitCommand(
136+
cli,
137+
['pull', 'origin', NEXT_SECURITY_RELEASE_BRANCH, '--rebase'],
138+
`This rebases local changes on origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
139+
);
140+
await runSecurityGitCommand(
141+
cli,
142+
['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH],
143+
`This retries pushing the security release branch to origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
144+
);
84145
}
85146
cli.ok(`Pushed commit: ${commitMessage} to ${NEXT_SECURITY_RELEASE_BRANCH}`);
86147
}
@@ -150,6 +211,11 @@ export function promptDependencies(cli) {
150211
}
151212

152213
export async function createIssue(title, content, repository, { cli, req }) {
214+
await confirmSecurityStep(
215+
cli,
216+
`create GitHub issue \`${repository.owner}/${repository.repo}: ${title}\``,
217+
`This creates an issue in ${repository.owner}/${repository.repo}.`
218+
);
153219
const data = await req.createIssue(title, content, repository);
154220
if (data.html_url) {
155221
cli.ok(`Created: ${data.html_url}`);
@@ -252,20 +318,30 @@ export class SecurityRelease {
252318
NEXT_SECURITY_RELEASE_FOLDER, 'vulnerabilities.json');
253319
}
254320

255-
updateReleaseFolder(releaseDate) {
321+
async updateReleaseFolder(releaseDate) {
256322
const folder = path.join(process.cwd(),
257323
NEXT_SECURITY_RELEASE_FOLDER);
258324
const newFolder = path.join(process.cwd(), 'security-release', releaseDate);
325+
await confirmSecurityStep(
326+
this.cli,
327+
`rename \`${folder}\` to \`${newFolder}\``,
328+
'This moves the next-security-release folder to the dated release folder.'
329+
);
259330
fs.renameSync(folder, newFolder);
260331
return newFolder;
261332
}
262333

263-
updateVulnerabilitiesJSON(content) {
334+
async updateVulnerabilitiesJSON(content) {
264335
try {
265336
const vulnerabilitiesJSONPath = this.getVulnerabilitiesJSONPath();
266337
this.cli.startSpinner(`Updating vulnerabilities.json from ${vulnerabilitiesJSONPath}...`);
267-
fs.writeFileSync(vulnerabilitiesJSONPath, JSON.stringify(content, null, 2));
268-
commitAndPushVulnerabilitiesJSON(vulnerabilitiesJSONPath,
338+
await writeSecurityFile(
339+
this.cli,
340+
vulnerabilitiesJSONPath,
341+
JSON.stringify(content, null, 2),
342+
'This updates vulnerabilities.json with the latest security release data.'
343+
);
344+
await commitAndPushVulnerabilitiesJSON(vulnerabilitiesJSONPath,
269345
'chore: updated vulnerabilities.json',
270346
{ cli: this.cli, repository: this.repository });
271347
this.cli.stopSpinner(`Done updating vulnerabilities.json from ${vulnerabilitiesJSONPath}`);

0 commit comments

Comments
 (0)