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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VgVDQQXAg4cmzsWHoJj61k
This commit is contained in:
Vitor Pamplona
2026-09-12 17:43:12 -04:00
co-authored by Claude Opus 5
parent 285a51e98f
commit 1ea820e699
2 changed files with 99 additions and 4 deletions
@@ -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
@@ -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<String, String>()
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