Address review feedback: scope-drift cleanup and small hardening

Addresses the consolidated review asks from PR #2802 rounds 1-3,
all of which are local cleanups; no externally-observable behavior
change beyond the two flagged hardening fixes (saveSettings
validation drop and handleSignedHwPayload rearm narrowing).

userActivity.ts handler:
  Rewrite the doc comment to match the shipped behavior. The previous
  text still described an unconditional surface-mount ping that was
  removed in 7b3e4dd for the security reason captured in that
  commit message (dApp-spawned signing popups would otherwise extend
  the alarm without user presence).

saveSettings.ts handler:
  Drop the {error: 'Invalid autoLockTimeoutMinutes'} early-return.
  The response type has no error variant the popup could surface,
  and the only producer is a <Select> populated from
  VALID_AUTO_LOCK_TIMEOUT_MINUTES and coerced before dispatch.
  Coerce malformed input to the default instead, matching what the
  storage-read path already does. Tests updated accordingly.

handleSignedHwPayload.ts handler:
  Move sessionTimer.resetSession() into the success branch. A
  malformed request (missing uuid, or no matching queue entry) is
  never a legitimate signal of user presence and must not extend
  the idle deadline. Tests flipped + a new no-matching-uuid case
  added.

signOut.ts handler:
  Stop the auto-lock alarm BEFORE mutating session state, flushing,
  and removing the temporary store. Eliminates the race window
  where a pending alarm could fire clearSession on already-cleared
  state and emit a duplicate SESSION_LOCKED broadcast. Test asserts
  the full new ordering: stop < dispatch < flush < remove <
  broadcast.

broadcast-session-state.ts:
  Replace bare 'catch {}' with a catch that warns on errors which
  are NOT the well-known 'no receivers' patterns. The common case
  (no UI surfaces open) stays silent; genuine broadcast failures
  are no longer invisible.

SessionLockListener:
  Move 'location' into a useRef so the runtime.onMessage listener
  doesn't re-register on every navigation. Functionally identical
  (cleanup unregisters first), but avoids re-binding the listener
  on a hot path.

loadSaveSettings.test.ts:
  Drop the entire unreferenced browser.alarms shim (alarmsGet /
  alarmsCreate / alarmsClear and the beforeEach that installs them
  on the polyfill). The current saveSettings handler operates
  entirely through the injected sessionTimer mock; the shim was
  scaffolding from the removed elapsed-idle short-circuit path.
  Also remove both '(result as any).wasLocked).toBeUndefined()'
  assertions, since the response type no longer admits the field,
  and rewrite the shortened-timeout test comment to describe the
  current save-as-activity behavior rather than the removed
  immediate-lock branch.

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

Author: Copilot

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

@@ -42,7 +42,7 @@ describe("handleSignedHwPayload", () => {
     expect(responseQueue).toEqual([]);
   });
 
