Address PR review feedback from @piyalbasu

Six concrete code issues raised on PR #2802:

1. signOut handler now dispatches lockHardwareWallet() alongside
   logOut() so HW sessions are locked on explicit sign-out. logOut
   resets state to initialState (isHardwareWalletLocked: false) but
   KEY_ID (hw: prefix) is intentionally not cleared, so without this
   the HW branch of buildHasPrivateKeySelector would report UNLOCKED
   after sign-out.

2. loginToAllAccounts's two catch blocks now rethrow after
   clearSession(). Previously execution fell through to
   startSession() + broadcastSessionState(SESSION_UNLOCKED), arming an
   idle alarm and telling every Freighter surface the wallet was
   unlocked even though it had just been cleared.

3. importHardwareWallet now arms the idle auto-lock alarm.
   popupMessageListener threads sessionTimer into the handler so
   HW-only sessions imported via this path are subject to the same
   auto-lock guarantees as hot-wallet sessions.

4. SessionLockListener wraps the SESSION_UNLOCKED fire-and-forget
   loadAccount in try/catch, and guards against an undefined response.
   Previously a rejection or undefined result could crash the surface
   with 'Cannot destructure property hasPrivateKey of undefined'.

5. SessionLockListener now dispatches lockAccount() unconditionally on
   SESSION_LOCKED; only the navigate is suppressed when the surface is
   already on /unlock-account. Without this, a surface parked on
   /unlock-account kept hasPrivateKey: true in redux while the
   background was locked, and ActivityTracker kept sending pings.

6. saveSettings now uses a precise SaveSettingsResponse type end-to-end
   (handler return, @shared/api client, popup duck thunk), removing
   the 'as unknown as typeof response' cast at the client boundary.
   sendMessageToBackground is generic over the response type so
   handler boundaries can be typed without per-call coercion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: Copilot

@shared/api/helpers/extensionMessaging.ts

@@ -71,17 +71,19 @@ export const sendMessageToContentScript = (msg: Msg): Promise<Response> => {
   });
 };
 
-export const sendMessageToBackground = async (msg: Msg): Promise<Response> => {
+export const sendMessageToBackground = async <T = Response>(
+  msg: Msg,
+): Promise<T> => {
   let res;
 
   if (DEV_SERVER) {
     // treat this as an external call because we're making the call from the browser, not the popup
     res = await sendMessageToContentScript(msg);
   } else {
-    res = (await browser.runtime.sendMessage(msg)) as Response;
+    res = await browser.runtime.sendMessage(msg);
   }
 
-  return res as Response;
+  return res as T;
 };
 
 export const FreighterApiNodeError = {

@shared/api/internal.ts

@@ -64,6 +64,7 @@ import {
   CollectibleContract,
   DiscoverData,
   RecentProtocolEntry,
+  SaveSettingsResponse,
 } from "./types";
 import {
   AccountBalancesInterface,
@@ -1630,43 +1631,35 @@ export const saveSettings = async ({
   isHideDustEnabled: boolean;
   isOpenSidebarByDefault: boolean;
   autoLockTimeoutMinutes: AutoLockTimeoutMinutes;
-}): Promise<Settings & IndexerSettings> => {
-  let response = {
+}): Promise<SaveSettingsResponse> => {
+  let response: SaveSettingsResponse = {
     allowList: DEFAULT_ALLOW_LIST,
     isDataSharingAllowed: false,
     networkDetails: MAINNET_NETWORK_DETAILS,
     networksList: DEFAULT_NETWORKS,
     isMemoValidationEnabled: true,
     isRpcHealthy: false,
-    userNotification: { enabled: false, message: "" },
-    settingsState: SettingsState.IDLE,
     isSorobanPublicEnabled: false,
     isNonSSLEnabled: false,
     isHideDustEnabled: true,
     isOpenSidebarByDefault: false,
     autoLockTimeoutMinutes: DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES,
-    error: "",
-    hiddenAssets: {},
   };
 
   try {
-    response = (await sendMessageToBackground({
+    response = await sendMessageToBackground<SaveSettingsResponse>({
       activePublicKey,
       isDataSharingAllowed,
       isMemoValidationEnabled,
       isHideDustEnabled,
       isOpenSidebarByDefault,
       autoLockTimeoutMinutes,
       type: SERVICE_TYPES.SAVE_SETTINGS,
-    })) as unknown as typeof response;
+    });
   } catch (e) {
     console.error(e);
   }
 
-  if (response.error) {
-    throw new Error(response.error);
-  }
-
   return response;
 };
 

@shared/api/types/types.ts

@@ -222,6 +222,20 @@ export interface IndexerSettings {
   userNotification: UserNotification;
 }
 
+export type SaveSettingsResponse = {
+  allowList: AllowList;
+  isDataSharingAllowed: boolean;
+  isMemoValidationEnabled: boolean;
+  networkDetails: NetworkDetails;
+  networksList: NetworkDetails[];
+  isRpcHealthy: boolean;
+  isSorobanPublicEnabled: boolean;
+  isNonSSLEnabled: boolean;
+  isHideDustEnabled: boolean;
+  isOpenSidebarByDefault: boolean;
+  autoLockTimeoutMinutes: AutoLockTimeoutMinutes;
+};
+
 export type Settings = {
   allowList: AllowList;
   networkDetails: NetworkDetails;

extension/src/background/messageListener/__tests__/importHardwareWallet.test.ts

@@ -0,0 +1,59 @@
+import { importHardwareWallet } from "../handlers/importHardwareWallet";
+
+const mockStoreHardwareWalletAccount = jest.fn().mockResolvedValue(undefined);
+const mockGetBipPath = jest.fn().mockResolvedValue("m/44'/148'/0'");
+
+jest.mock("../helpers/store-hardware-wallet", () => ({
+  storeHardwareWalletAccount: (...args: unknown[]) =>
+    mockStoreHardwareWalletAccount(...args),
+}));
+
+jest.mock("background/helpers/account", () => ({
+  getBipPath: (...args: unknown[]) => mockGetBipPath(...args),
+  getIsHardwareWalletActive: jest.fn().mockResolvedValue(true),
+}));
+
+describe("importHardwareWallet handler", () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  // Regression: HW-only imports flip the session into the "HW-active"
+  // state where `buildHasPrivateKeySelector` reports the wallet as
+  // unlocked, but the handler previously did not arm the idle
+  // auto-lock alarm. HW-only sessions imported via this path had no
+  // auto-lock at all, contradicting the PR's stated security goal.
+  it("arms the idle auto-lock alarm after storing the HW account", async () => {
+    const sessionStore = {
+      dispatch: jest.fn(),
+      getState: jest.fn().mockReturnValue({
+        session: { publicKey: "GBHW", allAccounts: [], isHardwareWalletLocked: false },
+      }),
+    } as any;
+    const localStore = {
+      getItem: jest.fn().mockResolvedValue("hw:GBHW"),
+    } as any;
+    const sessionTimer = {
+      startSession: jest.fn().mockResolvedValue(undefined),
+    } as any;
+
+    await importHardwareWallet({
+      request: {
+        publicKey: "GBHW",
+        hardwareWalletType: "Ledger",
+        bipPath: "m/44'/148'/0'",
+      } as any,
+      sessionStore,
+      localStore,
+      sessionTimer,
+    });
+
+    expect(mockStoreHardwareWalletAccount).toHaveBeenCalledTimes(1);
+    expect(sessionTimer.startSession).toHaveBeenCalledTimes(1);
+    // Account is stored before the timer arms so the alarm fires
+    // relative to an already-active HW session, not a future one.
+    expect(
+      mockStoreHardwareWalletAccount.mock.invocationCallOrder[0],
+    ).toBeLessThan(sessionTimer.startSession.mock.invocationCallOrder[0]);
+  });
+});

extension/src/background/messageListener/__tests__/login-all-accounts.test.ts

@@ -113,4 +113,80 @@ describe("loginToAllAccounts", () => {
       mockFlushSessionStore.mock.invocationCallOrder[0],
     ).toBeLessThan(mockBroadcastSessionState.mock.invocationCallOrder[0]);
   });
+
+  // Regression: the two catch blocks call `clearSession()` (which wipes
+  // hashKey, sets `isHardwareWalletLocked=true`, removes
+  // TEMPORARY_STORE_ID), but previously did not return/throw. Execution
+  // continued to the unconditional `startSession()` + `flushSessionStore()`
+  // + `broadcastSessionState(SESSION_UNLOCKED)` at the bottom, telling
+  // every Freighter surface the wallet was unlocked even though it had
+  // just been cleared. Both catches must rethrow so the caller knows the
+  // unlock failed and the unlock broadcast does not fire.
+  it("rethrows and does not broadcast SESSION_UNLOCKED when storing the active mnemonic fails", async () => {
+    mockStoreEncryptedTemporaryData.mockReset();
+    mockStoreEncryptedTemporaryData.mockRejectedValueOnce(
+      new Error("storage offline"),
+    );
+
+    const localStore = {
+      getItem: jest.fn().mockResolvedValue(""),
+      remove: jest.fn().mockResolvedValue(undefined),
+    } as any;
+    const sessionStore = {
+      dispatch: jest.fn().mockResolvedValue(undefined),
+      getState: jest
+        .fn()
+        .mockReturnValue({ session: { publicKey: "", allAccounts: [] } }),
+    } as any;
+    const sessionTimer = {
+      startSession: jest.fn().mockResolvedValue(undefined),
+    } as any;
+
+    await expect(
+      loginToAllAccounts(
+        "password",
+        localStore,
+        sessionStore,
+        {} as any,
+        sessionTimer,
+      ),
+    ).rejects.toThrow("storage offline");
+
+    expect(mockClearSession).toHaveBeenCalledTimes(1);
+    expect(sessionTimer.startSession).not.toHaveBeenCalled();
+    expect(mockBroadcastSessionState).not.toHaveBeenCalled();
+  });
+
+  it("rethrows and does not broadcast SESSION_UNLOCKED when storing the active hash key fails", async () => {
+    mockStoreActiveHashKey.mockReset();
+    mockStoreActiveHashKey.mockRejectedValueOnce(new Error("hash key write"));
+
+    const localStore = {
+      getItem: jest.fn().mockResolvedValue(""),
+      remove: jest.fn().mockResolvedValue(undefined),
+    } as any;
+    const sessionStore = {
+      dispatch: jest.fn().mockResolvedValue(undefined),
+      getState: jest
+        .fn()
+        .mockReturnValue({ session: { publicKey: "", allAccounts: [] } }),
+    } as any;
+    const sessionTimer = {
+      startSession: jest.fn().mockResolvedValue(undefined),
+    } as any;
+
+    await expect(
+      loginToAllAccounts(
+        "password",
+        localStore,
+        sessionStore,
+        {} as any,
+        sessionTimer,
+      ),
+    ).rejects.toThrow("hash key write");
+
+    expect(mockClearSession).toHaveBeenCalledTimes(1);
+    expect(sessionTimer.startSession).not.toHaveBeenCalled();
+    expect(mockBroadcastSessionState).not.toHaveBeenCalled();
+  });
 });

extension/src/background/messageListener/__tests__/signOut.test.ts

@@ -1,5 +1,6 @@
 import { SERVICE_TYPES } from "@shared/constants/services";
 
+import { lockHardwareWallet, logOut } from "background/ducks/session";
 import { signOut } from "../handlers/signOut";
 
 const mockFlushSessionStore = jest.fn().mockResolvedValue(undefined);
@@ -59,4 +60,47 @@ describe("signOut handler", () => {
       localStore.remove.mock.invocationCallOrder[0],
     ).toBeLessThan(mockBroadcastSessionState.mock.invocationCallOrder[0]);
   });
+
+  // Regression: `logOut` resets session state to `initialState`, which
+  // sets `isHardwareWalletLocked: false`. `KEY_ID` (`hw:…` prefix) is
+  // intentionally not cleared on sign-out, so
+  // `getIsHardwareWalletActive` still returns `true` afterwards — which
+  // means `buildHasPrivateKeySelector`'s HW branch would falsely
+  // report the wallet UNLOCKED for HW users after an explicit
+  // sign-out. The handler must also dispatch `lockHardwareWallet()`.
+  it("dispatches lockHardwareWallet alongside logOut so HW sessions are locked on sign-out", async () => {
+    const localStore = {
+      getItem: jest.fn().mockResolvedValue("MNEMONIC_PHRASE_CONFIRMED"),
+      remove: jest.fn().mockResolvedValue(undefined),
+    } as any;
+    const sessionStore = {
+      dispatch: jest.fn(),
+      getState: jest.fn().mockReturnValue({
+        session: { publicKey: "" },
+      }),
+    } as any;
+    const sessionTimer = {
+      stopSession: jest.fn().mockResolvedValue(undefined),
+    } as any;
+
+    await signOut({ localStore, sessionStore, sessionTimer });
+
+    const dispatched = sessionStore.dispatch.mock.calls.map(
+      (c: unknown[]) => c[0],
+    );
+    expect(dispatched).toEqual(
+      expect.arrayContaining([logOut(), lockHardwareWallet()]),
+    );
+    // lockHardwareWallet must run *after* logOut so logOut's
+    // initialState reset (which sets isHardwareWalletLocked: false)
+    // does not undo the HW-lock flag.
+    const logOutIdx = dispatched.findIndex(
+      (a: { type?: string }) => a?.type === logOut.type,
+    );
+    const lockHwIdx = dispatched.findIndex(
+      (a: { type?: string }) => a?.type === lockHardwareWallet.type,
+    );
+    expect(logOutIdx).toBeGreaterThanOrEqual(0);
+    expect(lockHwIdx).toBeGreaterThan(logOutIdx);
+  });
 });

