fix(deploy): pass gcloud arguments as an array instead of a joined string

src/schematics/deploy/actions.jasmine.ts

@@ -2,8 +2,8 @@
 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 { BuildTarget, DeployBuilderSchema, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
+import deploy, { buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs, deployToFunction } from './actions.js'
 import 'jasmine';
 
 let context: BuilderContext;
@@ -300,3 +300,44 @@ describe('universal deployment', () => {
     expect(spy).not.toHaveBeenCalled();
   });*/
 });
+
+describe('Cloud Run gcloud argv construction', () => {
+  // Regression coverage for the argv-injection fix: these options used to be interpolated
+  // into a single command string and split on whitespace, so a value containing a space
+  // would land as extra, unintended argv entries. They're now passed straight through as
+  // individual array elements.
+  const INJECTED_REGION = 'us-central1 --set-env-vars=INJECTED=owned';
+  const INJECTED_PROJECT = `${FIREBASE_PROJECT} --format=json`;
+
+  it('keeps a region value containing a space as a single --region argument', () => {
+    const options: DeployBuilderSchema = { firebaseProject: FIREBASE_PROJECT, region: INJECTED_REGION };
+    const args = buildCloudRunDeployArgs('my-service', options, []);
+
+    expect(args[args.indexOf('--region') + 1]).toBe(INJECTED_REGION);
+    expect(args).not.toContain('--set-env-vars=INJECTED=owned');
+  });
+
+  it('keeps a firebaseProject value containing a space as a single --project argument (deploy)', () => {
+    const options: DeployBuilderSchema = { firebaseProject: INJECTED_PROJECT, region: 'us-central1' };
+    const args = buildCloudRunDeployArgs('my-service', options, []);
+
+    expect(args[args.indexOf('--project') + 1]).toBe(INJECTED_PROJECT);
+    expect(args).not.toContain('--format=json');
+  });
+
+  it('keeps a firebaseProject value containing a space as a single --project argument (builds submit)', () => {
+    const options: DeployBuilderSchema = { firebaseProject: INJECTED_PROJECT };
+    const args = buildCloudRunBuildsSubmitArgs('cloudRunOut', 'my-service', options);
+
+    expect(args[args.indexOf('--project') + 1]).toBe(INJECTED_PROJECT);
+    expect(args).not.toContain('--format=json');
+  });
+
+  it('passes cloudRunOptions through as their own argv entries', () => {
+    const options: DeployBuilderSchema = { firebaseProject: FIREBASE_PROJECT, region: 'us-central1' };
+    const args = buildCloudRunDeployArgs('my-service', options, ['--vpc-connector', 'my-connector --unset-env-vars=OWNED']);
+
+    expect(args[args.indexOf('--vpc-connector') + 1]).toBe('my-connector --unset-env-vars=OWNED');
+    expect(args).not.toContain('--unset-env-vars=OWNED');
+  });
+});

src/schematics/deploy/actions.ts

