diff --git a/lib/core/storage/connection_secrets_store.dart b/lib/core/storage/connection_secrets_store.dart index 6424e33..cc85f79 100644 --- a/lib/core/storage/connection_secrets_store.dart +++ b/lib/core/storage/connection_secrets_store.dart @@ -36,12 +36,37 @@ class ConnectionSecretsStore { /// Production uses OS-backed storage; tests override [backend] (see `test/flutter_test_config.dart`). static SecretsStorageBackend backend = _FlutterSecureStorageBackend(); - static const _keyPrefix = 'querya.v1.conn'; + /// Identifies the current profile's local database (installed / portable / + /// migrated-legacy — see `AppDataRoot`), so that two independent profile + /// databases never collide in the shared OS keyring even when they each + /// mint the same connection id (every profile DB starts its own + /// `connections` autoincrement sequence at 1). [LocalDb] sets this once, + /// right after opening its database, before any secret is read or written. + static String? profileId; + + static const _legacyKeyPrefix = 'querya.v1.conn'; + + static String _requireProfileId() { + final id = profileId; + if (id == null) { + throw StateError( + 'ConnectionSecretsStore.profileId is not set; open LocalDb before ' + 'reading or writing connection secrets.', + ); + } + return id; + } static String _passwordKey(int connectionId) => - '$_keyPrefix.$connectionId.password'; + 'querya.v1.profile.${_requireProfileId()}.conn.$connectionId.password'; static String _connectionStringKey(int connectionId) => - '$_keyPrefix.$connectionId.connection_string'; + 'querya.v1.profile.${_requireProfileId()}.conn.$connectionId.connection_string'; + + /// Pre-#986 unnamespaced key format, kept only for [adoptLegacyKeysForConnection]. + static String _legacyPasswordKey(int connectionId) => + '$_legacyKeyPrefix.$connectionId.password'; + static String _legacyConnectionStringKey(int connectionId) => + '$_legacyKeyPrefix.$connectionId.connection_string'; static Future writeForConnection( int connectionId, { @@ -70,4 +95,41 @@ class ConnectionSecretsStore { await backend.delete(_passwordKey(connectionId)); await backend.delete(_connectionStringKey(connectionId)); } + + /// One-time migration (issue #986): before keyring keys were namespaced by + /// profile, every profile database shared the same `querya.v1.conn..*` + /// keys, so two profiles with a connection of the same id could overwrite + /// or delete each other's secret. Called by [LocalDb] for every existing + /// connection when a database upgrades to schema version 9; adopts this + /// profile's own legacy entry (if any) under the namespaced key, then + /// removes the legacy entry so a *different* profile no longer sees it as + /// "still shared". A no-op when no legacy entry exists for [connectionId]. + static Future adoptLegacyKeysForConnection(int connectionId) async { + String? legacyPassword; + String? legacyConnectionString; + try { + legacyPassword = await backend.read(_legacyPasswordKey(connectionId)); + legacyConnectionString = + await backend.read(_legacyConnectionStringKey(connectionId)); + } catch (_) { + return; + } + if (legacyPassword == null && legacyConnectionString == null) return; + try { + await writeForConnection( + connectionId, + password: legacyPassword, + connectionString: legacyConnectionString, + ); + } catch (_) { + return; + } + try { + await backend.delete(_legacyPasswordKey(connectionId)); + await backend.delete(_legacyConnectionStringKey(connectionId)); + } catch (_) { + // Best-effort cleanup; a leftover legacy key is harmless (it belongs to + // whichever profile adopts it first) but should not fail the upgrade. + } + } } diff --git a/lib/core/storage/local_db.dart b/lib/core/storage/local_db.dart index d34225c..6a05e44 100644 --- a/lib/core/storage/local_db.dart +++ b/lib/core/storage/local_db.dart @@ -1,4 +1,5 @@ import 'dart:io'; +import 'dart:math'; import 'package:flutter/foundation.dart'; import 'package:path/path.dart' as p; @@ -7,7 +8,17 @@ import 'package:querya_desktop/core/storage/connection_secrets_store.dart'; import 'package:sqflite_common_ffi/sqflite_ffi.dart'; const _dbName = 'querya.db'; -const _dbVersion = 8; +const _dbVersion = 9; + +/// `app_settings` key under which each profile database's random id is +/// stored (see [LocalDb._ensureProfileId] and issue #986). +const _profileIdSettingKey = 'profile_id'; + +String _generateProfileId() { + final rand = Random.secure(); + final bytes = List.generate(16, (_) => rand.nextInt(256)); + return bytes.map((b) => b.toRadixString(16).padLeft(2, '0')).join(); +} /// Fallback when [recordSqlQueryHistory] is called without `maxEntries`. /// Keep in sync with [kDefaultSqlHistoryMaxEntries] in `app_settings.dart`. @@ -80,9 +91,34 @@ class LocalDb { ); _db = db; _openFuture = null; + // Safety net for the common case (db already at the current version, so + // neither onCreate nor onUpgrade ran this open): _onCreate/_onUpgrade + // already set this when they do run, ahead of any secret read/write. + ConnectionSecretsStore.profileId ??= await _ensureProfileId(db); return db; } + /// Returns this profile database's random id (see issue #986), generating + /// and persisting one on first use. Namespaces [ConnectionSecretsStore] + /// keys so two profile databases never collide in the shared OS keyring. + Future _ensureProfileId(Database db) async { + final rows = await db.query( + 'app_settings', + columns: ['value'], + where: 'key = ?', + whereArgs: [_profileIdSettingKey], + limit: 1, + ); + final existing = rows.isNotEmpty ? rows.first['value'] as String? : null; + if (existing != null && existing.isNotEmpty) return existing; + final id = _generateProfileId(); + await db.rawInsert( + 'INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)', + [_profileIdSettingKey, id], + ); + return id; + } + /// Queries an active PRAGMA setting from the database for verification and diagnostic purposes. Future getPragma(String pragmaName) async { final db = await _open(); @@ -140,9 +176,21 @@ class LocalDb { CREATE INDEX idx_sql_query_history_lookup ON sql_query_history (connection_id, database_name, recorded_at DESC, id DESC) '''); + ConnectionSecretsStore.profileId = await _ensureProfileId(db); } Future _onUpgrade(Database db, int oldVersion, int newVersion) async { + // Every upgrade path needs app_settings before it can read/write the + // profile id; the oldVersion < 4 block below also creates this table for + // installs that predate it, but IF NOT EXISTS keeps this idempotent. + await db.execute(''' + CREATE TABLE IF NOT EXISTS app_settings ( + key TEXT PRIMARY KEY NOT NULL, + value TEXT NOT NULL + ) + '''); + ConnectionSecretsStore.profileId = await _ensureProfileId(db); + if (oldVersion < 2) { await db.execute('ALTER TABLE connections ADD COLUMN password TEXT'); await db.execute('ALTER TABLE connections ADD COLUMN database_name TEXT'); @@ -181,14 +229,8 @@ class LocalDb { await db.execute('DROP TABLE connections'); await db.execute('ALTER TABLE connections_new RENAME TO connections'); } - if (oldVersion < 4) { - await db.execute(''' - CREATE TABLE app_settings ( - key TEXT PRIMARY KEY NOT NULL, - value TEXT NOT NULL - ) - '''); - } + // (oldVersion < 4 used to CREATE TABLE app_settings here; the + // CREATE TABLE IF NOT EXISTS above now covers that install path too.) if (oldVersion < 5) { final rows = await db.query('connections'); for (final m in rows) { @@ -233,6 +275,18 @@ class LocalDb { ON sql_query_history (connection_id, database_name, recorded_at DESC, id DESC) '''); } + if (oldVersion < 9) { + // Adopt this profile's own pre-#986 unnamespaced keyring secrets (if + // any) under the namespaced key generated above, so this database's + // connections keep working after another profile with an overlapping + // connection id also upgrades. + final rows = await db.query('connections', columns: ['id']); + for (final m in rows) { + final id = _sqliteInt(m['id']); + if (id == null) continue; + await ConnectionSecretsStore.adoptLegacyKeysForConnection(id); + } + } } Future getAppSetting(String key) async { @@ -430,7 +484,8 @@ class LocalDb { /// the platform secure store to avoid IPC bottlenecks, Keychain lockups, and /// D-Bus timeouts during sidebar/startup population. Secrets are resolved /// on-demand when a connection is initiated. - Future> getConnections({bool hydrateSecrets = false}) async { + Future> getConnections( + {bool hydrateSecrets = false}) async { final db = await _open(); final rows = await db.query('connections', orderBy: 'sort_order ASC, name ASC'); @@ -451,7 +506,8 @@ class LocalDb { connectionString: secrets.connectionString ?? row.connectionString, ); } catch (e) { - debugPrint('LocalDb._hydrateConnection failed for connection ${row.id}: $e'); + debugPrint( + 'LocalDb._hydrateConnection failed for connection ${row.id}: $e'); return row; } } @@ -461,9 +517,11 @@ class LocalDb { _hydrateConnection(row); /// Retrieves a single connection by [id], optionally hydrating secrets. - Future getConnectionById(int id, {bool hydrateSecrets = false}) async { + Future getConnectionById(int id, + {bool hydrateSecrets = false}) async { final db = await _open(); - final rows = await db.query('connections', where: 'id = ?', whereArgs: [id]); + final rows = + await db.query('connections', where: 'id = ?', whereArgs: [id]); if (rows.isEmpty) return null; final row = ConnectionRow.fromMap(rows.first); if (!hydrateSecrets) return row; diff --git a/test/core/storage/local_db_profile_secrets_test.dart b/test/core/storage/local_db_profile_secrets_test.dart new file mode 100644 index 0000000..55e1828 --- /dev/null +++ b/test/core/storage/local_db_profile_secrets_test.dart @@ -0,0 +1,140 @@ +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:path_provider_platform_interface/path_provider_platform_interface.dart'; +import 'package:querya_desktop/core/storage/connection_secrets_store.dart'; +import 'package:querya_desktop/core/storage/local_db.dart'; + +import '../../memory_secrets_backend.dart'; + +class _FakePathProvider extends PathProviderPlatform { + _FakePathProvider(this._root); + final String _root; + + @override + Future getApplicationSupportPath() async => _root; + + @override + Future getTemporaryPath() async => _root; + + @override + Future getApplicationDocumentsPath() async => _root; + + @override + Future getApplicationCachePath() async => _root; + + @override + Future getLibraryPath() async => _root; + + @override + Future getExternalStoragePath() async => _root; + + @override + Future?> getExternalCachePaths() async => [_root]; + + @override + Future?> getExternalStoragePaths( + {StorageDirectory? type}) async => + [_root]; + + @override + Future getDownloadsPath() async => _root; +} + +/// Regression tests for issue #986: two independent profile databases (each +/// with its own `connections` autoincrement sequence starting at 1) must not +/// collide in the shared OS keyring when they mint the same connection id. +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late Directory rootA; + late Directory rootB; + + setUp(() async { + rootA = await Directory.systemTemp.createTemp('querya_profile_a_'); + rootB = await Directory.systemTemp.createTemp('querya_profile_b_'); + await LocalDb.initFfi(); + }); + + tearDown(() async { + await LocalDb.instance.close(); + ConnectionSecretsStore.profileId = null; + testMemorySecrets.clear(); + if (await rootA.exists()) await rootA.delete(recursive: true); + if (await rootB.exists()) await rootB.delete(recursive: true); + }); + + test( + 'two profile databases with the same connection id keep independent secrets', + () async { + // Profile A mints connection id 1 with a password. + PathProviderPlatform.instance = _FakePathProvider(rootA.path); + final idA = await LocalDb.instance.addConnection(const ConnectionRow( + type: 'postgres', + name: 'A1', + host: 'a-host', + port: 5432, + password: 'profile-a-secret', + createdAt: '2026-01-01T00:00:00Z', + )); + expect(idA, 1); + final profileIdA = ConnectionSecretsStore.profileId; + expect(profileIdA, isNotNull); + + await LocalDb.instance.close(); + ConnectionSecretsStore.profileId = null; + + // Profile B is a separate database that also mints connection id 1, but + // without a password. Saving it must not touch profile A's secret. + PathProviderPlatform.instance = _FakePathProvider(rootB.path); + final idB = await LocalDb.instance.addConnection(const ConnectionRow( + type: 'postgres', + name: 'B1', + host: 'b-host', + port: 5432, + createdAt: '2026-01-01T00:00:00Z', + )); + expect(idB, 1); + final profileIdB = ConnectionSecretsStore.profileId; + expect(profileIdB, isNotNull); + expect(profileIdB, isNot(profileIdA)); + + final loadedB = + (await LocalDb.instance.getConnections(hydrateSecrets: true)).single; + expect(loadedB.password, isNull); + + await LocalDb.instance.close(); + ConnectionSecretsStore.profileId = null; + + // Back in profile A, connection 1's password must still be intact. + PathProviderPlatform.instance = _FakePathProvider(rootA.path); + final loadedA = + (await LocalDb.instance.getConnections(hydrateSecrets: true)).single; + expect(loadedA.id, 1); + expect(loadedA.password, 'profile-a-secret'); + }); + + test('adoptLegacyKeysForConnection migrates a pre-#986 unnamespaced secret', + () async { + PathProviderPlatform.instance = _FakePathProvider(rootA.path); + final id = await LocalDb.instance.addConnection(const ConnectionRow( + type: 'redis', + name: 'Legacy', + host: 'legacy-host', + port: 6379, + createdAt: '2026-01-01T00:00:00Z', + )); + final profileId = ConnectionSecretsStore.profileId!; + + // Simulate a pre-#986 install: write directly under the old unnamespaced + // key format instead of the namespaced one. + await testMemorySecrets.write('querya.v1.conn.$id.password', 'legacy-pw'); + + await ConnectionSecretsStore.adoptLegacyKeysForConnection(id); + + final secrets = await ConnectionSecretsStore.readForConnection(id); + expect(secrets.password, 'legacy-pw'); + expect(await testMemorySecrets.read('querya.v1.conn.$id.password'), isNull); + expect(ConnectionSecretsStore.profileId, profileId); + }); +}