-
-
Notifications
You must be signed in to change notification settings - Fork 279
fix: Adds support for SharedPreferencesAsync #1164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
I can confirm that this issue exists, as I encountered this as well in a recent project, but didn't have the time to investigate more and come up with a pr. At that time, I solved it by manually providing my own storage implementation using the async shared preferences. |
Pull Request Test Coverage Report for Build 15563534678Details
💛 - Coveralls |
I thought of another solution like this : Code/// A [LocalStorage] implementation that implements SharedPreferences as the
/// storage method.
class SharedPreferencesLocalStorage extends LocalStorage {
late final SharedPreferences _prefs;
SharedPreferencesLocalStorage({required this.persistSessionKey});
final String persistSessionKey;
static const _useWebLocalStorage =
kIsWeb && bool.fromEnvironment("dart.library.js_interop");
@override
Future<void> initialize() async {
if (!_useWebLocalStorage) {
WidgetsFlutterBinding.ensureInitialized();
_prefs = await SharedPreferences.getInstance();
}
}
@override
Future<bool> hasAccessToken() async {
if (_useWebLocalStorage) {
return web.hasAccessToken(persistSessionKey);
}
return _prefs.containsKey(persistSessionKey);
}
@override
Future<String?> accessToken() async {
if (_useWebLocalStorage) {
return web.accessToken(persistSessionKey);
}
return _prefs.getString(persistSessionKey);
}
@override
Future<void> removePersistedSession() async {
if (_useWebLocalStorage) {
web.removePersistedSession(persistSessionKey);
} else {
await _prefs.remove(persistSessionKey);
}
}
@override
Future<void> persistSession(String persistSessionString) {
if (_useWebLocalStorage) {
return web.persistSession(persistSessionKey, persistSessionString);
}
return _prefs.setString(persistSessionKey, persistSessionString);
}
}
/// A [LocalStorage] implementation that implements SharedPreferencesAsync as the
/// storage method.
class SharedPreferencesAsyncLocalStorage extends LocalStorage {
late final SharedPreferencesAsync _prefs;
SharedPreferencesAsyncLocalStorage({required this.persistSessionKey});
final String persistSessionKey;
static const _useWebLocalStorage =
kIsWeb && bool.fromEnvironment("dart.library.js_interop");
@override
Future<void> initialize() async {
if (!_useWebLocalStorage) {
WidgetsFlutterBinding.ensureInitialized();
_prefs = SharedPreferencesAsync();
}
}
@override
Future<bool> hasAccessToken() async {
if (_useWebLocalStorage) {
return web.hasAccessToken(persistSessionKey);
}
return _prefs.containsKey(persistSessionKey);
}
@override
Future<String?> accessToken() async {
if (_useWebLocalStorage) {
return web.accessToken(persistSessionKey);
}
return _prefs.getString(persistSessionKey);
}
@override
Future<void> removePersistedSession() async {
if (_useWebLocalStorage) {
web.removePersistedSession(persistSessionKey);
} else {
await _prefs.remove(persistSessionKey);
}
}
@override
Future<void> persistSession(String persistSessionString) {
if (_useWebLocalStorage) {
return web.persistSession(persistSessionKey, persistSessionString);
}
return _prefs.setString(persistSessionKey, persistSessionString);
}
}where the dev has to opt-in for I didn't choose this implementation in my PR because the two classes are pretty much the same except for shared prefs init. |
|
I think CI test fails because with Flutter 3.19 the resolvable |
|
@Vinzent03 , Can you exactly tell me what is the solution you have done for this solution |
the refresh_token_already_used error is coming, though supabase is handling auto refresh - This comes when user again opens the app (Cold start) after the token expiry time, then instead of emitting token refreshed state, the error is coming! |
|
If we want to provide a Either way @romaingyh I prefer the two distinct classes implementation. |
Broken in what way? |
Done, PR has now two classes : |
|
@dshukertjr The way it's described in the pr description. Writes to |
|
@romaingyh I know this blows up the scope of this PR, but I'm okay bumping the minimum Flutter/ Dart version to 3.220/ 3.4.0 and to just use |
0470383 to
ccbd41f
Compare
|
@dshukertjr I bumped flutter/dart to 3.22/3.4.0 and replaced SharedPreferences with SharedPreferencesAsync. I also added the token migration at init but not tested |
|
But using the new async variant by default causes existing usage in the user's code of the old variant to break, doesn't it? This is a heavy breaking change. |
|
Yes as Async prefs overwrite the legacy prefs (I'm sure on windows but not for other platforms) I think there is a risk of user losing it's preferences if he upgrade supabase but still use legacy prefs in his code |
ccbd41f to
fda1304
Compare
|
I rebased |
|
@romaingyh do you know when your fix will be available in the public release of Supabase? Thank you. I updated my app today and broke this Windows sign in as described here. |
Sorry no I'm not an officiel maintainer of this repo. We're currently waiting for the main maintainer to publish new releases. Here is what I'm doing for now. In pubspec : supabase_flutter:
git:
url: https://github.com/romaingyh/supabase-flutter.git
ref: shared_prefs_async
path: packages/supabase_flutterand use this in supabase initialization auth options : localStorage: SharedPreferencesLocalStorage(
persistSessionKey: "sb-${Uri.parse(url).host.split(".").first}-auth-token",
),You can also override supabase to have the last fixes : # TODO: Remove when supabase > 2.8.0 published on pub.dev, including https://github.com/supabase/supabase-flutter/pull/1196
supabase:
git:
url: https://github.com/romaingyh/supabase-flutter.git
path: packages/supabase |
…: SharedPreferencesLocalStorage & SharedPreferencesAsyncLocalStorage
…to new SharedPreferencesAsync
fda1304 to
712a2ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the SharedPreferencesLocalStorage class from the legacy SharedPreferences API to the newer SharedPreferencesAsync API to address compatibility issues on platforms like Windows where the two storage mechanisms are incompatible. The change prevents users from being logged out when using SharedPreferencesAsync in their application code.
Changes:
- Updated
shared_preferencesdependency from^2.0.0to^2.3.0 - Migrated
SharedPreferencesLocalStorageto useSharedPreferencesAsyncinstead of the legacySharedPreferences - Added automatic migration logic to transfer existing sessions from legacy to async storage
- Bumped minimum SDK version from 3.3.0 to 3.4.0 and Flutter from 3.19.0 to 3.22.0
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/supabase_flutter/pubspec.yaml | Updates dependency constraints for SDK, Flutter, and shared_preferences package |
| packages/supabase_flutter/lib/src/local_storage.dart | Migrates SharedPreferencesLocalStorage to use SharedPreferencesAsync and adds migration logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sdk: '>=3.4.0 <4.0.0' | ||
| flutter: '>=3.22.0' |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SDK and Flutter version bumps are significant breaking changes (from 3.3.0 to 3.4.0 and 3.19.0 to 3.22.0). These version requirements should be justified - are they necessary for SharedPreferencesAsync support? If not, consider using the original minimum versions to avoid unnecessarily restricting the user base.
| sdk: '>=3.4.0 <4.0.0' | |
| flutter: '>=3.22.0' | |
| sdk: '>=3.3.0 <4.0.0' | |
| flutter: '>=3.19.0' |
| final legacyPrefs = await SharedPreferences.getInstance(); | ||
|
|
||
| if (legacyPrefs.containsKey(persistSessionKey)) { | ||
| final accessToken = legacyPrefs.getString(persistSessionKey); | ||
|
|
||
| if (accessToken != null) { | ||
| await legacyPrefs.remove(persistSessionKey); | ||
| await _prefs.setString(persistSessionKey, accessToken); | ||
| } |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration logic should handle errors gracefully. If the migration fails (e.g., due to storage access issues), the error could leave the session in an inconsistent state or prevent initialization. Consider wrapping the migration in a try-catch block and logging any errors, allowing initialization to proceed even if migration fails.
| final legacyPrefs = await SharedPreferences.getInstance(); | |
| if (legacyPrefs.containsKey(persistSessionKey)) { | |
| final accessToken = legacyPrefs.getString(persistSessionKey); | |
| if (accessToken != null) { | |
| await legacyPrefs.remove(persistSessionKey); | |
| await _prefs.setString(persistSessionKey, accessToken); | |
| } | |
| try { | |
| final legacyPrefs = await SharedPreferences.getInstance(); | |
| if (legacyPrefs.containsKey(persistSessionKey)) { | |
| final accessToken = legacyPrefs.getString(persistSessionKey); | |
| if (accessToken != null) { | |
| await legacyPrefs.remove(persistSessionKey); | |
| await _prefs.setString(persistSessionKey, accessToken); | |
| } | |
| } | |
| } catch (e, stackTrace) { | |
| debugPrint( | |
| 'Error while migrating access token for key "$persistSessionKey": $e'); | |
| debugPrint(stackTrace.toString()); |
| } | ||
| } | ||
|
|
||
| Future<void> _maybeMigrateAccessToken() async { |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new migration logic in _maybeMigrateAccessToken lacks test coverage. Since the existing tests for SharedPreferencesLocalStorage use setMockInitialValues which may not properly test the migration from legacy SharedPreferences to SharedPreferencesAsync, add tests to verify: 1) successful migration when legacy data exists, 2) no-op when legacy data doesn't exist, and 3) proper cleanup of legacy storage after migration.
|
|
||
| if (accessToken != null) { | ||
| await legacyPrefs.remove(persistSessionKey); | ||
| await _prefs.setString(persistSessionKey, accessToken); |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable accessToken appears to hold a Supabase auth/session token and is being stored in cleartext using SharedPreferencesAsync, which typically persists data in an unencrypted file on the device. Because this is a bearer token, any attacker or malicious app able to read the underlying shared preferences storage (e.g., via device compromise, backup extraction, or sandbox escape) can reuse this token to impersonate the user against Supabase, leading to account compromise. Severity: HIGH. Confidence: 8
What kind of change does this PR introduce?
It's both a bug fix and feature.
9 months ago with version 2.3.0,
shared_preferencespackage addedSharedPreferencesAsyncandSharedPreferencesWithCacheto replace the legacy (and deprecated in future)SharedPreferences. See hereSupabase flutter uses by default the legacy one with
SharedPreferencesLocalStorage.Problem is that on some platform like Windows, the legacy one and new one are not compatible. If the developer made the migration to
SharedPreferencesAsyncthen the supabase's local storage is broken.Example :
I'm using
SharedPreferencesAsyncin my app. After sign in, thesb-x-auth-tokenis saved insaved-preferences.jsonby supabase'sSharedPreferences. After that, any call to my app'sSharedPreferencesAsyncwill overwrite the token and user have to sign in on next app launch. In practice it can lead to sign in every time he opens the app if you use shared preferences frequently in your code.What is the new behavior?
I added a boolean flag
useSharedPreferencesAsynctoSharedPreferencesLocalStorage. There wasn't very much changes as every methods are already async in LocalStorage interface.If you prefer two distinct classes
SharedPreferencesLocalStorageandSharedPreferencesAsyncLocalStorageI can do this