fix(deploy): pass gcloud arguments as an array instead of a joined string

[herdiyana256]

`spawnAsync` built the gcloud command as a single template-literal string and split it on whitespace (`command.split(/\s+/)`) before handing it to `spawn()`. Any deploy option containing a space (`region`, `firebaseProject`, `functionName`, `cloudRunOptions.vpcConnector`, none of which have a schema `pattern`) would be split into extra argv entries, letting a value from `angular.json` add unintended flags to the `gcloud builds submit` / `gcloud run deploy` / `gcloud auth activate-service-account` invocations.

`spawnAsync` now takes `command` and `args` separately, matching `child_process.spawn`'s own signature, and the three call sites build their argument lists as arrays instead of interpolating into one string. This removes the join/split round-trip entirely rather than trying to validate each field individually, addressing the existing `// TODO validate serviceId, firebaseProject, and vpcConnector both to limit errors and opp for injection` comment.

Verified with a standalone repro pointing `spawnAsync` at a fake `gcloud` binary that records its argv: a `region` value of `"us-central1 --update-env-vars=..."` previously landed as two separate argv tokens (the injected flag reaching `gcloud` as its own argument); after this change it lands as a single `--region` value.

`npx tsc --noEmit` and `npx eslint src/schematics/deploy/actions.ts` both pass clean.

[armando-navarro]

Thanks for this, and for the unusually clear writeup and repro. I worked through it and everything checks out on my end.

I reproduced the original problem against the compiled deploy code:

• Driving deployToCloudRun with a region of us-central1 --set-env-vars=INJECTED=owned, the old whitespace split sent --set-env-vars=INJECTED=owned to gcloud as its own argument.
• With your change the same input stays a single --region value, so the split is gone.
• I also confirmed all three spawnAsync call sites are converted and that a space in a legitimate value (a path, a project name) no longer breaks the invocation.

A few things came up, none blocking your fix. The first is the one I would most suggest folding in while you are here.

Worth folding in: spawnAsync only treats exit code 1 as failure

This one predates your PR, so it is not something you introduced, but you are editing this exact function so it is a natural place to fix it. The close handler rejects only when code === 1 and resolves for anything else:

• gcloud's scripting docs only promise a "non-zero" exit on failure, not specifically 1, and gcloud commands do exit other non-zero values, so those failures currently resolve as success.
• A build killed by a signal (an out-of-memory gcloud builds submit, for instance) arrives with code === null, which also resolves as success.

The effect is that a failed builds submit or run deploy can be reported as a successful deploy and the schematic prints success anyway. Changing the guard to if (code !== 0) would make any non-zero or signal exit reject. Entirely your call whether to include it here or leave it for a follow-up.

Optional, defense-in-depth: the service name is a positional argument

gcloud run deploy takes the service name (functionName) as a positional argument, and the schema does not constrain that value.

• If it starts with a dash, gcloud's parser reads it as a flag rather than as the name.
• Your array change already stops space-splitting everywhere, and this is a narrower and pre-existing case, so it is not something you need to solve here.
• If you want to close it too, putting a literal -- right before the service name ('deploy', '--', serviceId) makes gcloud treat everything after it as values.

Optional: the argument form

You switched --region=${options.region} to --region, options.region. A couple of notes if you want to weigh keeping the = form:

• Both forms are valid gcloud syntax, but the docs note the = form is required when a value can start with -.
• With arguments passed as an array this is not a security concern either way, so it is purely a judgment call.

Keeping --region=, --project= would match gcloud's own recommendation, if you would rather.

Optional: a type nit on the optional values

region and firebaseProject are optional in the deploy schema, so as array elements they are technically string | undefined. Two things worth knowing before you decide whether to touch it:

• It has no runtime effect: Node coerces a missing value to the string "undefined", the same result as the old interpolation.
• It does not affect the build or any check that runs on the PR.

If you want the types exactly right, a small narrowing on those two would do it.

For a follow-up, not here

While I was in the file I noticed two older spots that build a shell command by interpolating values into a string, the same shape as what you fixed here:

• actions.ts:124, an execSync for the package-version lookup.
• actions.ts:247, an execSync running npm install on the Cloud Functions path, where the path comes from a user option.

They predate your change and are out of scope for this PR. I wanted to flag them in case you or we want to pick them up separately.

Would you be open to adding a small test for the arg construction? The path did not have coverage before, so a test that asserts the argv shape would lock your fix in. Happy to point at the existing actions.jasmine.ts harness if useful. I can take care of any of these suggestions myself as well, if you'd prefer.

Either way, thank you again, this is a good catch.

[herdiyana256]

Thanks for the thorough review. Pushed 7c2668e addressing the two I'd call must-do:

