Repository navigation
fix(cli): spawn npm without DEP0190 on win32 — pass a joined command line instead of an argv array - #15328
Merged
diegosouzapw merged 1 commit intoOct 6, 2026
Conversation
`npmExecOptions()` enables the shell on win32 so `npm.cmd` can be spawned at all (#11335), but `update.mjs` still handed `execFile` an argv array alongside it — and Node's `normalizeSpawnArguments` emits DEP0190 for exactly that combination (`options.shell` set AND `args.length > 0`). Node >= 24 therefore 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. on every `omniroute update` and on the doctor's version check — the command whose job is diagnosing the install. The two earlier fixes had collided: the shell is required for the `.cmd` shim and deprecated alongside an argv array. Node builds that command line itself in this path (`${file} ${args.join(" ")}`), so the same string is now handed over pre-joined with an EMPTY args array. `npmExecInvocation()` returns the exact `execFile` arguments for a platform. Hard Rule #13 now has to be enforced rather than assumed, because the shell is handed a raw string: a token outside a literal grammar throws instead of being joined, so a runtime value can never reach the shell's own parser. Off Windows the argv array is returned verbatim and no new restriction applies. Fixes #15327
diegosouzapw
merged commit Oct 6, 2026
6a8ee68
into
cheaper-inference:release/v3.8.52
3 checks passed
7 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Windows + Node >= 24,
omniroute update(and the doctor's version check) print a deprecation warning on every run:It is cosmetic, but it shows up on the command whose whole job is diagnosing the install, so it reads like the tool reporting a fault with itself.
Two earlier fixes collided:
shellon win32 sonpm.cmdcan be spawned at all — Node >= 24 refuses a.cmdwithout one (Command run failed with error : spawn EINVAL nodejs/node#52554).bin/cli/commands/update.mjskept the argv-array call shape, and Node'snormalizeSpawnArgumentswarns on exactly that combination:options.shellset andargs.length > 0.The shell is therefore required for the
.cmdshim and deprecated alongside an argv array — the argv array #11335 kept passing is the one thing that cannot go throughexecFilewith the shell enabled.Fix
Node builds that command line itself in this path (
${file} ${args.join(" ")}), so the same string is now handed over pre-joined with an empty args array.npmExecInvocation()returns the exactexecFilearguments for a platform:win32npm.cmd view omniroute version --prefer-online[]{ shell: true, windowsHide: true, timeout }npm["view", "omniroute", "version", "--prefer-online"]{ shell: false, timeout }Byte-identical input to the shell, minus the deprecation — the
npm.cmdspawn from #11335 is untouched, so the originalspawn EINVALcannot come back.Hard Rule #13 is now enforced rather than assumed. The shell is handed a raw string, so joining one is only safe while every token is a literal. A token outside a literal grammar (whitespace, a quote, a cmd/POSIX metacharacter, a backslash) throws instead of being concatenated, and the error names
escapeWindowsShellArg()frombin/cli/utils/winShellArgs.mjs— the helper #15327 points at — or the environment. A caller that genuinely has a runtime value fails loudly instead of letting the shell reinterpret it.Off Windows the argv array is returned verbatim and the tokens are not inspected at all, so the platforms that never needed a shell inherit no new restriction.
Scope is the CLI (
bin/cli/), as reported. The server-siderunNpm()insrc/lib/services/installers/utils.tspasses a runtimeargsarray withshell: truetoo; moving that to a joined string needs its--prefix/version handling reworked first, so it is left alone here rather than half-done.Related Issues
npm.cmd-needs-a-shell fix this collides with), Node warning after upgrading to v3.8.51 #15315, Omniroute 3.8.50 intermittently going offline #14240Validation
release/v3.8.52)release/v3.8.52is already red before this PR).Node ≥ 22 is required to observe the deprecation itself, and it is emitted at most once per process, so the regression guard asserts the property Node keys the warning on — no argv array travels with
shell: true— rather than trying to capture the warning. Also checked out-of-band that both shapes really spawn and produce identical stdout (echo hello world->hello worldon both paths).Not run locally:
npm run lint,npm run typecheck:coreand the fullnpm run test:unit. This checkout has nonode_modulesand the disk is at 98%, so the suite could not be installed; CI covers all three.Tests Added Or Updated
tests/unit/cli-npm-exec-dep0190-15327.test.ts(new) — the regression guard. TDD: 7 of its 9 tests fail against the pre-fix tree and all 9 pass after (the 2 that passed before are the unchanged-behavior controls). Covers: win32 passes no argv array withshell: true; the joined string carries every literal token verbatim with no quoting added or dropped; non-win32 keeps the argv array untouched;npmExecInvocationrefuses ten non-literal shapes (&&,;,|,$( ), backticks, embedded space, both quote characters, aC:\Users\John Doe\...path,^&calc) while passing them through on linux; the twelve literal shapes the CLI actually spawns are still accepted; both npm call sites inupdate.mjsroute through the helper;getLatestVersionpasses no argv array on win32 and still keeps it off win32; and the shell is still enabled exactly where [BUG] omniroute update fails on Windows — 'Could not check latest version' (execFile npm without shell in update.mjs, Node 24+ hardening, same class as #5379) #11335 needed it.tests/unit/cli-update-npm-win32-11335.test.ts(updated) — its two structural assertions pinned the argv-array call shape that fix(nodejs): Windows npm spawn passes an argv array with shell:true — Node 24 prints DEP0190 on omniroute update/doctor #15327 has to remove, so they now assert the new invariant: everynpmBin()call site goes throughnpmExecInvocation(),update.mjsno longer builds npm exec options itself, and the argv arrays it passes stay literal. Its behavioural assertions ([BUG] omniroute update fails on Windows — 'Could not check latest version' (execFile npm without shell in update.mjs, Node 24+ hardening, same class as #5379) #11335npm.cmd+ shell, non-win32 shell off, the--prefer-onlineinstall-mode strings) are unchanged and still pass.Coverage Notes
bin/is covered by the two files above. The new export is pure and fully exercised on both platform branches plus its rejection path; the only uncovered line would be theargs ?? []default, which no caller uses.Reviewer Notes
npmExecInvocationthrows on a non-literal token instead of quoting it. Escaping would need the double-caret ruleescapeWindowsShellArgalready implements for a different caller, and silently applying it to a value that was never validated would be the more dangerous default. Both current call sites pass literals, so nothing changes for them today — the throw is there so the next caller cannot quietly break Hard Rule deps: bump qs from 6.14.1 to 6.14.2 #13.getLatestVersion()gained an optional secondplatformparameter (defaults toprocess.platform) so the win32 path is testable from a non-Windows runner. Existing single-argument callers and tests are unaffected.printPostApplyGuidancelines Prettier wants to rewrap are pre-existing violations in the base file and were deliberately left alone to keep this diff to the defect.