From 9841aecd325c2da3db18c4fb1b9926d2fc61eb9c Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 15 Jun 2026 22:57:05 +0200 Subject: [PATCH] Store the GitHub sync token in the OS keystore, migrating off plaintext MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token moved from plaintext SharedPreferences into flutter_secure_storage (Android Keystore / libsecret). Only the non-secret owner/repo/clientId stay in prefs. Migration is confirm-before-delete: load() reads the keystore first and falls back to the legacy plaintext token, migrating it only once a secure write succeeds; save() likewise keeps writing plaintext if no secret service is available, so we degrade to — never below — the old behaviour. 157 tests, 100% line coverage, analyze clean. Verified on-device: Settings stays connected and sync succeeds after the one-time migration. Co-Authored-By: Claude Opus 4.8 --- lib/sync/sync_settings.dart | 65 ++++++++++++++++-- linux/flutter/generated_plugin_registrant.cc | 4 ++ linux/flutter/generated_plugins.cmake | 1 + pubspec.lock | 48 +++++++++++++ pubspec.yaml | 3 + test/capture_screen_test.dart | 2 + test/fake_secure_storage.dart | 50 ++++++++++++++ test/settings_screen_test.dart | 3 + test/sync_settings_test.dart | 72 +++++++++++++++++++- 9 files changed, 241 insertions(+), 7 deletions(-) create mode 100644 test/fake_secure_storage.dart diff --git a/lib/sync/sync_settings.dart b/lib/sync/sync_settings.dart index 419a442..e5c6e3d 100644 --- a/lib/sync/sync_settings.dart +++ b/lib/sync/sync_settings.dart @@ -1,11 +1,14 @@ +import 'package:flutter_secure_storage/flutter_secure_storage.dart'; import 'package:shared_preferences/shared_preferences.dart'; /// Locally-stored GitHub sync configuration. /// -/// NOTE: the token is currently stored in plain `SharedPreferences`. That is -/// acceptable for a personal dogfood build, but should move to -/// `flutter_secure_storage` (Android Keystore / libsecret) before this is -/// considered done. Tracked as a follow-up. +/// The GitHub token is kept in the OS keystore (Android Keystore / libsecret) +/// via [flutter_secure_storage]; only the non-secret owner/repo/clientId live +/// in `SharedPreferences`. Older builds stored the token in plaintext prefs; +/// [load]/[save] migrate it transparently and never drop the plaintext copy +/// until a secure write is confirmed (so we degrade to — never below — the old +/// behaviour when no secret service is available). class SyncSettings { const SyncSettings({ required this.owner, @@ -37,9 +40,18 @@ class SyncSettings { static const _kOwner = 'sync.owner'; static const _kRepo = 'sync.repo'; + // Legacy plaintext location for the token; read-only now and removed once the + // token has been migrated into secure storage. static const _kToken = 'sync.token'; static const _kClientId = 'sync.clientId'; + /// Key for the token inside the OS keystore. + static const _secureToken = 'sync.token'; + + /// Default options keep us off the deprecated `encryptedSharedPreferences` + /// path on Android and use libsecret on Linux. + static const _secure = FlutterSecureStorage(); + /// Loads settings, defaulting the repo to `kuhyx/todo-sync` and the client id /// to the baked-in [defaultClientId] so first run (and any reinstall) needs /// nothing but a single "Connect GitHub" tap. @@ -48,17 +60,58 @@ class SyncSettings { return SyncSettings( owner: prefs.getString(_kOwner) ?? 'kuhyx', repo: prefs.getString(_kRepo) ?? 'todo-sync', - token: prefs.getString(_kToken) ?? '', + token: await _loadToken(prefs), clientId: prefs.getString(_kClientId) ?? defaultClientId, ); } + /// Reads the token, preferring the keystore and falling back to the legacy + /// plaintext value. A legacy value is migrated into the keystore on read, but + /// only dropped from prefs once that secure write succeeds. + static Future _loadToken(SharedPreferences prefs) async { + String? secure; + try { + secure = await _secure.read(key: _secureToken); + } catch (_) { + // No secret service available — fall back to the legacy plaintext copy. + secure = null; + } + if (secure != null && secure.isNotEmpty) return secure; + + final legacy = prefs.getString(_kToken) ?? ''; + if (legacy.isNotEmpty && await _writeSecureToken(legacy)) { + await prefs.remove(_kToken); + } + return legacy; + } + Future save() async { final prefs = await SharedPreferences.getInstance(); await prefs.setString(_kOwner, owner); await prefs.setString(_kRepo, repo); - await prefs.setString(_kToken, token); await prefs.setString(_kClientId, clientId); + // Confirm-before-delete: only remove the plaintext copy once the keystore + // write succeeds; otherwise keep persisting it to prefs as before. + if (await _writeSecureToken(token)) { + await prefs.remove(_kToken); + } else { + await prefs.setString(_kToken, token); + } + } + + /// Writes [token] to the keystore (deleting the entry when empty). Returns + /// false if the platform secret service is unavailable. + static Future _writeSecureToken(String token) async { + try { + if (token.isEmpty) { + await _secure.delete(key: _secureToken); + } else { + await _secure.write(key: _secureToken, value: token); + } + return true; + } catch (_) { + return false; + } } SyncSettings copyWith({ diff --git a/linux/flutter/generated_plugin_registrant.cc b/linux/flutter/generated_plugin_registrant.cc index 7299b5c..3ccd551 100644 --- a/linux/flutter/generated_plugin_registrant.cc +++ b/linux/flutter/generated_plugin_registrant.cc @@ -7,12 +7,16 @@ #include "generated_plugin_registrant.h" #include +#include #include void fl_register_plugins(FlPluginRegistry* registry) { g_autoptr(FlPluginRegistrar) file_selector_linux_registrar = fl_plugin_registry_get_registrar_for_plugin(registry, "FileSelectorPlugin"); file_selector_plugin_register_with_registrar(file_selector_linux_registrar); + g_autoptr(FlPluginRegistrar) flutter_secure_storage_linux_registrar = + fl_plugin_registry_get_registrar_for_plugin(registry, "FlutterSecureStorageLinuxPlugin"); + flutter_secure_storage_linux_plugin_register_with_registrar(flutter_secure_storage_linux_registrar); g_autoptr(FlPluginRegistrar) url_launcher_linux_registrar = fl_plugin_registry_get_registrar_for_plugin(registry, "UrlLauncherPlugin"); url_launcher_plugin_register_with_registrar(url_launcher_linux_registrar); diff --git a/linux/flutter/generated_plugins.cmake b/linux/flutter/generated_plugins.cmake index 886932b..fbedf4a 100644 --- a/linux/flutter/generated_plugins.cmake +++ b/linux/flutter/generated_plugins.cmake @@ -4,6 +4,7 @@ list(APPEND FLUTTER_PLUGIN_LIST file_selector_linux + flutter_secure_storage_linux url_launcher_linux ) diff --git a/pubspec.lock b/pubspec.lock index d685aa9..e9915d5 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -222,6 +222,54 @@ packages: url: "https://pub.dev" source: hosted version: "6.0.0" + flutter_secure_storage: + dependency: "direct main" + description: + name: flutter_secure_storage + sha256: "7686b1d6a29985dcbb808c59518226e603e3bfa7c0ddfd1a0d00e4cda77c868e" + url: "https://pub.dev" + source: hosted + version: "10.3.1" + flutter_secure_storage_darwin: + dependency: transitive + description: + name: flutter_secure_storage_darwin + sha256: "82329fa5cdf343773b1b6897dea959105a29f092454259edff92f9f6637e8149" + url: "https://pub.dev" + source: hosted + version: "0.3.2" + flutter_secure_storage_linux: + dependency: transitive + description: + name: flutter_secure_storage_linux + sha256: a5f35ddab43cf5c8215d2feb4ce1957851f28c5c37e6f04335066a0602087bf5 + url: "https://pub.dev" + source: hosted + version: "3.0.1" + flutter_secure_storage_platform_interface: + dependency: transitive + description: + name: flutter_secure_storage_platform_interface + sha256: "8ceea1223bee3c6ac1a22dabd8feefc550e4729b3675de4b5900f55afcb435d6" + url: "https://pub.dev" + source: hosted + version: "2.0.1" + flutter_secure_storage_web: + dependency: transitive + description: + name: flutter_secure_storage_web + sha256: "073a62b3aeb866ab4ce795f960413948e51e5a42a9b0c8333b6daf5bb3208a1c" + url: "https://pub.dev" + source: hosted + version: "2.1.1" + flutter_secure_storage_windows: + dependency: transitive + description: + name: flutter_secure_storage_windows + sha256: "471951813a97006d899db4948acc654a4f28c440083ea08178935ce20b173ec1" + url: "https://pub.dev" + source: hosted + version: "4.2.2" flutter_test: dependency: "direct dev" description: flutter diff --git a/pubspec.yaml b/pubspec.yaml index dc04686..e39e751 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -44,6 +44,9 @@ dependencies: url_launcher: ^6.3.2 share_plus: ^13.1.0 file_selector: ^1.1.0 + # Stores the GitHub sync token in the OS keystore (Android Keystore / + # libsecret) instead of plaintext SharedPreferences. + flutter_secure_storage: ^10.3.1 dev_dependencies: flutter_test: sdk: flutter diff --git a/test/capture_screen_test.dart b/test/capture_screen_test.dart index 4ef9800..2781df9 100644 --- a/test/capture_screen_test.dart +++ b/test/capture_screen_test.dart @@ -10,6 +10,7 @@ import 'package:todo/ui/capture_screen.dart'; import 'package:todo/ui/settings_screen.dart'; import 'fake_note_repository.dart'; +import 'fake_secure_storage.dart'; void main() { // A real CRDT DB schedules sqflite timers that never drain under the @@ -24,6 +25,7 @@ void main() { LocalBackup? localBackup, }) async { SharedPreferences.setMockInitialValues(prefs); + installFakeSecureStorage(); // Tall surface so a pushed settings screen builds its whole ListView. tester.view.physicalSize = const Size(1200, 2800); tester.view.devicePixelRatio = 1.0; diff --git a/test/fake_secure_storage.dart b/test/fake_secure_storage.dart new file mode 100644 index 0000000..cfeda58 --- /dev/null +++ b/test/fake_secure_storage.dart @@ -0,0 +1,50 @@ +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// In-memory fake for the `flutter_secure_storage` platform channel so tests +/// never touch the real OS keystore. Install it from a test (or `setUp`) and it +/// auto-removes on tear down. +/// +/// Pass [throwing] to simulate a host with no secret service: every call raises +/// a [PlatformException], which exercises the plaintext-fallback paths in +/// [SyncSettings]. +void installFakeSecureStorage({ + Map? initial, + bool throwing = false, +}) { + const channel = MethodChannel('plugins.it_nomads.com/flutter_secure_storage'); + final store = {...?initial}; + final messenger = + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger; + + messenger.setMockMethodCallHandler(channel, (call) async { + if (throwing) { + throw PlatformException(code: 'unavailable'); + } + final args = (call.arguments as Map?) ?? const {}; + final key = args['key'] as String?; + switch (call.method) { + case 'read': + return store[key]; + case 'write': + store[key!] = args['value'] as String; + return null; + case 'delete': + store.remove(key); + return null; + case 'containsKey': + return store.containsKey(key); + case 'readAll': + return Map.from(store); + case 'deleteAll': + store.clear(); + return null; + default: + return null; + } + }); + + addTearDown(() { + messenger.setMockMethodCallHandler(channel, null); + }); +} diff --git a/test/settings_screen_test.dart b/test/settings_screen_test.dart index f7694c9..5772682 100644 --- a/test/settings_screen_test.dart +++ b/test/settings_screen_test.dart @@ -17,6 +17,7 @@ import 'package:url_launcher_platform_interface/link.dart'; import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart'; import 'fake_note_repository.dart'; +import 'fake_secure_storage.dart'; /// Stub file picker that returns a fixed in-memory file (no disk I/O, so the /// `_import` flow stays timer-free and deterministic under the widget tester). @@ -85,6 +86,7 @@ void main() { FakeNoteRepository? repository, }) async { SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(); // Tall surface so the whole settings ListView builds (its Backup section // is below the default 800×600 fold and would otherwise be lazy-skipped). tester.view.physicalSize = const Size(1200, 2800); @@ -316,6 +318,7 @@ void main() { tester, ) async { SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(); tester.view.physicalSize = const Size(1200, 2800); tester.view.devicePixelRatio = 1.0; addTearDown(tester.view.resetPhysicalSize); diff --git a/test/sync_settings_test.dart b/test/sync_settings_test.dart index 043b289..74c350c 100644 --- a/test/sync_settings_test.dart +++ b/test/sync_settings_test.dart @@ -2,11 +2,18 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:shared_preferences/shared_preferences.dart'; import 'package:todo/sync/sync_settings.dart'; +import 'fake_secure_storage.dart'; + void main() { + // installFakeSecureStorage touches the test binary messenger, which needs the + // binding up first (widget tests get this for free via testWidgets). + TestWidgetsFlutterBinding.ensureInitialized(); + test( 'load returns the kuhyx/todo-sync defaults on a fresh install', () async { SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(); final s = await SyncSettings.load(); expect(s.owner, 'kuhyx'); expect(s.repo, 'todo-sync'); @@ -16,8 +23,9 @@ void main() { }, ); - test('save then load round-trips all fields', () async { + test('save stores the token in the keystore, not in prefs', () async { SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(); await const SyncSettings( owner: 'me', repo: 'notes', @@ -25,6 +33,10 @@ void main() { clientId: 'cid', ).save(); + // Token must not linger in plaintext prefs once secured. + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('sync.token'), isNull); + final s = await SyncSettings.load(); expect(s.owner, 'me'); expect(s.repo, 'notes'); @@ -32,6 +44,64 @@ void main() { expect(s.clientId, 'cid'); }); + test('load reads the token straight from the keystore', () async { + SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(initial: {'sync.token': 'fromKeystore'}); + final s = await SyncSettings.load(); + expect(s.token, 'fromKeystore'); + }); + + test('load migrates a legacy plaintext token into the keystore', () async { + SharedPreferences.setMockInitialValues({'sync.token': 'legacy'}); + installFakeSecureStorage(); + + final s = await SyncSettings.load(); + expect(s.token, 'legacy'); + + // The plaintext copy is dropped once the secure write succeeds, and the + // value now resolves from the keystore on the next load. + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('sync.token'), isNull); + final again = await SyncSettings.load(); + expect(again.token, 'legacy'); + }); + + test( + 'load keeps the plaintext token when no secret service is available', + () async { + SharedPreferences.setMockInitialValues({'sync.token': 'plain'}); + installFakeSecureStorage(throwing: true); + + final s = await SyncSettings.load(); + expect(s.token, 'plain'); + // Never drop the only copy when the keystore write can't be confirmed. + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('sync.token'), 'plain'); + }, + ); + + test('save falls back to plaintext prefs when the keystore fails', () async { + SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(throwing: true); + await const SyncSettings(owner: 'o', repo: 'r', token: 'tok').save(); + + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('sync.token'), 'tok'); + }); + + test('save with an empty token clears the keystore entry', () async { + // Seed a keystore token, then save an empty token: it must be deleted and + // no plaintext copy written. + SharedPreferences.setMockInitialValues({}); + installFakeSecureStorage(initial: {'sync.token': 'old'}); + await const SyncSettings(owner: 'o', repo: 'r', token: '').save(); + + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString('sync.token'), isNull); + final s = await SyncSettings.load(); + expect(s.token, ''); + }); + test('isConfigured requires owner, repo and token', () { expect( const SyncSettings(owner: 'o', repo: 'r', token: 't').isConfigured,