fix(cli): spawn npm without DEP0190 on win32 — pass a joined command line instead of an argv array

bin/cli/commands/update.mjs

@@ -5,7 +5,7 @@ import { fileURLToPath } from "node:url";
 import { execFile } from "node:child_process";
 import { promisify } from "node:util";
 import { t } from "../i18n.mjs";
-import { npmBin, npmExecOptions } from "../npm-exec.mjs";
+import { npmBin, npmExecInvocation } from "../npm-exec.mjs";
 import { readPidFile, isPidRunning } from "../utils/pid.mjs";
 
 const execFileAsync = promisify(execFile);
@@ -30,15 +30,21 @@ export async function getCurrentVersion() {
 // `--prefer-online` forces npm to revalidate its HTTP cache against the registry.
 // Without it `npm view` can return a stale cached version (e.g. report 3.8.30 as
 // "latest" after 3.8.31 was published), so the updater told users on an old build
-// they were already on the latest version (#4376). `execFn` is injectable for tests.
-export async function getLatestVersion(execFn = execFileAsync) {
+// they were already on the latest version (#4376). `execFn` and `platform` are
+// injectable for tests.
+export async function getLatestVersion(execFn = execFileAsync, platform = process.platform) {
   try {
-    // argv is all literals, so enabling the shell on win32 cannot splice a
-    // runtime value into the command line (Hard Rule #13).
+    // Every token is a literal, so joining them into a command line on win32 cannot
+    // splice a runtime value into it (Hard Rule #13) — npmExecInvocation enforces
+    // that. It also hands execFile an EMPTY args array there, so the shell is
+    // enabled for npm.cmd without tripping DEP0190 (#15327).
     const { stdout } = await execFn(
-      npmBin(),
-      ["view", "omniroute", "version", "--prefer-online"],
-      npmExecOptions(process.platform, { timeoutMs: 15000 })
+      ...npmExecInvocation(
+        platform,
+        npmBin(platform),
+        ["view", "omniroute", "version", "--prefer-online"],
+        { timeoutMs: 15000 }
+      )
     );
     return stdout.trim();
   } catch {
@@ -154,9 +160,9 @@ export async function runUpdateCommand(opts = {}) {
   if (showChangelog) {
     try {
       const { stdout } = await execFileAsync(
-        npmBin(),
-        ["view", "omniroute", "changelog"],
-        npmExecOptions(process.platform, { timeoutMs: 15000 })
+        ...npmExecInvocation(process.platform, npmBin(), ["view", "omniroute", "changelog"], {
+          timeoutMs: 15000,
+        })
       );
       if (stdout.trim()) {
         console.log(stdout.trim());

bin/cli/npm-exec.mjs

@@ -11,6 +11,14 @@
 // line, not `execFile`. Every argv element passed alongside these options must be
 // a literal — never a runtime value — or it must be validated first. Callers that
 // need to pass a user-supplied name have to guard it themselves.
+//
+// DEP0190 (Node ≥ 24): `normalizeSpawnArguments` also emits a deprecation warning
+// whenever `shell` is set alongside a non-empty args array, because the shell only
+// concatenates that argv onto the command instead of escaping it. So the argv-array
+// form that #11335 needed is the one thing that cannot be used with the shell on
+// win32 — the two fixes collided. `npmExecInvocation` resolves it by handing
+// `execFile` the already-joined command line and an EMPTY args array, which is the
+// same string Node would have built, minus the warning.
 
 /** The npm binary to spawn on this platform. */
 export function npmBin(platform = process.platform) {
@@ -32,3 +40,65 @@ export function npmExecOptions(platform = process.platform, options = {}) {
   if (platform !== "win32") return { ...base, shell: false };
   return { ...base, shell: true, windowsHide: true };
 }
+
+/**
+ * A token that cannot change how a shell splits a command line: no whitespace, no
+ * quote, no cmd.exe or POSIX metacharacter, no backslash. Covers every shape the
+ * CLI actually spawns npm with — subcommands (`view`, `changelog`), flags
+ * (`--prefer-online`), package names, scoped names (`@scope/pkg`), dist-tags and
+ * semver ranges.
+ */
+const LITERAL_NPM_TOKEN = /^[A-Za-z0-9@._/:=+-]+$/;
+
+/**
+ * The `execFile` arguments for an npm call on `platform`, ready to spread:
+ * `execFile(...npmExecInvocation(platform, npmBin(platform), args, options))`.
+ *
+ * Off Windows there is no shell, so the argv array is returned verbatim and the
+ * tokens are not inspected at all — the platform that does not need a shell gets no
+ * new restriction. On win32 the tokens are joined into a single command string and
+ * the args array comes back EMPTY, which is the only shape that both spawns
+ * `npm.cmd` (it needs the shell, #11335) and avoids DEP0190 (#15327): Node's
+ * `normalizeSpawnArguments` warns on `shell` + a non-empty args array and then
+ * builds `${file} ${args.join(" ")}` itself, so an empty array with a pre-joined
+ * file produces byte-identical input to the shell.
+ *
+ * Hard Rule #13 is enforced here rather than assumed. Joining a command line is
+ * only safe while every token is a literal, so a token outside
+ * {@link LITERAL_NPM_TOKEN} throws instead of being concatenated — the call fails
+ * loudly rather than letting the shell reinterpret a runtime value. A caller that
+ * genuinely has one (a path, a user-supplied package name) must validate it first
+ * and escape it with `escapeWindowsShellArg` from `./utils/winShellArgs.mjs`, or
+ * pass it through the environment.
+ *
+ * @param {NodeJS.Platform} platform
+ * @param {string} bin
+ * @param {string[]} [args]
+ * @param {{ timeoutMs?: number, stdio?: string }} [options]
+ * @returns {[string, string[], Record<string, unknown>]}
+ */
+export function npmExecInvocation(
+  platform = process.platform,
+  bin = npmBin(platform),
+  args = [],
+  options = {}
+) {
+  const execOptions = npmExecOptions(platform, options);
+  const tokens = [bin, ...(args ?? [])];
+
+  if (execOptions.shell !== true) {
+    return [tokens[0], tokens.slice(1), execOptions];
+  }
+
+  for (const token of tokens) {
+    if (!LITERAL_NPM_TOKEN.test(token)) {
+      throw new Error(
+        `npmExecInvocation: refusing to join the non-literal token ${JSON.stringify(
+          token
+        )} into a win32 shell command line (Hard Rule #13) — validate it first, then escape it with escapeWindowsShellArg() from bin/cli/utils/winShellArgs.mjs, or pass it through the environment.`
+      );
+    }
+  }
+
+  return [tokens.join(" "), [], execOptions];
+}

tests/unit/cli-npm-exec-dep0190-15327.test.ts

@@ -0,0 +1,212 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+
+import { npmBin, npmExecOptions, npmExecInvocation } from "../../bin/cli/npm-exec.mjs";
+import * as update from "../../bin/cli/commands/update.mjs";
+
+// #15327 — on Windows + Node ≥ 24 every `omniroute update` (and the doctor's
+// version check) printed:
+//
+//   (node:NNNN) [DEP0190] DeprecationWarning: Passing args to a child process
+//   with shell option true can lead to security vulnerabilities, as the arguments
+//   are not escaped, only concatenated.
+//
+// #11335 had fixed the EINVAL by enabling `shell` on win32 so `npm.cmd` can be
+// spawned at all (nodejs/node#52554), but it kept the argv-array call shape. Node's
+// `normalizeSpawnArguments` warns on exactly that combination — `options.shell` set
+// AND `args.length > 0` — and then builds `${file} ${args.join(" ")}` itself. So
+// the two fixes collided: the shell is required for the `.cmd` shim and forbidden
+// alongside an argv array.
+//
+// The fix passes the already-joined command line as `file` with an EMPTY args
+// array — byte-identical input to the shell, minus the deprecation — and
+// npmExecInvocation refuses to build that string from anything but literals so
+// Hard Rule #13 keeps holding now that the shell sees a raw string.
+//
+// The warning itself is unreachable from a test (Node only emits DEP0190 on
+// Node ≥ 22, and only once per process), so the guard asserts the property Node
+// keys the warning on: no argv array travels with `shell: true`.
+
+const VERSION_ARGS = ["view", "omniroute", "version", "--prefer-online"];
+
+/** Node's own condition for DEP0190, from lib/child_process.js. */
+function dep0190WouldFire(args: string[], options: Record<string, unknown>): boolean {
+  return Boolean(options.shell) && args.length > 0;
+}
+
+test("#15327 win32 spawns npm with a joined command line and no argv array", () => {
+  const [file, args, options] = npmExecInvocation("win32", npmBin("win32"), VERSION_ARGS, {
+    timeoutMs: 15000,
+  });
+
+  // The shell still has to be on, or npm.cmd cannot be spawned at all (#11335).
+  assert.equal(options.shell, true, "win32 must keep the shell so npm.cmd can be spawned");
+  assert.equal(options.windowsHide, true);
+  assert.equal(options.timeout, 15000);
+
+  // …but the argv array that triggers DEP0190 must be empty.
+  assert.deepEqual(args, [], `win32 must pass no argv array, got ${JSON.stringify(args)}`);
+  assert.equal(
+    dep0190WouldFire(args, options),
+    false,
+    "spawning with shell:true AND a non-empty args array is what DEP0190 fires on"
+  );
+  assert.equal(file, "npm.cmd view omniroute version --prefer-online");
+});
+
+test("#15327 the joined command line carries only the literal tokens it was given", () => {
+  const [file] = npmExecInvocation("win32", npmBin("win32"), VERSION_ARGS);
+
+  // Every token survives the join verbatim — nothing quoted, escaped or reordered.
+  for (const token of [npmBin("win32"), ...VERSION_ARGS]) {
+    assert.ok(
+      (file as string).split(" ").includes(token),
+      `${JSON.stringify(token)} must appear verbatim in ${JSON.stringify(file)}`
+    );
+  }
+  assert.equal(
+    (file as string).split(" ").length,
+    1 + VERSION_ARGS.length,
+    "the join must add no quoting and drop no token"
+  );
+});
+
+test("#15327 off win32 the argv array is passed through untouched", () => {
+  for (const platform of ["linux", "darwin"] as const) {
+    const [file, args, options] = npmExecInvocation(platform, npmBin(platform), VERSION_ARGS, {
+      timeoutMs: 15000,
+    });
+
+    assert.equal(file, npmBin(platform));
+    assert.deepEqual(args, VERSION_ARGS, `${platform} must keep the argv array`);
+    assert.equal(options.shell, false);
+    // No shell, so nothing can trigger DEP0190 regardless of the argv.
+    assert.equal(dep0190WouldFire(args, options), false);
+  }
+});
+
+test("#15327 npmExecInvocation refuses to join a non-literal token (Hard Rule #13)", () => {
+  // The shell now receives a raw string, so a token that could change how the shell
+  // parses the line must fail loudly instead of being concatenated into it.
+  const unsafe = [
+    "omniroute && whoami",
+    "omniroute; whoami",
+    "omniroute | more",
+    "$(whoami)",
+    "`whoami`",
+    "a b",
+    'omniroute"',
+    "omniroute'",
+    "C:\\Users\\John Doe\\pkg",
+    "^&calc",
+  ];
+
+  for (const token of unsafe) {
+    assert.throws(
+      () => npmExecInvocation("win32", npmBin("win32"), [token]),
+      /Hard Rule #13/,
+      `${JSON.stringify(token)} must not be joined into a shell command line`
+    );
+  }
+
+  // The same tokens are harmless off Windows, where no shell parses the line — the
+  // new restriction must not leak onto the platforms that never needed one.
+  for (const token of unsafe) {
+    const [, args] = npmExecInvocation("linux", npmBin("linux"), [token]);
+    assert.deepEqual(args, [token], `linux must pass ${JSON.stringify(token)} through`);
+  }
+});
+
+test("#15327 npmExecInvocation still accepts the literal shapes the CLI spawns", () => {
+  const literals = [
+    "view",
+    "changelog",
+    "install",
+    "--prefer-online",
+    "--include=optional",
+    "--legacy-peer-deps",
+    "omniroute",
+    "@omniroute/opencode-plugin-v2",
+    "3.8.52",
+    "latest",
+    "npm.cmd",
+  ];
+
+  for (const token of literals) {
+    const [file, args, options] = npmExecInvocation("win32", "npm.cmd", [token]);
+    assert.deepEqual(args, []);
+    assert.equal(file, `npm.cmd ${token}`);
+    assert.equal(dep0190WouldFire(args, options), false);
+  }
+});
+
+test("#15327 the CLI's npm calls all resolve through npmExecInvocation", () => {
+  const src = fs.readFileSync(
+    new URL("../../bin/cli/commands/update.mjs", import.meta.url),
+    "utf8"
+  );
+
+  // The defect: an argv array handed to execFile alongside the win32 shell.
+  assert.equal(
+    /npmExecOptions\(\s*process\.platform/.test(src),
+    false,
+    "update.mjs must not build npm exec options itself — npmExecInvocation owns the win32 shape"
+  );
+
+  const binCalls = src.match(/npmBin\(/g) || [];
+  const invocationCalls = src.match(/npmExecInvocation\(/g) || [];
+  assert.ok(binCalls.length >= 2, "both the version and changelog lookups must be covered");
+  assert.equal(
+    binCalls.length,
+    invocationCalls.length,
+    "every npmBin() call site must go through npmExecInvocation()"
+  );
+});
+
+test("#15327 getLatestVersion passes no argv array on win32", async () => {
+  let captured: { file: string; args: string[]; options: Record<string, unknown> } | null = null;
+  const fakeExec = async (file: string, args: string[], options: Record<string, unknown>) => {
+    captured = { file, args, options };
+    return { stdout: "3.8.53\n" };
+  };
+
+  const latest = await update.getLatestVersion(fakeExec, "win32");
+
+  assert.equal(latest, "3.8.53");
+  assert.ok(captured, "exec must be invoked");
+  const call = captured as unknown as {
+    file: string;
+    args: string[];
+    options: Record<string, unknown>;
+  };
+
+  assert.deepEqual(call.args, [], "win32 must not pass an argv array alongside shell:true");
+  assert.equal(dep0190WouldFire(call.args, call.options), false);
+  assert.equal(call.file, "npm.cmd view omniroute version --prefer-online");
+  // The #4376 query itself is unchanged.
+  assert.ok(call.file.includes("--prefer-online"));
+});
+
+test("#15327 getLatestVersion keeps its argv array off win32", async () => {
+  let captured: { file: string; args: string[] } | null = null;
+  const fakeExec = async (file: string, args: string[]) => {
+    captured = { file, args };
+    return { stdout: "3.8.53\n" };
+  };
+
+  const latest = await update.getLatestVersion(fakeExec, "linux");
+  assert.equal(latest, "3.8.53");
+
+  const call = captured as unknown as { file: string; args: string[] };
+  assert.equal(call.file, "npm");
+  assert.deepEqual(call.args, ["view", "omniroute", "version", "--prefer-online"]);
+});
+
+test("#15327 the shell is still enabled exactly where #11335 needed it", () => {
+  // Guards the other half of the collision: dropping the argv array must not have
+  // been "fixed" by turning the shell off, which reintroduces the #11335 EINVAL.
+  assert.equal(npmExecOptions("win32").shell, true);
+  assert.equal(npmExecOptions("linux").shell, false);
+  assert.equal(npmExecOptions("darwin").shell, false);
+});

tests/unit/cli-update-npm-win32-11335.test.ts

@@ -55,24 +55,35 @@ test("#11335 every npm call in update.mjs routes through the helper", () => {
     'update.mjs must not spawn a literal "npm" — use npmBin()'
   );
 
-  const npmBinCalls = src.match(/npmBin\(\)/g) || [];
-  const optionCalls = src.match(/npmExecOptions\(/g) || [];
+  const npmBinCalls = src.match(/npmBin\(/g) || [];
+  // #15327 moved the win32 exec shape into npmExecInvocation(), which is now the only
+  // thing allowed to enable the shell — it hands execFile a joined command line with
+  // an empty argv array, since `shell: true` alongside a non-empty argv array is what
+  // DEP0190 fires on.
+  const invocationCalls = src.match(/npmExecInvocation\(/g) || [];
   assert.equal(
     npmBinCalls.length,
-    optionCalls.length,
-    "each npmBin() call site must pass npmExecOptions() alongside it"
+    invocationCalls.length,
+    "each npmBin() call site must go through npmExecInvocation() alongside it"
   );
   assert.ok(npmBinCalls.length >= 2, "both the version and changelog lookups must be covered");
+  assert.equal(
+    /npmExecOptions\(/.test(src),
+    false,
+    "update.mjs must not enable the shell itself — npmExecInvocation() owns that shape"
+  );
 });
 
 test("#11335 the shell is only enabled where argv is literal (Hard Rule #13)", () => {
   const src = fs.readFileSync(
     new URL("../../bin/cli/commands/update.mjs", import.meta.url),
     "utf8"
   );
-  // Both call sites pass a literal argv array; nothing interpolated reaches the
-  // shell. If that ever changes, this assertion is the thing that should fail.
-  const argvArrays = src.match(/npmBin\(\),\s*\n?\s*\[[^\]]*\]/g) || [];
+  // Both call sites pass a literal argv array to npmExecInvocation(); nothing
+  // interpolated reaches the shell. If that ever changes, this assertion is the thing
+  // that should fail. npmExecInvocation() additionally refuses to join a token that
+  // is not a literal — covered in cli-npm-exec-dep0190-15327.test.ts.
+  const argvArrays = src.match(/npmExecInvocation\([\s\S]*?\[([^\]]*)\]/g) || [];
   assert.ok(argvArrays.length >= 2);
   for (const argv of argvArrays) {
     assert.equal(/\$\{|\+\s*\w|\.\.\./.test(argv), false, `argv must stay literal: ${argv}`);