From e690292bbd46c1004c9cf5939f24485bf4d54737 Mon Sep 17 00:00:00 2001 From: nrobi144 Date: Sat, 23 May 2026 14:02:36 +0300 Subject: [PATCH] fix(desktop): fix nsec session not persisting across restarts LoginScreen's fire-and-forget save coroutine used rememberCoroutineScope which got cancelled when the composable left composition after login. Move saveCurrentAccount() to onLoginSuccess in Main.kt which uses the app-level scope that survives recomposition. Fixes both nsec login and generate-new-account flows. Co-Authored-By: Claude Opus 4.6 (1M context) --- ...fix-nsec-session-persistence-brainstorm.md | 167 ++++++++++++++++++ .../vitorpamplona/amethyst/desktop/Main.kt | 5 +- .../amethyst/desktop/ui/LoginScreen.kt | 16 +- 3 files changed, 174 insertions(+), 14 deletions(-) create mode 100644 desktopApp/plans/2026-05-23-fix-nsec-session-persistence-brainstorm.md diff --git a/desktopApp/plans/2026-05-23-fix-nsec-session-persistence-brainstorm.md b/desktopApp/plans/2026-05-23-fix-nsec-session-persistence-brainstorm.md new file mode 100644 index 0000000000..a91f20a34e --- /dev/null +++ b/desktopApp/plans/2026-05-23-fix-nsec-session-persistence-brainstorm.md @@ -0,0 +1,167 @@ +# Fix: nsec Session Not Persisting Across Desktop Restarts + +## Current Persistence Mechanism + +Two-layer storage architecture: + +| Layer | What | Where | Format | +|-------|------|-------|--------| +| Account metadata | npub, signerType, activeNpub | `~/.amethyst/accounts.json.enc` | AES-256-GCM encrypted JSON | +| Private keys | nsec hex, ephemeral keys, NWC URIs | OS keychain (macOS Keychain via `java-keyring` 1.0.4) | Plaintext in keychain entry | +| Encryption key | AES key for accounts.json.enc | OS keychain, alias `account-metadata-key` | Base64 in keychain entry | + +### Login Flow (nsec) + +1. User pastes nsec in `LoginCard` (Paste Key tab) +2. `LoginCard.onLogin(keyInput)` called synchronously +3. `LoginScreen.onLogin` calls `accountManager.loginWithKey(keyInput)` -- **synchronous**, sets `_accountState` immediately +4. On success, `.map {}` fires `scope.launch` (fire-and-forget coroutine): + - `accountManager.saveCurrentAccount()` on `Dispatchers.IO` + - `onLoginSuccess()` on Main +5. `onLoginSuccess` fires another `scope.launch(Dispatchers.IO)`: + - `accountManager.ensureCurrentAccountInStorage()` + - `accountManager.refreshAccountList()` + +### Save Path (`saveCurrentAccount`) + +1. Reads `currentAccount()` from in-memory `_accountState` +2. Calls `secureStorage.savePrivateKey(npub, privKeyHex)` -- writes nsec to OS keychain +3. Calls `accountStorage.saveAccount(info)` -- updates in-memory cache + writes `accounts.json.enc` +4. Calls `accountStorage.setCurrentAccount(npub)` -- updates activeNpub in cache + writes file again + +### Restore Path (`loadSavedAccount`) + +1. Reads `accountStorage.currentAccount()` -- decrypts `accounts.json.enc`, gets `activeNpub` +2. Finds matching `AccountInfo` in account list +3. For `SignerType.Internal`: calls `secureStorage.getPrivateKey(npub)` -- reads from OS keychain +4. If privkey found: creates `KeyPair` + `NostrSignerInternal`, sets `_accountState` +5. If privkey null: falls back to read-only (view-only mode) + +## Root Cause Hypotheses (Ranked by Likelihood) + +### H1: Fire-and-Forget Save Never Completes (HIGH) + +**The save runs in a fire-and-forget coroutine that can be lost.** + +In `LoginScreen.kt` lines 97-103: +```kotlin +accountManager.loginWithKey(keyInput).map { + scope.launch { // <-- fire-and-forget + withContext(Dispatchers.IO) { accountManager.saveCurrentAccount() } + onLoginSuccess() + } +} +``` + +`loginWithKey` sets `_accountState = LoggedIn` synchronously. The UI immediately recomposes to show `MainContent`. The `scope` here is `rememberCoroutineScope()` from `LoginScreen` -- but `LoginScreen` is no longer composed (it was inside the `AccountState.LoggedOut` branch). When the composable leaves composition, `rememberCoroutineScope` cancels all launched coroutines. + +**Race condition:** `loginWithKey()` updates `_accountState` -> recomposition -> `LoginScreen` exits composition -> its `scope` is cancelled -> the save coroutine is cancelled before `saveCurrentAccount()` completes. + +The `onLoginSuccess` callback in `Main.kt` also tries `ensureCurrentAccountInStorage()`, but it uses a **different** `scope.launch(Dispatchers.IO)` from the `App` composable's scope. This should survive. But if `saveCurrentAccount()` was the only path that writes the private key to the keychain, and it got cancelled, then `ensureCurrentAccountInStorage()` only writes the metadata (npub + signerType) without the private key. On next startup, `loadInternalAccount` finds no privkey and falls back to read-only -- which looks like "session gone." + +**Verdict:** The nsec is likely written to metadata but the keychain save is cancelled. On restart, the account loads as read-only (no nsec) which may appear as "not persisted." + +### H2: Keychain Write Fails Silently (MEDIUM) + +`java-keyring` 1.0.4 uses macOS Security framework. Known issues: +- First-time keychain access prompts the user for permission -- if dismissed, the write silently fails (caught as `BackendNotSupportedException` or `PasswordAccessException`) +- Sandboxed apps may not have keychain entitlements +- `./gradlew :desktopApp:run` runs via Gradle daemon which may lack keychain access + +If `saveToKeyring` throws, `SecureKeyStorage.savePrivateKey` catches it and falls to `saveToFallback`, which requires a console password. In a GUI app launched via `./gradlew :desktopApp:run`, there IS no console. `getFallbackPassword()` calls `System.console()` which returns null, then calls `readLine()` which blocks on stdin. This could hang or throw. + +### H3: AES Key Regenerated Between Sessions (MEDIUM-LOW) + +The `accounts.json.enc` encryption key is stored in keychain under `account-metadata-key`. If H2 applies (keychain write fails), a NEW AES key is generated on each app launch. The file written with the old key becomes unreadable on next launch -- `readMetadataFromDisk` catches `AEADBadTagException`, backs up the corrupt file, and returns empty `AccountMetadata`. This means `currentAccount()` returns null -> "No saved account". + +### H4: `cachedMetadata` Race Between Concurrent Coroutines (LOW) + +`DesktopAccountStorage.cachedMetadata` is a plain `var` with no synchronization. The two save paths (`saveCurrentAccount` from LoginScreen's scope, `ensureCurrentAccountInStorage` from App's scope) could interleave reads/writes. One could overwrite the other's changes. But this would cause partial data loss, not complete failure. + +### H5: `accounts.json.enc` Written But AES Key Lost (LOW) + +If the app is killed between writing the file and the keychain entry being flushed, the AES key may not persist. On next startup, a new key is generated, old file is unreadable. + +## Key Files Involved + +| File | Role | +|------|------| +| `desktopApp/.../Main.kt` | Startup flow, `loadSavedAccount()` call, `onLoginSuccess` callback | +| `desktopApp/.../ui/LoginScreen.kt` | Login UI, calls `loginWithKey` + fire-and-forget save | +| `desktopApp/.../account/AccountManager.kt` | `loginWithKey`, `saveCurrentAccount`, `loadSavedAccount`, `ensureCurrentAccountInStorage` | +| `desktopApp/.../account/DesktopAccountStorage.kt` | Encrypted metadata file I/O, `accounts.json.enc` | +| `commons/.../keystorage/SecureKeyStorage.kt` (jvmMain) | OS keychain via `java-keyring`, fallback encrypted file | +| `desktopApp/.../ui/auth/LoginCard.kt` | UI form, calls `onLogin` synchronously | + +## Proposed Fixes + +### Fix 1: Make `loginWithKey` Save Inline (Addresses H1) + +Change `loginWithKey` to a `suspend fun` that saves the account before returning. This eliminates the fire-and-forget race entirely. + +```kotlin +// Before: loginWithKey is sync, save is fire-and-forget +fun loginWithKey(keyInput: String): Result + +// After: loginWithKey saves inline +suspend fun loginWithKey(keyInput: String): Result { + // ... create keyPair, signer, state ... + _accountState.value = state + saveCurrentAccount() // save inline, not fire-and-forget + return Result.success(state) +} +``` + +LoginScreen would call it from a coroutine: +```kotlin +onLogin = { keyInput -> + scope.launch(Dispatchers.IO) { + accountManager.loginWithKey(keyInput).fold( + onSuccess = { withContext(Dispatchers.Main) { onLoginSuccess() } }, + onFailure = { /* show error */ } + ) + } + Result.success(Unit) // immediate return to LoginCard +} +``` + +### Fix 2: Use App-Level Scope for Save (Addresses H1, simpler) + +Move the save coroutine to the `onLoginSuccess` callback (which uses App-level scope that survives recomposition) and ensure it saves the private key too: + +```kotlin +onLoginSuccess = { + scope.launch(Dispatchers.IO) { + accountManager.saveCurrentAccount() // <-- add this, saves privkey to keychain + accountManager.ensureCurrentAccountInStorage() + accountManager.refreshAccountList() + } +} +``` + +### Fix 3: Validate Keychain Access on Startup (Addresses H2/H3) + +Add a keychain health check at startup. Write a test value, read it back, delete it. If it fails, show a warning dialog instead of silently falling to broken fallback. + +### Fix 4: Add Mutex to DesktopAccountStorage (Addresses H4) + +Wrap `cachedMetadata` access in a `Mutex` to prevent concurrent read/write races. + +### Fix 5: Log Save Result (Diagnostic) + +In `LoginScreen.kt`, log the result of `saveCurrentAccount()` so failures are visible: +```kotlin +val result = accountManager.saveCurrentAccount() +if (result.isFailure) { + Log.e("LoginScreen", "Failed to save account", result.exceptionOrNull()) +} +``` + +## Unanswered Questions + +- Is the keychain prompt appearing on first login? User may be dismissing it. +- Is `./gradlew :desktopApp:run` the launch method? Gradle daemon may lack keychain entitlements. +- Does `~/.amethyst/accounts.json.enc` exist after login? If yes, metadata saved but privkey lost (H1/H2). If no, metadata never written (H1 complete cancellation). +- Is the app packaged (`.dmg`/`.deb`) or run from source? Packaging affects keychain access. +- Does the user see read-only mode on restart, or the login screen? Read-only = privkey lost. Login screen = metadata lost. +- Is there a `accounts.json.enc.corrupt.*` backup file in `~/.amethyst/`? If yes, H3 confirmed. diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt index 3aa67d8601..2797a26071 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt @@ -945,8 +945,11 @@ fun App( if (current?.signerType is com.vitorpamplona.amethyst.commons.model.account.SignerType.Remote) { accountManager.startHeartbeat(scope) } - // Ensure account is in multi-account storage + refresh list + // Save account (privkey to keychain + metadata to disk) + // then ensure multi-account storage is up to date. + // Uses App-level scope so it survives LoginScreen leaving composition. scope.launch(Dispatchers.IO) { + accountManager.saveCurrentAccount() accountManager.ensureCurrentAccountInStorage() accountManager.refreshAccountList() } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt index 082c9a9408..e390b2df22 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt @@ -41,7 +41,6 @@ import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember -import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -57,9 +56,6 @@ import com.vitorpamplona.amethyst.desktop.account.AccountState import com.vitorpamplona.amethyst.desktop.network.RelayStatus import com.vitorpamplona.amethyst.desktop.ui.auth.LoginCard import com.vitorpamplona.amethyst.desktop.ui.auth.NewKeyWarningCard -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import org.jetbrains.compose.resources.stringResource @Composable @@ -69,7 +65,7 @@ fun LoginScreen( ) { var showNewKeyDialog by remember { mutableStateOf(false) } var generatedAccount by remember { mutableStateOf(null) } - val scope = rememberCoroutineScope() + val loginProgress by accountManager.loginProgress.collectAsState() Column( @@ -96,10 +92,7 @@ fun LoginScreen( LoginCard( onLogin = { keyInput -> accountManager.loginWithKey(keyInput).map { - scope.launch { - withContext(Dispatchers.IO) { accountManager.saveCurrentAccount() } - onLoginSuccess() - } + onLoginSuccess() } }, onGenerateNew = { @@ -127,10 +120,7 @@ fun LoginScreen( nsec = account.nsec, onContinue = { showNewKeyDialog = false - scope.launch { - withContext(Dispatchers.IO) { accountManager.saveCurrentAccount() } - onLoginSuccess() - } + onLoginSuccess() }, ) }