From 4169d0a2cd7abc7a6334acbb89bb9a690299520c Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Sat, 10 Oct 2026 16:39:38 +0300 Subject: [PATCH] fix(storage): an unreadable keyring is an error, not an empty password (#1303) The secure-store reads swallowed every failure and returned nulls, so a locked keyring or a missing secret service was reported as Authentication failed, and a following edit could save the missing password as empty. - readForConnection / readSshSecretsForConnection throw SecretsStoreUnavailableException when the store does not answer; an absent secret is still null. - The drivers let it through instead of connecting without a password, and the error text names the keyring, not the credentials. - Editing a connection stops before any write when the saved secrets cannot be read, and says so. --- lib/core/database/database_error_mapper.dart | 11 ++ lib/core/database/mongodb_connection.dart | 10 +- lib/core/database/mysql_connection.dart | 10 +- lib/core/database/postgres_connection.dart | 10 +- lib/core/database/redis_connection.dart | 10 +- .../storage/connection_secrets_store.dart | 151 +++++++++++------- .../connections/connections_panel.dart | 17 +- .../secrets_unavailable_connect_test.dart | 68 ++++++++ test/core/storage/local_db_secrets_test.dart | 72 ++++++++- 9 files changed, 271 insertions(+), 88 deletions(-) create mode 100644 test/core/database/secrets_unavailable_connect_test.dart diff --git a/lib/core/database/database_error_mapper.dart b/lib/core/database/database_error_mapper.dart index a140e7a3..04a28a6a 100644 --- a/lib/core/database/database_error_mapper.dart +++ b/lib/core/database/database_error_mapper.dart @@ -7,6 +7,7 @@ import 'package:querya_desktop/core/database/querya_database_exception.dart'; import 'package:querya_desktop/core/database/redis_connection.dart'; import 'package:querya_desktop/core/database/sqlite_connection.dart'; import 'package:querya_desktop/core/database/statement_queue.dart'; +import 'package:querya_desktop/core/storage/connection_secrets_store.dart'; /// Which engine produced an error; only used to tailor the wording. enum DatabaseDriver { postgres, mysql, sqlite, mongodb, redis, extension } @@ -21,6 +22,16 @@ QueryaDatabaseException mapDatabaseError( DatabaseDriver? driver, }) { if (error is QueryaDatabaseException) return error; + if (error is SecretsStoreUnavailableException) { + // Not a wrong password: the saved one could not be read at all (#1303). + return UnknownDatabaseException( + SecretsStoreUnavailableException.message, + detailedExplanation: error.cause.toString(), + remediationHint: SecretsStoreUnavailableException.hint, + originalError: error, + stackTrace: stackTrace, + ); + } final raw = _fullText(error); final lower = raw.toLowerCase(); diff --git a/lib/core/database/mongodb_connection.dart b/lib/core/database/mongodb_connection.dart index a4ea644e..9442243d 100644 --- a/lib/core/database/mongodb_connection.dart +++ b/lib/core/database/mongodb_connection.dart @@ -223,11 +223,11 @@ class MongoConnection { if ((effectivePassword == null || effectivePassword.isEmpty) && (effectiveConnectionString == null || effectiveConnectionString.isEmpty) && id > 0) { - try { - final secrets = await ConnectionSecretsStore.readForConnection(id); - effectivePassword = secrets.password; - effectiveConnectionString = secrets.connectionString; - } catch (_) {} + // A store that cannot be read throws (#1303): connecting with no password + // would be reported as a wrong one. + final secrets = await ConnectionSecretsStore.readForConnection(id); + effectivePassword = secrets.password; + effectiveConnectionString = secrets.connectionString; } try { diff --git a/lib/core/database/mysql_connection.dart b/lib/core/database/mysql_connection.dart index 062d6656..131204fa 100644 --- a/lib/core/database/mysql_connection.dart +++ b/lib/core/database/mysql_connection.dart @@ -206,11 +206,11 @@ class MysqlConnection { (effectiveConnectionString == null || effectiveConnectionString.isEmpty) && id > 0) { - try { - final secrets = await ConnectionSecretsStore.readForConnection(id); - effectivePassword = secrets.password; - effectiveConnectionString = secrets.connectionString; - } catch (_) {} + // A store that cannot be read throws (#1303): connecting with no password + // would be reported as a wrong one. + final secrets = await ConnectionSecretsStore.readForConnection(id); + effectivePassword = secrets.password; + effectiveConnectionString = secrets.connectionString; } try { diff --git a/lib/core/database/postgres_connection.dart b/lib/core/database/postgres_connection.dart index 077d2c50..b2e49f45 100644 --- a/lib/core/database/postgres_connection.dart +++ b/lib/core/database/postgres_connection.dart @@ -237,11 +237,11 @@ class PostgresConnection { if ((effectivePassword == null || effectivePassword.isEmpty) && (effectiveConnectionString == null || effectiveConnectionString.isEmpty) && id > 0) { - try { - final secrets = await ConnectionSecretsStore.readForConnection(id); - effectivePassword = secrets.password; - effectiveConnectionString = secrets.connectionString; - } catch (_) {} + // A store that cannot be read throws (#1303): connecting with no password + // would be reported as a wrong one. + final secrets = await ConnectionSecretsStore.readForConnection(id); + effectivePassword = secrets.password; + effectiveConnectionString = secrets.connectionString; } try { diff --git a/lib/core/database/redis_connection.dart b/lib/core/database/redis_connection.dart index c1366e1a..2ce98fe7 100644 --- a/lib/core/database/redis_connection.dart +++ b/lib/core/database/redis_connection.dart @@ -132,11 +132,11 @@ class RedisConnection { (effectiveConnectionString == null || effectiveConnectionString.isEmpty) && id > 0) { - try { - final secrets = await ConnectionSecretsStore.readForConnection(id); - effectivePassword = secrets.password; - effectiveConnectionString = secrets.connectionString; - } catch (_) {} + // A store that cannot be read throws (#1303): connecting with no password + // would be reported as a wrong one. + final secrets = await ConnectionSecretsStore.readForConnection(id); + effectivePassword = secrets.password; + effectiveConnectionString = secrets.connectionString; } _conn = redis.RedisConnection(); diff --git a/lib/core/storage/connection_secrets_store.dart b/lib/core/storage/connection_secrets_store.dart index 07f00e7c..363eb254 100644 --- a/lib/core/storage/connection_secrets_store.dart +++ b/lib/core/storage/connection_secrets_store.dart @@ -28,6 +28,30 @@ class _FlutterSecureStorageBackend implements SecretsStorageBackend { Future delete(String key) => _storage.delete(key: key); } +/// The system secret store (Keychain / Credential Manager / libsecret) did not +/// answer: it is locked, has no running secret service, or failed. +/// +/// Distinct from "there is no such secret", which reads as null: a password +/// that could not be read must not be taken for a connection without one +/// (#1303). +class SecretsStoreUnavailableException implements Exception { + SecretsStoreUnavailableException(this.cause); + + /// What the platform store threw. + final Object cause; + + static const String message = + 'Could not read the saved credentials from the system keyring ' + '(Keychain / Credential Manager / libsecret).'; + + /// What to do about it, for single-line surfaces. + static const String hint = + 'Unlock the keyring or start the secret service, then try again'; + + @override + String toString() => '$message $hint ($cause)'; +} + /// Stores connection passwords and connection strings outside of the SQLite file /// using the platform secure store (Keychain / Credential Manager / libsecret). class ConnectionSecretsStore { @@ -117,19 +141,13 @@ class ConnectionSecretsStore { String? passphrase, String? jumpPassword, })> readSshSecretsForConnection(int connectionId) async { + final String passwordKey, privateKeyKey, passphraseKey, jumpKey; try { - final password = await backend.read(_sshPasswordKey(connectionId)); - final privateKey = await backend.read(_sshPrivateKeyKey(connectionId)); - final passphrase = await backend.read(_sshPassphraseKey(connectionId)); - final jumpPassword = - await backend.read(_sshJumpPasswordKey(connectionId)); - return ( - password: password, - privateKey: privateKey, - passphrase: passphrase, - jumpPassword: jumpPassword, - ); - } catch (_) { + passwordKey = _sshPasswordKey(connectionId); + privateKeyKey = _sshPrivateKeyKey(connectionId); + passphraseKey = _sshPassphraseKey(connectionId); + jumpKey = _sshJumpPasswordKey(connectionId); + } on StateError { return ( password: null, privateKey: null, @@ -137,64 +155,81 @@ class ConnectionSecretsStore { jumpPassword: null, ); } + try { + return ( + password: await backend.read(passwordKey), + privateKey: await backend.read(privateKeyKey), + passphrase: await backend.read(passphraseKey), + jumpPassword: await backend.read(jumpKey), + ); + } catch (e) { + throw SecretsStoreUnavailableException(e); + } } static Future<({String? password, String? connectionString})> readForConnection( int connectionId, ) async { + final String passwordKey, connectionStringKey; try { - var password = await backend.read(_passwordKey(connectionId)); - var connectionString = - await backend.read(_connectionStringKey(connectionId)); - - // Fallback: If either secret wasn't found under the namespaced key, - // lazily check the legacy unnamespaced key (#1008). This guards against - // transient keyring failures during the one-time DB upgrade migration, - // and writes through to adopt the legacy secret immediately. - if (password == null || connectionString == null) { - try { - String? legacyPassword; - String? legacyConnectionString; - if (password == null) { - legacyPassword = - await backend.read(_legacyPasswordKey(connectionId)); - } - if (connectionString == null) { - legacyConnectionString = - await backend.read(_legacyConnectionStringKey(connectionId)); + passwordKey = _passwordKey(connectionId); + connectionStringKey = _connectionStringKey(connectionId); + } on StateError { + // No profile yet (nothing was ever stored): same as no secrets. + return (password: null, connectionString: null); + } + String? password; + String? connectionString; + try { + password = await backend.read(passwordKey); + connectionString = await backend.read(connectionStringKey); + } catch (e) { + throw SecretsStoreUnavailableException(e); + } + // Fallback: If either secret wasn't found under the namespaced key, + // lazily check the legacy unnamespaced key (#1008). This guards against + // transient keyring failures during the one-time DB upgrade migration, + // and writes through to adopt the legacy secret immediately. + if (password == null || connectionString == null) { + try { + String? legacyPassword; + String? legacyConnectionString; + if (password == null) { + legacyPassword = + await backend.read(_legacyPasswordKey(connectionId)); + } + if (connectionString == null) { + legacyConnectionString = + await backend.read(_legacyConnectionStringKey(connectionId)); + } + if (legacyPassword != null || legacyConnectionString != null) { + final effectivePassword = password ?? legacyPassword; + final effectiveConnectionString = + connectionString ?? legacyConnectionString; + await writeForConnection( + connectionId, + password: effectivePassword, + connectionString: effectiveConnectionString, + ); + if (legacyPassword != null) { + try { + await backend.delete(_legacyPasswordKey(connectionId)); + } catch (_) {} } - if (legacyPassword != null || legacyConnectionString != null) { - final effectivePassword = password ?? legacyPassword; - final effectiveConnectionString = - connectionString ?? legacyConnectionString; - await writeForConnection( - connectionId, - password: effectivePassword, - connectionString: effectiveConnectionString, - ); - if (legacyPassword != null) { - try { - await backend.delete(_legacyPasswordKey(connectionId)); - } catch (_) {} - } - if (legacyConnectionString != null) { - try { - await backend.delete(_legacyConnectionStringKey(connectionId)); - } catch (_) {} - } - password = effectivePassword; - connectionString = effectiveConnectionString; + if (legacyConnectionString != null) { + try { + await backend.delete(_legacyConnectionStringKey(connectionId)); + } catch (_) {} } - } catch (_) { - // Best-effort lazy adoption; return whatever was already read. + password = effectivePassword; + connectionString = effectiveConnectionString; } + } catch (_) { + // Best-effort lazy adoption; return whatever was already read. } - - return (password: password, connectionString: connectionString); - } catch (_) { - return (password: null, connectionString: null); } + return (password: password, connectionString: connectionString); } static Future deleteForConnection(int connectionId) async { diff --git a/lib/features/connections/connections_panel.dart b/lib/features/connections/connections_panel.dart index 04bc57a6..1bcc9a0c 100644 --- a/lib/features/connections/connections_panel.dart +++ b/lib/features/connections/connections_panel.dart @@ -79,6 +79,7 @@ import 'package:querya_desktop/core/extensions/extension_driver_session.dart'; import 'package:querya_desktop/core/extensions/local_extension_registry.dart'; import 'package:querya_desktop/features/connections/sdui_tree_builder.dart'; import 'package:querya_desktop/core/sdui/sdui_tree_schema.dart'; +import 'package:querya_desktop/core/storage/connection_secrets_store.dart'; import 'package:querya_desktop/core/storage/folders_storage.dart'; import 'package:querya_desktop/core/storage/local_db.dart'; import 'package:querya_desktop/core/theme/querya_typography.dart'; @@ -719,8 +720,20 @@ class ConnectionsPanelState extends State { Future _editConnection(ConnectionRow conn) async { final edited = await promptEditConnection(context, conn); if (edited == null || !mounted) return; - final toSave = await mergeSecretsForConnectionUpdate(edited); - await LocalDb.instance.updateConnection(toSave); + try { + final toSave = await mergeSecretsForConnectionUpdate(edited); + await LocalDb.instance.updateConnection(toSave); + } on SecretsStoreUnavailableException catch (e) { + // Saving would write the secrets it could not read as empty ones. + if (mounted) { + showAppToast( + context: context, + variant: AppToastVariant.error, + message: 'The connection was not saved. ${e.toString()}', + ); + } + return; + } await _loadData(); if (!mounted) return; ConnectionRow? updated; diff --git a/test/core/database/secrets_unavailable_connect_test.dart b/test/core/database/secrets_unavailable_connect_test.dart new file mode 100644 index 00000000..e34c0f5c --- /dev/null +++ b/test/core/database/secrets_unavailable_connect_test.dart @@ -0,0 +1,68 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/core/database/database_error_mapper.dart'; +import 'package:querya_desktop/core/database/mongodb_connection.dart'; +import 'package:querya_desktop/core/database/mysql_connection.dart'; +import 'package:querya_desktop/core/database/postgres_connection.dart'; +import 'package:querya_desktop/core/database/querya_database_exception.dart'; +import 'package:querya_desktop/core/database/redis_connection.dart'; +import 'package:querya_desktop/core/storage/connection_secrets_store.dart'; + +import '../../memory_secrets_backend.dart'; + +/// #1303: a keyring that cannot be read is not "no password". +void main() { + setUp(() { + ConnectionSecretsStore.profileId = 'test-profile'; + testMemorySecrets.clear(); + }); + + final unreadable = isA(); + + group('connect with a saved connection whose secrets cannot be read', () { + test('PostgreSQL', () async { + final c = PostgresConnection( + id: 3, name: 'pg', host: 'localhost', port: 5432); + testMemorySecrets.failNextRead = Exception('keyring is locked'); + await expectLater(c.connect(), throwsA(unreadable)); + expect(c.isConnected, isFalse); + }); + + test('MySQL', () async { + final c = MysqlConnection( + id: 3, name: 'my', host: 'localhost', port: 3306); + testMemorySecrets.failNextRead = Exception('keyring is locked'); + await expectLater(c.connect(), throwsA(unreadable)); + }); + + test('Redis', () async { + final c = RedisConnection(id: 3, name: 'r', host: 'localhost'); + testMemorySecrets.failNextRead = Exception('keyring is locked'); + await expectLater(c.connect(), throwsA(unreadable)); + }); + + test('MongoDB', () async { + final c = MongoConnection( + id: 3, name: 'mo', host: 'localhost', port: 27017); + testMemorySecrets.failNextRead = Exception('keyring is locked'); + await expectLater(c.connect(), throwsA(unreadable)); + }); + }); + + group('mapDatabaseError', () { + test('an unreadable store is not an authentication failure', () { + // Even when the platform's own text sounds like one. + final mapped = mapDatabaseError(SecretsStoreUnavailableException( + Exception('libsecret: authentication failed'))); + expect(mapped, isNot(isA())); + expect(mapped.message, contains('system keyring')); + expect(mapped.remediationHint, contains('Unlock the keyring')); + }); + + test('its one-line text names the keyring, not the credentials', () { + final text = describeDatabaseError(SecretsStoreUnavailableException( + Exception('The collection is locked'))); + expect(text, contains('keyring')); + expect(text.toLowerCase(), isNot(contains('check the username'))); + }); + }); +} diff --git a/test/core/storage/local_db_secrets_test.dart b/test/core/storage/local_db_secrets_test.dart index 1103bbf9..a520deba 100644 --- a/test/core/storage/local_db_secrets_test.dart +++ b/test/core/storage/local_db_secrets_test.dart @@ -367,7 +367,7 @@ void main() { }); test( - 'readForConnection and _hydrateConnection handle Keychain error gracefully without throwing', + 'a store that cannot be read throws, and hydrating a list survives it', () async { const row = ConnectionRow( type: 'mysql', @@ -380,20 +380,76 @@ void main() { ); final id = await LocalDb.instance.addConnection(row); - testMemorySecrets.failNextRead = StateError('org.freedesktop.DBus.Error.NoReply'); + testMemorySecrets.failNextRead = + Exception('org.freedesktop.DBus.Error.NoReply'); - // readForConnection should return nulls instead of throwing - final secrets = await ConnectionSecretsStore.readForConnection(id); - expect(secrets.password, isNull); - expect(secrets.connectionString, isNull); + // Not "no password": the caller must be able to tell (#1303). + await expectLater(ConnectionSecretsStore.readForConnection(id), + throwsA(isA())); - testMemorySecrets.failNextRead = StateError('Keychain locked'); + testMemorySecrets.failNextRead = Exception('Keychain locked'); + await expectLater(ConnectionSecretsStore.readSshSecretsForConnection(id), + throwsA(isA())); - // getConnections(hydrateSecrets: true) should still succeed and return the connection row + // A list still loads: that row is just without its password. + testMemorySecrets.failNextRead = Exception('Keychain locked'); final list = await LocalDb.instance.getConnections(hydrateSecrets: true); final conn = list.singleWhere((c) => c.id == id); expect(conn.name, 'MySQL_Faulty'); expect(conn.password, isNull); + + // And the secret is still there once the store answers again. + final secrets = await ConnectionSecretsStore.readForConnection(id); + expect(secrets.password, 'secret-password'); + }); + + test('an absent secret reads as null, not as an error', () async { + final secrets = await ConnectionSecretsStore.readForConnection(987654); + expect(secrets.password, isNull); + expect(secrets.connectionString, isNull); + final ssh = await ConnectionSecretsStore.readSshSecretsForConnection( + 987654); + expect(ssh.password, isNull); + }); + + test( + 'an edit while the store cannot be read changes nothing and keeps ' + 'the saved password', () async { + const row = ConnectionRow( + type: 'postgresql', + name: 'Before', + host: 'localhost', + port: 5432, + username: 'admin', + password: 'keep-me', + createdAt: '2026-01-01T00:00:00Z', + ); + final id = await LocalDb.instance.addConnection(row); + final edited = ConnectionRow( + id: id, + type: 'postgresql', + name: 'After', + host: 'localhost', + port: 5432, + username: 'admin', + createdAt: '2026-01-01T00:00:00Z', + ); + + // The merge reads the saved password first and stops there. + testMemorySecrets.failNextRead = Exception('Keychain locked'); + await expectLater(mergeSecretsForConnectionUpdate(edited), + throwsA(isA())); + + // Writing the unmerged row would erase it: updateConnection refuses to + // run on a store it cannot read. + testMemorySecrets.failNextRead = Exception('Keychain locked'); + await expectLater(LocalDb.instance.updateConnection(edited), + throwsA(isA())); + + final loaded = (await LocalDb.instance.getConnections(hydrateSecrets: true)) + .singleWhere((c) => c.id == id); + expect(loaded.name, 'Before'); + expect(loaded.password, 'keep-me'); }); }); }