From fc6df7815762432d8cc48e4a6350d6758efd9932 Mon Sep 17 00:00:00 2001 From: ZhuchkaTriplesix Date: Fri, 10 Jul 2026 15:30:13 +0300 Subject: [PATCH] fix(storage): keep connection metadata and secrets consistent on OS store failures (#276) - removeConnection always deletes the SQLite row even if secure-store delete fails (e.g. missing key or unavailable libsecret). - addConnection rolls back the SQLite insert (and any partial secrets) when writing to the OS secure store fails. - updateConnection restores the previous SQLite row and previous secrets when a secure-store write fails, then rethrows the original error. - Extend the in-memory secrets backend with failNextWrite/failNextDelete hooks and cover the failure paths with unit tests. --- lib/core/storage/local_db.dart | 78 +++++++++++++++++--- test/core/storage/local_db_secrets_test.dart | 76 +++++++++++++++++++ test/memory_secrets_backend.dart | 22 +++++- 3 files changed, 163 insertions(+), 13 deletions(-) diff --git a/lib/core/storage/local_db.dart b/lib/core/storage/local_db.dart index 1daae918..588e33d3 100644 --- a/lib/core/storage/local_db.dart +++ b/lib/core/storage/local_db.dart @@ -359,23 +359,51 @@ class LocalDb { /// Inserts a row and returns the SQLite row id. /// Password and connection string are stored in the OS secure store, not in SQLite. + /// + /// If writing secrets fails, the SQLite row is rolled back and the error is + /// rethrown so callers can surface a Keychain / libsecret failure. Future addConnection(ConnectionRow row) async { final db = await _open(); final id = await db.insert('connections', row.toPersistenceMap()); - await ConnectionSecretsStore.writeForConnection( - id, - password: row.password, - connectionString: row.connectionString, - ); + try { + await ConnectionSecretsStore.writeForConnection( + id, + password: row.password, + connectionString: row.connectionString, + ); + } catch (e) { + try { + await ConnectionSecretsStore.deleteForConnection(id); + } catch (_) { + // Best-effort cleanup of any partial secret writes. + } + await db.delete('connections', where: 'id = ?', whereArgs: [id]); + rethrow; + } return id; } - /// Atomically updates an existing connection row in SQLite and its secrets in the secure store. + /// Updates an existing connection row in SQLite and its secrets in the secure store. + /// + /// If writing secrets fails, the previous SQLite row and previous secrets are + /// restored (best effort) and the error is rethrown. Future updateConnection(ConnectionRow row) async { if (row.id == null) { throw ArgumentError('ConnectionRow.id cannot be null when calling updateConnection'); } final db = await _open(); + final previousMaps = await db.query( + 'connections', + where: 'id = ?', + whereArgs: [row.id], + ); + if (previousMaps.isEmpty) { + throw ArgumentError('No connection found with id ${row.id}'); + } + final previousRow = ConnectionRow.fromMap(previousMaps.first); + final previousSecrets = + await ConnectionSecretsStore.readForConnection(row.id!); + await db.transaction((txn) async { final count = await txn.update( 'connections', @@ -387,15 +415,41 @@ class LocalDb { throw ArgumentError('No connection found with id ${row.id}'); } }); - await ConnectionSecretsStore.writeForConnection( - row.id!, - password: row.password, - connectionString: row.connectionString, - ); + + try { + await ConnectionSecretsStore.writeForConnection( + row.id!, + password: row.password, + connectionString: row.connectionString, + ); + } catch (e) { + await db.update( + 'connections', + previousRow.toPersistenceMap(), + where: 'id = ?', + whereArgs: [row.id], + ); + try { + await ConnectionSecretsStore.writeForConnection( + row.id!, + password: previousSecrets.password, + connectionString: previousSecrets.connectionString, + ); + } catch (_) { + // Best-effort restore of previous secrets; surface the original error. + } + rethrow; + } } + /// Deletes a connection. SQLite deletion always proceeds even if the secure + /// store delete fails (e.g. missing key or unavailable libsecret daemon). Future removeConnection(int id) async { - await ConnectionSecretsStore.deleteForConnection(id); + try { + await ConnectionSecretsStore.deleteForConnection(id); + } catch (_) { + // Do not block removing the connection metadata when the OS store fails. + } final db = await _open(); await db.delete('connections', where: 'id = ?', whereArgs: [id]); } diff --git a/test/core/storage/local_db_secrets_test.dart b/test/core/storage/local_db_secrets_test.dart index fdb76f02..51032fd4 100644 --- a/test/core/storage/local_db_secrets_test.dart +++ b/test/core/storage/local_db_secrets_test.dart @@ -155,5 +155,81 @@ void main() { expect(secrets.password, 'new-secret-password'); expect(secrets.connectionString, 'postgres://root:new-secret-password@db.example.com:5433/mydb'); }); + + test('removeConnection still deletes SQLite row when secure-store delete fails', () async { + const row = ConnectionRow( + type: 'redis', + name: 'R3', + host: '127.0.0.1', + port: 6379, + password: 'x', + createdAt: '2026-01-01T00:00:00Z', + ); + final id = await LocalDb.instance.addConnection(row); + testMemorySecrets.failNextDelete = StateError('libsecret unavailable'); + + await LocalDb.instance.removeConnection(id); + + final list = await LocalDb.instance.getConnections(); + expect(list.where((c) => c.id == id), isEmpty); + }); + + test('addConnection rolls back SQLite row when secure-store write fails', () async { + testMemorySecrets.failNextWrite = StateError('keychain write failed'); + const row = ConnectionRow( + type: 'redis', + name: 'R4', + host: '127.0.0.1', + port: 6379, + password: 'secret', + createdAt: '2026-01-01T00:00:00Z', + ); + + await expectLater( + LocalDb.instance.addConnection(row), + throwsA(isA()), + ); + + final list = await LocalDb.instance.getConnections(); + expect(list.where((c) => c.name == 'R4'), isEmpty); + }); + + test('updateConnection rolls back SQLite and secrets when secure-store write fails', () async { + const initialRow = ConnectionRow( + type: 'postgres', + name: 'PG_Before', + host: 'localhost', + port: 5432, + username: 'admin', + password: 'old-password', + createdAt: '2026-01-01T00:00:00Z', + ); + final id = await LocalDb.instance.addConnection(initialRow); + + testMemorySecrets.failNextWrite = StateError('keychain write failed'); + final updatedRow = ConnectionRow( + id: id, + type: 'postgres', + name: 'PG_After', + host: 'db.example.com', + port: 5433, + username: 'root', + password: 'new-password', + createdAt: '2026-01-01T00:00:00Z', + ); + + await expectLater( + LocalDb.instance.updateConnection(updatedRow), + throwsA(isA()), + ); + + final list = await LocalDb.instance.getConnections(); + final loaded = list.singleWhere((c) => c.id == id); + expect(loaded.name, 'PG_Before'); + expect(loaded.host, 'localhost'); + expect(loaded.port, 5432); + expect(loaded.username, 'admin'); + expect(loaded.password, 'old-password'); + }); }); } diff --git a/test/memory_secrets_backend.dart b/test/memory_secrets_backend.dart index 1eb251a1..28efca49 100644 --- a/test/memory_secrets_backend.dart +++ b/test/memory_secrets_backend.dart @@ -8,11 +8,22 @@ final MemorySecretsStorageBackend testMemorySecrets = class MemorySecretsStorageBackend implements SecretsStorageBackend { final Map _values = {}; + /// When non-null, the next [write] throws this error (then clears the flag). + Object? failNextWrite; + + /// When non-null, the next [delete] throws this error (then clears the flag). + Object? failNextDelete; + @override Future read(String key) async => _values[key]; @override Future write(String key, String? value) async { + final fail = failNextWrite; + if (fail != null) { + failNextWrite = null; + throw fail; + } if (value == null || value.isEmpty) { _values.remove(key); } else { @@ -22,8 +33,17 @@ class MemorySecretsStorageBackend implements SecretsStorageBackend { @override Future delete(String key) async { + final fail = failNextDelete; + if (fail != null) { + failNextDelete = null; + throw fail; + } _values.remove(key); } - void clear() => _values.clear(); + void clear() { + _values.clear(); + failNextWrite = null; + failNextDelete = null; + } }