fix(functions): don't crash setting the enqueuer on a queue with no IAM policy by ivliag · Pull Request #11046 · firebase/firebase-tools

ivliag · 2026-09-06T14:49:32Z

Description

cloudtasks.setEnqueuer filters the queue's existing IAM policy without guarding bindings:

const policy: iam.Policy = {
  bindings: existing.bindings.filter((binding) => binding.role !== ENQUEUER_ROLE),

A queue whose IAM policy has never been set comes back from the API with no bindings field at all — iam.Policy declares it required, so nothing catches this at compile time:

$ gcloud tasks queues get-iam-policy myQueue --location=us-central1
etag: ACAB

so the filter throws TypeError: Cannot read properties of undefined (reading 'filter').

How it's reached. A task queue function deployed without an invoker option never calls setEnqueuer at all — upsertTaskQueue guards on if (endpoint.taskQueueTrigger.invoker) — so the queue is created and its policy left unset. Adding invoker later and redeploying takes the update path, which fetches the real (empty) policy and throws. The create path already sidesteps this by passing assumeEmpty, which is why a first deploy with invoker works fine.

What the user sees is misleading — the crash is swallowed into an IAM-permissions message that points at the wrong thing:

Cannot read properties of undefined (reading 'filter')
⚠  functions: Deploys failed. Skipping deletes.

Unable to set the invoker for the IAM policy on the following functions:
	deliverWorkflowFailureDigestProd(us-central1)

Some common causes of this:
- You may not have the roles/functions.admin IAM role. [...]

The account had roles/owner. Recovering takes an out-of-band gcloud tasks queues add-iam-policy-binding to give the policy any binding at all, after which deploys succeed — which is what pointed at the empty policy as the trigger.

This mirrors what run.ts already does for Cloud Run service policies (currentPolicy.bindings?.find, (currentPolicy.bindings || []).filter), so the fix normalizes on read in the same spirit. The same unguarded policy.bindings pattern exists in iam.ts, resourceManager.ts, extensions/diagnose.ts and deploy/extensions/v2FunctionHelper.ts, but those operate on project and extension policies, which in practice always have at least one binding — left alone to keep this fix narrow.

Scenarios Tested

New unit test can insert a binding into a policy that has never been set in src/gcp/cloudtasks.spec.ts. Verified it fails on main with exactly the production error, and passes with the fix:

1) CloudTasks setEnqueuer can insert a binding into a policy that has never been set:
   TypeError: Cannot read properties of undefined (reading 'filter')
    at Object.setEnqueuer (src/gcp/cloudtasks.ts:111:41)

Full src/gcp/cloudtasks.spec.ts suite: 13 passing.
prettier --check clean; eslint on the changed files reports 0 errors and the same 12 pre-existing warnings as main; tsc --noEmit clean.
Reproduced end to end against a real project: a task queue function first deployed without invoker, then redeployed with it, fails as above; seeding the queue policy with a single binding makes the same deploy succeed.

Sample Commands

No command or flag changes. Affects firebase deploy --only functions:<taskQueueFunction>.

google-cla · 2026-09-06T14:49:44Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

gemini-code-assist

Code Review

This pull request fixes a crash when redeploying a task queue function whose queue has no IAM policy set by ensuring that empty bindings are handled correctly. The feedback suggests making the bindings field optional in the iam.Policy interface to avoid using unknown as a type assertion in tests, which violates the repository style guide. Additionally, it is recommended to simplify the implementation by removing the withBindings helper function and instead initializing bindings in-place using the logical nullish assignment operator (??=).

gemini-code-assist · 2026-09-06T14:50:54Z


