From a4f9f590a49bcccf3cff10271f68bb24fcc857c0 Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Tue, 2 Dec 2025 08:31:47 -0800 Subject: [PATCH 1/3] Don't modify config for config related commands --- .../src/packagemanager/pip/runPipCommand.js | 23 +++ .../packagemanager/pip/runPipCommand.spec.js | 97 +++++++++++ test/e2e/pip.e2e.spec.js | 153 ++++++++++++++++++ 3 files changed, 273 insertions(+) diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js index 23485ff..37cfe76 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js @@ -46,6 +46,9 @@ function setFallbackCaBundleEnvironmentVariables(env, combinedCaPath) { * If the user has an existing PIP_CONFIG_FILE, a new temporary config is created that merges * their settings with safe-chain's, leaving the original file unchanged. * + * Special handling for 'pip config' commands: PIP_CONFIG_FILE is NOT overridden to allow + * users to read/write persistent config. Only CA environment variables are set for these commands. + * * @param {string} command - The pip command to execute (e.g., 'pip3') * @param {string[]} args - Command line arguments to pass to pip * @returns {Promise<{status: number}>} Exit status of the pip command @@ -59,6 +62,12 @@ export async function runPip(command, args) { // validates correctly under both MITM'd and tunneled HTTPS. const combinedCaPath = getCombinedCaBundlePath(); + // Commands that need access to persistent config/cache/state files + // These should not have PIP_CONFIG_FILE overridden as it would prevent them from + // reading/writing to the user's actual pip configuration and cache directories + const configRelatedCommands = ['config', 'cache', 'debug', 'completion']; + const isConfigRelatedCommand = args.length > 0 && configRelatedCommands.includes(args[0]); + // https://pip.pypa.io/en/stable/topics/https-certificates/ explains that the 'cert' param (which we're providing via INI file) // will tell pip to use the provided CA bundle for HTTPS verification. @@ -70,6 +79,20 @@ export async function runPip(command, args) { const pipConfigPath = path.join(tmpDir, `safe-chain-pip-${Date.now()}.ini`); let cleanupConfigPath = null; // Track temp file for cleanup + // For config-related commands, skip PIP_CONFIG_FILE override to allow persistent config/cache access + // Only set fallback CA environment variables which don't interfere with config operations + if (isConfigRelatedCommand) { + ui.writeVerbose(`Safe-chain: Skipping PIP_CONFIG_FILE override for 'pip ${args[0]}' command to allow persistent config/cache access.`); + setFallbackCaBundleEnvironmentVariables(env, combinedCaPath); + + const result = await safeSpawn(command, args, { + stdio: "inherit", + env, + }); + + return { status: result.status }; + } + // Note: Setting PIP_CONFIG_FILE overrides all pip config levels (Global/User/Site) per pip's loading order if (!env.PIP_CONFIG_FILE) { /** @type {{ global: { cert: string, proxy?: string } }} */ diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js index d0df961..cf121f6 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js @@ -62,6 +62,103 @@ describe("runPipCommand environment variable handling", () => { mock.reset(); }); + it("should NOT set PIP_CONFIG_FILE for 'pip config' commands to allow persistent config access", async () => { + const res = await runPip("pip3", ["config", "set", "global.index-url", "https://test.pypi.org/simple"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + // PIP_CONFIG_FILE should NOT be set for config commands + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip config commands" + ); + + // But CA environment variables should still be set + assert.strictEqual( + capturedArgs.options.env.REQUESTS_CA_BUNDLE, + "/tmp/test-combined-ca.pem", + "REQUESTS_CA_BUNDLE should still be set" + ); + assert.strictEqual( + capturedArgs.options.env.SSL_CERT_FILE, + "/tmp/test-combined-ca.pem", + "SSL_CERT_FILE should still be set" + ); + assert.strictEqual( + capturedArgs.options.env.PIP_CERT, + "/tmp/test-combined-ca.pem", + "PIP_CERT should still be set" + ); + }); + + it("should NOT set PIP_CONFIG_FILE for 'pip config get' commands", async () => { + const res = await runPip("pip3", ["config", "get", "global.index-url"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip config get" + ); + }); + + it("should NOT set PIP_CONFIG_FILE for 'pip config list' commands", async () => { + const res = await runPip("pip3", ["config", "list"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip config list" + ); + }); + + it("should NOT set PIP_CONFIG_FILE for 'pip cache' commands", async () => { + const res = await runPip("pip3", ["cache", "dir"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip cache commands" + ); + + // CA env vars should still be set + assert.strictEqual( + capturedArgs.options.env.SSL_CERT_FILE, + "/tmp/test-combined-ca.pem", + "SSL_CERT_FILE should still be set" + ); + }); + + it("should NOT set PIP_CONFIG_FILE for 'pip debug' commands", async () => { + const res = await runPip("pip3", ["debug"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip debug" + ); + }); + + it("should NOT set PIP_CONFIG_FILE for 'pip completion' commands", async () => { + const res = await runPip("pip3", ["completion", "--bash"]); + assert.strictEqual(res.status, 0); + assert.ok(capturedArgs, "safeSpawn should have been called"); + + assert.strictEqual( + capturedArgs.options.env.PIP_CONFIG_FILE, + undefined, + "PIP_CONFIG_FILE should NOT be set for pip completion" + ); + }); + it("should set PIP_CERT env var and create config file", async () => { const res = await runPip("pip3", ["install", "requests"]); assert.strictEqual(res.status, 0); diff --git a/test/e2e/pip.e2e.spec.js b/test/e2e/pip.e2e.spec.js index 9a1adec..fa5260c 100644 --- a/test/e2e/pip.e2e.spec.js +++ b/test/e2e/pip.e2e.spec.js @@ -336,4 +336,157 @@ describe("E2E: pip coverage", () => { `Output did not include expected text. Output was:\n${result.output}` ); }); + + it(`pip3 config set should work and persist configuration`, async () => { + const shell = await container.openShell("zsh"); + + // Set a config value + const setResult = await shell.runCommand( + "pip3 config set global.timeout 60" + ); + + assert.ok( + setResult.output.includes("Writing to"), + `pip3 config set should write config. Output was:\n${setResult.output}` + ); + + // Verify it was persisted by reading it back + const getResult = await shell.runCommand( + "pip3 config get global.timeout" + ); + + assert.ok( + getResult.output.includes("60"), + `Config value should be 60. Output was:\n${getResult.output}` + ); + }); + + it(`pip3 config list should show user configuration`, async () => { + const shell = await container.openShell("zsh"); + + // Set a value first + await shell.runCommand("pip3 config set global.timeout 90"); + + // List config + const listResult = await shell.runCommand("pip3 config list"); + + assert.ok( + listResult.output.includes("timeout") && listResult.output.includes("90"), + `Config list should show timeout=90. Output was:\n${listResult.output}` + ); + }); + + it(`pip3 config unset should remove configuration`, async () => { + const shell = await container.openShell("zsh"); + + // Set a value + await shell.runCommand("pip3 config set global.timeout 120"); + + // Verify it exists + const getResult = await shell.runCommand("pip3 config get global.timeout"); + assert.ok(getResult.output.includes("120")); + + // Unset it + const unsetResult = await shell.runCommand("pip3 config unset global.timeout"); + assert.ok( + unsetResult.output.includes("Writing to"), + `pip3 config unset should write config. Output was:\n${unsetResult.output}` + ); + }); + + it(`pip3 cache dir should return cache directory path`, async () => { + const shell = await container.openShell("zsh"); + + const result = await shell.runCommand("pip3 cache dir"); + + // Should output a directory path + assert.ok( + result.output.includes("/") && result.output.includes("cache"), + `Should output a cache directory path. Output was:\n${result.output}` + ); + }); + + it(`pip3 cache info should show cache information`, async () => { + const shell = await container.openShell("zsh"); + + // Install something first to populate cache + await shell.runCommand("pip3 install --break-system-packages certifi"); + + const result = await shell.runCommand("pip3 cache info"); + + // Output should contain cache-related information + assert.ok( + result.output.match(/cache|wheel|http/i), + `Should output cache information. Output was:\n${result.output}` + ); + }); + + it(`pip3 cache list should list cached packages`, async () => { + const shell = await container.openShell("zsh"); + + // Download a package to ensure something is in cache + await shell.runCommand("pip3 download certifi"); + + const result = await shell.runCommand("pip3 cache list certifi"); + + // Should show either cached wheels or "No locally built wheels" + assert.ok( + result.output.includes("certifi") || result.output.includes("No locally built"), + `Should output cache list information. Output was:\n${result.output}` + ); + }); + + it(`pip3 debug should output debug information`, async () => { + const shell = await container.openShell("zsh"); + + const result = await shell.runCommand("pip3 debug"); + + // Should contain debug information about pip environment + assert.ok( + result.output.match(/pip version|sys\.version|sys\.executable/i), + `Should output debug information. Output was:\n${result.output}` + ); + + // Should NOT show safe-chain's temporary config file in the debug output + assert.ok( + !result.output.includes("safe-chain-pip-"), + `Debug output should not reference safe-chain temp config. Output was:\n${result.output}` + ); + }); + + it(`pip3 completion should generate shell completion script`, async () => { + const shell = await container.openShell("zsh"); + + const result = await shell.runCommand("pip3 completion --zsh"); + + // Should output shell completion code + assert.ok( + result.output.includes("compdef") || result.output.includes("_pip") || result.output.includes("pip completion"), + `Should output completion code. Output was:\n${result.output}` + ); + }); + + it(`pip3 install still works after config operations`, async () => { + const shell = await container.openShell("zsh"); + + // Perform config operations + await shell.runCommand("pip3 config set global.timeout 60"); + await shell.runCommand("pip3 cache dir"); + + // Now install should still work with malware protection + const result = await shell.runCommand( + "pip3 install --break-system-packages certifi" + ); + + assert.ok( + result.output.includes("Successfully installed") || + result.output.includes("Requirement already satisfied"), + `Install should succeed after config operations. Output was:\n${result.output}` + ); + + assert.ok( + result.output.includes("no malware found."), + `Should still scan for malware. Output was:\n${result.output}` + ); + }); }); From 795e7af23e1daffea58860c5470cc86d711c342f Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Tue, 2 Dec 2025 08:44:43 -0800 Subject: [PATCH 2/3] Clean up comments --- .../safe-chain/src/packagemanager/pip/runPipCommand.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js index 37cfe76..f7050a5 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js @@ -46,7 +46,7 @@ function setFallbackCaBundleEnvironmentVariables(env, combinedCaPath) { * If the user has an existing PIP_CONFIG_FILE, a new temporary config is created that merges * their settings with safe-chain's, leaving the original file unchanged. * - * Special handling for 'pip config' commands: PIP_CONFIG_FILE is NOT overridden to allow + * Special handling for commands that modify config/cache/state: PIP_CONFIG_FILE is NOT overridden to allow * users to read/write persistent config. Only CA environment variables are set for these commands. * * @param {string} command - The pip command to execute (e.g., 'pip3') @@ -79,10 +79,12 @@ export async function runPip(command, args) { const pipConfigPath = path.join(tmpDir, `safe-chain-pip-${Date.now()}.ini`); let cleanupConfigPath = null; // Track temp file for cleanup - // For config-related commands, skip PIP_CONFIG_FILE override to allow persistent config/cache access - // Only set fallback CA environment variables which don't interfere with config operations if (isConfigRelatedCommand) { ui.writeVerbose(`Safe-chain: Skipping PIP_CONFIG_FILE override for 'pip ${args[0]}' command to allow persistent config/cache access.`); + + // Still set the fallback CA bundle environment variables to avoid edge cases where a + // plugin or extension triggers a network call during config introspection + // This can do no harm setFallbackCaBundleEnvironmentVariables(env, combinedCaPath); const result = await safeSpawn(command, args, { From 20e63a58be225379844495d64191c796cc592e7e Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Tue, 2 Dec 2025 09:45:04 -0800 Subject: [PATCH 3/3] Add a better e2e test to cover the issue --- test/e2e/pip.e2e.spec.js | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/test/e2e/pip.e2e.spec.js b/test/e2e/pip.e2e.spec.js index fa5260c..26ebb78 100644 --- a/test/e2e/pip.e2e.spec.js +++ b/test/e2e/pip.e2e.spec.js @@ -489,4 +489,54 @@ describe("E2E: pip coverage", () => { `Should still scan for malware. Output was:\n${result.output}` ); }); + + it(`pip3 download works after configuring pip settings`, async () => { + const shell = await container.openShell("zsh"); + + // Configure pip with timeout and extra index URL + const configTimeout = await shell.runCommand("pip3 config set global.timeout 60"); + assert.ok( + configTimeout.output.includes("Writing to"), + `Config set should succeed. Output was:\n${configTimeout.output}` + ); + + const configIndex = await shell.runCommand( + "pip3 config set global.extra-index-url https://pypi.org/simple" + ); + assert.ok( + configIndex.output.includes("Writing to"), + `Config set should succeed. Output was:\n${configIndex.output}` + ); + + // Verify config persisted + const listConfig = await shell.runCommand("pip3 config list"); + assert.ok( + listConfig.output.includes("timeout") && listConfig.output.includes("60"), + `Config should show timeout=60. Output was:\n${listConfig.output}` + ); + assert.ok( + listConfig.output.includes("extra-index-url") && listConfig.output.includes("pypi.org"), + `Config should show extra-index-url. Output was:\n${listConfig.output}` + ); + + // Now download packages with the configured settings + const downloadResult = await shell.runCommand( + "pip3 download -d /tmp/packages requests certifi" + ); + + assert.ok( + downloadResult.output.includes("no malware found."), + `Should scan for malware. Output was:\n${downloadResult.output}` + ); + + // Verify downloads succeeded + assert.ok( + downloadResult.output.includes("Saved") || downloadResult.output.includes("requests"), + `Download should succeed with configured settings. Output was:\n${downloadResult.output}` + ); + assert.ok( + downloadResult.output.includes("certifi"), + `Should download certifi. Output was:\n${downloadResult.output}` + ); + }); });