From 9f23478467cae6ba9f4f52ff5ea4bc98f646ba96 Mon Sep 17 00:00:00 2001 From: Piyal Basu Date: Fri, 28 Aug 2026 11:59:48 -0400 Subject: [PATCH 01/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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", () => {