fix(deploy): prevent command injection from angular.json in ng deploy

[herdiyana256]

The SSR to Cloud Functions deploy builder shells out with values taken verbatim from the workspace angular.json.

findPackageVersion builds `${packageManager} list ${name}` and passes it to execSync. packageManager comes from cli.packageManager and name from each architect.<project>.server.options.externalDependencies entry. Both are read with a raw JSON.parse(readFileSync('angular.json')), so the Angular CLI's own packageManager enum validation never applies. deployToFunction similarly runs `npm --prefix ${functionsOut} install`, where functionsOut derives from the outputPath deploy option. Any of these lets a malicious or cloned Angular workspace execute arbitrary commands the moment a developer runs ng deploy, for example externalDependencies: ["x; <command> #"]. This is the same threat model as the recently addressed deploy argv-injection, on distinct sinks that fix did not cover.

The fix stops running these values through a shell. cli.packageManager is validated against the supported set and each externalDependencies entry is rejected unless it is a plain package specifier, then the package manager and npm are launched with argument arrays through cross-spawn (v7). cross-spawn escapes each argument and, on Windows, launches the package manager's .cmd/.bat shim through cmd.exe, which child_process.execFile cannot do, so the fix is cross-platform and never falls back to shell: true (whose args-array form Node runtime-deprecates under DEP0190). Both calls funnel through a single processHost.runPackageBin, and tests assert the deploy code shells out only through that shell-free runner and validates its inputs first, so a later regression to a shell or a dropped validator call fails the suite.

Scope: this closes the shell-execution sinks on the Cloud Functions path (findPackageVersion and the functions npm install). The Cloud Run path still interpolates angular.json values into the gcloud commands that spawnAsync splits on whitespace; that is argument injection rather than shell execution and overlaps #3726, so it is left as follow-up. The outputPath schema pattern was dropped: it rejected legitimate path characters ((, ), $) yet never excluded the whitespace the Cloud Run path actually splits on, so it added false negatives on the one path it touched and false positives everywhere.

npm run test:node passes (154 specs, 0 failures); lint and typecheck clean.

[armando-navarro]

Thanks for this, and for continuing to work the ng deploy injection surface. I reproduced the hole you describe: with an externalDependencies entry like "x; touch /tmp/pwned #", the old execSync shape runs the injected command, and your execFileSync conversion stops it.

There is one thing I think blocks merging as-is, and a few smaller notes that do not.

Blocking: this breaks ng deploy on Windows

On Windows, package managers are normally launched through .cmd shims, and Node's child_process docs state that .cmd/.bat files "cannot be launched using child_process.execFile()". The old execSync ran through the shell, which resolves those shims, so it worked. execFileSync does not, so as written I believe two calls would throw on Windows:

