Merge pull request #67 from AikidoSec/exit-on-failed-change-detection

Exit installation when detecting changes failed due to non-zero exit code in dry-run

packages/safe-chain/src/main.js

@@ -15,6 +15,7 @@ export async function main(args) {
     }
   } catch (error) {
     ui.writeError("Failed to check for malicious packages:", error.message);
+    process.exit(1);
   }
 
   var result = getPackageManager().runCommand(args);

packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js

@@ -1,4 +1,3 @@
-import { ui } from "../../../environment/userInteraction.js";
 import { parseDryRunOutput } from "../parsing/parseNpmInstallDryRunOutput.js";
 import { dryRunNpmCommandAndOutput } from "../runNpmCommand.js";
 import { hasDryRunArg } from "../utils/npmCommands.js";
@@ -37,10 +36,17 @@ function checkChangesWithDryRun(args) {
 
   // Dry-run can return a non-zero status code in some cases
   //  e.g., when running "npm audit fix --dry-run", it returns exit code 1
-  //  when there are vulnurabilities that can be fixed.
+  //  when there are vulnerabilities that can be fixed.
+  if (dryRunOutput.status !== 0 && !canCommandReturnNonZeroOnSuccess(args)) {
+    throw new Error(
+      `Dry-run command failed with exit code ${dryRunOutput.status} and output:\n${dryRunOutput.output}`
+    );
+  }
+
   if (dryRunOutput.status !== 0 && !dryRunOutput.output) {
-    ui.writeError("Detecting changes failed.");
+    throw new Error(
-    return [];
+      `Dry-run command failed with exit code ${dryRunOutput.status} and produced no output.`
+    );
   }
 
   const parsedOutput = parseDryRunOutput(dryRunOutput.output);
@@ -48,3 +54,13 @@ function checkChangesWithDryRun(args) {
   // reverse the array to have the top-level packages first
   return parsedOutput.reverse();
 }
+
+function canCommandReturnNonZeroOnSuccess(args) {
+  if (args.length < 2) {
+    return false;
+  }
+
+  // `npm audit fix --dry-run` can return exit code 1 when it succesfully ran and
+  // there were vulnerabilities that could be fixed
+  return args[0] === "audit" && args[1] === "fix";
+}

packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.spec.js

@@ -0,0 +1,139 @@
+import { describe, it, mock } from "node:test";
+import assert from "node:assert/strict";
+
+describe("dryRunScanner", async () => {
+  const mockWriteError = mock.fn();
+  const mockDryRunNpmCommandAndOutput = mock.fn();
+
+  // Mock ui module
+  mock.module("../../../environment/userInteraction.js", {
+    namedExports: {
+      ui: {
+        writeError: mockWriteError,
+      },
+    },
+  });
+
+  // Mock dryRunNpmCommandAndOutput function
+  mock.module("../runNpmCommand.js", {
+    namedExports: {
+      dryRunNpmCommandAndOutput: mockDryRunNpmCommandAndOutput,
+    },
+  });
+
+  const { dryRunScanner } = await import("./dryRunScanner.js");
+
+  describe("doesCommandReturnNonZero", () => {
+    // We need to access the internal function for testing
+    // Since it's not exported, we'll test it indirectly through the main functionality
+
+    it("should handle npm audit fix commands that return non-zero", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+      mockDryRunNpmCommandAndOutput.mock.mockImplementationOnce(() => ({
+        status: 1,
+        output: "found 5 vulnerabilities that can be fixed",
+      }));
+
+      const scanner = dryRunScanner();
+      const result = scanner.scan(["audit", "fix"]);
+
+      // Should not throw an error for audit fix commands
+      assert.ok(Array.isArray(result));
+      assert.equal(mockWriteError.mock.callCount(), 0);
+    });
+
+    it("should throw error for unexpected non-zero exit codes", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+      mockDryRunNpmCommandAndOutput.mock.mockImplementationOnce(() => ({
+        status: 1,
+        output: "some error output",
+      }));
+
+      const scanner = dryRunScanner();
+
+      assert.throws(() => {
+        scanner.scan(["install", "lodash"]);
+      }, /Dry-run command failed with exit code 1/);
+    });
+
+    it("should handle zero exit codes normally", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+      mockDryRunNpmCommandAndOutput.mock.mockImplementationOnce(() => ({
+        status: 0,
+        output: "added 1 package",
+      }));
+
+      const scanner = dryRunScanner();
+      const result = scanner.scan(["install", "lodash"]);
+
+      assert.ok(Array.isArray(result));
+      assert.equal(mockWriteError.mock.callCount(), 0);
+    });
+
+    it("should throw error for non-zero exit with no output for audit fix", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+      mockDryRunNpmCommandAndOutput.mock.mockImplementationOnce(() => ({
+        status: 1,
+        output: "",
+      }));
+
+      const scanner = dryRunScanner();
+
+      assert.throws(() => {
+        scanner.scan(["audit", "fix"]);
+      }, /Dry-run command failed with exit code 1/);
+    });
+  });
+
+  describe("scanner functionality", () => {
+    it("should use dryRunCommand option when provided", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+      mockDryRunNpmCommandAndOutput.mock.mockImplementationOnce(() => ({
+        status: 0,
+        output: "no changes",
+      }));
+
+      const scanner = dryRunScanner({ dryRunCommand: "install" });
+      scanner.scan(["install-test", "lodash"]);
+
+      // Should call with "install" instead of "install-test"
+      assert.equal(mockDryRunNpmCommandAndOutput.mock.callCount(), 1);
+      const calledArgs =
+        mockDryRunNpmCommandAndOutput.mock.calls[0].arguments[0];
+      assert.deepEqual(calledArgs, ["install", "lodash"]);
+    });
+
+    it("should skip scanning when hasDryRunArg returns true", async () => {
+      mockDryRunNpmCommandAndOutput.mock.resetCalls();
+      mockWriteError.mock.resetCalls();
+
+      const scanner = dryRunScanner();
+      const shouldScan = scanner.shouldScan(["install", "--dry-run"]);
+
+      assert.equal(shouldScan, false);
+      // Should not call dryRunNpmCommandAndOutput since scanning is skipped
+      assert.equal(mockDryRunNpmCommandAndOutput.mock.callCount(), 0);
+    });
+
+    it("should skip scanning when skipScanWhen returns true", async () => {
+      const scanner = dryRunScanner({
+        skipScanWhen: (args) => args.includes("--skip"),
+      });
+      const shouldScan = scanner.shouldScan(["install", "--skip"]);
+
+      assert.equal(shouldScan, false);
+    });
+
+    it("should scan when conditions are met", async () => {
+      const scanner = dryRunScanner();
+      const shouldScan = scanner.shouldScan(["install", "lodash"]);
+
+      assert.equal(shouldScan, true);
+    });
+  });
+});

