mirror of
https://github.com/PR0M3TH3AN/bitvid.git
synced 2026-10-27 07:52:24 +00:00
[auth] Bug pass on login/signer/session surface: 4 fixes before promotion
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 <[email protected]>
This commit is contained in:
1 parent
b8e6921ac0
commit
4795d5edbd
6 files changed
+134
-35
No files matched your search
@@ -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) {
|
||||
|
||||
@@ -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,
|
||||
);
|
||||
}
|
||||
|
||||
+41
-4
@@ -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.
|
||||
*
|
||||
|
||||
@@ -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) {
|
||||
// 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();
|
||||
this.clearStoredSessionActor(clearTarget);
|
||||
}
|
||||
}
|
||||
|
||||
if (
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in new issue
Block a user