diff --git a/packages/safe-chain/src/main.js b/packages/safe-chain/src/main.js index 501dd95..81b1b3a 100644 --- a/packages/safe-chain/src/main.js +++ b/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); diff --git a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js b/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js index cd7f251..0db23cb 100644 --- a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js +++ b/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."); - return []; + throw new Error( + `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"; +} diff --git a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.spec.js b/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.spec.js new file mode 100644 index 0000000..4fb6272 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/packages/safe-chain/src/packagemanager/npm/runNpmCommand.js b/packages/safe-chain/src/packagemanager/npm/runNpmCommand.js index 97390da..70a0d17 100644 --- a/packages/safe-chain/src/packagemanager/npm/runNpmCommand.js +++ b/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) { diff --git a/packages/safe-chain/src/scanning/index.js b/packages/safe-chain/src/scanning/index.js index 2905bd6..48a3e3a 100644 --- a/packages/safe-chain/src/scanning/index.js +++ b/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}`);