Thanks for this, and for keeping at the ng deploy hardening. I reproduced what you describe: rendering the generated function with a crafted server outputPath gives a standalone require('child_process').execSync(...) statement in index.js, and a crafted functionsNodeVersion adds its own RUN line to the Dockerfile.

There is one gap I think should be filled before this merges, and a few smaller notes that should not hold it up.

Blocking: two more angular.json values still reach the generated function unguarded

functionName and region come from the same deploy options block as functionsNodeVersion, and both are written straight into the same generated index.js with no validation:

• functionName is written as a bare identifier:
  • exports.${functionName || DEFAULT_FUNCTION_NAME} sits in functions-templates.ts at lines 44 and 62, so this applies to both the default and the CF3v2 template.
  • A value of ssr; require('child_process').execSync('...'); var _x renders as exports.ssr; followed by the injected call, with the template's own = assignment becoming that variable's initializer.
  • The file still parses, so the injected call runs when the function loads.
• region is written inside a quoted string:
  • .region('${options.region || DEFAULT_FUNCTION_REGION}') at line 45 puts it in a single-quoted literal in the default template, so a ' breaks out exactly the way outputPath did.
  • The CF3v2 template passes region through JSON.stringify, so that path is already safe.

Neither has a pattern in schema.json (functionName and region are both plain type: string), so nothing upstream constrains them either.

Since the description says this prevents code-generation injection from angular.json values, either of these would unblock it for me:

• Extend the fix with a check on functionName and one on region, called before the template runs, matching the pattern you already established. This is the outcome I would prefer.
• Narrow the title and description to the two values this covers, and open a short follow-up issue for functionName and region, so the change matches its claim and the remaining exposure stays tracked rather than closed over.

Non-blocking notes

• Consider escaping where the value is written rather than only screening it on the way in. Your validators are a blocklist of dangerous characters, which has to stay ahead of every context the value lands in. Two places that could be structural instead:
  • For a string position, emit ${JSON.stringify(value)} and drop the quotes already in the template, since JSON.stringify supplies its own. Written as .region(${JSON.stringify(...)}) it escapes correctly, whereas leaving the existing quotes in place would yield .region('"us-central1"') and change the value.
  • For the exports.<name> position, an allowlist of valid JavaScript identifiers is easier to reason about than a list of rejects. It would also make a currently silent failure loud: a functionName containing a dash already generates a file that does not parse.
• A leading dash still gets through.
  • The character list does not reject -, and on the Cloud Run path the generated package.json sets start: node <serverOutputPath>/main.js.
  • So a server outputPath beginning with - reaches node as a flag rather than a path.
  • The comment above the check says a legitimate output directory never contains these characters, which reads stronger than what the character class enforces.
• Nothing fails if the checks stop being called.
  • The new specs exercise assertSafeOutputPath and assertSafeNodeVersion directly.
  • What I could not find is a test that fails if the builder stops calling them: removing the calls from deployToFunction and deployToCloudRun still passes the whole suite.
  • A test that drives one of those functions with a hostile outputPath and expects a throw would keep the protection from quietly disappearing later.
• Spec count in the description.
  • Locally npm run test:node reports 78 specs on this branch, not the 150 in the body.
  • The branch is based on an older commit, so a rebase on current main would refresh that number.

If I have misread any of this, point me at it and I will take another look.

This closes both gaps, thanks. Two things I think need changing before it merges, one of them introduced by the fix.

The functionName pattern rejects valid Cloud Run service names

