From 29bdb8dfded1576b4c7741ecfbb0bf4593341385 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Mon, 29 Jun 2026 20:45:30 -0300 Subject: [PATCH] fix(services): runNpm shell on win32 + prefix via env for Node 24 EINVAL (#5379) (#5474) Node 24 refuses execFile of npm.cmd without a shell (nodejs/node#52554), so embedded-service install (9Router/CLIProxy) failed with spawn EINVAL on Windows. runNpm now enables shell on win32 only; to stay Hard-Rule-#13 safe under a shell, the install --prefix is passed via npm_config_prefix (env) instead of an argv path (survives spaces), and the user-supplied version is constrained by SERVICE_VERSION_PATTERN at the route boundary. --- CHANGELOG.md | 4 + src/app/api/services/9router/install/route.ts | 8 +- .../api/services/cliproxy/install/route.ts | 8 +- src/lib/services/installers/ninerouter.ts | 14 +--- src/lib/services/installers/utils.ts | 71 ++++++++++++++++-- .../services/installers/ninerouter.test.ts | 5 +- .../installers/runNpm-shell-5379.test.ts | 74 +++++++++++++++++++ 7 files changed, 162 insertions(+), 22 deletions(-) create mode 100644 tests/unit/services/installers/runNpm-shell-5379.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f2404e3cc..e9a10bcd24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ ## [3.8.42] β€” TBD +### πŸ”§ Bug Fixes + +- **services (installer):** fix `spawn EINVAL` when installing an embedded service (9Router / CLIProxy) on **Windows + Node.js 24+**. Node 24 stopped letting `child_process.execFile()` run `.cmd` batch files without a shell (nodejs/node#52554), and npm on Windows is `npm.cmd`, so `runNpm()` threw `EINVAL` the moment a user clicked **Install**. `runNpm` now enables `shell` on win32 only. To keep Hard Rule #13 intact under a shell β€” where the shell, not `execFile`, parses argv β€” the install `--prefix` (a `DATA_DIR` path that can legitimately contain spaces, e.g. `C:\Users\John Doe\.omniroute\…`) is now passed via the `npm_config_prefix` **environment variable** instead of an argv path, and the user-supplied install `version` is constrained to a dist-tag/semver shape (`SERVICE_VERSION_PATTERN`) at the route boundary so it can never carry shell metacharacters. With the prefix in the environment and the version validated, every remaining argv entry is a static flag. Regression guards: `tests/unit/services/installers/runNpm-shell-5379.test.ts` (+ existing `ninerouter.test.ts` aligned to npm's `npm_config_prefix` env). ([#5379](https://github.com/diegosouzapw/OmniRoute/issues/5379)) + --- ## [3.8.41] β€” 2026-06-29 diff --git a/src/app/api/services/9router/install/route.ts b/src/app/api/services/9router/install/route.ts index a2df024eef..ee4a236489 100644 --- a/src/app/api/services/9router/install/route.ts +++ b/src/app/api/services/9router/install/route.ts @@ -1,11 +1,15 @@ import { z } from "zod"; import { install, InstallResult } from "@/lib/services/installers/ninerouter"; -import { InstallError } from "@/lib/services/installers/utils"; +import { InstallError, SERVICE_VERSION_PATTERN } from "@/lib/services/installers/utils"; import { createErrorResponse } from "@/lib/api/errorResponse"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const BodySchema = z.object({ - version: z.string().optional().default("latest"), + version: z + .string() + .regex(SERVICE_VERSION_PATTERN, "Invalid version: only letters, digits and . _ + - are allowed") + .optional() + .default("latest"), }); export async function POST(request: Request): Promise { diff --git a/src/app/api/services/cliproxy/install/route.ts b/src/app/api/services/cliproxy/install/route.ts index 591489cc24..a3ee88bc36 100644 --- a/src/app/api/services/cliproxy/install/route.ts +++ b/src/app/api/services/cliproxy/install/route.ts @@ -1,11 +1,15 @@ import { z } from "zod"; import { install, InstallResult } from "@/lib/services/installers/cliproxy"; -import { InstallError } from "@/lib/services/installers/utils"; +import { InstallError, SERVICE_VERSION_PATTERN } from "@/lib/services/installers/utils"; import { createErrorResponse } from "@/lib/api/errorResponse"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; const BodySchema = z.object({ - version: z.string().optional().default("latest"), + version: z + .string() + .regex(SERVICE_VERSION_PATTERN, "Invalid version: only letters, digits and . _ + - are allowed") + .optional() + .default("latest"), }); export async function POST(request: Request): Promise { diff --git a/src/lib/services/installers/ninerouter.ts b/src/lib/services/installers/ninerouter.ts index 44ef36eb5d..7535fa1715 100644 --- a/src/lib/services/installers/ninerouter.ts +++ b/src/lib/services/installers/ninerouter.ts @@ -77,16 +77,10 @@ export async function install(version = "latest"): Promise { } await runNpm( - [ - "install", - `${NINEROUTER_PACKAGE}@${version}`, - "--omit=dev", - "--no-audit", - "--no-fund", - "--prefix", - NINEROUTER_INSTALL_DIR, - ], - { cwd: NINEROUTER_INSTALL_DIR } + ["install", `${NINEROUTER_PACKAGE}@${version}`, "--omit=dev", "--no-audit", "--no-fund"], + // `--prefix` is passed via `prefix` (β†’ npm_config_prefix env) instead of an + // argv path so an install dir with spaces survives the Windows shell (#5379). + { cwd: NINEROUTER_INSTALL_DIR, prefix: NINEROUTER_INSTALL_DIR } ); const installedVersion = await getInstalledVersion(); diff --git a/src/lib/services/installers/utils.ts b/src/lib/services/installers/utils.ts index d69efc10be..a8efe66e48 100644 --- a/src/lib/services/installers/utils.ts +++ b/src/lib/services/installers/utils.ts @@ -73,10 +73,68 @@ function classifyError( return new InstallError(raw, `Falha na instalação: ${raw}`, 500); } -/** Runs npm with the given args array. Never uses shell interpolation. */ +/** + * Validates a user-supplied service version (npm dist-tag or semver). Constrained + * to letters, digits and `. _ + -`, with a leading alphanumeric, so the value can + * never carry shell metacharacters once `runNpm` runs under a shell on Windows + * (see `buildNpmExecOptions`). Accepts `latest`, `next`, `1.2.3`, `1.2.3-beta.1`, + * `1.2.3+build.5`; rejects `latest && calc`, `$(id)`, spaces, leading `-`, etc. + */ +export const SERVICE_VERSION_PATTERN = /^[A-Za-z0-9][A-Za-z0-9._+-]*$/; + +export interface NpmExecOptions { + cwd?: string; + timeout: number; + env: NodeJS.ProcessEnv; + maxBuffer: number; + shell?: boolean; +} + +/** + * Builds the `execFile` options for {@link runNpm}. + * + * On Windows, npm is `npm.cmd` (a batch wrapper). Node 24 refuses to `execFile` + * a `.cmd` without a shell (nodejs/node#52554 β€” manifests as `spawn EINVAL`, see + * issue #5379), so we enable `shell` on win32 only. + * + * Enabling the shell means the shell β€” not `execFile` β€” splits the command line, + * so NO runtime value may be interpolated into argv (Hard Rule #13). The install + * prefix (a DATA_DIR path that can legitimately contain spaces, e.g. + * `C:\Users\John Doe\.omniroute\…`) is therefore exported as the + * `npm_config_prefix` environment variable β€” npm's documented env form of + * `--prefix` β€” never as an argv entry. With the prefix moved to the environment + * and the version constrained by {@link SERVICE_VERSION_PATTERN}, every remaining + * argv entry is a static, metacharacter-free flag. + */ +export function buildNpmExecOptions( + platform: NodeJS.Platform, + options: { cwd?: string; timeoutMs: number; prefix?: string } +): NpmExecOptions { + const env: NodeJS.ProcessEnv = { ...process.env }; + if (options.prefix) { + env.npm_config_prefix = options.prefix; + } + const execOptions: NpmExecOptions = { + cwd: options.cwd, + timeout: options.timeoutMs, + env, + maxBuffer: 10 * 1024 * 1024, // 10 MB for npm output + }; + if (platform === "win32") { + execOptions.shell = true; + } + return execOptions; +} + +/** + * Runs npm with the given args array. Never uses shell interpolation: argv holds + * only static flags, and any install prefix is passed via `options.prefix` + * (exported as `npm_config_prefix`), not as an argv path. See + * {@link buildNpmExecOptions} for the Windows/Node-24 shell handling. + */ export function runNpm( args: string[], - options: { cwd?: string; timeoutMs?: number } = {} + options: { cwd?: string; timeoutMs?: number; prefix?: string } = {} ): Promise { const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS; // On Windows, npm is npm.cmd; on Unix it's npm. @@ -86,12 +144,11 @@ export function runNpm( execFile( npmBin, args, - { + buildNpmExecOptions(process.platform, { cwd: options.cwd, - timeout: timeoutMs, - env: process.env, - maxBuffer: 10 * 1024 * 1024, // 10 MB for npm output - }, + timeoutMs, + prefix: options.prefix, + }), (err, stdout, stderr) => { if (err) { const classified = classifyError( diff --git a/tests/unit/services/installers/ninerouter.test.ts b/tests/unit/services/installers/ninerouter.test.ts index 973f3b9f9c..fd56a9dd0d 100644 --- a/tests/unit/services/installers/ninerouter.test.ts +++ b/tests/unit/services/installers/ninerouter.test.ts @@ -24,11 +24,14 @@ set -e CMD="$1" shift if [ "$CMD" = "install" ]; then - # Find --prefix arg value + # Resolve the install prefix like real npm: an explicit --prefix arg wins, + # otherwise fall back to the npm_config_prefix env var (#5379 passes the + # prefix via env instead of argv so paths with spaces survive the win shell). PREFIX="" while [ $# -gt 0 ]; do if [ "$1" = "--prefix" ]; then PREFIX="$2"; shift 2; else shift; fi done + if [ -z "$PREFIX" ]; then PREFIX="$npm_config_prefix"; fi PKG_DIR="$PREFIX/node_modules/9router" mkdir -p "$PKG_DIR/app" echo '{"name":"9router","version":"0.4.59"}' > "$PKG_DIR/package.json" diff --git a/tests/unit/services/installers/runNpm-shell-5379.test.ts b/tests/unit/services/installers/runNpm-shell-5379.test.ts new file mode 100644 index 0000000000..0c8a9e64ad --- /dev/null +++ b/tests/unit/services/installers/runNpm-shell-5379.test.ts @@ -0,0 +1,74 @@ +/** + * Regression tests for #5379 β€” `spawn EINVAL` installing embedded services + * (9Router / CLIProxy) on Windows + Node.js 24+. + * + * Node 24 no longer lets `child_process.execFile()` run `.cmd` batch files on + * Windows without a shell (nodejs/node#52554). npm on Windows is `npm.cmd`, so + * `runNpm()` threw `EINVAL` immediately. The fix flips `shell` on win32. + * + * Because `shell: true` makes the shell β€” not execFile β€” parse the command line, + * NO runtime value may be interpolated into argv (Hard Rule #13). The install + * `--prefix` (a DATA_DIR path that can contain spaces, e.g. + * `C:\Users\John Doe\.omniroute\…`) is therefore passed via the + * `npm_config_prefix` environment variable instead of an argv entry, and the + * user-supplied install `version` is constrained by SERVICE_VERSION_PATTERN. + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + buildNpmExecOptions, + SERVICE_VERSION_PATTERN, +} from "../../../../src/lib/services/installers/utils.ts"; + +test("buildNpmExecOptions: win32 enables shell so npm.cmd runs on Node 24 (#5379)", () => { + const opts = buildNpmExecOptions("win32", { timeoutMs: 1000 }); + assert.equal(opts.shell, true); +}); + +test("buildNpmExecOptions: non-win32 platforms never enable shell", () => { + for (const platform of ["linux", "darwin", "freebsd"] as NodeJS.Platform[]) { + const opts = buildNpmExecOptions(platform, { timeoutMs: 1000 }); + assert.equal(opts.shell, undefined, `${platform} must not use a shell`); + } +}); + +test("buildNpmExecOptions: prefix is passed via npm_config_prefix env, never argv (Hard Rule #13)", () => { + const prefix = "C:\\Users\\John Doe\\.omniroute\\services\\9router"; + const opts = buildNpmExecOptions("win32", { timeoutMs: 1000, prefix }); + assert.equal(opts.env.npm_config_prefix, prefix); +}); + +test("buildNpmExecOptions: without a prefix npm_config_prefix is left untouched", () => { + const inherited = process.env.npm_config_prefix; + const opts = buildNpmExecOptions("linux", { timeoutMs: 1000 }); + assert.equal(opts.env.npm_config_prefix, inherited); +}); + +test("buildNpmExecOptions: carries cwd, timeout and maxBuffer through", () => { + const opts = buildNpmExecOptions("linux", { cwd: "/tmp/install", timeoutMs: 4242 }); + assert.equal(opts.cwd, "/tmp/install"); + assert.equal(opts.timeout, 4242); + assert.equal(opts.maxBuffer, 10 * 1024 * 1024); +}); + +test("SERVICE_VERSION_PATTERN: accepts dist-tags and semver", () => { + for (const v of ["latest", "next", "1.2.3", "1.2.3-beta.1", "1.2.3+build.5", "0.4.59"]) { + assert.ok(SERVICE_VERSION_PATTERN.test(v), `${v} should be valid`); + } +}); + +test("SERVICE_VERSION_PATTERN: rejects shell metacharacters (injection guard)", () => { + for (const v of [ + "latest && calc", + "1.2.3; rm -rf /", + "$(whoami)", + "`id`", + "a|b", + "a b", + "", + "-flag", + ]) { + assert.equal(SERVICE_VERSION_PATTERN.test(v), false, `${JSON.stringify(v)} must be rejected`); + } +});