Skip to content

fix: make UrlKeyFetcher http.Client injectable for testability#269

Open
demolaf wants to merge 2 commits intomainfrom
fix/injectable-http-client-token-verifier
Open

fix: make UrlKeyFetcher http.Client injectable for testability#269
demolaf wants to merge 2 commits intomainfrom
fix/injectable-http-client-token-verifier

Conversation

@demolaf
Copy link
Copy Markdown
Member

@demolaf demolaf commented May 7, 2026

Makes the HTTP client used by UrlKeyFetcher injectable so that tests can intercept the Google public key certificate endpoint without making real network calls.

This is a prerequisite for the real token verification tests added in the firebase_functions SDK (firebase/firebase-functions-dart#91). Previously UrlKeyFetcher always used a freshly-created http.Client, making it impossible to intercept the certificate fetch in tests.

@demolaf demolaf changed the title fix/injectable-http-client-token-verifier fix: make UrlKeyFetcher http.Client injectable for testability May 7, 2026
Copy link
Copy Markdown
Contributor

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

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 adds support for a custom http.Client in FirebaseTokenVerifier and its key fetchers. Reviewers identified a resource leak risk where a new client is created but not closed, recommending a fallback to the global http.get. Additionally, the review suggests removing redundant null-coalescing checks and extending the custom client support to the JwksFetcher for consistency.

Comment thread packages/firebase_admin_sdk/lib/src/utils/jwt.dart Outdated
Comment thread packages/firebase_admin_sdk/lib/src/utils/jwt.dart
Comment thread packages/firebase_admin_sdk/lib/src/utils/jwt.dart Outdated
Comment thread packages/firebase_admin_sdk/lib/src/utils/jwt.dart
Comment thread packages/firebase_admin_sdk/lib/src/utils/jwt.dart
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

Coverage Report

✅ Coverage 75.13% meets 40% threshold

Total Coverage: 75.13%
Lines Covered: 5159/6867

Package Breakdown

Package Coverage
google_cloud_firestore 78.02%
firebase_admin_sdk 72.60%

Minimum threshold: 40%

@demolaf demolaf force-pushed the fix/injectable-http-client-token-verifier branch from 0088819 to 7e3bf65 Compare May 7, 2026 10:11
@demolaf demolaf requested review from Lyokone and kevmoo May 7, 2026 11:05
Copy link
Copy Markdown
Contributor

@Lyokone Lyokone left a comment

Choose a reason for hiding this comment

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

Could we have a separate PR for formatting changes so it's easier to review actual changes from this PR? (I guess it's related to a Flutter version bump?)

@demolaf demolaf changed the base branch from main to cleanup-doc-comments May 7, 2026 12:10
@demolaf demolaf changed the base branch from cleanup-doc-comments to main May 7, 2026 12:11
@demolaf demolaf force-pushed the fix/injectable-http-client-token-verifier branch from c585122 to d37f6b6 Compare May 7, 2026 12:16
@demolaf demolaf changed the base branch from main to cleanup-doc-comments May 7, 2026 12:17
Base automatically changed from cleanup-doc-comments to main May 7, 2026 12:30
@demolaf demolaf force-pushed the fix/injectable-http-client-token-verifier branch from d37f6b6 to 0521042 Compare May 7, 2026 12: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.

2 participants