packages/safe-chain/src/packagemanager/npm/runNpmCommand.js

@@ -18,7 +18,7 @@ export function runNpm(args) {
 
 export function dryRunNpmCommandAndOutput(args) {
   try {
-    const npmCommand = `npm ${args.join(" ")} --dry-run`;
+    const npmCommand = `npm ${args.join(" ")} --ignore-scripts --dry-run`;
     const output = execSync(npmCommand, { stdio: "pipe" });
     return { status: 0, output: output.toString() };
   } catch (error) {

packages/safe-chain/src/scanning/index.js

@@ -21,7 +21,9 @@ export async function scanCommand(args) {
 
   let timedOut = false;
 
-  const spinner = ui.startProcess("Scanning for malicious packages...");
+  const spinner = ui.startProcess(
+    "Safe-chain: Scanning for malicious packages..."
+  );
   let audit;
 
   await Promise.race([
@@ -37,12 +39,14 @@ export async function scanCommand(args) {
         }
 
         if (changes.length > 0) {
-          spinner.setText(`Scanning ${changes.length} package(s)...`);
+          spinner.setText(
+            `Safe-chain: Scanning ${changes.length} package(s)...`
+          );
         }
 
         audit = await auditChanges(changes);
       } catch (error) {
-        spinner.fail(`Error while scanning: ${error.message}`);
+        spinner.fail(`Safe-chain: Error while scanning.`);
         throw error;
       }
     })(),
@@ -52,12 +56,12 @@ export async function scanCommand(args) {
   ]);
 
   if (timedOut) {
-    spinner.fail("Timeout exceeded while scanning.");
+    spinner.fail("Safe-chain: Timeout exceeded while scanning.");
     throw new Error("Timeout exceeded while scanning npm install command.");
   }
 
   if (!audit || audit.isAllowed) {
-    spinner.succeed("No malicious packages detected.");
+    spinner.succeed("Safe-chain: No malicious packages detected.");
   } else {
     printMaliciousChanges(audit.disallowedChanges, spinner);
     await onMalwareFound();
@@ -65,7 +69,7 @@ export async function scanCommand(args) {
 }
 
 function printMaliciousChanges(changes, spinner) {
-  spinner.fail(chalk.bold("Malicious changes detected:"));
+  spinner.fail("Safe-chain: " + chalk.bold("Malicious changes detected:"));
 
   for (const change of changes) {
     ui.writeInformation(` - ${change.name}@${change.version}`);