Fix start and other npm exec commands

[damyanpetev]

Closes #1512 .

Additional information related to this pull request:
See issue details here: #1512 (comment)

• reverted most of the changes from resolve code scanning vulnerability alerts #1331 and fix: command injection vulnerabilities in PackageManager and start command #1438 swapping exec with spawn because:

  • spawn is not equivalent to exec - it does not throw for bad execution exit, in fact it barely throws for anything other than call configuration, so it fails 'silently' for our use cases and required handling rework where called
  • spawn on Windows can't run npm since it's a script without shell evaluation option; used to be able to run npm.cmd but that's also not available since node@24+; I've checked and most secure calls into spawn that want to call in some node package rely on process.execPath and the path to the script (essentially calling node path/to/file.js) which could work for installed packages (think tsc), but no reliable way to resolve the npm path itself

• Dropped extra param where it wasn't really needed and added a sanitize function

• Swapped one older call using spawn w/ npm.cmd since that no longer works with node@24+

Note: CodeQL doesn't pick up the sanitize function out-of-the-box and my customization attempts have failed thus far - will leave for a separate PR. In fact, might re-work the command usage entirely to be async first and switch to writing to package.json instead with a single static command call at the end.

To fix the problem, avoid constructing shell command strings that embed fullPackageRegistry and instead supply the registry value as an argument to a child process API that does not invoke a shell (for example, child_process.spawnSync with shell: false). This prevents shell interpretation of metacharacters and sidesteps reliance on custom sanitization. The behavior (running npm whoami, npm login, and npm config set) should be preserved while changing only how these commands are invoked.

Concretely, we will:

• Add a small helper execSyncCmd in Util that wraps spawnSync to execute a command with an argv array and similar error handling to the existing execSync.
• Update PackageManager.ensureRegistryUser so that:
  • npm whoami is executed as Util.execSyncCmd("npm", ["whoami", --registry=${fullPackageRegistry}], { ... }).
  • npm login is executed similarly with its arguments array.
  • npm config set is executed as Util.execSyncCmd("npm", ["config", "set", "@infragistics:registry", fullPackageRegistry]);.
• Keep Util.sanitizeShellArg in place (it still helps validate/normalize the registry), but no longer depend on it for shell safety.

All changes will be contained within the shown snippets in packages/core/util/Util.ts (add the helper method) and packages/core/packages/PackageManager.ts (switch calls from execSync with a string to the new helper with argv arrays).

In general terms, the fix is to stop interpolating fullPackageRegistry into a shell command string and instead pass it as a separate argument to a non‑shell API. That means replacing execSync with execFileSync (which takes a program and an array of arguments) or, for the wrapper, adding a variant that accepts command and args separately. This avoids any shell parsing of the argument, so even if fullPackageRegistry contains problematic characters, they will be treated as literal argument content, not control characters.

The minimal, behavior‑preserving change here is to (1) add a new helper in Util that uses execFileSync with an explicit command and argument array, and (2) update PackageManager.ensureRegistryUser to call this new helper instead of building a string command. Specifically:

• In packages/core/util/Util.ts, add a static method, e.g. execFileSync(command: string, args: string[], options?: ExecSyncOptions), that wraps Node’s child_process.execFileSync. This keeps the same error‑handling semantics as the current execSync wrapper. We must also import execFileSync from child_process.
• In packages/core/packages/PackageManager.ts, change the three Util.execSync(...) calls inside ensureRegistryUser to use Util.execFileSync with explicit argument arrays:
  • npm whoami --registry=<url> → Util.execFileSync("npm", ["whoami", --registry=${fullPackageRegistry}], { ... })
  • npm login --registry=<url> --scope=@infragistics --auth-type=legacy → Util.execFileSync("npm", ["login", --registry=${fullPackageRegistry}, "--scope=@infragistics", "--auth-type=legacy"], { stdio: "inherit" })
  • npm config set @infragistics:registry <url> → Util.execFileSync("npm", ["config", "set", "@infragistics:registry", fullPackageRegistry]);
    This keeps command semantics identical while eliminating shell string construction with tainted data.

General approach: Avoid building shell command strings that include tainted data and then passing them to execSync. Instead, pass the command and its arguments as separate parameters to child_process.spawnSync (or execFileSync), which does not invoke a shell and thus prevents shell interpretation of user‑controlled data. Where a helper (Util.execSync) currently wraps execSync, add a parallel helper that wraps spawnSync with an argument array, and update call sites that build dynamic commands from library inputs to use the array‑based helper.

Concrete fix:

1. In packages/core/util/Util.ts, add a new static method, execFileSync, that accepts a command, an array of arguments, and optional options, and internally calls spawnSync. It should mirror the error‑handling logic of execSync as closely as possible (status checks, SIGINT handling) so behavior stays consistent.

