Skip to content

Commit 7b3617e

Browse files
herdiyana256armando-navarro
herdiyana256
authored and
armando-navarro
committed
fix(deploy): pass gcloud arguments as an array instead of a joined string (#3726)
spawnAsync built each gcloud command as one string and split it on whitespace before calling spawn(), so any deploy option containing a space was chopped into extra argv entries and a value from angular.json could add flags of its own. It now takes command and args separately and the three call sites pass arrays, removing the join and split entirely. The close handler also rejected only on exit code 1, so a gcloud failure with any other code, or a process killed by a signal, resolved as success and a failed deploy reported as done. It now rejects on any non-zero or null code. The Cloud Run argument construction moved into two exported functions so tests can assert the argv shape without mocking spawn, and functionName and region gained schema patterns, since both also reach the generated Cloud Functions source. (cherry picked from commit 778625a)
1 parent 5ddc4bf commit 7b3617e

3 files changed

Lines changed: 88 additions & 19 deletions

File tree

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

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@
22
import { join } from 'path';
33
import { BuilderContext, BuilderRun, ScheduleOptions, Target } from '@angular-devkit/architect';
44
import { JsonObject, logging } from '@angular-devkit/core';
5-
import { BuildTarget, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
6-
import deploy, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction, findPackageVersion, processHost } from './actions'
5+
import { BuildTarget, DeployBuilderSchema, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
6+
import deploy, { assertSafeDependencyName, assertSupportedPackageManager, buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs, deployToFunction, findPackageVersion, processHost } from './actions'
77
import 'jasmine';
88

99
let context: BuilderContext;
@@ -301,6 +301,47 @@ describe('universal deployment', () => {
301301
});*/
302302
});
303303

