fix(gotrue)!: assert asyncStorage is provided for PKCE flow in the constructor - #1489
Open
gourabsingha1 wants to merge 7 commits into
Open
fix(gotrue)!: assert asyncStorage is provided for PKCE flow in the constructor#1489gourabsingha1 wants to merge 7 commits into
gourabsingha1 wants to merge 7 commits into
Conversation
Collaborator
|
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.
spydon
force-pushed
the
fix/assert-async-storage-constructor-1446
branch
from
August 5, 2026 15:00
68a6a36 to
5297960
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 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
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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}
Contributor
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.
Contributor
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.
Contributor
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.
spydon
approved these changes
Aug 6, 2026
…age-constructor-1446 # Conflicts: # packages/gotrue/lib/src/gotrue_client.dart # packages/supabase/lib/src/supabase_client.dart # sdk-compliance.yaml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
spydon
force-pushed
the
fix/assert-async-storage-constructor-1446
branch
from
August 10, 2026 13:08
b494925 to
f5b6b3f
Compare
Collaborator
|
I'm not entirely sure we want to set a memory only storage option by default for the supabase package. I think this may hide issues developers may face of missing pkce values after a program restart or creating a new client. I would favor having no default, but still include the memory storage for the user to specify manually. This ensures they are more aware of that restriction. It also makes the new assert in the constructor basically redundant as it would only occur for users of the gotrue package only. I'm curious what you @spydon think of this. If we keep it as the default it think it would make more sense to set the memory storage as the storage option in the gotrue package itself rather than in the supabase package. |
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.
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