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

[herdiyana256]

The SSR deploy builders interpolate several angular.json-derived values straight into generated artifacts that are later executed.

A server build target's outputPath (read via getTargetOptions) is written raw into the generated Cloud Function index.js as require('./${path}/main') and into the generated package.json start script as node ${path}/main.js. functionsNodeVersion is written raw into the generated Cloud Run Dockerfile as FROM node:${version}-slim. None of these has any validation. A crafted server outputPath such as x').app(); require('child_process').execSync('...'); (' lands as a standalone statement in index.js and runs on every Cloud Function cold start (and locally during firebase serve preview); a crafted functionsNodeVersion injects extra RUN instructions executed during the Cloud Run container build. Reachable the moment a developer runs ng deploy on a malicious or cloned workspace. These are distinct sinks from the gcloud argv path and the execSync calls addressed separately.

The fix validates each build target's outputPath (assertSafeOutputPath) and functionsNodeVersion (assertSafeNodeVersion) before they reach code generation, rejecting values that carry quotes, newlines, or shell metacharacters, and adds a functionsNodeVersion schema pattern. Unit tests cover both validators.

npm run test:node passes (150 specs, 0 failures); lint and typecheck clean.

[armando-navarro]

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.

[herdiyana256]

Thanks for the careful review, and for reproducing both sinks. I took the outcome you preferred and closed the functionName / region gap in this PR rather than deferring it, and folded in the smaller notes too. Pushed as a single amended commit.

Blocking: functionName and region

• functionName: added assertSafeFunctionName, called in deployToFunction before either template runs, so it guards both the default and the CF3v2 exports.<name> positions. It uses a plain-identifier allowlist (^[A-Za-z_$][A-Za-z0-9_$]*$) rather than a blocklist, per your note that an allowlist is easier to reason about here. That also makes the dash case loud: a name like my-fn now throws instead of silently generating a file that does not parse.
• region: went structural. The default template now emits .region(${JSON.stringify(options.region || DEFAULT_FUNCTION_REGION)}) with the surrounding quotes dropped, so it escapes itself the same way the CF3v2 path already did through JSON.stringify. No separate screen needed for that value.

Non-blocking notes

• Escape vs screen: adopted structurally where the context is single (JSON.stringify for region, identifier allowlist for functionName). outputPath still goes through a screen because it lands in three different contexts in the same run (a JS string literal, the node <path>/main.js start script, and join(workspaceRoot, ...)), so there is no one encoding that fits all of them.
• Leading dash: fixed. assertSafeOutputPath now also rejects a value beginning with -, so a server outputPath cannot reach the start script as a node flag.
• Comment accuracy: reworded the comment above assertSafeOutputPath so it states what the character class and the dash check actually enforce, instead of the broader claim.
• A test that fails if the calls disappear: added a deploy codegen hardening is wired into the builders block that drives deployToFunction and deployToCloudRun with hostile outputPath, functionName, and functionsNodeVersion, and asserts they reject. I confirmed the intent by deleting the four call sites: those specs go red (5 failures), and pass again once restored. The region spec renders the function with a hostile region and checks it comes back JSON-escaped and still compiles, so a regression there is caught structurally.
• Spec count: rebased on current main. The count in the original description was stale; the suite is now 170 specs, 0 failures.

Also added pattern entries for functionName and region in schema.json so the constraints hold upstream as well.

npm run test:node (170 specs, 0 failures), npm run test:node-esm, ng lint, and a tsc -p tsconfig.build.json --noEmit are all clean.

[armando-navarro]

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.

[armando-navarro]

Following up on my review above, because I owe you a correction. I reviewed this PR without checking your other open ones first, and that was a mistake on my part.

#3726 already adds a functionName pattern, ^[A-Za-z][A-Za-z0-9_-]{0,62}$, which does what I asked for here and does it better: it permits hyphens, requires a leading letter, and bounds the length at 63, which my suggestion did not. Your 2026-08-07 comment there also points out that functionName and region reach the generated Cloud Functions source and not just argv, which is the same thing I wrote up here as a finding five days later.

So my ask above is really an ask to keep a pattern you had already written, in a PR that was waiting on me.

What I think should happen

The two PRs turn out to fit together rather than compete, since each covers a different layer:

• #3726's schema pattern is permissive enough for a Cloud Run service ID and still rejects whitespace, quotes and a leading dash.
• This PR's assertSafeFunctionName is the stricter identifier rule, and it only runs on the Functions path, which is the path that needs it.

Running both rules over the same inputs: ssr and my_fn pass everywhere, my-ssr-service passes the schema and works on Cloud Run while giving a clear error on the Functions path, and a b --project evil, x;id and -rf are rejected at the schema for both paths.

Concretely, if that reading matches yours: drop the functionName and region pattern changes from this PR and keep the runtime check and the template escaping, letting #3726 own the schema. The two branches currently conflict, so one of them has to give up those lines either way.

You wrote both, so you are better placed than me to say whether that split is right. If you would rather this PR own the schema and #3726 drop it, that works too and I will not argue for one over the other.

Sorry about that. The workflow approvals on your other two PRs are being sorted out as well.

[herdiyana256]

Agreed on the split, that reading matches mine. Each PR ends up owning the layer it fits, so no need to argue one over the other.

Dropped the functionName and region schema patterns from this branch and left them to #3726. Its functionName pattern ^[A-Za-z][A-Za-z0-9_-]{0,62}$ is the better one: it allows the hyphens a Cloud Run service ID needs, requires a leading letter, and bounds the length at 63, none of which mine did. Its region pattern ^[a-z]+-[a-z]+\d+$ is tighter than the ^[a-z0-9-]+$ I had. That clears the schema.json overlap; whichever branch lands first, the other leaves those lines untouched.

What stays here is the part #3726 does not cover:

• assertSafeFunctionName, the strict identifier check, still runs, and only in deployToFunction. That is the one path where the value becomes generated JavaScript (exports.<name>), so the identifier rule belongs there and nowhere else. On the Cloud Run path the same option is a service ID and fix(deploy): pass gcloud arguments as an array instead of a joined string #3726's schema pattern is the right constraint.
• region is escaped structurally in the template with JSON.stringify, so it no longer leans on a schema screen at all.
• the functionsNodeVersion schema pattern and assertSafeNodeVersion, for the Dockerfile FROM line, which are unique to this PR.

Narrowed the TODO above the gcloud calls to firebaseProject and vpcConnector. serviceId is options.functionName, which #3726's pattern now constrains, and the other two are still unguarded, so they stay on the list. That gcloud block is the part #3726 rewrites, so actions.ts will still take a normal merge between the two branches, but only in that block, not in the schema.

npm run test:node and test:node-esm are green (158 specs, 0 failures), ng lint and tsc -p tsconfig.build.json --noEmit are clean.