feat(auth): store PKCE verifiers per flow to survive overlapping flows - #1662
Open
spydon wants to merge 2 commits into
Open
feat(auth): store PKCE verifiers per flow to survive overlapping flows#1662spydon wants to merge 2 commits into
spydon wants to merge 2 commits into
Conversation
Each PKCE flow now keeps its code verifier in a slot of its own, keyed `supabase.auth.token-flow-<flowId>-code-verifier`, instead of a single fixed key that a later flow silently overwrote. At most five slots are kept, oldest evicted first, tracked in an index entry since GotrueAsyncStorage cannot enumerate keys. getOAuthSignInUrl and getLinkIdentityUrl return the id of the flow they started as OAuthResponse.flowId, and exchangeCodeForSession accepts it to pick that flow's verifier. A given flow id is looked up in its slot only: falling back to another flow's verifier would spend the single-use auth code. Without a flow id the verifier of the most recently started flow is used, so existing callers are unaffected. Flows that hand back no flow id, such as email OTP, password recovery and email change, can be told apart by enabling appendPkceFlowIdToRedirects. It appends the reserved `sb_flow_id` query parameter to the redirect URL, which round-trips through the auth server and is read back by getSessionFromUrl, so deep link handling picks the right verifier by itself. It is opt-in because the auth server matches redirect URLs against the allow list including the query string. SDK-1429
github-actions
Bot
added
the
auth
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPKCE now supports up to five concurrent flow-scoped verifiers. Flow IDs can pass through redirects and OAuth responses, and code exchange can select a specific verifier. Supabase and Flutter options expose this behavior, with tests covering storage, redirects, exchanges, and cleanup. ChangesConcurrent PKCE flow support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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.
🧹 Nitpick comments (3)
packages/gotrue/test/pkce_flow_test.dart (1)
263-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a prefix-collision case.
The filter in
appendPKCEFlowIdToRedirectkeeps a pair when it is neither equal tosb_flow_idnor prefixed withsb_flow_id=. No test covers a parameter whose name merely starts with the reserved name, such assb_flow_idx. A future change fromstartsWith('sb_flow_id=')tocontains('sb_flow_id')would drop that caller parameter and no test would fail.♻️ Proposed additional test
test('keeps a parameter whose name only shares the prefix', () { expect( appendPKCEFlowIdToRedirect( 'https://example.com/callback?sb_flow_idx=1', 'flow-one', ), 'https://example.com/callback?sb_flow_idx=1&sb_flow_id=flow-one', ); });🤖 Prompt for 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. In `@packages/gotrue/test/pkce_flow_test.dart` around lines 263 - 281, Add a test for appendPKCEFlowIdToRedirect using a URL with the unrelated parameter sb_flow_idx=1, and assert it is preserved while sb_flow_id=flow-one is appended. This should guard against prefix-based filtering removing parameters whose names only begin with sb_flow_id.packages/gotrue/lib/src/gotrue_client.dart (1)
492-502: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider truncating the logged flow id.
flowIdreaches this method from the callback URL throughgetSessionFromUrl, so its value is third-party input of unbounded length. Line 494 writes it into a log record without a length cap. A caller can push an arbitrarily long string, or newline characters, into the log output.The value is not secret, so the impact is limited to log noise and possible log-line injection. A truncation to a fixed prefix keeps the message useful.
♻️ Proposed change
final requestedFlowId = PKCEVerifierStore.validateFlowId(flowId); if (flowId != null && requestedFlowId == null) { - _log.warning('Ignoring malformed PKCE flow id: $flowId'); + final truncated = flowId.length > 64 ? flowId.substring(0, 64) : flowId; + _log.warning('Ignoring malformed PKCE flow id: $truncated'); }🤖 Prompt for 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. In `@packages/gotrue/lib/src/gotrue_client.dart` around lines 492 - 502, Update the malformed-flow warning in the PKCE handling around PKCEVerifierStore.validateFlowId to log only a bounded, sanitized prefix of flowId. Preserve the warning context while preventing unbounded or newline-containing callback input from being written directly to logs.packages/gotrue/lib/src/helper.dart (1)
88-113: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider stripping a stale flow id from the fragment as well.
The function removes an existing
sb_flow_idfrom the query only. It keeps the fragment untouched. A redirect URL that already carriessb_flow_idin its fragment therefore ends up with two values.
getSessionFromUrlreplaces#with&or?before parsing, so both copies land inqueryParameters, and the fragment copy is parsed last. The stale value can then win.This requires the app to place the reserved parameter in the fragment itself, so it is a hardening concern rather than an active defect. Documenting the limitation in the doc comment is enough if you prefer not to parse the fragment.
🤖 Prompt for 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. In `@packages/gotrue/lib/src/helper.dart` around lines 88 - 113, The redirect URL cleanup in the flow-id construction should also remove any existing Constants.pkceFlowIdParam from the fragment before appending the new value, preventing duplicate parsed parameters; alternatively, document this fragment limitation in the function’s doc comment if fragment parsing is intentionally out of scope. Keep the existing query-parameter filtering and fragment preservation behavior for all other parameters.
🤖 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.
Nitpick comments:
In `@packages/gotrue/lib/src/gotrue_client.dart`:
- Around line 492-502: Update the malformed-flow warning in the PKCE handling
around PKCEVerifierStore.validateFlowId to log only a bounded, sanitized prefix
of flowId. Preserve the warning context while preventing unbounded or
newline-containing callback input from being written directly to logs.
In `@packages/gotrue/lib/src/helper.dart`:
- Around line 88-113: The redirect URL cleanup in the flow-id construction
should also remove any existing Constants.pkceFlowIdParam from the fragment
before appending the new value, preventing duplicate parsed parameters;
alternatively, document this fragment limitation in the function’s doc comment
if fragment parsing is intentionally out of scope. Keep the existing
query-parameter filtering and fragment preservation behavior for all other
parameters.
In `@packages/gotrue/test/pkce_flow_test.dart`:
- Around line 263-281: Add a test for appendPKCEFlowIdToRedirect using a URL
with the unrelated parameter sb_flow_idx=1, and assert it is preserved while
sb_flow_id=flow-one is appended. This should guard against prefix-based
filtering removing parameters whose names only begin with sb_flow_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ae0ddca-b83c-4f08-a814-9a740a0dee27
📒 Files selected for processing (11)
packages/gotrue/lib/src/constants.dartpackages/gotrue/lib/src/gotrue_client.dartpackages/gotrue/lib/src/helper.dartpackages/gotrue/lib/src/pkce_verifier_store.dartpackages/gotrue/lib/src/types/auth_response.dartpackages/gotrue/test/pkce_flow_test.dartpackages/supabase/lib/src/supabase_client.dartpackages/supabase/lib/src/supabase_client_options.dartpackages/supabase_flutter/lib/src/clear_auth_url_parameters.dartpackages/supabase_flutter/lib/src/flutter_go_true_client_options.dartsdk-compliance.yaml
Tr00d
approved these changes
Aug 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds concurrent PKCE flow support to the auth clients by storing PKCE code verifiers per-flow (instead of a single overwritable key) and introducing an optional sb_flow_id redirect parameter so callbacks can be matched to the correct pending flow.
Changes:
- Introduces per-flow PKCE verifier slots with bounded eviction and legacy-key fallback for backwards compatibility.
- Extends the OAuth URL response and PKCE exchange path to carry/use an optional
flowId, including automatic selection fromsb_flow_idingetSessionFromUrl. - Propagates an opt-in
appendPkceFlowIdToRedirectsflag through Supabase/Supabase Flutter options and stripssb_flow_idfrom browser URLs after auth handling.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk-compliance.yaml | Registers new auth symbols for SDK parity/compliance tracking. |
| packages/supabase/lib/src/supabase_client.dart | Threads appendPkceFlowIdToRedirects into the GoTrue client initialization. |
| packages/supabase/lib/src/supabase_client_options.dart | Adds AuthClientOptions.appendPkceFlowIdToRedirects with docs/default. |
| packages/supabase_flutter/lib/src/flutter_go_true_client_options.dart | Exposes/pass-through/copyWith support for appendPkceFlowIdToRedirects. |
| packages/supabase_flutter/lib/src/clear_auth_url_parameters.dart | Ensures sb_flow_id is removed from query/fragment after auth handling. |
| packages/gotrue/test/pkce_flow_test.dart | Adds coverage for slot storage/eviction, flow id validation, redirect behavior, and overlapping flows. |
| packages/gotrue/lib/src/types/auth_response.dart | Adds OAuthResponse.flowId for PKCE flow identification. |
| packages/gotrue/lib/src/pkce_verifier_store.dart | New internal per-flow verifier store with bounded index + legacy mirroring. |
| packages/gotrue/lib/src/helper.dart | Adds helper to append/replace sb_flow_id in redirect URLs safely. |
| packages/gotrue/lib/src/gotrue_client.dart | Integrates the verifier store, flow-id-aware PKCE exchange, and redirect URL flow id appending. |
| packages/gotrue/lib/src/constants.dart | Defines sb_flow_id parameter name and max concurrent flow constant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+58
to
+63
| Future<List<String>> store({ | ||
| required String flowId, | ||
| required String verifier, | ||
| }) async { | ||
| await _storage.setItem(key: _slotKey(flowId), value: verifier); | ||
|
|
Comment on lines
+492
to
506
| final requestedFlowId = PKCEVerifierStore.validateFlowId(flowId); | ||
| if (flowId != null && requestedFlowId == null) { | ||
| _log.warning('Ignoring malformed PKCE flow id: $flowId'); | ||
| } | ||
|
|
||
| // A flow id that was given but is unusable must not fall back to the | ||
| // verifier of the most recent flow, for the same reason a missing slot does | ||
| // not: spending the auth code on the wrong verifier loses it for good. | ||
| final codeVerifierRawString = flowId != null && requestedFlowId == null | ||
| ? null | ||
| : await _pkceVerifierStore!.retrieve(flowId: requestedFlowId); | ||
|
|
||
| if (codeVerifierRawString == null) { | ||
| throw AuthException('Code verifier could not be found in local storage.'); | ||
| } |
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.
Summary
Each PKCE flow now keeps its code verifier in a slot of its own instead of a single fixed key that a later flow silently overwrote. Starting an OAuth sign-in while a password recovery was still pending used to break whichever of the two completed second.
Outcome: implemented
Closes #1648, SDK-1429.
What changed
packages/gotrue/lib/src/pkce_verifier_store.dart, new). Verifiers are stored undersupabase.auth.token-flow-<flowId>-code-verifier. At most five are kept, oldest evicted first, tracked in an index entry becauseGotrueAsyncStoragecannot enumerate keys. Flow ids are validated against a fixed shape before they are used to build a storage key, since they can arrive from a callback URL. Marked@internal.OAuthResponse.flowIdreturns the id of the flow thatgetOAuthSignInUrlorgetLinkIdentityUrlstarted,nullon the implicit flow.exchangeCodeForSession(authCode, {String? flowId})picks that flow's verifier. A given flow id is looked up in its slot only, deliberately without falling back to the most recent verifier: submitting a mismatched verifier would spend the single-use auth code. Without a flow id the most recently started flow's verifier is used, so existing callers behave exactly as before.getSessionFromUrlreads the reservedsb_flow_idquery parameter off the callback URL, which is howsupabase_flutterhandles auth deep links, so the right verifier is picked without the app threading anything itself.appendPkceFlowIdToRedirectsonGoTrueClient,AuthClientOptionsandFlutterAuthClientOptionsappendssb_flow_idtoredirectTo. It is the only way to tell concurrent email OTP, password recovery and email change flows apart, since those hand back no flow id. Opt-in and defaulting to false, because the auth server matches redirect URLs against the allow list including the query string, so an entry without a wildcard stops matching once the parameter is appended.sb_flow_idis stripped from the browser URL after a successful exchange on web.The legacy
supabase.auth.token-code-verifierkey is still written and read as a fallback, so nothing on the existing API breaks.Deviation from the reference
The issue text says
signInWithOtp,signInWithSSO,resend,updateUserandresetPasswordForEmailalso return a flow id. The reference implementation only adds it to the OAuth response, so this matches supabase-js as merged rather than as described.Reference
supabase-js PR supabase/supabase-js#2569, commit 97b58eb428556d768ae982c511fa10c4b7b8119f.
Compliance matrix
auth.sign_in.exchange_code_for_sessionstaysimplemented, withGoTrueClient.appendPkceFlowIdToRedirects,AuthClientOptions.appendPkceFlowIdToRedirectsandOAuthResponse.flowIdregistered under it.Tests
37 new tests in
packages/gotrue/test/pkce_flow_test.dartcovering the slot ring and its eviction, the index tolerating malformed contents, flow id validation, redirect parameter construction, and client level behavior: two overlapping flows keeping separate verifiers, exchanging with and without a flow id, unknown and malformed flow ids failing fast,getSessionFromUrlpicking the flow off the URL, and sign-out clearing everything.Full
gotrue(499),supabase(136) andsupabase_flutter(63) suites pass locally against the local Supabase stack.Summary by CodeRabbit
New Features
Bug Fixes