fix(auth): keep secure-storage faults from demoting a decided session (#924)

getAuthStatus's outer catch answers NOT_AUTHENTICATED, which routes an
account-bearing wallet to the onboarding auth stack instead of the lock
screen. Two writes on already-decided paths could reach it:

- Clearing a stale persisted LOCKED marker after deciding HASH_KEY_EXPIRED.
  secureDataStorage.remove swallows its own keychain errors today, so this
  is defense in depth via a best-effort clearPersistedAuthStatus helper.
  signIn's marker clear deliberately keeps the unguarded call: there the
  removal establishes the new session rather than tidying up after a
  decision, so a failure should surface.
- Persisting the soft-lock marker once the auto-lock timer has elapsed.
  secureDataStorage.setItem does throw, making this one reachable. The
  write is now best-effort and the path still returns LOCKED; the
  backgrounded-at timestamp is deliberately left unconsumed so the next
  check re-derives the same lock from elapsed time.

The store's softLock keeps its opposite retry-then-rethrow policy: it also
serves foreground-idle locks with no backgrounded-at timestamp to fall back
on, and it sets the in-memory LOCKED state before writing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: piyalbasu

__tests__/ducks/auth.test.ts

@@ -1730,6 +1730,80 @@ describe("auth duck", () => {
           SENSITIVE_STORAGE_KEYS.AUTH_STATUS,
         );
       });
