Skip to content

bug(secrets): legacy keyring migration (#986) can silently and permanently strand passwords #1008

Description

@ZhuchkaTriplesix

Summary

Issue #986's fix namespaces ConnectionSecretsStore keys per-profile and migrates each connection's pre-#986 unnamespaced keyring entry to the new namespaced key, via LocalDb._onUpgrade's oldVersion < 9 block:

if (oldVersion < 9) {
  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);
  }
}

This runs exactly once, as part of the one-time _dbVersion bump from 8 to 9. adoptLegacyKeysForConnection (lib/core/storage/connection_secrets_store.dart:89) swallows every keyring error:

try {
  legacyPassword = await backend.read(_legacyPasswordKey(connectionId));
  legacyConnectionString = await backend.read(_legacyConnectionStringKey(connectionId));
} catch (_) {
  return;
}

Impact

If the OS secure-storage backend is transiently unavailable at the moment of this one-time migration — e.g. a locked GNOME Keyring / KWallet, a D-Bus timeout, a Keychain prompt the user doesn't answer in time — the migration silently no-ops for that connection. Since the schema version has already been bumped to 9 in the same _onUpgrade call, this migration never runs again: the connection's password is permanently stranded under the old unnamespaced key, and readForConnection (which only reads the new namespaced key) will return null for it from then on, i.e. the user has to re-enter the password for that connection.

readForConnection itself also swallows errors into "no secret":

static Future<...> readForConnection(int connectionId) async {
  try {
    ...
  } catch (_) {
    return (password: null, connectionString: null);
  }
}

so a transient keyring failure at read time (not just at migration time) is indistinguishable from "this connection genuinely has no saved password."

Suggested fix

Make the migration lazy/retriable instead of one-shot:

  • In readForConnection, if the namespaced key comes back empty, fall back to reading the legacy key and adopt it right then (write-through), rather than relying solely on the one-time schema migration.
  • Or: track migration success per-connection (not just via the global schema version) so a failed migration is retried on next launch.

Found during a code-quality review of the dev branch (the #986 / schema v9 changes), not from a reported user incident — flagging because the failure mode (permanent, silent, one-shot) is a plausible way to lose saved passwords on upgrade for anyone with keyring hiccups at exactly the wrong moment.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingconnectionsDatabase connections, URI parsing, poolscoreCore library logic and services

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions