From ed6b8b46ce5acc65d623626ef1a8c53906befadf Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Mon, 10 Aug 2026 23:22:43 +0700 Subject: [PATCH 1/2] fix(deploy): prevent command injection from angular.json in ng deploy The SSR->Cloud Functions deploy builder shelled out with values read verbatim from the workspace angular.json. findPackageVersion built `${packageManager} list ${name}` for execSync, where packageManager comes from cli.packageManager and name from each server.options.externalDependencies entry (both parsed with a raw JSON.parse, so the Angular CLI's own validation never runs). deployToFunction likewise ran `npm --prefix ${functionsOut} install`, with functionsOut derived from the outputPath deploy option. A malicious or cloned Angular workspace could therefore run arbitrary commands the moment a developer runs ng deploy. Run the package manager and npm without a shell (execFileSync with argument arrays), validate cli.packageManager against the supported set, reject externalDependencies entries that are not plain package specifiers, and constrain outputPath in the deploy schema. Adds unit tests for both validators. --- src/schematics/deploy/actions.jasmine.ts | 35 +++++++++++++++- src/schematics/deploy/actions.ts | 51 ++++++++++++++++++++++-- src/schematics/deploy/schema.json | 3 +- 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/src/schematics/deploy/actions.jasmine.ts b/src/schematics/deploy/actions.jasmine.ts index 1795ea10d..7d07ff7ea 100644 --- a/src/schematics/deploy/actions.jasmine.ts +++ b/src/schematics/deploy/actions.jasmine.ts @@ -3,7 +3,7 @@ import { join } from 'path'; import { BuilderContext, BuilderRun, ScheduleOptions, Target } from '@angular-devkit/architect'; import { JsonObject, logging } from '@angular-devkit/core'; import { BuildTarget, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces'; -import deploy, { deployToFunction } from './actions.js' +import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction } from './actions.js' import 'jasmine'; let context: BuilderContext; @@ -300,3 +300,36 @@ describe('universal deployment', () => { expect(spy).not.toHaveBeenCalled(); });*/ }); + +describe('deploy input validation (command-injection hardening)', () => { + describe('assertSupportedPackageManager', () => { + ['npm', 'yarn', 'pnpm', 'cnpm', 'bun'].forEach((pm) => { + it(`allows the supported package manager "${pm}"`, () => { + expect(assertSupportedPackageManager(pm)).toBe(pm); + }); + }); + + it('rejects a package manager carrying a shell payload', () => { + expect(() => assertSupportedPackageManager('npm; touch /tmp/pwned #')) + .toThrowError(/Unsupported package manager/); + }); + + it('rejects an arbitrary executable path', () => { + expect(() => assertSupportedPackageManager('/tmp/evil')).toThrowError(/Unsupported package manager/); + }); + }); + + describe('assertSafeDependencyName', () => { + ['rxjs', '@angular/core', '@angular/*', 'some-pkg', 'a.b_c'].forEach((name) => { + it(`allows the valid dependency name "${name}"`, () => { + expect(assertSafeDependencyName(name)).toBe(name); + }); + }); + + ['evil; touch /tmp/pwned #', 'a b', '$(id)', '`id`', 'a|b', 'a&b', '-rf', '', 'a>b'].forEach((name) => { + it(`rejects the unsafe dependency name ${JSON.stringify(name)}`, () => { + expect(() => assertSafeDependencyName(name)).toThrowError(/Invalid dependency name/); + }); + }); + }); +}); diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index cecf255bf..b4be28635 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -1,4 +1,4 @@ -import { SpawnOptionsWithoutStdio, execSync, spawn } from 'child_process'; +import { SpawnOptionsWithoutStdio, execFileSync, spawn } from 'child_process'; import { existsSync, readFileSync, renameSync, writeFileSync } from 'fs'; import { dirname, join } from 'path'; import { fileURLToPath } from 'url'; @@ -121,8 +121,50 @@ const defaultFsHost: FSHost = { existsSync, }; +// Package managers AngularFire is willing to shell out to when resolving +// dependency versions. Kept in sync with the Angular CLI's own list. The value +// comes from `cli.packageManager` in angular.json, which this builder reads with +// a raw `JSON.parse`, i.e. it is NOT run through the Angular CLI's own schema +// validation, so it must be checked here before it is ever used as an argv0. +export const SUPPORTED_PACKAGE_MANAGERS = ['npm', 'yarn', 'pnpm', 'cnpm', 'bun']; + +export const assertSupportedPackageManager = (packageManager: string): string => { + if (!SUPPORTED_PACKAGE_MANAGERS.includes(packageManager)) { + throw new SchematicsException( + `Unsupported package manager "${packageManager}" in angular.json (cli.packageManager). ` + + `Expected one of: ${SUPPORTED_PACKAGE_MANAGERS.join(', ')}.` + ); + } + return packageManager; +}; + +// A dependency name comes from `architect..server.options.externalDependencies` +// in angular.json. Reject anything that is not a plain package specifier so it can +// neither inject shell metacharacters (defence in depth alongside execFileSync) nor +// be parsed as a CLI flag by the package manager (argument injection). +export const assertSafeDependencyName = (name: string): string => { + // Valid npm package names / esbuild external globs never contain whitespace or + // shell metacharacters, and never start with a dash. Reject anything else so the + // value can neither inject a shell command (defence in depth alongside + // execFileSync) nor be parsed as a package-manager flag (argument injection). + if (typeof name !== 'string' || name.length === 0 || name.startsWith('-') || + /[\s;&|$`(){}<>!\\'"]/.test(name)) { + throw new SchematicsException( + `Invalid dependency name ${JSON.stringify(name)} in angular.json (server externalDependencies).` + ); + } + return name; +}; + const findPackageVersion = (packageManager: string, name: string) => { - const match = execSync(`${packageManager} list ${name}`).toString().match(`[^|s]${escapeRegExp(name)}[@| ][^s]+(s.+)?$`); + // Run the package manager without a shell (execFileSync + argument array) so a + // dependency name or package-manager value taken from angular.json cannot be + // interpreted as a shell command. + const output = execFileSync(assertSupportedPackageManager(packageManager), [ + 'list', + assertSafeDependencyName(name), + ]).toString(); + const match = output.match(`[^|s]${escapeRegExp(name)}[@| ][^s]+(s.+)?$`); return match ? match[0].split(new RegExp(`${escapeRegExp(name)}[@| ]`))[1].split(/\s/)[0] : null; }; @@ -245,7 +287,10 @@ export const deployToFunction = async ( const siteTarget = options.target ?? context.target!.project; if (fsHost.existsSync(functionsPackageJsonPath)) { - execSync(`npm --prefix ${functionsOut} install`); + // Pass the output directory as an argument rather than interpolating it into + // a shell string; `functionsOut` derives from the `outputPath` deploy option + // (angular.json) and must not be able to inject shell commands. + execFileSync('npm', ['--prefix', functionsOut, 'install']); } else { console.error(`No package.json exists at ${functionsOut}`); } diff --git a/src/schematics/deploy/schema.json b/src/schematics/deploy/schema.json index 6335d3a8d..789962552 100644 --- a/src/schematics/deploy/schema.json +++ b/src/schematics/deploy/schema.json @@ -67,7 +67,8 @@ }, "outputPath": { "type": "string", - "description": "Where to output the deploy artifacts" + "description": "Where to output the deploy artifacts", + "pattern": "^[^;&|$`<>\\n\\r()]*$" }, "functionsRuntimeOptions": { "type": "object", From 2ce37b4b1f063525dd3cec7c4fe6fb2480c7e949 Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Wed, 12 Aug 2026 08:17:20 +0700 Subject: [PATCH 2/2] fix(deploy): launch package managers cross-platform via cross-spawn execFileSync cannot launch the .cmd/.bat shims that npm, yarn, pnpm and cnpm ship as on Windows, so the previous conversion broke `ng deploy` there on the default SSR-to-Cloud-Functions path. Route the npm install and the ` list` calls through cross-spawn instead: it escapes each argument and, on Windows, invokes the shim through cmd.exe, keeping the injection closed without falling back to `shell: true` (whose args-array form Node runtime-deprecates under DEP0190). Funnel both calls through a single exported processHost.runPackageBin so tests can assert the deploy code shells out only through the shell-free runner. Add specs that fail if a call site regresses to a shell or drops a validator. Drop the outputPath schema pattern, which rejected legit path characters while never guarding the whitespace that the Cloud Run path actually splits on; that argument-injection surface is tracked separately in #3726. --- package-lock.json | 11 +++++ package.json | 3 +- src/schematics/deploy/actions.jasmine.ts | 57 +++++++++++++++++++++++- src/schematics/deploy/actions.ts | 51 ++++++++++++++++----- src/schematics/deploy/schema.json | 3 +- 5 files changed, 111 insertions(+), 14 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8c679f4e4..2fd3efd8c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -45,6 +45,7 @@ "@angular/cli": "^21.0.0", "@angular/compiler-cli": "^21.0.0", "@angular/platform-server": "^21.0.0", + "@types/cross-spawn": "^6.0.6", "@types/fs-extra": "^7.0.0", "@types/gzip-size": "^5.1.1", "@types/inquirer": "^0.0.44", @@ -8667,6 +8668,16 @@ "@types/node": "*" } }, + "node_modules/@types/cross-spawn": { + "version": "6.0.6", + "resolved": "https://registry.npmjs.org/@types/cross-spawn/-/cross-spawn-6.0.6.tgz", + "integrity": "sha512-fXRhhUkG4H3TQk5dBhQ7m/JDdSNHKwR2BBia62lhwEIq9xGiQKLxd6LymNhn47SjXhsUEPmxi+PKw2OkW4LLjA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/eslint": { "version": "9.6.1", "resolved": "https://registry.npmjs.org/@types/eslint/-/eslint-9.6.1.tgz", diff --git a/package.json b/package.json index b960c91f5..b542feb59 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "@angular/platform-browser-dynamic": "^21.0.0", "@angular/router": "^21.0.0", "@schematics/angular": "^21.0.0", + "cross-spawn": "^7.0.3", "esbuild": "^0.24.0", "firebase": "^12.4.0", "firebase-functions": "^6.1.0", @@ -86,6 +87,7 @@ "@angular/cli": "^21.0.0", "@angular/compiler-cli": "^21.0.0", "@angular/platform-server": "^21.0.0", + "@types/cross-spawn": "^6.0.6", "@types/fs-extra": "^7.0.0", "@types/gzip-size": "^5.1.1", "@types/inquirer": "^0.0.44", @@ -99,7 +101,6 @@ "@typescript-eslint/eslint-plugin": "^8.33.0", "@typescript-eslint/parser": "^8.33.0", "conventional-changelog-cli": "^1.2.0", - "cross-spawn": "^7.0.3", "eslint": "^9.27.0", "eslint-plugin-import": "^2.31.0", "globals": "^13.21.0", diff --git a/src/schematics/deploy/actions.jasmine.ts b/src/schematics/deploy/actions.jasmine.ts index 7d07ff7ea..e6c8b839b 100644 --- a/src/schematics/deploy/actions.jasmine.ts +++ b/src/schematics/deploy/actions.jasmine.ts @@ -3,7 +3,7 @@ import { join } from 'path'; import { BuilderContext, BuilderRun, ScheduleOptions, Target } from '@angular-devkit/architect'; import { JsonObject, logging } from '@angular-devkit/core'; import { BuildTarget, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces'; -import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction } from './actions.js' +import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction, findPackageVersion, processHost } from './actions.js' import 'jasmine'; let context: BuilderContext; @@ -332,4 +332,59 @@ describe('deploy input validation (command-injection hardening)', () => { }); }); }); + + // These guard the fix at its call sites: the validators above are only useful + // if the deploy code keeps routing every command through the shell-free runner. + // A regression to execSync/`shell: true`, or a dropped validator call, fails here. + describe('call sites route through the shell-free runner', () => { + beforeEach(() => initMocks()); + + it('installs functions dependencies via the runner with an argv array and no shell', async () => { + // The install branch only runs when the generated package.json exists. + spyOn(fsHost, 'existsSync').and.returnValue(true); + const runSpy = spyOn(processHost, 'runPackageBin').and.returnValue(Buffer.from('')); + + await deployToFunction( + firebaseMock, + context, + workspaceRoot, + STATIC_BUILD_TARGET, + SERVER_BUILD_TARGET, + { preview: false }, + undefined, + fsHost + ); + + expect(runSpy).toHaveBeenCalledTimes(1); + const [command, args, options] = runSpy.calls.mostRecent().args; + expect(command).toBe('npm'); + expect(args).toEqual(['--prefix', join(workspaceRoot, 'dist'), 'install']); + // No shell: a `shell` option would reopen the injection this PR closes. + expect((options as any)?.shell).toBeFalsy(); + }); + + it('runs the package manager through the runner with a validated argv array', () => { + const runSpy = spyOn(processHost, 'runPackageBin').and.returnValue(Buffer.from('')); + + findPackageVersion('npm', 'rxjs'); + + expect(runSpy).toHaveBeenCalledWith('npm', ['list', 'rxjs']); + }); + + it('rejects an unsupported package manager before spawning anything', () => { + const runSpy = spyOn(processHost, 'runPackageBin'); + + expect(() => findPackageVersion('npm; touch /tmp/pwned #', 'rxjs')) + .toThrowError(/Unsupported package manager/); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it('rejects an unsafe dependency name before spawning anything', () => { + const runSpy = spyOn(processHost, 'runPackageBin'); + + expect(() => findPackageVersion('npm', 'evil; touch /tmp/pwned #')) + .toThrowError(/Invalid dependency name/); + expect(runSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index b4be28635..3b2bea072 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -1,9 +1,10 @@ -import { SpawnOptionsWithoutStdio, execFileSync, spawn } from 'child_process'; +import { SpawnOptionsWithoutStdio, SpawnSyncOptions, spawn } from 'child_process'; import { existsSync, readFileSync, renameSync, writeFileSync } from 'fs'; import { dirname, join } from 'path'; import { fileURLToPath } from 'url'; import { BuilderContext, targetFromTargetString } from '@angular-devkit/architect'; import { SchematicsException } from '@angular-devkit/schematics'; +import crossSpawn from 'cross-spawn'; import fsExtra from 'fs-extra'; import * as inquirer from 'inquirer'; import open from 'open'; @@ -156,11 +157,39 @@ export const assertSafeDependencyName = (name: string): string => { return name; }; -const findPackageVersion = (packageManager: string, name: string) => { - // Run the package manager without a shell (execFileSync + argument array) so a - // dependency name or package-manager value taken from angular.json cannot be - // interpreted as a shell command. - const output = execFileSync(assertSupportedPackageManager(packageManager), [ +// All shelling out from the deploy builder funnels through this single runner. +// cross-spawn (v7) resolves the platform-appropriate executable and escapes each +// argument, so a value taken from angular.json is passed as an argv entry and can +// never be parsed as shell syntax. Unlike child_process.execFile it can launch a +// Windows `.cmd`/`.bat` shim (npm, yarn, pnpm and cnpm all ship as `.cmd` shims +// there), so `ng deploy` keeps working cross-platform. We deliberately avoid +// `shell: true`, whose args-array form Node runtime-deprecates (DEP0190) because +// it concatenates the arguments without escaping, reopening the injection. +// It is exported as an object so tests can assert the deploy code shells out only +// through here and never regresses to execSync or `shell: true`. +export const processHost = { + runPackageBin( + command: string, + args: string[], + options: SpawnSyncOptions = {}, + ): Buffer { + const result = crossSpawn.sync(command, args, options); + if (result.error) { throw result.error; } + if (result.status !== 0) { + throw new SchematicsException( + `Command "${command}" exited with ${result.signal ? `signal ${result.signal}` : `code ${result.status}`}.` + ); + } + return result.stdout; + }, +}; + +export const findPackageVersion = (packageManager: string, name: string) => { + // Run the package manager without a shell (argument array, no `shell` option) + // so a dependency name or package-manager value taken from angular.json cannot + // be interpreted as a shell command. Both values are validated first, so an + // unsupported manager or unsafe name throws before anything is ever spawned. + const output = processHost.runPackageBin(assertSupportedPackageManager(packageManager), [ 'list', assertSafeDependencyName(name), ]).toString(); @@ -287,10 +316,12 @@ export const deployToFunction = async ( const siteTarget = options.target ?? context.target!.project; if (fsHost.existsSync(functionsPackageJsonPath)) { - // Pass the output directory as an argument rather than interpolating it into - // a shell string; `functionsOut` derives from the `outputPath` deploy option - // (angular.json) and must not be able to inject shell commands. - execFileSync('npm', ['--prefix', functionsOut, 'install']); + // Pass the output directory as an argv entry rather than interpolating it + // into a shell string; `functionsOut` derives from the `outputPath` deploy + // option (angular.json) and must not be able to inject shell commands. npm is + // a `.cmd` shim on Windows, which child_process.execFile cannot launch, so + // this goes through the shell-free cross-spawn runner instead. + processHost.runPackageBin('npm', ['--prefix', functionsOut, 'install'], { stdio: 'inherit' }); } else { console.error(`No package.json exists at ${functionsOut}`); } diff --git a/src/schematics/deploy/schema.json b/src/schematics/deploy/schema.json index 789962552..6335d3a8d 100644 --- a/src/schematics/deploy/schema.json +++ b/src/schematics/deploy/schema.json @@ -67,8 +67,7 @@ }, "outputPath": { "type": "string", - "description": "Where to output the deploy artifacts", - "pattern": "^[^;&|$`<>\\n\\r()]*$" + "description": "Where to output the deploy artifacts" }, "functionsRuntimeOptions": { "type": "object",