From 6b2db6dace79abde4c662ae403745990ecf506ab Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Fri, 24 Oct 2025 13:14:57 -0700 Subject: [PATCH] Fix ranges issue --- .../src/packagemanager/pip/runPipCommand.js | 6 +- .../safe-chain/src/scanning/audit/index.js | 1 + .../src/scanning/malwareDatabase.js | 3 +- packages/safe-chain/src/utils/safeSpawn.js | 32 +++++++++ .../safe-chain/src/utils/safeSpawn.spec.js | 70 +++++++++++++++++++ test/e2e/pip.e2e.spec.js | 20 ++++++ 6 files changed, 128 insertions(+), 4 deletions(-) diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js index 878d856..b09fc50 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js @@ -1,11 +1,11 @@ import { ui } from "../../environment/userInteraction.js"; -import { safeSpawn } from "../../utils/safeSpawn.js"; +import { safeSpawnPy } from "../../utils/safeSpawn.js"; import { mergeSafeChainProxyEnvironmentVariables } from "../../registryProxy/registryProxy.js"; export async function runPip(command, args) { try { - const result = await safeSpawn(command, args, { + const result = await safeSpawnPy(command, args, { stdio: "inherit", env: mergeSafeChainProxyEnvironmentVariables(process.env), }); @@ -24,7 +24,7 @@ export async function dryRunPipCommandAndOutput(command, args) { try { // Note: pip supports --dry-run for the "install" command only; "download" and "wheel" do not. // We don't mutate args here — callers should include --dry-run when appropriate. - const result = await safeSpawn( + const result = await safeSpawnPy( command, args, { diff --git a/packages/safe-chain/src/scanning/audit/index.js b/packages/safe-chain/src/scanning/audit/index.js index 215bfa0..e67ee0c 100644 --- a/packages/safe-chain/src/scanning/audit/index.js +++ b/packages/safe-chain/src/scanning/audit/index.js @@ -14,6 +14,7 @@ export async function auditChanges(changes) { ); for (const change of changes) { + console.log("**** Auditing change:", change); const malwarePackage = malwarePackages.find( (pkg) => pkg.name === change.name && pkg.version === change.version ); diff --git a/packages/safe-chain/src/scanning/malwareDatabase.js b/packages/safe-chain/src/scanning/malwareDatabase.js index b21733a..976b386 100644 --- a/packages/safe-chain/src/scanning/malwareDatabase.js +++ b/packages/safe-chain/src/scanning/malwareDatabase.js @@ -90,7 +90,8 @@ async function getMalwareDatabase() { function isMalwareStatus(status) { let malwareStatus = status.toUpperCase(); - return malwareStatus === MALWARE_STATUS_MALWARE; + return malwareStatus === MALWARE_STATUS_MALWARE + || malwareStatus === MALWARE_STATUS_TELEMETRY; } export const MALWARE_STATUS_OK = "OK"; diff --git a/packages/safe-chain/src/utils/safeSpawn.js b/packages/safe-chain/src/utils/safeSpawn.js index 253417c..1fcaef8 100644 --- a/packages/safe-chain/src/utils/safeSpawn.js +++ b/packages/safe-chain/src/utils/safeSpawn.js @@ -43,3 +43,35 @@ export async function safeSpawn(command, args, options = {}) { }); }); } + +/** + * To avoid any regression issues on the JS ecosystem, + * a py-friendly safeSpawn that avoids shell interpolation + * issues (e.g., '<', '>' in version specs). + * + * TL;DR: add support for shell::false + */ +export async function safeSpawnPy(command, args, options = {}) { + return new Promise((resolve) => { + const child = spawn(command, args, { ...options, shell: false }); + + let stdout = ""; + let stderr = ""; + + child.stdout?.on("data", (data) => { + stdout += data.toString(); + }); + + child.stderr?.on("data", (data) => { + stderr += data.toString(); + }); + + child.on("close", (code) => { + resolve({ status: code, stdout, stderr }); + }); + + child.on("error", (error) => { + resolve({ status: 1, stdout: "", stderr: error.message || String(error) }); + }); + }); +} diff --git a/packages/safe-chain/src/utils/safeSpawn.spec.js b/packages/safe-chain/src/utils/safeSpawn.spec.js index 6417084..847a009 100644 --- a/packages/safe-chain/src/utils/safeSpawn.spec.js +++ b/packages/safe-chain/src/utils/safeSpawn.spec.js @@ -87,3 +87,73 @@ describe("safeSpawn", () => { assert.strictEqual(spawnCalls[0].options.shell, true); }); }); + +describe("safeSpawnPy", () => { + let safeSpawnPy; + let spawnCalls = []; + + beforeEach(async () => { + spawnCalls = []; + + // Mock child_process for argument-array spawn signature + mock.module("child_process", { + namedExports: { + spawn: (command, args = [], options = {}) => { + spawnCalls.push({ command, args, options }); + const stdoutListeners = []; + const stderrListeners = []; + const stdout = { on: (event, cb) => { if (event === "data") stdoutListeners.push(cb); } }; + const stderr = { on: (event, cb) => { if (event === "data") stderrListeners.push(cb); } }; + const obj = { + stdout, + stderr, + on: (event, callback) => { + if (event === 'close') { + // Emit one chunk to stdout and stderr to verify piping works, then close with success + setTimeout(() => { + stdoutListeners.forEach((cb) => cb(Buffer.from("STDOUT-TEST"))); + stderrListeners.forEach((cb) => cb(Buffer.from(""))); + callback(0); + }, 0); + } + } + }; + return obj; + }, + }, + }); + + // Import after mocking; use a query to avoid ESM cache collisions with previous import + const safeSpawnModule = await import("./safeSpawn.js?py"); + safeSpawnPy = safeSpawnModule.safeSpawnPy; + }); + + afterEach(() => { + mock.reset(); + }); + + it("spawns without a shell and preserves args (inherit)", async () => { + const result = await safeSpawnPy("pip3", ["install", "Jinja2>=3.1,<3.2"], { stdio: "inherit" }); + + // Verifies no throw and status 0 + assert.strictEqual(result.status, 0); + + // Verify spawn signature + assert.strictEqual(spawnCalls.length, 1); + assert.strictEqual(spawnCalls[0].command, "pip3"); + assert.deepStrictEqual(spawnCalls[0].args, ["install", "Jinja2>=3.1,<3.2"]); + assert.strictEqual(spawnCalls[0].options.shell, false); + assert.strictEqual(spawnCalls[0].options.stdio, "inherit"); + }); + + it("captures stdout when stdio=pipe", async () => { + const result = await safeSpawnPy("pip3", ["install", "idna!=3.5,>=3.0", "--dry-run"], { stdio: "pipe" }); + + assert.strictEqual(result.status, 0); + assert.match(result.stdout || "", /STDOUT-TEST/); + + assert.strictEqual(spawnCalls.length, 1); + assert.strictEqual(spawnCalls[0].options.shell, false); + assert.strictEqual(spawnCalls[0].options.stdio, "pipe"); + }); +}); diff --git a/test/e2e/pip.e2e.spec.js b/test/e2e/pip.e2e.spec.js index 767c8fb..524c472 100644 --- a/test/e2e/pip.e2e.spec.js +++ b/test/e2e/pip.e2e.spec.js @@ -65,4 +65,24 @@ describe("E2E: pip coverage", () => { `Output did not include expected text. Output was:\n${result.output}` ); }); + + it(`pip3 install with extras such as requests[socks]`, async () => { + const shell = await container.openShell("zsh"); + const result = await shell.runCommand('pip3 install "requests[socks]==2.32.3"'); + + assert.ok( + result.output.includes("no malicious packages found."), + `Output did not include expected text. Output was:\n${result.output}` + ); + }); + + it(`pip3 install with range version specifier`, async () => { + const shell = await container.openShell("zsh"); + const result = await shell.runCommand('pip3 install "Jinja2>=3.1,<3.2"'); + + assert.ok( + result.output.includes("no malicious packages found."), + `Output did not include expected text. Output was:\n${result.output}` + ); + }); });