From 9f23478467cae6ba9f4f52ff5ea4bc98f646ba96 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 11:59:48 -0400 Subject: [PATCH 01/13] feat(auth): add throttled hash-key expiry refresh helper Moves isHashKeyExpired into services/autoLock and adds refreshHashKeyExpiration, the activity re-anchor primitive for #924. Not yet wired into getAuthStatus. Co-Authored-By: Claude Fable 5 --- __tests__/services/autoLock.test.ts | 111 ++++++++++++++++++++++++++++ src/config/constants.ts | 24 +++--- src/ducks/auth.ts | 20 +---- src/services/autoLock.ts | 64 ++++++++++++++++ 4 files changed, 191 insertions(+), 28 deletions(-) diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index ba0e38b28..42e301a21 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -1,14 +1,19 @@ import { AUTO_LOCK_TIMER, DEFAULT_AUTO_LOCK_TIMER, + HASH_KEY_EXPIRATION_MS, + HASH_KEY_REFRESH_THROTTLE_MS, SENSITIVE_STORAGE_KEYS, } from "config/constants"; +import { HashKey } from "config/types"; import { clearBackgroundedAt, getAutoLockTimer, getBackgroundedAt, + isHashKeyExpired, persistAutoLockTimer, recordBackgroundedAt, + refreshHashKeyExpiration, } from "services/autoLock"; import { secureDataStorage } from "services/storage/storageFactory"; @@ -145,4 +150,110 @@ describe("autoLock service", () => { ); }); }); + + describe("isHashKeyExpired", () => { + const baseKey: HashKey = { + hashKey: "mock-hash-key", + salt: "mock-salt", + expiresAt: Date.now() + 3_600_000, + generatedAt: Date.now(), + }; + + it("returns false for a valid, unexpired key", () => { + expect(isHashKeyExpired(baseKey)).toBe(false); + }); + + it("returns true when expiresAt has passed", () => { + expect( + isHashKeyExpired({ ...baseKey, expiresAt: Date.now() - 1000 }), + ).toBe(true); + }); + + it("returns true for a future generatedAt (clock rollback)", () => { + // expiresAt is still ahead of the rolled-back clock — only the + // generatedAt guard catches this. + expect( + isHashKeyExpired({ ...baseKey, generatedAt: Date.now() + 60_000 }), + ).toBe(true); + }); + + it("falls back to the plain expiry check for legacy keys without generatedAt", () => { + const { generatedAt, ...legacyKey } = baseKey; + expect(isHashKeyExpired(legacyKey)).toBe(false); + expect( + isHashKeyExpired({ ...legacyKey, expiresAt: Date.now() - 1000 }), + ).toBe(true); + }); + }); + + describe("refreshHashKeyExpiration", () => { + const staleValidKey: HashKey = { + hashKey: "mock-hash-key", + salt: "mock-salt", + // Stale: last anchored beyond the throttle window, but not expired. + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60_000), + expiresAt: Date.now() + 3_600_000, + }; + + it("re-stamps expiresAt and generatedAt for a valid, stale key", async () => { + const before = Date.now(); + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + const [key, value] = (secureDataStorage.setItem as jest.Mock).mock + .calls[0] as [string, string]; + expect(key).toBe(SENSITIVE_STORAGE_KEYS.HASH_KEY); + + const written = JSON.parse(value) as HashKey; + // Key material is untouched — only the timestamps move. + expect(written.hashKey).toBe(staleValidKey.hashKey); + expect(written.salt).toBe(staleValidKey.salt); + expect(written.generatedAt).toBeGreaterThanOrEqual(before); + expect(written.expiresAt).toBe( + (written.generatedAt as number) + HASH_KEY_EXPIRATION_MS, + ); + }); + + it("skips the write while generatedAt is within the throttle window", async () => { + await refreshHashKeyExpiration({ + ...staleValidKey, + generatedAt: Date.now() - 60_000, // freshly anchored + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + + it("never resurrects an expired key", async () => { + await refreshHashKeyExpiration({ + ...staleValidKey, + expiresAt: Date.now() - 1000, + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + + it("never refreshes a rolled-back-clock key (future generatedAt)", async () => { + await refreshHashKeyExpiration({ + ...staleValidKey, + generatedAt: Date.now() + 60_000, + expiresAt: Date.now() + 3_600_000, + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + + it("re-stamps a legacy key without generatedAt (and upgrades it)", async () => { + const { generatedAt, ...legacyKey } = staleValidKey; + await refreshHashKeyExpiration(legacyKey); + + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + const [, value] = (secureDataStorage.setItem as jest.Mock).mock + .calls[0] as [string, string]; + const written = JSON.parse(value) as HashKey; + expect(written.generatedAt).toBeDefined(); + expect(written.expiresAt).toBe( + (written.generatedAt as number) + HASH_KEY_EXPIRATION_MS, + ); + }); + }); }); diff --git a/src/config/constants.ts b/src/config/constants.ts index 572f52373..975d47b2a 100644 --- a/src/config/constants.ts +++ b/src/config/constants.ts @@ -94,17 +94,23 @@ export const ACCOUNTS_TO_VERIFY_ON_EXISTING_MNEMONIC_PHRASE = 6; // rather than taking the fast soft-lock unlock path. It is a separate, coarser // bound than the user-configurable soft auto-lock: the soft timer governs how // soon the wallet re-locks (fast unlock), while this caps how long key material -// may live in secure storage regardless of that choice. +// may live in secure storage without the wallet being used. // -// Set above the largest AUTO_LOCK_TIMER preset (24h) so that preset's soft-lock -// fast path stays reachable — if this were <= 24h, the hard expiry (anchored at -// sign-in and checked before the soft timer) would fire first and the 24h -// preset could never fast-unlock. 72h (3x the max preset) gives enough headroom -// that even a loosely-active 24h-preset user — one whose gaps stay just under -// 24h so the soft-lock never fires and never resets this clock — is unlikely to -// hit a surprise full re-auth. (The complete fix is to re-anchor this on -// activity rather than sign-in; tracked as a follow-up.) +// Anchored on activity, not sign-in (#924): every authenticated foreground +// auth check re-stamps expiresAt (throttled via HASH_KEY_REFRESH_THROTTLE_MS), +// so the bound is "72h with no foreground use" — an actively-used wallet never +// hard-expires, matching the Freighter extension's session model. It must stay +// strictly above the largest AUTO_LOCK_TIMER preset (24h): the hard expiry is +// checked before the soft timer, so if this were <= 24h a 24h-preset user +// would hit the full re-auth instead of that preset's fast unlock. export const HASH_KEY_EXPIRATION_MS = 72 * 60 * 60 * 1000; // 72 hours + +// Minimum age of a hash key's generatedAt anchor before an authenticated +// foreground auth check re-stamps it. getAuthStatus runs as often as every 5s +// while the app is active — this gates the secure-storage (keychain) write to +// at most one per hour rather than one per tick. Granularity is negligible +// against the 72h backstop (worst case the effective bound is 72h + 1h). +export const HASH_KEY_REFRESH_THROTTLE_MS = 60 * 60 * 1000; // 1 hour export const VISUAL_DELAY_MS = 500; const SECOND_IN_MS = 1000; diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index 8834bf524..85fe40d67 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -63,6 +63,7 @@ import { clearBackgroundedAt, getAutoLockTimer, getBackgroundedAt, + isHashKeyExpired, persistAutoLockTimer, } from "services/autoLock"; import { getAccount } from "services/stellar"; @@ -446,25 +447,6 @@ const loadPersistedNetwork = async ( */ const keyManager = createKeyManager(Networks.PUBLIC); -/** - * Checks if a hash key is expired. - * - * Clock-rollback backstop: a key whose generatedAt is in the future means the - * device clock was moved backward below the key's creation time — a rolled-back - * clock would otherwise keep `now <= expiresAt` true indefinitely and prevent - * the hard-expiry from ever forcing a full re-auth. Treat that as expired - * (mirrors getBackgroundedAt's future-timestamp guard for the soft timer). - * generatedAt is optional so keys persisted before this field fall back to the - * plain expiry check. - */ -const isHashKeyExpired = (hashKey: HashKey): boolean => { - const now = Date.now(); - if (hashKey.generatedAt !== undefined && hashKey.generatedAt > now) { - return true; - } - return now > hashKey.expiresAt; -}; - /** * Gets all accounts from the account list * diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index d68efb298..556ad6988 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -1,8 +1,11 @@ import { AUTO_LOCK_TIMER, DEFAULT_AUTO_LOCK_TIMER, + HASH_KEY_EXPIRATION_MS, + HASH_KEY_REFRESH_THROTTLE_MS, SENSITIVE_STORAGE_KEYS, } from "config/constants"; +import { HashKey } from "config/types"; import { getHashKey } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; @@ -88,6 +91,65 @@ const getBackgroundedAt = async (): Promise => { return parsedBackgroundedAt; }; +/** + * Checks if a hash key is expired. + * + * Clock-rollback backstop: a key whose generatedAt is in the future means the + * device clock was moved backward below the key's creation time — a rolled-back + * clock would otherwise keep `now <= expiresAt` true indefinitely and prevent + * the hard-expiry from ever forcing a full re-auth. Treat that as expired + * (mirrors getBackgroundedAt's future-timestamp guard for the soft timer). + * generatedAt is optional so keys persisted before this field fall back to the + * plain expiry check. + */ +const isHashKeyExpired = (hashKey: HashKey): boolean => { + const now = Date.now(); + if (hashKey.generatedAt !== undefined && hashKey.generatedAt > now) { + return true; + } + return now > hashKey.expiresAt; +}; + +/** + * Re-anchors the hash-key hard-expiry on use (#924): pushes expiresAt out to + * a full HASH_KEY_EXPIRATION_MS from now, so the backstop bounds *inactivity* + * rather than time since the last credential entry. + * + * Guards (defense in depth — the getAuthStatus call site is also gated): + * - An expired or rolled-back key is never refreshed; only signIn's + * credential-verified path may re-stamp those. + * - The write is throttled: a key anchored within HASH_KEY_REFRESH_THROTTLE_MS + * is left alone, so the 5s foreground auth tick doesn't hammer the keychain. + * A legacy key without generatedAt can't prove it was recently anchored, so + * it refreshes immediately (gaining generatedAt, after which the throttle + * applies). + * + * Takes the already-read HashKey rather than re-reading it, since every + * caller (getAuthStatus) has just loaded it to run the expiry checks. + */ +const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { + if (isHashKeyExpired(hashKey)) { + return; + } + + const now = Date.now(); + if ( + hashKey.generatedAt !== undefined && + now - hashKey.generatedAt < HASH_KEY_REFRESH_THROTTLE_MS + ) { + return; + } + + await secureDataStorage.setItem( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + JSON.stringify({ + ...hashKey, + expiresAt: now + HASH_KEY_EXPIRATION_MS, + generatedAt: now, + } satisfies HashKey), + ); +}; + /** * Whether an unlockable session is persisted on device (a hash key and a * temporary store both exist). Lets the background handler decide whether to @@ -110,5 +172,7 @@ export { recordBackgroundedAt, getBackgroundedAt, clearBackgroundedAt, + isHashKeyExpired, + refreshHashKeyExpiration, hasPersistedSession, }; From aefbfbebe8b8197ba34d13e8ededdd23d7dbde3b Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 12:08:02 -0400 Subject: [PATCH 02/13] feat(auth): re-anchor hash-key hard-expiry on activity (#924) getAuthStatus now re-stamps expiresAt/generatedAt on every authenticated active-state check (throttled to 1/hour), so the 72h backstop bounds inactivity instead of time-since-sign-in. Expired, rolled-back, locked, and backgrounded states never refresh. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 163 +++++++++++++++++++++++++++++++++-- src/ducks/auth.ts | 35 +++++++- 2 files changed, 189 insertions(+), 9 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 49ef20117..e82314108 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -3,6 +3,8 @@ import { act, renderHook } from "@testing-library/react-hooks"; import { AUTO_LOCK_TIMER, DEFAULT_AUTO_LOCK_TIMER, + HASH_KEY_EXPIRATION_MS, + HASH_KEY_REFRESH_THROTTLE_MS, NETWORKS, STORAGE_KEYS, SENSITIVE_STORAGE_KEYS, @@ -1750,12 +1752,10 @@ describe("auth duck", () => { ); }); - it("should stay authenticated and consume the timestamp WITHOUT refreshing the hash key TTL", async () => { + it("should consume the timestamp and re-anchor a stale hash-key TTL on active use (#924)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - // The jest AppState mock has no real currentState; the consume - // branch only runs when the app is actively foregrounded const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; @@ -1763,7 +1763,15 @@ describe("auth duck", () => { backgroundedAt: Date.now() - 60000, // 1 minute ago autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, }); + // Stale anchor: beyond the refresh throttle, but far from expired. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }); + const before = Date.now(); await act(async () => { const status = await result.current.getAuthStatus(); expect(status).toBe(AUTH_STATUS.AUTHENTICATED); @@ -1773,8 +1781,153 @@ describe("auth duck", () => { expect(secureDataStorage.remove).toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT, ); - // ...but the hash key expiry must NOT advance without credential - // verification (key material lifetime stays bounded) + // ...and the hard-expiry is re-anchored on activity: same key + // material, fresh expiresAt/generatedAt. + const hashKeyWrites = ( + secureDataStorage.setItem as jest.Mock + ).mock.calls.filter(([key]) => key === SENSITIVE_STORAGE_KEYS.HASH_KEY); + expect(hashKeyWrites).toHaveLength(1); + const written = JSON.parse(hashKeyWrites[0][1] as string) as { + hashKey: string; + salt: string; + expiresAt: number; + generatedAt: number; + }; + expect(written.hashKey).toBe("mock-hash-key"); + expect(written.salt).toBe("mock-salt"); + expect(written.generatedAt).toBeGreaterThanOrEqual(before); + expect(written.expiresAt).toBe( + written.generatedAt + HASH_KEY_EXPIRATION_MS, + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should NOT re-anchor a freshly-stamped hash key (write throttle)", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - 60000, // anchored 1 minute ago + expiresAt: Date.now() + ONE_HOUR_MS, + }); + + await act(async () => { + const status = await result.current.getAuthStatus(); + expect(status).toBe(AUTH_STATUS.AUTHENTICATED); + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should NOT re-anchor from the periodic background check", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "background"; + + mockAuthenticatedStorage({ + backgroundedAt: Date.now() - 60000, // within the timer + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + // Stale anchor — would refresh if the app were active. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }); + + await act(async () => { + const status = await result.current.getAuthStatus(); + expect(status).toBe(AUTH_STATUS.AUTHENTICATED); + }); + + // A backgrounded device must not extend its own deadline (and the + // still-counting soft timer must not be consumed either). + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should NOT re-anchor an expired hash key (idle device forces full re-auth)", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, // e.g. cold start where nothing was recorded + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - 73 * ONE_HOUR_MS, + expiresAt: Date.now() - ONE_HOUR_MS, // hard-expired + }); + + await act(async () => { + const status = await result.current.getAuthStatus(); + expect(status).toBe(AUTH_STATUS.HASH_KEY_EXPIRED); + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should NOT re-anchor a rolled-back-clock hash key (generatedAt guard, #905)", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + // Clock rolled back below the key's anchor: generatedAt is in the + // future, expiresAt still looks fine by wall clock. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() + 60000, + expiresAt: Date.now() + ONE_HOUR_MS, + }); + + await act(async () => { + const status = await result.current.getAuthStatus(); + expect(status).toBe(AUTH_STATUS.HASH_KEY_EXPIRED); + }); + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index 85fe40d67..dc54ab6df 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -65,6 +65,7 @@ import { getBackgroundedAt, isHashKeyExpired, persistAutoLockTimer, + refreshHashKeyExpiration, } from "services/autoLock"; import { getAccount } from "services/stellar"; import { @@ -542,16 +543,42 @@ const getAuthStatus = async (): Promise => { if (AppState.currentState === "active") { // Returned within the timer: consume the timestamp so the foreground - // interval can't lock mid-use. The hash-key TTL is deliberately NOT - // refreshed here — it's only anchored at credential-verified moments - // (signIn / generateHashKey) so key material stays bounded however - // often the app is reopened. + // interval can't lock mid-use. (The hash-key TTL re-anchor happens + // below, on the shared AUTHENTICATED path — not here — so it also + // covers ticks where no backgrounded-at timestamp exists.) await clearBackgroundedAt(); } // Still backgrounded (periodic background check): leave the timestamp // intact so the timer keeps counting from the original moment. } + // Re-anchor the hash-key hard-expiry on use (#924): every authenticated + // foreground auth check pushes the deadline out, so the backstop bounds + // *inactivity* — HASH_KEY_EXPIRATION_MS with no foreground use — instead + // of time since the last credential entry, and an actively-used wallet + // never hard-expires (parity with the extension's session model). Gated + // to the active app state so the periodic background check can't extend + // the deadline of a pocketed device, and throttled inside the helper so + // the 5s foreground tick doesn't hammer the keychain. This runs strictly + // after the LOCKED / hard-expiry / clock-rollback checks above, so an + // expired or rolled-back key can never be resurrected here — those still + // require signIn's credential-verified re-stamp. + if (hashKey && AppState.currentState === "active") { + try { + await refreshHashKeyExpiration(hashKey); + } catch (error) { + // The re-anchor is opportunistic: a failed keychain write must not + // demote an authenticated session (the outer catch returns + // NOT_AUTHENTICATED). Worst case the key keeps its old deadline and + // hard-expires as it would have before the re-anchor existed. + logger.error( + "getAuthStatus", + "Failed to refresh hash key expiration", + error, + ); + } + } + // All conditions for authentication are met return AUTH_STATUS.AUTHENTICATED; } catch (error) { From dc7e9a3856073d11adf3779665a514db92afcab1 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 12:12:59 -0400 Subject: [PATCH 03/13] test(auth): cover soft-timer preservation and re-anchor write failure (#924) Adds the assertion the background-check test's comment already promised (a backgrounded device must not consume the still-counting backgrounded-at timestamp), and a test that a failed re-anchor keychain write leaves the session AUTHENTICATED rather than demoting it via getAuthStatus's outer catch. Both were mutation-verified to fail without the code they cover. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 51 ++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index e82314108..57fd4dd98 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -1866,6 +1866,57 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); + expect(secureDataStorage.remove).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT, + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should stay AUTHENTICATED when the re-anchor keychain write fails", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + // Stale anchor on a healthy key: the re-anchor will attempt a write. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }); + // The keychain rejects the re-anchor write (this is the only setItem + // getAuthStatus performs on the AUTHENTICATED path). + (secureDataStorage.setItem as jest.Mock).mockRejectedValue( + new Error("keychain unavailable"), + ); + + await act(async () => { + const status = await result.current.getAuthStatus(); + // The re-anchor is opportunistic: a failed write must not escape to + // the outer catch and demote the session to NOT_AUTHENTICATED. The + // key simply keeps its old deadline. + expect(status).toBe(AUTH_STATUS.AUTHENTICATED); + }); + + expect(logger.error).toHaveBeenCalledWith( + "getAuthStatus", + "Failed to refresh hash key expiration", + expect.any(Error), + ); + // ...and specifically NOT via the outer catch-all. + expect(logger.error).not.toHaveBeenCalledWith( + "validateAuth", + "Failed to validate auth", + expect.anything(), + ); (AppState as { currentState: typeof previousAppState }).currentState = previousAppState; From 766399669695e17350345b39d4710136e152dd7b Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 12:24:13 -0400 Subject: [PATCH 04/13] fix(auth): guard the hash-key re-anchor against TOCTOU resurrection (#924) refreshHashKeyExpiration re-reads the stored key immediately before the write and re-stamps only if it is field-for-field identical to the caller-validated snapshot, so a logout or corruption wipe landing in the window can no longer be resurrected with a fresh 72h deadline. The getAuthStatus gate also requires a temporary store, so an orphan key from a partial wipe hard-expires instead of being re-anchored by every tick. Also stops a rejected-keychain mock leaking into later tests, and restores AppState.currentState in an afterEach so a failing auto-lock test can no longer cascade into unrelated call-count assertions. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 98 ++++++++++++++++++++++++++++- __tests__/services/autoLock.test.ts | 46 ++++++++++++++ src/ducks/auth.ts | 7 ++- src/services/autoLock.ts | 22 ++++++- 4 files changed, 168 insertions(+), 5 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 57fd4dd98..7527fe4b8 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -1680,6 +1680,17 @@ describe("auth duck", () => { describe("getAuthStatus with auto-lock timer", () => { const ONE_HOUR_MS = 3600000; + // These tests drive AppState.currentState and restore it inline, but a + // failing expect() aborts the test before its restore line — leaking + // "active" into the rest of the file and cascading into unrelated + // failures (e.g. the isSessionAuthValid call-count tests). Restore here + // so a failure stays attributable to the test that caused it. + const defaultAppState = AppState.currentState; + afterEach(() => { + (AppState as { currentState: typeof defaultAppState }).currentState = + defaultAppState; + }); + const mockAuthenticatedStorage = ({ backgroundedAt, autoLockTimer, @@ -1894,7 +1905,10 @@ describe("auth duck", () => { }); // The keychain rejects the re-anchor write (this is the only setItem // getAuthStatus performs on the AUTHENTICATED path). - (secureDataStorage.setItem as jest.Mock).mockRejectedValue( + // `Once` (not a sticky mockRejectedValue): jest config has no + // resetMocks, so a sticky rejection would leak a broken keychain into + // every later test in this file. + (secureDataStorage.setItem as jest.Mock).mockRejectedValueOnce( new Error("keychain unavailable"), ); @@ -1922,6 +1936,88 @@ describe("auth duck", () => { previousAppState; }); + it("should not resurrect a hash key wiped mid-check (TOCTOU)", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + const staleKey = { + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }; + // The entry-time read sees a healthy key; by the time the re-anchor + // re-reads at write time, a concurrent logout / corruption wipe + // (clearTemporaryData) has removed it. + (getHashKey as jest.Mock) + .mockResolvedValueOnce(staleKey) + .mockResolvedValue(null); + + await act(async () => { + await result.current.getAuthStatus(); + }); + + // The in-flight tick's status is not the point (it validated a key + // that was live at entry) — the point is that the write is refused, + // so wiped key material is never resurrected with a fresh deadline. + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + + it("should NOT re-anchor an orphan hash key with no temporary store", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + const previousAppState = AppState.currentState; + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + // Partial wipe: the hash key survived but the temporary store is gone. + // The `!hashKey && !temporaryStore` guard can't catch this (the key IS + // present), so the re-anchor gate must refuse it — otherwise every + // later tick would push the orphan's deadline out indefinitely. + (secureDataStorage.getItem as jest.Mock).mockImplementation((key) => { + if (key === SENSITIVE_STORAGE_KEYS.AUTO_LOCK_TIMER_SETTING) { + return Promise.resolve(AUTO_LOCK_TIMER.ONE_HOUR); + } + // No TEMPORARY_STORE, no backgrounded-at, no persisted AUTH_STATUS. + return Promise.resolve(null); + }); + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }); + + await act(async () => { + await result.current.getAuthStatus(); + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + + (AppState as { currentState: typeof previousAppState }).currentState = + previousAppState; + }); + it("should NOT re-anchor an expired hash key (idle device forces full re-auth)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index 42e301a21..e51b615b6 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -15,6 +15,7 @@ import { recordBackgroundedAt, refreshHashKeyExpiration, } from "services/autoLock"; +import { getHashKey } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; jest.mock("services/storage/storageFactory", () => ({ @@ -196,6 +197,10 @@ describe("autoLock service", () => { }; it("re-stamps expiresAt and generatedAt for a valid, stale key", async () => { + // The TOCTOU guard re-reads at write time: storage still holds the + // exact key the caller validated. + (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); + const before = Date.now(); await refreshHashKeyExpiration(staleValidKey); @@ -242,8 +247,49 @@ describe("autoLock service", () => { expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); + it("refuses to write when the stored key was wiped mid-check (TOCTOU)", async () => { + // A concurrent logout / corruption wipe (clearTemporaryData) removed the + // key between the caller's read and this write. Re-stamping the stale + // snapshot here would resurrect wiped key material with a fresh 72h + // deadline — and, since the key would then exist without a temporary + // store, the `!hashKey && !temporaryStore` guard would never fire. + (getHashKey as jest.Mock).mockResolvedValue(null); + + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + + it("refuses to write when the stored key changed mid-check (TOCTOU)", async () => { + // e.g. a concurrent signIn already re-stamped the key with a + // credential-verified anchor; our stale snapshot must not clobber it. + (getHashKey as jest.Mock).mockResolvedValue({ + ...staleValidKey, + generatedAt: Date.now(), + expiresAt: Date.now() + HASH_KEY_EXPIRATION_MS, + }); + + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + + it("writes exactly once when the stored key is identical to the snapshot", async () => { + (getHashKey as jest.Mock).mockResolvedValue({ ...staleValidKey }); + + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + expect(secureDataStorage.setItem).toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + }); + it("re-stamps a legacy key without generatedAt (and upgrades it)", async () => { const { generatedAt, ...legacyKey } = staleValidKey; + (getHashKey as jest.Mock).mockResolvedValue(legacyKey); + await refreshHashKeyExpiration(legacyKey); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index dc54ab6df..615a8bfbf 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -562,8 +562,11 @@ const getAuthStatus = async (): Promise => { // the 5s foreground tick doesn't hammer the keychain. This runs strictly // after the LOCKED / hard-expiry / clock-rollback checks above, so an // expired or rolled-back key can never be resurrected here — those still - // require signIn's credential-verified re-stamp. - if (hashKey && AppState.currentState === "active") { + // require signIn's credential-verified re-stamp. An orphan key (present + // with no temporary store, e.g. a partial wipe) is never re-anchored + // either, so it still hard-expires and gets cleaned up rather than having + // its deadline pushed out forever by each tick. + if (hashKey && temporaryStore && AppState.currentState === "active") { try { await refreshHashKeyExpiration(hashKey); } catch (error) { diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index 556ad6988..3e4393906 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -124,8 +124,9 @@ const isHashKeyExpired = (hashKey: HashKey): boolean => { * it refreshes immediately (gaining generatedAt, after which the throttle * applies). * - * Takes the already-read HashKey rather than re-reading it, since every - * caller (getAuthStatus) has just loaded it to run the expiry checks. + * Takes the caller-validated HashKey snapshot to run the guards and throttle + * (every caller has just loaded it for the expiry checks), then re-reads the + * stored key once at write time for the TOCTOU check below. */ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { if (isHashKeyExpired(hashKey)) { @@ -140,6 +141,23 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { return; } + // TOCTOU guard: the caller validated this key several awaits ago; a logout + // or corruption wipe (clearTemporaryData) may have removed or replaced it + // since. Re-stamp only the exact key that was validated — never resurrect + // a wiped key, never clobber a concurrent credential-verified re-stamp. + // The remaining window is the single await between this read and the write + // (same bound as getActiveMnemonicPhrase's re-check pattern in auth.ts). + const currentHashKey = await getHashKey(); + if ( + !currentHashKey || + currentHashKey.hashKey !== hashKey.hashKey || + currentHashKey.salt !== hashKey.salt || + currentHashKey.expiresAt !== hashKey.expiresAt || + currentHashKey.generatedAt !== hashKey.generatedAt + ) { + return; + } + await secureDataStorage.setItem( SENSITIVE_STORAGE_KEYS.HASH_KEY, JSON.stringify({ From a5590172ea292a17448080a7ec02180cd90a081e Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 12:44:45 -0400 Subject: [PATCH 05/13] fix(auth): throttle re-anchor attempts and tighten #924 docs/tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final-review fixes for the activity-anchored hash-key hard-expiry. The generatedAt throttle only advances when the keychain write succeeds, so a persistently failing secureDataStorage.setItem meant every 5s getAuthStatus tick re-attempted the read + write + logger.error — ~720 error lines/hour, drowning the log buffer during exactly the incident you'd want breadcrumbs for. Add a module-level attempt throttle, set before the write so a throwing write still counts, bounding that failure mode to one attempt per window. Export resetHashKeyRefreshAttemptThrottle (mirroring clearSessionAuthValidMemo) so the module state can't leak across tests, and reset it in both suites. Also: - Spread the TOCTOU re-read key rather than the caller's snapshot when re-stamping: identical today, but correct-by-construction if HashKey ever gains a field the explicit comparison stops covering. - Fix the HASH_KEY_REFRESH_THROTTLE_MS comment, which had the bound backwards (a skipped write shortens the idle window to 71-72h, it does not extend it). - Document the ~96h foregrounded-idle worst case on HASH_KEY_EXPIRATION_MS. - Correct the isSessionAuthValid comment: the active path can now perform a throttled keychain write, so it is no longer secure reads only. - Note why the isSessionAuthValid getHashKey call counts hold only under the jest AppState mock's non-active default. - Drop the inline AppState restores made redundant by the describe-level afterEach, and add the missing blank line in constants.ts. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 51 +++++++++-------------------- __tests__/services/autoLock.test.ts | 25 ++++++++++++++ src/config/constants.ts | 10 ++++-- src/ducks/auth.ts | 5 +-- src/services/autoLock.ts | 33 ++++++++++++++++++- 5 files changed, 83 insertions(+), 41 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 7527fe4b8..209447cf7 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -41,6 +41,7 @@ import { clearScreenshotDek } from "helpers/screenshotCrypto"; import { AppState } from "react-native"; import { getSupportedBiometryType, BIOMETRY_TYPE } from "react-native-keychain"; import { clearAuthKeypairCache } from "services/auth/authKeypairCache"; +import { resetHashKeyRefreshAttemptThrottle } from "services/autoLock"; import { clearNonSensitiveData, clearTemporaryData, @@ -1686,9 +1687,16 @@ describe("auth duck", () => { // failures (e.g. the isSessionAuthValid call-count tests). Restore here // so a failure stays attributable to the test that caused it. const defaultAppState = AppState.currentState; + // These integration tests exercise the real refreshHashKeyExpiration, + // whose attempt throttle is module-level state: without a reset, the + // first test to attempt a write would suppress every later one. + beforeEach(() => { + resetHashKeyRefreshAttemptThrottle(); + }); afterEach(() => { (AppState as { currentState: typeof defaultAppState }).currentState = defaultAppState; + resetHashKeyRefreshAttemptThrottle(); }); const mockAuthenticatedStorage = ({ @@ -1767,7 +1775,6 @@ describe("auth duck", () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -1810,16 +1817,12 @@ describe("auth duck", () => { expect(written.expiresAt).toBe( written.generatedAt + HASH_KEY_EXPIRATION_MS, ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should NOT re-anchor a freshly-stamped hash key (write throttle)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -1842,16 +1845,12 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should NOT re-anchor from the periodic background check", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "background"; mockAuthenticatedStorage({ @@ -1880,16 +1879,12 @@ describe("auth duck", () => { expect(secureDataStorage.remove).not.toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT, ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should stay AUTHENTICATED when the re-anchor keychain write fails", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -1931,16 +1926,12 @@ describe("auth duck", () => { "Failed to validate auth", expect.anything(), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should not resurrect a hash key wiped mid-check (TOCTOU)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -1971,16 +1962,12 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should NOT re-anchor an orphan hash key with no temporary store", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -2013,16 +2000,12 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should NOT re-anchor an expired hash key (idle device forces full re-auth)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -2045,16 +2028,12 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should NOT re-anchor a rolled-back-clock hash key (generatedAt guard, #905)", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -2079,16 +2058,12 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), ); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should return HASH_KEY_EXPIRED when the hash key expired even if within the timer", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); - const previousAppState = AppState.currentState; (AppState as { currentState: string }).currentState = "active"; mockAuthenticatedStorage({ @@ -2113,9 +2088,6 @@ describe("auth duck", () => { // Hard-expiry must evict the derived auth keypair — retention, not just // use, ends at expiry (the eviction-coverage gap raised in review). expect(clearAuthKeypairCache).toHaveBeenCalled(); - - (AppState as { currentState: typeof previousAppState }).currentState = - previousAppState; }); it("should return HASH_KEY_EXPIRED (not LOCKED) when backgrounded beyond BOTH the timer and the hash-key TTL", async () => { @@ -2820,6 +2792,13 @@ describe("auth duck", () => { expect(await isSessionAuthValid()).toBe(false); }); + // getHashKey call counts are used here purely as a proxy for "how many + // funnel runs happened". That 1:1 mapping only holds because the jest + // AppState mock's default state is not "active": in production the active + // path also runs the #924 re-anchor, which re-reads the key (a second + // getHashKey per funnel run). Don't tighten these assertions against a + // production-like AppState — the counts would drift for reasons that have + // nothing to do with the memo/in-flight dedup under test. it("dedupes a concurrent burst into a single funnel run (in-flight promise shared)", async () => { mockFunnelStorage(); (getHashKey as jest.Mock).mockClear(); diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index e51b615b6..2f59b8c24 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -14,6 +14,7 @@ import { persistAutoLockTimer, recordBackgroundedAt, refreshHashKeyExpiration, + resetHashKeyRefreshAttemptThrottle, } from "services/autoLock"; import { getHashKey } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; @@ -38,6 +39,10 @@ jest.mock("services/storage/helpers", () => ({ describe("autoLock service", () => { beforeEach(() => { jest.clearAllMocks(); + // The attempt throttle is module-level state that would otherwise leak + // across tests: the first write-expecting case would consume the window + // and every later one would skip its write. + resetHashKeyRefreshAttemptThrottle(); }); describe("getAutoLockTimer", () => { @@ -286,6 +291,26 @@ describe("autoLock service", () => { ); }); + it("throttles repeat ATTEMPTS, not just successful anchors", async () => { + // The generatedAt throttle only advances when the write succeeds, so a + // persistently failing keychain would be retried (and logged) on every + // 5s auth tick. The attempt throttle bounds that to one try per window. + (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); + + await refreshHashKeyExpiration(staleValidKey); + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + + // Same stale snapshot (as if the write had failed and generatedAt never + // moved): the attempt throttle suppresses the retry. + await refreshHashKeyExpiration(staleValidKey); + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + + // Once the attempt window elapses, the retry is allowed again. + resetHashKeyRefreshAttemptThrottle(); + await refreshHashKeyExpiration(staleValidKey); + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(2); + }); + it("re-stamps a legacy key without generatedAt (and upgrades it)", async () => { const { generatedAt, ...legacyKey } = staleValidKey; (getHashKey as jest.Mock).mockResolvedValue(legacyKey); diff --git a/src/config/constants.ts b/src/config/constants.ts index 975d47b2a..cd8fbb904 100644 --- a/src/config/constants.ts +++ b/src/config/constants.ts @@ -102,15 +102,21 @@ export const ACCOUNTS_TO_VERIFY_ON_EXISTING_MNEMONIC_PHRASE = 6; // hard-expires, matching the Freighter extension's session model. It must stay // strictly above the largest AUTO_LOCK_TIMER preset (24h): the hard expiry is // checked before the soft timer, so if this were <= 24h a 24h-preset user -// would hit the full re-auth instead of that preset's fast unlock. +// would hit the full re-auth instead of that preset's fast unlock. A device +// left foregrounded and untouched keeps re-anchoring until the foreground-idle +// soft lock trips (up to 24h on the max preset), so the worst case from last +// human interaction to hard expiry is ~96h. export const HASH_KEY_EXPIRATION_MS = 72 * 60 * 60 * 1000; // 72 hours // Minimum age of a hash key's generatedAt anchor before an authenticated // foreground auth check re-stamps it. getAuthStatus runs as often as every 5s // while the app is active — this gates the secure-storage (keychain) write to // at most one per hour rather than one per tick. Granularity is negligible -// against the 72h backstop (worst case the effective bound is 72h + 1h). +// against the 72h backstop: a skipped write leaves generatedAt at most 1h +// behind the last activity, so the real idle-to-expiry window is 71-72h +// rather than exactly 72h. export const HASH_KEY_REFRESH_THROTTLE_MS = 60 * 60 * 1000; // 1 hour + export const VISUAL_DELAY_MS = 500; const SECOND_IN_MS = 1000; diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index 615a8bfbf..9241779dd 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -3271,8 +3271,9 @@ export const clearSessionAuthValidMemo = (): void => { * so it evaluates account existence, persisted LOCKED, hash-key hard-expiry AND * the auto-lock timer (`backgroundedAt` + `autoLockTimer`), rather than * re-implementing a subset. Result is memoized for SESSION_AUTH_VALID_TTL_MS - * (see above). Does not decrypt the temporary store (no PBKDF2); persisted/secure - * reads only. + * (see above). Does not decrypt the temporary store (no scrypt); secure-storage + * reads plus at most one throttled hash-key re-anchor write per hour on the + * active path. */ export const isSessionAuthValid = async (): Promise => { const memo = sessionAuthValidMemo; diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index 3e4393906..05968b37b 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -110,6 +110,22 @@ const isHashKeyExpired = (hashKey: HashKey): boolean => { return now > hashKey.expiresAt; }; +// Last time a re-anchor write was attempted (module-level, process-lifetime). +// The generatedAt throttle below only advances when the write SUCCEEDS; if +// the keychain write fails persistently, generatedAt never moves and every +// 5s auth tick would retry (and log) forever. Throttling attempts bounds +// that failure mode to one attempt per throttle window. In-memory on +// purpose: a process restart retrying immediately is fine. +let lastRefreshAttemptAt = 0; + +/** + * Resets the module-level attempt throttle (tests only — module state would + * otherwise leak across cases in the same file). + */ +const resetHashKeyRefreshAttemptThrottle = (): void => { + lastRefreshAttemptAt = 0; +}; + /** * Re-anchors the hash-key hard-expiry on use (#924): pushes expiresAt out to * a full HASH_KEY_EXPIRATION_MS from now, so the backstop bounds *inactivity* @@ -123,6 +139,10 @@ const isHashKeyExpired = (hashKey: HashKey): boolean => { * A legacy key without generatedAt can't prove it was recently anchored, so * it refreshes immediately (gaining generatedAt, after which the throttle * applies). + * - Refresh *attempts* are throttled too (lastRefreshAttemptAt): the + * generatedAt throttle only advances on a successful write, so a + * persistently failing keychain would otherwise be retried (and logged) + * on every 5s auth tick. * * Takes the caller-validated HashKey snapshot to run the guards and throttle * (every caller has just loaded it for the expiry checks), then re-reads the @@ -141,6 +161,12 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { return; } + if (now - lastRefreshAttemptAt < HASH_KEY_REFRESH_THROTTLE_MS) { + return; + } + // Set before the write so a throwing write still counts as an attempt. + lastRefreshAttemptAt = now; + // TOCTOU guard: the caller validated this key several awaits ago; a logout // or corruption wipe (clearTemporaryData) may have removed or replaced it // since. Re-stamp only the exact key that was validated — never resurrect @@ -161,7 +187,11 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { await secureDataStorage.setItem( SENSITIVE_STORAGE_KEYS.HASH_KEY, JSON.stringify({ - ...hashKey, + // Spread the re-read key, not the caller's snapshot: the comparison + // above proves they are identical today, but if HashKey ever gains a + // field the explicit check stops covering it and spreading the stale + // snapshot would silently revert it. + ...currentHashKey, expiresAt: now + HASH_KEY_EXPIRATION_MS, generatedAt: now, } satisfies HashKey), @@ -192,5 +222,6 @@ export { clearBackgroundedAt, isHashKeyExpired, refreshHashKeyExpiration, + resetHashKeyRefreshAttemptThrottle, hasPersistedSession, }; From 5b8da14496b5bc976ab98fc8d63dbc13b2f93d23 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 14:16:22 -0400 Subject: [PATCH 06/13] fix(auth): reset a future-dated re-anchor attempt marker on clock rollback A backward clock change left lastRefreshAttemptAt in the future, which suppressed every hash-key re-anchor until wall time caught up; past ~71h of rollback that hard-expired even the freshly re-authenticated key despite continued use. Treat a future marker as invalid, mirroring the module's other future-timestamp guards. Flagged by Copilot on #993. Co-Authored-By: Claude Fable 5 --- __tests__/services/autoLock.test.ts | 44 +++++++++++++++++++++++++++++ src/services/autoLock.ts | 9 ++++++ 2 files changed, 53 insertions(+) diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index 2f59b8c24..58947da63 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -311,6 +311,50 @@ describe("autoLock service", () => { expect(secureDataStorage.setItem).toHaveBeenCalledTimes(2); }); + it("recovers the attempt throttle after a backward clock change", async () => { + // A refresh attempt stamps lastRefreshAttemptAt with the pre-rollback + // wall time. If the clock then rolls back, that marker sits in the + // future and `now - marker < throttle` holds until wall time catches + // up — for a >71h rollback, long enough to hard-expire even the fresh + // key a forced re-auth just minted, despite continued use. A future + // marker must be treated as invalid, like the module's other + // future-timestamp guards. + const T0 = 1_800_000_000_000; + const dateSpy = jest.spyOn(Date, "now").mockReturnValue(T0); + + try { + const preRollbackKey: HashKey = { + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: T0 - (HASH_KEY_REFRESH_THROTTLE_MS + 60_000), + expiresAt: T0 + 3_600_000, + }; + (getHashKey as jest.Mock).mockResolvedValue(preRollbackKey); + await refreshHashKeyExpiration(preRollbackKey); + // Attempt marker is now stamped at T0. + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); + + // Clock rolls back 72h. The old key hard-expires via the future- + // generatedAt guard and a full re-auth mints a new key at the + // rolled-back time; model it an hour+ later, when it is stale + // enough that a re-anchor SHOULD fire. + const T1 = T0 - 72 * 60 * 60 * 1000; + dateSpy.mockReturnValue(T1); + const reAuthedKey: HashKey = { + hashKey: "mock-hash-key-2", + salt: "mock-salt-2", + generatedAt: T1 - (HASH_KEY_REFRESH_THROTTLE_MS + 60_000), + expiresAt: T1 + 3_600_000, + }; + (getHashKey as jest.Mock).mockResolvedValue(reAuthedKey); + + await refreshHashKeyExpiration(reAuthedKey); + expect(secureDataStorage.setItem).toHaveBeenCalledTimes(2); + } finally { + dateSpy.mockRestore(); + } + }); + it("re-stamps a legacy key without generatedAt (and upgrades it)", async () => { const { generatedAt, ...legacyKey } = staleValidKey; (getHashKey as jest.Mock).mockResolvedValue(legacyKey); diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index 05968b37b..7ed37946f 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -161,6 +161,15 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { return; } + // A backward clock change strands the attempt marker in the future, and + // the check below would then suppress every re-anchor until wall time + // caught back up — for a >71h rollback, long enough to hard-expire even + // the fresh key the forced re-auth just minted, despite continued use. + // Mirror this module's other future-timestamp guards: a future marker is + // invalid, reset it. + if (lastRefreshAttemptAt > now) { + lastRefreshAttemptAt = 0; + } if (now - lastRefreshAttemptAt < HASH_KEY_REFRESH_THROTTLE_MS) { return; } From f8c1ad1f7f9d53248a911efdef0c1bfcd6d8203f Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Mon, 31 Aug 2026 11:26:24 -0400 Subject: [PATCH 07/13] fix(auth): require a live temporary store at re-anchor write time The TOCTOU re-read compared only the hash key, so a wipe that removed the temporary store (but not yet the key) between getAuthStatus's entry-time read and the write could still resurrect or extend an orphan key for a fresh 72h. The write-time re-read now also requires the temp store to exist, moving the orphan invariant into the helper itself rather than only the call-site gate. From /code-review confirmed findings on #993. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 47 +++++++++++++++++++++++++++++ __tests__/services/autoLock.test.ts | 23 ++++++++++++++ src/services/autoLock.ts | 24 ++++++++++----- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 209447cf7..879488ef0 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -1964,6 +1964,53 @@ describe("auth duck", () => { ); }); + it("should not re-anchor when the temporary store is wiped mid-check (TOCTOU)", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + const staleKey = { + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }; + // The key survives, but a wipe removes the temporary store between + // getAuthStatus's entry-time read (which saw it present) and the + // helper's write-time re-read. The call-site gate passed on stale + // data, so the helper's own session check must refuse the write — + // otherwise the wipe race leaves an orphan key valid for 72h. + (getHashKey as jest.Mock).mockResolvedValue(staleKey); + let tempStoreReads = 0; + (secureDataStorage.getItem as jest.Mock).mockImplementation((key) => { + if (key === SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE) { + tempStoreReads += 1; + return Promise.resolve( + tempStoreReads === 1 ? "encrypted-temp-store" : null, + ); + } + if (key === SENSITIVE_STORAGE_KEYS.AUTO_LOCK_TIMER_SETTING) { + return Promise.resolve(AUTO_LOCK_TIMER.ONE_HOUR); + } + return Promise.resolve(null); + }); + + await act(async () => { + await result.current.getAuthStatus(); + }); + + expect(tempStoreReads).toBeGreaterThan(1); + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + }); + it("should NOT re-anchor an orphan hash key with no temporary store", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index 58947da63..5cfff527d 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -201,6 +201,29 @@ describe("autoLock service", () => { expiresAt: Date.now() + 3_600_000, }; + beforeEach(() => { + // The write-time re-read also requires a session to still exist (a + // present temporary store); arm it for the write-expecting tests — + // orphan-case tests override with null. + (secureDataStorage.getItem as jest.Mock).mockResolvedValue( + "encrypted-temp-store", + ); + }); + + it("never re-anchors when the temporary store is gone at write time (orphan key)", async () => { + // A wipe that removed the temp store but not (yet) the hash key must + // not have the key's deadline pushed out — it should hard-expire. + (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); + (secureDataStorage.getItem as jest.Mock).mockResolvedValue(null); + + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.getItem).toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE, + ); + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + it("re-stamps expiresAt and generatedAt for a valid, stale key", async () => { // The TOCTOU guard re-reads at write time: storage still holds the // exact key the caller validated. diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index 7ed37946f..dbd61cc68 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -146,7 +146,8 @@ const resetHashKeyRefreshAttemptThrottle = (): void => { * * Takes the caller-validated HashKey snapshot to run the guards and throttle * (every caller has just loaded it for the expiry checks), then re-reads the - * stored key once at write time for the TOCTOU check below. + * stored key and the temporary store once at write time for the TOCTOU check + * below — the re-anchor requires the exact validated key and a live session. */ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { if (isHashKeyExpired(hashKey)) { @@ -177,14 +178,23 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { lastRefreshAttemptAt = now; // TOCTOU guard: the caller validated this key several awaits ago; a logout - // or corruption wipe (clearTemporaryData) may have removed or replaced it - // since. Re-stamp only the exact key that was validated — never resurrect - // a wiped key, never clobber a concurrent credential-verified re-stamp. - // The remaining window is the single await between this read and the write - // (same bound as getActiveMnemonicPhrase's re-check pattern in auth.ts). - const currentHashKey = await getHashKey(); + // or corruption wipe (clearTemporaryData) may have removed or replaced it — + // or removed just the temporary store — since. Re-stamp only the exact key + // that was validated, and only while a session still exists on disk: never + // resurrect a wiped key, never clobber a concurrent credential-verified + // re-stamp, and never push out the deadline of an orphan key (no temp + // store), which must hard-expire and get cleaned up instead. Checking the + // store here rather than only at the call site keeps the orphan invariant + // with the helper for any future caller. The remaining window is the single + // await between this read and the write (same bound as + // getActiveMnemonicPhrase's re-check pattern in auth.ts). + const [currentHashKey, temporaryStore] = await Promise.all([ + getHashKey(), + secureDataStorage.getItem(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), + ]); if ( !currentHashKey || + !temporaryStore || currentHashKey.hashKey !== hashKey.hashKey || currentHashKey.salt !== hashKey.salt || currentHashKey.expiresAt !== hashKey.expiresAt || From 5e7c764e261dbbd50bfd1a4aca7aad03df7c1d98 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Mon, 31 Aug 2026 12:05:10 -0400 Subject: [PATCH 08/13] fix(auth): close the JS-level wipe race on the hash-key re-anchor write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write-time re-read proved storage state only up to the point it resolved; a clearTemporaryData wipe starting while it was in flight could still land its removes after the re-anchor's write, re-creating wiped key material. clearTemporaryData now bumps a generation counter synchronously at entry, and refreshHashKeyExpiration compares it — captured before the re-read, checked with no await before the write — refusing the write when a wipe began mid-refresh. A wipe starting after the write is dispatched lands last and wins, so either ordering ends with the key absent. Raised by Copilot on #993. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 36 ++++++++++++++++ __tests__/services/autoLock.test.ts | 19 ++++++++- __tests__/services/storage/helpers.test.ts | 49 ++++++++++++++++++++++ src/services/autoLock.ts | 20 +++++++-- src/services/storage/helpers.ts | 24 ++++++++++- 5 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 __tests__/services/storage/helpers.test.ts diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 879488ef0..978201fc7 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -46,6 +46,7 @@ import { clearNonSensitiveData, clearTemporaryData, getHashKey, + getWipeGeneration, } from "services/storage/helpers"; // Import mocked modules import { rnBiometrics } from "services/storage/secureStorage"; @@ -145,6 +146,7 @@ jest.mock("services/storage/helpers", () => ({ clearNonSensitiveData: jest.fn(), clearTemporaryData: jest.fn(), getHashKey: jest.fn(), + getWipeGeneration: jest.fn(() => 0), })); jest.mock("config/logger", () => ({ @@ -2011,6 +2013,40 @@ describe("auth duck", () => { ); }); + it("should not re-anchor when a wipe begins during the write-time re-read", async () => { + const { result } = renderHook(() => useAuthenticationStore()); + restoreGetAuthStatus(); + + (AppState as { currentState: string }).currentState = "active"; + + mockAuthenticatedStorage({ + backgroundedAt: null, + autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, + }); + // Key and temp store both read back healthy — but a + // clearTemporaryData wipe started while the re-read was in flight + // (generation moved), so its removes may land after the re-anchor's + // write. The generation check must refuse the write. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), + expiresAt: Date.now() + ONE_HOUR_MS, + }); + (getWipeGeneration as jest.Mock) + .mockReturnValueOnce(0) + .mockReturnValue(1); + + await act(async () => { + await result.current.getAuthStatus(); + }); + + expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + expect.any(String), + ); + }); + it("should NOT re-anchor an orphan hash key with no temporary store", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index 5cfff527d..cac17f663 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -16,7 +16,7 @@ import { refreshHashKeyExpiration, resetHashKeyRefreshAttemptThrottle, } from "services/autoLock"; -import { getHashKey } from "services/storage/helpers"; +import { getHashKey, getWipeGeneration } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; jest.mock("services/storage/storageFactory", () => ({ @@ -34,6 +34,7 @@ jest.mock("services/storage/storageFactory", () => ({ jest.mock("services/storage/helpers", () => ({ getHashKey: jest.fn(), + getWipeGeneration: jest.fn(() => 0), })); describe("autoLock service", () => { @@ -378,6 +379,22 @@ describe("autoLock service", () => { } }); + it("refuses the write when a wipe starts between the re-read and the write", async () => { + // Storage still holds a matching key and a live temp store, but a + // clearTemporaryData wipe BEGAN while the re-read was in flight (its + // removes may land after our write would). The generation check — + // captured before the re-read, compared synchronously before the + // write — must refuse rather than race the wipe. + (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); + (getWipeGeneration as jest.Mock) + .mockReturnValueOnce(0) + .mockReturnValue(1); + + await refreshHashKeyExpiration(staleValidKey); + + expect(secureDataStorage.setItem).not.toHaveBeenCalled(); + }); + it("re-stamps a legacy key without generatedAt (and upgrades it)", async () => { const { generatedAt, ...legacyKey } = staleValidKey; (getHashKey as jest.Mock).mockResolvedValue(legacyKey); diff --git a/__tests__/services/storage/helpers.test.ts b/__tests__/services/storage/helpers.test.ts new file mode 100644 index 000000000..a63cd43ac --- /dev/null +++ b/__tests__/services/storage/helpers.test.ts @@ -0,0 +1,49 @@ +import { + clearTemporaryData, + getWipeGeneration, +} from "services/storage/helpers"; +import { secureDataStorage } from "services/storage/storageFactory"; + +jest.mock("services/storage/storageFactory", () => ({ + dataStorage: { + getItem: jest.fn(), + setItem: jest.fn().mockResolvedValue(undefined), + remove: jest.fn().mockResolvedValue(undefined), + }, + secureDataStorage: { + getItem: jest.fn(), + setItem: jest.fn().mockResolvedValue(undefined), + remove: jest.fn().mockResolvedValue(undefined), + }, +})); + +describe("storage helpers", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe("wipe generation", () => { + it("bumps synchronously when a wipe begins, before any remove lands", async () => { + const before = getWipeGeneration(); + + // Hold the removes open so the wipe is genuinely in flight. + const releaseRemoves: Array<() => void> = []; + (secureDataStorage.remove as jest.Mock).mockImplementation( + () => + new Promise((resolve) => { + releaseRemoves.push(resolve); + }), + ); + + const wipe = clearTemporaryData(); + + // An in-flight opportunistic writer (the hash-key re-anchor) checking + // the generation now — mid-wipe — must already see it moved. + expect(getWipeGeneration()).toBe(before + 1); + + releaseRemoves.forEach((release) => release()); + await wipe; + expect(getWipeGeneration()).toBe(before + 1); + }); + }); +}); diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index dbd61cc68..a385fa35f 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -6,7 +6,7 @@ import { SENSITIVE_STORAGE_KEYS, } from "config/constants"; import { HashKey } from "config/types"; -import { getHashKey } from "services/storage/helpers"; +import { getHashKey, getWipeGeneration } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; /** @@ -185,9 +185,10 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { // re-stamp, and never push out the deadline of an orphan key (no temp // store), which must hard-expire and get cleaned up instead. Checking the // store here rather than only at the call site keeps the orphan invariant - // with the helper for any future caller. The remaining window is the single - // await between this read and the write (same bound as - // getActiveMnemonicPhrase's re-check pattern in auth.ts). + // with the helper for any future caller. The wipe-generation check below + // covers the read-to-write gap itself, so within the JS thread no + // interleaving with clearTemporaryData can resurrect wiped material. + const wipeGenerationAtRead = getWipeGeneration(); const [currentHashKey, temporaryStore] = await Promise.all([ getHashKey(), secureDataStorage.getItem(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), @@ -203,6 +204,17 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { return; } + // The re-read above only proves the key survived up to the point those + // reads resolved — a clearTemporaryData wipe starting while they were in + // flight could still have its removes land after the write below. The + // wipe bumps its generation synchronously at entry, so this check (no + // await between it and the write) refuses that interleaving: a wipe that + // instead starts after the write is dispatched has its removes land last + // and wins. Either ordering ends with no resurrected key. + if (getWipeGeneration() !== wipeGenerationAtRead) { + return; + } + await secureDataStorage.setItem( SENSITIVE_STORAGE_KEYS.HASH_KEY, JSON.stringify({ diff --git a/src/services/storage/helpers.ts b/src/services/storage/helpers.ts index 2cd0c1b44..b5368f33b 100644 --- a/src/services/storage/helpers.ts +++ b/src/services/storage/helpers.ts @@ -5,6 +5,22 @@ import { secureDataStorage, } from "services/storage/storageFactory"; +// Monotonic counter bumped at the START of every clearTemporaryData wipe. +// Lets an in-flight opportunistic writer (the hash-key re-anchor in +// services/autoLock) detect that a wipe began after it validated storage and +// refuse its write, instead of racing the wipe's removes and re-creating +// just-wiped key material. JS is single-threaded, so a bump is visible to any +// check that runs after the wipe starts; a write dispatched before the bump +// has its removes land after it, so either ordering ends with the key absent. +let wipeGeneration = 0; + +/** + * Current wipe generation — see wipeGeneration above. Capture before an + * optimistic read-validate-write sequence and compare (synchronously, with no + * await in between) right before the write. + */ +const getWipeGeneration = (): number => wipeGeneration; + /** * Clears the hash key, temporary store, and derived key cache from secure storage. * @@ -15,6 +31,7 @@ import { * the logout-wipe path alongside clearAllWebViewData. */ const clearTemporaryData = async (): Promise => { + wipeGeneration += 1; await Promise.all([ secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.HASH_KEY), secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), @@ -50,4 +67,9 @@ const getHashKey = async (): Promise => { return hashKey ? (JSON.parse(hashKey) as HashKey) : null; }; -export { clearTemporaryData, clearNonSensitiveData, getHashKey }; +export { + clearTemporaryData, + clearNonSensitiveData, + getHashKey, + getWipeGeneration, +}; From bb83074a09b0afb4c6910a731af10081d0f6d36a Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Mon, 31 Aug 2026 13:10:27 -0400 Subject: [PATCH 09/13] fix(auth): surface hard expiry through a persisted soft lock (#924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under sign-in anchoring (#905), LOCKED deliberately outranked the expiry check so routine lock-screen expiry kept the fast unlock reachable. With the TTL re-anchored on activity, an expired key under a persisted soft lock means 72h of genuine idleness — the exact case AC2 requires to force the full password re-auth. getAuthStatus now reports it as HASH_KEY_EXPIRED (consuming the stale LOCKED marker), and the signIn fast path refuses to re-stamp an expired key, falling through to the full rebuild. Raised by Copilot on #993. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 52 ++++++++++++++++++++++++++++++++++++ src/ducks/auth.ts | 30 +++++++++++++++------ 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 978201fc7..007e05446 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -1635,6 +1635,58 @@ describe("auth duck", () => { }); }); + it("should return HASH_KEY_EXPIRED (not LOCKED) when a persisted soft lock holds an expired key (#924)", async () => { + 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); + }); + + // Soft-locked session sitting past the whole backstop window: with + // the TTL anchored on activity, this only happens after 72h of no + // use — exactly the case that must surface as the full re-auth + // rather than the fast unlock (#924 AC2). Under sign-in anchoring + // (#905) lock-screen expiry was routine, so LOCKED deliberately won; + // activity anchoring makes it the genuine-idle signal. + (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("encrypted-temp-store"); + } + return Promise.resolve(null); + }); + + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: Date.now() - 73 * 3600000, + expiresAt: Date.now() - 3600000, // hard-expired an hour ago + }); + (secureDataStorage.remove as jest.Mock).mockResolvedValue(undefined); + + await act(async () => { + const status = await result.current.getAuthStatus(); + expect(status).toBe(AUTH_STATUS.HASH_KEY_EXPIRED); + }); + + // The stale soft-lock marker is consumed so the next check doesn't + // resurrect the fast path. + expect(secureDataStorage.remove).toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.AUTH_STATUS, + ); + }); + it("should clear invalid LOCKED status if temp store doesn't exist", async () => { const { result } = renderHook(() => useAuthenticationStore()); diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index 9241779dd..6abe45386 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -492,14 +492,21 @@ const getAuthStatus = async (): Promise => { } // Read from SECURE storage (encrypted) to prevent tampering. - // LOCKED check comes before isHashKeyExpired: if the user locked the app, - // the hash key may have expired while it was sitting on the lock screen. - // We still want LOCKED (not HASH_KEY_EXPIRED) so the signIn fast path runs - // (TTL refresh + derived key cache) rather than forcing a full re-derivation. + // Hard expiry outranks a persisted soft lock (#924). Under sign-in + // anchoring (#905) the key routinely expired while sitting on the lock + // screen, so LOCKED deliberately won to keep the fast unlock reachable; + // with the TTL re-anchored on activity, an expired key here means a full + // backstop window (72h) of no use — exactly the genuine-idle case that + // must force the full password re-auth instead of the fast path. The + // stale soft-lock marker is consumed so later checks can't resurrect it. const persistedAuthStatus = await secureDataStorage.getItem( SENSITIVE_STORAGE_KEYS.AUTH_STATUS, ); if (persistedAuthStatus === AUTH_STATUS.LOCKED) { + if (hashKey && isHashKeyExpired(hashKey)) { + await secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.AUTH_STATUS); + return AUTH_STATUS.HASH_KEY_EXPIRED; + } if (temporaryStore) { return AUTH_STATUS.LOCKED; } @@ -510,8 +517,7 @@ const getAuthStatus = async (): Promise => { // Hard expiry wins over the soft timer lock: checked before the timer so a // session backgrounded past the hash-key TTL forces a full re-auth instead - // of a fast-path LOCKED that would just refresh the expired key. (The - // persisted-LOCKED branch above keeps the fast path — there it's intended.) + // of a fast-path LOCKED that would just refresh the expired key. if (hashKey && isHashKeyExpired(hashKey)) { return AUTH_STATUS.HASH_KEY_EXPIRED; } @@ -1521,10 +1527,18 @@ const signIn = async ({ const existingTempStore = await secureDataStorage.getItem( SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE, ); - if (existingHashKey && existingTempStore) { + if ( + existingHashKey && + existingTempStore && + !isHashKeyExpired(existingHashKey) + ) { // Fast path: temp store is intact — just refresh the hard-expiry TTL. // Re-anchor generatedAt too (this is a credential-verified moment) so the - // clock-rollback backstop stays aligned with the new expiry. + // clock-rollback backstop stays aligned with the new expiry. An expired + // or rolled-back key never takes this path (getAuthStatus reports it as + // HASH_KEY_EXPIRED, but expiry can also cross between that check and + // this call): it falls through to the full rebuild below, which mints a + // fresh key instead of re-stamping stale material. const refreshNow = Date.now(); await secureDataStorage.setItem( SENSITIVE_STORAGE_KEYS.HASH_KEY, From f03bea2a7773ece99862aabb340644f1b5f00866 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Mon, 31 Aug 2026 13:51:54 -0400 Subject: [PATCH 10/13] test(auth): cover the LOCKED sign-in expired-key rebuild fallback (#924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fast-path guard added in bb83074a had no direct coverage. Two cases (expired, clock-rolled-back) now assert the LOCKED sign-in falls through to the full temporary-store rebuild — a fresh temp store is written and no write re-stamps the stale key material — using stateful hash-key mocks so the background getActiveAccount sees the rebuilt key. Both go red if the guard is removed (mutation-verified). Raised by Copilot on #993. Co-Authored-By: Claude Fable 5 --- __tests__/ducks/auth.test.ts | 125 +++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index 007e05446..b80f512bd 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -2513,6 +2513,131 @@ describe("auth duck", () => { expect(decryptDataWithPassword).not.toHaveBeenCalled(); expect(encryptDataWithPassword).not.toHaveBeenCalled(); }); + + it.each([ + [ + "expired", + { + hashKey: "stale-mock-hash-key", + salt: "stale-mock-salt", + generatedAt: Date.now() - 73 * 3600000, + expiresAt: Date.now() - 3600000, // hard-expired an hour ago + }, + ], + [ + "rolled-back (future generatedAt)", + { + hashKey: "stale-mock-hash-key", + salt: "stale-mock-salt", + generatedAt: Date.now() + 3600000, // clock moved backward + expiresAt: Date.now() + 3600000, + }, + ], + ])( + "should rebuild the session instead of re-stamping a %s key on LOCKED sign-in (#924)", + async (_label, staleKey) => { + // Expiry can cross between the getAuthStatus check and the unlock + // (or a stale persisted LOCKED can carry an expired key straight + // into signIn): the fast path must refuse to re-stamp the stale + // material and fall through to the full temporary-store rebuild, + // which mints a fresh hash key. + mockKeyManager.loadKey.mockResolvedValueOnce({ + id: mockAccount.id, + publicKey: mockAccount.publicKey, + privateKey: "mock-private-key", + extra: { mnemonicPhrase: "mock mnemonic phrase" }, + }); + mockKeyManager.loadAllKeyIds.mockResolvedValueOnce([mockAccount.id]); + + (dataStorage.getItem as jest.Mock).mockImplementation((key) => { + if (key === STORAGE_KEYS.ACTIVE_ACCOUNT_ID) { + return Promise.resolve(mockAccount.id); + } + if (key === STORAGE_KEYS.ACCOUNT_LIST) { + return Promise.resolve(JSON.stringify([mockAccount])); + } + return Promise.resolve(null); + }); + + // Stateful hash-key storage: reads reflect writes, so once the + // rebuild mints a fresh key the background getActiveAccount sees + // it (a static stale mock would knock the session back to LOCKED + // regardless of which branch ran). + let storedHashKey: Record = staleKey; + (getHashKey as jest.Mock).mockImplementation(() => + Promise.resolve(storedHashKey), + ); + (secureDataStorage.setItem as jest.Mock).mockImplementation( + (key, value) => { + if (key === SENSITIVE_STORAGE_KEYS.HASH_KEY) { + storedHashKey = JSON.parse(value as string) as Record< + string, + unknown + >; + } + return Promise.resolve(undefined); + }, + ); + + (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.HASH_KEY) { + return Promise.resolve(JSON.stringify(storedHashKey)); + } + return Promise.resolve(null); + }); + + (decryptDataWithDerivedKey as jest.Mock).mockReturnValue( + JSON.stringify({ + privateKeys: { [mockAccount.id]: "mock-private-key" }, + mnemonicPhrase: "mock mnemonic phrase", + }), + ); + (deriveKeyFromPassword as jest.Mock).mockResolvedValue( + new Uint8Array([1, 2, 3, 4, 5, 6, 7, 8]), + ); + + const { result } = renderHook(() => useAuthenticationStore()); + + act(() => { + useAuthenticationStore.setState({ + authStatus: AUTH_STATUS.LOCKED, + signIn: originalStoreMethods.signIn, + }); + }); + + await act(async () => { + await result.current.signIn({ password: "test-password" }); + }); + + await act(async () => { + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + }); + + expect(result.current.authStatus).toBe(AUTH_STATUS.AUTHENTICATED); + + // The full rebuild ran: a fresh temporary store was written (the + // TTL-refresh branch never touches TEMPORARY_STORE)... + expect(secureDataStorage.setItem).toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE, + expect.any(String), + ); + // ...and no write re-stamped the stale key material. + const hashKeyWrites = ( + secureDataStorage.setItem as jest.Mock + ).mock.calls.filter( + ([key]) => key === SENSITIVE_STORAGE_KEYS.HASH_KEY, + ); + expect(hashKeyWrites.length).toBeGreaterThan(0); + hashKeyWrites.forEach(([, value]) => { + expect(value).not.toContain("stale-mock-hash-key"); + }); + }, + ); }); describe("account switching", () => { From 0d3aa8cb5e39c1660b6926a9558632249e5ec21f Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Tue, 22 Sep 2026 14:54:23 -0400 Subject: [PATCH 11/13] 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) --- __tests__/ducks/auth.test.ts | 175 +++++++++++++++++++++++++++++++++++ src/ducks/auth.ts | 70 ++++++++++++-- 2 files changed, 238 insertions(+), 7 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index b80f512bd..81c19c8e1 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__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()); diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index 6abe45386..cb92e1070 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -462,6 +462,36 @@ const getAllAccounts = async (): Promise => { 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 => { + 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 => { ); 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 => { // 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; } From 0b5366dc59e6dfe4354b4061e6e7627b6b1b33ba Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Mon, 9 Nov 2026 13:14:17 -0500 Subject: [PATCH 12/13] fix(auth): generalize the wipe generation to every hash-key mutation Two Copilot findings on #993 showed the re-anchor's generation check was scoped too narrowly in both dimensions: - What it tracked: only clearTemporaryData bumped the counter, so a credential-verified signIn/createTemporaryStore write landing between the re-anchor's re-read and its write would be reverted to the old key/salt while the temporary store had already been re-encrypted with the new one. Every credential-verified write now goes through setHashKey, which bumps the same counter synchronously before the write is dispatched; the re-anchor stays the one writer that bypasses it because it is the one checking. - When it was captured: inside the helper, after the caller had already loaded hashKey/temporaryStore. A wipe starting in that gap bumped the counter before the capture, so a stale re-read compared equal and the write could recreate the key after the removes landed. getAuthStatus now captures the generation before its load and passes it in, so any mutation that began after the load is refused. The comments no longer claim that a mutation dispatched after the re-anchor's write "lands last and wins": that holds on iOS (serial methodQueue) but not on Android, where react-native-keychain 10.0.0 runs setGenericPassword in a mutex'd coroutine on Dispatchers.Default and resetGenericPassword synchronously on the bridge thread. That window is the residual the PR description already names; closing it needs a JS-level serialization of hash-key mutations, left as a decision. Test hygiene from the third finding: the "persisting the LOCKED marker fails" rejection is one-shot, and the following re-anchor test asserts no error was logged so it demonstrably runs the success path. The claimed leak did not reproduce (the fixture re-arms setItem per test), but the sticky value was a latent hazard; the sibling in the HASH_KEY_EXPIRED marker-clear test is one-shot as well, the "fails throughout" softLock test keeps its sticky rejection (getAuthStatus's own swallowed write plus softLock's attempt and retry all have to fail) and resets it in afterEach, and the helpers test re-arms setItem/remove per case so its held-open promises can't hang later tests. The generation-refusal integration test asserts both generation reads were consumed so it can't pass vacuously and leak the queued value into the next case. Verification: new helper and autoLock tests seen failing against the pre-fix code (write proceeded; setHashKey undefined); a mutant routing both auth.ts writes around setHashKey flips all three LOCKED sign-in tests red. tsc clean; full suite 5207 passed / 348 suites after a yarn install --immutable (worktree node_modules was behind the merged lockfile). Co-Authored-By: Claude Fable 5.1 --- __tests__/ducks/auth.test.ts | 81 +++++++++++++++---- __tests__/services/autoLock.test.ts | 90 +++++++++++++--------- __tests__/services/storage/helpers.test.ts | 51 ++++++++++-- src/ducks/auth.ts | 25 +++--- src/services/autoLock.ts | 40 ++++++---- src/services/storage/helpers.ts | 46 +++++++---- 6 files changed, 232 insertions(+), 101 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index fd26690c4..c4c75e781 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -47,7 +47,8 @@ import { clearNonSensitiveData, clearTemporaryData, getHashKey, - getWipeGeneration, + getHashKeyGeneration, + setHashKey, } from "services/storage/helpers"; // Import mocked modules import { rnBiometrics } from "services/storage/secureStorage"; @@ -143,12 +144,24 @@ jest.mock("services/stellar", () => ({ getAccount: jest.fn(), })); -jest.mock("services/storage/helpers", () => ({ - clearNonSensitiveData: jest.fn(), - clearTemporaryData: jest.fn(), - getHashKey: jest.fn(), - getWipeGeneration: jest.fn(() => 0), -})); +jest.mock("services/storage/helpers", () => { + const { secureDataStorage: mockedSecureStorage } = jest.requireMock< + typeof import("services/storage/storageFactory") + >("services/storage/storageFactory"); + const { SENSITIVE_STORAGE_KEYS: keys } = + jest.requireActual("config/constants"); + return { + clearNonSensitiveData: jest.fn(), + clearTemporaryData: jest.fn(), + getHashKey: jest.fn(), + getHashKeyGeneration: jest.fn(() => 0), + // Delegates to the mocked secure storage so assertions on HASH_KEY + // writes see credential-verified writes and re-anchors alike. + setHashKey: jest.fn((hashKey: unknown) => + mockedSecureStorage.setItem(keys.HASH_KEY, JSON.stringify(hashKey)), + ), + }; +}); jest.mock("config/logger", () => ({ logger: { @@ -1951,7 +1964,7 @@ describe("auth duck", () => { (getHashKey as jest.Mock).mockResolvedValue(hashKey); - (secureDataStorage.remove as jest.Mock).mockRejectedValue( + (secureDataStorage.remove as jest.Mock).mockRejectedValueOnce( new Error("keychain unavailable"), ); @@ -2073,7 +2086,7 @@ describe("auth duck", () => { autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, }); - (secureDataStorage.setItem as jest.Mock).mockRejectedValue( + (secureDataStorage.setItem as jest.Mock).mockRejectedValueOnce( new Error("Failed to store item in keychain"), ); @@ -2114,6 +2127,10 @@ describe("auth duck", () => { expect(status).toBe(AUTH_STATUS.AUTHENTICATED); }); + // The write must have SUCCEEDED, not been swallowed by the + // re-anchor's catch — the call list below alone can't tell the two + // apart. + expect(logger.error).not.toHaveBeenCalled(); // Timestamp is consumed... expect(secureDataStorage.remove).toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.AUTO_LOCK_BACKGROUNDED_AT, @@ -2330,7 +2347,7 @@ describe("auth duck", () => { ); }); - it("should not re-anchor when a wipe begins during the write-time re-read", async () => { + it("should not re-anchor when a wipe or credential-verified re-key begins during the check", async () => { const { result } = renderHook(() => useAuthenticationStore()); restoreGetAuthStatus(); @@ -2340,24 +2357,29 @@ describe("auth duck", () => { backgroundedAt: null, autoLockTimer: AUTO_LOCK_TIMER.ONE_HOUR, }); - // Key and temp store both read back healthy — but a - // clearTemporaryData wipe started while the re-read was in flight - // (generation moved), so its removes may land after the re-anchor's - // write. The generation check must refuse the write. + // Key and temp store both read back healthy — but the hash-key + // generation moved between getAuthStatus loading the key and the + // re-anchor's write (a clearTemporaryData wipe or a signIn rebuild + // began meanwhile, with its removes/write possibly still in flight). + // The generation check must refuse the write. (getHashKey as jest.Mock).mockResolvedValue({ hashKey: "mock-hash-key", salt: "mock-salt", generatedAt: Date.now() - (HASH_KEY_REFRESH_THROTTLE_MS + 60000), expiresAt: Date.now() + ONE_HOUR_MS, }); - (getWipeGeneration as jest.Mock) + (getHashKeyGeneration as jest.Mock) .mockReturnValueOnce(0) - .mockReturnValue(1); + .mockReturnValueOnce(1); await act(async () => { await result.current.getAuthStatus(); }); + // Both reads consumed: the capture before the load and the helper's + // write-time check. An earlier early-return would leave the queued 1 + // for the next test's capture and pass this one vacuously. + expect(getHashKeyGeneration).toHaveBeenCalledTimes(2); expect(secureDataStorage.setItem).not.toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.any(String), @@ -2560,6 +2582,14 @@ describe("auth duck", () => { }); describe("softLock", () => { + afterEach(() => { + // jest.clearAllMocks keeps implementations; drop the sticky + + // keychain-outage rejection installed by the test below. + + (secureDataStorage.setItem as jest.Mock).mockReset(); + }); + it("should set LOCKED with isSoftLocked and persist the status without touching navigation", async () => { const { result } = renderHook(() => useAuthenticationStore()); @@ -2689,6 +2719,9 @@ describe("auth duck", () => { salt: "mock-salt", expiresAt: Date.now() + 3600000, }); + // Sticky on purpose: getAuthStatus's own swallowed LOCKED write, + // then softLock's attempt and retry, must all fail. The describe's + // afterEach resets it so the outage can't leak into later tests. (secureDataStorage.setItem as jest.Mock).mockRejectedValue( new Error("Failed to store item in keychain"), ); @@ -2840,7 +2873,16 @@ describe("auth duck", () => { SENSITIVE_STORAGE_KEYS.AUTH_STATUS, ); - // Verify TTL was refreshed on the existing hash key (no re-encryption) + // Verify TTL was refreshed on the existing hash key (no re-encryption), + // through the generation-bumping writer so an in-flight re-anchor yields. + expect(setHashKey).toHaveBeenCalledWith( + expect.objectContaining({ + hashKey: "old-mock-hash-key", + salt: "old-mock-salt", + expiresAt: expect.any(Number), + generatedAt: expect.any(Number), + }), + ); expect(secureDataStorage.setItem).toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.HASH_KEY, expect.stringContaining('"expiresAt":'), @@ -2972,6 +3014,11 @@ describe("auth duck", () => { hashKeyWrites.forEach(([, value]) => { expect(value).not.toContain("stale-mock-hash-key"); }); + // The fresh key went through the generation-bumping writer, so a + // re-anchor in flight during the rebuild yields to it. + expect(setHashKey).toHaveBeenCalledWith( + expect.not.objectContaining({ hashKey: "stale-mock-hash-key" }), + ); }, ); }); diff --git a/__tests__/services/autoLock.test.ts b/__tests__/services/autoLock.test.ts index cac17f663..8a7440e27 100644 --- a/__tests__/services/autoLock.test.ts +++ b/__tests__/services/autoLock.test.ts @@ -16,7 +16,7 @@ import { refreshHashKeyExpiration, resetHashKeyRefreshAttemptThrottle, } from "services/autoLock"; -import { getHashKey, getWipeGeneration } from "services/storage/helpers"; +import { getHashKey, getHashKeyGeneration } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; jest.mock("services/storage/storageFactory", () => ({ @@ -34,7 +34,7 @@ jest.mock("services/storage/storageFactory", () => ({ jest.mock("services/storage/helpers", () => ({ getHashKey: jest.fn(), - getWipeGeneration: jest.fn(() => 0), + getHashKeyGeneration: jest.fn(() => 0), })); describe("autoLock service", () => { @@ -202,6 +202,10 @@ describe("autoLock service", () => { expiresAt: Date.now() + 3_600_000, }; + // What getAuthStatus captured before loading the key; the mocked + // getHashKeyGeneration also returns 0, so storage reads as unmoved. + const GENERATION_AT_LOAD = 0; + beforeEach(() => { // The write-time re-read also requires a session to still exist (a // present temporary store); arm it for the write-expecting tests — @@ -217,7 +221,7 @@ describe("autoLock service", () => { (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); (secureDataStorage.getItem as jest.Mock).mockResolvedValue(null); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.getItem).toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE, @@ -231,7 +235,7 @@ describe("autoLock service", () => { (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); const before = Date.now(); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); const [key, value] = (secureDataStorage.setItem as jest.Mock).mock @@ -249,29 +253,38 @@ describe("autoLock service", () => { }); it("skips the write while generatedAt is within the throttle window", async () => { - await refreshHashKeyExpiration({ - ...staleValidKey, - generatedAt: Date.now() - 60_000, // freshly anchored - }); + await refreshHashKeyExpiration( + { + ...staleValidKey, + generatedAt: Date.now() - 60_000, // freshly anchored + }, + GENERATION_AT_LOAD, + ); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); it("never resurrects an expired key", async () => { - await refreshHashKeyExpiration({ - ...staleValidKey, - expiresAt: Date.now() - 1000, - }); + await refreshHashKeyExpiration( + { + ...staleValidKey, + expiresAt: Date.now() - 1000, + }, + GENERATION_AT_LOAD, + ); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); it("never refreshes a rolled-back-clock key (future generatedAt)", async () => { - await refreshHashKeyExpiration({ - ...staleValidKey, - generatedAt: Date.now() + 60_000, - expiresAt: Date.now() + 3_600_000, - }); + await refreshHashKeyExpiration( + { + ...staleValidKey, + generatedAt: Date.now() + 60_000, + expiresAt: Date.now() + 3_600_000, + }, + GENERATION_AT_LOAD, + ); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); @@ -284,7 +297,7 @@ describe("autoLock service", () => { // store, the `!hashKey && !temporaryStore` guard would never fire. (getHashKey as jest.Mock).mockResolvedValue(null); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); @@ -298,7 +311,7 @@ describe("autoLock service", () => { expiresAt: Date.now() + HASH_KEY_EXPIRATION_MS, }); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); @@ -306,7 +319,7 @@ describe("autoLock service", () => { it("writes exactly once when the stored key is identical to the snapshot", async () => { (getHashKey as jest.Mock).mockResolvedValue({ ...staleValidKey }); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); expect(secureDataStorage.setItem).toHaveBeenCalledWith( @@ -321,17 +334,17 @@ describe("autoLock service", () => { // 5s auth tick. The attempt throttle bounds that to one try per window. (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); // Same stale snapshot (as if the write had failed and generatedAt never // moved): the attempt throttle suppresses the retry. - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); // Once the attempt window elapses, the retry is allowed again. resetHashKeyRefreshAttemptThrottle(); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(2); }); @@ -354,7 +367,7 @@ describe("autoLock service", () => { expiresAt: T0 + 3_600_000, }; (getHashKey as jest.Mock).mockResolvedValue(preRollbackKey); - await refreshHashKeyExpiration(preRollbackKey); + await refreshHashKeyExpiration(preRollbackKey, GENERATION_AT_LOAD); // Attempt marker is now stamped at T0. expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); @@ -372,25 +385,30 @@ describe("autoLock service", () => { }; (getHashKey as jest.Mock).mockResolvedValue(reAuthedKey); - await refreshHashKeyExpiration(reAuthedKey); + await refreshHashKeyExpiration(reAuthedKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(2); } finally { dateSpy.mockRestore(); } }); - it("refuses the write when a wipe starts between the re-read and the write", async () => { - // Storage still holds a matching key and a live temp store, but a - // clearTemporaryData wipe BEGAN while the re-read was in flight (its - // removes may land after our write would). The generation check — - // captured before the re-read, compared synchronously before the - // write — must refuse rather than race the wipe. + it("refuses the write when the key was mutated after the caller loaded it", async () => { + // Storage still reads back the exact validated key and a live temp + // store — but the hash-key generation moved since the caller captured + // it before its load. That covers a clearTemporaryData wipe or a + // credential-verified setHashKey that began anywhere between the + // caller's load and this write, including before this helper was even + // entered (its removes/write may still be in flight behind the stale + // re-read). Compared synchronously before the write, so the mutation's + // result is what ends up on disk. (getHashKey as jest.Mock).mockResolvedValue(staleValidKey); - (getWipeGeneration as jest.Mock) - .mockReturnValueOnce(0) - .mockReturnValue(1); + // Once: `beforeEach` only clears calls, so a sticky value would leak + // the refusal into the following tests. + (getHashKeyGeneration as jest.Mock).mockReturnValueOnce( + GENERATION_AT_LOAD + 1, + ); - await refreshHashKeyExpiration(staleValidKey); + await refreshHashKeyExpiration(staleValidKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).not.toHaveBeenCalled(); }); @@ -399,7 +417,7 @@ describe("autoLock service", () => { const { generatedAt, ...legacyKey } = staleValidKey; (getHashKey as jest.Mock).mockResolvedValue(legacyKey); - await refreshHashKeyExpiration(legacyKey); + await refreshHashKeyExpiration(legacyKey, GENERATION_AT_LOAD); expect(secureDataStorage.setItem).toHaveBeenCalledTimes(1); const [, value] = (secureDataStorage.setItem as jest.Mock).mock diff --git a/__tests__/services/storage/helpers.test.ts b/__tests__/services/storage/helpers.test.ts index a63cd43ac..c859d4138 100644 --- a/__tests__/services/storage/helpers.test.ts +++ b/__tests__/services/storage/helpers.test.ts @@ -1,6 +1,9 @@ +import { SENSITIVE_STORAGE_KEYS } from "config/constants"; +import { HashKey } from "config/types"; import { clearTemporaryData, - getWipeGeneration, + getHashKeyGeneration, + setHashKey, } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; @@ -20,11 +23,15 @@ jest.mock("services/storage/storageFactory", () => ({ describe("storage helpers", () => { beforeEach(() => { jest.clearAllMocks(); + // clearAllMocks keeps implementations: the held-open promises the tests + // below install would otherwise hang every later setItem/remove. + (secureDataStorage.setItem as jest.Mock).mockResolvedValue(undefined); + (secureDataStorage.remove as jest.Mock).mockResolvedValue(undefined); }); - describe("wipe generation", () => { + describe("hash-key generation", () => { it("bumps synchronously when a wipe begins, before any remove lands", async () => { - const before = getWipeGeneration(); + const before = getHashKeyGeneration(); // Hold the removes open so the wipe is genuinely in flight. const releaseRemoves: Array<() => void> = []; @@ -39,11 +46,45 @@ describe("storage helpers", () => { // An in-flight opportunistic writer (the hash-key re-anchor) checking // the generation now — mid-wipe — must already see it moved. - expect(getWipeGeneration()).toBe(before + 1); + expect(getHashKeyGeneration()).toBe(before + 1); releaseRemoves.forEach((release) => release()); await wipe; - expect(getWipeGeneration()).toBe(before + 1); + expect(getHashKeyGeneration()).toBe(before + 1); + }); + + it("bumps synchronously when a credential-verified key write begins, before it lands", async () => { + // A signIn rebuild mints a new key while a re-anchor's re-read is in + // flight. The re-anchor's write-time check must already see the + // generation moved, or it would write the OLD key back under a + // temporary store the rebuild re-encrypted with the new one. + const before = getHashKeyGeneration(); + + let releaseWrite: () => void = () => {}; + (secureDataStorage.setItem as jest.Mock).mockImplementation( + () => + new Promise((resolve) => { + releaseWrite = resolve; + }), + ); + + const newKey: HashKey = { + hashKey: "new-hash-key", + salt: "new-salt", + generatedAt: 1_800_000_000_000, + expiresAt: 1_800_000_000_000 + 1, + }; + const write = setHashKey(newKey); + + expect(getHashKeyGeneration()).toBe(before + 1); + expect(secureDataStorage.setItem).toHaveBeenCalledWith( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + JSON.stringify(newKey), + ); + + releaseWrite(); + await write; + expect(getHashKeyGeneration()).toBe(before + 1); }); }); }); diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index c1278050c..fa6180825 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -73,6 +73,8 @@ import { clearNonSensitiveData, clearTemporaryData, getHashKey, + getHashKeyGeneration, + setHashKey, } from "services/storage/helpers"; import { biometricDataStorage, @@ -506,6 +508,9 @@ const getAuthStatus = async (): Promise => { // Check if any accounts exist at all const hasAccount = (await getAllAccounts()).length > 0; + // Captured before the load so the re-anchor below can tell whether a + // wipe or credential-verified re-key began at any point after it. + const hashKeyGenerationAtLoad = getHashKeyGeneration(); const [hashKey, temporaryStore] = await Promise.all([ getHashKey(), secureDataStorage.getItem(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), @@ -631,7 +636,7 @@ const getAuthStatus = async (): Promise => { // its deadline pushed out forever by each tick. if (hashKey && temporaryStore && AppState.currentState === "active") { try { - await refreshHashKeyExpiration(hashKey); + await refreshHashKeyExpiration(hashKey, hashKeyGenerationAtLoad); } catch (error) { // The re-anchor is opportunistic: a failed keychain write must not // demote an authenticated session (the outer catch returns @@ -1036,10 +1041,7 @@ const createTemporaryStore = async (input: { if (shouldRefreshHashKey) { hashKeyObj = await generateHashKey(password); // Persist the new hash key for future decryptions - await secureDataStorage.setItem( - SENSITIVE_STORAGE_KEYS.HASH_KEY, - JSON.stringify(hashKeyObj), - ); + await setHashKey(hashKeyObj); } else { const retrievedHashKey = await getHashKey(); @@ -1604,14 +1606,11 @@ const signIn = async ({ // this call): it falls through to the full rebuild below, which mints a // fresh key instead of re-stamping stale material. const refreshNow = Date.now(); - await secureDataStorage.setItem( - SENSITIVE_STORAGE_KEYS.HASH_KEY, - JSON.stringify({ - ...existingHashKey, - expiresAt: refreshNow + HASH_KEY_EXPIRATION_MS, - generatedAt: refreshNow, - }), - ); + await setHashKey({ + ...existingHashKey, + expiresAt: refreshNow + HASH_KEY_EXPIRATION_MS, + generatedAt: refreshNow, + }); } else { // Hash key or temp store missing — rebuild session. await createTemporaryStore({ diff --git a/src/services/autoLock.ts b/src/services/autoLock.ts index a385fa35f..9665b5251 100644 --- a/src/services/autoLock.ts +++ b/src/services/autoLock.ts @@ -6,7 +6,7 @@ import { SENSITIVE_STORAGE_KEYS, } from "config/constants"; import { HashKey } from "config/types"; -import { getHashKey, getWipeGeneration } from "services/storage/helpers"; +import { getHashKey, getHashKeyGeneration } from "services/storage/helpers"; import { secureDataStorage } from "services/storage/storageFactory"; /** @@ -145,11 +145,16 @@ const resetHashKeyRefreshAttemptThrottle = (): void => { * on every 5s auth tick. * * Takes the caller-validated HashKey snapshot to run the guards and throttle - * (every caller has just loaded it for the expiry checks), then re-reads the - * stored key and the temporary store once at write time for the TOCTOU check - * below — the re-anchor requires the exact validated key and a live session. + * (every caller has just loaded it for the expiry checks) and the hash-key + * generation the caller captured BEFORE that load, then re-reads the stored + * key and the temporary store once at write time for the TOCTOU check below — + * the re-anchor requires the exact validated key, a live session, and no + * wipe or credential-verified write having begun since the caller's load. */ -const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { +const refreshHashKeyExpiration = async ( + hashKey: HashKey, + hashKeyGenerationAtLoad: number, +): Promise => { if (isHashKeyExpired(hashKey)) { return; } @@ -185,10 +190,7 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { // re-stamp, and never push out the deadline of an orphan key (no temp // store), which must hard-expire and get cleaned up instead. Checking the // store here rather than only at the call site keeps the orphan invariant - // with the helper for any future caller. The wipe-generation check below - // covers the read-to-write gap itself, so within the JS thread no - // interleaving with clearTemporaryData can resurrect wiped material. - const wipeGenerationAtRead = getWipeGeneration(); + // with the helper for any future caller. const [currentHashKey, temporaryStore] = await Promise.all([ getHashKey(), secureDataStorage.getItem(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), @@ -205,13 +207,19 @@ const refreshHashKeyExpiration = async (hashKey: HashKey): Promise => { } // The re-read above only proves the key survived up to the point those - // reads resolved — a clearTemporaryData wipe starting while they were in - // flight could still have its removes land after the write below. The - // wipe bumps its generation synchronously at entry, so this check (no - // await between it and the write) refuses that interleaving: a wipe that - // instead starts after the write is dispatched has its removes land last - // and wins. Either ordering ends with no resurrected key. - if (getWipeGeneration() !== wipeGenerationAtRead) { + // reads resolved. A clearTemporaryData wipe or a credential-verified + // setHashKey that began anywhere after the caller loaded the key — even + // before this helper was entered — can still have its removes/write land + // after the write below, and a stale re-read would not show it. Both bump + // the generation synchronously at entry, so comparing against the value the + // caller captured before its load (no await between here and the write) + // refuses every such interleaving. A mutation that starts after the write + // is dispatched is outside this check and relies on native ordering: the + // iOS keychain bridge is a serial queue, but Android runs set in a mutex'd + // coroutine and reset synchronously, so a remove can land before a write + // dispatched earlier. That residual is accepted and shared with signIn's + // fast-path read-modify-write. + if (getHashKeyGeneration() !== hashKeyGenerationAtLoad) { return; } diff --git a/src/services/storage/helpers.ts b/src/services/storage/helpers.ts index 6d11e0aa3..130875e01 100644 --- a/src/services/storage/helpers.ts +++ b/src/services/storage/helpers.ts @@ -5,21 +5,38 @@ import { secureDataStorage, } from "services/storage/storageFactory"; -// Monotonic counter bumped at the START of every clearTemporaryData wipe. -// Lets an in-flight opportunistic writer (the hash-key re-anchor in -// services/autoLock) detect that a wipe began after it validated storage and -// refuse its write, instead of racing the wipe's removes and re-creating -// just-wiped key material. JS is single-threaded, so a bump is visible to any -// check that runs after the wipe starts; a write dispatched before the bump -// has its removes land after it, so either ordering ends with the key absent. -let wipeGeneration = 0; +// Monotonic counter bumped synchronously at the START of every mutation of +// the stored hash key that the opportunistic re-anchor (services/autoLock) +// must yield to: a clearTemporaryData wipe or a credential-verified write via +// setHashKey. Lets a re-anchor in flight detect that storage moved after its +// caller loaded the key and refuse its write, instead of racing the mutation +// and re-creating just-wiped material or writing the old key back under a +// temporary store already re-encrypted with a new one. JS is single-threaded, +// so a bump is visible to any check that runs after the mutation starts. A +// re-anchor write dispatched before the bump is outside this check and relies +// on native keychain ordering (serial on iOS, not guaranteed on Android). +let hashKeyGeneration = 0; /** - * Current wipe generation — see wipeGeneration above. Capture before an - * optimistic read-validate-write sequence and compare (synchronously, with no - * await in between) right before the write. + * Current hash-key generation — see hashKeyGeneration above. Capture BEFORE + * loading the key for an optimistic read-validate-write sequence and compare + * (synchronously, with no await in between) right before the write. */ -const getWipeGeneration = (): number => wipeGeneration; +const getHashKeyGeneration = (): number => hashKeyGeneration; + +/** + * Persists a credential-verified hash key (sign-in, sign-up, import). Every + * such write must come through here so the generation bump above happens + * before the write is dispatched; the re-anchor is the only writer that may + * bypass it, because it is the one checking the generation. + */ +const setHashKey = async (hashKey: HashKey): Promise => { + hashKeyGeneration += 1; + await secureDataStorage.setItem( + SENSITIVE_STORAGE_KEYS.HASH_KEY, + JSON.stringify(hashKey), + ); +}; /** * Clears the hash key, temporary store, and derived key cache from secure storage. @@ -31,7 +48,7 @@ const getWipeGeneration = (): number => wipeGeneration; * the logout-wipe path alongside clearAllWebViewData. */ const clearTemporaryData = async (): Promise => { - wipeGeneration += 1; + hashKeyGeneration += 1; await Promise.all([ secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.HASH_KEY), secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE), @@ -74,5 +91,6 @@ export { clearTemporaryData, clearNonSensitiveData, getHashKey, - getWipeGeneration, + getHashKeyGeneration, + setHashKey, }; From 99494f6ea0320e8a082acc402a599010168cd1c2 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Tue, 10 Nov 2026 12:57:31 -0500 Subject: [PATCH 13/13] fix(auth): keep the LOCKED marker when the key reads as expired Cassio's review found that clearing the persisted soft-lock marker on hard expiry (added in bb83074 for the "~96h not enforced" thread) let a clock change undo the lock: expiry is judged by Date.now(), so moving the clock past expiresAt (or back before generatedAt) made getAuthStatus return HASH_KEY_EXPIRED and delete the marker; restoring the clock then found a valid key, no marker and no backgrounded-at, and resolved AUTHENTICATED with no password. On main the branch returned LOCKED regardless of the clock. The marker is now left in place and the branch just returns HASH_KEY_EXPIRED. It is the only clock-independent lock state, and every route back to AUTHENTICATED (signIn after the password, signUp and importWallet via resetAutoLockForNewWallet) already clears it. A genuine 72h idle still gets the full rebuild, and the LOCKED fast path's !isHashKeyExpired guard keeps an expired key off it. Trade-off: a clock rolled back after a real expiry gets the fast unlock instead of the full rebuild; it still needs the password. Tests: Cassio's two clock-restore cases (forward past expiresAt, back before generatedAt) against stateful secure storage, seen failing on 0b5366dc with Received: "AUTHENTICATED"; the expired-under-soft-lock test now asserts the marker is NOT removed; the marker-clear-failure it.each drops its expired-key row since that path no longer clears anything. Co-Authored-By: Claude Fable 5.1 --- __tests__/ducks/auth.test.ts | 107 ++++++++++++++++++++++++++++++----- src/ducks/auth.ts | 8 ++- 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/__tests__/ducks/auth.test.ts b/__tests__/ducks/auth.test.ts index c4c75e781..5979f3a06 100644 --- a/__tests__/ducks/auth.test.ts +++ b/__tests__/ducks/auth.test.ts @@ -1768,6 +1768,94 @@ describe("auth duck", () => { }); describe("getAuthStatus with LOCKED state", () => { + // The hard-expiry decision reads the wall clock, which the device holder + // controls. The persisted marker is the only clock-independent lock state, + // so restoring the clock should land back on LOCKED (password required), + // not AUTHENTICATED. + it.each([ + ["moved forward past expiresAt", 73 * 3600000], + ["moved back before generatedAt", -24 * 3600000], + ])( + "should stay LOCKED when the clock is %s and then restored", + async (_label, clockOffsetMs) => { + const realNow = Date.now(); + const nowSpy = jest.spyOn(Date, "now").mockReturnValue(realNow); + const getItemMock = secureDataStorage.getItem as jest.Mock; + const setItemMock = secureDataStorage.setItem as jest.Mock; + const removeMock = secureDataStorage.remove as jest.Mock; + const savedImpls = [ + getItemMock.getMockImplementation(), + setItemMock.getMockImplementation(), + removeMock.getMockImplementation(), + ] as const; + resetHashKeyRefreshAttemptThrottle(); + + try { + 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); + }); + + // Stateful secure storage, so a removed marker stays removed. + const secureStore = new Map([ + [SENSITIVE_STORAGE_KEYS.AUTH_STATUS, AUTH_STATUS.LOCKED], + [SENSITIVE_STORAGE_KEYS.TEMPORARY_STORE, "encrypted-temp-store"], + ]); + getItemMock.mockImplementation((key: string) => + Promise.resolve(secureStore.get(key) ?? null), + ); + setItemMock.mockImplementation((key: string, value: string) => { + secureStore.set(key, value); + return Promise.resolve(); + }); + removeMock.mockImplementation((key: string) => { + secureStore.delete(key); + return Promise.resolve(); + }); + + // Soft-locked with a key anchored an hour ago: valid on the real clock. + (getHashKey as jest.Mock).mockResolvedValue({ + hashKey: "mock-hash-key", + salt: "mock-salt", + generatedAt: realNow - 3600000, + expiresAt: realNow + 71 * 3600000, + }); + + nowSpy.mockReturnValue(realNow + clockOffsetMs); + await act(async () => { + expect(await result.current.getAuthStatus()).toBe( + AUTH_STATUS.HASH_KEY_EXPIRED, + ); + }); + + nowSpy.mockReturnValue(realNow); + await act(async () => { + expect(await result.current.getAuthStatus()).toBe( + AUTH_STATUS.LOCKED, + ); + }); + expect(secureStore.get(SENSITIVE_STORAGE_KEYS.AUTH_STATUS)).toBe( + AUTH_STATUS.LOCKED, + ); + } finally { + nowSpy.mockRestore(); + getItemMock.mockImplementation(savedImpls[0]); + setItemMock.mockImplementation(savedImpls[1]); + removeMock.mockImplementation(savedImpls[2]); + resetHashKeyRefreshAttemptThrottle(); + } + }, + ); it("should return LOCKED when persisted status is LOCKED and temp store exists", async () => { const { result } = renderHook(() => useAuthenticationStore()); @@ -1854,9 +1942,10 @@ describe("auth duck", () => { expect(status).toBe(AUTH_STATUS.HASH_KEY_EXPIRED); }); - // The stale soft-lock marker is consumed so the next check doesn't - // resurrect the fast path. - expect(secureDataStorage.remove).toHaveBeenCalledWith( + // The marker survives: it is the only clock-independent lock state, + // so consuming it here would let a clock change undo the lock (see + // the clock-restore cases below). signIn clears it after the password. + expect(secureDataStorage.remove).not.toHaveBeenCalledWith( SENSITIVE_STORAGE_KEYS.AUTH_STATUS, ); }); @@ -1911,18 +2000,6 @@ describe("auth duck", () => { // 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", { diff --git a/src/ducks/auth.ts b/src/ducks/auth.ts index fa6180825..46ed42b20 100644 --- a/src/ducks/auth.ts +++ b/src/ducks/auth.ts @@ -534,13 +534,17 @@ const getAuthStatus = async (): Promise => { // with the TTL re-anchored on activity, an expired key here means a full // backstop window (72h) of no use — exactly the genuine-idle case that // must force the full password re-auth instead of the fast path. The - // stale soft-lock marker is consumed so later checks can't resurrect it. + // marker is deliberately left in place: expiry is judged by the device + // clock, and the marker is the only clock-independent lock state, so + // clearing it would let a clock change (forward past expiresAt, or back + // before generatedAt) followed by a restore resolve AUTHENTICATED with no + // password. The store's signIn wrapper clears it once the password is + // verified (the duck-level signIn below does not touch it). const persistedAuthStatus = await secureDataStorage.getItem( SENSITIVE_STORAGE_KEYS.AUTH_STATUS, ); if (persistedAuthStatus === AUTH_STATUS.LOCKED) { if (hashKey && isHashKeyExpired(hashKey)) { - await clearPersistedAuthStatus(); return AUTH_STATUS.HASH_KEY_EXPIRED; } if (temporaryStore) {