From 5304a7744a67da875ed73a1ce744f03fccbfc63a Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Mon, 3 Nov 2025 14:41:29 +0100 Subject: [PATCH 1/3] Add better error handling, tests and type checks for configFile.js --- packages/safe-chain/src/config/configFile.js | 30 +++- .../safe-chain/src/config/configFile.spec.js | 140 ++++++++++++++++++ 2 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 packages/safe-chain/src/config/configFile.spec.js diff --git a/packages/safe-chain/src/config/configFile.js b/packages/safe-chain/src/config/configFile.js index 7273f7a..ab96bb1 100644 --- a/packages/safe-chain/src/config/configFile.js +++ b/packages/safe-chain/src/config/configFile.js @@ -3,14 +3,32 @@ import path from "path"; import os from "os"; import { ui } from "../environment/userInteraction.js"; +/** + * @typedef {Object} SafeChainConfig + * @property {any} scanTimeout // This should be a number + */ + /** * @returns {number} */ export function getScanTimeout() { - const config = /** @type {{scanTimeout?: number}} */ (readConfigFile()); + const config = readConfigFile(); - // @ts-expect-error values of process.env can be string | undefined - return parseInt(process.env.AIKIDO_SCAN_TIMEOUT_MS) || config.scanTimeout || 10000 // Default to 10 seconds + if (process.env.AIKIDO_SCAN_TIMEOUT_MS) { + const scanTimeout = Number(process.env.AIKIDO_SCAN_TIMEOUT_MS); + if (!Number.isNaN(scanTimeout) && scanTimeout > 0) { + return scanTimeout; + } + } + + if (config.scanTimeout) { + const scanTimeout = Number(config.scanTimeout); + if (!Number.isNaN(scanTimeout) && scanTimeout > 0) { + return scanTimeout; + } + } + + return 10000; // Default to 10 seconds } /** @@ -68,13 +86,15 @@ export function readDatabaseFromLocalCache() { } /** - * @returns {unknown} + * @returns {SafeChainConfig} */ function readConfigFile() { const configFilePath = getConfigFilePath(); if (!fs.existsSync(configFilePath)) { - return {}; + return { + scanTimeout: undefined, + }; } const data = fs.readFileSync(configFilePath, "utf8"); diff --git a/packages/safe-chain/src/config/configFile.spec.js b/packages/safe-chain/src/config/configFile.spec.js new file mode 100644 index 0000000..f52d20b --- /dev/null +++ b/packages/safe-chain/src/config/configFile.spec.js @@ -0,0 +1,140 @@ +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert"; +import { getScanTimeout } from "./configFile.js"; +import fs from "fs"; +import path from "path"; +import os from "os"; + +describe("getScanTimeout", () => { + let originalEnv; + let aikidoDir; + let configPath; + let configBackupPath; + + beforeEach(() => { + // Save original environment + originalEnv = process.env.AIKIDO_SCAN_TIMEOUT_MS; + + // Use the actual .aikido directory + aikidoDir = path.join(os.homedir(), ".aikido"); + configPath = path.join(aikidoDir, "config.json"); + configBackupPath = path.join(aikidoDir, "config.json.backup"); + + // Backup existing config if it exists + if (fs.existsSync(configPath)) { + fs.copyFileSync(configPath, configBackupPath); + } + }); + + afterEach(() => { + // Restore original environment + if (originalEnv !== undefined) { + process.env.AIKIDO_SCAN_TIMEOUT_MS = originalEnv; + } else { + delete process.env.AIKIDO_SCAN_TIMEOUT_MS; + } + + // Restore original config file + if (fs.existsSync(configBackupPath)) { + fs.copyFileSync(configBackupPath, configPath); + fs.unlinkSync(configBackupPath); + } else if (fs.existsSync(configPath)) { + fs.unlinkSync(configPath); + } + }); + + it("should return default timeout of 10000ms when no config or env var is set", () => { + delete process.env.AIKIDO_SCAN_TIMEOUT_MS; + if (fs.existsSync(configPath)) { + fs.unlinkSync(configPath); + } + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 10000); + }); + + it("should return timeout from config file when set", () => { + delete process.env.AIKIDO_SCAN_TIMEOUT_MS; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 5000 })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 5000); + }); + + it("should prioritize environment variable over config file", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "20000"; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 5000 })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 20000); + }); + + it("should handle invalid environment variable and fall back to config", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "invalid"; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 7000 })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 7000); + }); + + it("should ignore zero and negative values and fall back to default", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "0"; + + let timeout = getScanTimeout(); + assert.strictEqual(timeout, 10000); + + process.env.AIKIDO_SCAN_TIMEOUT_MS = "-5000"; + + timeout = getScanTimeout(); + assert.strictEqual(timeout, 10000); + }); + + it("should ignore textual non-numeric values in environment variable and fall back to config", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "fast"; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 8000 })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 8000); + }); + + it("should ignore textual non-numeric values in config file and fall back to default", () => { + delete process.env.AIKIDO_SCAN_TIMEOUT_MS; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "slow" })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 10000); + }); + + it("should ignore textual non-numeric values in both env and config, fall back to default", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "quick"; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "medium" })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 10000); + }); + + it("should ignore mixed alphanumeric strings in environment variable", () => { + process.env.AIKIDO_SCAN_TIMEOUT_MS = "5000ms"; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 6000 })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 6000); + }); + + it("should ignore mixed alphanumeric strings in config file", () => { + delete process.env.AIKIDO_SCAN_TIMEOUT_MS; + fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "3000ms" })); + + const timeout = getScanTimeout(); + + assert.strictEqual(timeout, 10000); + }); +}); From 1e7cd74364b2c80336d659eca26c99c8db169841 Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Mon, 3 Nov 2025 14:49:44 +0100 Subject: [PATCH 2/3] Mock filesystem in configFile.spec.js --- packages/safe-chain/src/config/configFile.js | 5 +- .../safe-chain/src/config/configFile.spec.js | 102 ++++++++++++------ 2 files changed, 71 insertions(+), 36 deletions(-) diff --git a/packages/safe-chain/src/config/configFile.js b/packages/safe-chain/src/config/configFile.js index ab96bb1..cf36b12 100644 --- a/packages/safe-chain/src/config/configFile.js +++ b/packages/safe-chain/src/config/configFile.js @@ -5,7 +5,10 @@ import { ui } from "../environment/userInteraction.js"; /** * @typedef {Object} SafeChainConfig - * @property {any} scanTimeout // This should be a number + * + * This should be a number, but can be anything because it is user-input. + * We cannot trust the input and should add the necessary validations. + * @property {any} scanTimeout */ /** diff --git a/packages/safe-chain/src/config/configFile.spec.js b/packages/safe-chain/src/config/configFile.spec.js index f52d20b..8ec980c 100644 --- a/packages/safe-chain/src/config/configFile.spec.js +++ b/packages/safe-chain/src/config/configFile.spec.js @@ -1,29 +1,32 @@ -import { describe, it, beforeEach, afterEach } from "node:test"; +import { describe, it, beforeEach, afterEach, mock } from "node:test"; import assert from "node:assert"; -import { getScanTimeout } from "./configFile.js"; -import fs from "fs"; -import path from "path"; -import os from "os"; describe("getScanTimeout", () => { let originalEnv; - let aikidoDir; - let configPath; - let configBackupPath; + let fsMock; + let getScanTimeout; - beforeEach(() => { + beforeEach(async () => { // Save original environment originalEnv = process.env.AIKIDO_SCAN_TIMEOUT_MS; - // Use the actual .aikido directory - aikidoDir = path.join(os.homedir(), ".aikido"); - configPath = path.join(aikidoDir, "config.json"); - configBackupPath = path.join(aikidoDir, "config.json.backup"); + // Mock fs module + fsMock = { + existsSync: mock.fn(() => false), + readFileSync: mock.fn(() => "{}"), + writeFileSync: mock.fn(), + mkdirSync: mock.fn(), + }; - // Backup existing config if it exists - if (fs.existsSync(configPath)) { - fs.copyFileSync(configPath, configBackupPath); - } + mock.module("fs", { + namedExports: fsMock, + }); + + // Re-import the module to get the mocked version + const configFileModule = await import( + `./configFile.js?update=${Date.now()}` + ); + getScanTimeout = configFileModule.getScanTimeout; }); afterEach(() => { @@ -34,20 +37,14 @@ describe("getScanTimeout", () => { delete process.env.AIKIDO_SCAN_TIMEOUT_MS; } - // Restore original config file - if (fs.existsSync(configBackupPath)) { - fs.copyFileSync(configBackupPath, configPath); - fs.unlinkSync(configBackupPath); - } else if (fs.existsSync(configPath)) { - fs.unlinkSync(configPath); - } + // Reset all mocks + mock.restoreAll(); }); it("should return default timeout of 10000ms when no config or env var is set", () => { delete process.env.AIKIDO_SCAN_TIMEOUT_MS; - if (fs.existsSync(configPath)) { - fs.unlinkSync(configPath); - } + // Mock: config file doesn't exist + fsMock.existsSync.mock.mockImplementation(() => false); const timeout = getScanTimeout(); @@ -56,7 +53,11 @@ describe("getScanTimeout", () => { it("should return timeout from config file when set", () => { delete process.env.AIKIDO_SCAN_TIMEOUT_MS; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 5000 })); + // Mock: config file exists with scanTimeout: 5000 + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: 5000 }) + ); const timeout = getScanTimeout(); @@ -65,7 +66,11 @@ describe("getScanTimeout", () => { it("should prioritize environment variable over config file", () => { process.env.AIKIDO_SCAN_TIMEOUT_MS = "20000"; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 5000 })); + // Mock: config file exists with scanTimeout: 5000 + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: 5000 }) + ); const timeout = getScanTimeout(); @@ -74,7 +79,11 @@ describe("getScanTimeout", () => { it("should handle invalid environment variable and fall back to config", () => { process.env.AIKIDO_SCAN_TIMEOUT_MS = "invalid"; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 7000 })); + // Mock: config file exists with scanTimeout: 7000 + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: 7000 }) + ); const timeout = getScanTimeout(); @@ -82,6 +91,9 @@ describe("getScanTimeout", () => { }); it("should ignore zero and negative values and fall back to default", () => { + // Mock: config file doesn't exist + fsMock.existsSync.mock.mockImplementation(() => false); + process.env.AIKIDO_SCAN_TIMEOUT_MS = "0"; let timeout = getScanTimeout(); @@ -95,7 +107,11 @@ describe("getScanTimeout", () => { it("should ignore textual non-numeric values in environment variable and fall back to config", () => { process.env.AIKIDO_SCAN_TIMEOUT_MS = "fast"; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 8000 })); + // Mock: config file exists with scanTimeout: 8000 + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: 8000 }) + ); const timeout = getScanTimeout(); @@ -104,7 +120,11 @@ describe("getScanTimeout", () => { it("should ignore textual non-numeric values in config file and fall back to default", () => { delete process.env.AIKIDO_SCAN_TIMEOUT_MS; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "slow" })); + // Mock: config file exists with scanTimeout: "slow" + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: "slow" }) + ); const timeout = getScanTimeout(); @@ -113,7 +133,11 @@ describe("getScanTimeout", () => { it("should ignore textual non-numeric values in both env and config, fall back to default", () => { process.env.AIKIDO_SCAN_TIMEOUT_MS = "quick"; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "medium" })); + // Mock: config file exists with scanTimeout: "medium" + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: "medium" }) + ); const timeout = getScanTimeout(); @@ -122,7 +146,11 @@ describe("getScanTimeout", () => { it("should ignore mixed alphanumeric strings in environment variable", () => { process.env.AIKIDO_SCAN_TIMEOUT_MS = "5000ms"; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: 6000 })); + // Mock: config file exists with scanTimeout: 6000 + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: 6000 }) + ); const timeout = getScanTimeout(); @@ -131,7 +159,11 @@ describe("getScanTimeout", () => { it("should ignore mixed alphanumeric strings in config file", () => { delete process.env.AIKIDO_SCAN_TIMEOUT_MS; - fs.writeFileSync(configPath, JSON.stringify({ scanTimeout: "3000ms" })); + // Mock: config file exists with scanTimeout: "3000ms" + fsMock.existsSync.mock.mockImplementation(() => true); + fsMock.readFileSync.mock.mockImplementation(() => + JSON.stringify({ scanTimeout: "3000ms" }) + ); const timeout = getScanTimeout(); From 8c872b3861d8dab2ef8fac63b1ac97a5835a8f4c Mon Sep 17 00:00:00 2001 From: Sander Declerck Date: Mon, 3 Nov 2025 14:54:42 +0100 Subject: [PATCH 3/3] Better error handling and extract validation logic to a re-usable function. --- packages/safe-chain/src/config/configFile.js | 31 ++++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/packages/safe-chain/src/config/configFile.js b/packages/safe-chain/src/config/configFile.js index cf36b12..901e7d7 100644 --- a/packages/safe-chain/src/config/configFile.js +++ b/packages/safe-chain/src/config/configFile.js @@ -18,15 +18,15 @@ export function getScanTimeout() { const config = readConfigFile(); if (process.env.AIKIDO_SCAN_TIMEOUT_MS) { - const scanTimeout = Number(process.env.AIKIDO_SCAN_TIMEOUT_MS); - if (!Number.isNaN(scanTimeout) && scanTimeout > 0) { + const scanTimeout = validateTimeout(process.env.AIKIDO_SCAN_TIMEOUT_MS); + if (scanTimeout != null) { return scanTimeout; } } if (config.scanTimeout) { - const scanTimeout = Number(config.scanTimeout); - if (!Number.isNaN(scanTimeout) && scanTimeout > 0) { + const scanTimeout = validateTimeout(config.scanTimeout); + if (scanTimeout != null) { return scanTimeout; } } @@ -34,6 +34,19 @@ export function getScanTimeout() { return 10000; // Default to 10 seconds } +/** + * + * @param {any} value + * @returns {number?} + */ +function validateTimeout(value) { + const timeout = Number(value); + if (!Number.isNaN(timeout) && timeout > 0) { + return timeout; + } + return null; +} + /** * @param {import("../api/aikido.js").MalwarePackage[]} data * @param {string | number} version @@ -100,8 +113,14 @@ function readConfigFile() { }; } - const data = fs.readFileSync(configFilePath, "utf8"); - return JSON.parse(data); + try { + const data = fs.readFileSync(configFilePath, "utf8"); + return JSON.parse(data); + } catch { + return { + scanTimeout: undefined, + }; + } } /**