diff --git a/lib/services/base_shared_preferences_service.dart b/lib/services/base_shared_preferences_service.dart index 2a438d9c..18fc1fd2 100644 --- a/lib/services/base_shared_preferences_service.dart +++ b/lib/services/base_shared_preferences_service.dart @@ -244,27 +244,15 @@ abstract class BaseSharedPreferencesService { /// [UnreadableSensitivePreferenceException], which the startup gate /// classifies as repairable so the user gets the same consented repair as an /// unparseable store. - T? _readTolerant(String key, T? Function() read) { - try { - return read(); - } on TypeError catch (error, stackTrace) { - if (isSensitivePrefKey(key)) { - appLogger.e('Credential preference "$key" is unreadable', error: error, stackTrace: stackTrace); - Error.throwWithStackTrace(UnreadableSensitivePreferenceException(key, error), stackTrace); - } - appLogger.w('Dropping preference "$key" with an unreadable stored type', error: error, stackTrace: stackTrace); - unawaited( - _cache.remove(key).catchError((Object e, StackTrace s) { - appLogger.d('Could not drop unreadable preference "$key"', error: e, stackTrace: s); - }), - ); - return null; - } - } + T? _readTolerant(String key, T? Function() read) => readPreferenceTolerantly(_cache, key, read); - /// Nullable string read routed through [_readTolerant]. Use instead of - /// `prefs.getString(...)` wherever a mistyped stored value must not throw. - String? readNullableString(String key) => _readTolerant(key, () => _cache.getString(key)); + /// Nullable reads routed through [readPreferenceTolerantly]. Use these + /// instead of `prefs.getX(...)` wherever a mistyped stored value must not + /// throw — which is everywhere except a call site that deliberately probes + /// two types to migrate between them. + String? readNullableString(String key) => readTolerantString(_cache, key); + bool? readNullableBool(String key) => _readTolerant(key, () => _cache.getBool(key)); + int? readNullableInt(String key) => _readTolerant(key, () => _cache.getInt(key)); /// Typed read helpers — return the stored value or [defaultValue] when missing. bool readBool(String key, {bool defaultValue = false}) => @@ -504,3 +492,43 @@ class JsonPref extends Pref { @override Future writeTo(BaseSharedPreferencesService svc, T value) => svc.writeString(key, encode(value)); } + +/// Reads a preference, tolerating a stored value whose type no longer matches +/// the declaration. +/// +/// `SharedPreferencesWithCache.getX` is an `as T?` cast, so a value written by +/// an older build, hand-edited, or partially recovered throws `TypeError` +/// rather than returning null. A value we cannot read is indistinguishable +/// from one that was never written, so drop the key and fall back to the +/// declared default instead of letting it propagate — before #1732 a single +/// mistyped preference could fail the entire startup gate. +/// +/// Credential slots are exempt: silently dropping one would sign the user out +/// with no explanation. Those raise [UnreadableSensitivePreferenceException], +/// which the startup gate classifies as repairable so the user gets the same +/// consented repair as an unparseable store. +/// +/// Takes the cache directly so the credential stores — which hold a +/// [SharedPreferencesWithCache] rather than a [BaseSharedPreferencesService] — +/// get the same treatment as the settings layer. +T? readPreferenceTolerantly(SharedPreferencesWithCache cache, String key, T? Function() read) { + try { + return read(); + } on TypeError catch (error, stackTrace) { + if (isSensitivePrefKey(key)) { + appLogger.e('Credential preference "$key" is unreadable', error: error, stackTrace: stackTrace); + Error.throwWithStackTrace(UnreadableSensitivePreferenceException(key, error), stackTrace); + } + appLogger.w('Dropping preference "$key" with an unreadable stored type', error: error, stackTrace: stackTrace); + unawaited( + cache.remove(key).catchError((Object e, StackTrace s) { + appLogger.d('Could not drop unreadable preference "$key"', error: e, stackTrace: s); + }), + ); + return null; + } +} + +/// Tolerant string read for a bare [SharedPreferencesWithCache]. +String? readTolerantString(SharedPreferencesWithCache cache, String key) => + readPreferenceTolerantly(cache, key, () => cache.getString(key)); diff --git a/lib/services/credential_vault.dart b/lib/services/credential_vault.dart index 7362c5e9..7809a5e8 100644 --- a/lib/services/credential_vault.dart +++ b/lib/services/credential_vault.dart @@ -6,6 +6,7 @@ import 'package:flutter/foundation.dart' show visibleForTesting; import '../utils/app_logger.dart'; import 'base_shared_preferences_service.dart'; +import 'sensitive_prefs.dart'; /// Encrypts credentials before they are persisted in Drift config/token /// columns. The database no longer stores raw server tokens; registries @@ -18,7 +19,7 @@ import 'base_shared_preferences_service.dart'; class CredentialVault { CredentialVault._(); - static const String _keyPref = 'credential_vault_key_v1'; + static const String _keyPref = credentialVaultKeyPref; static const String _prefix = 'enc:v1:'; static final AesGcm _algorithm = AesGcm.with256bits(); static Future? _secretKey; @@ -152,7 +153,10 @@ class CredentialVault { } catch (e) { appLogger.d('CredentialVault: prefs reload before key check failed', error: e); } - final stored = prefs.getString(_keyPref); + // Tolerant read: a wrong-typed key must surface as a repairable + // failure, not be mistaken for 'no key yet' and silently replaced — + // that would orphan every ciphertext in the database (#1732). + final stored = readTolerantString(prefs, _keyPref); if (stored != null && stored.isNotEmpty) { return SecretKey(base64Decode(stored)); } @@ -160,13 +164,17 @@ class CredentialVault { await prefs.setString(_keyPref, base64Encode(bytes)); try { await prefs.reloadCache(); - final settled = prefs.getString(_keyPref); - if (settled != null && settled.isNotEmpty) { - return SecretKey(base64Decode(settled)); - } } catch (e) { appLogger.d('CredentialVault: prefs re-read after key write failed', error: e); } + // Outside the catch: if another isolate raced us and left a wrong-typed + // value, swallowing it here would return a key that never durably + // landed, and every ciphertext written under it would be unreadable on + // the next launch. Surface it for repair instead (#1732). + final settled = readTolerantString(prefs, _keyPref); + if (settled != null && settled.isNotEmpty) { + return SecretKey(base64Decode(settled)); + } return SecretKey(bytes); }(); } diff --git a/lib/services/seerr/seerr_session_store.dart b/lib/services/seerr/seerr_session_store.dart index c100a447..c48c17df 100644 --- a/lib/services/seerr/seerr_session_store.dart +++ b/lib/services/seerr/seerr_session_store.dart @@ -18,7 +18,9 @@ class SeerrSessionStore { Future load(String userUuid) async { final prefs = await BaseSharedPreferencesService.sharedCache(); - final raw = prefs.getString(_scopedKey(userUuid)); + // Outside the try below on purpose: an unreadable credential must + // reach the repair prompt, not be swallowed as 'no session'. + final raw = readTolerantString(prefs, _scopedKey(userUuid)); if (raw == null) return null; try { final session = SeerrSession.decode(raw); diff --git a/lib/services/sensitive_prefs.dart b/lib/services/sensitive_prefs.dart index 4fe92c5b..b39d0713 100644 --- a/lib/services/sensitive_prefs.dart +++ b/lib/services/sensitive_prefs.dart @@ -67,9 +67,6 @@ String? profileScopedCredentialBaseKey(String key) { return null; } -/// Whether [key] is a profile-scoped or global tracker session slot. -bool isTrackerSessionPrefKey(String key) => trackerSessionBaseKeys.contains(profileScopedCredentialBaseKey(key)); - /// Whether [key] is a profile-scoped or global Seerr session slot. bool isSeerrSessionPrefKey(String key) => profileScopedCredentialBaseKey(key) == seerrSessionBaseKey; diff --git a/lib/services/settings_service.dart b/lib/services/settings_service.dart index 1fd1b11d..01d536d0 100644 --- a/lib/services/settings_service.dart +++ b/lib/services/settings_service.dart @@ -14,6 +14,7 @@ import '../i18n/strings.g.dart'; import '../models/mpv_config_models.dart'; import '../models/external_player_models.dart'; import 'base_shared_preferences_service.dart'; +import 'sensitive_prefs.dart'; import 'device_performance.dart'; import 'shortcut_action.dart'; export 'base_shared_preferences_service.dart' @@ -94,7 +95,7 @@ class _BufferSizePref extends IntPref { int readFrom(BaseSharedPreferencesService svc) { // SharedPreferences updates in-memory cache synchronously, so the // unawaited disk-flush futures are safe here (idempotent if re-run). - if (svc.prefs.getBool(_bufferSizeMigratedKey) != true) { + if (svc.readNullableBool(_bufferSizeMigratedKey) != true) { svc.prefs.remove(key); svc.prefs.setBool(_bufferSizeMigratedKey, true); } @@ -141,7 +142,7 @@ class _EpisodePosterModePref extends EnumPref { @override EpisodePosterMode readFrom(BaseSharedPreferencesService svc) { - final legacyValue = svc.prefs.getBool(_legacyUseSeasonPosterKey); + final legacyValue = svc.readNullableBool(_legacyUseSeasonPosterKey); if (legacyValue != null) { final migrated = legacyValue ? EpisodePosterMode.seasonPoster : EpisodePosterMode.seriesPoster; svc.prefs.remove(_legacyUseSeasonPosterKey); @@ -158,7 +159,7 @@ class _AppLocalePref extends Pref { @override AppLocale readFrom(BaseSharedPreferencesService svc) { - final code = svc.prefs.getString(key); + final code = svc.readNullableString(key); if (code == null || code.isEmpty) { return resolvePreferredAppLocale(PlatformDispatcher.instance.locales); } @@ -176,7 +177,7 @@ class _AutoPipPref extends Pref { @override bool readFrom(BaseSharedPreferencesService svc) { if (!PlatformDetector.supportsPictureInPicture()) return false; - return svc.prefs.getBool(key) ?? !Platform.isMacOS; + return svc.readNullableBool(key) ?? !Platform.isMacOS; } @override @@ -189,7 +190,7 @@ class _UseExternalPlayerPref extends Pref { @override bool readFrom(BaseSharedPreferencesService svc) { if (!PlatformDetector.supportsExternalPlayers()) return false; - return svc.prefs.getBool(key) ?? false; + return svc.readNullableBool(key) ?? false; } @override @@ -203,7 +204,7 @@ class _AudioPassthroughPref extends Pref { @override bool readFrom(BaseSharedPreferencesService svc) { - final stored = svc.prefs.getBool(key); + final stored = svc.readNullableBool(key); if (stored != null) return stored; // Android TV on ExoPlayer defaults to bitstreaming AC3/EAC3/DTS to the TV/AVR // (Media3 picks bitstream vs PCM via AudioCapabilities), preserving surround. @@ -251,10 +252,10 @@ class _MpvConfigTextPref extends StringPref { @override String readFrom(BaseSharedPreferencesService svc) { - final text = svc.prefs.getString(key); + final text = svc.readNullableString(key); if (text != null) return text; - final legacyJson = svc.prefs.getString(_legacyMpvConfigEntriesKey); + final legacyJson = svc.readNullableString(_legacyMpvConfigEntriesKey); if (legacyJson == null) return ''; try { @@ -596,6 +597,8 @@ class SettingsService extends BaseSharedPreferencesService { @override Future onInit() async { + _assertCredentialsReadable(); + const legacyRecentRoomsKey = 'watch_together_recent_rooms'; await prefs.remove(legacyRecentRoomsKey); @@ -609,6 +612,23 @@ class SettingsService extends BaseSharedPreferencesService { } } + /// Raises [UnreadableSensitivePreferenceException] if any stored credential + /// has a type we cannot read. + /// + /// The credential stores themselves — `CredentialVault`, `TrackerAccountStore`, + /// `SeerrSessionStore` — are consulted long after startup, where a throw + /// would surface as an unhandled provider error rather than the repair + /// prompt. Checking here puts the failure inside a fatal gate step, while + /// the store is open and a surgical single-key repair is still possible + /// (#1732). + /// + /// One pass over the already-cached key set; no I/O. + void _assertCredentialsReadable() { + for (final key in prefs.keys) { + if (isSensitivePrefKey(key)) readTolerantString(prefs, key); + } + } + /// Resolves a video mute toggle without replacing the saved volume with 0. /// /// `persistedVolume` is the non-zero value callers should keep in [volume], diff --git a/lib/services/storage_service.dart b/lib/services/storage_service.dart index 00537545..dc3d974f 100644 --- a/lib/services/storage_service.dart +++ b/lib/services/storage_service.dart @@ -152,7 +152,7 @@ class StorageService extends BaseSharedPreferencesService { } String? getServerEndpoint(ServerId serverId) { - return prefs.getString('$_prefixServerEndpoint$serverId'); + return readNullableString('$_prefixServerEndpoint$serverId'); } Future clearServerEndpoint(ServerId serverId) async { @@ -184,7 +184,7 @@ class StorageService extends BaseSharedPreferencesService { /// sees the same device across launches; not Plex-specific in itself — /// Jellyfin's `DeviceId` header reuses the same value too. Future getOrCreateClientIdentifier() async { - final existing = prefs.getString(_keyClientId); + final existing = readNullableString(_keyClientId); if (existing != null && existing.isNotEmpty) return existing; final generated = const Uuid().v4(); await prefs.setString(_keyClientId, generated); @@ -383,7 +383,7 @@ class StorageService extends BaseSharedPreferencesService { 'Only ConnectionBootstrap._promoteActiveProfileFromLegacy may read this.', ) String? getCurrentUserUUID() { - return prefs.getString(_keyCurrentUserUUID); + return readNullableString(_keyCurrentUserUUID); } /// Clears the legacy `currentUserUUID` slot. Used by the upgrade migration. @@ -409,7 +409,7 @@ class StorageService extends BaseSharedPreferencesService { 'Only ConnectionBootstrap.migrateLegacyPlexAccount may use this.', ) String? getServersListJson() { - return prefs.getString(_keyServersList); + return readNullableString(_keyServersList); } /// Clear the legacy servers list. @@ -429,7 +429,7 @@ class StorageService extends BaseSharedPreferencesService { // Active app-level profile (kids mode / multi-user gating) - String? getActiveProfileId() => prefs.getString(_keyActiveProfileId); + String? getActiveProfileId() => readNullableString(_keyActiveProfileId); Future setActiveProfileId(String id) async { await prefs.setString(_keyActiveProfileId, id); @@ -449,7 +449,7 @@ class StorageService extends BaseSharedPreferencesService { } String? getPlexHomeUsersCacheJson(String connectionId) { - return prefs.getString('$_prefixPlexHomeUsers$connectionId'); + return readNullableString('$_prefixPlexHomeUsers$connectionId'); } Future clearPlexHomeUsersCache(String connectionId) async { @@ -469,7 +469,7 @@ class StorageService extends BaseSharedPreferencesService { } DateTime? getProfileLastUsed(String profileId) { - final ms = prefs.getInt('$_prefixProfileLastUsed$profileId'); + final ms = readNullableInt('$_prefixProfileLastUsed$profileId'); return ms == null ? null : DateTime.fromMillisecondsSinceEpoch(ms); } @@ -498,7 +498,7 @@ class StorageService extends BaseSharedPreferencesService { /// Helper to read and decode JSON `List` from preferences List? _getStringList(String key) { - final jsonString = prefs.getString(key); + final jsonString = readNullableString(key); if (jsonString == null) return null; try { @@ -515,7 +515,7 @@ class StorageService extends BaseSharedPreferencesService { /// [legacyStringOk] - If true, returns {'key': value, 'descending': false} /// when value is a plain string (for legacy library sort) Map? _readJsonMap(String key, {bool legacyStringOk = false}) { - final jsonString = prefs.getString(key); + final jsonString = readNullableString(key); if (jsonString == null) return null; return decodeJsonStringToMap(jsonString, legacyStringOk: legacyStringOk); @@ -554,7 +554,7 @@ class StorageService extends BaseSharedPreferencesService { _forEachScopedKey(baseKey, (key) => _filterServerEntriesFromStringList(key, serverId)); Future _clearSelectedLibraryForServer(String key, ServerId serverId) async { - final selected = prefs.getString(key); + final selected = readNullableString(key); if (selected != null && _belongsToServer(selected, serverId)) { await prefs.remove(key); } diff --git a/lib/services/trackers/tracker_account_store.dart b/lib/services/trackers/tracker_account_store.dart index fc45fd02..65aaa3e8 100644 --- a/lib/services/trackers/tracker_account_store.dart +++ b/lib/services/trackers/tracker_account_store.dart @@ -30,7 +30,7 @@ class TrackerAccountStore { Future load(String userUuid) async { final prefs = await BaseSharedPreferencesService.sharedCache(); - final raw = prefs.getString(_scopedKey(userUuid)); + final raw = readTolerantString(prefs, _scopedKey(userUuid)); if (raw == null) return null; try { return TrackerSession.decode(raw, service: service); diff --git a/test/services/prefs_type_tolerance_test.dart b/test/services/prefs_type_tolerance_test.dart new file mode 100644 index 00000000..fd048ccc --- /dev/null +++ b/test/services/prefs_type_tolerance_test.dart @@ -0,0 +1,155 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/main.dart'; +import 'package:plezy/profiles/profile.dart'; +import 'package:plezy/services/base_shared_preferences_service.dart'; +import 'package:plezy/services/credential_vault.dart'; +import 'package:plezy/services/prefs_recovery.dart'; +import 'package:plezy/services/seerr/seerr_session_store.dart'; +import 'package:plezy/services/sensitive_prefs.dart'; +import 'package:plezy/services/startup_diagnostics.dart'; +import 'package:plezy/services/settings_service.dart'; +import 'package:plezy/services/storage_service.dart'; +import 'package:plezy/services/trackers/tracker_account_store.dart'; +import 'package:plezy/services/trackers/tracker_constants.dart'; + +import '../test_helpers/prefs.dart'; + +/// A stored value whose type no longer matches its declaration used to escape +/// as a raw `TypeError`. Inside the startup gate that failed the whole launch +/// with no indication of which preference was at fault (#1732). +void main() { + group('ordinary preferences', () { + test('a wrong stored type falls back to the declared default', () async { + // `enable_debug_logging` is a BoolPref read by the very first gate step. + resetSharedPreferencesForTest(initialAsync: {'enable_debug_logging': 'yes'}); + final settings = await SettingsService.getInstance(); + + expect(settings.read(SettingsService.enableDebugLogging), isFalse); + }); + + test('the unreadable key is dropped so the next launch starts clean', () async { + resetSharedPreferencesForTest(initialAsync: {'enable_debug_logging': 'yes'}); + final settings = await SettingsService.getInstance(); + + settings.read(SettingsService.enableDebugLogging); + // The removal is deliberately fire-and-forget; let it land. + await Future.delayed(Duration.zero); + + expect(settings.prefs.containsKey('enable_debug_logging'), isFalse); + }); + + test('a readable value is returned unchanged', () async { + resetSharedPreferencesForTest(initialAsync: {'enable_debug_logging': true}); + final settings = await SettingsService.getInstance(); + + expect(settings.read(SettingsService.enableDebugLogging), isTrue); + }); + }); + + group('credential preferences', () { + // Silently discarding a credential would sign the user out with no + // explanation, so every credential family has to reach the repair prompt + // instead of being dropped or mistaken for "not set". + + test('the legacy Plex slot fails the storage step so the gate offers a repair', () async { + // `StorageService.onInit` primes log redaction from this slot, so an + // unreadable value fails a fatal gate step rather than being dropped. + resetSharedPreferencesForTest(initialAsync: {legacyPlexTokenPref: 42}); + + await expectLater( + StorageService.getInstance(), + throwsA(isA().having((e) => e.key, 'key', legacyPlexTokenPref)), + ); + + // Still present: only an explicit, consented repair may remove it. + final prefs = await BaseSharedPreferencesService.sharedCache(); + expect(prefs.containsKey(legacyPlexTokenPref), isTrue); + }); + + test('the gate classifies an unreadable credential as repairable', () async { + final record = describeStartupFailure( + const StartupPhaseException(StartupPhase.storage, _FakeUnreadable()), + StackTrace.empty, + ); + expect(record.repairable, isFalse, reason: 'sanity: an unrelated error is not repairable'); + + final real = describeStartupFailure( + StartupPhaseException( + StartupPhase.storage, + UnreadableSensitivePreferenceException(credentialVaultKeyPref, TypeError()), + ), + StackTrace.empty, + ); + expect(real.repairable, isTrue); + expect(real.phase, StartupPhase.storage); + }); + + test('the credential-vault key surfaces instead of being silently replaced', () async { + // The dangerous outcome is not an exception: it is treating an + // unreadable key as "no key yet", generating a fresh one and orphaning + // every token stored as ciphertext in the database. + resetSharedPreferencesForTest(initialAsync: {credentialVaultKeyPref: 1234}); + CredentialVault.resetKeyForTesting(); + addTearDown(CredentialVault.resetKeyForTesting); + + await expectLater( + CredentialVault.protect('anything'), + throwsA(isA().having((e) => e.key, 'key', credentialVaultKeyPref)), + ); + + final prefs = await BaseSharedPreferencesService.sharedCache(); + expect(prefs.containsKey(credentialVaultKeyPref), isTrue); + }); + + test('every credential family fails settings init, where the repair prompt lives', () async { + // The stores themselves are consulted long after startup, so a throw + // there would be an unhandled provider error rather than a repair + // prompt. `SettingsService.getInstance()` is a fatal gate step. + final families = { + 'vault key': credentialVaultKeyPref, + 'tracker session': profileScopedPrefsKey('abc', 'trakt_session'), + 'seerr session': profileScopedPrefsKey('abc', seerrSessionBaseKey), + }; + + for (final entry in families.entries) { + resetSharedPreferencesForTest(initialAsync: {entry.value: 7}); + await expectLater( + SettingsService.getInstance(), + throwsA(isA().having((e) => e.key, 'key', entry.value)), + reason: entry.key, + ); + } + }); + + test('the stores themselves still refuse to guess', () async { + final trackerKey = profileScopedPrefsKey('abc', 'trakt_session'); + resetSharedPreferencesForTest(initialAsync: {trackerKey: 7}); + await expectLater( + trackerAccountStore(TrackerService.trakt).load('abc'), + throwsA(isA().having((e) => e.key, 'key', trackerKey)), + ); + + // `SeerrSessionStore.load` wraps decoding in a catch-all that returns + // null; the read has to sit outside it or the credential vanishes with + // no prompt at all. + final seerrKey = profileScopedPrefsKey('abc', seerrSessionBaseKey); + resetSharedPreferencesForTest(initialAsync: {seerrKey: 7}); + await expectLater( + const SeerrSessionStore().load('abc'), + throwsA(isA().having((e) => e.key, 'key', seerrKey)), + ); + }); + + test('a readable credential is returned untouched', () async { + resetSharedPreferencesForTest(initialAsync: {legacyPlexTokenPref: 'a-real-token'}); + final storage = await StorageService.getInstance(); + + expect(storage.readNullableString(legacyPlexTokenPref), 'a-real-token'); + }); + }); +} + +/// Stands in for an error the in-app repair cannot address. +class _FakeUnreadable implements Exception { + const _FakeUnreadable(); +}