• execFileSync('npm', ['--prefix', functionsOut, 'install']) in deployToFunction, on the default SSR-to-Cloud-Functions deploy path. npm is hardcoded here and is always a .cmd shim on Windows, so this one breaks regardless of the configured package manager.
• execFileSync(packageManager, ['list', ...]) in findPackageVersion, whenever the configured manager is a .cmd shim, which is the standard form for npm, yarn, pnpm, and cnpm (pnpm's standalone .exe build being the exception).

The fix needs to launch the Windows .cmd shim in a cross-platform way while still escaping the arguments, so the injection this PR closes stays closed. cross-spawn (v7) does this: on Windows it runs the shim through cmd.exe with each argument escaped, so a metacharacter is not interpreted as shell syntax.

I would not reach for a plain shell: true on these execFileSync calls as the fix, for two reasons:

• With an args array, shell: true concatenates the arguments into one command line without that escaping, which is the combination Node now runtime-deprecates (DEP0190).
• Not every value interpolated here is covered by the new validators. When the deploy outputPath option is unset, functionsOut falls back to the server build target's outputPath, which the schema pattern does not guard.

I have not run this on Windows, so if your setup launches these differently I would like to hear it.

Non-blocking notes

• Spec count in the description. Locally npm run test:node reports 138 specs, not the 150 in the PR body. Worth correcting.
• The tests cover the validators but not their call sites. The new specs exercise assertSupportedPackageManager and assertSafeDependencyName directly, but I could not find a test that fails if the deploy code stops calling them:
  • Undoing the switch to execFileSync, or dropping a validator call from findPackageVersion, still passes the whole suite.
  • A test that spies on the command call, confirming it uses no shell and goes through the validator, would guard the fix itself so a later regression fails.
• Scope of the description. "prevent command injection from angular.json" is broader than what this PR changes:
  • deployToCloudRun still interpolates angular.json values (outputPath, functionName, firebaseProject, region) into the gcloud commands, which spawnAsync splits on whitespace. That is argument injection rather than shell execution, and it overlaps the surface from #3726.
  • I would narrow the description to the calls this PR changes, or note the Cloud Run path as follow-up.
• outputPath schema pattern. ^[^;&|$<>\n\r()]*$rejects</code>(<code>, </code>)<code>, and $`, which can appear in legitimate output paths, and it does not exclude whitespace. Two options:
  • Drop it, since the Functions path now goes through execFileSync and no longer needs it.
  • Or keep it and exclude whitespace too, so it also constrains cloudRunOut whenever the deploy outputPath is set.

I am happy to take another look once the Windows path is sorted.

[herdiyana256]

Thanks for the careful review, and for confirming the repro. Pushed 2ce37b4 addressing everything.

Blocking: Windows .cmd shims

Fixed by moving both calls to cross-spawn (v7), exactly as you suggested. Rather than call it inline I funnelled the two sites through one small runner:

export const processHost = {
  runPackageBin(command, args, options = {}) {
    const result = crossSpawn.sync(command, args, options);
    if (result.error) { throw result.error; }
    if (result.status !== 0) { throw new SchematicsException(...); }
    return result.stdout;
  },
};

findPackageVersion and the functions npm install now go through it. On Windows cross-spawn launches the .cmd/.bat shim through cmd.exe with each argument escaped, so both the hardcoded npm install and a .cmd-shim package manager work again, and the injection stays closed. No shell: true, so we do not hit the DEP0190 args-array concatenation you flagged. cross-spawn is moved from devDependencies to dependencies since it now runs at deploy time.

Tests now guard the call sites

Added a call sites route through the shell-free runner block. It spies on processHost.runPackageBin and asserts:

• the functions npm install calls it with ['--prefix', <out>, 'install'] and no shell option,
• findPackageVersion calls it with a validated ['list', <name>] argv,
• an unsupported package manager and an unsafe dependency name each throw before anything is spawned (runPackageBin never called).

Reverting to execSync/shell: true, or dropping either validator call, now fails the suite.

outputPath schema pattern

Dropped it. As you noted the Functions path no longer needs it, and the pattern was the worst of both: it rejected legitimate (, ) and $ in paths while never excluding the whitespace that the Cloud Run spawnAsync path actually splits on, so it did not even constrain cloudRunOut. Excluding whitespace instead would have broken legitimate paths (e.g. Windows Program Files), so removal is cleaner.

Description scope and Cloud Run

Narrowed the description to the two shell-execution sinks this PR changes, and called out the Cloud Run gcloud interpolation as argument injection that overlaps #3726 and is left as follow-up.

Spec count

On a clean npm run build:jasmine both npm run test:node and test:node-esm report the same number here, now 154 with the four call-site specs added (150 before). The 138 you saw looks like a stale jasmine build; a fresh build:jasmine lined the two runners up for me. Happy to double-check if you still see a gap after a clean build.

Lint and typecheck are clean. I have not run it on Windows either, but the cross-spawn path is the standard cross-platform launcher for exactly this shim problem.