From 58b3f86b2b6764755e55c3bd0d713749de1bb34c Mon Sep 17 00:00:00 2001 From: koniz-dev Date: Sat, 31 Oct 2026 03:35:12 +0700 Subject: [PATCH 1/2] fix(security): apply token-store operations in the order they are issued On web, flutter_secure_storage awaits WebCrypto encryption before a write reaches localStorage but removes a key at once, so a logout's clear, issued after a token refresh had checked the session and started writing, landed first, and the write then left a live access token on a signed-out browser. SecureTokenStore now queues every read, write and delete behind the ones issued before it, so the order the session guards rely on holds on every platform. A login that completes after a stale write still keeps its own tokens, since nothing clears on a stale return. Refs koniz-dev/flutter-starter#986 Co-Authored-By: Claude Opus 5.5 --- .../storage/adapters/secure_token_store.dart | 70 ++++++-- .../repositories/auth_repository_impl.dart | 11 +- .../secure_token_store_order_test.dart | 153 ++++++++++++++++++ 3 files changed, 214 insertions(+), 20 deletions(-) create mode 100644 test/core/storage/adapters/secure_token_store_order_test.dart diff --git a/lib/core/storage/adapters/secure_token_store.dart b/lib/core/storage/adapters/secure_token_store.dart index 12a18bf8..b001fc78 100644 --- a/lib/core/storage/adapters/secure_token_store.dart +++ b/lib/core/storage/adapters/secure_token_store.dart @@ -1,45 +1,85 @@ +import 'dart:async'; + import 'package:flutter_starter/core/constants/app_constants.dart'; import 'package:flutter_starter/core/contracts/storage_contracts.dart'; import 'package:flutter_starter/core/errors/exceptions.dart'; import 'package:flutter_starter/core/storage/secure_storage_service.dart'; /// Secure token store implementation backed by [SecureStorageService]. +/// +/// ## Every operation runs in the order it was issued +/// +/// Each read, write and delete waits for the ones issued before it to settle. +/// The session guards in `AuthRepositoryImpl.refreshToken()` and +/// `AuthInterceptor` check the session immediately before a write, which only +/// helps if a logout's clear, issued after that check, also *lands* after the +/// write. flutter_secure_storage does not promise that on every platform: on +/// web, a write awaits WebCrypto encryption before it reaches `localStorage`, +/// while a delete removes the key at once, so a clear issued after a pending +/// write ran before it and the write then re-stored a token on a signed-out +/// browser. Queuing here makes the order the code issues the order the store +/// applies, on every platform, without clearing anything a newer session +/// wrote. class SecureTokenStore implements ITokenStore { /// Creates a token store backed by secure platform storage. SecureTokenStore(this._secureStorageService); final SecureStorageService _secureStorageService; - @override - Future getAccessToken() { - return _secureStorageService.getString(AppConstants.tokenKey); + /// Tail of the operation queue; null while idle. + Future? _queue; + + /// Runs [operation] once every operation issued before it has settled. A + /// failed operation does not wedge the queue. + Future _inOrder(Future Function() operation) { + final previous = _queue; + final done = Completer(); + _queue = done.future; + Future run() async { + try { + if (previous != null) await previous; + return await operation(); + } finally { + done.complete(); + if (identical(_queue, done.future)) _queue = null; + } + } + + return run(); } @override - Future setAccessToken(String token) async { - return await _secureStorageService.setString(AppConstants.tokenKey, token); - } + Future getAccessToken() => _inOrder( + () => _secureStorageService.getString(AppConstants.tokenKey), + ); + + @override + Future setAccessToken(String token) => _inOrder( + () => _secureStorageService.setString(AppConstants.tokenKey, token), + ); /// Throws [CacheException] if the Keychain/Keystore delete fails. @override - Future clearAccessToken() => _remove(AppConstants.tokenKey); + Future clearAccessToken() => + _inOrder(() => _remove(AppConstants.tokenKey)); @override - Future getRefreshToken() { - return _secureStorageService.getString(AppConstants.refreshTokenKey); - } + Future getRefreshToken() => _inOrder( + () => _secureStorageService.getString(AppConstants.refreshTokenKey), + ); @override - Future setRefreshToken(String token) async { - return await _secureStorageService.setString( + Future setRefreshToken(String token) => _inOrder( + () => _secureStorageService.setString( AppConstants.refreshTokenKey, token, - ); - } + ), + ); /// Throws [CacheException] if the Keychain/Keystore delete fails. @override - Future clearRefreshToken() => _remove(AppConstants.refreshTokenKey); + Future clearRefreshToken() => + _inOrder(() => _remove(AppConstants.refreshTokenKey)); /// Deletes both tokens, attempting the second even when the first fails. /// diff --git a/lib/features/auth/data/repositories/auth_repository_impl.dart b/lib/features/auth/data/repositories/auth_repository_impl.dart index 7c8ae13a..838dd9c3 100644 --- a/lib/features/auth/data/repositories/auth_repository_impl.dart +++ b/lib/features/auth/data/repositories/auth_repository_impl.dart @@ -316,11 +316,12 @@ class AuthRepositoryImpl implements AuthRepository { ); // Each check sits directly before the write it guards, with no await // between them, so a logout that starts after the check issues its clear - // after that write. Where the platform store handles calls in order - // (flutter_secure_storage on Android and iOS), the clear also lands after - // it; elsewhere this narrows the window rather than closing it. A single - // check before both writes let a logout landing during the first write - // clear the store, and the refresh token was then written after it. + // after that write - and the token store applies its operations in the + // order they are issued (SecureTokenStore queues them; on web the + // plugin alone would let the clear land first), so the clear also lands + // after it. A single check before both writes let a logout landing + // during the first write clear the store, and the refresh token was then + // written after it. if (!sessionGeneration.isCurrent(generation)) return stale; await localDataSource.cacheToken(authResponse.token); if (authResponse.refreshToken != null) { diff --git a/test/core/storage/adapters/secure_token_store_order_test.dart b/test/core/storage/adapters/secure_token_store_order_test.dart new file mode 100644 index 00000000..00163345 --- /dev/null +++ b/test/core/storage/adapters/secure_token_store_order_test.dart @@ -0,0 +1,153 @@ +// flutter_secure_storage on web awaits WebCrypto encryption before a write +// reaches localStorage, but removes a key at once. So a logout's clear, issued +// after a token refresh had checked the session and started writing, ran +// before that write landed - and the write then put a live access token back +// on a signed-out browser. SecureTokenStore now applies its operations in the +// order they are issued. These run it over a storage that reorders like the +// web plugin. + +import 'dart:async'; + +import 'package:flutter_starter/core/constants/app_constants.dart'; +import 'package:flutter_starter/core/errors/failures.dart'; +import 'package:flutter_starter/core/storage/adapters/secure_token_store.dart'; +import 'package:flutter_starter/core/storage/secure_storage_service.dart'; +import 'package:flutter_starter/core/utils/result.dart'; +import 'package:flutter_starter/features/auth/data/datasources/auth_local_datasource.dart'; +import 'package:flutter_starter/features/auth/data/datasources/auth_remote_datasource.dart'; +import 'package:flutter_starter/features/auth/data/models/auth_response_model.dart'; +import 'package:flutter_starter/features/auth/data/repositories/auth_repository_impl.dart'; +import 'package:flutter_starter/features/auth/domain/auth_error_codes.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mocktail/mocktail.dart'; + +import '../../../helpers/in_memory_stores.dart'; + +class _MockSecureStorage extends Mock implements SecureStorageService {} + +/// A secure storage whose writes land after an asynchronous step (the web +/// plugin's encryption) and whose deletes land at once. +class _WebLikeStorage { + _WebLikeStorage() { + when(() => mock.getString(any())).thenAnswer( + (i) async => values[i.positionalArguments[0] as String], + ); + when(() => mock.setString(any(), any())).thenAnswer((i) async { + final key = i.positionalArguments[0] as String; + final value = i.positionalArguments[1] as String; + writeStarted.remove(key)?.complete(); + await Future.delayed(const Duration(milliseconds: 20)); + values[key] = value; + return true; + }); + when(() => mock.remove(any())).thenAnswer((i) async { + values.remove(i.positionalArguments[0] as String); + return true; + }); + } + + final mock = _MockSecureStorage(); + final Map values = {}; + + /// Completes when a write to the key starts. + final Map> writeStarted = {}; + + Future whenWriteStarts(String key) => + (writeStarted[key] = Completer()).future; +} + +class _Remote implements AuthRemoteDataSource { + static AuthResponseModel _pair(String prefix) => AuthResponseModel.fromJson({ + 'user': {'id': '42', 'email': 'a@b.c'}, + 'token': '$prefix-access', + 'refresh_token': '$prefix-refresh', + }); + + @override + Future login(String email, String password) async => + _pair('login'); + + @override + Future register( + String email, + String password, + String name, + ) => throw UnimplementedError(); + + /// Offline: the remote logout fails at once, so the local clear follows + /// immediately. + @override + Future logout() async => throw Exception('offline'); + + @override + Future refreshToken(String refreshToken) async => + _pair('new'); +} + +void main() { + late _WebLikeStorage storage; + late SecureTokenStore tokens; + late AuthRepositoryImpl repository; + + setUp(() { + storage = _WebLikeStorage() + ..values[AppConstants.tokenKey] = 'old-access' + ..values[AppConstants.refreshTokenKey] = 'old-refresh'; + tokens = SecureTokenStore(storage.mock); + repository = AuthRepositoryImpl( + remoteDataSource: _Remote(), + localDataSource: AuthLocalDataSourceImpl( + storageService: InMemoryKeyValueStore(), + tokenStore: tokens, + ), + ); + }); + + void expectStale(Result result) { + expect( + (result as ResultFailure).failure, + isA().having( + (f) => f.code, + 'code', + AuthErrorCodes.sessionTerminated, + ), + ); + } + + test('a clear issued after a pending write lands after it', () async { + final write = tokens.setAccessToken('late'); + final clear = tokens.clearAllTokens(); + await (write, clear).wait; + + expect(storage.values[AppConstants.tokenKey], isNull); + expect(storage.values[AppConstants.refreshTokenKey], isNull); + }); + + test('a logout during the refresh write leaves no token behind', () async { + final started = storage.whenWriteStarts(AppConstants.tokenKey); + final refresh = repository.refreshToken(); + await started; + final logout = repository.logout(); + + final (result, _) = await (refresh, logout).wait; + + expect(await tokens.getAccessToken(), isNull); + expect(await tokens.getRefreshToken(), isNull); + expectStale(result); + }); + + test('a login completing after the stale refresh write keeps its own ' + 'tokens', () async { + final started = storage.whenWriteStarts(AppConstants.tokenKey); + final refresh = repository.refreshToken(); + await started; + final login = repository.login('a@b.c', 'pw'); + + final (result, signedIn) = await (refresh, login).wait; + + expect(signedIn, isA>()); + expect(await tokens.getAccessToken(), 'login-access'); + expect(await tokens.getRefreshToken(), 'login-refresh'); + expectStale(result); + }); +} From 985ac8db0551eac6534ff5622b1b83e55b3d33e7 Mon Sep 17 00:00:00 2001 From: koniz-dev Date: Sat, 31 Oct 2026 03:41:12 +0700 Subject: [PATCH 2/2] docs(security): state the ordering guarantee on ITokenStore, and the queue's cost Refs koniz-dev/flutter-starter#986 Co-Authored-By: Claude Opus 5.5 --- lib/core/contracts/storage_contracts.dart | 8 ++++++++ lib/core/storage/adapters/secure_token_store.dart | 5 +++++ 2 files changed, 13 insertions(+) diff --git a/lib/core/contracts/storage_contracts.dart b/lib/core/contracts/storage_contracts.dart index a966403b..ff5b90ff 100644 --- a/lib/core/contracts/storage_contracts.dart +++ b/lib/core/contracts/storage_contracts.dart @@ -24,6 +24,14 @@ abstract class IKeyValueStore { } /// Dedicated token/session storage contract. +/// +/// An implementation must apply its operations in the order they are issued: +/// a clear issued after a write lands after that write. The session guards in +/// `AuthRepositoryImpl.refreshToken()` and `AuthInterceptor` check the session +/// immediately before a write and rely on a logout's later clear removing it. +/// `SecureTokenStore` queues its operations to guarantee this, because the +/// platform store does not on every target. An in-memory store whose methods +/// complete synchronously meets it trivially. abstract class ITokenStore { Future getAccessToken(); Future setAccessToken(String token); diff --git a/lib/core/storage/adapters/secure_token_store.dart b/lib/core/storage/adapters/secure_token_store.dart index b001fc78..32bedfe0 100644 --- a/lib/core/storage/adapters/secure_token_store.dart +++ b/lib/core/storage/adapters/secure_token_store.dart @@ -20,6 +20,11 @@ import 'package:flutter_starter/core/storage/secure_storage_service.dart'; /// browser. Queuing here makes the order the code issues the order the store /// applies, on every platform, without clearing anything a newer session /// wrote. +/// +/// The cost: one platform call that never completes holds every later token +/// operation behind it, the token read on each request and a logout's clear +/// included. Keychain and Keystore failures surface as errors, which do not +/// hold the queue; a call that hangs outright would. class SecureTokenStore implements ITokenStore { /// Creates a token store backed by secure platform storage. SecureTokenStore(this._secureStorageService);