Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…#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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Re-anchors hash-key expiration on foreground activity while preserving hard-expiry and clock-rollback protections.
Changes:
- Adds throttled, TOCTOU-aware hash-key expiration refresh.
- Integrates refresh into active authenticated checks.
- Expands unit and integration coverage for expiry behavior and races.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/services/autoLock.ts |
Adds expiry validation and refresh logic. |
src/ducks/auth.ts |
Refreshes valid keys during foreground authentication checks. |
src/config/constants.ts |
Defines and documents the one-hour refresh throttle. |
__tests__/services/autoLock.test.ts |
Tests refresh guards, throttling, and TOCTOU behavior. |
__tests__/ducks/auth.test.ts |
Tests authentication-flow integration and failure cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
iOS Simulator preview build is ready: https://github.com/stellar/freighter-mobile/releases/tag/untagged-69c7c0f8bb6e9a86edaa |
|
Android Emulator preview APK is ready: https://github.com/stellar/freighter-mobile/releases/tag/untagged-52f38b8aec2f4542ba80 |
…lback 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
The fast-path guard added in bb83074 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 <noreply@anthropic.com>
| if (hashKey && isHashKeyExpired(hashKey)) { | ||
| await secureDataStorage.remove(SENSITIVE_STORAGE_KEYS.AUTH_STATUS); | ||
| return AUTH_STATUS.HASH_KEY_EXPIRED; | ||
| } |
TL;DR
Re-anchors the hash-key hard-expiry backstop on activity instead of sign-in (closes #924, follow-up to #905): every authenticated check while the app is actively foregrounded pushes the 72h deadline out, throttled to at most one secure-storage write per hour. An actively-used wallet — including one on the 24h auto-lock preset — can no longer hit a surprise full re-authentication, while a device left idle for the backstop window still forces a full password re-auth on next open, matching the extension's session model.
Security posture, stated explicitly: the hard expiry now fires 71–72h after the last foreground use, or up to ~96h after the last human interaction in the degenerate case where the app is left foregrounded and untouched on the 24h preset until the foreground-idle soft lock trips. Expired, clock-rolled-back, soft-locked, and backgrounded states can never extend the deadline, and a key wiped by logout mid-check cannot be resurrected.
Implementation details (for agents)
What changed:
src/services/autoLock.ts— newrefreshHashKeyExpiration(hashKey): re-stampsexpiresAt/generatedAttonow + HASH_KEY_EXPIRATION_MSiff the key is valid. Guards, in order: never refresh an expired or clock-rolled-back key (isHashKeyExpired, which moved here fromducks/auth.ts— auth.ts already imports from this module, so the move avoids a cycle); skip whilegeneratedAtis younger than the 1h throttle; skip while a prior attempt is younger than the throttle (module-levellastRefreshAttemptAt, so a persistently failing keychain write can't retry+log every 5s tick); TOCTOU re-read — the write proceeds only if the stored key is field-for-field identical to the validated snapshot, and spreads the re-read key, so a concurrentclearTemporaryDatawipe or credential-verified re-stamp is never clobbered. A legacy key withoutgeneratedAtrefreshes immediately and self-upgrades; no migration needed.freighter-mobile/src/services/autoLock.ts
Lines 100 to 199 in a559017
src/ducks/auth.ts— the single call site ingetAuthStatus, gated byhashKey && temporaryStore && AppState.currentState === "active"and placed strictly after the LOCKED / hard-expiry / rollback / soft-lock checks (all of which return early), wrapped in try/catch so a keychain-write failure logs instead of escaping to the outer catch (which returns NOT_AUTHENTICATED and would demote an authenticated session). ThetemporaryStorecondition means an orphan key (temp store wiped, key remove failed) is never re-anchored — it hard-expires and gets cleaned up. Removed the now-false "TTL deliberately NOT refreshed here" comment.freighter-mobile/src/ducks/auth.ts
Lines 544 to 583 in a559017
src/config/constants.ts—HASH_KEY_EXPIRATION_MSstays 72h (it must exceed the 24h max preset so that preset's fast unlock stays reachable — hard expiry is checked before the soft timer); comment rewritten for the new inactivity semantics, including the 71–72h effective bound and the ~96h foregrounded-idle worst case. NewHASH_KEY_REFRESH_THROTTLE_MS = 1h.freighter-mobile/src/config/constants.ts
Lines 92 to 118 in a559017
src/hooks/useAuthCheck.ts— untouched: every foreground transition, cold start, and periodic tick already funnels throughgetAuthStatus.__tests__/services/autoLock.test.tsunit-covers the helper (stale refresh, throttle, attempt throttle + reset, expired/rollback refusal, legacy key, TOCTOU: missing/different/identical stored key);__tests__/ducks/auth.test.tsintegration-coversgetAuthStatusagainst the REAL helper (re-anchor on active use, throttle, background tick never refreshes nor consumes the soft timer, expired/rollback still expire, wipe-mid-check not resurrected, orphan key not re-anchored, write failure stays AUTHENTICATED without touching the outer catch). Mutation-verified during development: removing the try/catch, the active gate, or the TOCTOU guard each flips a specific test red.Verification: full suite 3065 passed / 226 suites / 0 failures (same totals as base plus the 20 new tests);
yarn lint:tsclean; eslint clean on all touchedsrcfiles. Pre-existing repo-wideyarn lint:checkfailures (56 errors, incl. aquotes/prettier conflict on one line ofauth.test.ts) are verbatim onmainand untouched — CI runsyarn test --cionly.Follow-ups / out of scope:
signInLOCKED fast path still hand-rolls the same anchor write (deliberately unthrottled/unguarded there — credential-verified and it should clobber); extracting a sharedanchorHashKeyExpirationwould keep the shape in one place.getAuthStatus; this PR stops re-anchoring it so it now hard-expires, but the status semantics deserve their own issue.clearTemporaryDatabegins, checked with no await before the write) refuses a re-anchor that interleaves with a wipe. Residual exposure is native keychain queue reordering only — the same assumption the existingsignInfast-path read-modify-write already makes.Closes #924
🤖 Generated with Claude Code