Merge branch 'main' into mgoho/chore/update-angular

Author: armando-navarro

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, { 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;
@@ -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) => {

src/schematics/deploy/actions.ts

@@ -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) => {
@@ -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;
       }
@@ -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,
@@ -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;
@@ -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.`);
   }
 

src/schematics/deploy/schema.json

@@ -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" }],
@@ -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",

src/schematics/utils.jasmine.ts

@@ -0,0 +1,123 @@
+import { ConnectorConfig } from './interfaces.js';
+import {
+  connectorConfigObjectLiteral,
+  isValidPackageSpecifier,
+  resolveDataConnectProviderConfig,
+} from './utils.js';
+import 'jasmine';
+
+// eslint-disable-next-line @typescript-eslint/no-implied-eval
+const evalObjectLiteral = (literal: string) => new Function(`return (${literal});`)();
+
+describe('connectorConfigObjectLiteral', () => {
+
+  it('escapes a quote-bearing value so it round-trips as inert string data', () => {
+    const injectionAttempt = 'us-central1"; console.log("INJECTED"); const _z="x';
+    const literal = connectorConfigObjectLiteral({
+      location: injectionAttempt,
+      connector: 'my-connector',
+      service: 'my-service',
+    } as ConnectorConfig);
+    const evaluated = evalObjectLiteral(literal);
+    expect(evaluated.location).toBe(injectionAttempt);
+  });
+
+  it('coerces a non-string value from an unquoted yaml scalar to a string', () => {
+    const literal = connectorConfigObjectLiteral({
+      location: 'us-central1',
+      connector: 'my-connector',
+      service: 123,
+    } as unknown as ConnectorConfig);
+    const evaluated = evalObjectLiteral(literal);
+    expect(evaluated.service).toBe('123');
+    expect(typeof evaluated.service).toBe('string');
+  });
+
+  it('coerces a boolean-like yaml scalar to a string', () => {
+    const literal = connectorConfigObjectLiteral({
+      location: 'us-central1',
+      connector: true,
+      service: 'my-service',
+    } as unknown as ConnectorConfig);
+    const evaluated = evalObjectLiteral(literal);
+    expect(evaluated.connector).toBe('true');
+    expect(typeof evaluated.connector).toBe('string');
+  });
+
+});
+
+describe('isValidPackageSpecifier', () => {
+
+  it('accepts a normal scoped package specifier', () => {
+    expect(isValidPackageSpecifier('@my-org/my-connector')).toBeTrue();
+  });
+
+  it('rejects a value containing a double quote', () => {
+    expect(isValidPackageSpecifier('foo"; console.log("INJECTED"); const _z="x')).toBeFalse();
+  });
+
+  it('rejects a value containing a backslash', () => {
+    expect(isValidPackageSpecifier('foo\\bar')).toBeFalse();
+  });
+
+  it('rejects a value containing a newline', () => {
+    expect(isValidPackageSpecifier('foo\nbar')).toBeFalse();
+  });
+
+});
+
+describe('resolveDataConnectProviderConfig', () => {
+
+  it('resolves to the connectorConfig object literal when there is no package', () => {
+    const resolution = resolveDataConnectProviderConfig({
+      connectorYaml: { connectorId: 'my-connector' },
+      connectorConfig: {
+        location: 'us-central1',
+        connector: 'my-connector',
+        service: 'my-service',
+      },
+    });
+    expect(resolution.kind).toBe('literal');
+  });
+
+  it('resolves to an external import when package is a valid specifier', () => {
+    const resolution = resolveDataConnectProviderConfig({
+      connectorYaml: { connectorId: 'my-connector' },
+      connectorConfig: {
+        location: 'us-central1',
+        connector: 'my-connector',
+        service: 'my-service',
+      },
+      package: '@my-org/my-connector',
+    });
+    expect(resolution).toEqual({ kind: 'external', package: '@my-org/my-connector' });
+  });
+
+  it('falls back to the connectorConfig literal when package is not a valid specifier', () => {
+    const resolution = resolveDataConnectProviderConfig({
+      connectorYaml: { connectorId: 'my-connector' },
+      connectorConfig: {
+        location: 'us-central1',
+        connector: 'my-connector',
+        service: 'my-service',
+      },
+      package: 'foo"; console.log("INJECTED")',
+    });
+    expect(resolution.kind).toBe('literal');
+  });
+
+  it('does not throw and falls back to an empty literal when there is no javascriptSdk config', () => {
+    expect(() => resolveDataConnectProviderConfig({
+      connectorYaml: { connectorId: 'my-connector' },
+    })).not.toThrow();
+    expect(resolveDataConnectProviderConfig({
+      connectorYaml: { connectorId: 'my-connector' },
+    })).toEqual({ kind: 'literal', literal: '{}' });
+  });
+
+  it('does not throw and falls back to an empty literal when config is null', () => {
+    expect(() => resolveDataConnectProviderConfig(null)).not.toThrow();
+    expect(resolveDataConnectProviderConfig(null)).toEqual({ kind: 'literal', literal: '{}' });
+  });
+
+});

src/schematics/utils.ts

@@ -7,10 +7,10 @@ import {
   Tree,
   chain,
 } from "@angular-devkit/schematics";
-import { NodePackageInstallTask } from "@angular-devkit/schematics/tasks";
+import { NodePackageInstallTask } from "@angular-devkit/schematics/tasks/index.js";
 import { addRootProvider } from "@schematics/angular/utility";
 import { parse } from "yaml";
-import { overwriteIfExists, safeReadJSON, stringifyFormatted } from "./common";
+import { overwriteIfExists, safeReadJSON, stringifyFormatted } from "./common.js";
 import {
   ConnectorConfig,
   ConnectorYaml,
@@ -148,6 +148,38 @@ ${addZonePatch ? "import 'zone.js/dist/zone-patch-rxjs';" : ""}`
   return host;
 }
 
