From 9c02f6f3bfa65cbb70e7ad863b68a58e8cf2ed83 Mon Sep 17 00:00:00 2001 From: TSavo Date: Wed, 8 Apr 2026 13:52:27 -0700 Subject: [PATCH] fix(test): isolate sysbin in install-preflight tests to prevent host PATH leakage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "node missing" / "npm missing" runtime preflight tests need a PATH where the host's real `node` and `npm` are NOT visible, so the error branches are actually exercised. The previous `TEST_SYSTEM_PATH = "/usr/bin:/bin"` literal leaks `/usr/bin/node` on any Linux distribution that installs Node via `apt install nodejs` (i.e. most of them). On those hosts the affected tests assert the wrong code path — they expect "node missing" but the preflight finds the system `/usr/bin/node` and reports a version mismatch instead. The tests pass on the upstream CI runners because Node is installed under `/opt/hostedtoolcache/node/...` there (via `actions/setup-node`) rather than `/usr/bin/`, so the leak is invisible in CI. Developers running `npm test` locally on Ubuntu hit it consistently. This replaces the literal with `buildIsolatedSystemPath()`, a small helper that creates a tmpdir under `os.tmpdir()` at module load and symlinks every entry from `/usr/bin` and `/bin` into it — except `node`, `npm`, and `npx`, which are deliberately excluded. The result is a "system bin" that has everything the tests legitimately need (`git`, `bash`, `sh`, `python`, `grep`, etc.) but lacks the three binaries the runtime preflight is supposed to detect as missing. Tests that need a fake `node` or `npm` continue to work unchanged: they write a stub into `fakeBin` and prepend it to PATH (`${fakeBin}:${TEST_SYSTEM_PATH}`); the fake still wins because it comes first. Tests that use `PATH: TEST_SYSTEM_PATH` directly (without `fakeBin`) now correctly see neither node nor npm. The `symlinkSync` catch block ONLY ignores `EEXIST` (the expected case when /bin is a symlink to /usr/bin and we already linked the same name on the first pass). Any other error (EPERM, EACCES, EINVAL, ENOENT…) is re-thrown — swallowing them would leave TEST_SYSTEM_PATH partially populated and turn into a confusing downstream test failure. The tmpdir is intentionally not cleaned up — it's tiny (a few hundred symlinks), the OS reaps it on reboot, and cleanup would require an `afterAll` hook in every describe block in this file. The on-disk cost is negligible. Closes #1621 (sub-item 2 of three). Signed-off-by: T Savo --- test/install-preflight.test.js | 48 +++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/test/install-preflight.test.js b/test/install-preflight.test.js index 14f1b4cb172..c41246c7f5e 100644 --- a/test/install-preflight.test.js +++ b/test/install-preflight.test.js @@ -11,7 +11,53 @@ const INSTALLER = path.join(import.meta.dirname, "..", "install.sh"); const CURL_PIPE_INSTALLER = path.join(import.meta.dirname, "..", "install.sh"); const INSTALLER_PAYLOAD = path.join(import.meta.dirname, "..", "scripts", "install.sh"); const GITHUB_INSTALL_URL = "git+https://github.com/NVIDIA/NemoClaw.git"; -const TEST_SYSTEM_PATH = "/usr/bin:/bin"; + +/** + * Build an isolated "system bin" directory used by every test in this file + * via TEST_SYSTEM_PATH. The directory mirrors /usr/bin and /bin via symlinks + * — EXCEPT for `node`, `npm`, and `npx`, which are deliberately excluded. + * + * Why: the runtime preflight tests need a PATH where the host's real `node` + * and `npm` are NOT visible, so the "node missing" / "npm missing" error + * branches are actually exercised. The previous `"/usr/bin:/bin"` literal + * leaks /usr/bin/node on any Linux distribution that installs Node via + * `apt install nodejs` (i.e. most of them), causing those tests to assert + * the wrong code path on developer machines while passing on the upstream + * CI runners (where Node is installed under /opt/hostedtoolcache/, not + * /usr/bin/). + * + * Tests that need a fake `node` or `npm` continue to write a stub into + * `fakeBin` and prepend it to PATH (`${fakeBin}:${TEST_SYSTEM_PATH}`); the + * fake still wins because it comes first. + * + * The directory lives under `os.tmpdir()` and is intentionally not cleaned + * up — it's tiny (a few hundred symlinks), the OS reaps it on reboot, and + * cleanup would require an `afterAll` hook in every describe block. + */ +function buildIsolatedSystemPath() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-preflight-sysbin-")); + const EXCLUDE = new Set(["node", "npm", "npx"]); + for (const sysDir of ["/usr/bin", "/bin"]) { + if (!fs.existsSync(sysDir)) continue; + for (const name of fs.readdirSync(sysDir)) { + if (EXCLUDE.has(name)) continue; + try { + fs.symlinkSync(path.join(sysDir, name), path.join(dir, name)); + } catch (err) { + // Only swallow EEXIST — the expected case is when /bin is a symlink + // to /usr/bin (modern Linux) and we already linked the same name on + // the first pass. Any other error (EPERM, EACCES, EINVAL, ENOENT…) + // would leave TEST_SYSTEM_PATH partially populated and turn into a + // confusing downstream test failure, so re-throw it. + if (err && err.code === "EEXIST") continue; + throw err; + } + } + } + return dir; +} + +const TEST_SYSTEM_PATH = buildIsolatedSystemPath(); function writeExecutable(target, contents) { fs.writeFileSync(target, contents, { mode: 0o755 });