Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
90 changes: 89 additions & 1 deletion src/schematics/deploy/actions.jasmine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
});
});
});
84 changes: 80 additions & 4 deletions src/schematics/deploy/actions.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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.<project>.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;
};

Expand Down Expand Up @@ -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}`);
}
Expand Down