From 4795d5edbd35658f55f3a6804ab55b20c6ab1a88 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Thu, 2 Jul 2026 21:59:52 -0400 Subject: [PATCH] [auth] Bug pass on login/signer/session surface: 4 fixes before promotion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requested pre-promotion sweep of login methods, active signers, and anon session actors. Four real bugs found and fixed: 1. Logging out could wipe EVERY saved account's remembered nsec key. The no-arg clearStoredSessionActor() removed the whole per-account v2 map, and it was reachable from SignerManager.logout() (non-persisted nsec cleanup) and from the blocked-key cleanup in registerPrivateKeySigner — same bug class as the NIP-46 session-map wipe fixed earlier. Now: the no-arg form clears only the legacy v1 slot; logout targets the departing account (and skips entirely when it can't name one); the blocked-key cleanup targets the blocked pubkey (mirroring the already-targeted twin in unlockStoredSessionActor). Spec-corrected the test that had encoded the wipe-all as intended behavior. 2. Removing a saved profile left its keys on the device. The switcher's remove-account flow cleared NWC + the keep-unlocked cache but kept the remembered (encrypted) nsec key and the NIP-46 session. New client.forgetStoredAccount(pubkey) (SignerManager.forgetStoredSessionActor + clearStoredNip46Session) forgets both WITHOUT touching the active session's in-memory actors; wired into handleProfileLogoutRequest. 3. Logout never actually re-locked a kept-unlocked device. The forget read detail.pubkey / this.pubkey — both already nulled by authService.logout() (the detail only carries previousPubkey) — so the cached decrypted key silently survived logout. Now reads previousPubkey. 4. restoreUnlockedSigner bypassed access control: a since-blacklisted account's cached key would silently restore its signer. It now accepts a validator (mirroring unlockStoredSessionActor), forgets the cached key on denial, and both app-level call sites (_initAutoLogin, ensureEncryptionCapableSigner) pass an accessControl.canAccess gate. Also verified-safe during the pass (no action): logout's persisted-nsec keep logic under the ephemeral session-actor overwrite; NIP-46 logout keepStored; the ephemeral actor as a harmless extra DM-decrypt candidate; corrupt-map recovery clears; anon blacklist-seeding now always has a signing-capable actor. Tests: session-actor multi-account suite spec-corrected + re-verified (no-arg clear keeps per-account keys); 57 tests across session-actor / nip46 / unlocked-key / switch / signer-gate / feed-gate suites green. Lint + build green. Co-Authored-By: Claude Opus 4.8 --- js/app.js | 24 +++++++++++- js/app/authSessionCoordinator.js | 25 ++++++------ js/nostr/client.js | 45 ++++++++++++++++++++-- js/nostr/managers/SignerManager.js | 39 +++++++++++++++++-- js/nostr/sessionActor.js | 14 +++---- tests/session-actor-multi-account.test.mjs | 24 +++++++++--- 6 files changed, 135 insertions(+), 36 deletions(-) diff --git a/js/app.js b/js/app.js index 9acbf248..f9803039 100644 --- a/js/app.js +++ b/js/app.js @@ -1264,7 +1264,9 @@ class Application { // encrypted); safe no-op when nothing is cached or for NIP-07/46. try { if (typeof nostrClient.restoreUnlockedSigner === "function") { - await nostrClient.restoreUnlockedSigner(normalizedSaved); + await nostrClient.restoreUnlockedSigner(normalizedSaved, { + validator: this.buildAccessControlValidator(), + }); } } catch (error) { devLogger.warn( @@ -2186,7 +2188,9 @@ class Application { !isSignerCapable(resolveSignerCapabilities(getActiveSigner()), need) ) { try { - const restored = await nostrClient.restoreUnlockedSigner(normalized); + const restored = await nostrClient.restoreUnlockedSigner(normalized, { + validator: this.buildAccessControlValidator(), + }); if ( restored?.restored && isSignerCapable(resolveSignerCapabilities(getActiveSigner()), need) @@ -2260,6 +2264,22 @@ class Application { return { ok: true, unlocked: true }; } + // Access-control gate passed into keep-unlocked signer restores so a + // since-blocked account's cached key is forgotten instead of restoring. + buildAccessControlValidator() { + return (pk) => { + if ( + accessControl && + typeof accessControl.canAccess === "function" && + !accessControl.canAccess(pk) + ) { + const error = new Error("Access restricted for this account."); + error.code = "access-denied"; + throw error; + } + }; + } + // True when the active (or given) account currently has a cached "keep // unlocked" key — i.e. there is something to lock (TODO #51). isSessionKeptUnlocked(pubkey) { diff --git a/js/app/authSessionCoordinator.js b/js/app/authSessionCoordinator.js index af2d4902..ea3059e3 100644 --- a/js/app/authSessionCoordinator.js +++ b/js/app/authSessionCoordinator.js @@ -999,15 +999,14 @@ export function createAuthSessionCoordinator(deps) { clearWatchHistoryConversationKeyCache(); clearWatchHistoryDecryptedChunkCache(); - // Forget any cached "keep unlocked" key for the account being logged out, so - // logout actually re-locks it (TODO #51). + // Forget the cached "keep unlocked" key so logout re-locks (TODO #51). + // MUST read previousPubkey: authService.logout already nulled the active + // pubkey, so detail.pubkey/this.pubkey are empty here. try { - const loggedOutPubkey = detail?.pubkey || this.pubkey; - if ( - loggedOutPubkey && - typeof nostrClient.forgetUnlockedSigner === "function" - ) { - nostrClient.forgetUnlockedSigner(loggedOutPubkey); + const loggedOutPubkey = + detail?.previousPubkey || detail?.pubkey || this.pubkey; + if (loggedOutPubkey) { + nostrClient.forgetUnlockedSigner?.(loggedOutPubkey); } } catch (error) { devLogger.warn("Failed to forget cached unlock during logout:", error); @@ -1712,14 +1711,14 @@ export function createAuthSessionCoordinator(deps) { } } - // Removing a saved profile must also forget its cached "keep unlocked" key. + // A removed profile must not leave keys on the device: forget its cached + // "keep unlocked" key + stored credentials (nsec key, NIP-46 session). try { - if (typeof nostrClient.forgetUnlockedSigner === "function") { - nostrClient.forgetUnlockedSigner(normalizedTarget); - } + nostrClient.forgetUnlockedSigner?.(normalizedTarget); + nostrClient.forgetStoredAccount?.(normalizedTarget); } catch (error) { devLogger.warn( - "[Application] Failed to forget cached unlock for removed profile:", + "[Application] Failed to forget credentials for removed profile:", error, ); } diff --git a/js/nostr/client.js b/js/nostr/client.js index 4b75b729..41f72c4b 100644 --- a/js/nostr/client.js +++ b/js/nostr/client.js @@ -2539,9 +2539,10 @@ export class NostrClient { try { opts.validator(adapter.pubkey); } catch (error) { - // Don't leave a since-blocked key persisted on this device. + // Don't leave the since-blocked key persisted on this device. Targeted: + // only THIS account's entry — the no-arg clear wiped every saved key. try { - clearStoredSessionActorEntry(); + clearStoredSessionActorEntry(adapter.pubkey); } catch (cleanupError) { devLogger.warn( "[nostr] Failed to clear stored session after access denial:", @@ -2738,8 +2739,11 @@ export class NostrClient { // Restore an nsec signer from the "keep unlocked" cache without a passphrase // (TODO #51). Reads the cached decrypted key for `targetPubkey`, verifies it // derives to that pubkey (defense-in-depth; a mismatch forgets the cache), and - // registers it app-wide. Returns { restored: boolean }. - async restoreUnlockedSigner(targetPubkey) { + // registers it app-wide. Pass a `validator` (access-control gate, like + // unlockStoredSessionActor's) where available — a since-blocked account's + // cached key is then forgotten instead of silently restoring its signer. + // Returns { restored: boolean }. + async restoreUnlockedSigner(targetPubkey, { validator } = {}) { const pubkey = typeof targetPubkey === "string" && HEX64_REGEX.test(targetPubkey.trim().toLowerCase()) ? targetPubkey.trim().toLowerCase() @@ -2771,6 +2775,17 @@ export class NostrClient { return { restored: false, reason: "pubkey-mismatch" }; } + // Access-control gate (mirrors unlockStoredSessionActor): a since-blocked + // account must not silently restore its signer — forget its cached key. + if (typeof validator === "function") { + try { + validator(pubkey); + } catch (error) { + forgetUnlockedKey(pubkey); + return { restored: false, reason: "access-denied", error }; + } + } + try { const adapter = await this.registerPrivateKeySigner({ privateKey: privateKeyHex, @@ -3064,6 +3079,28 @@ export class NostrClient { return this.signerManager.clearStoredSessionActor(); } + // Forget ONE removed account's stored credentials (remembered nsec key + + // NIP-46 remote-signer session) without touching the active session. Used by + // the profile switcher's remove-account flow so a removed profile doesn't + // leave its keys on the device. + forgetStoredAccount(pubkey) { + const normalized = + typeof pubkey === "string" ? pubkey.trim().toLowerCase() : ""; + if (!normalized) { + return; + } + try { + this.signerManager.forgetStoredSessionActor(normalized); + } catch (error) { + devLogger.warn("[nostr] Failed to forget stored nsec key:", error); + } + try { + clearStoredNip46Session(normalized); + } catch (error) { + devLogger.warn("[nostr] Failed to forget stored NIP-46 session:", error); + } + } + /** * Logs out the current user and clears all session state. * diff --git a/js/nostr/managers/SignerManager.js b/js/nostr/managers/SignerManager.js index 43a819f9..e4e9b521 100644 --- a/js/nostr/managers/SignerManager.js +++ b/js/nostr/managers/SignerManager.js @@ -203,12 +203,36 @@ export class SignerManager { return newActor.pubkey; } - clearStoredSessionActor() { - clearStoredSessionActorEntry(); + // Forget a stored (remembered) key. Pass the account's pubkey — the no-arg + // form only clears the legacy v1 slot and must never be used to log out one + // account (it used to wipe EVERY saved account's key). + clearStoredSessionActor(pubkey) { + clearStoredSessionActorEntry(pubkey); this.sessionActor = null; this.lockedSessionActor = null; } + // Forget ONE account's remembered key WITHOUT touching the current session's + // in-memory actors (used when removing a non-active saved profile). Only drops + // the in-memory references if they belong to the removed account. + forgetStoredSessionActor(pubkey) { + const target = + typeof pubkey === "string" ? pubkey.trim().toLowerCase() : ""; + if (!target) { + return; + } + clearStoredSessionActorEntry(target); + const matches = (actor) => + typeof actor?.pubkey === "string" && + actor.pubkey.trim().toLowerCase() === target; + if (matches(this.lockedSessionActor)) { + this.lockedSessionActor = null; + } + if (matches(this.sessionActor) && !this.sessionActor?.privateKey) { + this.sessionActor = null; + } + } + async ensureActiveSignerForPubkey(pubkey) { const normalizedPubkey = typeof pubkey === "string" && pubkey.trim() @@ -1083,8 +1107,15 @@ export class SignerManager { previousSessionActor.persisted !== true; if (shouldClearStoredSession) { - this.lockedSessionActor = null; - this.clearStoredSessionActor(); + // Forget ONLY the departing account's remembered key. The no-arg clear + // used to wipe the whole per-account map, so logging out one non-persisted + // nsec login destroyed every OTHER saved account's key. Skip entirely when + // we can't name the account — never wipe-all implicitly. + const clearTarget = previousSessionActor.pubkey || previousPubkey || ""; + if (clearTarget) { + this.lockedSessionActor = null; + this.clearStoredSessionActor(clearTarget); + } } if ( diff --git a/js/nostr/sessionActor.js b/js/nostr/sessionActor.js index 18b98b99..1cba12bd 100644 --- a/js/nostr/sessionActor.js +++ b/js/nostr/sessionActor.js @@ -677,19 +677,19 @@ export function clearStoredSessionActor(pubkey) { return; } - clearStoredSessionActorIndexedDb(); - + // No target pubkey: clear ONLY the legacy v1 "last saved" slot. Wiping the + // per-account v2 map here deleted EVERY saved account's remembered nsec key — + // the no-arg callers were logout/teardown paths for ONE account, so logging + // out (or a blocked-key cleanup) silently destroyed the other saved accounts' + // keys and broke switching back to them. Same bug class as the NIP-46 + // session-map wipe. A specific account is forgotten via + // clearStoredSessionActor(pubkey), which remains targeted. if (typeof localStorage !== "undefined") { try { localStorage.removeItem(SESSION_ACTOR_STORAGE_KEY); } catch (error) { devLogger.warn("[nostr] Failed to clear stored session actor:", error); } - try { - localStorage.removeItem(SESSION_ACTORS_MAP_STORAGE_KEY); - } catch (error) { - devLogger.warn("[nostr] Failed to clear session actor map:", error); - } } } diff --git a/tests/session-actor-multi-account.test.mjs b/tests/session-actor-multi-account.test.mjs index a54cf89c..6b0eec7e 100644 --- a/tests/session-actor-multi-account.test.mjs +++ b/tests/session-actor-multi-account.test.mjs @@ -4,7 +4,7 @@ // key survived, so switching among saved nsec accounts silently failed. // // test_integrity_note: -// change_type: ["new_tests"] +// change_type: ["new_tests", "spec_correction"] // scenarios: // - id: SCN-nsec-multi-account-storage // given: "two nsec accounts each persist their encrypted key" @@ -83,15 +83,27 @@ test("no-arg read returns the last-saved account as the default", () => { assert.equal(readStoredSessionActorEntry().privateKeyEncrypted, "cipher-B"); }); -test("no-arg clear wipes every stored account (logout)", () => { +// spec_correction: the previous test asserted "no-arg clear wipes every stored +// account (logout)". That encoded a bug: the no-arg clear is reached from +// per-account paths (SignerManager.logout's non-persisted-nsec cleanup, the +// blocked-key cleanup in registerPrivateKeySigner), so logging out ONE account +// silently destroyed every OTHER saved account's remembered key and broke +// switching back to them — the same bug class as the NIP-46 session-map wipe. +// Correct behavior: no-arg clears only the legacy v1 "last saved" slot; the +// per-account v2 map survives. Forgetting a specific account uses the targeted +// clearStoredSessionActor(pubkey) (tested above). +test("no-arg clear drops the legacy v1 slot but KEEPS per-account saved keys", () => { persistSessionActor(actor(PUB_A, "cipher-A")); persistSessionActor(actor(PUB_B, "cipher-B")); - clearStoredSessionActor(); + clearStoredSessionActor(); // e.g. one account's logout cleanup - assert.deepEqual(listStoredSessionActorPubkeys(), []); - assert.equal(readStoredSessionActorEntry(PUB_A), null); - assert.equal(readStoredSessionActorEntry(), null); + // Both accounts remain switchable — their remembered keys survive. + assert.deepEqual(listStoredSessionActorPubkeys().sort(), [PUB_A, PUB_B].sort()); + assert.equal(readStoredSessionActorEntry(PUB_A).privateKeyEncrypted, "cipher-A"); + assert.equal(readStoredSessionActorEntry(PUB_B).privateKeyEncrypted, "cipher-B"); + // The legacy single-slot default is gone. + assert.equal(localStorage.getItem(SESSION_ACTOR_STORAGE_KEY), null); }); test("a legacy v1 single-slot entry migrates into the per-pubkey map", () => {