feat: add confirmation steps to any write operation (#1092)

Author: RafaelGSS

docs/git-node.md

@@ -440,6 +440,12 @@ $ git node vote \
 
 Manage or starts a security release process.
 
+Every `git node security` action asks for permission before each mutating step.
+Read-only operations, such as reading files or fetching report data, run without
+confirmation. Each confirmation names the command or service action that will
+write state and explains the side effect, such as writing files, committing and
+pushing changes, creating issues, or updating HackerOne reports.
+
 <a id="git-node-security-prerequisites"></a>
 
 ### Prerequisites

lib/prepare_security.js

@@ -12,6 +12,8 @@ import {
   getSupportedVersions,
   getReportSeverity,
   pickReport,
+  confirmSecurityStep,
+  writeSecurityFile,
   SecurityRelease
 } from './security-release/security-release.js';
 import _ from 'lodash';
@@ -72,6 +74,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
 
     if (vulnerabilityJSON.buildIssue) {
       this.cli.info('Commenting on nodejs/build issue');
+      await confirmSecurityStep(
+        this.cli,
+        `comment on GitHub issue \`${vulnerabilityJSON.buildIssue}\``,
+        'This posts that the security release is out on the build tracking issue.'
+      );
       await this.req.commentIssue(
         vulnerabilityJSON.buildIssue,
         'Security release is out'
@@ -80,6 +87,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
 
     if (vulnerabilityJSON.dockerIssue) {
       this.cli.info('Commenting on nodejs/docker-node issue');
+      await confirmSecurityStep(
+        this.cli,
+        `comment on GitHub issue \`${vulnerabilityJSON.dockerIssue}\``,
+        'This posts that the security release is out on the docker-node tracking issue.'
+      );
       await this.req.commentIssue(
         vulnerabilityJSON.dockerIssue,
         'Security release is out'
@@ -91,11 +103,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
         vulnerabilityJSON.releaseDate}?`,
       { defaultAnswer: true });
     if (updateFolder) {
-      this.updateReleaseFolder(
+      await this.updateReleaseFolder(
         vulnerabilityJSON.releaseDate.replaceAll('/', '-')
       );
       const securityReleaseFolder = path.join(process.cwd(), 'security-release');
-      commitAndPushVulnerabilitiesJSON(
+      await commitAndPushVulnerabilitiesJSON(
         securityReleaseFolder,
         'chore: change next-security-release folder',
         { cli: this.cli, repository: this.repository }
@@ -110,7 +122,7 @@ export default class PrepareSecurityRelease extends SecurityRelease {
 
   async startVulnerabilitiesJSONCreation(releaseDate, content, excludedReports = []) {
     // checkout on the next-security-release branch
-    checkoutOnSecurityReleaseBranch(this.cli, this.repository);
+    await checkoutOnSecurityReleaseBranch(this.cli, this.repository);
 
     // choose the reports to include in the security release
     const reports = await this.chooseReports(excludedReports);
@@ -134,7 +146,7 @@ export default class PrepareSecurityRelease extends SecurityRelease {
 
     // commit and push the vulnerabilities.json file
     const commitMessage = 'chore: create vulnerabilities.json for next security release';
-    commitAndPushVulnerabilitiesJSON(filePath,
+    await commitAndPushVulnerabilitiesJSON(filePath,
       commitMessage,
       { cli: this.cli, repository: this.repository });
 
@@ -270,17 +282,31 @@ export default class PrepareSecurityRelease extends SecurityRelease {
     }, null, 2) + '\n';
 
     const folderPath = path.resolve(NEXT_SECURITY_RELEASE_FOLDER);
-    await fs.promises.mkdir(folderPath, { recursive: true });
-
     const fullPath = path.join(folderPath, 'vulnerabilities.json');
-    await fs.promises.writeFile(fullPath, fileContent);
+    await confirmSecurityStep(
+      this.cli,
+      `create directory \`${folderPath}\``,
+      'This creates the security release folder if it does not already exist.'
+    );
+    await fs.promises.mkdir(folderPath, { recursive: true });
+    await writeSecurityFile(
+      this.cli,
+      fullPath,
+      fileContent,
+      'This creates vulnerabilities.json for the next security release.'
+    );
     this.cli.stopSpinner(`Created ${fullPath}`);
 
     return fullPath;
   }
 
   async createPullRequest(content) {
     const { owner, repo } = this.repository;
+    await confirmSecurityStep(
+      this.cli,
+      `create GitHub pull request \`${owner}/${repo}: ${this.title}\``,
+      `This opens a pull request from ${NEXT_SECURITY_RELEASE_BRANCH} to main.`
+    );
     const response = await this.req.createPullRequest(
       this.title,
       content ?? 'List of vulnerabilities to be included in the next security release',
@@ -361,13 +387,23 @@ export default class PrepareSecurityRelease extends SecurityRelease {
     this.cli.startSpinner('Closing HackerOne reports');
     for (const report of jsonReports) {
       this.cli.updateSpinner(`Closing report ${report.id}...`);
+      await confirmSecurityStep(
+        this.cli,
+        `resolve HackerOne report \`${report.id}\``,
+        'This marks the HackerOne report as resolved.'
+      );
       await this.req.updateReportState(
         report.id,
         'resolved',
         'Closing as resolved'
       );
 
       this.cli.updateSpinner(`Requesting disclosure to report ${report.id}...`);
+      await confirmSecurityStep(
+        this.cli,
+        `request disclosure for HackerOne report \`${report.id}\``,
+        'This asks HackerOne to disclose the resolved report.'
+      );
       await this.req.requestDisclosure(report.id);
     }
     this.cli.stopSpinner('Done closing H1 Reports and requesting disclosure');
@@ -385,6 +421,11 @@ export default class PrepareSecurityRelease extends SecurityRelease {
     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' });
       }

lib/security-announcement.js

@@ -1,4 +1,3 @@
-import fs from 'node:fs';
 import {
   NEXT_SECURITY_RELEASE_REPOSITORY,
   checkoutOnSecurityReleaseBranch,
@@ -7,7 +6,8 @@ import {
   validateDate,
   formatDateToYYYYMMDD,
   commitAndPushVulnerabilitiesJSON,
-  createIssue
+  createIssue,
+  writeSecurityFile
 } from './security-release/security-release.js';
 import auth from './auth.js';
 import Request from './request.js';
@@ -30,7 +30,7 @@ export default class SecurityAnnouncement {
     this.req = new Request(credentials);
 
     // checkout on security release branch
-    checkoutOnSecurityReleaseBranch(cli, this.repository);
+    await checkoutOnSecurityReleaseBranch(cli, this.repository);
     // read vulnerabilities JSON file
     const content = getVulnerabilitiesJSON(cli);
     // validate the release date read from vulnerabilities JSON
@@ -52,9 +52,14 @@ export default class SecurityAnnouncement {
     content.dockerIssue = dockerIssue;
 
     const vulnerabilitiesJSONPath = getVulnerabilitiesJSONPath();
-    fs.writeFileSync(vulnerabilitiesJSONPath, JSON.stringify(content, null, 2));
+    await writeSecurityFile(
+      cli,
+      vulnerabilitiesJSONPath,
+      JSON.stringify(content, null, 2),
+      'This records the build and docker tracking issue links in vulnerabilities.json.'
+    );
     const commitMessage = 'chore: add build and docker issue link';
-    commitAndPushVulnerabilitiesJSON([vulnerabilitiesJSONPath],
+    await commitAndPushVulnerabilitiesJSON([vulnerabilitiesJSONPath],
       commitMessage, { cli: this.cli, repository: this.repository });
 
     this.cli.ok('Added docker and build issue in vulnerabilities.json');

lib/security-release/security-release.js

@@ -30,7 +30,34 @@ export const PLACEHOLDERS = {
   downloads: '%DOWNLOADS%'
 };
 
-export function checkRemote(cli, repository) {
+function formatCommand(command, args) {
+  return [command, ...args].join(' ');
+}
+
+export async function confirmSecurityStep(cli, action, detail) {
+  let message = `Allow action: ${action}?`;
+  if (detail) {
+    message += `\n\n${detail}`;
+  }
+
+  const allowed = await cli.prompt(message, { defaultAnswer: false });
+  if (!allowed) {
+    throw new Error(`Aborted: ${action}.`);
+  }
+}
+
+export async function runSecurityGitCommand(cli, args, detail) {
+  const command = formatCommand('git', args);
+  await confirmSecurityStep(cli, `run \`${command}\``, detail);
+  return runSync('git', args);
+}
+
+export async function writeSecurityFile(cli, filePath, content, detail) {
+  await confirmSecurityStep(cli, `write \`${filePath}\``, detail);
+  return fs.writeFileSync(filePath, content);
+}
+
+export async function checkRemote(cli, repository) {
   const remote = runSync('git', ['ls-remote', '--get-url', 'origin']).trim();
   const { owner, repo } = repository;
   const securityReleaseOrigin = [
@@ -44,23 +71,31 @@ export function checkRemote(cli, repository) {
   }
 }
 
-export function checkoutOnSecurityReleaseBranch(cli, repository) {
-  checkRemote(cli, repository);
+export async function checkoutOnSecurityReleaseBranch(cli, repository) {
+  await checkRemote(cli, repository);
   const currentBranch = runSync('git', ['branch', '--show-current']).trim();
   cli.info(`Current branch: ${currentBranch} `);
 
   if (currentBranch !== NEXT_SECURITY_RELEASE_BRANCH) {
-    runSync('git', ['checkout', '-B', NEXT_SECURITY_RELEASE_BRANCH]);
+    await runSecurityGitCommand(
+      cli,
+      ['checkout', '-B', NEXT_SECURITY_RELEASE_BRANCH],
+      `This checks out or recreates the ${NEXT_SECURITY_RELEASE_BRANCH} branch locally.`
+    );
     cli.ok(`Checkout on branch: ${NEXT_SECURITY_RELEASE_BRANCH} `);
   };
 }
 
-export function commitAndPushVulnerabilitiesJSON(filePath, commitMessage, { cli, repository }) {
-  checkRemote(cli, repository);
+export async function commitAndPushVulnerabilitiesJSON(
+  filePath,
+  commitMessage,
+  { cli, repository }
+) {
+  await checkRemote(cli, repository);
 
   if (Array.isArray(filePath)) {
-    for (const path of filePath) {
-      runSync('git', ['add', path]);
+    for (const currentPath of filePath) {
+      runSync('git', ['add', currentPath]);
     }
   } else {
     runSync('git', ['add', filePath]);
@@ -72,15 +107,31 @@ export function commitAndPushVulnerabilitiesJSON(filePath, commitMessage, { cli,
     return;
   }
 
-  runSync('git', ['commit', '-m', commitMessage]);
+  await runSecurityGitCommand(
+    cli,
+    ['commit', '-m', commitMessage],
+    `This creates a local commit with message: ${commitMessage}`
+  );
 
   try {
-    runSync('git', ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH]);
+    await runSecurityGitCommand(
+      cli,
+      ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH],
+      `This pushes the security release branch to origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
+    );
   } catch (error) {
     cli.warn('Rebasing...');
     // try to pull rebase and push again
-    runSync('git', ['pull', 'origin', NEXT_SECURITY_RELEASE_BRANCH, '--rebase']);
-    runSync('git', ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH]);
+    await runSecurityGitCommand(
+      cli,
+      ['pull', 'origin', NEXT_SECURITY_RELEASE_BRANCH, '--rebase'],
+      `This rebases local changes on origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
+    );
+    await runSecurityGitCommand(
+      cli,
+      ['push', '-u', 'origin', NEXT_SECURITY_RELEASE_BRANCH],
+      `This retries pushing the security release branch to origin/${NEXT_SECURITY_RELEASE_BRANCH}.`
+    );
   }
   cli.ok(`Pushed commit: ${commitMessage} to ${NEXT_SECURITY_RELEASE_BRANCH}`);
 }
@@ -150,6 +201,11 @@ export function promptDependencies(cli) {
 }
 
 export async function createIssue(title, content, repository, { cli, req }) {
+  await confirmSecurityStep(
+    cli,
+    `create GitHub issue \`${repository.owner}/${repository.repo}: ${title}\``,
+    `This creates an issue in ${repository.owner}/${repository.repo}.`
+  );
   const data = await req.createIssue(title, content, repository);
   if (data.html_url) {
     cli.ok(`Created: ${data.html_url}`);
@@ -252,20 +308,30 @@ export class SecurityRelease {
       NEXT_SECURITY_RELEASE_FOLDER, 'vulnerabilities.json');
   }
 
-  updateReleaseFolder(releaseDate) {
+  async updateReleaseFolder(releaseDate) {
     const folder = path.join(process.cwd(),
       NEXT_SECURITY_RELEASE_FOLDER);
     const newFolder = path.join(process.cwd(), 'security-release', releaseDate);
+    await confirmSecurityStep(
+      this.cli,
+      `rename \`${folder}\` to \`${newFolder}\``,
+      'This moves the next-security-release folder to the dated release folder.'
+    );
     fs.renameSync(folder, newFolder);
     return newFolder;
   }
 
-  updateVulnerabilitiesJSON(content) {
+  async updateVulnerabilitiesJSON(content) {
     try {
       const vulnerabilitiesJSONPath = this.getVulnerabilitiesJSONPath();
       this.cli.startSpinner(`Updating vulnerabilities.json from ${vulnerabilitiesJSONPath}...`);
-      fs.writeFileSync(vulnerabilitiesJSONPath, JSON.stringify(content, null, 2));
-      commitAndPushVulnerabilitiesJSON(vulnerabilitiesJSONPath,
+      await writeSecurityFile(
+        this.cli,
+        vulnerabilitiesJSONPath,
+        JSON.stringify(content, null, 2),
+        'This updates vulnerabilities.json with the latest security release data.'
+      );
+      await commitAndPushVulnerabilitiesJSON(vulnerabilitiesJSONPath,
         'chore: updated vulnerabilities.json',
         { cli: this.cli, repository: this.repository });
       this.cli.stopSpinner(`Done updating vulnerabilities.json from ${vulnerabilitiesJSONPath}`);