From db34adf753561bfd153fbbf0ac9e9f6a89972126 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 13 Oct 2026 00:22:05 -0400 Subject: [PATCH 1/4] fix(install): strip npm_config_allow_scripts from the per-client installs (#2518) npm exports every non-default config value to lifecycle scripts as npm_config_*, whichever npmrc set it. With allow-scripts in a user's ~/.npmrc, the root install accepted it and then handed it to the install-clients postinstall as npm_config_allow_scripts; the child `npm install` read that as --allow-scripts on the command line, which npm 11.19+ rejects in a project install (EALLOWSCRIPTS), so a from-source install failed at clients/web. The child installs now run with that one variable removed. Nothing is lost: each child npm reads the same npmrc files itself, where the setting is allowed. The script's main guard compares real paths, since Node resolves symlinks for import.meta.url but not for argv[1], so the cascade cannot skip silently when the checkout is reached through a symlink. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- scripts/install-clients.mjs | 95 ++++++++++++++------- scripts/install-clients.test.mjs | 140 +++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 28 deletions(-) create mode 100644 scripts/install-clients.test.mjs diff --git a/scripts/install-clients.mjs b/scripts/install-clients.mjs index 815ef2da2..9485eaf87 100644 --- a/scripts/install-clients.mjs +++ b/scripts/install-clients.mjs @@ -19,47 +19,86 @@ * * Set INSPECTOR_SKIP_CLIENT_INSTALL=1 to skip explicitly — e.g. CI that * installs each client itself and wants to avoid the redundant pass. + * + * The child installs run with `npm_config_allow_scripts` stripped from their + * environment (#2518). npm exports every non-default config value to lifecycle + * scripts as `npm_config_*`, whichever npmrc set it, and a child npm reads that + * env var as if it were `--allow-scripts` on the command line — which npm + * 11.19+ rejects outright in a project install (EALLOWSCRIPTS). So a user with + * `allow-scripts` in their ~/.npmrc could install the root and then fail on + * the first client. Stripping it loses nothing: each child npm reads the same + * npmrc files itself, as npmrc, where the setting is allowed. */ import { spawnSync } from "node:child_process"; -import { existsSync } from "node:fs"; +import { existsSync, realpathSync } from "node:fs"; import { dirname, join, resolve, sep } from "node:path"; import { fileURLToPath } from "node:url"; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const CLIENTS = ["web", "cli", "mcpdo", "tui", "launcher"]; -if (process.env.INSPECTOR_SKIP_CLIENT_INSTALL) { - console.log( - "[install-clients] INSPECTOR_SKIP_CLIENT_INSTALL set — skipping client installs.", - ); - process.exit(0); +/** + * The environment for a per-client `npm install`: the parent's, minus the + * config npm refuses to receive through the environment in a project install. + * npm matches `npm_config_*` case-insensitively, so the strip does too. + * @param {NodeJS.ProcessEnv} env + * @returns {NodeJS.ProcessEnv} + */ +export function childInstallEnv(env) { + const next = { ...env }; + for (const key of Object.keys(next)) { + if (key.toLowerCase() === "npm_config_allow_scripts") { + delete next[key]; + } + } + return next; } -// Installed as a dependency (lives under node_modules) → nothing to do. -if (repoRoot.split(sep).includes("node_modules")) { - process.exit(0); -} +function main() { + if (process.env.INSPECTOR_SKIP_CLIENT_INSTALL) { + console.log( + "[install-clients] INSPECTOR_SKIP_CLIENT_INSTALL set — skipping client installs.", + ); + process.exit(0); + } -for (const name of CLIENTS) { - const dir = join(repoRoot, "clients", name); - // The published tarball ships build/ output only, no client package.json. - if (!existsSync(join(dir, "package.json"))) { - continue; + // Installed as a dependency (lives under node_modules) → nothing to do. + if (repoRoot.split(sep).includes("node_modules")) { + process.exit(0); } - console.log( - `[install-clients] Installing dependencies for clients/${name}...`, - ); - const result = spawnSync("npm", ["install"], { - cwd: dir, - stdio: "inherit", - // npm is npm.cmd on Windows, which needs a shell to resolve. - shell: process.platform === "win32", - }); - if (result.status !== 0) { - console.error( - `[install-clients] npm install failed in clients/${name} (exit ${result.status ?? "unknown"}).`, + + for (const name of CLIENTS) { + const dir = join(repoRoot, "clients", name); + // The published tarball ships build/ output only, no client package.json. + if (!existsSync(join(dir, "package.json"))) { + continue; + } + console.log( + `[install-clients] Installing dependencies for clients/${name}...`, ); - process.exit(result.status ?? 1); + const result = spawnSync("npm", ["install"], { + cwd: dir, + stdio: "inherit", + env: childInstallEnv(process.env), + // npm is npm.cmd on Windows, which needs a shell to resolve. + shell: process.platform === "win32", + }); + if (result.status !== 0) { + console.error( + `[install-clients] npm install failed in clients/${name} (exit ${result.status ?? "unknown"}).`, + ); + process.exit(result.status ?? 1); + } } } + +// Compare real paths: Node resolves symlinks for import.meta.url but not for +// argv[1], so a checkout reached through a symlink (macOS /var -> /private/var) +// would otherwise skip the cascade silently. +if ( + process.argv[1] && + realpathSync(process.argv[1]) === fileURLToPath(import.meta.url) +) { + main(); +} diff --git a/scripts/install-clients.test.mjs b/scripts/install-clients.test.mjs new file mode 100644 index 000000000..78c1d8345 --- /dev/null +++ b/scripts/install-clients.test.mjs @@ -0,0 +1,140 @@ +/** + * Tests for scripts/install-clients.mjs — the root postinstall cascade. + * + * The unit cases pin `childInstallEnv`. The end-to-end cases run the real + * script and the real npm against a throwaway repo layout, because #2518 is + * an interaction between two npm processes: the parent exports a user's + * npmrc `allow-scripts` as `npm_config_allow_scripts`, and npm 11.19+ refuses + * that env var in a project install. Only running npm shows the cascade + * surviving it. + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { + copyFileSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { childInstallEnv } from "./install-clients.mjs"; + +const SCRIPT = join( + dirname(fileURLToPath(import.meta.url)), + "install-clients.mjs", +); + +test("childInstallEnv strips npm_config_allow_scripts", () => { + const env = childInstallEnv({ + PATH: "/bin", + npm_config_allow_scripts: "esbuild", + }); + assert.deepEqual(env, { PATH: "/bin" }); +}); + +test("childInstallEnv strips the key whatever its case, as npm reads it", () => { + const env = childInstallEnv({ + NPM_CONFIG_ALLOW_SCRIPTS: "a", + Npm_Config_Allow_Scripts: "b", + }); + assert.deepEqual(env, {}); +}); + +test("childInstallEnv keeps every other npm config and does not mutate its input", () => { + const input = { + npm_config_allow_scripts: "esbuild", + npm_config_registry: "https://registry.example/", + npm_config_allow_scripts_pin: "true", + }; + const env = childInstallEnv(input); + assert.deepEqual(env, { + npm_config_registry: "https://registry.example/", + npm_config_allow_scripts_pin: "true", + }); + assert.equal(input.npm_config_allow_scripts, "esbuild"); +}); + +/** + * A repo-shaped temp dir: scripts/install-clients.mjs plus one dependency-free + * client, and a HOME whose .npmrc sets allow-scripts — the reporter's setup. + */ +function fixture() { + const root = mkdtempSync(join(tmpdir(), "install-clients-")); + mkdirSync(join(root, "scripts")); + copyFileSync(SCRIPT, join(root, "scripts", "install-clients.mjs")); + const client = join(root, "clients", "web"); + mkdirSync(client, { recursive: true }); + writeFileSync( + join(client, "package.json"), + JSON.stringify({ name: "fixture-web", version: "1.0.0", private: true }), + ); + const home = join(root, "home"); + mkdirSync(home); + writeFileSync(join(home, ".npmrc"), "allow-scripts=esbuild\n"); + return { root, client, home }; +} + +/** The env a parent npm hands its postinstall when ~/.npmrc sets allow-scripts. */ +function parentNpmEnv(home) { + const env = childInstallEnv(process.env); + for (const key of Object.keys(env)) { + if (key.toLowerCase().startsWith("npm_config_")) delete env[key]; + } + return { + ...env, + HOME: home, + USERPROFILE: home, + npm_config_allow_scripts: "esbuild", + npm_config_offline: "true", + npm_config_audit: "false", + npm_config_fund: "false", + npm_config_update_notifier: "false", + }; +} + +const npmCmd = { shell: process.platform === "win32", encoding: "utf8" }; + +const npmVersion = spawnSync("npm", ["--version"], npmCmd).stdout.trim(); +const [major, minor] = npmVersion.split(".").map(Number); +const npmGatesAllowScripts = major > 11 || (major === 11 && minor >= 19); + +test( + "premise: npm rejects allow-scripts arriving through the environment", + { skip: !npmGatesAllowScripts && `npm ${npmVersion} predates the gate` }, + () => { + const { root, client, home } = fixture(); + try { + const result = spawnSync("npm", ["install"], { + ...npmCmd, + cwd: client, + env: parentNpmEnv(home), + }); + assert.notEqual(result.status, 0); + assert.match(result.stderr, /EALLOWSCRIPTS/); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, +); + +test("the cascade installs a client when the parent npm exported allow-scripts", () => { + const { root, home } = fixture(); + try { + const result = spawnSync( + process.execPath, + [join(root, "scripts", "install-clients.mjs")], + { ...npmCmd, cwd: root, env: parentNpmEnv(home) }, + ); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Installing dependencies for clients\/web/); + assert.doesNotMatch(result.stderr, /EALLOWSCRIPTS/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); From e3c76aba8917b0eb810a2cb78215669138dd5921 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 13 Oct 2026 00:31:24 -0400 Subject: [PATCH 2/4] test(install): pin the symlinked-checkout guard and spawn node without a shell (#2518) The symlink case now launches the fixture through a deliberate symlink (a junction on Windows) and the plain case runs from a canonical path, so reverting the realpath guard fails on every platform rather than only where tmpdir happens to be an alias. Node is spawned without a shell, so a Windows temp path containing a space is not re-split. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- scripts/install-clients.test.mjs | 45 ++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/scripts/install-clients.test.mjs b/scripts/install-clients.test.mjs index 78c1d8345..06c5f7909 100644 --- a/scripts/install-clients.test.mjs +++ b/scripts/install-clients.test.mjs @@ -16,7 +16,9 @@ import { copyFileSync, mkdirSync, mkdtempSync, + realpathSync, rmSync, + symlinkSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; @@ -65,7 +67,9 @@ test("childInstallEnv keeps every other npm config and does not mutate its input * client, and a HOME whose .npmrc sets allow-scripts — the reporter's setup. */ function fixture() { - const root = mkdtempSync(join(tmpdir(), "install-clients-")); + // Canonical, so a symlinked tmpdir (macOS /var) can't make the plain case + // exercise the symlink path by accident; that has its own case below. + const root = realpathSync(mkdtempSync(join(tmpdir(), "install-clients-"))); mkdirSync(join(root, "scripts")); copyFileSync(SCRIPT, join(root, "scripts", "install-clients.mjs")); const client = join(root, "clients", "web"); @@ -123,13 +127,24 @@ test( }, ); +/** Run the fixture's copy of the script the way npm runs the postinstall. */ +function runCascade(script, cwd, home) { + // process.execPath is directly executable everywhere, so no shell: on + // Windows a shell would re-split a temp path containing a space. + return spawnSync(process.execPath, [script], { + encoding: "utf8", + cwd, + env: parentNpmEnv(home), + }); +} + test("the cascade installs a client when the parent npm exported allow-scripts", () => { const { root, home } = fixture(); try { - const result = spawnSync( - process.execPath, - [join(root, "scripts", "install-clients.mjs")], - { ...npmCmd, cwd: root, env: parentNpmEnv(home) }, + const result = runCascade( + join(root, "scripts", "install-clients.mjs"), + root, + home, ); assert.equal(result.status, 0, result.stderr); assert.match(result.stdout, /Installing dependencies for clients\/web/); @@ -138,3 +153,23 @@ test("the cascade installs a client when the parent npm exported allow-scripts", rmSync(root, { recursive: true, force: true }); } }); + +test("the cascade still runs when the checkout is reached through a symlink", () => { + const { root, home } = fixture(); + const linkParent = mkdtempSync(join(tmpdir(), "install-clients-link-")); + const link = join(linkParent, "checkout"); + try { + // A junction needs no privilege on Windows; the type is ignored elsewhere. + symlinkSync(root, link, "junction"); + const result = runCascade( + join(link, "scripts", "install-clients.mjs"), + link, + home, + ); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Installing dependencies for clients\/web/); + } finally { + rmSync(linkParent, { recursive: true, force: true }); + rmSync(root, { recursive: true, force: true }); + } +}); From 1ac6e2ff60137cc7372c91e9a0c29db390ea769b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 13 Oct 2026 00:40:29 -0400 Subject: [PATCH 3/4] test(install): pin the env wiring with a stub npm, independent of the runner's npm (#2518) CI's Node 22 bundles npm 10, which predates the allow-scripts gate, so the real-npm cases skip or pass there whether or not the strip is wired in. A stub npm on PATH fails whenever npm_config_allow_scripts reaches it, so dropping the env wiring now fails on every runner. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- scripts/install-clients.test.mjs | 62 +++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/scripts/install-clients.test.mjs b/scripts/install-clients.test.mjs index 06c5f7909..b1100dcd1 100644 --- a/scripts/install-clients.test.mjs +++ b/scripts/install-clients.test.mjs @@ -22,7 +22,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join } from "node:path"; +import { delimiter, dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { childInstallEnv } from "./install-clients.mjs"; @@ -173,3 +173,63 @@ test("the cascade still runs when the checkout is reached through a symlink", () rmSync(root, { recursive: true, force: true }); } }); + +/** + * A stand-in `npm` on PATH that fails, as npm 11.19+ does, when + * allow-scripts reaches it through the environment. It pins the spawn wiring + * whatever npm the runner has: CI's Node 22 bundles npm 10, which predates + * the gate, so against the real npm there the cascade passes with or without + * the strip. + */ +function stubNpm() { + const dir = mkdtempSync(join(tmpdir(), "install-clients-npm-")); + const js = join(dir, "npm-stub.cjs"); + writeFileSync( + js, + `const hit = Object.keys(process.env).some( + (k) => k.toLowerCase() === "npm_config_allow_scripts", +); +if (hit) { + console.error("npm error code EALLOWSCRIPTS"); + process.exit(1); +} +`, + ); + writeFileSync( + join(dir, "npm"), + `#!/bin/sh\nexec "${process.execPath}" "${js}" "$@"\n`, + { mode: 0o755 }, + ); + writeFileSync(join(dir, "npm.cmd"), `@"${process.execPath}" "${js}" %*\r\n`); + return dir; +} + +test("the cascade hands npm an environment without allow-scripts (stub npm)", () => { + const { root, home } = fixture(); + const npmDir = stubNpm(); + try { + const env = { + ...parentNpmEnv(home), + PATH: `${npmDir}${delimiter}${process.env.PATH ?? ""}`, + }; + // The stub itself must reject the variable, or this case proves nothing. + const probe = spawnSync("npm", ["install"], { + ...npmCmd, + cwd: root, + env, + }); + assert.equal(probe.status, 1); + assert.match(probe.stderr, /EALLOWSCRIPTS/); + + const result = spawnSync( + process.execPath, + [join(root, "scripts", "install-clients.mjs")], + { encoding: "utf8", cwd: root, env }, + ); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Installing dependencies for clients\/web/); + } finally { + rmSync(npmDir, { recursive: true, force: true }); + rmSync(root, { recursive: true, force: true }); + } +}); From 996500e72f9d9a16d9931aaeac0f4f1d5bc56f3d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 13 Oct 2026 00:48:53 -0400 Subject: [PATCH 4/4] fix(install): canonicalize both sides of the main guard (#2518) --preserve-symlinks-main keeps the symlink in import.meta.url, so canonicalizing only argv[1] still skipped the cascade silently for a checkout reached through a symlink under that flag. The symlink case now runs with and without it. Co-Authored-By: Claude Opus 5.5 Signed-off-by: cliffhall --- scripts/install-clients.mjs | 7 ++--- scripts/install-clients.test.mjs | 46 +++++++++++++++++--------------- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/scripts/install-clients.mjs b/scripts/install-clients.mjs index 9485eaf87..244c21dcd 100644 --- a/scripts/install-clients.mjs +++ b/scripts/install-clients.mjs @@ -93,12 +93,13 @@ function main() { } } -// Compare real paths: Node resolves symlinks for import.meta.url but not for -// argv[1], so a checkout reached through a symlink (macOS /var -> /private/var) +// Compare real paths on both sides. By default Node resolves symlinks for +// import.meta.url but not for argv[1], and --preserve-symlinks-main reverses +// that, so a checkout reached through a symlink (macOS /var -> /private/var) // would otherwise skip the cascade silently. if ( process.argv[1] && - realpathSync(process.argv[1]) === fileURLToPath(import.meta.url) + realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url)) ) { main(); } diff --git a/scripts/install-clients.test.mjs b/scripts/install-clients.test.mjs index b1100dcd1..9c2ac28ac 100644 --- a/scripts/install-clients.test.mjs +++ b/scripts/install-clients.test.mjs @@ -128,10 +128,10 @@ test( ); /** Run the fixture's copy of the script the way npm runs the postinstall. */ -function runCascade(script, cwd, home) { +function runCascade(script, cwd, home, nodeArgs = []) { // process.execPath is directly executable everywhere, so no shell: on // Windows a shell would re-split a temp path containing a space. - return spawnSync(process.execPath, [script], { + return spawnSync(process.execPath, [...nodeArgs, script], { encoding: "utf8", cwd, env: parentNpmEnv(home), @@ -154,25 +154,29 @@ test("the cascade installs a client when the parent npm exported allow-scripts", } }); -test("the cascade still runs when the checkout is reached through a symlink", () => { - const { root, home } = fixture(); - const linkParent = mkdtempSync(join(tmpdir(), "install-clients-link-")); - const link = join(linkParent, "checkout"); - try { - // A junction needs no privilege on Windows; the type is ignored elsewhere. - symlinkSync(root, link, "junction"); - const result = runCascade( - join(link, "scripts", "install-clients.mjs"), - link, - home, - ); - assert.equal(result.status, 0, result.stderr); - assert.match(result.stdout, /Installing dependencies for clients\/web/); - } finally { - rmSync(linkParent, { recursive: true, force: true }); - rmSync(root, { recursive: true, force: true }); - } -}); +for (const nodeArgs of [[], ["--preserve-symlinks-main"]]) { + const how = nodeArgs.length ? ` (${nodeArgs.join(" ")})` : ""; + test(`the cascade still runs when the checkout is reached through a symlink${how}`, () => { + const { root, home } = fixture(); + const linkParent = mkdtempSync(join(tmpdir(), "install-clients-link-")); + const link = join(linkParent, "checkout"); + try { + // A junction needs no privilege on Windows; the type is ignored elsewhere. + symlinkSync(root, link, "junction"); + const result = runCascade( + join(link, "scripts", "install-clients.mjs"), + link, + home, + nodeArgs, + ); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /Installing dependencies for clients\/web/); + } finally { + rmSync(linkParent, { recursive: true, force: true }); + rmSync(root, { recursive: true, force: true }); + } + }); +} /** * A stand-in `npm` on PATH that fails, as npm 11.19+ does, when