Merge pull request #2908 from nrobi144/fix/account-security-hardening
fix(desktop): Account security hardening — NWC to keychain, single source of truth
This commit is contained in:
@@ -887,24 +887,20 @@ fun App(
|
||||
// Load account list from encrypted storage
|
||||
accountManager.refreshAccountListOnStartup()
|
||||
|
||||
if (accountManager.hasBunkerAccount()) {
|
||||
// Show connecting UI while dedicated NIP-46 client connects
|
||||
accountManager.setConnectingRelays()
|
||||
}
|
||||
val result = accountManager.loadSavedAccount()
|
||||
if (result.isSuccess) {
|
||||
// Ensure loaded account is in multi-account storage
|
||||
accountManager.ensureCurrentAccountInStorage()
|
||||
accountManager.refreshAccountList()
|
||||
|
||||
val current = accountManager.currentAccount()
|
||||
if (current?.signerType is com.vitorpamplona.amethyst.commons.model.account.SignerType.Remote) {
|
||||
accountManager.startHeartbeat(scope)
|
||||
}
|
||||
} else if (accountManager.hasBunkerAccount()) {
|
||||
// Corrupt bunker state — fall back to login screen
|
||||
accountManager.logout(deleteKey = true)
|
||||
// Load per-account NWC
|
||||
if (current != null) {
|
||||
accountManager.loadNwcConnection(current.npub)
|
||||
}
|
||||
}
|
||||
// If failure: state remains LoggedOut → login screen shows automatically
|
||||
}
|
||||
|
||||
onDispose {
|
||||
@@ -965,18 +961,14 @@ fun App(
|
||||
val account = accountState as AccountState.LoggedIn
|
||||
val nwcConnection by accountManager.nwcConnection.collectAsState()
|
||||
|
||||
// Lazy-load Namecoin services — almost never used, no need to keep in
|
||||
// memory from the start (matches Android lazy pattern)
|
||||
// Lazy-load Namecoin services
|
||||
val namecoinPreferences = remember { DesktopNamecoinPreferences() }
|
||||
val namecoinService =
|
||||
remember {
|
||||
DesktopNamecoinNameService(preferencesProvider = { namecoinPreferences.current })
|
||||
}
|
||||
|
||||
// Load NWC connection on first composition
|
||||
LaunchedEffect(Unit) {
|
||||
accountManager.loadNwcConnection()
|
||||
}
|
||||
// NWC loaded during startup in loadSavedAccount flow
|
||||
|
||||
val currentTorStatus = torManager.status.collectAsState().value
|
||||
androidx.compose.runtime.CompositionLocalProvider(
|
||||
@@ -1498,10 +1490,7 @@ fun RelaySettingsScreen(
|
||||
var nwcInput by remember { mutableStateOf("") }
|
||||
var nwcError by remember { mutableStateOf<String?>(null) }
|
||||
|
||||
// Load NWC on first composition
|
||||
LaunchedEffect(Unit) {
|
||||
accountManager.loadNwcConnection()
|
||||
}
|
||||
val nwcScope = rememberCoroutineScope()
|
||||
|
||||
com.vitorpamplona.amethyst.desktop.ui.ReadingColumn {
|
||||
val sidePadding =
|
||||
@@ -1567,7 +1556,7 @@ fun RelaySettingsScreen(
|
||||
)
|
||||
}
|
||||
OutlinedButton(
|
||||
onClick = { accountManager.clearNwcConnection() },
|
||||
onClick = { nwcScope.launch { accountManager.clearNwcConnection(account.npub) } },
|
||||
colors =
|
||||
ButtonDefaults.outlinedButtonColors(
|
||||
contentColor = MaterialTheme.colorScheme.error,
|
||||
@@ -1597,11 +1586,13 @@ fun RelaySettingsScreen(
|
||||
)
|
||||
Button(
|
||||
onClick = {
|
||||
val result = accountManager.setNwcConnection(nwcInput)
|
||||
result.fold(
|
||||
onSuccess = { nwcInput = "" },
|
||||
onFailure = { nwcError = it.message ?: "Invalid connection string" },
|
||||
)
|
||||
nwcScope.launch {
|
||||
val result = accountManager.setNwcConnection(account.npub, nwcInput)
|
||||
result.fold(
|
||||
onSuccess = { nwcInput = "" },
|
||||
onFailure = { nwcError = it.message ?: "Invalid connection string" },
|
||||
)
|
||||
}
|
||||
},
|
||||
enabled = nwcInput.isNotBlank(),
|
||||
) {
|
||||
|
||||
+90
-98
@@ -67,8 +67,6 @@ import kotlinx.coroutines.sync.Mutex
|
||||
import kotlinx.coroutines.sync.withLock
|
||||
import kotlinx.coroutines.withTimeout
|
||||
import java.io.File
|
||||
import java.nio.file.Files
|
||||
import java.nio.file.attribute.PosixFilePermission
|
||||
|
||||
sealed class AccountState {
|
||||
data object LoggedOut : AccountState()
|
||||
@@ -111,8 +109,15 @@ class AccountManager internal constructor(
|
||||
File(homeDir, ".amethyst")
|
||||
}
|
||||
|
||||
private val _storageCorruption = MutableStateFlow<StorageCorruption?>(null)
|
||||
val storageCorruption: StateFlow<StorageCorruption?> = _storageCorruption.asStateFlow()
|
||||
|
||||
fun clearStorageCorruption() {
|
||||
_storageCorruption.value = null
|
||||
}
|
||||
|
||||
val accountStorage: DesktopAccountStorage by lazy {
|
||||
DesktopAccountStorage(secureStorage, homeDir)
|
||||
DesktopAccountStorage(secureStorage, homeDir, onCorruption = { _storageCorruption.value = it })
|
||||
}
|
||||
|
||||
private val _allAccounts = MutableStateFlow<ImmutableList<AccountInfo>>(persistentListOf())
|
||||
@@ -232,16 +237,28 @@ class AccountManager internal constructor(
|
||||
|
||||
suspend fun loadSavedAccount(): Result<AccountState.LoggedIn> =
|
||||
try {
|
||||
val lastNpub = getLastNpub()
|
||||
val bunkerUri = getBunkerUri()
|
||||
// Clean up legacy files (one-time, silent)
|
||||
File(amethystDir, "last_account.txt").delete()
|
||||
File(amethystDir, "bunker_uri.txt").delete()
|
||||
File(amethystDir, "nwc_connection.txt").delete()
|
||||
|
||||
if (bunkerUri != null) {
|
||||
loadBunkerAccount(bunkerUri, lastNpub)
|
||||
} else if (lastNpub != null) {
|
||||
loadInternalAccount(lastNpub)
|
||||
} else {
|
||||
Result.failure(Exception("No saved account"))
|
||||
// Single source of truth: accounts.json.enc
|
||||
val activeNpub =
|
||||
accountStorage.currentAccount()
|
||||
?: return Result.failure(Exception("No saved account"))
|
||||
|
||||
val accounts = accountStorage.loadAccounts()
|
||||
val info =
|
||||
accounts.find { it.npub == activeNpub }
|
||||
?: return Result.failure(Exception("Account not found in storage"))
|
||||
|
||||
when (info.signerType) {
|
||||
is SignerType.Internal -> loadInternalAccount(activeNpub)
|
||||
is SignerType.Remote -> loadBunkerAccount((info.signerType as SignerType.Remote).bunkerUri, activeNpub)
|
||||
is SignerType.ViewOnly -> loadReadOnlyAccount(activeNpub)
|
||||
}
|
||||
} catch (e: kotlin.coroutines.cancellation.CancellationException) {
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
Result.failure(e)
|
||||
}
|
||||
@@ -298,7 +315,6 @@ class AccountManager internal constructor(
|
||||
}
|
||||
|
||||
val resolvedNpub = npub ?: pubKeyHex.hexToByteArray().toNpub()
|
||||
if (npub == null) saveLastNpub(resolvedNpub)
|
||||
|
||||
val state =
|
||||
AccountState.LoggedIn(
|
||||
@@ -439,18 +455,14 @@ class AccountManager internal constructor(
|
||||
ephemeralPrivKeyHex: String,
|
||||
npub: String,
|
||||
) {
|
||||
saveLastNpub(npub)
|
||||
secureStorage.savePrivateKey(bunkerEphemeralKeyAlias(npub), ephemeralPrivKeyHex)
|
||||
saveBunkerUri(bunkerUri)
|
||||
|
||||
// Also save to multi-account storage
|
||||
// Save to multi-account storage (sole source of truth)
|
||||
val info = AccountInfo(npub = npub, signerType = SignerType.Remote(bunkerUri))
|
||||
addAccountToStorage(info)
|
||||
accountStorage.setCurrentAccount(npub)
|
||||
}
|
||||
|
||||
fun hasBunkerAccount(): Boolean = getBunkerFile().exists()
|
||||
|
||||
fun setConnectingRelays() {
|
||||
_accountState.value = AccountState.ConnectingRelays
|
||||
}
|
||||
@@ -481,8 +493,6 @@ class AccountManager internal constructor(
|
||||
}
|
||||
}
|
||||
|
||||
saveLastNpub(current.npub)
|
||||
|
||||
// Always save to multi-account storage (including read-only)
|
||||
val info = AccountInfo(npub = current.npub, signerType = current.signerType)
|
||||
addAccountToStorage(info)
|
||||
@@ -564,23 +574,30 @@ class AccountManager internal constructor(
|
||||
// Clean up remote signer if bunker account
|
||||
if (current.signerType is SignerType.Remote) {
|
||||
(current.signer as? NostrSignerRemote)?.closeSubscription()
|
||||
if (deleteKey) {
|
||||
try {
|
||||
secureStorage.deletePrivateKey(bunkerEphemeralKeyAlias(current.npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
getBunkerFile().delete()
|
||||
}
|
||||
}
|
||||
if (deleteKey) {
|
||||
// Remove ALL credentials for this account
|
||||
try {
|
||||
secureStorage.deletePrivateKey(current.npub)
|
||||
clearLastNpub()
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
try {
|
||||
secureStorage.deletePrivateKey(bunkerEphemeralKeyAlias(current.npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
try {
|
||||
secureStorage.deletePrivateKey(nwcKeyAlias(current.npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
// Remove from metadata — fixes ghost accounts
|
||||
try {
|
||||
accountStorage.deleteAccount(current.npub)
|
||||
} catch (_: Exception) {
|
||||
}
|
||||
}
|
||||
}
|
||||
disconnectNip46Client()
|
||||
_nwcConnection.value = null
|
||||
_signerConnectionState.value = SignerConnectionState.NotRemote
|
||||
_lastPingTimeSec.value = null
|
||||
_accountState.value = AccountState.LoggedOut
|
||||
@@ -708,6 +725,10 @@ class AccountManager internal constructor(
|
||||
secureStorage.deletePrivateKey(bunkerEphemeralKeyAlias(npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
try {
|
||||
secureStorage.deletePrivateKey(nwcKeyAlias(npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
|
||||
refreshAccountList()
|
||||
|
||||
@@ -758,7 +779,7 @@ class AccountManager internal constructor(
|
||||
}
|
||||
|
||||
// Reload NWC for the new account
|
||||
loadNwcConnection()
|
||||
loadNwcConnection(targetNpub)
|
||||
|
||||
return newState
|
||||
}
|
||||
@@ -785,91 +806,62 @@ class AccountManager internal constructor(
|
||||
|
||||
fun currentAccount(): AccountState.LoggedIn? = _accountState.value as? AccountState.LoggedIn
|
||||
|
||||
// --- NWC ---
|
||||
// --- NWC (per-account, secret in keychain) ---
|
||||
|
||||
private fun nwcKeyAlias(npub: String) = "nwc_$npub"
|
||||
|
||||
fun hasNwcSetup(): Boolean = _nwcConnection.value != null
|
||||
|
||||
fun setNwcConnection(uri: String): Result<Nip47WalletConnect.Nip47URINorm> =
|
||||
suspend fun setNwcConnection(
|
||||
npub: String,
|
||||
uri: String,
|
||||
): Result<Nip47WalletConnect.Nip47URINorm> =
|
||||
try {
|
||||
val parsed = Nip47WalletConnect.parse(uri)
|
||||
if (parsed.secret == null) throw IllegalArgumentException("NWC URI has no secret")
|
||||
|
||||
// Store entire URI in keychain (not plaintext file)
|
||||
secureStorage.savePrivateKey(nwcKeyAlias(npub), uri)
|
||||
|
||||
_nwcConnection.value = parsed
|
||||
saveNwcUri(uri)
|
||||
Result.success(parsed)
|
||||
} catch (e: kotlin.coroutines.cancellation.CancellationException) {
|
||||
throw e
|
||||
} catch (e: Exception) {
|
||||
Result.failure(e)
|
||||
}
|
||||
|
||||
fun clearNwcConnection() {
|
||||
_nwcConnection.value = null
|
||||
getNwcFile().delete()
|
||||
}
|
||||
|
||||
fun loadNwcConnection() {
|
||||
val uri = getNwcFile().takeIf { it.exists() }?.readText()?.trim()
|
||||
if (!uri.isNullOrEmpty()) {
|
||||
try {
|
||||
_nwcConnection.value = Nip47WalletConnect.parse(uri)
|
||||
} catch (_: Exception) {
|
||||
getNwcFile().delete()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// --- File storage helpers ---
|
||||
|
||||
private fun ensureAmethystDir() {
|
||||
if (!amethystDir.exists()) {
|
||||
amethystDir.mkdirs()
|
||||
}
|
||||
suspend fun clearNwcConnection(npub: String) {
|
||||
try {
|
||||
Files.setPosixFilePermissions(
|
||||
amethystDir.toPath(),
|
||||
setOf(
|
||||
PosixFilePermission.OWNER_READ,
|
||||
PosixFilePermission.OWNER_WRITE,
|
||||
PosixFilePermission.OWNER_EXECUTE,
|
||||
),
|
||||
)
|
||||
} catch (_: UnsupportedOperationException) {
|
||||
// Windows — file system ACLs handle this
|
||||
secureStorage.deletePrivateKey(nwcKeyAlias(npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
}
|
||||
_nwcConnection.value = null
|
||||
}
|
||||
|
||||
suspend fun loadNwcConnection(npub: String) {
|
||||
val secret =
|
||||
try {
|
||||
secureStorage.getPrivateKey(nwcKeyAlias(npub))
|
||||
} catch (_: SecureStorageException) {
|
||||
null
|
||||
}
|
||||
|
||||
if (secret == null) {
|
||||
_nwcConnection.value = null
|
||||
return
|
||||
}
|
||||
|
||||
// Reconstruct Nip47URINorm from the NWC URI stored in the keychain.
|
||||
// The secret is the full NWC URI in hex form.
|
||||
// For simplicity, the entire NWC URI is stored in keychain (not just the secret).
|
||||
// This avoids needing to persist pubkey+relay separately.
|
||||
try {
|
||||
_nwcConnection.value = Nip47WalletConnect.parse(secret)
|
||||
} catch (_: Exception) {
|
||||
_nwcConnection.value = null
|
||||
}
|
||||
}
|
||||
|
||||
private fun saveNwcUri(uri: String) {
|
||||
ensureAmethystDir()
|
||||
getNwcFile().writeText(uri)
|
||||
}
|
||||
|
||||
private fun getNwcFile(): File = File(amethystDir, "nwc_connection.txt")
|
||||
|
||||
private fun getLastNpub(): String? {
|
||||
val file = getPrefsFile()
|
||||
return if (file.exists()) file.readText().trim().takeIf { it.isNotEmpty() } else null
|
||||
}
|
||||
|
||||
private fun saveLastNpub(npub: String) {
|
||||
ensureAmethystDir()
|
||||
getPrefsFile().writeText(npub)
|
||||
}
|
||||
|
||||
private fun clearLastNpub() {
|
||||
getPrefsFile().delete()
|
||||
}
|
||||
|
||||
private fun getPrefsFile(): File = File(amethystDir, "last_account.txt")
|
||||
|
||||
private fun getBunkerUri(): String? {
|
||||
val file = getBunkerFile()
|
||||
return if (file.exists()) file.readText().trim().takeIf { it.isNotEmpty() } else null
|
||||
}
|
||||
|
||||
private fun saveBunkerUri(uri: String) {
|
||||
ensureAmethystDir()
|
||||
getBunkerFile().writeText(uri)
|
||||
}
|
||||
|
||||
private fun getBunkerFile(): File = File(amethystDir, "bunker_uri.txt")
|
||||
}
|
||||
|
||||
internal fun parseBunkerRelays(uri: String): Set<NormalizedRelayUrl> {
|
||||
|
||||
+50
-1
@@ -47,9 +47,22 @@ import javax.crypto.spec.SecretKeySpec
|
||||
*
|
||||
* File: ~/.amethyst/accounts.json.enc
|
||||
*/
|
||||
sealed class StorageCorruption(
|
||||
val backupPath: String?,
|
||||
) {
|
||||
class FileCorrupted(
|
||||
backupPath: String?,
|
||||
) : StorageCorruption(backupPath)
|
||||
|
||||
class JsonMalformed(
|
||||
backupPath: String?,
|
||||
) : StorageCorruption(backupPath)
|
||||
}
|
||||
|
||||
class DesktopAccountStorage(
|
||||
private val secureStorage: SecureKeyStorage,
|
||||
private val homeDir: File = File(System.getProperty("user.home")),
|
||||
private val onCorruption: (StorageCorruption) -> Unit = {},
|
||||
) : AccountStorage {
|
||||
companion object {
|
||||
private const val METADATA_KEY_ALIAS = "account-metadata-key"
|
||||
@@ -115,16 +128,50 @@ class DesktopAccountStorage(
|
||||
val file = getAccountsFile()
|
||||
if (!file.exists()) return AccountMetadata()
|
||||
|
||||
val encrypted = file.readBytes()
|
||||
if (encrypted.size < GCM_IV_SIZE) {
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
return AccountMetadata()
|
||||
}
|
||||
|
||||
return try {
|
||||
val encrypted = file.readBytes()
|
||||
val decrypted = decrypt(encrypted)
|
||||
mapper.readValue<AccountMetadata>(decrypted)
|
||||
} catch (e: javax.crypto.AEADBadTagException) {
|
||||
Log.e("DesktopAccountStorage", "GCM auth tag mismatch — file corrupted or key lost", e)
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: javax.crypto.BadPaddingException) {
|
||||
Log.e("DesktopAccountStorage", "Decryption failed — file corrupted", e)
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: com.fasterxml.jackson.core.JacksonException) {
|
||||
Log.e("DesktopAccountStorage", "JSON malformed after decryption", e)
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.JsonMalformed(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: Exception) {
|
||||
Log.e("DesktopAccountStorage", "Failed to read accounts metadata", e)
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
}
|
||||
}
|
||||
|
||||
private fun backupCorruptFile(file: File): String? =
|
||||
try {
|
||||
val backup = File(file.parent, "accounts.json.enc.corrupt.${System.currentTimeMillis()}")
|
||||
java.nio.file.Files
|
||||
.copy(file.toPath(), backup.toPath())
|
||||
file.delete()
|
||||
backup.absolutePath
|
||||
} catch (_: Exception) {
|
||||
null
|
||||
}
|
||||
|
||||
private suspend fun writeMetadataToDisk(metadata: AccountMetadata) {
|
||||
ensureDir()
|
||||
val json = mapper.writeValueAsBytes(metadata)
|
||||
@@ -226,6 +273,8 @@ internal data class AccountInfoDto(
|
||||
val bunkerUri: String? = null,
|
||||
val displayName: String? = null,
|
||||
val isTransient: Boolean = false,
|
||||
val nwcPubKey: String? = null, // NWC wallet service pubkey (non-secret)
|
||||
val nwcRelay: String? = null, // NWC wallet relay URL (non-secret)
|
||||
) {
|
||||
fun toAccountInfo(): AccountInfo =
|
||||
AccountInfo(
|
||||
|
||||
-12
@@ -27,7 +27,6 @@ import kotlin.io.path.createTempDirectory
|
||||
import kotlin.test.AfterTest
|
||||
import kotlin.test.BeforeTest
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
class AccountManagerBunkerLoginTest {
|
||||
@@ -50,17 +49,6 @@ class AccountManagerBunkerLoginTest {
|
||||
tempDir.deleteRecursively()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun hasBunkerAccountReturnsFalseWhenNoFile() {
|
||||
assertFalse(manager.hasBunkerAccount())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun hasBunkerAccountReturnsTrueWhenFileExists() {
|
||||
File(amethystDir, "bunker_uri.txt").writeText("bunker://${"a".repeat(64)}?relay=wss://r.com")
|
||||
assertTrue(manager.hasBunkerAccount())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun setConnectingRelaysUpdatesState() {
|
||||
manager.setConnectingRelays()
|
||||
|
||||
+51
-17
@@ -21,6 +21,7 @@
|
||||
package com.vitorpamplona.amethyst.desktop.account
|
||||
|
||||
import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage
|
||||
import com.vitorpamplona.amethyst.commons.model.account.AccountInfo
|
||||
import com.vitorpamplona.amethyst.commons.model.account.SignerType
|
||||
import com.vitorpamplona.quartz.nip01Core.core.toHexKey
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
|
||||
@@ -45,6 +46,8 @@ class AccountManagerLoadAccountTest {
|
||||
@BeforeTest
|
||||
fun setup() {
|
||||
storage = mockk(relaxed = true)
|
||||
// Return null so DesktopAccountStorage generates a fresh AES key
|
||||
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
|
||||
tempDir = createTempDirectory("acctmgr-load-test").toFile()
|
||||
amethystDir = File(tempDir, ".amethyst")
|
||||
amethystDir.mkdirs()
|
||||
@@ -57,9 +60,9 @@ class AccountManagerLoadAccountTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountNoNpubReturnsFailure() =
|
||||
fun loadSavedAccountNoAccountsReturnsFailure() =
|
||||
runTest {
|
||||
// No last_account.txt file
|
||||
// Empty accounts.json.enc (no accounts saved)
|
||||
val result = manager.loadSavedAccount()
|
||||
assertTrue(result.isFailure)
|
||||
}
|
||||
@@ -71,10 +74,11 @@ class AccountManagerLoadAccountTest {
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
val privKeyHex = keyPair.privKey!!.toHexKey()
|
||||
|
||||
// Write last_account.txt
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
// Save account via storage API
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
|
||||
// Mock storage to return the private key
|
||||
// Mock keychain to return the private key
|
||||
coEvery { storage.getPrivateKey(npub) } returns privKeyHex
|
||||
|
||||
val result = manager.loadSavedAccount()
|
||||
@@ -90,7 +94,8 @@ class AccountManagerLoadAccountTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns null
|
||||
|
||||
val result = manager.loadSavedAccount()
|
||||
@@ -106,10 +111,13 @@ class AccountManagerLoadAccountTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
File(amethystDir, "bunker_uri.txt").writeText(
|
||||
"bunker://$validHex?relay=wss://r.com",
|
||||
manager.accountStorage.saveAccount(
|
||||
AccountInfo(npub = npub, signerType = SignerType.Remote("bunker://$validHex?relay=wss://r.com")),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns null
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
} returns null
|
||||
@@ -121,23 +129,49 @@ class AccountManagerLoadAccountTest {
|
||||
@Test
|
||||
fun loadSavedAccountBunkerSuccess() =
|
||||
runTest {
|
||||
val walletKeyPair = KeyPair()
|
||||
val validHex = walletKeyPair.pubKey.toHexKey()
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
val ephemeralKeyPair = KeyPair()
|
||||
val ephemeralPrivKeyHex = ephemeralKeyPair.privKey!!.toHexKey()
|
||||
val validHex = keyPair.pubKey.toHexKey()
|
||||
val ephemeralKey = KeyPair()
|
||||
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
File(amethystDir, "bunker_uri.txt").writeText(
|
||||
"bunker://$validHex?relay=wss://r.com",
|
||||
manager.accountStorage.saveAccount(
|
||||
AccountInfo(npub = npub, signerType = SignerType.Remote("bunker://$validHex?relay=wss://relay.nsec.app")),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
} returns ephemeralPrivKeyHex
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns ephemeralKey.privKey!!.toHexKey()
|
||||
|
||||
val result = manager.loadSavedAccount()
|
||||
assertTrue(result.isSuccess)
|
||||
val state = result.getOrThrow()
|
||||
assertIs<SignerType.Remote>(state.signerType)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountDeletesLegacyFiles() =
|
||||
runTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
val privKeyHex = keyPair.privKey!!.toHexKey()
|
||||
|
||||
// Create legacy files
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
File(amethystDir, "bunker_uri.txt").writeText("bunker://test")
|
||||
File(amethystDir, "nwc_connection.txt").writeText("nostr+walletconnect://test")
|
||||
|
||||
// Save via storage
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns privKeyHex
|
||||
|
||||
manager.loadSavedAccount()
|
||||
|
||||
// Legacy files should be deleted
|
||||
assertTrue(!File(amethystDir, "last_account.txt").exists())
|
||||
assertTrue(!File(amethystDir, "bunker_uri.txt").exists())
|
||||
assertTrue(!File(amethystDir, "nwc_connection.txt").exists())
|
||||
}
|
||||
}
|
||||
|
||||
+2
@@ -24,6 +24,7 @@ import com.vitorpamplona.amethyst.commons.domain.nip46.SignerConnectionState
|
||||
import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
|
||||
import com.vitorpamplona.quartz.nip19Bech32.toNsec
|
||||
import io.mockk.coEvery
|
||||
import io.mockk.coVerify
|
||||
import io.mockk.mockk
|
||||
import kotlinx.coroutines.test.runTest
|
||||
@@ -44,6 +45,7 @@ class AccountManagerLogoutTest {
|
||||
@BeforeTest
|
||||
fun setup() {
|
||||
storage = mockk(relaxed = true)
|
||||
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
|
||||
tempDir = createTempDirectory("acctmgr-logout-test").toFile()
|
||||
manager = AccountManager(storage, tempDir)
|
||||
}
|
||||
|
||||
+38
-22
@@ -55,6 +55,7 @@ class AccountManagerNip46IsolationTest {
|
||||
@BeforeTest
|
||||
fun setup() {
|
||||
storage = mockk(relaxed = true)
|
||||
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
|
||||
tempDir = createTempDirectory("acctmgr-nip46-iso-test").toFile()
|
||||
amethystDir = File(tempDir, ".amethyst")
|
||||
amethystDir.mkdirs()
|
||||
@@ -69,16 +70,21 @@ class AccountManagerNip46IsolationTest {
|
||||
@Test
|
||||
fun bunkerLoadCreatesRemoteSignerInternally() =
|
||||
runTest {
|
||||
val validHex = "a".repeat(64)
|
||||
val walletKeyPair = KeyPair()
|
||||
val validHex = walletKeyPair.pubKey.toHexKey()
|
||||
val ephemeralKeyPair = KeyPair()
|
||||
File(amethystDir, "bunker_uri.txt").writeText(
|
||||
"bunker://$validHex?relay=wss://relay.nsec.app",
|
||||
)
|
||||
File(amethystDir, "last_account.txt").writeText(
|
||||
ephemeralKeyPair.pubKey.toNpub(),
|
||||
val npub = ephemeralKeyPair.pubKey.toNpub()
|
||||
val bunkerUri = "bunker://$validHex?relay=wss://relay.nsec.app"
|
||||
|
||||
manager.accountStorage.saveAccount(
|
||||
com.vitorpamplona.amethyst.commons.model.account.AccountInfo(
|
||||
npub = npub,
|
||||
signerType = SignerType.Remote(bunkerUri),
|
||||
),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns ephemeralKeyPair.privKey!!.toHexKey()
|
||||
|
||||
val result = manager.loadSavedAccount()
|
||||
@@ -130,16 +136,21 @@ class AccountManagerNip46IsolationTest {
|
||||
@Test
|
||||
fun logoutResetsStateAfterBunkerLoad() =
|
||||
runTest {
|
||||
val validHex = "a".repeat(64)
|
||||
val walletKeyPair = KeyPair()
|
||||
val validHex = walletKeyPair.pubKey.toHexKey()
|
||||
val ephemeralKeyPair = KeyPair()
|
||||
File(amethystDir, "bunker_uri.txt").writeText(
|
||||
"bunker://$validHex?relay=wss://relay.nsec.app",
|
||||
)
|
||||
File(amethystDir, "last_account.txt").writeText(
|
||||
ephemeralKeyPair.pubKey.toNpub(),
|
||||
val npub = ephemeralKeyPair.pubKey.toNpub()
|
||||
val bunkerUri = "bunker://$validHex?relay=wss://relay.nsec.app"
|
||||
|
||||
manager.accountStorage.saveAccount(
|
||||
com.vitorpamplona.amethyst.commons.model.account.AccountInfo(
|
||||
npub = npub,
|
||||
signerType = SignerType.Remote(bunkerUri),
|
||||
),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns ephemeralKeyPair.privKey!!.toHexKey()
|
||||
|
||||
manager.loadSavedAccount()
|
||||
@@ -155,23 +166,28 @@ class AccountManagerNip46IsolationTest {
|
||||
@Test
|
||||
fun logoutThenReloadCreatesFreshNip46Client() =
|
||||
runTest {
|
||||
val validHex = "a".repeat(64)
|
||||
val walletKeyPair = KeyPair()
|
||||
val validHex = walletKeyPair.pubKey.toHexKey()
|
||||
val ephemeralKeyPair = KeyPair()
|
||||
val npub = ephemeralKeyPair.pubKey.toNpub()
|
||||
val bunkerUri = "bunker://$validHex?relay=wss://relay.nsec.app"
|
||||
|
||||
fun writeBunkerFiles() {
|
||||
File(amethystDir, "bunker_uri.txt").writeText(bunkerUri)
|
||||
File(amethystDir, "last_account.txt").writeText(
|
||||
ephemeralKeyPair.pubKey.toNpub(),
|
||||
suspend fun setupBunkerAccount() {
|
||||
manager.accountStorage.saveAccount(
|
||||
com.vitorpamplona.amethyst.commons.model.account.AccountInfo(
|
||||
npub = npub,
|
||||
signerType = SignerType.Remote(bunkerUri),
|
||||
),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
}
|
||||
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns ephemeralKeyPair.privKey!!.toHexKey()
|
||||
|
||||
// First load
|
||||
writeBunkerFiles()
|
||||
setupBunkerAccount()
|
||||
manager.loadSavedAccount()
|
||||
val firstSigner = manager.currentAccount()?.signer
|
||||
assertNotNull(firstSigner)
|
||||
@@ -181,7 +197,7 @@ class AccountManagerNip46IsolationTest {
|
||||
assertIs<AccountState.LoggedOut>(manager.accountState.value)
|
||||
|
||||
// Second load — should create a fresh NIP-46 client
|
||||
writeBunkerFiles()
|
||||
setupBunkerAccount()
|
||||
manager.loadSavedAccount()
|
||||
val secondSigner = manager.currentAccount()?.signer
|
||||
assertNotNull(secondSigner)
|
||||
|
||||
+8
-1
@@ -55,6 +55,7 @@ class AccountManagerStateTransitionTest {
|
||||
@BeforeTest
|
||||
fun setup() {
|
||||
storage = mockk(relaxed = true)
|
||||
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
|
||||
tempDir = createTempDirectory("acctmgr-state-test").toFile()
|
||||
amethystDir = File(tempDir, ".amethyst")
|
||||
amethystDir.mkdirs()
|
||||
@@ -81,7 +82,13 @@ class AccountManagerStateTransitionTest {
|
||||
// Then load an internal account (simulating successful load)
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
File(amethystDir, "last_account.txt").writeText(npub)
|
||||
manager.accountStorage.saveAccount(
|
||||
com.vitorpamplona.amethyst.commons.model.account.AccountInfo(
|
||||
npub = npub,
|
||||
signerType = com.vitorpamplona.amethyst.commons.model.account.SignerType.Internal,
|
||||
),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns keyPair.privKey!!.toHexKey()
|
||||
manager.loadSavedAccount()
|
||||
|
||||
|
||||
@@ -0,0 +1,462 @@
|
||||
---
|
||||
title: "fix: Account Security Hardening"
|
||||
type: fix
|
||||
status: active
|
||||
date: 2026-05-14
|
||||
origin: docs/brainstorms/2026-05-14-fix-account-security-hardening-brainstorm.md
|
||||
---
|
||||
|
||||
# Account Security Hardening
|
||||
|
||||
## Deepening Insights (2026-05-14)
|
||||
|
||||
1. **NWC URI reconstruction**: `Nip47URINorm` has a direct constructor `(pubKeyHex, relayUri, secret, lud16)` — no URI string parsing needed. Store pubKeyHex+relayUri in metadata, secret in keychain, reconstruct via constructor.
|
||||
2. **SecureKeyStorage alias**: Accepts any arbitrary string — `"nwc_npub1..."` confirmed to work. No format validation on alias.
|
||||
3. **Corruption detection**: Use `StateFlow<StorageCorruption?>` on `AccountManager` (matches existing `forceLogoutReason` pattern). Distinguish key-lost (`getPrivateKey("account-metadata-key")` returns null) vs file-corrupted (`AEADBadTagException`) vs JSON-malformed (`JacksonException`).
|
||||
4. **NWC save failure**: Don't fall back to plaintext or in-memory — fail the connection entirely. NWC secrets control real money.
|
||||
5. **Cold boot ConnectingRelays**: Must read `accounts.json.enc` first to know account type, then show ConnectingRelays UI if Remote. Can't show it before reading.
|
||||
6. **ensureCurrentAccountInStorage**: No longer needed — `loadSavedAccount()` only loads from `accounts.json.enc`, so account is guaranteed to be there.
|
||||
7. **Backup pattern**: Use timestamped backups (`accounts.json.enc.corrupt.<millis>`) to avoid overwriting previous backups.
|
||||
|
||||
## Overview
|
||||
|
||||
Single PR fixing 6 security and correctness issues in desktop account management.
|
||||
Makes `accounts.json.enc` the sole source of truth, moves NWC secrets to OS keychain,
|
||||
makes NWC per-account, and eliminates stale file bugs. (see brainstorm for full rationale)
|
||||
|
||||
## Problem Statement
|
||||
|
||||
1. **CRITICAL:** NWC wallet secret in plaintext `nwc_connection.txt` — funds can be stolen
|
||||
2. **HIGH:** NWC is global — switching accounts doesn't switch wallets
|
||||
3. **HIGH:** Cold boot reads stale `bunker_uri.txt` before `accounts.json.enc` — nsec misidentified as bunker
|
||||
4. **MEDIUM:** `logout(deleteKey=true)` doesn't remove from `accounts.json.enc` — ghost accounts
|
||||
5. **MEDIUM:** `accounts.json.enc` corruption silently returns empty — data loss
|
||||
6. **LOW:** `bunker_uri.txt` is singleton — can't support multi-bunker
|
||||
|
||||
## Implementation Phases
|
||||
|
||||
### Phase 1: NWC Secret to Keychain + Per-Account (Issues 1, 2)
|
||||
|
||||
#### 1a: Add NWC fields to AccountInfoDto
|
||||
|
||||
**File:** `DesktopAccountStorage.kt:223-258`
|
||||
|
||||
```kotlin
|
||||
internal data class AccountInfoDto(
|
||||
val npub: String,
|
||||
val signerKind: String,
|
||||
val bunkerUri: String? = null,
|
||||
val displayName: String? = null,
|
||||
val isTransient: Boolean = false,
|
||||
val nwcPubKey: String? = null, // NEW: wallet service pubkey (non-secret)
|
||||
val nwcRelay: String? = null, // NEW: wallet relay URL (non-secret)
|
||||
)
|
||||
```
|
||||
|
||||
Jackson handles new nullable fields transparently — old `accounts.json.enc` files
|
||||
deserialize without them (null defaults). `AccountInfo` in commons also needs
|
||||
matching fields (or `AccountInfoDto.toAccountInfo()` maps them).
|
||||
|
||||
#### 1b: Rewrite NWC methods in AccountManager
|
||||
|
||||
**File:** `AccountManager.kt:788-844`
|
||||
|
||||
Replace `setNwcConnection()`, `clearNwcConnection()`, `loadNwcConnection()`.
|
||||
|
||||
**Key insight from deepening:** `Nip47URINorm` has a direct constructor — no URI
|
||||
string reconstruction needed. Store components, rebuild via constructor.
|
||||
|
||||
```kotlin
|
||||
// --- NWC (per-account, secret in keychain) ---
|
||||
|
||||
private fun nwcKeyAlias(npub: String) = "nwc_$npub"
|
||||
|
||||
fun setNwcConnection(npub: String, uri: String): Result<Nip47WalletConnect.Nip47URINorm> =
|
||||
try {
|
||||
val parsed = Nip47WalletConnect.parse(uri)
|
||||
val secret = parsed.secret ?: throw IllegalArgumentException("NWC URI has no secret")
|
||||
|
||||
// Secret → keychain (fail entirely if keychain unavailable — don't fall back to plaintext)
|
||||
secureStorage.savePrivateKey(nwcKeyAlias(npub), secret)
|
||||
|
||||
// Non-secret parts → accounts.json.enc
|
||||
scope.launch {
|
||||
val info = accountStorage.loadAccounts().find { it.npub == npub }
|
||||
if (info != null) {
|
||||
accountStorage.saveAccount(
|
||||
info.copy(
|
||||
nwcPubKey = parsed.pubKeyHex,
|
||||
nwcRelay = parsed.relayUri.toString(),
|
||||
),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
_nwcConnection.value = parsed
|
||||
Result.success(parsed)
|
||||
} catch (e: Exception) {
|
||||
Result.failure(e)
|
||||
}
|
||||
|
||||
fun clearNwcConnection(npub: String) {
|
||||
try { secureStorage.deletePrivateKey(nwcKeyAlias(npub)) } catch (_: SecureStorageException) {}
|
||||
scope.launch {
|
||||
val info = accountStorage.loadAccounts().find { it.npub == npub }
|
||||
if (info != null) {
|
||||
accountStorage.saveAccount(info.copy(nwcPubKey = null, nwcRelay = null))
|
||||
}
|
||||
}
|
||||
_nwcConnection.value = null
|
||||
}
|
||||
|
||||
fun loadNwcConnection(npub: String) {
|
||||
val secret = try {
|
||||
secureStorage.getPrivateKey(nwcKeyAlias(npub))
|
||||
} catch (_: SecureStorageException) { null }
|
||||
|
||||
if (secret != null) {
|
||||
scope.launch {
|
||||
val info = accountStorage.loadAccounts().find { it.npub == npub }
|
||||
if (info?.nwcPubKey != null && info.nwcRelay != null) {
|
||||
// Reconstruct directly — no URI parsing needed
|
||||
_nwcConnection.value = Nip47WalletConnect.Nip47URINorm(
|
||||
pubKeyHex = info.nwcPubKey,
|
||||
relayUri = NormalizedRelayUrl(info.nwcRelay),
|
||||
secret = secret,
|
||||
)
|
||||
}
|
||||
}
|
||||
} else {
|
||||
_nwcConnection.value = null
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### 1c: Delete legacy NWC file helpers
|
||||
|
||||
**Remove from AccountManager.kt:**
|
||||
- `saveNwcUri()` (line 839-841)
|
||||
- `getNwcFile()` (line 844)
|
||||
|
||||
**Delete on startup:** Add to `loadSavedAccount()`:
|
||||
```kotlin
|
||||
// Clean up legacy plaintext NWC file
|
||||
File(amethystDir, "nwc_connection.txt").delete()
|
||||
```
|
||||
|
||||
#### 1d: Update callers
|
||||
|
||||
- `switchAccount()` (line 761): `loadNwcConnection()` → `loadNwcConnection(targetNpub)`
|
||||
- `Main.kt`: any `loadNwcConnection()` calls → pass npub
|
||||
- Wallet column: `setNwcConnection(uri)` → `setNwcConnection(npub, uri)`
|
||||
|
||||
---
|
||||
|
||||
### Phase 2: Cold Boot from accounts.json.enc (Issue 3)
|
||||
|
||||
#### 2a: Rewrite loadSavedAccount()
|
||||
|
||||
**File:** `AccountManager.kt:233-247`
|
||||
|
||||
Replace entirely:
|
||||
|
||||
```kotlin
|
||||
suspend fun loadSavedAccount(): Result<AccountState.LoggedIn> =
|
||||
try {
|
||||
val activeNpub = accountStorage.currentAccount()
|
||||
?: return Result.failure(Exception("No saved account"))
|
||||
|
||||
val accounts = accountStorage.loadAccounts()
|
||||
val info = accounts.find { it.npub == activeNpub }
|
||||
?: return Result.failure(Exception("Account not found in storage"))
|
||||
|
||||
// Clean up legacy files (one-time)
|
||||
File(amethystDir, "last_account.txt").delete()
|
||||
File(amethystDir, "bunker_uri.txt").delete()
|
||||
File(amethystDir, "nwc_connection.txt").delete()
|
||||
|
||||
when (info.signerType) {
|
||||
is SignerType.Internal -> loadInternalAccount(activeNpub)
|
||||
is SignerType.Remote -> loadBunkerAccount(
|
||||
(info.signerType as SignerType.Remote).bunkerUri,
|
||||
activeNpub,
|
||||
)
|
||||
is SignerType.ViewOnly -> loadReadOnlyAccount(activeNpub)
|
||||
}
|
||||
} catch (e: Exception) {
|
||||
Result.failure(e)
|
||||
}
|
||||
```
|
||||
|
||||
#### 2b: Rewrite saveCurrentAccount()
|
||||
|
||||
**File:** `AccountManager.kt:460-492`
|
||||
|
||||
Remove `saveLastNpub()` call (line 484). The `accountStorage.setCurrentAccount()`
|
||||
call is sufficient.
|
||||
|
||||
#### 2c: Remove legacy file helpers
|
||||
|
||||
**Remove from AccountManager.kt:**
|
||||
- `getLastNpub()` (line 846-848)
|
||||
- `saveLastNpub()` (line 851-853)
|
||||
- `clearLastNpub()` (line 856-857)
|
||||
- `getPrefsFile()` (line 860)
|
||||
- `getBunkerUri()` (line 862-864)
|
||||
- `saveBunkerUri()` (line 867-869)
|
||||
- `getBunkerFile()` (line 872)
|
||||
- `hasBunkerAccount()` — uses `getBunkerUri()`, no longer needed
|
||||
|
||||
#### 2d: Update saveBunkerAccount()
|
||||
|
||||
Stop writing to `bunker_uri.txt`. The bunker URI is already saved in
|
||||
`accounts.json.enc` via `AccountInfoDto.bunkerUri`.
|
||||
|
||||
#### 2e: Update Main.kt startup
|
||||
|
||||
**File:** `Main.kt:831-853`
|
||||
|
||||
**Deepening insight:** `ensureCurrentAccountInStorage()` is no longer needed
|
||||
since `loadSavedAccount()` only loads from `accounts.json.enc` (account is
|
||||
guaranteed to be there). `ConnectingRelays` UI must come AFTER reading
|
||||
metadata to know the account type.
|
||||
|
||||
```kotlin
|
||||
scope.launch(Dispatchers.IO) {
|
||||
accountManager.refreshAccountListOnStartup()
|
||||
|
||||
val result = accountManager.loadSavedAccount()
|
||||
if (result.isSuccess) {
|
||||
accountManager.refreshAccountList()
|
||||
|
||||
val current = accountManager.currentAccount()
|
||||
if (current?.signerType is SignerType.Remote) {
|
||||
accountManager.startHeartbeat(scope)
|
||||
}
|
||||
// Load per-account NWC
|
||||
accountManager.loadNwcConnection(current!!.npub)
|
||||
}
|
||||
// If failure: state remains LoggedOut → login screen shows automatically
|
||||
}
|
||||
```
|
||||
|
||||
Removed:
|
||||
- `hasBunkerAccount()` — was based on `bunker_uri.txt` existence
|
||||
- `ensureCurrentAccountInStorage()` — no longer needed
|
||||
- `logout(deleteKey=true)` fallback for corrupt bunker state — corruption handling in Phase 4 covers this
|
||||
|
||||
---
|
||||
|
||||
### Phase 3: Logout Cleanup (Issue 4)
|
||||
|
||||
#### 3a: Fix logout(deleteKey=true)
|
||||
|
||||
**File:** `AccountManager.kt:561-589`
|
||||
|
||||
```kotlin
|
||||
suspend fun logout(deleteKey: Boolean = false) {
|
||||
val current = currentAccount()
|
||||
if (current != null) {
|
||||
if (current.signerType is SignerType.Remote) {
|
||||
(current.signer as? NostrSignerRemote)?.closeSubscription()
|
||||
}
|
||||
if (deleteKey) {
|
||||
try { secureStorage.deletePrivateKey(current.npub) } catch (_: SecureStorageException) {}
|
||||
try { secureStorage.deletePrivateKey(bunkerEphemeralKeyAlias(current.npub)) } catch (_: SecureStorageException) {}
|
||||
try { secureStorage.deletePrivateKey(nwcKeyAlias(current.npub)) } catch (_: SecureStorageException) {}
|
||||
// Remove from metadata — fixes ghost accounts
|
||||
accountStorage.deleteAccount(current.npub)
|
||||
}
|
||||
}
|
||||
disconnectNip46Client()
|
||||
_nwcConnection.value = null
|
||||
_signerConnectionState.value = SignerConnectionState.NotRemote
|
||||
_lastPingTimeSec.value = null
|
||||
_accountState.value = AccountState.LoggedOut
|
||||
stopHeartbeat()
|
||||
}
|
||||
```
|
||||
|
||||
Key changes:
|
||||
- Add `secureStorage.deletePrivateKey(nwcKeyAlias(current.npub))`
|
||||
- Add `accountStorage.deleteAccount(current.npub)` when `deleteKey=true`
|
||||
- Remove `getBunkerFile().delete()` and `clearLastNpub()` (files eliminated)
|
||||
|
||||
---
|
||||
|
||||
### Phase 4: Corruption Handling (Issue 5)
|
||||
|
||||
**Deepening insight:** Use `StateFlow` pattern (matches existing `forceLogoutReason`).
|
||||
Distinguish key-lost vs file-corrupted. Use timestamped backups.
|
||||
|
||||
#### 4a: Add StorageCorruption sealed class to AccountManager
|
||||
|
||||
```kotlin
|
||||
sealed class StorageCorruption {
|
||||
data class KeyLost(val backupPath: String?) : StorageCorruption()
|
||||
data class FileCorrupted(val backupPath: String?) : StorageCorruption()
|
||||
data class JsonMalformed(val backupPath: String?) : StorageCorruption()
|
||||
}
|
||||
|
||||
private val _storageCorruption = MutableStateFlow<StorageCorruption?>(null)
|
||||
val storageCorruption: StateFlow<StorageCorruption?> = _storageCorruption.asStateFlow()
|
||||
fun clearStorageCorruption() { _storageCorruption.value = null }
|
||||
```
|
||||
|
||||
#### 4b: Rewrite readMetadataFromDisk in DesktopAccountStorage
|
||||
|
||||
**File:** `DesktopAccountStorage.kt:114-126`
|
||||
|
||||
Accept a corruption callback (constructor param), catch specific exceptions:
|
||||
|
||||
```kotlin
|
||||
class DesktopAccountStorage(
|
||||
homeDir: File,
|
||||
private val secureStorage: SecureKeyStorage,
|
||||
private val onCorruption: (StorageCorruption) -> Unit = {}, // NEW
|
||||
) : AccountStorage {
|
||||
// ...
|
||||
|
||||
private suspend fun readMetadataFromDisk(): AccountMetadata {
|
||||
val file = getAccountsFile()
|
||||
if (!file.exists()) return AccountMetadata()
|
||||
|
||||
val encrypted = file.readBytes()
|
||||
if (encrypted.size < GCM_IV_SIZE) {
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
return AccountMetadata()
|
||||
}
|
||||
|
||||
return try {
|
||||
val decrypted = decrypt(encrypted)
|
||||
mapper.readValue<AccountMetadata>(decrypted)
|
||||
} catch (e: javax.crypto.AEADBadTagException) {
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: javax.crypto.BadPaddingException) {
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: com.fasterxml.jackson.core.JacksonException) {
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.JsonMalformed(backup))
|
||||
AccountMetadata()
|
||||
} catch (e: Exception) {
|
||||
Log.e("DesktopAccountStorage", "Failed to read accounts metadata", e)
|
||||
val backup = backupCorruptFile(file)
|
||||
onCorruption(StorageCorruption.FileCorrupted(backup))
|
||||
AccountMetadata()
|
||||
}
|
||||
}
|
||||
|
||||
private fun backupCorruptFile(file: File): String? =
|
||||
try {
|
||||
val backup = File(file.parent, "accounts.json.enc.corrupt.${System.currentTimeMillis()}")
|
||||
Files.copy(file.toPath(), backup.toPath())
|
||||
file.delete()
|
||||
backup.absolutePath
|
||||
} catch (_: Exception) { null }
|
||||
}
|
||||
```
|
||||
|
||||
#### 4c: Wire in AccountManager
|
||||
|
||||
Pass corruption callback when constructing `DesktopAccountStorage`:
|
||||
|
||||
```kotlin
|
||||
val accountStorage = DesktopAccountStorage(
|
||||
homeDir = homeDir,
|
||||
secureStorage = secureStorage,
|
||||
onCorruption = { _storageCorruption.value = it },
|
||||
)
|
||||
```
|
||||
|
||||
#### 4d: Show warning dialog in Main.kt
|
||||
|
||||
Observe `storageCorruption` StateFlow (same pattern as `forceLogoutReason`):
|
||||
|
||||
```kotlin
|
||||
val corruption by accountManager.storageCorruption.collectAsState()
|
||||
corruption?.let { c ->
|
||||
AlertDialog(
|
||||
onDismissRequest = { accountManager.clearStorageCorruption() },
|
||||
title = { Text("Account Data Issue") },
|
||||
text = {
|
||||
Text(when (c) {
|
||||
is StorageCorruption.KeyLost -> "Encryption key not found. Account list cannot be recovered."
|
||||
is StorageCorruption.FileCorrupted -> "Account data was corrupted."
|
||||
is StorageCorruption.JsonMalformed -> "Account data was partially written."
|
||||
} + if (c.backupPath != null) "\n\nA backup was saved." else "")
|
||||
},
|
||||
confirmButton = {
|
||||
Button(onClick = { accountManager.clearStorageCorruption() }) { Text("OK") }
|
||||
},
|
||||
)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Files Modified
|
||||
|
||||
| File | Lines | Changes |
|
||||
|------|-------|---------|
|
||||
| `AccountManager.kt` | 233-247, 460-492, 561-589, 788-872 | Rewrite loadSavedAccount, saveCurrentAccount, logout, NWC methods; remove 8 legacy helpers |
|
||||
| `DesktopAccountStorage.kt` | 114-126, 223-258 | Corruption backup; add nwcPubKey/nwcRelay to AccountInfoDto |
|
||||
| `Main.kt` | 835-852 | Remove hasBunkerAccount check; add corruption warning dialog |
|
||||
|
||||
## Files Deleted (Legacy, stop reading/writing)
|
||||
|
||||
| File | Content | Risk |
|
||||
|------|---------|------|
|
||||
| `~/.amethyst/nwc_connection.txt` | NWC URI with spending secret | CRITICAL — deleted on startup |
|
||||
| `~/.amethyst/last_account.txt` | Last active npub | Low — deleted on startup |
|
||||
| `~/.amethyst/bunker_uri.txt` | Bunker URI | Medium — deleted on startup |
|
||||
|
||||
## Test Cases
|
||||
|
||||
| # | Test | File | Type |
|
||||
|---|------|------|------|
|
||||
| 1 | nsec login → save → cold boot loads Internal | `AccountManagerColdBootTest.kt` | Unit |
|
||||
| 2 | bunker login → save → cold boot loads Remote with URI | `AccountManagerColdBootTest.kt` | Unit |
|
||||
| 3 | bunker login → switch to nsec → cold boot loads Internal | `AccountManagerColdBootTest.kt` | Unit |
|
||||
| 4 | Two bunker accounts → switch → each uses own URI | `AccountManagerSwitchTest.kt` | Unit |
|
||||
| 5 | NWC set for account A → switch to B → back to A → NWC restored | `AccountManagerNwcTest.kt` | Unit |
|
||||
| 6 | NWC secret stored in keychain, NOT in any file | `AccountManagerNwcTest.kt` | Unit |
|
||||
| 7 | logout(deleteKey=true) removes from metadata + keychain | `AccountManagerLogoutTest.kt` | Unit |
|
||||
| 8 | Corrupted accounts.json.enc → .bak created, empty returned | `DesktopAccountStorageCorruptionTest.kt` | Unit |
|
||||
| 9 | Legacy files deleted on first cold boot | `AccountManagerMigrationTest.kt` | Unit |
|
||||
| 10 | Fresh install → "No saved account" | `AccountManagerColdBootTest.kt` | Unit |
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `nwc_connection.txt` never written; deleted on startup if exists
|
||||
- [ ] `last_account.txt` never written; deleted on startup if exists
|
||||
- [ ] `bunker_uri.txt` never written; deleted on startup if exists
|
||||
- [ ] NWC secret stored via `secureStorage.savePrivateKey("nwc_<npub>", secret)`
|
||||
- [ ] `loadSavedAccount()` routes by `SignerType` from `accounts.json.enc`
|
||||
- [ ] `logout(deleteKey=true)` removes account from `accounts.json.enc`
|
||||
- [ ] Corrupted `accounts.json.enc` → `.bak` backup + warning dialog
|
||||
- [ ] `switchAccount()` loads correct NWC per-account
|
||||
- [ ] All 10 test cases pass
|
||||
- [ ] `./gradlew spotlessCheck` passes
|
||||
- [ ] No plaintext secrets in `~/.amethyst/` directory
|
||||
|
||||
## Risk Analysis
|
||||
|
||||
| Risk | Severity | Mitigation |
|
||||
|------|----------|------------|
|
||||
| Existing users lose NWC connection | Low | Acceptable — reconnect wallet once (brainstorm decision) |
|
||||
| Existing users lose account on cold boot | Medium | accounts.json.enc already stores everything; only users without it are affected (new installs) |
|
||||
| SecureKeyStorage fallback password prompt for NWC | Low | Same pattern as nsec storage — well-tested |
|
||||
| Jackson backward compat for new AccountInfoDto fields | Low | Nullable fields with defaults — Jackson handles gracefully |
|
||||
|
||||
## Sources
|
||||
|
||||
- **Origin brainstorm:** docs/brainstorms/2026-05-14-fix-account-security-hardening-brainstorm.md
|
||||
- `AccountManager.kt` — cold boot (233-247), save (460-492), logout (561-589), NWC (788-844), legacy files (846-872)
|
||||
- `DesktopAccountStorage.kt` — corruption (114-126), AccountInfoDto (223-258)
|
||||
- `Main.kt` — startup (831-853)
|
||||
- `SecureKeyStorage.kt` — savePrivateKey/getPrivateKey/deletePrivateKey (commons/jvmMain)
|
||||
Reference in New Issue
Block a user