From e7c48ecd1c94b172041899894f585e440590786d Mon Sep 17 00:00:00 2001 From: Artur Jankowski Date: Fri, 13 Nov 2026 11:45:13 +0100 Subject: [PATCH] test: isolate tests from the real keychain and home directory Running `npm test` could delete your Box CLI environments. After #731, some `login` and `logout` tests that never ran before started running. A few of them start the CLI with no environments set up. The CLI then saves an empty list of environments, and in tests that list went straight to your real system keychain, replacing whatever you had there. Other tests also wrote files into your real `~/.box` and `~/Downloads`. Tests now run in a sandbox. Before any test starts, `test/helpers/isolate-environment.js`: - gives the tests a temporary home folder instead of your real one, and empties it before each test; - swaps the system keychain for a simple in-memory one. Your real keychain and home folder are no longer touched. A few tests only passed because an earlier test had left a file or folder behind. They now set up what they need themselves. --- .mocharc.yml | 3 +- test/commands/bulk.test.js | 22 ++-- test/commands/files.test.js | 6 ++ test/fixtures/bulk/saveTest.txt | 1 - test/helpers/isolate-environment.js | 153 ++++++++++++++++++++++++++++ test/token-cache.test.js | 9 -- 6 files changed, 173 insertions(+), 21 deletions(-) delete mode 100644 test/fixtures/bulk/saveTest.txt create mode 100644 test/helpers/isolate-environment.js diff --git a/.mocharc.yml b/.mocharc.yml index f69495a0..54eca61d 100644 --- a/.mocharc.yml +++ b/.mocharc.yml @@ -1,3 +1,4 @@ recursive: true timeout: '10000' -reporter: 'spec' \ No newline at end of file +reporter: 'spec' +require: 'test/helpers/isolate-environment.js' diff --git a/test/commands/bulk.test.js b/test/commands/bulk.test.js index fce77a08..28f9173e 100644 --- a/test/commands/bulk.test.js +++ b/test/commands/bulk.test.js @@ -32,11 +32,8 @@ describe('Bulk', function () { '..', 'fixtures/bulk/input.csv' ), - saveFilePath = path.join( - __dirname, - '..', - 'fixtures/bulk/saveTest.txt' - ), + // Inside the temp home, so the test does not overwrite a tracked file + saveFilePath = path.join(os.homedir(), 'saveTest.txt'), csvOutput = getFixture('bulk/post_collaborations_csv.csv'), multipleFlagValuesInputFilePath = path.join( __dirname, @@ -198,6 +195,10 @@ describe('Bulk', function () { .post('/2.0/collaborations', addCollaborationBody3) .reply(200, addCollaborationFixture3) ) + .do(() => { + // Existing file, to check that -y overwrites it without asking + fs.writeFileSync(saveFilePath, 'foo', 'utf8'); + }) .stdout() .stderr() .command([ @@ -728,11 +729,8 @@ describe('Bulk', function () { __dirname, '../fixtures/bulk/input_metadata_update.json' ), - saveFilePath = path.join( - __dirname, - '..', - 'fixtures/bulk/saveTest.txt' - ); + // Inside the temp home, so the test does not overwrite a tracked file + saveFilePath = path.join(os.homedir(), 'saveTest.txt'); let addCollaborationBody1 = { item: { @@ -935,6 +933,10 @@ describe('Bulk', function () { .post('/2.0/collaborations', addCollaborationBody3) .reply(200, addCollaborationFixture3) ) + .do(() => { + // Existing file, to check that -y overwrites it without asking + fs.writeFileSync(saveFilePath, 'foo', 'utf8'); + }) .stdout() .stderr() .command([ diff --git a/test/commands/files.test.js b/test/commands/files.test.js index c3246682..caae86c9 100644 --- a/test/commands/files.test.js +++ b/test/commands/files.test.js @@ -2202,6 +2202,7 @@ describe('Files', function () { }) ) .do(() => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, saveAsFileName), 'foo', @@ -2235,6 +2236,7 @@ describe('Files', function () { api.get(`/2.0/files/${fileId}`).reply(200, getFileFixture) ) .do(() => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, saveAsFileName), 'foo', @@ -2552,6 +2554,7 @@ describe('Files', function () { }) ) .do(() => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, saveAsFileName), 'foo', @@ -2586,6 +2589,7 @@ describe('Files', function () { api.get(`/2.0/files/${fileId}`).reply(200, getFileFixture) ) .do(() => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, saveAsFileName), 'foo', @@ -2767,6 +2771,7 @@ describe('Files', function () { api.get(statusUrl).reply(200, downloadStatusFixture) ) .do(async () => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); await fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, fileName), 'foo' @@ -2798,6 +2803,7 @@ describe('Files', function () { } ); test.do(async () => { + fs.mkdirSync(DEFAULT_DOWNLOAD_PATH, { recursive: true }); await fs.writeFileSync( path.join(DEFAULT_DOWNLOAD_PATH, fileName), 'foo' diff --git a/test/fixtures/bulk/saveTest.txt b/test/fixtures/bulk/saveTest.txt deleted file mode 100644 index 19102815..00000000 --- a/test/fixtures/bulk/saveTest.txt +++ /dev/null @@ -1 +0,0 @@ -foo \ No newline at end of file diff --git a/test/helpers/isolate-environment.js b/test/helpers/isolate-environment.js new file mode 100644 index 00000000..9c1c5b49 --- /dev/null +++ b/test/helpers/isolate-environment.js @@ -0,0 +1,153 @@ +'use strict'; + +/** + * Test setup, not a test. Mocha loads this file once, before any test file + * (see `require` in .mocharc.yml). It makes sure tests never read or change + * the developer's real CLI configuration, keychain entries, or files. + * + * 1. Temporary home directory + * + * HOME (USERPROFILE on Windows) is pointed at a new temp directory. Every + * path the CLI builds from the home directory, such as `~/.box` (settings, + * environments, token caches) and `~/Downloads/Box-Downloads`, now ends up + * there instead of in the real home directory. + * + * This has to run before any file from src/ is loaded, because + * src/box-command.js reads os.homedir() once, when it is loaded. The check + * below throws if that already happened. + * + * Before each test the temp home is emptied and an empty `~/.box` is + * created again (the CLI creates that folder at startup and other code + * expects it). This way a file written by one test is never seen by the + * next one. The whole directory is deleted when the test run ends. + * + * 2. In-memory keychain + * + * src/secure-storage.js stores secrets in the system keychain through one + * of two npm modules: + * - macOS: `keychain` (macOS Keychain) + * - Windows and Linux: `@github/keytar` (Windows Credential Manager, + * or libsecret on Linux) + * Their getPassword/setPassword/deletePassword functions are replaced with + * versions that use a plain Map, cleared before each test. If keytar cannot + * be loaded (for example on Linux without libsecret), nothing is replaced; + * the CLI then stores secrets in files, which are in the temp home anyway. + * + * Why replace these modules and not the methods of secure-storage.js? + * Because then the real secure-storage.js code still runs in tests and + * only the last step, talking to the OS, is fake: + * + * CLI command -> src/secure-storage.js -> keychain/keytar -> OS keychain + * ^ replaced here + * + * test/secure-storage.test.js depends on this and does not break it: + * - It stubs the `keychain` functions with sinon to check that + * secure-storage.js calls them. When sinon restores them after the + * test, it puts back our in-memory versions, not the real ones. + * - It removes src/secure-storage.js from the require cache and loads it + * again. The new copy calls require('keychain') and gets the same, + * already replaced module object, because that module stays cached. + */ + +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const srcDir = path.resolve(__dirname, '..', '..', 'src') + path.sep; +const loadedSrcModule = Object.keys(require.cache).find((file) => + file.startsWith(srcDir) +); +if (loadedSrcModule) { + throw new Error( + `${loadedSrcModule} was loaded before the test home directory was set up` + ); +} + +const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), 'boxcli-test-home-')); +process.env.HOME = tempHome; +process.env.USERPROFILE = tempHome; +if (os.homedir() !== tempHome) { + throw new Error( + `os.homedir() returned ${os.homedir()} instead of the test home directory ${tempHome}` + ); +} +process.on('exit', () => { + try { + fs.rmSync(tempHome, { recursive: true, force: true, maxRetries: 3 }); + } catch { + // A file may still be open (Windows). Leaving the temp dir behind is + // better than failing a run whose tests all passed. + } +}); + +// Empties the temp home, then recreates an empty ~/.box. The CLI creates that +// folder at startup, and code such as the token cache expects it to exist. +function resetTempHome() { + for (const entry of fs.readdirSync(tempHome)) { + fs.rmSync(path.join(tempHome, entry), { + recursive: true, + force: true, + maxRetries: 3, + }); + } + fs.mkdirSync(path.join(tempHome, '.box')); +} + +function loadOptional(packageName) { + try { + return require(packageName); + } catch { + // Not loadable on this machine. Secure storage is then unavailable and + // the CLI uses files in the temp home. + return null; + } +} + +const store = new Map(); +const keyOf = (service, account) => `${service}\u0000${account}`; + +const keychain = + process.platform === 'darwin' ? loadOptional('keychain') : null; +const keytar = + process.platform === 'darwin' ? null : loadOptional('@github/keytar'); + +// macOS backend: callback API, reports a missing entry as an error. +if (keychain) { + keychain.getPassword = (opts, fn) => { + const key = keyOf(opts.service, opts.account); + if (store.has(key)) { + fn(null, store.get(key)); + } else { + fn(new Error('Could not find password'), null); + } + }; + keychain.setPassword = (opts, fn) => { + store.set(keyOf(opts.service, opts.account), opts.password); + fn(null); + }; + keychain.deletePassword = (opts, fn) => { + if (store.delete(keyOf(opts.service, opts.account))) { + fn(null); + } else { + fn(new Error('Could not find password')); + } + }; +} + +// Windows/Linux backend: promise API. +if (keytar) { + keytar.getPassword = async (service, account) => + store.get(keyOf(service, account)) ?? null; + keytar.setPassword = async (service, account, password) => { + store.set(keyOf(service, account), password); + }; + keytar.deletePassword = async (service, account) => + store.delete(keyOf(service, account)); +} + +exports.mochaHooks = { + beforeEach() { + store.clear(); + resetTempHome(); + }, +}; diff --git a/test/token-cache.test.js b/test/token-cache.test.js index 6da729eb..30b834cb 100644 --- a/test/token-cache.test.js +++ b/test/token-cache.test.js @@ -31,15 +31,6 @@ describe('CLITokenCache', function () { }); afterEach(function () { - // Clean up any test files - if (fs.existsSync(testFilePath)) { - try { - fs.unlinkSync(testFilePath); - } catch { - // Ignore cleanup errors - } - } - // Restore stubs if (keytarStub) { keytarStub.restore();