2. In packages/core/packages/PackageManager.ts, stop composing npm commands as single strings for the cases where arguments contain library input:

   • In installPackages, instead of command = \${managerCommand} install --quiet`andUtil.execSync(command, ...), build const args = ["install", "--quiet"];and callUtil.execFileSync(managerCommand, args, options)`.
   • In removePackage, instead of command = \${managerCommand} uninstall ${sanitizePackage} --quiet --save`, build const args = ["uninstall", sanitizePackage, "--quiet", "--save"];and callUtil.execFileSync(managerCommand, args, options)`.
   • In addPackage, modify getInstallCommand usage so that it returns args instead of a string, or, if we cannot modify that function here, reconstruct the equivalent array in addPackage. Since we only see the call site, we’ll construct the array directly in addPackage using managerCommand and sanitizePackage.

3. Keep Util.sanitizeShellArg as‑is (it still adds a defense in depth against malformed or unexpected package names), but after the change it will no longer be relied upon for shell escaping, only for validating/normalizing input.

Imports: Util.ts already imports spawnSync and SpawnSyncOptions, so we can reuse those; no new external packages are needed.

---

[damyanpetev]

Wouldn't be reverting if spawn was a valid option for npm commands cross-plat...

[coveralls]

coverage: 70.495% (+0.1%) from 70.394%
when pulling cbb6502 on dpetev/fix-start-process-exec
into 0157241 on master.

In general, to fix this problem we should avoid constructing shell commands as single strings that are then interpreted by a shell. Instead, we should call npm with an API that accepts an executable and a list of arguments (for example, spawnSync or execFile) so that the registry URL is passed as a discrete argument and is not subject to shell parsing. This removes the need to rely on ad-hoc sanitization for correctness and security.

The best change with minimal functional impact is to update Util.execSync (the central helper) so that it can accept either a string or an array form, and internally dispatch to child_process.execSync when given a string (preserving all existing behaviour) or to child_process.spawnSync when given an executable and argument array. Then we adjust the two npm calls in PackageManager.ensureRegistryUser to use the safer array form: Util.execSync(["npm", "whoami", --registry=${fullPackageRegistry}], options) and Util.execSync(["npm", "login", --registry=${fullPackageRegistry}, "--scope=@infragistics", "--auth-type=legacy"], { stdio: "inherit" }), plus the npm config set call similarly. This keeps the commands logically identical while avoiding shell parsing of fullPackageRegistry.

Concretely:

• In packages/core/util/Util.ts, change the signature of Util.execSync to accept command: string | string[]. Inside, if typeof command === "string", call execSync(command, options) as before; if it is an array, treat command[0] as the executable and command.slice(1) as its arguments, and invoke spawnSync with those, mimicking execSync’s behaviour and error handling as closely as practical (throw on non-zero exit, surface stdout/stderr, respect options.stdio to the extent possible).
• Preserve the existing error-handling logic for the string path, and for the array path, add analogous checks on result.status and result.stderr, throwing an Error when the spawned process fails.
• In packages/core/packages/PackageManager.ts, update the three Util.execSync calls in ensureRegistryUser to use the array form described above. We keep the existing fullPackageRegistry sanitizer and log messages, but the value is no longer interpolated into a shell string.

This keeps overall behaviour the same from the caller’s perspective while structurally eliminating the unsafe shell command construction for these flows.

In general, to fix this issue you should stop concatenating potentially untrusted input into a single string command executed by a shell. Instead, call child_process.execFileSync/spawnSync (or equivalent) with the command name and a list of arguments so that the shell does not interpret special characters. If you must use a shell, then you should properly quote/escape untrusted arguments using a robust library (for example shell-quote) rather than a custom sanitizer.

For this specific case, the commands being run are plain npm commands without pipes or redirections, so they can be safely executed without a shell. A minimal change that preserves existing functionality is:

• Add a new helper method in Util (e.g. execFileSyncSafe) that wraps child_process.spawnSync (already imported) to execute a command and arguments array without a shell, reusing the existing error-handling semantics of execSync as closely as practical.
• Update the three problematic calls in PackageManager.ensureRegistryUser:
  • Util.execSync(\npm whoami --registry=${fullPackageRegistry}`, ...)`
  • Util.execSync(\npm login --registry=${fullPackageRegistry} --scope=@Infragistics --auth-type=legacy`, ...)`
  • Util.execSync(\npm config set @Infragistics:registry ${fullPackageRegistry}`);`
    to instead call the new helper with explicit argument arrays:
  • Util.execFileSyncSafe("npm", ["whoami", --registry=${fullPackageRegistry}], {...});
  • Util.execFileSyncSafe("npm", ["login", --registry=${fullPackageRegistry}, "--scope=@infragistics", "--auth-type=legacy"], {...});
  • Util.execFileSyncSafe("npm", ["config", "set", "@infragistics:registry", fullPackageRegistry]);

Because spawnSync is already imported in Util.ts, no new imports are needed. The new helper should throw an Error in a way consistent with execSync when the child process fails (non‑zero exit code), so callers like ensureRegistryUser keep working as before.

---