fix: prune secret versions labelled firebase-managed=functions - #11067
Open
IzaakGough wants to merge 2 commits into
Open
fix: prune secret versions labelled firebase-managed=functions#11067IzaakGough wants to merge 2 commits into
IzaakGough wants to merge 2 commits into
Conversation
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.
Contributor
There was a problem hiding this comment.
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.
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
marked this pull request as ready for review
September 10, 2026 11:31
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11066.
What was broken
pruneSecretsqueriedlabels.firebase-managed=true, butlabels()has writtenfirebase-managed=functionssince 13.6.1. The query matched nothing, sofunctions:secrets:setprintedRemoving secret versions:with an empty list andfunctions:secrets:prunereportedAll secrets are in usewithout examining anything. Every other call site usesisFunctionsManaged, 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=truesecrets, and excludes App Hosting secrets. The trade is listing all secrets in the project rather than a filtered page.Second commit:
functions:secrets:setnow declaressecretmanager.secrets.list,versions.listandversions.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:setdestroyed 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). Thesecrets:setprompt says "destroy the stale version", singular, and--forceskips it, so the list is only printed as it is destroyed. Restricting the version query tostate: ENABLED, or naming the versions in the prompt, would both be reasonable; say which you prefer and I will add it.functions:secrets:prunepassesdefault: trueto its confirm, andguard()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.