Skip to content

fix: prune secret versions labelled firebase-managed=functions - #11067

Open
IzaakGough wants to merge 2 commits into
mainfrom
@invertase/fix-issue-11066
Open

fix: prune secret versions labelled firebase-managed=functions#11067
IzaakGough wants to merge 2 commits into
mainfrom
@invertase/fix-issue-11066

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #11066.

What was broken

pruneSecrets queried labels.firebase-managed=true, but labels() has written firebase-managed=functions since 13.6.1. The query matched nothing, so functions:secrets:set printed Removing secret versions: with an empty list and functions:secrets:prune reported All secrets are in use without examining anything. Every other call site uses isFunctionsManaged, which accepts both values.

What changed

List without a label filter and select with isFunctionsManaged. That keeps one definition of "managed", still matches the older =true secrets, and excludes App Hosting secrets. The trade is listing all secrets in the project rather than a filtered page.

Second commit: functions:secrets:set now declares secretmanager.secrets.list, versions.list and versions.destroy. It has always performed those calls in its prune step, but they were unreachable while the query returned nothing, so a caller on a custom role would have hit a raw 403 after the redeploy rather than the upfront permission check.

Verification

Unit tests cover both label values plus the unmanaged and App Hosting cases, and the new test fails against the old code.

Also exercised live against a real project with a deployed function using a secret. Rotating with functions:secrets:set destroyed the superseded versions and left the in-use one enabled; the same rotation on the unfixed build left them enabled. An App Hosting labelled secret, an unlabelled secret and an extensions-managed secret were all left alone.

For the reviewer

This path has been dormant since 13.6.1, so the first run after this lands destroys the whole backlog at once, including versions that were deliberately disabled rather than destroyed (the version query is NOT state: DESTROYED). The secrets:set prompt says "destroy the stale version", singular, and --force skips it, so the list is only printed as it is destroyed. Restricting the version query to state: ENABLED, or naming the versions in the prompt, would both be reasonable; say which you prefer and I will add it.

functions:secrets:prune passes default: true to its confirm, and guard() returns the default in non-interactive mode, so in CI it destroys without a prompt and without --force. Worth deciding whether that should require an explicit flag before this change makes it reachable. Happy to add it here.

pruneSecrets queried labels.firebase-managed=true, but secrets created
since 13.6.1 are labelled firebase-managed=functions, so the query
returned nothing and both functions:secrets:set and
functions:secrets:prune reported success over an empty list.

List without a label filter and select with isFunctionsManaged, which
already accepts both values and excludes App Hosting secrets.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the secret pruning logic to support both firebase-managed=true and firebase-managed=functions labels by fetching all secrets and filtering them client-side. The reviewer suggests optimizing this by performing parallel, server-side filtered API requests for both labels to avoid performance issues in large GCP projects, and updating the unit tests accordingly.

Comment thread src/functions/secrets.ts
Comment thread src/functions/secrets.spec.ts
Pruning stale versions lists and destroys secret versions, which the
command did not declare. The calls were unreachable while pruneSecrets
returned nothing, so a caller on a custom role would have failed with a
raw 403 partway through instead of the upfront permission check.
@IzaakGough
IzaakGough marked this pull request as ready for review September 10, 2026 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

functions:secrets:set and :prune never destroy anything: pruneSecrets filters firebase-managed=true, but labels() writes firebase-managed=functions

2 participants