From 87fcb7239a34a19d9fbd65b3f0bf75ec839cb729 Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Mon, 17 Nov 2025 10:03:38 -0800 Subject: [PATCH] Adapt per review --- .../src/packagemanager/pip/runPipCommand.js | 46 +++++++++---------- .../packagemanager/pip/runPipCommand.spec.js | 24 +++++----- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js index 309ee47..f37e9b0 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js @@ -32,8 +32,7 @@ export async function runPip(command, args) { const tmpDir = os.tmpdir(); const pipConfigPath = path.join(tmpDir, `safe-chain-pip-${Date.now()}.ini`); - if (!env.PIP_CONFIG_FILE) { - // Build pip config INI + if (!env.PIP_CONFIG_FILE) { // Build pip config INI /** @type {{ global: { cert: string, proxy?: string } }} */ const configObj = { global: { cert: combinedCaPath } }; if (proxy) { @@ -43,9 +42,7 @@ export async function runPip(command, args) { await fs.writeFile(pipConfigPath, pipConfig); env.PIP_CONFIG_FILE = pipConfigPath; - } else if (fsSync.existsSync(env.PIP_CONFIG_FILE)) { - // Existing pip config file present and exists on disk. - // Lets merge in our cert and proxy settings if not already present + } else if (fsSync.existsSync(env.PIP_CONFIG_FILE)) { // Merge pip config INI ui.writeVerbose("Safe-chain: Merging user provided PIP_CONFIG_FILE with safe-chain certificate and proxy settings."); const userConfig = env.PIP_CONFIG_FILE; @@ -56,24 +53,20 @@ export async function runPip(command, args) { // Ensure [global] section exists parsed.global = parsed.global || {}; - // Adding CERT and PROXY - // If either is already set, there's no neeed to throw an error - // MITM might fail and throw later if the proxy config is invalid - // Cert - if (typeof parsed.global.cert === "undefined") { - ui.writeVerbose("Safe-chain: Adding cert to temporary PIP_CONFIG_FILE."); - parsed.global.cert = combinedCaPath; + if (typeof parsed.global.cert !== "undefined") { + ui.writeWarning("Safe-chain: User defined cert found in PIP_CONFIG_FILE. It will be overwritten in the temporary config."); } + parsed.global.cert = combinedCaPath; // Proxy - if (typeof parsed.global.proxy === "undefined") { - if (proxy) { - ui.writeVerbose("Safe-chain: Adding proxy to temporary PIP_CONFIG_FILE."); - parsed.global.proxy = proxy; - } + if (typeof parsed.global.proxy !== "undefined") { + ui.writeWarning("Safe-chain: User defined proxy found in PIP_CONFIG_FILE. It will be overwritten in the temporary config."); } - + if (proxy) { + parsed.global.proxy = proxy; + } + const updated = ini.stringify(parsed); // Save to a new temp file to avoid overwriting user's original config @@ -86,15 +79,20 @@ export async function runPip(command, args) { } // REQUESTS_CA_BUNDLE, SSL_CERT_FILE and PIP_CERT as extra safety nets. - if (!env.REQUESTS_CA_BUNDLE) { - env.REQUESTS_CA_BUNDLE = combinedCaPath; + if (env.REQUESTS_CA_BUNDLE) { + ui.writeWarning("Safe-chain: User defined REQUESTS_CA_BUNDLE found in environment. It will be overwritten."); } - if (!env.SSL_CERT_FILE) { - env.SSL_CERT_FILE = combinedCaPath; + env.REQUESTS_CA_BUNDLE = combinedCaPath; + + if (env.SSL_CERT_FILE) { + ui.writeWarning("Safe-chain: User defined SSL_CERT_FILE found in environment. It will be overwritten."); } - if (!env.PIP_CERT) { - env.PIP_CERT = combinedCaPath; + env.SSL_CERT_FILE = combinedCaPath; + + if (env.PIP_CERT) { + ui.writeWarning("Safe-chain: User defined PIP_CERT found in environment. It will be overwritten."); } + env.PIP_CERT = combinedCaPath; const result = await safeSpawn(command, args, { stdio: "inherit", diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js index ecd8e0f..f67077a 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.spec.js @@ -217,10 +217,10 @@ describe("runPipCommand environment variable handling", () => { assert.strictEqual(originalParsed.global.cert, undefined, "original file should not gain cert"); assert.strictEqual(originalParsed.global.proxy, "http://original:9999", "original proxy remains"); - // New file merged: cert added (was missing), proxy preserved (was present) + // New file: cert and proxy always overwritten const newParsed = ini.parse(await fs.readFile(newCfgPath, "utf-8")); - assert.strictEqual(newParsed.global.cert, "/tmp/test-combined-ca.pem", "new cert injected"); - assert.strictEqual(newParsed.global.proxy, "http://original:9999", "existing proxy should be preserved in new file"); + assert.strictEqual(newParsed.global.cert, "/tmp/test-combined-ca.pem", "cert always overwritten in temp config"); + assert.strictEqual(newParsed.global.proxy, "http://localhost:8080", "proxy always overwritten in temp config"); customEnv = null; }); @@ -247,11 +247,11 @@ describe("runPipCommand environment variable handling", () => { assert.strictEqual(originalParsed.global.cert, "/path/to/existing.pem", "original cert preserved"); assert.strictEqual(originalParsed.global.proxy, "http://original:9999", "original proxy preserved"); - // New temp config preserves existing values (no override when already set) - const newContent = await fs.readFile(newCfgPath, "utf-8"); - const newParsed = ini.parse(newContent); - assert.strictEqual(newParsed.global.cert, "/path/to/existing.pem", "existing cert preserved in new temp config"); - assert.strictEqual(newParsed.global.proxy, "http://original:9999", "existing proxy preserved in new temp config"); + // New temp config: cert and proxy always overwritten + const newContent = await fs.readFile(newCfgPath, "utf-8"); + const newParsed = ini.parse(newContent); + assert.strictEqual(newParsed.global.cert, "/tmp/test-combined-ca.pem", "cert always overwritten in temp config"); + assert.strictEqual(newParsed.global.proxy, "http://localhost:8080", "proxy always overwritten in temp config"); customEnv = null; }); @@ -272,10 +272,10 @@ describe("runPipCommand environment variable handling", () => { assert.strictEqual(originalParsed.global.cert, "/path/to/existing.pem", "original cert unchanged"); assert.strictEqual(originalParsed.global.proxy, undefined, "original proxy still missing"); - // New file preserves existing cert and adds proxy (since it was missing) - const newParsed = ini.parse(await fs.readFile(newCfgPath, "utf-8")); - assert.strictEqual(newParsed.global.cert, "/path/to/existing.pem", "existing cert preserved (not overridden)"); - assert.strictEqual(newParsed.global.proxy, "http://localhost:8080", "proxy added from env"); + // New file: cert and proxy always overwritten + const newParsed = ini.parse(await fs.readFile(newCfgPath, "utf-8")); + assert.strictEqual(newParsed.global.cert, "/tmp/test-combined-ca.pem", "cert always overwritten in temp config"); + assert.strictEqual(newParsed.global.proxy, "http://localhost:8080", "proxy always overwritten in temp config"); customEnv = null; }); });