From 8859c46a85aec587f494ccc81683989f998f480b Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Sat, 1 Aug 2026 01:51:04 +0700 Subject: [PATCH 1/2] fix(deploy): pass gcloud arguments as an array instead of a joined string spawnAsync built the gcloud command as a single template-literal string and split it on whitespace before handing it to spawn(). Any deploy option containing a space (region, firebaseProject, functionName, cloudRunOptions.vpcConnector, none of which have a schema pattern) would be split into extra argv entries, letting a value from angular.json add unintended flags to the gcloud builds submit / run deploy / auth activate-service-account invocations. spawnAsync now takes command and args separately, matching child_process spawn's own signature, and the three call sites build their argument lists as arrays instead of interpolating into one string. This removes the join/split round-trip entirely rather than trying to validate each field. --- src/schematics/deploy/actions.ts | 40 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index 9a2fb7cde..b13492b2b 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -32,11 +32,11 @@ const DEFAULT_CLOUD_RUN_OPTIONS: Partial = { const spawnAsync = async ( command: string, + args: string[], options?: SpawnOptionsWithoutStdio ) => new Promise((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) => { @@ -353,25 +353,37 @@ 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', [ + '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, + '--platform', 'managed', + '--allow-unauthenticated', + '--region', options.region, + '--quiet', + ]); // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const siteTarget = options.target ?? context.target!.project; @@ -405,7 +417,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.`); } From 7c2668eef5a7f5245e5953002c5511ae468d71bb Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Tue, 4 Aug 2026 00:11:44 +0700 Subject: [PATCH 2/2] fix(deploy): treat any non-zero gcloud exit code as failure, add argv construction tests spawnAsync's close handler only rejected on code === 1. gcloud's own docs only promise a non-zero exit on failure, and a killed process (e.g. an out-of-memory gcloud builds submit) reports code === null, both of which previously resolved as success, so a failed deploy could be reported as successful. Now rejects on any code !== 0. Also extracts the gcloud args construction for both cloud run calls (buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs) into pure, exported functions, and adds tests asserting a value containing a space (region, firebaseProject, a cloudRunOptions value) stays a single argv entry rather than being split into extra flags, locking in the fix from the previous commit without needing to mock child_process.spawn. --- src/schematics/deploy/actions.jasmine.ts | 45 +++++++++++++++++++++- src/schematics/deploy/actions.ts | 48 +++++++++++++++--------- 2 files changed, 74 insertions(+), 19 deletions(-) diff --git a/src/schematics/deploy/actions.jasmine.ts b/src/schematics/deploy/actions.jasmine.ts index 1795ea10d..c692001fc 100644 --- a/src/schematics/deploy/actions.jasmine.ts +++ b/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'); + }); +}); diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index b13492b2b..c741ab0f4 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -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, @@ -368,22 +396,8 @@ export const deployToCloudRun = async ( if (cloudRunOptions.vpcConnector) { deployArguments.push('--vpc-connector', cloudRunOptions.vpcConnector); } 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, - '--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;