+
+      // Clearing the stale marker is a cleanup, not the decision. If it were
+      // allowed to reach getAuthStatus's outer catch, a known-expired session
+      // would answer NOT_AUTHENTICATED and RootNavigator would route an
+      // account-bearing wallet to the onboarding auth stack instead of the
+      // lock screen that collects the required password re-auth.
+      it.each([
+        [
+          "an expired key under a persisted soft lock",
+          {
+            temporaryStore: "encrypted-temp-store",
+            hashKey: {
+              hashKey: "mock-hash-key",
+              salt: "mock-salt",
+              generatedAt: Date.now() - 73 * 3600000,
+              expiresAt: Date.now() - 3600000,
+            },
+          },
+        ],
+        [
+          "a persisted soft lock with no temporary store",
+          {
+            temporaryStore: null,
+            hashKey: {
+              hashKey: "mock-hash-key",
+              salt: "mock-salt",
+              expiresAt: Date.now() + 3600000,
+            },
+          },
+        ],
+      ])(
+        "should still return HASH_KEY_EXPIRED for %s when clearing the stale marker fails",
+        async (_label, { temporaryStore, hashKey }) => {
+          const { result } = renderHook(() => useAuthenticationStore());
+
+          act(() => {
+            useAuthenticationStore.setState({
+              getAuthStatus: originalStoreMethods.getAuthStatus,
+            });
+          });
+
+          (dataStorage.getItem as jest.Mock).mockImplementation((key) => {
+            if (key === STORAGE_KEYS.ACCOUNT_LIST) {
+              return Promise.resolve(JSON.stringify([mockAccount]));
+            }
+            return Promise.resolve(null);
+          });
+
+          (secureDataStorage.getItem as jest.Mock).mockImplementation((key) => {
+            if (key === SENSITIVE_STORAGE_KEYS.AUTH_STATUS) {
+              return Promise.resolve(AUTH_STATUS.LOCKED);
+            }
+            if (key === SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE) {
+              return Promise.resolve(temporaryStore);
+            }
+            return Promise.resolve(null);
+          });
+
+          (getHashKey as jest.Mock).mockResolvedValue(hashKey);
+
+          (secureDataStorage.remove as jest.Mock).mockRejectedValue(
+            new Error("keychain unavailable"),
+          );
+
+          await act(async () => {
+            const status = await result.current.getAuthStatus();
+            expect(status).toBe(AUTH_STATUS.HASH_KEY_EXPIRED);
+          });
+
+          expect(secureDataStorage.remove).toHaveBeenCalledWith(
+            SENSITIVE_STORAGE_KEYS.AUTH_STATUS,
+          );
+        },
+      );
     });
 
     describe("getAuthStatus with auto-lock timer", () => {
@@ -1825,6 +1899,36 @@ describe("auth duck", () => {
         );
       });
 
+      // Unlike the reads above, secureDataStorage.setItem really does throw on
+      // a keychain failure. Letting that reach the outer catch would answer
+      // NOT_AUTHENTICATED for a session just decided to be LOCKED, dropping an
+      // account-bearing wallet onto the onboarding stack.
+      it("should still soft-lock when persisting the LOCKED marker fails", async () => {
+        const { result } = renderHook(() => useAuthenticationStore());
+        restoreGetAuthStatus();
+
+        mockAuthenticatedStorage({
+          backgroundedAt: Date.now() - 2 * ONE_HOUR_MS,
+          autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR,
+        });
+
+        (secureDataStorage.setItem as jest.Mock).mockRejectedValue(
+          new Error("Failed to store item in keychain"),
+        );
+
+        await act(async () => {
+          const status = await result.current.getAuthStatus();
+          expect(status).toBe(AUTH_STATUS.LOCKED);
+        });
+
+        // The timestamp is deliberately NOT consumed: with no persisted
+        // marker, the next check must re-derive the same lock from elapsed
+        // background time rather than resolving AUTHENTICATED.
+        expect(secureDataStorage.remove).not.toHaveBeenCalledWith(
+          SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT,
+        );
+      });
+
       it("should consume the timestamp and re-anchor a stale hash-key TTL on active use (#924)", async () => {
         const { result } = renderHook(() => useAuthenticationStore());
         restoreGetAuthStatus();
@@ -2383,6 +2487,77 @@ describe("auth duck", () => {
         expect(result.current.isSoftLocked).toBe(true);
       });
 
+      // Companion to the module-level "persisting the LOCKED marker fails"
+      // test, which starts from NOT_AUTHENTICATED (cold start) and so never
+      // reaches softLock. On the AUTHENTICATED -> LOCKED transition the store
+      // funnels through softLock, whose deliberate policy is retry-once-then-
+      // rethrow. The contract that matters is that the in-memory lock lands
+      // first, so a keychain outage surfaces the fault without ever leaving
+      // the wallet unlocked.
+      it("should still land the in-memory soft lock when the keychain write fails throughout", async () => {
+        const { result } = renderHook(() => useAuthenticationStore());
+        act(() => {
+          useAuthenticationStore.setState({
+            getAuthStatus: originalStoreMethods.getAuthStatus,
+            softLock: originalStoreMethods.softLock,
+            authStatus: AUTH_STATUS.AUTHENTICATED,
+            isSoftLocked: false,
+          });
+        });
+
+        (dataStorage.getItem as jest.Mock).mockImplementation((key) => {
+          if (key === STORAGE_KEYS.ACCOUNT_LIST) {
+            return Promise.resolve(JSON.stringify([mockAccount]));
+          }
+          return Promise.resolve(null);
+        });
+        (secureDataStorage.getItem as jest.Mock).mockImplementation((key) => {
+          if (key === SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE) {
+            return Promise.resolve("encrypted-temp-store");
+          }
+          if (key === SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT) {
+            return Promise.resolve(String(Date.now() - 7200000)); // 2h ago
+          }
+          if (key === SENSITIVE_STORAGE_KEYS.AUTO_LOCK_TIMER_SETTING) {
+            return Promise.resolve(AUTO_LOCK_TIMER.ONE_HOUR);
+          }
+          return Promise.resolve(null);
+        });
+        (getHashKey as jest.Mock).mockResolvedValue({
+          hashKey: "mock-hash-key",
+          salt: "mock-salt",
+          expiresAt: Date.now() + 3600000,
+        });
+        (secureDataStorage.setItem as jest.Mock).mockRejectedValue(
+          new Error("Failed to store item in keychain"),
+        );
+
+        const observedInvalidStates: string[] = [];
+        const unsubscribe = useAuthenticationStore.subscribe((state) => {
+          if (state.authStatus === AUTH_STATUS.LOCKED && !state.isSoftLocked) {
+            observedInvalidStates.push(state.authStatus);
+          }
+        });
+
+        await act(async () => {
+          // softLock rethrows after its retry, by design — the module-level
+          // swallow moves the failure here rather than hiding it.
+          await expect(result.current.getAuthStatus()).rejects.toThrow(
+            "Failed to store item in keychain",
+          );
+        });
+
+        unsubscribe();
+        expect(observedInvalidStates).toHaveLength(0);
+        expect(result.current.authStatus).toBe(AUTH_STATUS.LOCKED);
+        expect(result.current.isSoftLocked).toBe(true);
+        // The backgrounded-at timestamp survives, so a cold start after the
+        // outage re-derives the same lock instead of resolving AUTHENTICATED.
+        expect(secureDataStorage.remove).not.toHaveBeenCalledWith(
+          SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT,
+        );
+      });
+
       it("should make navigateToLockScreen a no-op while soft-locked", () => {
         const { result } = renderHook(() => useAuthenticationStore());
 

src/ducks/auth.ts

@@ -462,6 +462,36 @@ const getAllAccounts = async (): Promise<Account[]> => {
   return JSON.parse(accountListRaw) as Account[];
 };
 
+/**
+ * Clears a stale persisted soft-lock marker, best-effort.
+ *
+ * Only for `getAuthStatus`'s expiry paths, which have already decided the
+ * session is over and are just tidying up after themselves. That cleanup must
+ * not be able to change the decision: an error escaping to `getAuthStatus`'s
+ * outer catch downgrades a known HASH_KEY_EXPIRED session to
+ * NOT_AUTHENTICATED, which routes an account-bearing wallet to the onboarding
+ * auth stack instead of the lock screen that collects the required password
+ * re-auth. The marker is an optimisation; the returned status is the security
+ * decision.
+ *
+ * `secureDataStorage.remove` swallows its own keychain errors today, so this is
+ * defense in depth rather than a live fix. Deliberately NOT used for signIn's
+ * marker clear: there the removal is part of establishing the new session, not
+ * cleanup after a decision, so a failure genuinely means the persisted state
+ * disagrees with the session and should surface.
+ */
+const clearPersistedAuthStatus = async (): Promise<void> => {
+  try {
+    await secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.AUTH_STATUS);
+  } catch (error) {
+    logger.error(
+      "clearPersistedAuthStatus",
+      "Failed to clear stale persisted auth status",
+      error,
+    );
+  }
+};
+
 /**
  * Validates the authentication status of the user
  *
@@ -504,14 +534,14 @@ const getAuthStatus = async (): Promise<AuthStatus> => {
     );
     if (persistedAuthStatus === AUTH_STATUS.LOCKED) {
       if (hashKey && isHashKeyExpired(hashKey)) {
-        await secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.AUTH_STATUS);
+        await clearPersistedAuthStatus();
         return AUTH_STATUS.HASH_KEY_EXPIRED;
       }
       if (temporaryStore) {
         return AUTH_STATUS.LOCKED;
       }
       // Temp store missing: LOCKED state is invalid, treat as expired
-      await secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.AUTH_STATUS);
+      await clearPersistedAuthStatus();
       return AUTH_STATUS.HASH_KEY_EXPIRED;
     }
 
@@ -539,11 +569,37 @@ const getAuthStatus = async (): Promise<AuthStatus> => {
       // unlock path (all presets are positive durations, so no zero/null case
       // to exclude).
       if (elapsedInBackground >= autoLockTimerMs && temporaryStore) {
-        await secureDataStorage.setItem(
-          SENSITIVE_STORAGE_KEYS.AUTH_STATUS,
-          AUTH_STATUS.LOCKED,
-        );
-        await clearBackgroundedAt();
+        // Persisting the marker is best-effort here: unlike `remove`,
+        // `secureDataStorage.setItem` does throw on a keychain failure, and
+        // letting that reach the outer catch would answer NOT_AUTHENTICATED
+        // for a session we have just decided is LOCKED - sending an
+        // account-bearing wallet to the onboarding stack instead of the lock
+        // screen. That matters most on a cold start, where no in-memory lock
+        // state exists yet to fall back on. On failure the backgrounded-at
+        // timestamp is deliberately left intact (the clear is inside the try),
+        // so the next check re-derives the same lock from elapsed time and the
+        // wallet is never auto-unlockable.
+        //
+        // The store's softLock() takes the opposite line on the same write -
+        // retry once, then rethrow - and both are right: softLock also serves
+        // foreground-idle locks that have no backgrounded-at timestamp to
+        // re-derive from, so there a lost write really can leave the wallet
+        // unlocked on the next cold launch. It also sets the in-memory LOCKED
+        // state before writing, so its throw surfaces the fault without
+        // unlocking anything.
+        try {
+          await secureDataStorage.setItem(
+            SENSITIVE_STORAGE_KEYS.AUTH_STATUS,
+            AUTH_STATUS.LOCKED,
+          );
+          await clearBackgroundedAt();
+        } catch (error) {
+          logger.error(
+            "getAuthStatus",
+            "Failed to persist soft-lock auth status",
+            error,
+          );
+        }
         return AUTH_STATUS.LOCKED;
       }