diff --git a/packages/safe-chain/bin/aikido-npm.js b/packages/safe-chain/bin/aikido-npm.js index d8b8c3e..0e9f302 100755 --- a/packages/safe-chain/bin/aikido-npm.js +++ b/packages/safe-chain/bin/aikido-npm.js @@ -1,21 +1,10 @@ #!/usr/bin/env node -import { execSync } from "child_process"; import { main } from "../src/main.js"; import { initializePackageManager } from "../src/packagemanager/currentPackageManager.js"; const packageManagerName = "npm"; -initializePackageManager(packageManagerName, getNpmVersion()); +initializePackageManager(packageManagerName); var exitCode = await main(process.argv.slice(2)); process.exit(exitCode); - -function getNpmVersion() { - try { - return execSync("npm --version").toString().trim(); - } catch { - // Default to 0.0.0 if npm is not found - // That way we don't use any unsupported features - return "0.0.0"; - } -} diff --git a/packages/safe-chain/bin/aikido-npx.js b/packages/safe-chain/bin/aikido-npx.js index 7f06c7c..d3dfdd6 100755 --- a/packages/safe-chain/bin/aikido-npx.js +++ b/packages/safe-chain/bin/aikido-npx.js @@ -4,7 +4,7 @@ import { main } from "../src/main.js"; import { initializePackageManager } from "../src/packagemanager/currentPackageManager.js"; const packageManagerName = "npx"; -initializePackageManager(packageManagerName, process.versions.node); +initializePackageManager(packageManagerName); var exitCode = await main(process.argv.slice(2)); process.exit(exitCode); diff --git a/packages/safe-chain/bin/aikido-pnpm.js b/packages/safe-chain/bin/aikido-pnpm.js index 7177159..0a06217 100755 --- a/packages/safe-chain/bin/aikido-pnpm.js +++ b/packages/safe-chain/bin/aikido-pnpm.js @@ -4,7 +4,7 @@ import { main } from "../src/main.js"; import { initializePackageManager } from "../src/packagemanager/currentPackageManager.js"; const packageManagerName = "pnpm"; -initializePackageManager(packageManagerName, process.versions.node); +initializePackageManager(packageManagerName); var exitCode = await main(process.argv.slice(2)); process.exit(exitCode); diff --git a/packages/safe-chain/bin/aikido-pnpx.js b/packages/safe-chain/bin/aikido-pnpx.js index 4bb6840..cdb6504 100755 --- a/packages/safe-chain/bin/aikido-pnpx.js +++ b/packages/safe-chain/bin/aikido-pnpx.js @@ -4,7 +4,7 @@ import { main } from "../src/main.js"; import { initializePackageManager } from "../src/packagemanager/currentPackageManager.js"; const packageManagerName = "pnpx"; -initializePackageManager(packageManagerName, process.versions.node); +initializePackageManager(packageManagerName); var exitCode = await main(process.argv.slice(2)); process.exit(exitCode); diff --git a/packages/safe-chain/bin/aikido-yarn.js b/packages/safe-chain/bin/aikido-yarn.js index 002a956..fd87606 100755 --- a/packages/safe-chain/bin/aikido-yarn.js +++ b/packages/safe-chain/bin/aikido-yarn.js @@ -4,7 +4,7 @@ import { main } from "../src/main.js"; import { initializePackageManager } from "../src/packagemanager/currentPackageManager.js"; const packageManagerName = "yarn"; -initializePackageManager(packageManagerName, process.versions.node); +initializePackageManager(packageManagerName); var exitCode = await main(process.argv.slice(2)); process.exit(exitCode); diff --git a/packages/safe-chain/src/packagemanager/currentPackageManager.js b/packages/safe-chain/src/packagemanager/currentPackageManager.js index 2a10d86..2f019a1 100644 --- a/packages/safe-chain/src/packagemanager/currentPackageManager.js +++ b/packages/safe-chain/src/packagemanager/currentPackageManager.js @@ -14,9 +14,9 @@ const state = { packageManagerName: null, }; -export function initializePackageManager(packageManagerName, version) { +export function initializePackageManager(packageManagerName) { if (packageManagerName === "npm") { - state.packageManagerName = createNpmPackageManager(version); + state.packageManagerName = createNpmPackageManager(); } else if (packageManagerName === "npx") { state.packageManagerName = createNpxPackageManager(); } else if (packageManagerName === "yarn") { diff --git a/packages/safe-chain/src/packagemanager/npm/createPackageManager.js b/packages/safe-chain/src/packagemanager/npm/createPackageManager.js index bf38209..731f406 100644 --- a/packages/safe-chain/src/packagemanager/npm/createPackageManager.js +++ b/packages/safe-chain/src/packagemanager/npm/createPackageManager.js @@ -1,34 +1,27 @@ import { commandArgumentScanner } from "./dependencyScanner/commandArgumentScanner.js"; -import { dryRunScanner } from "./dependencyScanner/dryRunScanner.js"; import { nullScanner } from "./dependencyScanner/nullScanner.js"; import { runNpm } from "./runNpmCommand.js"; import { getNpmCommandForArgs, npmInstallCommand, - npmCiCommand, - npmInstallTestCommand, - npmInstallCiTestCommand, npmUpdateCommand, - npmAuditCommand, npmExecCommand, } from "./utils/npmCommands.js"; -export function createNpmPackageManager(version) { - // From npm v10.4.0 onwards, the npm commands output detailed information - // when using the --dry-run flag. - // We use that information to scan for dependency changes. - // For older versions of npm we have to rely on parsing the command arguments. - const supportedScanners = isPriorToNpm10_4(version) - ? npm10_3AndBelowSupportedScanners - : npm10_4AndAboveSupportedScanners; - +export function createNpmPackageManager() { function isSupportedCommand(args) { - const scanner = findDependencyScannerForCommand(supportedScanners, args); + const scanner = findDependencyScannerForCommand( + commandScannerMapping, + args + ); return scanner.shouldScan(args); } function getDependencyUpdatesForCommand(args) { - const scanner = findDependencyScannerForCommand(supportedScanners, args); + const scanner = findDependencyScannerForCommand( + commandScannerMapping, + args + ); return scanner.scan(args); } @@ -39,40 +32,12 @@ export function createNpmPackageManager(version) { }; } -const npm10_4AndAboveSupportedScanners = { - [npmInstallCommand]: dryRunScanner(), - [npmUpdateCommand]: dryRunScanner(), - [npmCiCommand]: dryRunScanner(), - [npmAuditCommand]: dryRunScanner({ - skipScanWhen: (args) => !args.includes("fix"), - }), - [npmExecCommand]: commandArgumentScanner({ ignoreDryRun: true }), // exec command doesn't support dry-run - - // Running dry-run on install-test and install-ci-test will install & run tests. - // We only want to know if there are changes in the dependencies. - // So we run change the dry-run command to only check the install. - [npmInstallTestCommand]: dryRunScanner({ dryRunCommand: npmInstallCommand }), - [npmInstallCiTestCommand]: dryRunScanner({ dryRunCommand: npmCiCommand }), -}; - -const npm10_3AndBelowSupportedScanners = { +const commandScannerMapping = { [npmInstallCommand]: commandArgumentScanner(), [npmUpdateCommand]: commandArgumentScanner(), [npmExecCommand]: commandArgumentScanner({ ignoreDryRun: true }), // exec command doesn't support dry-run }; -function isPriorToNpm10_4(version) { - try { - const [major, minor] = version.split(".").map(Number); - if (major < 10) return true; - if (major === 10 && minor < 4) return true; - return false; - } catch { - // Default to true: if version parsing fails, assume it's an older version - return true; - } -} - function findDependencyScannerForCommand(scanners, args) { const command = getNpmCommandForArgs(args); if (!command) { diff --git a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js b/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js deleted file mode 100644 index 6189b2f..0000000 --- a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.js +++ /dev/null @@ -1,67 +0,0 @@ -import { parseDryRunOutput } from "../parsing/parseNpmInstallDryRunOutput.js"; -import { dryRunNpmCommandAndOutput } from "../runNpmCommand.js"; -import { hasDryRunArg } from "../utils/npmCommands.js"; - -export function dryRunScanner(scannerOptions) { - return { - scan: (args) => scanDependencies(scannerOptions, args), - shouldScan: (args) => shouldScanDependencies(scannerOptions, args), - }; -} - -function scanDependencies(scannerOptions, args) { - let dryRunArgs = args; - - if (scannerOptions?.dryRunCommand) { - // Replace the first argument with the dryRunCommand (eg: "install" instead of "install-test") - dryRunArgs = [scannerOptions.dryRunCommand, ...args.slice(1)]; - } - - return checkChangesWithDryRun(dryRunArgs); -} - -function shouldScanDependencies(scannerOptions, args) { - if (hasDryRunArg(args)) { - return false; - } - - if (scannerOptions?.skipScanWhen && scannerOptions.skipScanWhen(args)) { - return false; - } - - return true; -} - -async function checkChangesWithDryRun(args) { - const dryRunOutput = await dryRunNpmCommandAndOutput(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 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) { - throw new Error( - `Dry-run command failed with exit code ${dryRunOutput.status} and produced no output.` - ); - } - - const parsedOutput = parseDryRunOutput(dryRunOutput.output); - - // 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 deleted file mode 100644 index 88d7681..0000000 --- a/packages/safe-chain/src/packagemanager/npm/dependencyScanner/dryRunScanner.spec.js +++ /dev/null @@ -1,139 +0,0 @@ -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 = await 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(); - - await assert.rejects(async () => { - await 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 = await 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(); - - await assert.rejects(async () => { - await 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" }); - await 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/parsing/parseNpmInstallDryRunOutput.js b/packages/safe-chain/src/packagemanager/npm/parsing/parseNpmInstallDryRunOutput.js deleted file mode 100644 index 3c1e673..0000000 --- a/packages/safe-chain/src/packagemanager/npm/parsing/parseNpmInstallDryRunOutput.js +++ /dev/null @@ -1,57 +0,0 @@ -export function parseDryRunOutput(output) { - const lines = output.split(/\r?\n/); - const packageChanges = []; - - for (const line of lines) { - if (line.startsWith("add ")) { - packageChanges.push(parseAdd(line)); - } else if (line.startsWith("remove ")) { - packageChanges.push(parseRemove(line)); - } else if (line.startsWith("change ")) { - packageChanges.push(parseChange(line)); - } - } - - return packageChanges; -} - -function parseAdd(line) { - const splitLine = getLineParts(line); - const packageName = splitLine[1]; - const packageVersion = splitLine[splitLine.length - 1]; - return addedPackage(packageName, packageVersion); -} - -function addedPackage(name, version) { - return { type: "add", name, version }; -} - -function parseRemove(line) { - const splitLine = getLineParts(line); - const packageName = splitLine[1]; - const packageVersion = splitLine[splitLine.length - 1]; - return removedPackage(packageName, packageVersion); -} - -function removedPackage(name, version) { - return { type: "remove", name, version }; -} - -function parseChange(line) { - const splitLine = getLineParts(line); - const packageName = splitLine[1]; - const packageVersion = splitLine[splitLine.length - 1]; - const oldVersion = splitLine[2]; - return changedPackage(packageName, packageVersion, oldVersion); -} - -function getLineParts(line) { - return line - .split(" ") - .map((part) => part.trim()) - .filter((part) => part !== ""); -} - -function changedPackage(name, version, oldVersion) { - return { type: "change", name, version, oldVersion }; -} diff --git a/packages/safe-chain/src/packagemanager/npm/parsing/parseNpmInstallDryRunOutput.spec.js b/packages/safe-chain/src/packagemanager/npm/parsing/parseNpmInstallDryRunOutput.spec.js deleted file mode 100644 index cd7c2b1..0000000 --- a/packages/safe-chain/src/packagemanager/npm/parsing/parseNpmInstallDryRunOutput.spec.js +++ /dev/null @@ -1,134 +0,0 @@ -import { describe, it } from "node:test"; -import assert from "node:assert"; -import { parseDryRunOutput } from "./parseNpmInstallDryRunOutput.js"; - -describe("parseNpmInstallDryRunOutput", () => { - it("should parse added packages", () => { - const output = ` -add @jest/transform 29.7.0 -add @jest/test-result 29.7.0 -add @jest/reporters 29.7.0 -add @jest/console 29.7.0 -add jest-cli 29.7.0 -add import-local 3.2.0 -add @jest/types 29.6.3 -add @jest/core 29.7.0 -add jest 29.7.0 - -added 267 packages in 831ms - -32 packages are looking for funding - run \`npm fund\` for details`; - - const expected = [ - { name: "@jest/transform", version: "29.7.0", type: "add" }, - { name: "@jest/test-result", version: "29.7.0", type: "add" }, - { name: "@jest/reporters", version: "29.7.0", type: "add" }, - { name: "@jest/console", version: "29.7.0", type: "add" }, - { name: "jest-cli", version: "29.7.0", type: "add" }, - { name: "import-local", version: "3.2.0", type: "add" }, - { name: "@jest/types", version: "29.6.3", type: "add" }, - { name: "@jest/core", version: "29.7.0", type: "add" }, - { name: "jest", version: "29.7.0", type: "add" }, - ]; - - const result = parseDryRunOutput(output); - - assert.deepEqual(result, expected); - }); - - it("should parse removed packages", () => { - const output = ` -remove react 19.1.0 - - removed 1 package in 115ms`; - - const expected = [{ name: "react", version: "19.1.0", type: "remove" }]; - - const result = parseDryRunOutput(output); - - assert.deepEqual(result, expected); - }); - - it("should parse changed packages", () => { - const output = ` -change react 19.0.0 => 19.1.0 - -changed 1 package in 204ms`; - - const expected = [ - { - name: "react", - version: "19.1.0", - oldVersion: "19.0.0", - type: "change", - }, - ]; - - const result = parseDryRunOutput(output); - - assert.deepEqual(result, expected); - }); - - it("should parse mixed package changes", () => { - const output = ` -add @jest/transform 29.7.0 -add @jest/test-result 29.7.0 -add @jest/reporters 29.7.0 -add @jest/console 29.7.0 -add jest-cli 29.7.0 -add import-local 3.2.0 -add @jest/types 29.6.3 -add @jest/core 29.7.0 -add jest 29.7.0 -remove react 19.1.0 -change lodash 4.17.0 => 4.18.0 - -removed 1 package in 115ms`; - - const expected = [ - { name: "@jest/transform", version: "29.7.0", type: "add" }, - { name: "@jest/test-result", version: "29.7.0", type: "add" }, - { name: "@jest/reporters", version: "29.7.0", type: "add" }, - { name: "@jest/console", version: "29.7.0", type: "add" }, - { name: "jest-cli", version: "29.7.0", type: "add" }, - { name: "import-local", version: "3.2.0", type: "add" }, - { name: "@jest/types", version: "29.6.3", type: "add" }, - { name: "@jest/core", version: "29.7.0", type: "add" }, - { name: "jest", version: "29.7.0", type: "add" }, - { name: "react", version: "19.1.0", type: "remove" }, - { - name: "lodash", - version: "4.18.0", - oldVersion: "4.17.0", - type: "change", - }, - ]; - - const result = parseDryRunOutput(output); - - assert.deepEqual(result, expected); - }); - - it("should work with npm v22.0.0", () => { - const output = ` -add @jest/types 29.6.3 -add @jest/core 29.7.0 -add jest 29.7.0 - -added 257 packages in 791ms - -44 packages are looking for funding - run \`npm fund\` for details`; - - const expected = [ - { name: "@jest/types", version: "29.6.3", type: "add" }, - { name: "@jest/core", version: "29.7.0", type: "add" }, - { name: "jest", version: "29.7.0", type: "add" }, - ]; - - const result = parseDryRunOutput(output); - - assert.deepEqual(result, expected); - }); -}); diff --git a/test/e2e/npm.e2e.spec.js b/test/e2e/npm.e2e.spec.js index c744835..ba836e7 100644 --- a/test/e2e/npm.e2e.spec.js +++ b/test/e2e/npm.e2e.spec.js @@ -62,48 +62,24 @@ describe("E2E: npm coverage", () => { it(`safe-chain blocks download of malicious packages already in package.json`, async () => { const shell = await container.openShell("zsh"); - const npmVersion = (await shell.runCommand("npm --version")).output.trim(); - const majorVersion = parseInt(npmVersion.split(".")[0]); - const minorVersion = parseInt(npmVersion.split(".")[1]); - const isBelow10_4 = - majorVersion < 10 || (majorVersion === 10 && minorVersion < 4); await shell.runCommand( 'echo \'{"name":"test-project","version":"1.0.0","dependencies":{"safe-chain-test":"0.0.1-security"}}\' > package.json' ); var result = await shell.runCommand("npm install"); - if (isBelow10_4) { - assert.ok( - result.output.includes("blocked 1 malicious package downloads"), - `Output did not include expected text. Output was:\n${result.output}` - ); - assert.ok( - result.output.includes("- safe-chain-test"), - `Output did not include expected text. Output was:\n${result.output}` - ); - assert.ok( - result.output.includes( - "Exiting without installing malicious packages." - ), - `Output did not include expected text. Output was:\n${result.output}` - ); - } else { - assert.ok( - result.output.includes("Malicious changes detected:"), - `Output did not include expected text. Output was:\n${result.output}` - ); - assert.ok( - result.output.includes("- safe-chain-test"), - `Output did not include expected text. Output was:\n${result.output}` - ); - assert.ok( - result.output.includes( - "Exiting without installing malicious packages." - ), - `Output did not include expected text. Output was:\n${result.output}` - ); - } + assert.ok( + result.output.includes("blocked 1 malicious package downloads"), + `Output did not include expected text. Output was:\n${result.output}` + ); + assert.ok( + result.output.includes("- safe-chain-test"), + `Output did not include expected text. Output was:\n${result.output}` + ); + assert.ok( + result.output.includes("Exiting without installing malicious packages."), + `Output did not include expected text. Output was:\n${result.output}` + ); }); it("safe-chain blocks npx from executing malicious packages", async () => {