Skip to content

Commit 2ce37b4

Browse files
committed
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 `<pm> 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.
1 parent ed6b8b4 commit 2ce37b4

5 files changed

Lines changed: 111 additions & 14 deletions

File tree

‎package-lock.json‎

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
"@angular/platform-browser-dynamic": "^21.0.0",
6161
"@angular/router": "^21.0.0",
6262
"@schematics/angular": "^21.0.0",
63+
"cross-spawn": "^7.0.3",
6364
"esbuild": "^0.24.0",
6465
"firebase": "^12.4.0",
6566
"firebase-functions": "^6.1.0",
@@ -86,6 +87,7 @@
8687
"@angular/cli": "^21.0.0",
8788
"@angular/compiler-cli": "^21.0.0",
8889
"@angular/platform-server": "^21.0.0",
90+
"@types/cross-spawn": "^6.0.6",
8991
"@types/fs-extra": "^7.0.0",
9092
"@types/gzip-size": "^5.1.1",
9193
"@types/inquirer": "^0.0.44",
@@ -99,7 +101,6 @@
99101
"@typescript-eslint/eslint-plugin": "^8.33.0",
100102
"@typescript-eslint/parser": "^8.33.0",
101103
"conventional-changelog-cli": "^1.2.0",
102-
"cross-spawn": "^7.0.3",
103104
"eslint": "^9.27.0",
104105
"eslint-plugin-import": "^2.31.0",
105106
"globals": "^13.21.0",

‎src/schematics/deploy/actions.jasmine.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { join } from 'path';
33
import { BuilderContext, BuilderRun, ScheduleOptions, Target } from '@angular-devkit/architect';
44
import { JsonObject, logging } from '@angular-devkit/core';
55
import { BuildTarget, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
6-
import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction } from './actions.js'
6+
import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction, findPackageVersion, processHost } from './actions.js'
77
import 'jasmine';
88

99
let context: BuilderContext;
@@ -332,4 +332,59 @@ describe('deploy input validation (command-injection hardening)', () => {
332332
});
333333
});
334334
});
335+
336+
// These guard the fix at its call sites: the validators above are only useful
337+
// if the deploy code keeps routing every command through the shell-free runner.
338+
// A regression to execSync/`shell: true`, or a dropped validator call, fails here.
339+
describe('call sites route through the shell-free runner', () => {
340+
beforeEach(() => initMocks());
341+
342+
it('installs functions dependencies via the runner with an argv array and no shell', async () => {
343+
// The install branch only runs when the generated package.json exists.
344+
spyOn(fsHost, 'existsSync').and.returnValue(true);
345+
const runSpy = spyOn(processHost, 'runPackageBin').and.returnValue(Buffer.from(''));
346+
347+
await deployToFunction(
348+
firebaseMock,
349+
context,
350+
workspaceRoot,
351+
STATIC_BUILD_TARGET,
352+
SERVER_BUILD_TARGET,
353+
{ preview: false },
354+
undefined,
355+
fsHost
356+
);
357+
358+
expect(runSpy).toHaveBeenCalledTimes(1);
359+
const [command, args, options] = runSpy.calls.mostRecent().args;
360+
expect(command).toBe('npm');
361+
expect(args).toEqual(['--prefix', join(workspaceRoot, 'dist'), 'install']);
362+
// No shell: a `shell` option would reopen the injection this PR closes.
363+
expect((options as any)?.shell).toBeFalsy();
364+
});
365+
366+
it('runs the package manager through the runner with a validated argv array', () => {
367+
const runSpy = spyOn(processHost, 'runPackageBin').and.returnValue(Buffer.from(''));
368+
369+
findPackageVersion('npm', 'rxjs');
370+
371+
expect(runSpy).toHaveBeenCalledWith('npm', ['list', 'rxjs']);
372+
});
373+
374+
it('rejects an unsupported package manager before spawning anything', () => {
375+
const runSpy = spyOn(processHost, 'runPackageBin');
376+
377+
expect(() => findPackageVersion('npm; touch /tmp/pwned #', 'rxjs'))
378+
.toThrowError(/Unsupported package manager/);
379+
expect(runSpy).not.toHaveBeenCalled();
380+
});
381+
382+
it('rejects an unsafe dependency name before spawning anything', () => {
383+
const runSpy = spyOn(processHost, 'runPackageBin');
384+
385+
expect(() => findPackageVersion('npm', 'evil; touch /tmp/pwned #'))
386+
.toThrowError(/Invalid dependency name/);
387+
expect(runSpy).not.toHaveBeenCalled();
388+
});
389+
});
335390
});

‎src/schematics/deploy/actions.ts‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
import { SpawnOptionsWithoutStdio, execFileSync, spawn } from 'child_process';
1+
import { SpawnOptionsWithoutStdio, SpawnSyncOptions, spawn } from 'child_process';
22
import { existsSync, readFileSync, renameSync, writeFileSync } from 'fs';
33
import { dirname, join } from 'path';
44
import { fileURLToPath } from 'url';
55
import { BuilderContext, targetFromTargetString } from '@angular-devkit/architect';
66
import { SchematicsException } from '@angular-devkit/schematics';
7+
import crossSpawn from 'cross-spawn';
78
import fsExtra from 'fs-extra';
89
import * as inquirer from 'inquirer';
910
import open from 'open';
@@ -156,11 +157,39 @@ export const assertSafeDependencyName = (name: string): string => {
156157
return name;
157158
};
158159

159-
const findPackageVersion = (packageManager: string, name: string) => {
160-
// Run the package manager without a shell (execFileSync + argument array) so a
161-
// dependency name or package-manager value taken from angular.json cannot be
162-
// interpreted as a shell command.
163-
const output = execFileSync(assertSupportedPackageManager(packageManager), [
160+
// All shelling out from the deploy builder funnels through this single runner.
161+
// cross-spawn (v7) resolves the platform-appropriate executable and escapes each
162+
// argument, so a value taken from angular.json is passed as an argv entry and can
163+
// never be parsed as shell syntax. Unlike child_process.execFile it can launch a
164+
// Windows `.cmd`/`.bat` shim (npm, yarn, pnpm and cnpm all ship as `.cmd` shims
165+
// there), so `ng deploy` keeps working cross-platform. We deliberately avoid
166+
// `shell: true`, whose args-array form Node runtime-deprecates (DEP0190) because
167+
// it concatenates the arguments without escaping, reopening the injection.
168+
// It is exported as an object so tests can assert the deploy code shells out only
169+
// through here and never regresses to execSync or `shell: true`.
170+
export const processHost = {
171+
runPackageBin(
172+
command: string,
173+
args: string[],
174+
options: SpawnSyncOptions = {},
175+
): Buffer {
176+
const result = crossSpawn.sync(command, args, options);
177+
if (result.error) { throw result.error; }
178+
if (result.status !== 0) {
179+
throw new SchematicsException(
180+
`Command "${command}" exited with ${result.signal ? `signal ${result.signal}` : `code ${result.status}`}.`
181+
);
182+
}
183+
return result.stdout;
184+
},
185+
};
186+
187+
export const findPackageVersion = (packageManager: string, name: string) => {
188+
// Run the package manager without a shell (argument array, no `shell` option)
189+
// so a dependency name or package-manager value taken from angular.json cannot
190+
// be interpreted as a shell command. Both values are validated first, so an
191+
// unsupported manager or unsafe name throws before anything is ever spawned.
192+
const output = processHost.runPackageBin(assertSupportedPackageManager(packageManager), [
164193
'list',
165194
assertSafeDependencyName(name),
166195
]).toString();
@@ -287,10 +316,12 @@ export const deployToFunction = async (
287316
const siteTarget = options.target ?? context.target!.project;
288317

289318
if (fsHost.existsSync(functionsPackageJsonPath)) {
290-
// Pass the output directory as an argument rather than interpolating it into
291-
// a shell string; `functionsOut` derives from the `outputPath` deploy option
292-
// (angular.json) and must not be able to inject shell commands.
293-
execFileSync('npm', ['--prefix', functionsOut, 'install']);
319+
// Pass the output directory as an argv entry rather than interpolating it
320+
// into a shell string; `functionsOut` derives from the `outputPath` deploy
321+
// option (angular.json) and must not be able to inject shell commands. npm is
322+
// a `.cmd` shim on Windows, which child_process.execFile cannot launch, so
323+
// this goes through the shell-free cross-spawn runner instead.
324+
processHost.runPackageBin('npm', ['--prefix', functionsOut, 'install'], { stdio: 'inherit' });
294325
} else {
295326
console.error(`No package.json exists at ${functionsOut}`);
296327
}

‎src/schematics/deploy/schema.json‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,7 @@
6767
},
6868
"outputPath": {
6969
"type": "string",
70-
"description": "Where to output the deploy artifacts",
71-
"pattern": "^[^;&|$`<>\\n\\r()]*$"
70+
"description": "Where to output the deploy artifacts"
7271
},
7372
"functionsRuntimeOptions": {
7473
"type": "object",

0 commit comments

Comments
 (0)