extension/src/background/messageListener/handlers/importHardwareWallet.ts

@@ -9,15 +9,18 @@ import {
   publicKeySelector,
 } from "background/ducks/session";
 import { getBipPath } from "background/helpers/account";
+import { SessionTimer } from "background/helpers/session";
 
 export const importHardwareWallet = async ({
   request,
   sessionStore,
   localStore,
+  sessionTimer,
 }: {
   request: ImportHardWareWalletMessage;
   sessionStore: Store;
   localStore: DataStorageAccess;
+  sessionTimer: SessionTimer;
 }) => {
   const { publicKey, hardwareWalletType, bipPath } = request;
 
@@ -28,6 +31,12 @@ export const importHardwareWallet = async ({
     sessionStore,
     localStore,
   });
+  // Importing a hardware wallet flips the session into the "HW-active"
+  // state where `buildHasPrivateKeySelector` reports the wallet as
+  // unlocked. Arm the idle auto-lock alarm here so HW-only sessions
+  // are subject to the same idle-lock guarantees as hot-wallet
+  // sessions — otherwise an HW-only import has no auto-lock at all.
+  await sessionTimer.startSession();
   const hasPrivateKeySelector = buildHasPrivateKeySelector(localStore);
 
   return {

extension/src/background/messageListener/handlers/saveSettings.ts

@@ -1,6 +1,7 @@
 import { Store } from "redux";
 
 import { SaveSettingsMessage } from "@shared/api/types/message-request";
+import { SaveSettingsResponse } from "@shared/api/types/types";
 import { coerceAutoLockTimeoutMinutes } from "@shared/constants/autoLock";
 import {
   getAllowList,
@@ -35,7 +36,7 @@ export const saveSettings = async ({
   localStore: DataStorageAccess;
   sessionStore: Store;
   sessionTimer: SessionTimer;
-}) => {
+}): Promise<SaveSettingsResponse> => {
   const {
     isDataSharingAllowed,
     isMemoValidationEnabled,

extension/src/background/messageListener/handlers/signOut.ts

@@ -1,7 +1,11 @@
 import { Store } from "redux";
 import { SERVICE_TYPES } from "@shared/constants/services";
 
-import { logOut, publicKeySelector } from "background/ducks/session";
+import {
+  lockHardwareWallet,
+  logOut,
+  publicKeySelector,
+} from "background/ducks/session";
 import { DataStorageAccess } from "background/helpers/dataStorageAccess";
 import { SessionTimer } from "background/helpers/session";
 import { broadcastSessionState } from "../helpers/broadcast-session-state";
@@ -27,6 +31,14 @@ export const signOut = async ({
   // state and emit a duplicate SESSION_LOCKED broadcast.
   await sessionTimer.stopSession();
   sessionStore.dispatch(logOut());
+  // `logOut` resets session state to `initialState`, which sets
+  // `isHardwareWalletLocked: false`. The KEY_ID (`hw:…` prefix) is
+  // intentionally not cleared on sign-out (so the HW account can be
+  // re-unlocked without re-importing), which means
+  // `getIsHardwareWalletActive` still reports `true`. Without this
+  // dispatch, `buildHasPrivateKeySelector`'s HW branch would report
+  // the wallet UNLOCKED for HW users immediately after sign-out.
+  sessionStore.dispatch(lockHardwareWallet());
   await flushSessionStore(sessionStore);
   await localStore.remove(TEMPORARY_STORE_ID);
   await broadcastSessionState(SERVICE_TYPES.SESSION_LOCKED);

extension/src/background/messageListener/helpers/login-all-accounts.ts

@@ -98,6 +98,11 @@ export const loginToAllAccounts = async (
     captureException(
       `Error storing encrypted temporary data: ${JSON.stringify(e)}`,
     );
+    // Rethrow so we don't fall through to startSession() +
+    // broadcastSessionState(SESSION_UNLOCKED) below, which would tell
+    // every Freighter surface the wallet is unlocked even though
+    // clearSession() just wiped it.
+    throw e;
   }
 
   for (let i = 0; i < keyIdList.length; i += 1) {
@@ -135,6 +140,10 @@ export const loginToAllAccounts = async (
   } catch (e) {
     await clearSession({ localStore, sessionStore });
     captureException(`Error storing active hash key: ${JSON.stringify(e)}`);
+    // Rethrow so we don't fall through to startSession() +
+    // broadcastSessionState(SESSION_UNLOCKED) below — the session was
+    // just cleared and the wallet must not be reported as unlocked.
+    throw e;
   }
 
   // start the timer now that we have active private key