fix(deploy): prevent code-generation injection from angular.json values - #3739

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, and a note for whenever you rebase.

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.