Skip to content
45 changes: 43 additions & 2 deletions src/schematics/deploy/actions.jasmine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, { assertSafeDependencyName, assertSupportedPackageManager, deployToFunction, findPackageVersion, processHost } from './actions.js'
import { BuildTarget, DeployBuilderSchema, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
import deploy, { assertSafeDependencyName, assertSupportedPackageManager, buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs, deployToFunction, findPackageVersion, processHost } from './actions.js'
import 'jasmine';

let context: BuilderContext;
Expand Down Expand Up @@ -301,6 +301,47 @@ describe('universal deployment', () => {
});*/
});

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');
});
});

describe('deploy input validation (command-injection hardening)', () => {
describe('assertSupportedPackageManager', () => {
['npm', 'yarn', 'pnpm', 'cnpm', 'bun'].forEach((pm) => {
Expand Down
56 changes: 41 additions & 15 deletions src/schematics/deploy/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,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) => {
Expand All @@ -53,7 +53,7 @@ const spawnAsync = async (
reject(error);
});
spawnProcess.on('close', (code) => {
if (code === 1) {
if (code !== 0) {
reject(Buffer.concat(errorChunks).toString());
return;
}
Expand Down Expand Up @@ -356,6 +356,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,
Expand Down Expand Up @@ -430,25 +458,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;
Expand Down Expand Up @@ -482,7 +508,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]);
console.log(`Using Google Application Credentials.`);
}

Expand Down
6 changes: 4 additions & 2 deletions src/schematics/deploy/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@
},
"functionName": {
"type": "string",
"description": "The name of the Cloud Function or Cloud Run serviceId to deploy SSR to"
"pattern": "^[A-Za-z][A-Za-z0-9_-]{0,62}$",
"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."
},
"functionsNodeVersion": {
"oneOf": [{ "type": "number" }, { "type": "string" }],
Expand All @@ -63,7 +64,8 @@
},
"region": {
"type": "string",
"description": "The region to deploy Cloud Functions or Cloud Run to"
"pattern": "^[a-z]+-[a-z]+\\d+$",
"description": "The region to deploy Cloud Functions or Cloud Run to, e.g. us-central1"
},
"outputPath": {
"type": "string",
Expand Down
Loading