diff --git a/extension/src/background/ducks/session.ts b/extension/src/background/ducks/session.ts index aea5410407..d946fa1cf3 100644 --- a/extension/src/background/ducks/session.ts +++ b/extension/src/background/ducks/session.ts @@ -201,7 +201,19 @@ export const buildHasPrivateKeySelector = (localStore: DataStorageAccess) => const isHardwareWalletActive = await getIsHardwareWalletActive({ localStore, }); - if (isHardwareWalletActive && !session?.isHardwareWalletLocked) { + // A hardware-wallet session is unlocked only when it has not been + // idle-locked AND the session still holds a public key. The session + // store lives in `storage.session`, which the browser wipes on restart + // and on extension reload/update. After a wipe, `isHardwareWalletLocked` + // falls back to its `false` default, so without the public key check + // an empty session would read as unlocked. The popup would then show + // the unlock screen (no public key) while already treating the wallet + // as unlocked, and a correct password would never navigate away. + if ( + isHardwareWalletActive && + !session?.isHardwareWalletLocked && + !!session?.publicKey + ) { return true; } if (!session?.hashKey?.key) { diff --git a/extension/src/background/helpers/__tests__/session.test.ts b/extension/src/background/helpers/__tests__/session.test.ts index a8e77ce57c..a5414b6fed 100644 --- a/extension/src/background/helpers/__tests__/session.test.ts +++ b/extension/src/background/helpers/__tests__/session.test.ts @@ -219,7 +219,44 @@ describe("buildHasPrivateKeySelector", () => { } as any); await expect( - selector({ session: { isHardwareWalletLocked: false } } as any), + selector({ + session: { isHardwareWalletLocked: false, publicKey: "G-HW-KEY" }, + } as any), ).resolves.toBe(true); }); + + it("returns false for idle-locked hardware-wallet sessions", async () => { + mockGetIsHardwareWalletActive.mockResolvedValue(true); + const selector = buildHasPrivateKeySelector({ + getItem: jest.fn().mockResolvedValue(null), + } as any); + + await expect( + selector({ + session: { isHardwareWalletLocked: true, publicKey: "G-HW-KEY" }, + } as any), + ).resolves.toBe(false); + }); + + // The browser wipes `storage.session` on restart and on extension + // reload/update. The rehydrated session has no public key and + // `isHardwareWalletLocked` falls back to `false`. That empty session + // must read as locked, or the unlock screen never navigates away. + it("returns false for a wiped hardware-wallet session with no public key", async () => { + mockGetIsHardwareWalletActive.mockResolvedValue(true); + const selector = buildHasPrivateKeySelector({ + getItem: jest.fn().mockResolvedValue(null), + } as any); + + await expect( + selector({ + session: { + publicKey: "", + hashKey: { key: "" }, + allAccounts: [], + isHardwareWalletLocked: false, + }, + } as any), + ).resolves.toBe(false); + }); }); diff --git a/extension/src/background/messageListener/__tests__/hardwareWalletWipedSessionUnlock.test.ts b/extension/src/background/messageListener/__tests__/hardwareWalletWipedSessionUnlock.test.ts new file mode 100644 index 0000000000..21cf467f28 --- /dev/null +++ b/extension/src/background/messageListener/__tests__/hardwareWalletWipedSessionUnlock.test.ts @@ -0,0 +1,165 @@ +import { configureStore } from "@reduxjs/toolkit"; +import { combineReducers } from "redux"; + +import { sessionSlice } from "background/ducks/session"; +import { + HAS_ACCOUNT_SUBSCRIPTION, + KEY_ID, + KEY_ID_LIST, +} from "constants/localStorageTypes"; + +import { confirmPassword } from "../handlers/confirmPassword"; +import { loadAccount } from "../handlers/loadAccount"; + +// Persisting the store and broadcasting to other surfaces are side effects +// that are not under test here. +jest.mock("background/store", () => ({ + flushSessionStore: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock("../helpers/broadcast-session-state", () => ({ + broadcastSessionState: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock("@sentry/browser", () => ({ + captureException: jest.fn(), +})); + +const HW_PUBLIC_KEY = "GBHWLEDGERPUBLICKEY"; +const HW_KEY_ID = `hw:${HW_PUBLIC_KEY}`; +const HOT_KEY_ID = "hot-key-id-0"; +const HOT_PUBLIC_KEY = "GBHOTWALLETPUBLICKEY"; +const PASSWORD = "correct-password"; + +const buildLocalStore = (initial: Record) => { + const data: Record = { ...initial }; + return { + getItem: jest.fn((key: string) => Promise.resolve(data[key] ?? null)), + setItem: jest.fn((key: string, value: unknown) => { + data[key] = value; + return Promise.resolve(); + }), + remove: jest.fn((key: string) => { + delete data[key]; + return Promise.resolve(); + }), + }; +}; + +// Only the hot (mnemonic) keystore can be unlocked with a password. A +// hardware wallet key ID has no keystore, so loading it always fails. +const buildKeyManager = () => ({ + loadKey: jest.fn((keyId: string, password: string) => { + if (keyId === HOT_KEY_ID && password === PASSWORD) { + return Promise.resolve({ + publicKey: HOT_PUBLIC_KEY, + privateKey: "SHOTWALLETSECRET", + extra: { mnemonicPhrase: "test mnemonic phrase", imported: false }, + }); + } + return Promise.reject(new Error("Could not load key")); + }), +}); + +// A fresh store with no preloaded state is what the background rebuilds +// after the browser wipes `storage.session` (browser restart, extension +// reload or update): no public key, no hash key and +// `isHardwareWalletLocked: false`. +const buildWipedSessionStore = () => + configureStore({ + reducer: combineReducers({ session: sessionSlice.reducer }), + }); + +describe("unlocking a wiped session with a hardware wallet active", () => { + it("reports locked after the wipe, then unlocks the hardware wallet with the password", async () => { + const localStore = buildLocalStore({ + [KEY_ID]: HW_KEY_ID, + [KEY_ID_LIST]: [HOT_KEY_ID, HW_KEY_ID], + // Already subscribed, so `logIn` does not call the indexer. + [HAS_ACCOUNT_SUBSCRIPTION]: { [HW_KEY_ID]: true }, + }); + const sessionStore = buildWipedSessionStore(); + const keyManager = buildKeyManager(); + const sessionTimer = { + startSession: jest.fn().mockResolvedValue(undefined), + }; + + // 1. The wiped session must read as locked. Before the fix this + // returned `hasPrivateKey: true` with an empty public key, so the + // unlock screen mounted already "unlocked" and never navigated. + const beforeUnlock = await loadAccount({ + localStore: localStore as any, + sessionStore, + }); + expect(beforeUnlock.publicKey).toBe(""); + expect(beforeUnlock.hasPrivateKey).toBe(false); + + // 2. The correct password unlocks the session. + const unlockResult = await confirmPassword({ + request: { password: PASSWORD } as any, + localStore: localStore as any, + sessionStore, + keyManager: keyManager as any, + sessionTimer: sessionTimer as any, + }); + + expect(unlockResult).not.toHaveProperty("error"); + expect(unlockResult).toMatchObject({ + publicKey: HW_PUBLIC_KEY, + hasPrivateKey: true, + }); + + // The password is checked against the hot keystore, never against + // the hardware wallet key ID (which has no keystore). + expect(keyManager.loadKey.mock.calls[0]).toEqual([HOT_KEY_ID, PASSWORD]); + expect(keyManager.loadKey).not.toHaveBeenCalledWith( + HW_KEY_ID, + expect.anything(), + ); + expect(sessionTimer.startSession).toHaveBeenCalledTimes(1); + + // 3. The background now reports the hardware wallet as the unlocked + // active account, so `hasPrivateKey` goes false -> true and the + // unlock screen navigates. + const afterUnlock = await loadAccount({ + localStore: localStore as any, + sessionStore, + }); + expect(afterUnlock.publicKey).toBe(HW_PUBLIC_KEY); + expect(afterUnlock.hasPrivateKey).toBe(true); + expect(afterUnlock.allAccounts.map((a) => a.publicKey)).toEqual([ + HOT_PUBLIC_KEY, + HW_PUBLIC_KEY, + ]); + }); + + it("stays locked when the password is wrong", async () => { + const localStore = buildLocalStore({ + [KEY_ID]: HW_KEY_ID, + [KEY_ID_LIST]: [HOT_KEY_ID, HW_KEY_ID], + [HAS_ACCOUNT_SUBSCRIPTION]: { [HW_KEY_ID]: true }, + }); + const sessionStore = buildWipedSessionStore(); + const sessionTimer = { + startSession: jest.fn().mockResolvedValue(undefined), + }; + + const unlockResult = await confirmPassword({ + request: { password: "wrong-password" } as any, + localStore: localStore as any, + sessionStore, + keyManager: buildKeyManager() as any, + sessionTimer: sessionTimer as any, + }); + + expect(unlockResult).toEqual({ error: "Incorrect password" }); + expect(sessionTimer.startSession).not.toHaveBeenCalled(); + + const afterAttempt = await loadAccount({ + localStore: localStore as any, + sessionStore, + }); + expect(afterAttempt.publicKey).toBe(""); + expect(afterAttempt.hasPrivateKey).toBe(false); + }); +}); diff --git a/extension/src/background/messageListener/__tests__/userActivity.test.ts b/extension/src/background/messageListener/__tests__/userActivity.test.ts index 4d17bd3cce..c1d1f71769 100644 --- a/extension/src/background/messageListener/__tests__/userActivity.test.ts +++ b/extension/src/background/messageListener/__tests__/userActivity.test.ts @@ -1,3 +1,5 @@ +import { TEMPORARY_STORE_ID } from "constants/localStorageTypes"; + import { userActivity } from "../handlers/userActivity"; const mockGetIsHardwareWalletActive = jest.fn(); @@ -18,7 +20,12 @@ const makeSessionStore = (state: any) => getState: () => state, }) as any; -const makeLocalStore = () => ({}) as any; +const makeLocalStore = (temporaryStore: Record = {}) => + ({ + getItem: jest.fn((key: string) => + Promise.resolve(key === TEMPORARY_STORE_ID ? temporaryStore : null), + ), + }) as any; describe("userActivity handler", () => { beforeEach(() => { @@ -37,17 +44,36 @@ describe("userActivity handler", () => { const result = await userActivity({ sessionTimer, sessionStore, - localStore: makeLocalStore(), + localStore: makeLocalStore({ "key-id-0": "encrypted-blob" }), }); expect(result).toEqual({ ok: true }); expect(sessionTimer.resetSession).toHaveBeenCalledTimes(1); }); + it("does NOT reset the timer when the hash key exists but the temporary store is empty", async () => { + mockGetIsHardwareWalletActive.mockResolvedValue(false); + const sessionTimer = makeSessionTimer(); + const sessionStore = makeSessionStore({ + session: { + hashKey: { key: "abc" }, + isHardwareWalletLocked: false, + }, + }); + const result = await userActivity({ + sessionTimer, + sessionStore, + localStore: makeLocalStore({}), + }); + expect(result).toEqual({ ok: false }); + expect(sessionTimer.resetSession).not.toHaveBeenCalled(); + }); + it("resets the timer when a hardware wallet is active and unlocked", async () => { mockGetIsHardwareWalletActive.mockResolvedValue(true); const sessionTimer = makeSessionTimer(); const sessionStore = makeSessionStore({ session: { + publicKey: "GBHW", hashKey: { key: "" }, isHardwareWalletLocked: false, }, @@ -84,6 +110,7 @@ describe("userActivity handler", () => { const sessionTimer = makeSessionTimer(); const sessionStore = makeSessionStore({ session: { + publicKey: "GBHW", hashKey: { key: "" }, isHardwareWalletLocked: true, }, @@ -96,4 +123,26 @@ describe("userActivity handler", () => { expect(result).toEqual({ ok: false }); expect(sessionTimer.resetSession).not.toHaveBeenCalled(); }); + + // After the browser wipes `storage.session`, the session has no public + // key and `isHardwareWalletLocked` falls back to `false`. That session + // is locked, so activity must not rearm the idle alarm. + it("does NOT reset the timer for a wiped hardware-wallet session with no public key", async () => { + mockGetIsHardwareWalletActive.mockResolvedValue(true); + const sessionTimer = makeSessionTimer(); + const sessionStore = makeSessionStore({ + session: { + publicKey: "", + hashKey: { key: "" }, + isHardwareWalletLocked: false, + }, + }); + const result = await userActivity({ + sessionTimer, + sessionStore, + localStore: makeLocalStore(), + }); + expect(result).toEqual({ ok: false }); + expect(sessionTimer.resetSession).not.toHaveBeenCalled(); + }); }); diff --git a/extension/src/background/messageListener/handlers/userActivity.ts b/extension/src/background/messageListener/handlers/userActivity.ts index 6c50e1acbc..b4330b9458 100644 --- a/extension/src/background/messageListener/handlers/userActivity.ts +++ b/extension/src/background/messageListener/handlers/userActivity.ts @@ -1,11 +1,9 @@ import { Store } from "redux"; -import { getIsHardwareWalletActive } from "background/helpers/account"; import { DataStorageAccess } from "background/helpers/dataStorageAccess"; import { SessionTimer } from "background/helpers/session"; import { - hashKeySelector, - isHardwareWalletLockedSelector, + buildHasPrivateKeySelector, SessionState, } from "background/ducks/session"; @@ -28,8 +26,9 @@ import { * the idle alarm. A ping that arrives on a locked wallet is dropped: * there is no live session to extend. * - * Unlocked = either a hot-wallet session (`hashKey` is set) or an - * active hardware-wallet session that has not been idle-locked. + * "Unlocked" uses the same `buildHasPrivateKeySelector` rule as + * `loadAccount` and `saveSettings`, so the idle alarm and the popup + * never disagree about whether the wallet is unlocked. */ export const userActivity = async ({ sessionTimer, @@ -40,13 +39,12 @@ export const userActivity = async ({ sessionStore: Store; localStore: DataStorageAccess; }) => { - const state = sessionStore.getState() as SessionState; - const hashKey = hashKeySelector(state); - const hotUnlocked = !!hashKey?.key; - const isHwActive = await getIsHardwareWalletActive({ localStore }); - const hwUnlocked = isHwActive && !isHardwareWalletLockedSelector(state); + const hasPrivateKeySelector = buildHasPrivateKeySelector(localStore); + const isUnlocked = await hasPrivateKeySelector( + sessionStore.getState() as SessionState, + ); - if (!hotUnlocked && !hwUnlocked) { + if (!isUnlocked) { return { ok: false }; }