From 1ea820e699543db083c44c97432ddb3d10c22db7 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 12 Sep 2026 17:43:12 -0400 Subject: [PATCH] fix(desktop): allow first-launch key bootstrap and stop caching unwritten state Two defects found reviewing the strict-keychain fix. 1. Fresh Linux/Windows installs could never persist an account. getPrivateKeyOrThrow turns any PasswordAccessException into a SecureStorageException on non-macOS backends, but every backend java-keyring ships throws that exact exception for a *genuinely absent* credential: - WinCredentialStoreBackend: CredReadA false (ERROR_NOT_FOUND) -> throw - FreedesktopKeyringBackend: empty object paths -> throwNoExistingCredentialException - KWalletBackend: hasEntry false -> "Password is not in wallet" So the strict lookup structurally cannot report "definitively absent" there, the create branch in getOrCreateKey was unreachable, and nothing between it and AccountManager.addAccountToStorage catches the throw. getOrCreateKey now bootstraps a fresh key when the strict lookup fails *and* accounts.json.enc does not exist. With no ciphertext on disk there is nothing a new key can orphan, so the invariant the strict contract protects is untouched: once the file exists the exception propagates exactly as before. 2. writeCachedMetadata updated the in-memory cache before the disk write, so a failed write (keychain refusal, I/O error, disk full) left the session serving accounts that were never persisted -- a save that reported success and vanished on the next launch. Persist first, cache second. Both are pinned by new tests, and both were mutation-checked: reverting either fix fails exactly one of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VgVDQQXAg4cmzsWHoJj61k --- .../desktop/account/DesktopAccountStorage.kt | 42 +++++++++++-- .../account/DesktopAccountStorageTest.kt | 61 +++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt index 9ef274a2ad..e1813ff197 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.desktop.account import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper import com.fasterxml.jackson.module.kotlin.readValue import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage +import com.vitorpamplona.amethyst.commons.keystorage.SecureStorageException import com.vitorpamplona.amethyst.commons.model.account.AccountInfo import com.vitorpamplona.amethyst.commons.model.account.AccountStorage import com.vitorpamplona.amethyst.commons.model.account.SignerType @@ -145,9 +146,17 @@ class DesktopAccountStorage( return loaded } + /** + * Persists first, caches second. + * + * If the disk write fails (keychain refused, I/O error, disk full) the in-memory + * cache must NOT be left claiming a state that was never written: the rest of the + * session would serve accounts that vanish on the next launch, and the user would + * see a successful save that silently did nothing. + */ private suspend fun writeCachedMetadata(metadata: AccountMetadata) { - cachedMetadata = metadata writeMetadataToDisk(metadata) + cachedMetadata = metadata } // --- Encrypted file I/O --- @@ -280,7 +289,9 @@ class DesktopAccountStorage( * - key exists in keychain: use it * - keychain confirms definitively absent: generate + persist a fresh key * - any other outcome (user cancelled/denied prompt, keychain locked, - * backend transient error): propagate the exception, do NOT rotate. + * backend transient error): propagate the exception, do NOT rotate -- + * unless there is no accounts.json.enc yet, in which case there is no + * ciphertext to orphan and we bootstrap a fresh key (see below). * * Rotating the AES key on an ambiguous miss silently destroys the ability * to decrypt the existing accounts.json.enc, wiping the logged-in accounts @@ -289,14 +300,37 @@ class DesktopAccountStorage( private suspend fun getOrCreateKey(): ByteArray { cachedKey?.let { return it } - val existing = secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS) + val existing = + try { + secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS) + } catch (e: SecureStorageException) { + // Bootstrap escape. Every non-macOS backend java-keyring ships + // (Windows Credential Store, Freedesktop Secret Service, KWallet) + // throws PasswordAccessException for a *genuinely absent* credential, + // so the strict lookup structurally cannot report "definitively + // absent" there. Without this branch a fresh Linux/Windows install + // could never mint the key and could never persist an account. + // + // Minting is only safe while there is no accounts.json.enc: with no + // ciphertext on disk there is nothing a new key can orphan. Once the + // file exists the strict contract applies and we propagate. + if (getAccountsFile().exists()) throw e + Log.w( + "DesktopAccountStorage", + "Keychain lookup failed and no accounts file exists; bootstrapping a fresh metadata key", + e, + ) + null + } + if (existing != null) { val key = Base64.getDecoder().decode(existing) cachedKey = key return key } - // Definitively absent: safe to create and persist a fresh key. + // Definitively absent (or bootstrapping with nothing on disk): safe to + // create and persist a fresh key. val key = ByteArray(AES_KEY_SIZE).also { SecureRandom().nextBytes(it) } secureStorage.savePrivateKey(METADATA_KEY_ALIAS, Base64.getEncoder().encodeToString(key)) cachedKey = key diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt index c1c4a88993..a4d2e9a260 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorageTest.kt @@ -265,6 +265,67 @@ class DesktopAccountStorageTest { } } + @Test + fun `getOrCreateKey ambiguous error with no accounts file bootstraps a fresh key`() = + runTest { + // Every non-macOS backend java-keyring ships (Windows Credential Store, + // Freedesktop Secret Service, KWallet) throws PasswordAccessException for a + // *genuinely absent* credential, which the strict lookup surfaces as + // SecureStorageException. With no accounts.json.enc there is no ciphertext + // a new key could orphan, so a fresh install must still be able to mint one + // -- otherwise Linux/Windows can never persist an account at all. + val saved = mutableMapOf() + val throwingStorage: SecureKeyStorage = mockk() + coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws + SecureStorageException("Keyring backend refused access or returned ambiguous not-found") + coEvery { throwingStorage.savePrivateKey(any(), any()) } answers { + saved[firstArg()] = secondArg() + } + coEvery { throwingStorage.getPrivateKey(any()) } answers { saved[firstArg()] } + coEvery { throwingStorage.hasPrivateKey(any()) } answers { saved.containsKey(firstArg()) } + + val file = File(File(tempDir, ".amethyst"), "accounts.json.enc") + assertFalse(file.exists()) + + val fresh = DesktopAccountStorage(throwingStorage, tempDir) + fresh.saveAccount(AccountInfo("npub1freshinstall", SignerType.Internal)) + + assertNotNull(saved["account-metadata-key"]) + assertTrue(file.exists()) + assertEquals(listOf("npub1freshinstall"), fresh.loadAccounts().map { it.npub }) + // The escape is bootstrap-only: once the file exists the strict contract + // applies again -- pinned by `getOrCreateKey keyring throws ambiguous error + // does not rotate key or touch file` above. + } + + // --- Cache must never claim a state that was not persisted --- + + @Test + fun `failed disk write does not poison the in-memory cache`() = + runTest { + storage.saveAccount(AccountInfo("npub1persisted", SignerType.Internal)) + assertEquals(listOf("npub1persisted"), storage.loadAccounts().map { it.npub }) + + // Block the atomic-write temp path so writeMetadataToDisk fails. + val temp = File(File(tempDir, ".amethyst"), "accounts.json.enc.tmp") + assertTrue(temp.mkdirs()) + + assertFails { + runBlocking { + storage.saveAccount(AccountInfo("npub1phantom", SignerType.Internal)) + } + } + + // Same instance: the cache must still reflect only what reached the disk, + // not the account the failed save handed it. + assertEquals(listOf("npub1persisted"), storage.loadAccounts().map { it.npub }) + + // And the on-disk file agrees. + temp.delete() + val relaunched = DesktopAccountStorage(secureStorage, tempDir) + assertEquals(listOf("npub1persisted"), relaunched.loadAccounts().map { it.npub }) + } + // --- Bug 2: read failure must not silently reset the file --- @Test