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 1795ea10d..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, { deployToFunction } from './actions.js' +import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction, findPackageVersion, processHost } from './actions.js' import 'jasmine'; let context: BuilderContext; @@ -300,3 +300,91 @@ 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/); + }); + }); + }); + + // 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 cecf255bf..3b2bea072 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -1,9 +1,10 @@ -import { SpawnOptionsWithoutStdio, execSync, 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'; @@ -121,8 +122,78 @@ const defaultFsHost: FSHost = { existsSync, }; -const findPackageVersion = (packageManager: string, name: string) => { - const match = execSync(`${packageManager} list ${name}`).toString().match(`[^|s]${escapeRegExp(name)}[@| ][^s]+(s.+)?$`); +// 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; +}; + +// 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(); + 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 +316,12 @@ 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 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}`); }