304+
describe('Cloud Run gcloud argv construction', () => {
305+
// Regression coverage for the argv-injection fix: these options used to be interpolated
306+
// into a single command string and split on whitespace, so a value containing a space
307+
// would land as extra, unintended argv entries. They're now passed straight through as
308+
// individual array elements.
309+
const INJECTED_REGION = 'us-central1 --set-env-vars=INJECTED=owned';
310+
const INJECTED_PROJECT = `${FIREBASE_PROJECT} --format=json`;
311+
312+
it('keeps a region value containing a space as a single --region argument', () => {
313+
const options: DeployBuilderSchema = { firebaseProject: FIREBASE_PROJECT, region: INJECTED_REGION };
314+
const args = buildCloudRunDeployArgs('my-service', options, []);
315+
316+
expect(args[args.indexOf('--region') + 1]).toBe(INJECTED_REGION);
317+
expect(args).not.toContain('--set-env-vars=INJECTED=owned');
318+
});
319+
320+
it('keeps a firebaseProject value containing a space as a single --project argument (deploy)', () => {
321+
const options: DeployBuilderSchema = { firebaseProject: INJECTED_PROJECT, region: 'us-central1' };
322+
const args = buildCloudRunDeployArgs('my-service', options, []);
323+
324+
expect(args[args.indexOf('--project') + 1]).toBe(INJECTED_PROJECT);
325+
expect(args).not.toContain('--format=json');
326+
});
327+
328+
it('keeps a firebaseProject value containing a space as a single --project argument (builds submit)', () => {
329+
const options: DeployBuilderSchema = { firebaseProject: INJECTED_PROJECT };
330+
const args = buildCloudRunBuildsSubmitArgs('cloudRunOut', 'my-service', options);
331+
332+
expect(args[args.indexOf('--project') + 1]).toBe(INJECTED_PROJECT);
333+
expect(args).not.toContain('--format=json');
334+
});
335+
336+
it('passes cloudRunOptions through as their own argv entries', () => {
337+
const options: DeployBuilderSchema = { firebaseProject: FIREBASE_PROJECT, region: 'us-central1' };
338+
const args = buildCloudRunDeployArgs('my-service', options, ['--vpc-connector', 'my-connector --unset-env-vars=OWNED']);
339+
340+
expect(args[args.indexOf('--vpc-connector') + 1]).toBe('my-connector --unset-env-vars=OWNED');
341+
expect(args).not.toContain('--unset-env-vars=OWNED');
342+
});
343+
});
344+
304345
describe('deploy input validation (command-injection hardening)', () => {
305346
describe('assertSupportedPackageManager', () => {
306347
['npm', 'yarn', 'pnpm', 'cnpm', 'bun'].forEach((pm) => {

‎src/schematics/deploy/actions.ts‎

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ const DEFAULT_CLOUD_RUN_OPTIONS: Partial<CloudRunOptions> = {
2828

2929
const spawnAsync = async (
3030
command: string,
31+
args: string[],
3132
options?: SpawnOptionsWithoutStdio
3233
) =>
3334
new Promise<Buffer>((resolve, reject) => {
34-
const [spawnCommand, ...args] = command.split(/\s+/);
35-
const spawnProcess = spawn(spawnCommand, args, options);
35+
const spawnProcess = spawn(command, args, options);
3636
const chunks: Buffer[] = [];
3737
const errorChunks: Buffer[] = [];
3838
spawnProcess.stdout.on('data', (data) => {
@@ -47,7 +47,7 @@ const spawnAsync = async (
4747
reject(error);
4848
});
4949
spawnProcess.on('close', (code) => {
50-
if (code === 1) {
50+
if (code !== 0) {
5151
reject(Buffer.concat(errorChunks).toString());
5252
return;
5353
}
@@ -349,6 +349,34 @@ export const deployToFunction = async (
349349
};
350350

351351

352+
// Exported (rather than kept private) so the argv shape can be asserted directly in tests,
353+
// without having to mock child_process.spawn.
354+
export const buildCloudRunBuildsSubmitArgs = (
355+
cloudRunOut: string,
356+
serviceId: string,
357+
options: DeployBuilderOptions
358+
): string[] => [
359+
'builds', 'submit', cloudRunOut,
360+
'--tag', `gcr.io/${options.firebaseProject}/${serviceId}`,
361+
'--project', options.firebaseProject,
362+
'--quiet',
363+
];
364+
365+
export const buildCloudRunDeployArgs = (
366+
serviceId: string,
367+
options: DeployBuilderOptions,
368+
deployArguments: string[]
369+
): string[] => [
370+
'run', 'deploy', serviceId,
371+
'--image', `gcr.io/${options.firebaseProject}/${serviceId}`,
372+
'--project', options.firebaseProject,
373+
...deployArguments,
374+
'--platform', 'managed',
375+
'--allow-unauthenticated',
376+
'--region', options.region,
377+
'--quiet',
378+
];
379+
352380
export const deployToCloudRun = async (
353381
firebaseTools: FirebaseTools,
354382
context: BuilderContext,
@@ -423,25 +451,23 @@ export const deployToCloudRun = async (
423451
throw new SchematicsException('Cloud Run preview not supported.');
424452
}
425453

426-
const deployArguments: any[] = [];
454+
const deployArguments: string[] = [];
427455
const cloudRunOptions = options.cloudRunOptions || {};
428456
Object.entries(DEFAULT_CLOUD_RUN_OPTIONS).forEach(([k, v]) => {
429457
cloudRunOptions[k] ||= v;
430458
});
431459
// lean on the schema for validation (rather than sanitize)
432-
if (cloudRunOptions.cpus) { deployArguments.push('--cpu', cloudRunOptions.cpus); }
433-
if (cloudRunOptions.maxConcurrency) { deployArguments.push('--concurrency', cloudRunOptions.maxConcurrency); }
434-
if (cloudRunOptions.maxInstances) { deployArguments.push('--max-instances', cloudRunOptions.maxInstances); }
435-
if (cloudRunOptions.memory) { deployArguments.push('--memory', cloudRunOptions.memory); }
436-
if (cloudRunOptions.minInstances) { deployArguments.push('--min-instances', cloudRunOptions.minInstances); }
437-
if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout); }
460+
if (cloudRunOptions.cpus) { deployArguments.push('--cpu', cloudRunOptions.cpus.toString()); }
461+
if (cloudRunOptions.maxConcurrency) { deployArguments.push('--concurrency', cloudRunOptions.maxConcurrency.toString()); }
462+
if (cloudRunOptions.maxInstances) { deployArguments.push('--max-instances', cloudRunOptions.maxInstances.toString()); }
463+
if (cloudRunOptions.memory) { deployArguments.push('--memory', cloudRunOptions.memory.toString()); }
464+
if (cloudRunOptions.minInstances) { deployArguments.push('--min-instances', cloudRunOptions.minInstances.toString()); }
465+
if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout.toString()); }
438466
if (cloudRunOptions.vpcConnector) { deployArguments.push('--vpc-connector', cloudRunOptions.vpcConnector); }
439467

440-
// TODO validate serviceId, firebaseProject, and vpcConnector both to limit errors and opp for injection
441-
442468
context.logger.info(`📦 Deploying to Cloud Run`);
443-
await spawnAsync(`gcloud builds submit ${cloudRunOut} --tag gcr.io/${options.firebaseProject}/${serviceId} --project ${options.firebaseProject} --quiet`);
444-
await spawnAsync(`gcloud run deploy ${serviceId} --image gcr.io/${options.firebaseProject}/${serviceId} --project ${options.firebaseProject} ${deployArguments.join(' ')} --platform managed --allow-unauthenticated --region=${options.region} --quiet`);
469+
await spawnAsync('gcloud', buildCloudRunBuildsSubmitArgs(cloudRunOut, serviceId, options));
470+
await spawnAsync('gcloud', buildCloudRunDeployArgs(serviceId, options, deployArguments));
445471

446472
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
447473
const siteTarget = options.target ?? context.target!.project;
@@ -475,7 +501,7 @@ export default async function deploy(
475501
}
476502

477503
if (!firebaseToken && process.env.GOOGLE_APPLICATION_CREDENTIALS) {
478-
await spawnAsync(`gcloud auth activate-service-account --key-file ${process.env.GOOGLE_APPLICATION_CREDENTIALS}`);
504+
await spawnAsync('gcloud', ['auth', 'activate-service-account', '--key-file', process.env.GOOGLE_APPLICATION_CREDENTIALS]);
479505
console.log(`Using Google Application Credentials.`);
480506
}
481507

‎src/schematics/deploy/schema.json‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@
5151
},
5252
"functionName": {
5353
"type": "string",
54-
"description": "The name of the Cloud Function or Cloud Run serviceId to deploy SSR to"
54+
"pattern": "^[A-Za-z][A-Za-z0-9_-]{0,62}$",
55+
"description": "The name of the Cloud Function or Cloud Run serviceId to deploy SSR to. Must start with a letter and contain only letters, numbers, hyphens and underscores; on the Cloud Functions path this value also becomes a JavaScript identifier, so use only letters and numbers there."
5556
},
5657
"functionsNodeVersion": {
5758
"oneOf": [{ "type": "number" }, { "type": "string" }],
@@ -63,7 +64,8 @@
6364
},
6465
"region": {
6566
"type": "string",
66-
"description": "The region to deploy Cloud Functions or Cloud Run to"
67+
"pattern": "^[a-z]+-[a-z]+\\d+$",
68+
"description": "The region to deploy Cloud Functions or Cloud Run to, e.g. us-central1"
6769
},
6870
"outputPath": {
6971
"type": "string",

0 commit comments

Comments
 (0)