@@ -32,11 +32,11 @@ const DEFAULT_CLOUD_RUN_OPTIONS: Partial<CloudRunOptions> = {
 
 const spawnAsync = async (
   command: string,
+  args: string[],
   options?: SpawnOptionsWithoutStdio
 ) =>
   new Promise<Buffer>((resolve, reject) => {
-    const [spawnCommand, ...args] = command.split(/\s+/);
-    const spawnProcess = spawn(spawnCommand, args, options);
+    const spawnProcess = spawn(command, args, options);
     const chunks: Buffer[] = [];
     const errorChunks: Buffer[] = [];
     spawnProcess.stdout.on('data', (data) => {
@@ -51,7 +51,7 @@ const spawnAsync = async (
       reject(error);
     });
     spawnProcess.on('close', (code) => {
-      if (code === 1) {
+      if (code !== 0) {
         reject(Buffer.concat(errorChunks).toString());
         return;
       }
@@ -279,6 +279,34 @@ export const deployToFunction = async (
 };
 
 
+// Exported (rather than kept private) so the argv shape can be asserted directly in tests,
+// without having to mock child_process.spawn.
+export const buildCloudRunBuildsSubmitArgs = (
+  cloudRunOut: string,
+  serviceId: string,
+  options: DeployBuilderOptions
+): string[] => [
+  'builds', 'submit', cloudRunOut,
+  '--tag', `gcr.io/${options.firebaseProject}/${serviceId}`,
+  '--project', options.firebaseProject,
+  '--quiet',
+];
+
+export const buildCloudRunDeployArgs = (
+  serviceId: string,
+  options: DeployBuilderOptions,
+  deployArguments: string[]
+): string[] => [
+  'run', 'deploy', serviceId,
+  '--image', `gcr.io/${options.firebaseProject}/${serviceId}`,
+  '--project', options.firebaseProject,
+  ...deployArguments,
+  '--platform', 'managed',
+  '--allow-unauthenticated',
+  '--region', options.region,
+  '--quiet',
+];
+
 export const deployToCloudRun = async (
   firebaseTools: FirebaseTools,
   context: BuilderContext,
@@ -353,25 +381,23 @@ export const deployToCloudRun = async (
     throw new SchematicsException('Cloud Run preview not supported.');
   }
 
-  const deployArguments: any[] = [];
+  const deployArguments: string[] = [];
   const cloudRunOptions = options.cloudRunOptions || {};
   Object.entries(DEFAULT_CLOUD_RUN_OPTIONS).forEach(([k, v]) => {
     cloudRunOptions[k] ||= v;
   });
   // lean on the schema for validation (rather than sanitize)
-  if (cloudRunOptions.cpus) { deployArguments.push('--cpu', cloudRunOptions.cpus); }
-  if (cloudRunOptions.maxConcurrency) { deployArguments.push('--concurrency', cloudRunOptions.maxConcurrency); }
-  if (cloudRunOptions.maxInstances) { deployArguments.push('--max-instances', cloudRunOptions.maxInstances); }
-  if (cloudRunOptions.memory) { deployArguments.push('--memory', cloudRunOptions.memory); }
-  if (cloudRunOptions.minInstances) { deployArguments.push('--min-instances', cloudRunOptions.minInstances); }
-  if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout); }
+  if (cloudRunOptions.cpus) { deployArguments.push('--cpu', cloudRunOptions.cpus.toString()); }
+  if (cloudRunOptions.maxConcurrency) { deployArguments.push('--concurrency', cloudRunOptions.maxConcurrency.toString()); }
+  if (cloudRunOptions.maxInstances) { deployArguments.push('--max-instances', cloudRunOptions.maxInstances.toString()); }
+  if (cloudRunOptions.memory) { deployArguments.push('--memory', cloudRunOptions.memory.toString()); }
+  if (cloudRunOptions.minInstances) { deployArguments.push('--min-instances', cloudRunOptions.minInstances.toString()); }
+  if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout.toString()); }
   if (cloudRunOptions.vpcConnector) { deployArguments.push('--vpc-connector', cloudRunOptions.vpcConnector); }
 
-  // TODO validate serviceId, firebaseProject, and vpcConnector both to limit errors and opp for injection
-
   context.logger.info(`📦 Deploying to Cloud Run`);
-  await spawnAsync(`gcloud builds submit ${cloudRunOut} --tag gcr.io/${options.firebaseProject}/${serviceId} --project ${options.firebaseProject} --quiet`);
-  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`);
+  await spawnAsync('gcloud', buildCloudRunBuildsSubmitArgs(cloudRunOut, serviceId, options));
+  await spawnAsync('gcloud', buildCloudRunDeployArgs(serviceId, options, deployArguments));
 
   // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
   const siteTarget = options.target ?? context.target!.project;
@@ -405,7 +431,7 @@ export default async function deploy(
   }
 
   if (!firebaseToken && process.env.GOOGLE_APPLICATION_CREDENTIALS) {
-    await spawnAsync(`gcloud auth activate-service-account --key-file ${process.env.GOOGLE_APPLICATION_CREDENTIALS}`);
+    await spawnAsync('gcloud', ['auth', 'activate-service-account', '--key-file', process.env.GOOGLE_APPLICATION_CREDENTIALS as string]);
     console.log(`Using Google Application Credentials.`);
   }