-  it("still records user activity before returning an error for a missing uuid", async () => {
+  it("does NOT extend the idle alarm when the uuid is missing", async () => {
     const sessionTimer = makeSessionTimer();
 
     const result = await handleSignedHwPayload({
@@ -51,10 +51,25 @@ describe("handleSignedHwPayload", () => {
       sessionTimer,
     });
 
-    expect(sessionTimer.resetSession).toHaveBeenCalledTimes(1);
+    // A malformed request is never a legitimate signal of user
+    // presence — only the success branch rearms the idle timer.
+    expect(sessionTimer.resetSession).not.toHaveBeenCalled();
     expect(result).toEqual({ error: "Transaction not found" });
   });
 
+  it("does NOT extend the idle alarm when no queue entry matches", async () => {
+    const sessionTimer = makeSessionTimer();
+
+    const result = await handleSignedHwPayload({
+      request: { uuid: "uuid-missing", signedPayload: "signed-xdr" } as any,
+      responseQueue: [{ uuid: "uuid-other", response: jest.fn() }] as any,
+      sessionTimer,
+    });
+
+    expect(sessionTimer.resetSession).not.toHaveBeenCalled();
+    expect(result).toEqual({ error: "Session timed out" });
+  });
+
   it("tracks hardware-wallet lock and unlock state transitions", () => {
     let state = { session: sessionSlice.reducer(undefined, { type: "init" }) };
     expect(isHardwareWalletLockedSelector(state)).toBe(false);

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

@@ -6,22 +6,6 @@ import {
   TEMPORARY_STORE_ID,
 } from "constants/localStorageTypes";
 import { DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES } from "@shared/constants/autoLock";
-import browser from "webextension-polyfill";
-
-const alarmsGet = jest.fn();
-const alarmsCreate = jest.fn().mockResolvedValue(undefined);
-const alarmsClear = jest.fn().mockResolvedValue(undefined);
-
-beforeEach(() => {
-  alarmsGet.mockReset();
-  alarmsCreate.mockClear();
-  alarmsClear.mockClear();
-  (browser as any).alarms = {
-    get: alarmsGet,
-    create: alarmsCreate,
-    clear: alarmsClear,
-  };
-});
 
 jest.mock("background/helpers/account", () => ({
   getAllowList: jest.fn().mockResolvedValue([]),
@@ -212,26 +196,38 @@ describe("saveSettings autoLockTimeoutMinutes", () => {
     isOpenSidebarByDefault: false,
   };
 
-  it("rejects invalid autoLockTimeoutMinutes values", async () => {
+  it("coerces invalid autoLockTimeoutMinutes to the default rather than rejecting", async () => {
+    // The Preferences `<Select>` only emits values from
+    // `VALID_AUTO_LOCK_TIMEOUT_MINUTES`, but a malformed message (e.g.
+    // from a future client revision) should still produce a sensible
+    // stored value rather than silently dropping the whole save.
     const localStore = makeLocalStore();
     const result = await saveSettings({
       request: { ...baseRequest, autoLockTimeoutMinutes: 7 } as any,
       localStore,
       sessionStore: makeSessionStore(),
       sessionTimer: makeSessionTimer(),
     });
-    expect((result as any).error).toBe("Invalid autoLockTimeoutMinutes");
-    expect(localStore.setItem).not.toHaveBeenCalled();
+    expect((result as any).error).toBeUndefined();
+    expect(localStore.setItem).toHaveBeenCalledWith(
+      AUTO_LOCK_TIMEOUT_MINUTES_ID,
+      DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES,
+    );
   });
 
-  it("rejects non-numeric autoLockTimeoutMinutes", async () => {
+  it("coerces non-numeric autoLockTimeoutMinutes to the default", async () => {
+    const localStore = makeLocalStore();
     const result = await saveSettings({
       request: { ...baseRequest, autoLockTimeoutMinutes: "15" } as any,
-      localStore: makeLocalStore(),
+      localStore,
       sessionStore: makeSessionStore(),
       sessionTimer: makeSessionTimer(),
     });
-    expect((result as any).error).toBe("Invalid autoLockTimeoutMinutes");
+    expect((result as any).error).toBeUndefined();
+    expect(localStore.setItem).toHaveBeenCalledWith(
+      AUTO_LOCK_TIMEOUT_MINUTES_ID,
+      DEFAULT_AUTO_LOCK_TIMEOUT_MINUTES,
+    );
   });
 
   it("persists a valid timeout and reschedules when unlocked", async () => {
@@ -251,7 +247,6 @@ describe("saveSettings autoLockTimeoutMinutes", () => {
     );
     expect(sessionTimer.resetSession).toHaveBeenCalledTimes(1);
     expect((result as any).autoLockTimeoutMinutes).toBe(30);
-    expect((result as any).wasLocked).toBeUndefined();
   });
 
   it("does not reschedule the timer when the wallet is locked", async () => {
@@ -266,27 +261,21 @@ describe("saveSettings autoLockTimeoutMinutes", () => {
     expect(sessionTimer.stopSession).not.toHaveBeenCalled();
   });
 
-  it("rearms (rather than locking) when the user shortens the timeout", async () => {
-    // Saving settings is itself a user action, so shortening the
-    // timeout should restart the idle clock with the new value rather
-    // than synthesizing an immediate lock — even when the new threshold
-    // is already smaller than the elapsed idle time of the in-flight
-    // alarm. Set up `alarmsGet` to return an alarm whose remaining time
-    // (1 min) is much less than the new 5 min timeout, i.e. elapsed
-    // idle (59 min) ≫ new timeout (5 min). The previous implementation
-    // would have detected this and locked immediately; the current
-    // implementation must simply rearm.
-    alarmsGet.mockResolvedValue({
-      scheduledTime: Date.now() + 1 * 60_000,
-    });
+  it("treats a shortened timeout as user activity and rearms (does not lock immediately)", async () => {
+    // Saving settings is itself a user action: the handler always
+    // rearms the idle timer with the new timeout when the wallet is
+    // unlocked. There is no immediate-lock branch even when the new
+    // threshold is far below the elapsed idle time — the popup never
+    // sees a `wasLocked` flag and the session store is never mutated
+    // from this path.
     const localStore = makeLocalStore(60);
     const sessionTimer = makeSessionTimer();
     const sessionStore = {
       getState: () => ({ session: { hashKey: { key: "k" } } }),
       dispatch: jest.fn(),
     } as any;
 
-    const result = await saveSettings({
+    await saveSettings({
       request: { ...baseRequest, autoLockTimeoutMinutes: 5 } as any,
       localStore,
       sessionStore,
@@ -296,7 +285,6 @@ describe("saveSettings autoLockTimeoutMinutes", () => {
     expect(sessionTimer.resetSession).toHaveBeenCalledTimes(1);
     expect(sessionTimer.stopSession).not.toHaveBeenCalled();
     expect(sessionStore.dispatch).not.toHaveBeenCalled();
-    expect((result as any).wasLocked).toBeUndefined();
   });
 });
 

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

@@ -19,7 +19,7 @@ describe("signOut handler", () => {
     jest.clearAllMocks();
   });
 
-  it("flushes the session store before clearing temporary storage and broadcasting the lock", async () => {
+  it("stops the alarm before mutating session state and broadcasts the lock", async () => {
     const localStore = {
       getItem: jest.fn().mockResolvedValue("MNEMONIC_PHRASE_CONFIRMED"),
       remove: jest.fn().mockResolvedValue(undefined),
@@ -36,16 +36,27 @@ describe("signOut handler", () => {
 
     await signOut({ localStore, sessionStore, sessionTimer });
 
+    expect(sessionTimer.stopSession).toHaveBeenCalledTimes(1);
     expect(mockFlushSessionStore).toHaveBeenCalledWith(sessionStore);
     expect(localStore.remove).toHaveBeenCalledTimes(1);
     expect(mockBroadcastSessionState).toHaveBeenCalledWith(
       SERVICE_TYPES.SESSION_LOCKED,
     );
+
+    // stopSession must run before any state mutation so a pending
+    // alarm can't fire `clearSession` (and emit a duplicate
+    // SESSION_LOCKED broadcast) between the dispatch and the clear.
+    expect(
+      sessionTimer.stopSession.mock.invocationCallOrder[0],
+    ).toBeLessThan(sessionStore.dispatch.mock.invocationCallOrder[0]);
+    expect(
+      sessionStore.dispatch.mock.invocationCallOrder[0],
+    ).toBeLessThan(mockFlushSessionStore.mock.invocationCallOrder[0]);
     expect(
       mockFlushSessionStore.mock.invocationCallOrder[0],
     ).toBeLessThan(localStore.remove.mock.invocationCallOrder[0]);
     expect(
-      mockBroadcastSessionState.mock.invocationCallOrder[0],
-    ).toBeGreaterThan(mockFlushSessionStore.mock.invocationCallOrder[0]);
+      localStore.remove.mock.invocationCallOrder[0],
+    ).toBeLessThan(mockBroadcastSessionState.mock.invocationCallOrder[0]);
   });
 });

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

@@ -18,13 +18,6 @@ export const handleSignedHwPayload = async ({
 }) => {
   const { signedPayload, uuid } = request;
 
-  // A user just completed a hardware-wallet signature — that is a real
-  // user action, so extend the idle session. Without this the popup
-  // ping is the only path that refreshes the alarm, and a slow HW
-  // signing flow could outlast the timeout while the user is actively
-  // working.
-  await sessionTimer.resetSession();
-
   if (!uuid) {
     captureException("handleSignedHwPayload: missing uuid in request");
     return { error: "Transaction not found" };
@@ -40,6 +33,12 @@ export const handleSignedHwPayload = async ({
     transactionResponse &&
     typeof transactionResponse.response === "function"
   ) {
+    // A user just completed a hardware-wallet signature — that is a
+    // real user action, so extend the idle session. We only rearm on
+    // the success branch; a malformed request or a missing queue
+    // entry is never a legitimate signal of user presence and must
+    // not be allowed to extend the deadline.
+    await sessionTimer.resetSession();
     transactionResponse.response(signedPayload);
     return {};
   }

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

@@ -1,10 +1,7 @@
 import { Store } from "redux";
 
 import { SaveSettingsMessage } from "@shared/api/types/message-request";
-import {
-  coerceAutoLockTimeoutMinutes,
-  isValidAutoLockTimeoutMinutes,
-} from "@shared/constants/autoLock";
+import { coerceAutoLockTimeoutMinutes } from "@shared/constants/autoLock";
 import {
   getAllowList,
   getFeatureFlags,
@@ -47,9 +44,16 @@ export const saveSettings = async ({
     autoLockTimeoutMinutes,
   } = request;
 
-  if (!isValidAutoLockTimeoutMinutes(autoLockTimeoutMinutes)) {
-    return { error: "Invalid autoLockTimeoutMinutes" };
-  }
+  // `autoLockTimeoutMinutes` originates from the Preferences `<Select>`,
+  // whose options are populated from `VALID_AUTO_LOCK_TIMEOUT_MINUTES`
+  // and coerced through `coerceAutoLockTimeoutMinutes` before dispatch.
+  // We coerce again here as defence-in-depth: a malformed value (e.g.
+  // from a future client revision or a corrupted message) is clamped to
+  // the default rather than rejected, since the response type has no
+  // error variant the popup could surface to the user.
+  const safeAutoLockTimeoutMinutes = coerceAutoLockTimeoutMinutes(
+    autoLockTimeoutMinutes,
+  );
 
   await localStore.setItem(DATA_SHARING_ID, isDataSharingAllowed);
   await localStore.setItem(IS_VALIDATING_MEMO_ID, isMemoValidationEnabled);
@@ -60,7 +64,7 @@ export const saveSettings = async ({
   );
   await localStore.setItem(
     AUTO_LOCK_TIMEOUT_MINUTES_ID,
-    autoLockTimeoutMinutes,
+    safeAutoLockTimeoutMinutes,
   );
 
   // Saving settings is itself a user action, so it counts as activity:

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

@@ -20,12 +20,15 @@ export const signOut = async ({
   sessionStore: Store;
   sessionTimer: SessionTimer;
 }) => {
+  // Cancel any pending auto-lock alarm FIRST — the wallet is being
+  // locked explicitly, so the idle timer no longer needs to fire.
+  // Clearing before mutating state eliminates the race window where
+  // a pending alarm could fire `clearSession` on already-cleared
+  // state and emit a duplicate SESSION_LOCKED broadcast.
+  await sessionTimer.stopSession();
   sessionStore.dispatch(logOut());
   await flushSessionStore(sessionStore);
   await localStore.remove(TEMPORARY_STORE_ID);
-  // Cancel any pending auto-lock alarm — the wallet is being locked
-  // explicitly, so the idle timer no longer needs to fire.
-  await sessionTimer.stopSession();
   await broadcastSessionState(SERVICE_TYPES.SESSION_LOCKED);
 
   return {

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

@@ -12,19 +12,21 @@ import {
 /**
  * Handle a USER_ACTIVITY ping from an extension page.
  *
- * Pings come from two sources inside `useActivityPing`:
- *  1. An unconditional mount ping fired by every Freighter surface
- *     (popup, sidebar, fullscreen) as it loads.
- *  2. Throttled user-input events while the popup believes the wallet
- *     is unlocked.
+ * Pings originate from `useActivityPing`, which fires (throttled to
+ * one per 5 s) on direct user input — `mousedown`, `keydown`,
+ * `touchstart`, `wheel` — inside any Freighter surface (popup,
+ * sidebar, standalone signing window, grant-access window). Surface
+ * mounts deliberately do NOT ping: a dApp-spawned signing popup is
+ * programmatic, not proof of user presence, and a mount-ping would
+ * let a malicious dApp keep the session alive indefinitely.
  *
- * Either way, the popup-side `isUnlocked` is a delayed reflection of
+ * The popup-side `isUnlocked` it gates on is a delayed reflection of
  * the background's session state (it depends on `loadAccount` having
  * dispatched `saveAccount` into the popup's redux store). The
  * background is the source of truth, so this handler authoritatively
- * checks whether the wallet is currently unlocked before rearming the
- * idle alarm. A ping that arrives on a locked wallet is dropped: there
- * is no live session to extend.
+ * re-checks whether the wallet is currently unlocked before rearming
+ * 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.

extension/src/background/messageListener/helpers/broadcast-session-state.ts

@@ -2,12 +2,25 @@ import browser from "webextension-polyfill";
 
 import { SERVICE_TYPES } from "@shared/constants/services";
 
+// Chrome's runtime.sendMessage rejects with this message whenever no
+// extension contexts are listening for the broadcast. That is the
+// common case (e.g. the user has every Freighter surface closed when
+// the idle alarm fires) and is harmless. Anything else is unexpected
+// and worth logging so it doesn't get silently swallowed.
+const NO_RECEIVER_PATTERNS = [
+  "Could not establish connection",
+  "Receiving end does not exist",
+];
+
 export const broadcastSessionState = async (
   type: SERVICE_TYPES.SESSION_LOCKED | SERVICE_TYPES.SESSION_UNLOCKED,
 ): Promise<void> => {
   try {
     await browser.runtime.sendMessage({ type });
-  } catch {
-    // No receivers — harmless.
+  } catch (e) {
+    const message = e instanceof Error ? e.message : String(e);
+    if (!NO_RECEIVER_PATTERNS.some((p) => message.includes(p))) {
+      console.warn(`broadcastSessionState(${type}) failed:`, e);
+    }
   }
 };

extension/src/popup/components/SessionLockListener/index.tsx

@@ -1,4 +1,4 @@
-import { useEffect } from "react";
+import { useEffect, useRef } from "react";
 import { useDispatch } from "react-redux";
 import { useLocation, useNavigate } from "react-router-dom";
 import browser from "webextension-polyfill";
@@ -32,6 +32,16 @@ export const SessionLockListener = () => {
   const navigate = useNavigate();
   const location = useLocation();
 
+  // Mirror `location` into a ref so the `runtime.onMessage` listener
+  // can read the latest value without being a dependency of the
+  // effect that registers it. Without this the listener would be
+  // re-registered on every navigation — functionally fine (the
+  // cleanup unregisters first), but unnecessary churn on a hot path.
+  const locationRef = useRef(location);
+  useEffect(() => {
+    locationRef.current = location;
+  }, [location]);
+
   useEffect(() => {
     // IMPORTANT: this handler must be a *synchronous* function. Every
     // Freighter UI surface (popup, sidebar, fullscreen) registers a
@@ -61,13 +71,15 @@ export const SessionLockListener = () => {
         return undefined;
       }
 
+      const currentLocation = locationRef.current;
+
       if (type === SERVICE_TYPES.SESSION_LOCKED) {
         // Already on the unlock screen — nothing to do. Avoids clobbering
         // an existing `state.from` set by an earlier reroute.
-        if (location.pathname === ROUTES.unlockAccount) return undefined;
+        if (currentLocation.pathname === ROUTES.unlockAccount) return undefined;
         dispatch(lockAccount());
-        navigate(`${ROUTES.unlockAccount}${location.search}`, {
-          state: { from: location },
+        navigate(`${ROUTES.unlockAccount}${currentLocation.search}`, {
+          state: { from: currentLocation },
         });
         return undefined;
       }
@@ -94,7 +106,7 @@ export const SessionLockListener = () => {
     return () => {
       browser.runtime.onMessage.removeListener(handler);
     };
-  }, [dispatch, navigate, location]);
+  }, [dispatch, navigate]);
 
   return null;
 };