From 61f7d33fdfa4f0baa0b083e77aea9b458f5d4c43 Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Sun, 5 Jul 2026 12:50:41 +0200 Subject: [PATCH] fix(profiles): treat vault decrypt failures as lost credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed MAC check (key/ciphertext divergence: restored backup, clobbered prefs, racing key generation across isolates) threw from CredentialVault.reveal on the startup profile-settings path and crash-looped the app until data was wiped — one device logged 31 fatals in 16 minutes on 2.8.0. Decrypt failure now means the credential is lost, never a crash: reveal() returns null, ProfileConnectionRegistry maps it onto the existing empty-token lazy-fetch sentinel and heals the row so later boots re-acquire the token instead of re-failing, and revealConnectionConfig degrades tokens to empty strings without marking them migrated. Key init also reloads prefs before deciding to generate and re-reads after writing, adopting whatever landed so all isolates converge on a single key instead of orphaning ciphertext. --- lib/profiles/profile_connection_registry.dart | 16 ++++ lib/services/credential_vault.dart | 67 +++++++++++++--- test/services/credential_vault_test.dart | 80 +++++++++++++++++++ 3 files changed, 150 insertions(+), 13 deletions(-) create mode 100644 test/services/credential_vault_test.dart diff --git a/lib/profiles/profile_connection_registry.dart b/lib/profiles/profile_connection_registry.dart index 1906cac9..5b87fe14 100644 --- a/lib/profiles/profile_connection_registry.dart +++ b/lib/profiles/profile_connection_registry.dart @@ -131,6 +131,14 @@ class ProfileConnectionRegistry { ); } + /// Reset the stored token to the empty-string lazy-fetch sentinel (used + /// when the vault can no longer decrypt it). + Future _clearToken(String profileId, String connectionId) async { + await (_db.update(_db.profileConnections) + ..where((t) => t.profileId.equals(profileId) & t.connectionId.equals(connectionId))) + .write(const ProfileConnectionsCompanion(userToken: Value(''), tokenAcquiredAt: Value(null))); + } + /// Mark the row as recently used. Future markUsed(String profileId, String connectionId) async { await (_db.update(_db.profileConnections) @@ -204,6 +212,14 @@ class ProfileConnectionRegistry { final userToken = row.userToken.isEmpty ? null : await CredentialVault.reveal(row.userToken); if (hasPlaintextToken) { unawaited(recordToken(row.profileId, row.connectionId, userToken!)); + } else if (userToken == null && row.userToken.isNotEmpty) { + // Vault couldn't decrypt the stored token (key/ciphertext divergence). + // Clear it to the empty-string lazy-fetch sentinel so the binder + // re-acquires a token on next use instead of re-failing every boot. + appLogger.w( + 'ProfileConnectionRegistry: clearing undecryptable token for ${row.profileId}/${row.connectionId}', + ); + unawaited(_clearToken(row.profileId, row.connectionId)); } return ProfileConnection( profileId: row.profileId, diff --git a/lib/services/credential_vault.dart b/lib/services/credential_vault.dart index 3cddb42c..7362c5e9 100644 --- a/lib/services/credential_vault.dart +++ b/lib/services/credential_vault.dart @@ -2,7 +2,9 @@ import 'dart:convert'; import 'dart:math'; import 'package:cryptography/cryptography.dart'; +import 'package:flutter/foundation.dart' show visibleForTesting; +import '../utils/app_logger.dart'; import 'base_shared_preferences_service.dart'; /// Encrypts credentials before they are persisted in Drift config/token @@ -21,6 +23,12 @@ class CredentialVault { static final AesGcm _algorithm = AesGcm.with256bits(); static Future? _secretKey; + /// Drops the memoized key so tests can simulate key loss/divergence. + @visibleForTesting + static void resetKeyForTesting() { + _secretKey = null; + } + static bool isProtected(String? value) => value != null && value.startsWith(_prefix); static Future protect(String value) async { @@ -30,16 +38,26 @@ class CredentialVault { return '$_prefix${jsonEncode({'n': base64Encode(box.nonce), 'c': base64Encode(box.cipherText), 'm': base64Encode(box.mac.bytes)})}'; } - static Future reveal(String value) async { + /// Decrypts a protected value, or returns it unchanged when it isn't + /// protected. Returns null when decryption fails — a failed MAC check + /// (key/ciphertext divergence: restored backup, clobbered prefs, racing + /// key generation) or a corrupt payload means the credential is *lost*, + /// never a reason to crash; callers treat null as "re-acquire the token". + static Future reveal(String value) async { if (!isProtected(value)) return value; - final payload = jsonDecode(value.substring(_prefix.length)) as Map; - final box = SecretBox( - base64Decode(payload['c'] as String), - nonce: base64Decode(payload['n'] as String), - mac: Mac(base64Decode(payload['m'] as String)), - ); - final clear = await _algorithm.decrypt(box, secretKey: await _getSecretKey()); - return utf8.decode(clear); + try { + final payload = jsonDecode(value.substring(_prefix.length)) as Map; + final box = SecretBox( + base64Decode(payload['c'] as String), + nonce: base64Decode(payload['n'] as String), + mac: Mac(base64Decode(payload['m'] as String)), + ); + final clear = await _algorithm.decrypt(box, secretKey: await _getSecretKey()); + return utf8.decode(clear); + } catch (e) { + appLogger.w('CredentialVault: failed to decrypt stored credential, treating as lost', error: e); + return null; + } } static Future> protectConnectionConfig(String kind, Map config) async { @@ -70,8 +88,11 @@ class CredentialVault { var migrated = false; final token = tokenKey == null ? null : copy[tokenKey]; if (token is String && token.isNotEmpty) { - migrated = !isProtected(token); - copy[tokenKey!] = await reveal(token); + final revealed = await reveal(token); + // An undecryptable token becomes the empty string — the shared + // "no credential, re-auth" shape — and must not be rewritten back. + migrated = revealed != null && !isProtected(token); + copy[tokenKey!] = revealed ?? ''; } if (kind == 'plex') { final result = await _revealPlexServers(copy['servers']); @@ -109,8 +130,9 @@ class CredentialVault { final server = Map.from(raw); final token = server['accessToken']; if (token is String && token.isNotEmpty) { - migrated = migrated || !isProtected(token); - server['accessToken'] = await reveal(token); + final revealed = await reveal(token); + migrated = migrated || (revealed != null && !isProtected(token)); + server['accessToken'] = revealed ?? ''; } servers.add(server); } @@ -120,12 +142,31 @@ class CredentialVault { static Future _getSecretKey() { return _secretKey ??= () async { final prefs = await BaseSharedPreferencesService.sharedCache(); + // The cached snapshot can predate a key written by another isolate + // (background downloader, first-run migration); generating "fresh" over + // it would clobber the real key and orphan every stored ciphertext. + // Reload before deciding, and after writing re-read and adopt whatever + // actually landed so all isolates converge on a single key. + try { + await prefs.reloadCache(); + } catch (e) { + appLogger.d('CredentialVault: prefs reload before key check failed', error: e); + } final stored = prefs.getString(_keyPref); if (stored != null && stored.isNotEmpty) { return SecretKey(base64Decode(stored)); } final bytes = List.generate(32, (_) => Random.secure().nextInt(256)); 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); + } return SecretKey(bytes); }(); } diff --git a/test/services/credential_vault_test.dart b/test/services/credential_vault_test.dart new file mode 100644 index 00000000..7d79ae33 --- /dev/null +++ b/test/services/credential_vault_test.dart @@ -0,0 +1,80 @@ +import 'dart:convert'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/services/credential_vault.dart'; + +import '../test_helpers/prefs.dart'; + +void main() { + setUp(() { + resetSharedPreferencesForTest(); + CredentialVault.resetKeyForTesting(); + }); + + group('CredentialVault.reveal', () { + test('round-trips a protected value', () async { + final protected = await CredentialVault.protect('super-secret'); + expect(protected, startsWith('enc:v1:')); + expect(await CredentialVault.reveal(protected), 'super-secret'); + }); + + test('returns unprotected values unchanged', () async { + expect(await CredentialVault.reveal('plaintext-token'), 'plaintext-token'); + }); + + test('returns null instead of throwing on a tampered MAC', () async { + final protected = await CredentialVault.protect('super-secret'); + final payload = jsonDecode(protected.substring('enc:v1:'.length)) as Map; + payload['m'] = base64Encode(List.filled(16, 0)); + final tampered = 'enc:v1:${jsonEncode(payload)}'; + + expect(await CredentialVault.reveal(tampered), isNull); + }); + + test('returns null instead of throwing on a corrupt payload', () async { + expect(await CredentialVault.reveal('enc:v1:not-json'), isNull); + expect(await CredentialVault.reveal('enc:v1:{"n":"!!","c":"!!","m":"!!"}'), isNull); + expect(await CredentialVault.reveal('enc:v1:{"n":"AAAA"}'), isNull); + }); + + test('returns null when the key diverged from the ciphertext', () async { + final protected = await CredentialVault.protect('super-secret'); + + // Simulate the key being lost/regenerated (cleared prefs, clobbered by + // another isolate, restored backup) while the ciphertext survived. + resetSharedPreferencesForTest(); + CredentialVault.resetKeyForTesting(); + + expect(await CredentialVault.reveal(protected), isNull); + }); + }); + + group('CredentialVault.revealConnectionConfig', () { + test('maps an undecryptable account token to the empty string without migrating', () async { + final protected = await CredentialVault.protect('tok'); + resetSharedPreferencesForTest(); + CredentialVault.resetKeyForTesting(); + + final result = await CredentialVault.revealConnectionConfig('plex', { + 'accountToken': protected, + 'servers': [ + {'accessToken': protected}, + ], + }); + + expect(result.config['accountToken'], ''); + expect((result.config['servers'] as List).single['accessToken'], ''); + expect(result.migrated, isFalse); + }); + + test('still reveals and flags plaintext tokens for migration', () async { + final result = await CredentialVault.revealConnectionConfig('plex', { + 'accountToken': 'plain-tok', + 'servers': const [], + }); + + expect(result.config['accountToken'], 'plain-tok'); + expect(result.migrated, isTrue); + }); + }); +}