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.

[armando-navarro]

This addresses the blocker, thanks for turning it around so quickly.

One change I would like before this goes in, and it is about the dependency rather than the fix itself.

Please move cross-spawn back to a devDependency

The schematics build bundles it, so it never needs to be installed alongside the published package:

• It is not in the external list in compileSchematics, so esbuild inlines it.
• The root package.json is "private": true, so it is not the manifest consumers install. That is src/package.json, which does not list cross-spawn.
• No file in dist/packages-dist resolves cross-spawn externally.

I checked this end to end rather than by reading the config, since it is the reason I am asking. With cross-spawn moved to devDependencies I ran a full build, packed the tarball, and installed it into a clean project.

Everything else here looks good to me, and the Windows problem is properly solved.

[herdiyana256]

Done in b7747c3, cross-spawn moved back to devDependencies.

You were right on every point, and it turned out the manifest was the only thing out of step: the lockfile already had cross-spawn recorded under the root devDependencies (with the resolved node_modules/cross-spawn entry marked "dev": true), so this commit just realigns package.json with what the lockfile already believed. npm install --package-lock-only produces no diff.

I verified the bundling end to end rather than by reading the config:

• Full npm run build passes, including loadCompiledSchematics, which requires each compiled bundle, so the deploy schematic still loads with cross-spawn inlined and no external resolve.
• The packed dist/packages-dist/package.json does not list cross-spawn (it is src/package.json, as you noted), and dist/packages-dist/schematics/deploy/actions.js contains the inlined cross-spawn internals.
• test:node and test:node-esm both report 142 specs, 0 failures.

Lint and typecheck are clean. Thanks for checking the tarball install by hand, that is what surfaced the mismatch.

[armando-navarro]

Thanks for making the requested change. Everything looks good now. Approving PR and merging.