fix(gotrue)!: assert asyncStorage is provided for PKCE flow in the constructor - #1489
fix(gotrue)!: assert asyncStorage is provided for PKCE flow in the constructor#1489gourabsingha1 wants to merge 7 commits into
Conversation
|
As stated in your linked issue this is considered as a breaking change so won't plan this for main yet but later for v3 when we start actively working on it. |
spydon
changed the title
The constructor assertion trips every GoTrueClient and SupabaseClient built without a pkce storage, including the suites main gained while this branch sat on the old v3 base.
68a6a36 to
5297960
Compare
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesThe change adds PKCE async storage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SupabaseClient
participant GoTrueClient
participant MemoryGotrueAsyncStorage
SupabaseClient->>MemoryGotrueAsyncStorage: Create fallback storage for PKCE
SupabaseClient->>GoTrueClient: Initialize with PKCE and storage
GoTrueClient->>MemoryGotrueAsyncStorage: Store PKCE verifier
GoTrueClient-->>SupabaseClient: Return OAuth sign-in URL with code challenge
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gotrue/lib/src/gotrue_client.dart`:
- Around line 156-159: Replace the constructor assert in
packages/gotrue/lib/src/gotrue_client.dart:156-159 with runtime validation
enforcing that PKCE requires non-null asyncStorage, failing during construction.
Update the corresponding constructor test in
packages/gotrue/test/client_test.dart:869-894 to expect the runtime failure
rather than AssertionError.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25a0956e-2ada-4718-8e25-8c26c6a81ebe
📒 Files selected for processing (18)
packages/gotrue/lib/src/gotrue_client.dartpackages/gotrue/test/admin_delete_user_test.dartpackages/gotrue/test/admin_test.dartpackages/gotrue/test/client_test.dartpackages/gotrue/test/header_isolation_test.dartpackages/gotrue/test/mfa_enroll_test.dartpackages/gotrue/test/src/gotrue_admin_custom_providers_api_test.dartpackages/gotrue/test/src/gotrue_admin_mfa_api_test.dartpackages/gotrue/test/src/gotrue_admin_oauth_api_test.dartpackages/gotrue/test/src/gotrue_mfa_api_test.dartpackages/gotrue/test/src/gotrue_oauth_api_test.dartpackages/supabase/test/client_test.dartpackages/supabase/test/mock_test.dartpackages/supabase/test/realtime_test.dartpackages/supabase/test/stream_filter_test.dartpackages/supabase/test/stream_integration_test.dartpackages/supabase/test/trace_propagation_test.dartpackages/supabase/test/utils.dart
The constructor assert makes asyncStorage mandatory for the pkce flow, which is the default. Without a storage of its own the supabase package would have made every SupabaseClient throw, so ship a MemoryGotrueAsyncStorage in gotrue and use it when no storage is passed. The gotrue tests keep the pkce flow and pass a storage instead of switching to the implicit flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gotrue/test/client_test.dart`:
- Around line 869-881: Replace the PKCE storage requirement assertion in the
GoTrueClient constructor with unconditional runtime validation that throws an
ArgumentError or StateError when PKCE is configured without asyncStorage. Update
the “Constructing a client without an asyncStorage” test to expect that runtime
exception while preserving the existing message assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b273ef0f-b007-4cea-a877-831595787079
📒 Files selected for processing (11)
packages/gotrue/example/main.dartpackages/gotrue/lib/src/gotrue_client.dartpackages/gotrue/lib/src/types/gotrue_async_storage.dartpackages/gotrue/test/client_test.dartpackages/gotrue/test/header_isolation_test.dartpackages/gotrue/test/memory_async_storage_test.dartpackages/gotrue/test/utils.dartpackages/supabase/lib/src/supabase_client.dartpackages/supabase/lib/src/supabase_client_options.dartpackages/supabase/test/client_test.dartsdk-compliance.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/gotrue/test/header_isolation_test.dart
- packages/gotrue/lib/src/gotrue_client.dart
isNotNull on a freshly constructed client can never fail, which DCM flags as a misused test matcher.
There was a problem hiding this comment.
Pull request overview
This PR improves PKCE developer ergonomics in the Supabase Flutter SDK monorepo by validating (and/or supplying) the async storage required to persist PKCE code verifiers. It adds an in-memory GotrueAsyncStorage implementation and updates clients/tests to use it where appropriate.
Changes:
- Added a
GoTrueClientconstructor assert for missingasyncStoragewhenflowTypeis PKCE, plus guidance in docs/examples. - Introduced
MemoryGotrueAsyncStorage(in-memory PKCE verifier storage) and used it as a fallback inSupabaseClientwhen PKCE is enabled without a custom storage. - Updated/added tests to cover the new storage and to ensure PKCE flows work (or fail fast) under the new configuration rules.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk-compliance.yaml | Registers the new public MemoryGotrueAsyncStorage API symbols for compliance tracking. |
| packages/supabase/test/client_test.dart | Adds a test asserting PKCE works without explicitly passing pkceAsyncStorage to SupabaseClient. |
| packages/supabase/lib/src/supabase_client.dart | Falls back to MemoryGotrueAsyncStorage() when PKCE is used and no storage is provided. |
| packages/supabase/lib/src/supabase_client_options.dart | Documents PKCE verifier storage behavior for AuthClientOptions.pkceAsyncStorage. |
| packages/gotrue/test/utils.dart | Reworks TestAsyncStorage to reuse MemoryGotrueAsyncStorage. |
| packages/gotrue/test/src/gotrue_oauth_api_test.dart | Supplies asyncStorage in OAuth API tests to satisfy PKCE requirements. |
| packages/gotrue/test/src/gotrue_mfa_api_test.dart | Supplies asyncStorage in MFA API tests to satisfy PKCE requirements. |
| packages/gotrue/test/src/gotrue_admin_oauth_api_test.dart | Supplies asyncStorage in admin OAuth API tests. |
| packages/gotrue/test/src/gotrue_admin_mfa_api_test.dart | Supplies asyncStorage in admin MFA API tests. |
| packages/gotrue/test/src/gotrue_admin_custom_providers_api_test.dart | Supplies asyncStorage in admin custom provider tests. |
| packages/gotrue/test/mfa_enroll_test.dart | Supplies asyncStorage in MFA enroll test. |
| packages/gotrue/test/memory_async_storage_test.dart | New unit tests covering MemoryGotrueAsyncStorage behavior. |
| packages/gotrue/test/header_isolation_test.dart | Supplies asyncStorage in header isolation test. |
| packages/gotrue/test/client_test.dart | Updates client tests for PKCE storage requirements and adds constructor-assert test. |
| packages/gotrue/test/admin_test.dart | Supplies asyncStorage in admin test setup. |
| packages/gotrue/test/admin_delete_user_test.dart | Supplies asyncStorage in delete-user admin test. |
| packages/gotrue/lib/src/types/gotrue_async_storage.dart | Adds MemoryGotrueAsyncStorage implementation. |
| packages/gotrue/lib/src/gotrue_client.dart | Adds constructor assert requiring asyncStorage for PKCE, with guidance in docs. |
| packages/gotrue/example/main.dart | Updates example to pass MemoryGotrueAsyncStorage for PKCE flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Root cause
When
flowTypeisAuthFlowType.pkce(the default) and noasyncStorageis provided, callinggetOAuthSignInUrl()orgetLinkIdentityUrl()fails late at call time. To provide a better developer experience and fail fast, the misconfiguration should be validated up front in the constructor.Fix
Added an
assertin theGoTrueClientconstructor to validate thatasyncStorage != nullifflowType == AuthFlowType.pkce.Tests
gotrue/test/client_test.dartto verify that GoTrueClient throws anAssertionErrorifasyncStorageis missing when using PKCE flow.gotrueandsupabasepackages to pass a mock storage or specifyimplicitflow type where no storage was configured.Fixes #1446
Summary by CodeRabbit
New Features
Bug Fixes
Tests