functionName does double duty. On the Cloud Functions path it becomes the exports.<name> target in generated JavaScript, where a plain identifier is exactly the right constraint. On the Cloud Run path the same option is the service ID (serviceId = options.functionName, and the option's own description says so), and there the constraint does not fit:

• Google's Cloud Run API reference says a service ID "must begin with letter, and cannot end with hyphen", so hyphens mid-name are allowed. A service named my-ssr-service is legitimate and is rejected by the new ^[A-Za-z_$][A-Za-z0-9_$]*$.

• The pattern permits _ and $, which cannot work in a service name, since it lands in the assigned hostname (https://SERVICE_NAME-PROJECT_NUMBER.REGION.run.app) and the docs describe that as a DNS segment.

• This is enforced rather than advisory. @angular-devkit/architect validates builder options against the builder schema before the builder runs, so a Cloud Run user with a hyphenated service name gets refused where it worked before.

Only the schema needs to change. assertSafeFunctionName can stay exactly as it is, since deployToCloudRun generates no JavaScript from this value, so the identifier requirement only ever needs to apply on the Functions path.

• Widen the functionName pattern rather than removing it. It is doing real work on the Cloud Run path.

• ^[A-Za-z](?:[A-Za-z0-9_$-]*[A-Za-z0-9_$])?$ is one option. It accepts ssr, my-ssr-service and my_fn, rejects whitespace, shell metacharacters and a leading dash, and enforces Cloud Run's "cannot end with hyphen" rule.

Also please narrow the TODO just above those gcloud calls

actions.ts carries // TODO validate serviceId, firebaseProject, and vpcConnector both to limit errors and opp for injection. Once the pattern lands, that comment is misleading in a way worth fixing in the same change:

• The serviceId part is then handled, so the comment overstates what is still missing.
• firebaseProject and vpcConnector genuinely are still unguarded, and both reach the same whitespace-split command, so the comment should not be deleted either.
• Dropping serviceId from the list and leaving the other two keeps it accurate and keeps the remaining work visible.

If you read the Cloud Run side differently, tell me. I could not find a page where Google states the full service-name character set, so I am going off the "cannot end with hyphen" wording and the hostname format.

Thanks, both of the things I asked for are done, and the split is working: schema.json and the template file now merge with main cleanly, which was the whole point.

Other than the merge conflict that needs resolving, nothing here blocks it from my side. See the last paragraph for a point regarding merge conflict resolution.

One small correction, one question.

One sentence in the spec file promises more than it delivers

I rechecked the guards by removing them one at a time rather than all at once:

• Removing either assertSafeOutputPath(staticBuildOptions.outputPath, ...) call, in deployToFunction or in deployToCloudRun, leaves all 158 specs green.
• The other four call sites each turn the suite red.
• The cause looks like withServerOutputPath, which returns a fixed { outputPath: 'dist/browser' } for the build target and only varies the server one, so no spec ever passes a hostile static path.

To be clear, that is not a hole in the protection. The static target's outputPath is only ever used as a filesystem path, so unlike the server one it never reaches generated source or a command, and there is nothing exploitable for a spec to assert. Guarding it anyway is sensible.

The part worth changing is the comment at actions.jasmine.ts:350, which says each spec fails if its assert is removed. I took that at face value and it cost me some time. Narrowing the sentence to the four it covers would do it.

A question about the functionsNodeVersion pattern on the Functions path

The new pattern applies to both deploy paths, but the Dockerfile it protects is only written on the Cloud Run path.

• The value also becomes engines.node in the generated package.json, which the builder passes to satisfies() as a semver range.
• satisfies treats 20.x, >=18 and ^20 as valid ranges, and the pattern rejects all three.
• Your runtime assertSafeNodeVersion is already scoped to deployToCloudRun, which seems right to me.

I could not find a Firebase doc showing a range in engines.node, so this may be entirely theoretical. Do you know whether anyone deploys with one? If not, leaving it as is seems fine to me, and I would rather ask than guess.

Optional, no need to act on either of these here

Two small things, both fine to leave for a follow-up.

• The character class still allows whitespace and glob characters. dist/x --experimental-flag, dist/* and dist/?pp all pass, and the generated start script node <path>/main.js is run through a shell, so the shell expands the glob or splits on the space before node sees it. Nothing can chain a second command, since the metacharacters that would do that are blocked, so this is breakage rather than injection. Adding \s, * and ? to the class would close all three together. This is the other half of the space-and-dash note from my first review.
• assertSafeNodeVersion runs later than your other two guards. The assertSafeOutputPath calls happen before anything else in deployToCloudRun, but the version check sits after removeSync(cloudRunOut), both copySync calls and the package.json write. So a rejected version still wipes and half-refills the output directory before throwing. Nothing irreplaceable is lost, since that directory is rebuilt on every deploy anyway, but moving the call up beside the other two would make the failure cost nothing and match the pattern you already established.

One thing for whenever you resolve the existing merge conflict: main no longer has the TODO at all, since #3726 removed it when it landed. Your narrowed version is the one that should survive, because firebaseProject and vpcConnector really are still unvalidated. It would be worth adding the outputPath deploy option to that same line, since it has no rule either.

Sorry for so many review rounds. I felt the extra code you added last commit warranted a deeper look at things.

The containment check in the region spec cannot fail

The comment above it says that compiling the source without running it shows nothing broke out of the string literal. I don't think that holds:

• The same payload rendered through the pre-fix template also parses cleanly under new Script, because it leaves ('').runWith(...) behind, which is valid JavaScript.
• Running that pre-fix source with a stubbed require does execute the injected call.
• Reverting the JSON.stringify fails only the toContain expectation, so the new Script line passes on the vulnerable source and the fixed one alike.

What would pin it is running the rendered source with a stubbed require and asserting the injected call never fires, which is what the comment describes.

The character class is off in both directions, and one direction is my fault

Characters it lets through:

• dist/[ab] and dist/[a-z]pp pass, and with a generated start script of node dist/[ab]/main.js, npm start ran dist/a/main.js.
• ~/x passes and expands to the home directory.

Adding [, ] and ~ closes both cases and still allows ../dist/browser.

The other direction is mine. I asked for \s, * and ? with the Cloud Run start script in mind, and since all four call sites share the helper, they now apply in places where the characters I asked to add can't cause a problem:

• The Cloud Run server outputPath is the only one that reaches a shell, through the generated scripts.start that CMD [ "npm", "start" ] runs.
• The static target's outputPath reaches join(), and from there only filesystem calls: copySync and renameSync in both builders, plus moveSync on the Cloud Functions path.
• On the Cloud Functions path getPackageJson is called without a main, so the start script is firebase functions:shell and the server outputPath lands in require('./<path>/main'), where a space, a tab or a glob character all resolve fine, though a newline would not (the class already rejected those before my note).

So a folder name with a space in it is now rejected at three of the four call sites, and at none of those three could the space have caused a problem. Two ways to fix it:

• Scope the whitespace and glob rules to the Cloud Run server path.
• Quote the path in the generated start script, which fixes the underlying thing rather than screening for it. With scripts.start set to node "dist/my app/main.js" the app starts, while the unquoted form the builder writes today fails.

The functionsNodeVersion rule turns away tags that work

The Dockerfile line is FROM node:<version>-slim, so on the Cloud Run path this value is a Docker tag, and digits-and-dots refuses most of the ones the official image publishes:

• node:lts-slim, node:current-slim, node:iron-slim and node:jod-slim all resolve, and so do node:lts-bookworm-slim and node:22-bookworm-slim. Every one of those is refused now, at validation, before anything runs.
• 20.x, >=18 and ^20 are refused too, and rightly so. node:20.x-slim does not exist, and none of the three is a runtime firebase-tools accepts either.

Would ^(?!latest$)[\w][\w.-]{0,127}$ work for you? That is Docker's own tag grammar with latest excluded.

• It still refuses all five values your assertSafeNodeVersion specs reject, so the suite stays green. Newlines, spaces, tabs, ;, &&, $(...), backticks, quotes, backslashes and a leading dash all stay out.
• The value lands right after node:, so pointing the build at different image content would need a space, a slash, a colon or an @, and all four are refused.
• It adds one bound your current expression does not have. Docker caps a tag at 128 characters, and ^\d+(\.\d+)*$ accepts any length.
• latest stays refused, which is also the honest answer for it. The slim variant of latest is published as node:slim, so node:latest-slim has never existed.
• Two things move together if you take it, the pattern in schema.json and the expression in assertSafeNodeVersion, and the schema description needs a matching edit since it currently promises a plain version.

I ran it both ways before suggesting it. With a bare character set your latest spec fails, and with the version above the suite is 209 passing and lint is clean.

Smaller things

• The assertSafeFunctionName spec lists _app and $fn as valid, but the functionName pattern that #3726 put in the schema rejects both, so neither can reach the builder through a validated angular.json.

• assertSafeOutputPath is typed to return the path, but all four callers discard it, and its two new siblings return nothing. It seems it doesn't need to return anything.

• The region spec finds the generated index.js by position, spy.calls.argsFor(1)[1], so it depends on the package.json write staying first. Selecting the write whose path ends with index.js survives a reorder.

• The region escaping has no test in functions-templates.jasmine.ts, which already has a defaultFunction group. Asserting there that a hostile region comes out JSON.stringify-ed would test the change directly, rather than only through a full builder run that can go red for unrelated reasons.

• deploy codegen input validation (injection hardening) sits next to the existing deploy input validation (command-injection hardening), and the two differ only by the word "codegen". I found the suite output hard to scan with both in it.

The four as casts

The as unknown as on withOutputPaths goes away by annotating its return type and handing back promises, with the unrecognized target throwing rather than returning undefined:

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 }); }
  throw new Error(`Invalid target: ${JSON.stringify(target)}.`);
};

• Throwing there is also closer to the real thing. architect.js throws Invalid target: <target> when a target has no options, so it never hands back an empty object or nothing at all.
• The other three drop with no replacement, and each compiles without its cast: assertSafeNodeVersion(v as string | number | undefined) on line 459, assertSafeFunctionName(n as string | undefined) on 473, and spy.calls.argsFor(1)[1] as string on 543. The first two restate the element type the array literal already gives you.

Naming

p, v and n in the new forEach callbacks. Try to use parameter/variable names that tell you what is being represented.

If I have any of this wrong, say so and I will take another look. Thanks again for all the hard work you've been putting in. I expect this to be the final review round.

Thank you so much for all your work on this @herdiyana256, as well as the other PRs you've created to fix these very important injection issues.

I am approving this PR and merging it now. I look forward to reviewing any future PRs you may create to improve AngularFire.