+// config.package and the connectorConfig values below come from the project's own
+// dataconnect.yaml/connector.yaml, not from a trusted schema. isValidPackageSpecifier
+// rejects the characters that would let a value break out of the double-quoted string
+// literal it's interpolated into; it does not otherwise validate that the value is a
+// well-formed module specifier.
+const PACKAGE_SPECIFIER_PATTERN = /^[^'"\\\n\r]+$/;
+export function isValidPackageSpecifier(pkg: string): boolean {
+  return PACKAGE_SPECIFIER_PATTERN.test(pkg);
+}
+
+export function connectorConfigObjectLiteral(connectorConfig: ConnectorConfig): string {
+  return `{${(Object.keys(connectorConfig) as (keyof ConnectorConfig)[]).map(
+    (key) => `${key}: ${JSON.stringify(String(connectorConfig[key]))}`
+  ).join(',')}}`;
+}
+
+export type DataConnectProviderConfigResolution =
+  | { kind: "external"; package: string }
+  | { kind: "literal"; literal: string };
+
+export function resolveDataConnectProviderConfig(
+  config: DataConnectConnectorConfig | null | undefined
+): DataConnectProviderConfigResolution {
+  if (config?.package && isValidPackageSpecifier(config.package)) {
+    return { kind: "external", package: config.package };
+  }
+  if (config?.connectorConfig) {
+    return { kind: "literal", literal: connectorConfigObjectLiteral(config.connectorConfig) };
+  }
+  return { kind: "literal", literal: "{}" };
+}
+
 export function featureToRules(
   features: FEATURES[],
   projectName: string,
@@ -216,27 +248,21 @@ export function featureToRules(
         case FEATURES.DataConnect:
           return addRootProvider(projectName, ({ code, external }) => {
             external("getDataConnect", "@angular/fire/data-connect");
-            let configAsStr = "{}";
 
-            const config = dataConnectConfig;
+            const resolution = resolveDataConnectProviderConfig(dataConnectConfig);
+            const configAsStr = resolution.kind === "external"
+              ? external("connectorConfig", resolution.package)
+              : resolution.literal;
+
             let angularConfig: undefined | string;
-            if (config) {
-              if (config.package) {
-                configAsStr = external("connectorConfig", config.package);
-              } else {
-                configAsStr = `{${Object.keys(config.connectorConfig as ConnectorConfig).map(
-                  (key) => `${key}: "${(config.connectorConfig as ConnectorConfig)[key]}"`
-                ).join(',')}}`;
-              }
-              if (config.angular) {
-                angularConfig = `, ${external(
-                  "provideTanStackQuery",
-                  "@tanstack/angular-query-experimental"
-                )}(new ${external(
-                  "QueryClient",
-                  "@tanstack/angular-query-experimental"
-                )}())`;
-              }
+            if (dataConnectConfig?.angular) {
+              angularConfig = `, ${external(
+                "provideTanStackQuery",
+                "@tanstack/angular-query-experimental"
+              )}(new ${external(
+                "QueryClient",
+                "@tanstack/angular-query-experimental"
+              )}())`;
             }
             return code`${external(
               "provideDataConnect",