+    it("can insert a binding into a policy that has never been set", async () => {
+      // A queue with no IAM policy yet comes back without a `bindings` field at all.
+      ct.getIamPolicy.resolves({ etag: "", version: 3 } as unknown as iam.Policy);


According to the repository style guide, we should never use any or unknown as an escape hatch. Using as unknown as iam.Policy is necessary here because iam.Policy incorrectly defines bindings as a required field, even though the GCP API can return policies without it. To adhere to the style guide and prevent similar issues across the codebase, we should update the Policy interface in src/gcp/iam.ts to make bindings optional: bindings?: Binding[];. Once bindings is optional, we can safely write this test without the unknown escape hatch.

References

Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. ^(link)

Dropped the unknown cast — the fixture is now typed Partial<iam.Policy>, which is a real type rather than an escape hatch:

const noBindings: Partial<iam.Policy> = { etag: "", version: 3 }; ct.getIamPolicy.resolves(noBindings as iam.Policy);

On making bindings optional in iam.Policy — I agree the type is wrong today, but I measured it before changing it and it is not contained. bindings?: Binding[] produces 10 TS18048 errors across 5 files:

src/deploy/extensions/v2FunctionHelper.ts(49,5) (60,5) src/extensions/diagnose.ts(46,5) (62,7) src/gcp/cloudtasks.ts(195,17) (201,7) src/gcp/iam.ts(240,19) (243,7) src/gcp/resourceManager.ts(92,9) (98,21)

Note it also breaks inside this file: with bindings optional, existing.bindings ??= [] is no longer enough, because the policy object literal built from it is then typed optional too and policy.bindings.push(...) stops compiling. So the two suggestions in this review conflict — taking the type change means restructuring setEnqueuer further, not just adding ??=.

Each of the other 8 sites needs a real decision (guard, default, or assert) in project- and extension-policy code paths I have no way to exercise. That is a worthwhile cleanup, but it is a different change from this crash fix, and I would rather not fold it into a first-time contribution where I cannot verify the extension paths. Happy to open it as a follow-up if a maintainer wants it — or to take it here if you would rather have it in one go.

gemini-code-assist · 2026-09-06T14:50:54Z

+/**
+ * A queue whose IAM policy has never been set comes back from the API with no
+ * `bindings` field at all, even though iam.Policy declares it required. Fill it in
+ * on read so callers can treat it as the empty array it represents. `run.ts` already
+ * applies the same guard to Cloud Run service policies.
+ */
+function withBindings(policy: iam.Policy): iam.Policy {
+  return { ...policy, bindings: policy.bindings ?? [] };
+}


Instead of defining a helper function withBindings that performs shallow copying of the policy object, we can mutate the retrieved policy in-place using the logical nullish assignment operator (??=). This simplifies the code, avoids unnecessary object allocation, and removes the need for the helper function entirely.

Removed the helper; both fetch sites now do existing.bindings ??= [] in place.

gemini-code-assist · 2026-09-06T14:50:54Z

    };
  } else {
-    existing = await (module.exports.getIamPolicy as typeof getIamPolicy)(name);
+    existing = withBindings(await (module.exports.getIamPolicy as typeof getIamPolicy)(name));


Simplify the assignment by fetching the policy directly and initializing bindings in-place.

existing = await (module.exports.getIamPolicy as typeof getIamPolicy)(name); existing.bindings ??= [];

gemini-code-assist · 2026-09-06T14:50:54Z

      // Re-fetch on conflict
      if (err?.context?.response?.statusCode === 429) {
-        existing = await (module.exports.getIamPolicy as typeof getIamPolicy)(name);
+        existing = withBindings(await (module.exports.getIamPolicy as typeof getIamPolicy)(name));


Simplify the assignment by fetching the policy directly and initializing bindings in-place.

Suggested change

existing = withBindings(await (module.exports.getIamPolicy as typeof getIamPolicy)(name));

existing = await (module.exports.getIamPolicy as typeof getIamPolicy)(name);

existing.bindings ??= [];

ivliag · 2026-09-06T15:01:21Z

@googlebot I signed it!

…AM policy `setEnqueuer` filters the queue's existing IAM policy without guarding `bindings`. A queue whose policy has never been set comes back from the API with no `bindings` field at all, so the filter throws: TypeError: Cannot read properties of undefined (reading 'filter') This surfaces as a failed deploy with "Unable to set the invoker for the IAM policy on the following functions", which points at missing roles rather than at the real cause. It is reachable whenever a task queue function is redeployed after first being deployed without an `invoker` option: the first deploy creates the queue but skips `setEnqueuer` entirely, leaving the policy unset, and every later deploy that does declare an `invoker` takes the update path and throws. Recovering needs an out-of-band `gcloud tasks queues add-iam-policy-binding` to give the policy any binding at all. The create path already sidesteps this by passing `assumeEmpty`, and `run.ts` guards the same way for Cloud Run service policies.

- Replace the `withBindings` helper with an in-place `existing.bindings ??= []` at both fetch sites: same behavior, no extra allocation, no helper. - Type the test fixture as `Partial<iam.Policy>` instead of casting through `unknown`, per the style guide's ban on unknown as an escape hatch.

gemini-code-assist Bot reviewed Sep 6, 2026

View reviewed changes

ivliag added 2 commits September 6, 2026 16:06

ivliag force-pushed the fix/cloudtasks-set-enqueuer-empty-policy branch from 6ccea96 to 580e31a Compare September 6, 2026 15:06

Navigation Menu

Uh oh!

fix(functions): don't crash setting the enqueuer on a queue with no IAM policy - #11046

fix(functions): don't crash setting the enqueuer on a queue with no IAM policy#11046
ivliag wants to merge 2 commits into
firebase:mainfrom
ivliag:fix/cloudtasks-set-enqueuer-empty-policy

ivliag commented Sep 6, 2026

Uh oh!

google-cla Bot commented Sep 6, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Uh oh!

ivliag Sep 6, 2026

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Uh oh!

ivliag Sep 6, 2026

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Uh oh!

ivliag Sep 6, 2026

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Uh oh!

ivliag Sep 6, 2026

Uh oh!

ivliag commented Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	existing = withBindings(await (module.exports.getIamPolicy as typeof getIamPolicy)(name));
	existing = await (module.exports.getIamPolicy as typeof getIamPolicy)(name);
	existing.bindings ??= [];

Uh oh!

Conversation

ivliag commented Sep 6, 2026

Description

Scenarios Tested

Sample Commands

Uh oh!

google-cla Bot commented Sep 6, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivliag Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivliag Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivliag Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivliag Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivliag commented Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants