From c39f803c9762d70b14fc176bc851bec39fafa299 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 03:28:52 -0500 Subject: [PATCH] fix(security): app-lock grace survives activity recreation; clarify passphrase eviction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two lifecycle-consistency fixes from PR #45's review (issue #101). 1. Grace across Back/recreation. The AppLockGate state machine was a field of the Activity-scoped AppLockViewModel, so Back on the task root (which finishes the Activity and clears its ViewModelStore on API 29/30) dropped the grace marker and re-armed a fresh LOCKED gate, demanding full re-auth on return — unlike leaving via Home. Provide AppLockGate as an application-scoped @Singleton (SecurityModule) and inject it into the ViewModel, so the same instance is reused across recreation and the 30s grace behaves identically for Back and Home. A genuine cold start (process death) still constructs a fresh, LOCKED gate. 2. PassphraseSession eviction. The KDoc promised the passphrase is "cleared on lock, timeout," but nothing re-locked it on grace expiry and full eviction is not achievable without a DB close/reopen (provideDatabase runs once per process; owned by #93 / #111). Correct the KDoc to state the process-lifetime limitation explicitly and add a code comment at the timeout re-lock deferring full eviction to #93 / #111. We deliberately do NOT call session.lock() on timeout: it is the only separately-held copy but also drives EncryptedCacheGuard, so clearing it while merely locked (not exited) would stall background sync/push even though the DB stays open — not a correct partial eviction. No DatabaseModule changes. Tests (JVM): extend AppLockGateTest to cover grace surviving a reused-instance recreation within and beyond the window, and a fresh gate starting LOCKED; add AppLockViewModelTest asserting the gate is an injected dependency the ViewModel delegates to (onBackground/onAuthError). Co-Authored-By: Claude Fable 5 --- .../libremail/data/security/AppLockGate.kt | 8 ++ .../data/security/PassphraseSession.kt | 29 +++++-- .../kotlin/org/libremail/di/SecurityModule.kt | 14 +++ .../org/libremail/ui/lock/AppLockViewModel.kt | 19 +++- .../data/security/AppLockGateTest.kt | 41 +++++++++ .../libremail/ui/lock/AppLockViewModelTest.kt | 87 +++++++++++++++++++ 6 files changed, 189 insertions(+), 9 deletions(-) create mode 100644 app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt diff --git a/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt b/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt index 83c2a47..621ddbf 100644 --- a/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt +++ b/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt @@ -15,6 +15,14 @@ enum class LockState { LOCKED, UNLOCKED } * - Returning to the foreground within [graceMillis] of backgrounding stays unlocked; after the * grace period it re-locks. Foregrounding without a preceding background (e.g. a configuration * change / rotation) does not re-lock an already-unlocked session. + * + * Lifetime: provided as an application-scoped singleton (see `SecurityModule`) so the grace window + * survives Activity recreation. If this state lived in the Activity-scoped [org.libremail.ui.lock + * .AppLockViewModel]'s own field, Back on the task root — which finishes the Activity and clears its + * ViewModelStore on API 29/30 — would drop the [backgroundedAt] marker and re-arm a fresh LOCKED gate, + * so returning within the grace period would wrongly demand re-auth (unlike leaving via Home). Being + * process-scoped, the same instance is reused across recreation, while a genuine cold start (process + * death) constructs a fresh one that correctly starts [LockState.LOCKED]. */ class AppLockGate(private val graceMillis: Long = DEFAULT_GRACE_MILLIS) { diff --git a/app/src/main/kotlin/org/libremail/data/security/PassphraseSession.kt b/app/src/main/kotlin/org/libremail/data/security/PassphraseSession.kt index 6d3979d..3c9183d 100644 --- a/app/src/main/kotlin/org/libremail/data/security/PassphraseSession.kt +++ b/app/src/main/kotlin/org/libremail/data/security/PassphraseSession.kt @@ -12,12 +12,25 @@ import javax.inject.Singleton * * When app-lock is enabled the database passphrase is sealed by an auth-bound Keystore key * ([DatabaseKeyCipher]) and is only recoverable after the user passes a `BiometricPrompt`. The - * unwrapped passphrase is kept here for the lifetime of the unlocked session — never persisted — and - * cleared on lock, timeout, or when app-lock is disabled. `DatabaseModule` reads it through [await] - * so the encrypted database is opened only after authentication. + * unwrapped passphrase is kept here for the unlocked session — never persisted. `DatabaseModule` + * reads it through [await] so the encrypted database is opened only after authentication. * - * The value is an immutable [String] to stay consistent with the rest of the passphrase plumbing; it - * cannot be zeroed in place, which is an accepted limitation of the existing design. + * Eviction — current limitation: the passphrase is NOT guaranteed to leave the process when the app + * re-locks. [lock] is called on app-lock disable and cache reset ([DatabaseKeyStore]), but it is + * deliberately NOT called on the app-lock UI gate's grace-expiry/timeout re-lock, and even if it were + * it would only drop THIS holder's reference: + * - the value is an immutable [String] and cannot be zeroed in place — [lock] merely releases it + * for GC; + * - once the encrypted cache has been opened, SQLCipher keeps the key inside the already-open + * Room/native handle. `DatabaseModule.provideDatabase` runs once per process, so nothing here + * closes that handle; the key stays resident for the process lifetime regardless of [lock]; + * - this holder's unlocked-ness also drives [EncryptedCacheGuard], so clearing it while the app is + * merely locked (not exited) would stall background sync/push even though the DB is still open. + * + * Truly evicting the passphrase on lock/timeout therefore requires a database close/reopen mechanism, + * which is owned by the DB-lifecycle work in issues #93 (provideDatabase) and #111 (DB + * re-architecture) and is intentionally out of scope here. Until those land, do not rely on [lock] + * for cryptographic erasure of the passphrase from the process. */ @Singleton class PassphraseSession @Inject constructor() { @@ -29,7 +42,11 @@ class PassphraseSession @Inject constructor() { passphrase.value = value } - /** Clear the passphrase from memory (on lock, timeout, or app-lock disable). */ + /** + * Drop this holder's reference to the passphrase (on app-lock disable / cache reset). See the + * class KDoc: this is a partial eviction only — it does not close the open database, so SQLCipher + * keeps the key resident for the process lifetime (full eviction is deferred to #93 / #111). + */ fun lock() { passphrase.value = null } diff --git a/app/src/main/kotlin/org/libremail/di/SecurityModule.kt b/app/src/main/kotlin/org/libremail/di/SecurityModule.kt index 7f97534..d10da7f 100644 --- a/app/src/main/kotlin/org/libremail/di/SecurityModule.kt +++ b/app/src/main/kotlin/org/libremail/di/SecurityModule.kt @@ -3,9 +3,11 @@ package org.libremail.di import dagger.Binds import dagger.Module +import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.components.SingletonComponent import org.libremail.data.security.AndroidAppLockManager +import org.libremail.data.security.AppLockGate import org.libremail.data.security.AppLockManager import javax.inject.Singleton @@ -16,4 +18,16 @@ abstract class SecurityModule { @Binds @Singleton abstract fun bindAppLockManager(impl: AndroidAppLockManager): AppLockManager + + companion object { + /** + * The app-lock UI gate is application-scoped, NOT Activity/ViewModel-scoped, so its grace + * window survives Activity recreation (e.g. Back finishing the task root on API 29/30, which + * clears the Activity's ViewModelStore). See [AppLockGate] for why this must outlive the + * ViewModel. A fresh process constructs a new instance that correctly starts LOCKED. + */ + @Provides + @Singleton + fun provideAppLockGate(): AppLockGate = AppLockGate() + } } diff --git a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt index c394d21..88670c5 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt @@ -73,10 +73,13 @@ class AppLockViewModel @Inject constructor( private val databaseKeyCipher: DatabaseKeyCipher, private val session: PassphraseSession, private val syncScheduler: SyncScheduler, + // Application-scoped (see SecurityModule): the gate is injected rather than owned by this + // Activity-scoped ViewModel so the inactivity grace window survives Activity recreation — Back on + // the task root finishes the Activity and clears its ViewModelStore on API 29/30, which would + // otherwise drop the grace marker and force a full re-auth on return within the grace period. + private val gate: AppLockGate, ) : ViewModel() { - private val gate = AppLockGate() - private val _uiState = MutableStateFlow(AppLockUiState.Checking) val uiState: StateFlow = _uiState.asStateFlow() @@ -134,7 +137,17 @@ class AppLockViewModel @Inject constructor( LockAction.REQUIRE_AUTH -> { val state = gate.onForeground(foregroundAt, appLockEnabled = true) - if (state == LockState.UNLOCKED) _uiState.value = AppLockUiState.Unlocked else emitLocked() + if (state == LockState.UNLOCKED) { + _uiState.value = AppLockUiState.Unlocked + } else { + // Re-lock on grace expiry (timeout). The SQLCipher passphrase is intentionally + // NOT evicted here: PassphraseSession is the only separately-held copy, but + // clearing it flips EncryptedCacheGuard to "locked" and would stall background + // sync/push while locked, and the already-open Room handle keeps the key + // resident regardless. Full eviction needs the DB close/reopen owned by #93 / + // #111 — see PassphraseSession's KDoc. + emitLocked() + } } } } diff --git a/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt b/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt index 1ab1bee..2c8d80c 100644 --- a/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt +++ b/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt @@ -100,4 +100,45 @@ class AppLockGateTest { gate.onForeground(now = 1_000L, appLockEnabled = true) // stale -> LOCKED, marker retained assertEquals(LockState.LOCKED, gate.onForeground(now = 3_000L, appLockEnabled = true)) } + + @Test + fun `grace survives activity recreation when the gate instance is reused (app-scoped singleton)`() { + // #101: Back on the task root (API 29/30) finishes the Activity and clears its ViewModelStore, + // so AppLockViewModel is destroyed and re-created. Because the gate is application-scoped the + // SAME instance is reused across that recreation: the background marker recorded before the + // store was cleared is still present, so returning within grace stays unlocked — identical to + // leaving via Home and returning. + val survivingGate = AppLockGate(grace) + survivingGate.onForeground(now = 0, appLockEnabled = true) + survivingGate.onAuthenticated() + survivingGate.onBackground(now = 1_000) // ON_STOP as Back finishes the Activity + // ...Activity destroyed + re-created; the same singleton gate is handed to the new ViewModel... + assertEquals( + LockState.UNLOCKED, + survivingGate.onForeground(now = 1_000 + grace, appLockEnabled = true), + ) + } + + @Test + fun `grace still expires across activity recreation when the reused gate is out of the window`() { + val survivingGate = AppLockGate(grace) + survivingGate.onForeground(now = 0, appLockEnabled = true) + survivingGate.onAuthenticated() + survivingGate.onBackground(now = 1_000) + assertEquals( + LockState.LOCKED, + survivingGate.onForeground(now = 1_000 + grace + 1, appLockEnabled = true), + ) + } + + @Test + fun `a fresh gate (process death, or the pre-fix Activity-scoped bug) starts locked`() { + // Contrast with the reused instance above: if the gate were Activity/ViewModel-scoped (the + // pre-fix bug) or after a genuine cold start (process death), recreation builds a FRESH gate. + // It must start LOCKED and stay locked on the first foreground even within the grace window — + // the re-auth-on-Back inconsistency #101 fixes, and the correct cold-start behavior. + val freshGate = AppLockGate(grace) + assertEquals(LockState.LOCKED, freshGate.state) + assertEquals(LockState.LOCKED, freshGate.onForeground(now = 1_000, appLockEnabled = true)) + } } diff --git a/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt new file mode 100644 index 0000000..7efcaa0 --- /dev/null +++ b/app/src/test/kotlin/org/libremail/ui/lock/AppLockViewModelTest.kt @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.ui.lock + +import android.os.SystemClock +import io.mockk.every +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.unmockkAll +import io.mockk.verify +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.libremail.data.security.AppLockGate +import org.libremail.data.settings.AppSettings +import org.libremail.data.settings.SettingsRepository +import kotlin.test.assertEquals +import kotlin.test.assertIs + +/** + * Wiring guard for #101: the app-lock gate must be an INJECTED, application-scoped dependency so its + * grace window survives Activity recreation — NOT a field the Activity-scoped ViewModel constructs + * itself (which Back on the task root would drop on API 29/30). These tests exercise the synchronous + * paths that delegate to the injected gate; the grace math itself is covered exhaustively — and + * deterministically — by AppLockGateTest. Broader ViewModel coverage is issue #100. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class AppLockViewModelTest { + + private val dispatcher = UnconfinedTestDispatcher() + + @Before + fun setUp() = Dispatchers.setMain(dispatcher) + + @After + fun tearDown() { + Dispatchers.resetMain() + unmockkAll() + } + + private fun viewModel(gate: AppLockGate, appLock: Boolean = true): AppLockViewModel { + val settings = mockk() + every { settings.settings } returns flowOf(AppSettings(appLock = appLock)) + return AppLockViewModel( + context = mockk(relaxed = true), + settingsRepository = settings, + appLockManager = mockk(relaxed = true), + databaseKeyStore = mockk(relaxed = true), + databaseKeyCipher = mockk(relaxed = true), + session = mockk(relaxed = true), + syncScheduler = mockk(relaxed = true), + gate = gate, + ) + } + + @Test + fun `onBackground records the background on the injected gate`() = runTest(dispatcher) { + mockkStatic(SystemClock::class) + every { SystemClock.elapsedRealtime() } returns 5_000L + val gate = mockk(relaxed = true) + + viewModel(gate).onBackground() + + // Delegating to the gate passed into the constructor (an injected, shareable singleton) rather + // than an internally-constructed one is the whole point of the #101 hoist: the same instance + // must survive Activity recreation so the grace marker persists. + verify { gate.onBackground(5_000L) } + } + + @Test + fun `onAuthError re-locks through the injected gate and surfaces the error`() = runTest(dispatcher) { + val gate = mockk(relaxed = true) + val vm = viewModel(gate) + + vm.onAuthError("boom") + + verify { gate.lock() } + val state = assertIs(vm.uiState.value) + assertEquals("boom", state.error) + } +} -- 2.47.3