From 7b5a70065567ebe54f757e89f4a8f1df71cca1dd Mon Sep 17 00:00:00 2001 From: Reinier Criel Date: Mon, 8 Dec 2025 15:18:06 -0800 Subject: [PATCH] Fix some issues --- .../src/packagemanager/pip/runPipCommand.js | 2 +- .../src/registryProxy/certBundle.js | 64 +++++---------- .../src/registryProxy/certBundle.spec.js | 82 ++++++++++++------- .../src/registryProxy/registryProxy.js | 9 +- 4 files changed, 78 insertions(+), 79 deletions(-) diff --git a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js index dc9a1ad..e9f05c7 100644 --- a/packages/safe-chain/src/packagemanager/pip/runPipCommand.js +++ b/packages/safe-chain/src/packagemanager/pip/runPipCommand.js @@ -93,7 +93,7 @@ export async function runPip(command, args) { try { const env = mergeSafeChainProxyEnvironmentVariables(process.env); - // Always provide Python with a complete CA bundle (Safe Chain CA + Mozilla + Node built-in roots) + // Always provide Python with a complete CA bundle (Safe Chain CA + Mozilla + Node built-in roots + user certs) // so that any network request made by pip, including those outside explicit CLI args, // validates correctly under both MITM'd and tunneled HTTPS. const combinedCaPath = getCombinedCaBundlePath(); diff --git a/packages/safe-chain/src/registryProxy/certBundle.js b/packages/safe-chain/src/registryProxy/certBundle.js index 98810d6..ab0ac63 100644 --- a/packages/safe-chain/src/registryProxy/certBundle.js +++ b/packages/safe-chain/src/registryProxy/certBundle.js @@ -48,16 +48,16 @@ function isParsable(pem) { let cachedPath = null; /** - * Build a combined CA bundle for Python and Node HTTPS flows. - * - Includes Safe Chain CA (for MITM of known registries) - * - Includes Mozilla roots via npm `certifi` (public HTTPS) - * - Includes Node's built-in root certificates as a portable fallback + * Build a combined CA bundle. + * Automatically includes: + * - Safe Chain CA (for MITM of known registries) + * - Mozilla roots via certifi (for public HTTPS) + * - Node's built-in root certificates (fallback) + * - User's custom certificates (if NODE_EXTRA_CA_CERTS environment variable is set) + * * @returns {string} Path to the combined CA bundle PEM file */ export function getCombinedCaBundlePath() { - if (cachedPath && fs.existsSync(cachedPath)) return cachedPath; - - // Concatenate PEM files const parts = []; // 1) Safe Chain CA (for MITM'd registries) @@ -90,11 +90,23 @@ export function getCombinedCaBundlePath() { // Ignore if unavailable } + // 4) User's NODE_EXTRA_CA_CERTS (if set) + const userCertPath = process.env.NODE_EXTRA_CA_CERTS; + if (userCertPath) { + const userPem = readUserCertificateFile(userCertPath); + if (userPem) { + parts.push(userPem.trim()); + ui.writeVerbose(`Safe-chain: Merging user's NODE_EXTRA_CA_CERTS from ${userCertPath}`); + } else { + ui.writeWarning(`Safe-chain: Could not read or parse user's NODE_EXTRA_CA_CERTS from ${userCertPath}`); + } + } + const combined = parts.filter(Boolean).join("\n"); - const target = path.join(os.tmpdir(), "safe-chain-ca-bundle.pem"); + const target = path.join(os.tmpdir(), `safe-chain-ca-bundle-${Date.now()}.pem`); fs.writeFileSync(target, combined, { encoding: "utf8" }); cachedPath = target; - return cachedPath; + return target; } /** @@ -166,38 +178,4 @@ function readUserCertificateFile(certPath) { } } -/** - * Combine user's existing NODE_EXTRA_CA_CERTS with Safe Chain's CA certificate. - * If user has NODE_EXTRA_CA_CERTS set, it's merged with Safe Chain CA. - * - * @param {string | undefined} userCertPath - User's existing NODE_EXTRA_CA_CERTS path (if any) - * @returns {string} Path to the final CA bundle - */ -export function getCombinedCaBundlePathWithUserCerts(userCertPath) { - const parts = []; - // 1) Safe Chain CA - const safeChainPath = getCaCertPath(); - try { - const safeChainPem = fs.readFileSync(safeChainPath, "utf8"); - if (isParsable(safeChainPem)) parts.push(safeChainPem.trim()); - } catch { - // Ignore if Safe Chain CA is not available - } - - // 2) User's certificates - if (userCertPath) { - const userPem = readUserCertificateFile(userCertPath); - if (userPem) { - parts.push(userPem.trim()); - ui.writeVerbose(`Safe-chain: Merging user's NODE_EXTRA_CA_CERTS from ${userCertPath}`); - } else { - ui.writeWarning(`Safe-chain: Could not read or parse user's NODE_EXTRA_CA_CERTS from ${userCertPath}`); - } - } - - const finalCombined = parts.filter(Boolean).join("\n"); - const target = path.join(os.tmpdir(), `safe-chain-ca-bundle-${Date.now()}.pem`); - fs.writeFileSync(target, finalCombined, { encoding: "utf8" }); - return target; -} diff --git a/packages/safe-chain/src/registryProxy/certBundle.spec.js b/packages/safe-chain/src/registryProxy/certBundle.spec.js index dd718af..e3b58fb 100644 --- a/packages/safe-chain/src/registryProxy/certBundle.spec.js +++ b/packages/safe-chain/src/registryProxy/certBundle.spec.js @@ -77,12 +77,13 @@ describe("certBundle.getCombinedCaBundlePath", () => { }); }); -describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { +describe("certBundle.getCombinedCaBundlePath with user certs", () => { beforeEach(() => { mock.restoreAll(); + delete process.env.NODE_EXTRA_CA_CERTS; }); - it("returns a path with Safe Chain CA when no user cert provided", async () => { + it("returns a path with full CA bundle (Safe Chain + Mozilla + Node roots) when no user cert in env", async () => { // Mock getCaCertPath to return valid cert const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); const safeChainPath = path.join(tmpDir, "safechain.pem"); @@ -94,15 +95,17 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(undefined); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); - assert.match(contents, /-----BEGIN CERTIFICATE-----/, "Should contain Safe Chain CA"); + assert.match(contents, /-----BEGIN CERTIFICATE-----/, "Should contain certificate blocks"); + // Should include base bundle (Safe Chain + Mozilla/Node roots) + assert.ok(contents.length > 1000, "Bundle should be substantial with Mozilla/Node roots included"); }); - it("merges user cert with Safe Chain CA", async () => { + it("merges user cert with full base bundle (Safe Chain CA + Mozilla + Node roots)", async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); // Create Safe Chain CA @@ -114,6 +117,7 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { const userCertPath = path.join(tmpDir, "user-cert.pem"); const userCert = getValidCert(); fs.writeFileSync(userCertPath, userCert, "utf8"); + process.env.NODE_EXTRA_CA_CERTS = userCertPath; mock.module("./certUtils.js", { namedExports: { @@ -121,8 +125,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(userCertPath); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -136,6 +140,7 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "certtest-")); const safeChainPath = path.join(tmpDir, "safechain.pem"); fs.writeFileSync(safeChainPath, getValidCert(), "utf8"); + process.env.NODE_EXTRA_CA_CERTS = "/nonexistent/path.pem"; mock.module("./certUtils.js", { namedExports: { @@ -143,8 +148,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts("/nonexistent/path.pem"); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -159,7 +164,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { fs.writeFileSync(safeChainPath, getValidCert(), "utf8"); const userCertPath = path.join(tmpDir, "invalid.pem"); - fs.writeFileSync(userCertPath, "NOT A VALID CERTIFICATE", "utf8"); + fs.writeFileSync(userCertPath, "NOT A VALID PEM", "utf8"); + process.env.NODE_EXTRA_CA_CERTS = userCertPath; mock.module("./certUtils.js", { namedExports: { @@ -167,8 +173,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(userCertPath); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -188,8 +194,9 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts("../../../etc/passwd"); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + process.env.NODE_EXTRA_CA_CERTS = "../../../etc/passwd"; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -221,8 +228,9 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(symlinkPath); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + process.env.NODE_EXTRA_CA_CERTS = symlinkPath; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -245,8 +253,9 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(certDir); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + process.env.NODE_EXTRA_CA_CERTS = certDir; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -265,8 +274,9 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(" "); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + process.env.NODE_EXTRA_CA_CERTS = " "; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); @@ -280,8 +290,10 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { // Create a real file with CRLF content to test Windows line ending support const userCertPath = path.join(tmpDir, "user-cert-crlf.pem"); - const crlfCert = getValidCert().replace(/\n/g, "\r\n"); - fs.writeFileSync(userCertPath, crlfCert, "utf8"); + const userCert = getValidCert(); + const certWithCRLF = userCert.replace(/\n/g, "\r\n"); + fs.writeFileSync(userCertPath, certWithCRLF, "utf8"); + process.env.NODE_EXTRA_CA_CERTS = userCertPath; mock.module("./certUtils.js", { namedExports: { @@ -289,8 +301,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); - const bundlePath = getCombinedCaBundlePathWithUserCerts(userCertPath); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); const certCount = (contents.match(/-----BEGIN CERTIFICATE-----/g) || []).length; @@ -308,7 +320,7 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); // Test that Windows path syntax is recognized (even if files don't exist on macOS/Linux) // These should gracefully fail (return Safe Chain CA only) rather than crash @@ -319,7 +331,8 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { ]; for (const winPath of winPaths) { - const bundlePath = getCombinedCaBundlePathWithUserCerts(winPath); + process.env.NODE_EXTRA_CA_CERTS = winPath; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), `Bundle should exist for ${winPath}`); const contents = fs.readFileSync(bundlePath, "utf8"); assert.match(contents, /-----BEGIN CERTIFICATE-----/, "Should contain Safe Chain CA"); @@ -337,7 +350,7 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { }, }); - const { getCombinedCaBundlePathWithUserCerts } = await import("./certBundle.js"); + const { getCombinedCaBundlePath } = await import("./certBundle.js"); // Test various Windows-style traversal attempts const traversalPaths = [ @@ -347,13 +360,20 @@ describe("certBundle.getCombinedCaBundlePathWithUserCerts", () => { "../../../etc/passwd", // Unix-style for comparison ]; + // First, get baseline bundle without user certs to know expected cert count + delete process.env.NODE_EXTRA_CA_CERTS; + const baselineBundlePath = getCombinedCaBundlePath(); + const baselineContents = fs.readFileSync(baselineBundlePath, "utf8"); + const baselineCertCount = (baselineContents.match(/-----BEGIN CERTIFICATE-----/g) || []).length; + for (const badPath of traversalPaths) { - const bundlePath = getCombinedCaBundlePathWithUserCerts(badPath); + process.env.NODE_EXTRA_CA_CERTS = badPath; + const bundlePath = getCombinedCaBundlePath(); assert.ok(fs.existsSync(bundlePath), "Bundle path should exist"); const contents = fs.readFileSync(bundlePath, "utf8"); - // Only Safe Chain CA should be present (user cert rejected due to traversal) + // Should contain base bundle (Safe Chain + Mozilla + Node roots) but NOT user cert const certCount = (contents.match(/-----BEGIN CERTIFICATE-----/g) || []).length; - assert.strictEqual(certCount, 1, `Traversal path ${badPath} should be rejected; only Safe Chain CA included`); + assert.strictEqual(certCount, baselineCertCount, `Traversal path ${badPath} should be rejected; base bundle only (no user cert added)`); } }); }); diff --git a/packages/safe-chain/src/registryProxy/registryProxy.js b/packages/safe-chain/src/registryProxy/registryProxy.js index 9402830..3097b09 100644 --- a/packages/safe-chain/src/registryProxy/registryProxy.js +++ b/packages/safe-chain/src/registryProxy/registryProxy.js @@ -2,7 +2,7 @@ import * as http from "http"; import { tunnelRequest } from "./tunnelRequestHandler.js"; import { mitmConnect } from "./mitmRequestHandler.js"; import { handleHttpProxyRequest } from "./plainHttpProxy.js"; -import { getCombinedCaBundlePathWithUserCerts } from "./certBundle.js"; +import { getCombinedCaBundlePath } from "./certBundle.js"; import { ui } from "../environment/userInteraction.js"; import chalk from "chalk"; import { createInterceptorForUrl } from "./interceptors/createInterceptorForEcoSystem.js"; @@ -37,8 +37,7 @@ function getSafeChainProxyEnvironmentVariables() { } const proxyUrl = `http://localhost:${state.port}`; - const userNodeExtraCaCerts = process.env.NODE_EXTRA_CA_CERTS; - const caCertPath = getCombinedCaBundlePathWithUserCerts(userNodeExtraCaCerts); + const caCertPath = getCombinedCaBundlePath(); return { HTTPS_PROXY: proxyUrl, @@ -121,7 +120,9 @@ function stopServer(server) { } catch { resolve(); } - setTimeout(() => resolve(), SERVER_STOP_TIMEOUT_MS); + setTimeout(() => { + resolve(); + }, SERVER_STOP_TIMEOUT_MS); }); }