• spawnAsync's close handler now rejects on code !== 0 instead of only code === 1, per your note.
• Extracted the gcloud args construction for both Cloud Run calls into pure, exported functions (buildCloudRunBuildsSubmitArgs, buildCloudRunDeployArgs) and added tests in actions.jasmine.ts asserting a value with a space (region, firebaseProject, a cloudRunOptions value) stays a single argv entry. Went this route instead of mocking child_process.spawn directly, since it locks in the actual regression without needing to fight ESM module mocking.

Left the rest as follow-ups rather than guessing:

• The -- positional guard for serviceId: agree it's worth doing, but I don't have a way to verify gcloud's actual argparse behavior around -- placement (specifically whether flags after it, like --image, still parse normally) without a real gcloud install to test against. Didn't want to land something unverified that could silently break legitimate deploys.
• --region value vs --region=value: leaving as-is since you noted it's not a security difference either way now.
• The two other execSync string-interpolation spots (124, 247) and the type nit on optional values: agreed these are out of scope here, happy to see them picked up separately.

npx tsc --noEmit and npx eslint src/schematics/deploy/actions.ts src/schematics/deploy/actions.jasmine.ts both pass clean. Full build:jasmine needs the monorepo package build first (not something I have available to run standalone), so I validated the extracted argv-construction functions and the exact test assertions in an isolated esbuild-transpiled run instead, which passed.

[armando-navarro]

Thank you, this is a great turnaround, and both changes look right to me.

I pulled 7c2668e and ran the full build with the node suite:

• It passes at 59 specs, and I checked your new argv tests are doing real work by recombining --region with its value and watching the suite go red.
• Extracting buildCloudRunDeployArgs and buildCloudRunBuildsSubmitArgs as pure functions and asserting on them is a nicer approach than mocking spawn, and it reads well.

One correction I owe you, and it cuts against my own earlier note: you were right not to land the -- change unverified.

• My original placement (a -- before the service name) would have pushed the flags behind the separator and broken the deploy, which is the failure you raised.
• I then thought a trailing -- (service name last) would be the safe form, but I could not verify that against a real gcloud either. The gcloud run deploy reference documents no -- separator, and the one documented use of -- in gcloud is passing arguments through to an external program rather than marking the end of flags, so I am not going to assert any -- form here.
• I should also be straight that the underlying worry, a leading-dash service name being read as a flag, is something I reasoned about from argument-parser behavior, not something I confirmed on a real gcloud.

If you ever do want to close that edge without depending on gcloud's parser at all, the surest route is a pattern on functionName in the schema so a value starting with a dash never reaches the command. It is a pre-existing edge and entirely optional.

Thank you for holding the line on not shipping something untested, that instinct was the right one.

Leaving the rest as follow-ups sounds right to me. Thanks again for the careful work on this.

[herdiyana256]

Thanks for going back and correcting your own note on the -- placement. That saved me from a bad follow-up, and it is the second time in this review that checking beat reasoning.

Picked up your schema suggestion in 530af9b, and while implementing it I found the argument for it is stronger than the leading-dash edge alone. Both values also land in the generated Cloud Functions source, not just in argv:

// functions-templates.ts, defaultFunction / functionGen2
exports.${functionName || DEFAULT_FUNCTION_NAME} = functions
  .region('${options.region || DEFAULT_FUNCTION_REGION}')

functionName becomes a property name in generated JavaScript and region sits inside a single-quoted string literal there, so neither is constrained by the array change in this PR. A schema pattern covers both sinks at the source, which is the part I like about your suggestion: it does not depend on gcloud's parser, and it does not depend on my getting every downstream interpolation right either.

• functionName: ^[A-Za-z][A-Za-z0-9_-]{0,62}$. Has to satisfy two consumers at once, so it permits letters, digits, hyphens and underscores but must start with a letter. That covers Cloud Run service IDs and the JS identifiers the Functions template needs, rejects a leading dash, and rejects whitespace and quotes. The default ssr passes.
• region: ^[a-z]+-[a-z]+\d+$. I checked this against every current GCP region I could enumerate, 39 of them including the awkward ones (northamerica-northeast2, australia-southeast1, me-central2, africa-south1, europe-west12). None are rejected.

I deliberately left firebaseProject and cloudRunOptions.vpcConnector alone. They are named in the original TODO, but I do not have a documented naming rule for either that I am confident enough to encode, and a wrong pattern there breaks real deploys. Better as its own change by someone who can check them.

Also merged main in, since the branch had drifted 8 commits behind.

On verification, to be precise about what I did and did not run: npx eslint is clean on the touched files, and schema.json parses. npx tsc --noEmit reports 181 errors, but they are all the unbuilt @angular/fire self-imports and the count is identical on an unmodified origin/main checkout, with zero of them in src/schematics/deploy. I confirmed the two regexes by running them over the region list and over the injection payloads rather than by eye.

I think that clears everything except the follow-ups we both agreed to leave. Since the remaining items are optional and the workflow runs need a maintainer to approve them, would you mind approving when you get a chance?