From 0aa47ddf425666636394fd436bd6ff7360915882 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 09:52:34 -0400 Subject: [PATCH 01/10] Read the PIN record three-way so an unreadable one keeps every restriction --- .../keep/PinUnlockScreenUnreadableTest.kt | 182 +++++++++++++++++ .../keep/storage/PinStoreUnreadableTest.kt | 171 ++++++++++++++++ .../kotlin/io/privkey/keep/PinUnlockScreen.kt | 56 +++++- .../io/privkey/keep/storage/PinStore.kt | 185 ++++++++++++++++-- app/src/main/res/values/strings_pin.xml | 4 + 5 files changed, 577 insertions(+), 21 deletions(-) create mode 100644 app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt create mode 100644 app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt diff --git a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt new file mode 100644 index 00000000..a45b0018 --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt @@ -0,0 +1,182 @@ +package io.privkey.keep + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onAllNodesWithText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import io.privkey.keep.storage.KeystoreEncryptedPrefs +import io.privkey.keep.storage.PinRecord +import io.privkey.keep.storage.PinStore +import io.privkey.keep.storage.breakNameResolution +import io.privkey.keep.storage.corruptStoredValue +import io.privkey.keep.storage.forgetWriteProbe +import io.privkey.keep.storage.writeAndFindStoredName +import io.privkey.keep.ui.theme.KeepAndroidTheme +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith + +/** + * The way out of an unreadable PIN record, which is what makes failing the read closed + * safe to do at all. + * + * With the record unreadable the gate asks, the hash in the same file cannot verify any + * PIN, and waiting does not help, so without this screen state the person is locked out + * of an app holding their keys. These assert that the state is reported, that the + * removal is offered, and that it is not offered for a record that reads fine. + */ +@RunWith(AndroidJUnit4::class) +class PinUnlockScreenUnreadableTest { + + // createComposeRule() is deprecated in favor of the v2 API; the classic rule + // is intentional here, and the project builds with -Werror. + @Suppress("DEPRECATION") + @get:Rule + val compose = createComposeRule() + + private val context = InstrumentationRegistry.getInstrumentation().targetContext + + private val unreadableTitle get() = context.getString(R.string.pin_unlock_unreadable_title) + private val removeLabel get() = context.getString(R.string.pin_unlock_unreadable_remove) + + @Before + fun setup() { + clear() + } + + @After + fun teardown() { + clear() + } + + private fun clear() { + context.deleteSharedPreferences(PinStore.PREFS_NAME) + } + + private fun showScreen( + onBiometricAuth: (suspend () -> Boolean)? = null, + onUnlocked: () -> Unit = {} + ) { + compose.setContent { + KeepAndroidTheme { + PinUnlockScreen( + pinStore = PinStore(context), + onUnlocked = onUnlocked, + onBiometricAuth = onBiometricAuth + ) + } + } + } + + private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = 5_000) { + compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() + } + + @Test + fun anUnreadableRecordIsReportedAndOffersRemoval() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + showScreen() + + awaitText(unreadableTitle) + compose.onNodeWithText(unreadableTitle).assertIsDisplayed() + compose.onNodeWithText(removeLabel).assertIsDisplayed() + // Not the lockout state, which an unreadable record also reports and which + // offers only waiting. + compose.onNodeWithText( + context.getString(R.string.pin_unlock_locked_out_title) + ).assertDoesNotExist() + } + + // The readable case, so the assertions above cannot pass on a screen that offers + // removal whatever it reads. + @Test + fun aReadablePinOffersNoRemoval() { + PinStore(context).setPin(GOOD_PIN) + showScreen() + + awaitText(context.getString(R.string.pin_unlock_button)) + compose.onNodeWithText(unreadableTitle).assertDoesNotExist() + compose.onNodeWithText(removeLabel).assertDoesNotExist() + } + + @Test + fun removingAnUnreadableRecordClearsItAndUnlocks() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + var unlocked = false + showScreen(onUnlocked = { unlocked = true }) + + awaitText(unreadableTitle) + compose.onNodeWithText(removeLabel).performClick() + + compose.waitUntil(timeoutMillis = 5_000) { unlocked } + assertTrue("removal has to unlock, or the screen has no exit", unlocked) + assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) + assertFalse(PinStore(context).requiresAuthentication()) + } + + /** + * With biometrics enrolled the removal waits on them, because it takes away an + * authentication factor and the person asking has not authenticated. A refused + * prompt must leave the record alone. + */ + @Test + fun aRefusedBiometricLeavesTheRecordAlone() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + var unlocked = false + showScreen(onBiometricAuth = { false }, onUnlocked = { unlocked = true }) + + awaitText(unreadableTitle) + compose.onNodeWithText(removeLabel).performClick() + + compose.waitForIdle() + assertFalse("a refused prompt must not remove the PIN", unlocked) + assertEquals(PinRecord.UNREADABLE, PinStore(context).pinRecord()) + } + + @Test + fun anAcceptedBiometricRemovesTheRecord() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + var unlocked = false + showScreen(onBiometricAuth = { true }, onUnlocked = { unlocked = true }) + + awaitText(unreadableTitle) + compose.onNodeWithText(removeLabel).performClick() + + compose.waitUntil(timeoutMillis = 5_000) { unlocked } + assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) + } + + /** + * A single corrupted value rather than the whole file, since that is the other + * shape the fault takes and the screen must reach the same state for it. + */ + @Test + fun aSingleUnreadableValueReachesTheSameState() { + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putBoolean("pin_enabled", true).commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + showScreen() + + awaitText(unreadableTitle) + compose.onNodeWithText(removeLabel).assertIsDisplayed() + forgetWriteProbe(context, PinStore.PREFS_NAME) + } + + private companion object { + const val GOOD_PIN = "4917" + } +} diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt new file mode 100644 index 00000000..4360766b --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt @@ -0,0 +1,171 @@ +package io.privkey.keep.storage + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith + +/** + * What the PIN store answers when its record cannot be read. + * + * Every value in this file carries a restriction: whether a PIN is set, how long the + * lockout has left, how much of the attempt budget is spent, and whether a biometric + * reset is owed. The ordinary getters answered the caller's default for both "nothing + * stored" and "stored and will not decrypt", and for all four of those the default is + * the permissive answer, so a record that would not decrypt read as no PIN, no lockout, + * no attempts used and no reset owed. All three PIN gates ask through + * requiresAuthentication, so that one answer stopped every one of them asking. + * + * Faults are staged on disk in both the shapes a lost Keystore alias produces: one + * value that will not decrypt, and the file's own HMAC key gone, which makes resolving + * any name in it raise rather than answer. + */ +@RunWith(AndroidJUnit4::class) +class PinStoreUnreadableTest { + + private val context: Context get() = ApplicationProvider.getApplicationContext() + + @Before + fun setup() { + clear() + } + + @After + fun teardown() { + clear() + } + + private fun clear() { + context.deleteSharedPreferences(PinStore.PREFS_NAME) + } + + /** Corrupts the stored pin_enabled value alone, leaving the rest of the file sound. */ + private fun breakTheEnabledFlag() { + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putBoolean("pin_enabled", true).commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + } + + // The control. Without it every assertion below would pass on a store that reported + // a fault whatever it read. + @Test + fun a_readable_pin_enables_the_gate_and_verifies() { + val store = PinStore(context) + assertTrue(store.setPin(GOOD_PIN)) + + assertEquals(PinRecord.SET, store.pinRecord()) + assertTrue(store.isPinEnabled()) + assertTrue(store.verifyPin(GOOD_PIN)) + assertFalse(store.verifyPin(WRONG_PIN)) + } + + @Test + fun a_store_with_no_pin_does_not_ask() { + val store = PinStore(context) + assertEquals(PinRecord.NOT_SET, store.pinRecord()) + assertFalse(store.isPinEnabled()) + assertFalse(store.requiresAuthentication()) + } + + /** + * The headline. A flag that will not decrypt must not read as "no PIN set", because + * that answer takes the gate off at MainActivity, Nip55Activity and + * Nip55ContentProvider at once, without anything raising or being logged. + */ + @Test + fun an_unreadable_pin_flag_keeps_the_gate_asking() { + breakTheEnabledFlag() + + val store = PinStore(context) + assertEquals(PinRecord.UNREADABLE, store.pinRecord()) + assertTrue( + "an unreadable flag must not read as no PIN", + store.isPinEnabled() + ) + assertTrue( + "and the gates ask through this one", + store.requiresAuthentication() + ) + forgetWriteProbe(context, PinStore.PREFS_NAME) + } + + /** + * The throttle is the other half: an unreadable record used to report a full attempt + * budget and no lockout, which is what an attacker able to damage this file would + * want, since the hash may still be intact and brute-forceable. + */ + @Test + fun an_unreadable_pin_record_reports_the_throttle_in_force() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + + val store = PinStore(context) + assertEquals(PinRecord.UNREADABLE, store.pinRecord()) + assertTrue("the lockout must not read as expired", store.isLockedOut()) + assertEquals("nor the attempt budget as unspent", 0, store.getRemainingAttempts()) + assertTrue("nor a reset owed as not owed", store.requiresBiometricReset()) + assertFalse("and no PIN can verify against it", store.verifyPin(GOOD_PIN)) + } + + /** + * Failing closed is only safe because of this: the hash is in the same file, so no + * PIN can verify, and without a way out the person would be locked out of the app + * with their keys inside it. + * + * Staged with the HMAC key broken, which is the mode where removing a single key + * raises instead of answering, so this also pins that the clear is the delete that + * works here. + */ + @Test + fun clearing_an_unreadable_pin_record_lets_a_new_pin_be_set() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + assertTrue(PinStore(context).requiresAuthentication()) + + assertTrue("the clear has to answer, not raise", PinStore(context).clearPin()) + + val repaired = PinStore(context) + assertEquals(PinRecord.NOT_SET, repaired.pinRecord()) + assertFalse(repaired.requiresAuthentication()) + assertTrue("and a new PIN can be set afterwards", repaired.setPin(OTHER_PIN)) + assertTrue(repaired.verifyPin(OTHER_PIN)) + assertFalse(repaired.verifyPin(GOOD_PIN)) + } + + /** + * The clear is reachable from the unlock screen, which is shown to someone who has + * not authenticated, so it refuses a record it can read. Checked rather than + * documented, because a comment would not stop the call. + */ + @Test + fun the_clear_refuses_a_pin_it_can_read() { + val store = PinStore(context) + assertTrue(store.setPin(GOOD_PIN)) + + assertFalse("a readable PIN must not be removable this way", store.clearPin()) + + val after = PinStore(context) + assertEquals(PinRecord.SET, after.pinRecord()) + assertTrue("and it still verifies", after.verifyPin(GOOD_PIN)) + } + + @Test + fun the_clear_refuses_when_no_pin_is_set() { + assertFalse(PinStore(context).clearPin()) + } + + private companion object { + // None of these trip isWeakPin: not sequential, not repeated, not in the list. + const val GOOD_PIN = "4917" + const val OTHER_PIN = "5283" + const val WRONG_PIN = "7194" + } +} diff --git a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt index 0578ebfd..de70c1d0 100644 --- a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt +++ b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt @@ -16,6 +16,7 @@ import androidx.compose.ui.text.input.KeyboardType import androidx.compose.ui.text.input.PasswordVisualTransformation import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp +import io.privkey.keep.storage.PinRecord import io.privkey.keep.storage.PinStore import kotlinx.coroutines.delay import kotlinx.coroutines.launch @@ -32,6 +33,8 @@ fun PinUnlockScreen( var isLockedOut by remember { mutableStateOf(pinStore.isLockedOut()) } var lockoutRemaining by remember { mutableStateOf(pinStore.getLockoutRemainingMs()) } var biometricResetRequired by remember { mutableStateOf(pinStore.requiresBiometricReset()) } + var pinRecord by remember { mutableStateOf(pinStore.pinRecord()) } + var removeFailed by remember { mutableStateOf(false) } val focusRequester = remember { FocusRequester() } val coroutineScope = rememberCoroutineScope() val context = LocalContext.current @@ -116,7 +119,58 @@ fun PinUnlockScreen( Spacer(modifier = Modifier.height(32.dp)) - if (biometricResetRequired && onBiometricAuth != null && !isLockedOut) { + // Ahead of the lockout and biometric-reset branches, because an unreadable + // record makes both of those report their strictest answer and neither has + // a way out: no PIN can verify against a hash that will not decrypt, and + // waiting does not make the record readable. This is the only exit, so it + // has to be the state the screen shows. + if (pinRecord == PinRecord.UNREADABLE) { + Text( + text = stringResource(R.string.pin_unlock_unreadable_title), + style = MaterialTheme.typography.titleMedium, + color = MaterialTheme.colorScheme.error + ) + Spacer(modifier = Modifier.height(8.dp)) + Text( + text = stringResource(R.string.pin_unlock_unreadable_body), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center + ) + if (removeFailed) { + Spacer(modifier = Modifier.height(8.dp)) + Text( + text = stringResource(R.string.pin_unlock_unreadable_remove_failed), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.error, + textAlign = TextAlign.Center + ) + } + Spacer(modifier = Modifier.height(24.dp)) + Button( + onClick = { + coroutineScope.launch { + // Behind biometrics when the device has them, since this + // removes an authentication factor and the person asking + // has not authenticated. With no factor enrolled there is + // nothing to ask for, and refusing would leave the app with + // no way in at all, which is the same convention + // requireAuthThen follows elsewhere. + val authed = onBiometricAuth?.invoke() ?: true + if (!authed) return@launch + removeFailed = !pinStore.clearPin() + if (!removeFailed) { + pinRecord = pinStore.pinRecord() + pinStore.refreshSession() + onUnlocked() + } + } + }, + modifier = Modifier.fillMaxWidth() + ) { + Text(stringResource(R.string.pin_unlock_unreadable_remove)) + } + } else if (biometricResetRequired && onBiometricAuth != null && !isLockedOut) { Text( text = stringResource(R.string.pin_unlock_biometric_required_title), style = MaterialTheme.typography.titleMedium, diff --git a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt index 26d69340..b51b9e02 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt @@ -1,7 +1,6 @@ package io.privkey.keep.storage import android.content.Context -import android.content.SharedPreferences import android.os.SystemClock import android.util.Base64 import android.util.Log @@ -11,10 +10,25 @@ import java.security.SecureRandom import javax.crypto.SecretKeyFactory import javax.crypto.spec.PBEKeySpec +/** + * What the stored record can say about whether a PIN is set. + * + * Three-way for the same reason [PrefRead] is: this record carries a restriction, and + * the ordinary getters answer the caller's default for both "nothing is stored" and + * "something is stored and will not decrypt", which are opposite facts here. The first + * means no PIN gate; the second means a PIN may be set and cannot be verified. + */ +enum class PinRecord { NOT_SET, SET, UNREADABLE } + class PinStore(private val context: Context) { companion object { - private const val PREFS_NAME = "keep_pin_protection" + /** + * Not private: the tests that stage an unreadable PIN record have to reach the + * real file, and a copy of the literal there would silently stop matching if + * this one changed. + */ + internal const val PREFS_NAME = "keep_pin_protection" private const val KEY_PIN_HASH = "pin_hash" private const val KEY_PIN_SALT = "pin_salt" private const val KEY_PIN_ENABLED = "pin_enabled" @@ -62,17 +76,82 @@ class PinStore(private val context: Context) { ) } - private val prefs: SharedPreferences = run { + private val prefs: ReadablePrefs = run { val newPrefs = KeystoreEncryptedPrefs.create(context, PREFS_NAME) LegacyPrefsMigration.migrateIfNeeded(context, PREFS_NAME, newPrefs) } + /** + * Every read below goes through [PrefRead] and resolves an unreadable value to the + * answer that keeps a restriction in force, never the one that lifts it. This file + * holds the PIN gate, the lockout, the attempt counter and the biometric-reset + * requirement, and all four are restrictions, so the collapsing getters turned a + * record that would not decrypt into no PIN, no lockout, no attempts used and no + * reset owed. The gates at MainActivity, Nip55Activity and Nip55ContentProvider all + * ask through [requiresAuthentication], so that answer stopped every one of them + * asking. + * + * Failing closed on its own would lock the person out of the app, since the hash in + * the same file is equally unreadable and no PIN can then verify, which is why + * [clearPin] exists and why the unlock screen offers it. + */ + @Synchronized + fun pinRecord(): PinRecord = when (val read = prefs.readBoolean(KEY_PIN_ENABLED)) { + is PrefRead.Found -> if (read.value) PinRecord.SET else PinRecord.NOT_SET + PrefRead.Absent -> PinRecord.NOT_SET + PrefRead.Unreadable -> PinRecord.UNREADABLE + } + + /** True when a PIN may be set, which includes a record that cannot be read. */ @Synchronized - fun isPinEnabled(): Boolean = prefs.getBoolean(KEY_PIN_ENABLED, false) + fun isPinEnabled(): Boolean = pinRecord() != PinRecord.NOT_SET + + /** + * Deletes the whole PIN record, the way back from a record that cannot be read. + * + * Refuses unless the record is actually unreadable, so this cannot become a way to + * remove a working PIN without verifying it. That is a check rather than a comment + * because the call is reachable from the unlock screen, which is the one screen + * shown to someone who has not authenticated yet. + * + * Clears the file rather than the keys, which is what works when the fault is the + * file's own HMAC key: resolving a name to remove it decrypts that key, so a + * targeted remove raises instead of answering, while a clear resolves no names and + * takes the damaged key material with it. + */ + @Synchronized + fun clearPin(): Boolean { + if (pinRecord() != PinRecord.UNREADABLE) return false + return runCatching { prefs.edit().clear().commit() }.getOrDefault(false) + } @Synchronized fun requiresAuthentication(): Boolean = isPinEnabled() && !isSessionValid() + // Both answers are named at every call site below. The bug these replace was that + // the getters gave one answer for both, and for roughly half of this file's keys + // the absent answer is the permissive one. + private fun longAt(key: String, ifAbsent: Long, ifUnreadable: Long): Long = + when (val read = prefs.readLong(key)) { + is PrefRead.Found -> read.value + PrefRead.Absent -> ifAbsent + PrefRead.Unreadable -> ifUnreadable + } + + private fun intAt(key: String, ifAbsent: Int, ifUnreadable: Int): Int = + when (val read = prefs.readInt(key)) { + is PrefRead.Found -> read.value + PrefRead.Absent -> ifAbsent + PrefRead.Unreadable -> ifUnreadable + } + + private fun boolAt(key: String, ifAbsent: Boolean, ifUnreadable: Boolean): Boolean = + when (val read = prefs.readBoolean(key)) { + is PrefRead.Found -> read.value + PrefRead.Absent -> ifAbsent + PrefRead.Unreadable -> ifUnreadable + } + @Synchronized fun isWeakPin(pin: String): Boolean { if (!pin.all { it.isDigit() }) return true @@ -122,8 +201,18 @@ class PinStore(private val context: Context) { val invalidLength = pin.length > MAX_PIN_LENGTH val lockedOut = isLockedOut() - val storedHash = prefs.getString(KEY_PIN_HASH, null) - val salt = prefs.getString(KEY_PIN_SALT, null) + // A record that will not decrypt cannot verify anything, and must not spend the + // attempt budget on a fault that is not a wrong PIN: burning attempts here would + // drive the lockout for a record no PIN can ever satisfy. The constant-time dance + // below is kept for the readable path, where a timing difference would separate + // "no PIN set" from "wrong PIN"; this state is already named on screen, so there + // is nothing for it to leak. + val hashRead = prefs.readString(KEY_PIN_HASH) + val saltRead = prefs.readString(KEY_PIN_SALT) + if (hashRead == PrefRead.Unreadable || saltRead == PrefRead.Unreadable) return false + + val storedHash = (hashRead as? PrefRead.Found)?.value + val salt = (saltRead as? PrefRead.Found)?.value val pinNotSet = storedHash == null || salt == null val effectiveSalt = salt ?: DUMMY_SALT @@ -202,8 +291,10 @@ class PinStore(private val context: Context) { @Synchronized fun isSessionValid(): Boolean { if (!isPinEnabled()) return true - val sessionStartedAtRealtime = prefs.getLong(KEY_SESSION_STARTED_AT_REALTIME, 0) - val sessionStartedAtWall = prefs.getLong(KEY_SESSION_STARTED_AT_WALL, 0) + // An unreadable timestamp answers 0, which is the no-session value, so an + // unreadable session is not a valid one. + val sessionStartedAtRealtime = longAt(KEY_SESSION_STARTED_AT_REALTIME, ifAbsent = 0, ifUnreadable = 0) + val sessionStartedAtWall = longAt(KEY_SESSION_STARTED_AT_WALL, ifAbsent = 0, ifUnreadable = 0) if (sessionStartedAtRealtime == 0L || sessionStartedAtWall == 0L) return false val timeout = getSessionTimeout() if (timeout == 0L) return false @@ -224,7 +315,11 @@ class PinStore(private val context: Context) { } @Synchronized - fun getSessionTimeout(): Long = prefs.getLong(KEY_SESSION_TIMEOUT, DEFAULT_SESSION_TIMEOUT_MS) + // 0 is a configured option here, meaning authenticate every time, so it is the + // strictest answer available rather than an invented one. An unreadable timeout + // must not grant the longest session anyone had set. + fun getSessionTimeout(): Long = + longAt(KEY_SESSION_TIMEOUT, ifAbsent = DEFAULT_SESSION_TIMEOUT_MS, ifUnreadable = 0) @Synchronized fun setSessionTimeout(timeoutMs: Long): Boolean { @@ -253,18 +348,43 @@ class PinStore(private val context: Context) { @Synchronized fun getLockoutRemainingMs(): Long { - val lockoutDuration = prefs.getLong(KEY_LOCKOUT_DURATION, 0) + // Checked for the whole record before any arithmetic, because these values live + // in one file under one key, so a fault takes them together and the sums below + // would be computed from guesses. Reports the longest configured lockout rather + // than none: the throttle is what makes a short PIN worth having, and this is + // the one case where someone able to write this file could otherwise clear it. + // Waiting is not the way out; an unreadable record puts the unlock screen in + // its own state, which offers the reset. + if (pinRecord() == PinRecord.UNREADABLE) return LOCKOUT_DURATIONS_MS.last() + + val lockoutDuration = longAt( + KEY_LOCKOUT_DURATION, + ifAbsent = 0, + ifUnreadable = LOCKOUT_DURATIONS_MS.last() + ) if (lockoutDuration == 0L) { maybeDecayLockoutLevel() return 0 } - val savedSetElapsed = prefs.getLong(KEY_LOCKOUT_SET_AT_ELAPSED, 0) + // Treated as set now, so a duration that cannot be placed in time still runs + // its full length instead of reading as long expired. + val savedSetElapsed = longAt( + KEY_LOCKOUT_SET_AT_ELAPSED, + ifAbsent = 0, + ifUnreadable = SystemClock.elapsedRealtime() + ) val currentElapsed = SystemClock.elapsedRealtime() val rebootDetected = currentElapsed < savedSetElapsed if (rebootDetected) { - val lockoutWallClock = prefs.getLong(KEY_LOCKOUT_WALL_CLOCK, 0) + // 0 here clears the lockout below, so an unreadable wall clock reports now + // instead, which leaves the full duration to run. + val lockoutWallClock = longAt( + KEY_LOCKOUT_WALL_CLOCK, + ifAbsent = 0, + ifUnreadable = System.currentTimeMillis() + ) if (lockoutWallClock == 0L) { clearLockoutTimestampsInternal() return 0 @@ -293,7 +413,9 @@ class PinStore(private val context: Context) { } @Synchronized - fun requiresBiometricReset(): Boolean = prefs.getBoolean(KEY_BIOMETRIC_RESET_REQUIRED, false) + // A reset that is owed and cannot be read is still owed. + fun requiresBiometricReset(): Boolean = + boolAt(KEY_BIOMETRIC_RESET_REQUIRED, ifAbsent = false, ifUnreadable = true) @Synchronized fun clearBiometricResetRequirement(): Boolean { @@ -313,10 +435,13 @@ class PinStore(private val context: Context) { } private fun maybeDecayLockoutLevel() { - val currentLevel = prefs.getInt(KEY_LOCKOUT_LEVEL, 0) + // Decay only ever lowers the escalation level, so an unreadable level or + // timestamp must not drive it: both answer "nothing to decay" and leave the + // escalation where it is. + val currentLevel = intAt(KEY_LOCKOUT_LEVEL, ifAbsent = 0, ifUnreadable = 0) if (currentLevel == 0) return - val lastCleared = prefs.getLong(KEY_LAST_LOCKOUT_CLEARED, 0) + val lastCleared = longAt(KEY_LAST_LOCKOUT_CLEARED, ifAbsent = 0, ifUnreadable = 0) if (lastCleared == 0L) return val now = System.currentTimeMillis() @@ -346,14 +471,19 @@ class PinStore(private val context: Context) { .putLong(KEY_LOCKOUT_WALL_CLOCK, 0) .putLong(KEY_LOCKOUT_DURATION, 0) .putLong(KEY_LAST_LOCKOUT_CLEARED, System.currentTimeMillis()) - if (!prefs.getBoolean(KEY_BIOMETRIC_RESET_REQUIRED, false)) { + // Through the fail-closed read: an unreadable requirement is still owed, so the + // attempt counter is kept rather than cleared. + if (!requiresBiometricReset()) { editor.putInt(KEY_FAILED_ATTEMPTS, 0) } editor.commit() } @Synchronized - fun getFailedAttempts(): Int = prefs.getInt(KEY_FAILED_ATTEMPTS, 0) + // An unreadable counter reports the budget spent, so getRemainingAttempts answers 0 + // rather than a full allowance. + fun getFailedAttempts(): Int = + intAt(KEY_FAILED_ATTEMPTS, ifAbsent = 0, ifUnreadable = MAX_FAILED_ATTEMPTS) @Synchronized fun getRemainingAttempts(): Int = MAX_FAILED_ATTEMPTS - getFailedAttempts() @@ -363,7 +493,13 @@ class PinStore(private val context: Context) { val editor = prefs.edit().putInt(KEY_FAILED_ATTEMPTS, attempts) if (attempts >= MAX_FAILED_ATTEMPTS) { - val currentLevel = prefs.getInt(KEY_LOCKOUT_LEVEL, 0) + // An unreadable level takes the top of the ladder, not the bottom: reading + // it as 0 would hand out the shortest lockout and restart the escalation. + val currentLevel = intAt( + KEY_LOCKOUT_LEVEL, + ifAbsent = 0, + ifUnreadable = LOCKOUT_DURATIONS_MS.size - 1 + ) val duration = LOCKOUT_DURATIONS_MS[currentLevel.coerceIn(0, LOCKOUT_DURATIONS_MS.size - 1)] val newLevel = (currentLevel + 1).coerceAtMost(LOCKOUT_DURATIONS_MS.size - 1) editor.putLong(KEY_LOCKOUT_SET_AT_ELAPSED, SystemClock.elapsedRealtime()) @@ -372,7 +508,13 @@ class PinStore(private val context: Context) { editor.putInt(KEY_FAILED_ATTEMPTS, 0) editor.putInt(KEY_LOCKOUT_LEVEL, newLevel) - val epoch = prefs.getInt(KEY_LOCKOUT_EPOCH, 0) + 1 + // An unreadable epoch counts as having reached the threshold, so the + // biometric reset below is required rather than postponed. + val epoch = intAt( + KEY_LOCKOUT_EPOCH, + ifAbsent = 0, + ifUnreadable = LOCKOUT_EPOCHS_BEFORE_BIOMETRIC + ) + 1 editor.putInt(KEY_LOCKOUT_EPOCH, epoch) if (epoch >= LOCKOUT_EPOCHS_BEFORE_BIOMETRIC) { editor.putBoolean(KEY_BIOMETRIC_RESET_REQUIRED, true) @@ -385,7 +527,10 @@ class PinStore(private val context: Context) { } private fun clearFailedAttemptsAndResetEpoch() { - val currentLevel = prefs.getInt(KEY_LOCKOUT_LEVEL, 0) + // 0 is the strict answer here, unlike the other level reads: it only decides + // whether to stamp the decay clock, and not stamping leaves the escalation + // where it is. Don't align this with the others. + val currentLevel = intAt(KEY_LOCKOUT_LEVEL, ifAbsent = 0, ifUnreadable = 0) val editor = prefs.edit() .putInt(KEY_FAILED_ATTEMPTS, 0) .putInt(KEY_LOCKOUT_EPOCH, 0) diff --git a/app/src/main/res/values/strings_pin.xml b/app/src/main/res/values/strings_pin.xml index ddf4c197..0256a1cf 100644 --- a/app/src/main/res/values/strings_pin.xml +++ b/app/src/main/res/values/strings_pin.xml @@ -22,6 +22,10 @@ Too many failed PIN attempts. Verify your identity with biometrics to continue. Locked out Try again in %1$d seconds + PIN cannot be read + The stored PIN is damaged or its encryption key is gone, so no PIN can unlock the app and signing stays blocked. Removing it clears the PIN and the lockout; your keys and accounts are untouched. You can set a new PIN afterwards. + Remove PIN + The PIN could not be removed. It is still stored on this device. Too many attempts. Try again later. Incorrect PIN. %1$d attempts remaining. From bb71e608cfa394f0e1a132df3e7df08e3eea7326 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 09:59:32 -0400 Subject: [PATCH 02/10] Re-anchor a lockout whose start cannot be read instead of restarting it on every read --- .../keep/storage/PinStoreUnreadableTest.kt | 38 ++++++++++++++++ .../io/privkey/keep/storage/PinStore.kt | 44 +++++++++++++------ 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt index 4360766b..d19454e9 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt @@ -157,6 +157,40 @@ class PinStoreUnreadableTest { assertTrue("and it still verifies", after.verifyPin(GOOD_PIN)) } + /** + * One damaged lockout timestamp while the rest of the record reads fine. + * + * The flag reads as SET here, so the unlock screen shows the lockout rather than the + * state that offers removal, which makes counting down the only way out. A number + * substituted for the timestamp on each read would re-anchor the lockout to the + * moment of every call and hold it at its full length forever. The lockout has to run + * its full length once and then end. + */ + @Test + fun an_unreadable_lockout_anchor_runs_once_and_then_counts_down() { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit() + .putBoolean("pin_enabled", true) + .putLong("lockout_duration", LONGEST_LOCKOUT_MS) + .commit() + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putLong("lockout_set_at_elapsed", 1L).commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + + assertEquals(PinRecord.SET, PinStore(context).pinRecord()) + val first = PinStore(context).getLockoutRemainingMs() + assertTrue("a lost anchor must not read as an expired lockout", first > 0) + + Thread.sleep(COUNTDOWN_PROBE_MS) + val second = PinStore(context).getLockoutRemainingMs() + assertTrue( + "the lockout has to count down rather than restart on every read: $first then $second", + first - second >= COUNTDOWN_PROBE_MS - COUNTDOWN_SLACK_MS + ) + forgetWriteProbe(context, PinStore.PREFS_NAME) + } + @Test fun the_clear_refuses_when_no_pin_is_set() { assertFalse(PinStore(context).clearPin()) @@ -167,5 +201,9 @@ class PinStoreUnreadableTest { const val GOOD_PIN = "4917" const val OTHER_PIN = "5283" const val WRONG_PIN = "7194" + // The top of PinStore's lockout ladder. + const val LONGEST_LOCKOUT_MS = 3_600_000L + const val COUNTDOWN_PROBE_MS = 200L + const val COUNTDOWN_SLACK_MS = 50L } } diff --git a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt index b51b9e02..1859bcf1 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt @@ -367,24 +367,24 @@ class PinStore(private val context: Context) { return 0 } - // Treated as set now, so a duration that cannot be placed in time still runs - // its full length instead of reading as long expired. - val savedSetElapsed = longAt( - KEY_LOCKOUT_SET_AT_ELAPSED, - ifAbsent = 0, - ifUnreadable = SystemClock.elapsedRealtime() - ) + // An anchor that will not decrypt is its own case rather than a number. Either + // number is wrong: 0 makes the subtraction below expire the lockout at once, and + // "now" substituted on each read re-anchors it to the moment of every call, which + // holds it at full length forever. With the PIN flag reading fine the screen + // shows the lockout and not the removal, so counting down is the only way out. + // Writing a fresh anchor makes the lockout run its full length once and then + // end, and the write heals the damaged value, since it does not read first. + val setAtRead = prefs.readLong(KEY_LOCKOUT_SET_AT_ELAPSED) + val wallRead = prefs.readLong(KEY_LOCKOUT_WALL_CLOCK) + if (setAtRead == PrefRead.Unreadable || wallRead == PrefRead.Unreadable) { + return reanchorLockout(lockoutDuration) + } + val savedSetElapsed = (setAtRead as? PrefRead.Found)?.value ?: 0L val currentElapsed = SystemClock.elapsedRealtime() val rebootDetected = currentElapsed < savedSetElapsed if (rebootDetected) { - // 0 here clears the lockout below, so an unreadable wall clock reports now - // instead, which leaves the full duration to run. - val lockoutWallClock = longAt( - KEY_LOCKOUT_WALL_CLOCK, - ifAbsent = 0, - ifUnreadable = System.currentTimeMillis() - ) + val lockoutWallClock = (wallRead as? PrefRead.Found)?.value ?: 0L if (lockoutWallClock == 0L) { clearLockoutTimestampsInternal() return 0 @@ -413,6 +413,22 @@ class PinStore(private val context: Context) { } @Synchronized + /** + * Starts [duration] over from now, for a lockout whose anchor could not be read. + * + * If the write fails the next read finds the anchor still unreadable and comes back + * here, so the lockout holds rather than lifting. That is the strict direction, at + * the cost of a lockout that cannot end while storage refuses writes. + */ + private fun reanchorLockout(duration: Long): Long { + prefs.edit() + .putLong(KEY_LOCKOUT_SET_AT_ELAPSED, SystemClock.elapsedRealtime()) + .putLong(KEY_LOCKOUT_WALL_CLOCK, System.currentTimeMillis()) + .putLong(KEY_LOCKOUT_DURATION, duration) + .commit() + return duration + } + // A reset that is owed and cannot be read is still owed. fun requiresBiometricReset(): Boolean = boolAt(KEY_BIOMETRIC_RESET_REQUIRED, ifAbsent = false, ifUnreadable = true) From 8a3725ee6ab8c5215e59117b53754b4e5509d5e9 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 10:13:56 -0400 Subject: [PATCH 03/10] Decide an unreadable PIN record by its hash too and gate removal on the strongest factor available --- .../keep/PinUnlockScreenUnreadableTest.kt | 90 +++++++++++++-- .../keep/storage/PinStoreUnreadableTest.kt | 70 ++++++++++++ .../kotlin/io/privkey/keep/BiometricHelper.kt | 31 +++++ .../kotlin/io/privkey/keep/MainActivity.kt | 37 ++++++ .../kotlin/io/privkey/keep/PinUnlockScreen.kt | 61 ++++++++-- .../io/privkey/keep/storage/PinStore.kt | 108 ++++++++++-------- .../io/privkey/keep/storage/PrefRead.kt | 13 +++ app/src/main/res/values/strings_pin.xml | 3 +- 8 files changed, 340 insertions(+), 73 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt index a45b0018..d8ea5fb8 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt @@ -1,5 +1,6 @@ package io.privkey.keep +import androidx.compose.runtime.remember import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onAllNodesWithText @@ -61,16 +62,26 @@ class PinUnlockScreenUnreadableTest { context.deleteSharedPreferences(PinStore.PREFS_NAME) } + /** + * [authorizePinRemoval] stands in for whichever factor production asks for. It answers + * true by default so the cases below assert what the screen does once removal is + * authorized; the case that refuses it passes false. + */ private fun showScreen( onBiometricAuth: (suspend () -> Boolean)? = null, + authorizePinRemoval: suspend () -> Boolean = { true }, onUnlocked: () -> Unit = {} ) { compose.setContent { + // Remembered, not rebuilt per recomposition: production hands this screen one + // store for the life of the process. + val store = remember { PinStore(context) } KeepAndroidTheme { PinUnlockScreen( - pinStore = PinStore(context), + pinStore = store, onUnlocked = onUnlocked, - onBiometricAuth = onBiometricAuth + onBiometricAuth = onBiometricAuth, + authorizePinRemoval = authorizePinRemoval ) } } @@ -125,37 +136,92 @@ class PinUnlockScreenUnreadableTest { } /** - * With biometrics enrolled the removal waits on them, because it takes away an - * authentication factor and the person asking has not authenticated. A refused - * prompt must leave the record alone. + * Removal takes away an authentication factor before anyone has authenticated, so it + * waits on whatever the caller gates it with. A refusal must leave the record alone. */ @Test - fun aRefusedBiometricLeavesTheRecordAlone() { + fun aRefusedAuthorizationLeavesTheRecordAlone() { PinStore(context).setPin(GOOD_PIN) breakNameResolution(context, PinStore.PREFS_NAME) var unlocked = false - showScreen(onBiometricAuth = { false }, onUnlocked = { unlocked = true }) + showScreen(authorizePinRemoval = { false }, onUnlocked = { unlocked = true }) awaitText(unreadableTitle) compose.onNodeWithText(removeLabel).performClick() compose.waitForIdle() - assertFalse("a refused prompt must not remove the PIN", unlocked) + assertFalse("a refused authorization must not remove the PIN", unlocked) assertEquals(PinRecord.UNREADABLE, PinStore(context).pinRecord()) } + /** + * With a biometric available the screen offers a way in that destroys nothing. The + * record has to survive it, since a transient fault may clear, and removing a PIN is + * the step that cannot be taken back. + */ @Test - fun anAcceptedBiometricRemovesTheRecord() { + fun aBiometricUnlocksWithoutRemovingTheRecord() { PinStore(context).setPin(GOOD_PIN) breakNameResolution(context, PinStore.PREFS_NAME) var unlocked = false - showScreen(onBiometricAuth = { true }, onUnlocked = { unlocked = true }) + var removalAsked = false + showScreen( + onBiometricAuth = { true }, + authorizePinRemoval = { removalAsked = true; true }, + onUnlocked = { unlocked = true } + ) awaitText(unreadableTitle) - compose.onNodeWithText(removeLabel).performClick() + compose.onNodeWithText(context.getString(R.string.pin_unlock_use_biometrics)).performClick() compose.waitUntil(timeoutMillis = 5_000) { unlocked } - assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) + assertFalse("the biometric path must not go through removal", removalAsked) + assertEquals( + "and the record must be left as it was", + PinRecord.UNREADABLE, + PinStore(context).pinRecord() + ) + } + + // Without a biometric there is no non-destructive way in, so it is not offered. + @Test + fun noBiometricMeansNoBiometricButton() { + PinStore(context).setPin(GOOD_PIN) + breakNameResolution(context, PinStore.PREFS_NAME) + showScreen(onBiometricAuth = null) + + awaitText(unreadableTitle) + compose.onNodeWithText(context.getString(R.string.pin_unlock_use_biometrics)) + .assertDoesNotExist() + compose.onNodeWithText(removeLabel).assertIsDisplayed() + } + + /** + * The record recovers between the screen being drawn and the tap. The clear refuses a + * record it can read, and before this the screen stayed in this state with a button + * that would refuse forever. It has to go back to asking for the PIN. + */ + @Test + fun aRecordThatRecoversBeforeTheTapReturnsToPinEntry() { + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putBoolean("pin_enabled", true).commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + var unlocked = false + showScreen(onUnlocked = { unlocked = true }) + awaitText(unreadableTitle) + + // Repaired underneath the screen: setPin overwrites the damaged flag and writes a + // usable hash, so the record now reads SET. + assertTrue(PinStore(context).setPin(GOOD_PIN)) + compose.onNodeWithText(removeLabel).performClick() + + awaitText(context.getString(R.string.pin_unlock_button)) + compose.onNodeWithText(unreadableTitle).assertDoesNotExist() + assertFalse("a readable PIN has to be entered, not skipped", unlocked) + assertEquals("and it was not removed", PinRecord.SET, PinStore(context).pinRecord()) + forgetWriteProbe(context, PinStore.PREFS_NAME) } /** diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt index d19454e9..e251cbac 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt @@ -191,6 +191,76 @@ class PinStoreUnreadableTest { forgetWriteProbe(context, PinStore.PREFS_NAME) } + /** + * A hash that will not decrypt beside a flag that reads fine. The flag alone decided + * the record before, so this read as SET: every PIN was refused as incorrect, no + * attempt was spent, and the removal was never offered, which on a device with no + * biometric is a lockout with no way out. + */ + @Test + fun an_unreadable_hash_beside_a_readable_flag_is_unreadable() { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit() + .putBoolean("pin_enabled", true) + .putString("pin_salt", "AAAAAAAAAAAAAAAAAAAAAA==") + .commit() + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putString("pin_hash", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=").commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + + val store = PinStore(context) + assertEquals(PinRecord.UNREADABLE, store.pinRecord()) + assertTrue("and the way out is offered for it", store.clearPin()) + assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) + } + + /** + * The reverse: a broken flag beside a hash and salt that still read. disablePin removes + * the hash and salt, so a usable pair means a PIN was set and never disabled. Reading + * that as UNREADABLE would offer to delete a PIN that can still be verified. + */ + @Test + fun a_broken_flag_beside_a_usable_verifier_is_still_set() { + assertTrue(PinStore(context).setPin(GOOD_PIN)) + // The stored name is an HMAC of the key under the file's own random key, so it is + // found by removing the flag and seeing which entry goes, then writing it back, + // which lands on the same name. + val before = rawPrefs(context, PinStore.PREFS_NAME).all.keys.toSet() + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit().remove("pin_enabled").commit() + val flagName = (before - rawPrefs(context, PinStore.PREFS_NAME).all.keys.toSet()).single() + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit().putBoolean("pin_enabled", true).commit() + corruptStoredValue(context, PinStore.PREFS_NAME, flagName) + + val store = PinStore(context) + assertEquals(PinRecord.SET, store.pinRecord()) + assertTrue("the PIN still verifies", store.verifyPin(GOOD_PIN)) + assertFalse("so it must not be removable this way", store.clearPin()) + } + + /** + * Nothing in the app writes the session timeout, so a value here that will not read + * can never be rewritten. Answering 0 for it made every session invalid for good, + * which refuses all background signing while a PIN is set. A fresh session has to be + * valid again. + */ + @Test + fun an_unreadable_session_timeout_still_lets_a_session_open() { + val store = PinStore(context) + assertTrue(store.setPin(GOOD_PIN)) + val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) + .edit().putLong("session_timeout", 300_000L).commit() + } + corruptStoredValue(context, PinStore.PREFS_NAME, storedName) + + val reopened = PinStore(context) + reopened.refreshSession() + assertTrue("a session opened now has to count as open", reopened.isSessionValid()) + assertFalse(reopened.requiresAuthentication()) + forgetWriteProbe(context, PinStore.PREFS_NAME) + } + @Test fun the_clear_refuses_when_no_pin_is_set() { assertFalse(PinStore(context).clearPin()) diff --git a/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt b/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt index ad6c2973..8443eae7 100644 --- a/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt +++ b/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt @@ -115,6 +115,37 @@ class BiometricHelper private constructor( } } + /** + * Prompts for the device's own lock-screen credential, for an action that has to be + * authorized when no biometric can be used. + * + * A prompt that allows DEVICE_CREDENTIAL must not set a negative button, which is why + * this does not go through [resolvePromptInfo]: the builder rejects the combination. + * Never short-circuited by the biometric timeout window, since the callers are + * removing an authentication factor and need a credential entered now. + */ + suspend fun authenticateWithDeviceCredential(title: String, subtitle: String): Boolean { + val promptInfo = BiometricPrompt.PromptInfo.Builder() + .setTitle(title) + .setSubtitle(subtitle) + .setAllowedAuthenticators(BiometricManager.Authenticators.DEVICE_CREDENTIAL) + .build() + return suspendCoroutine { continuation -> + val callback = object : BiometricPrompt.AuthenticationCallback() { + override fun onAuthenticationSucceeded(result: BiometricPrompt.AuthenticationResult) { + continuation.resume(true) + } + + override fun onAuthenticationError(errorCode: Int, errString: CharSequence) { + continuation.resume(false) + } + + override fun onAuthenticationFailed() {} + } + authenticator.authenticate(promptInfo, null, callback) + } + } + suspend fun authenticateWithCrypto( cipher: Cipher, title: String? = null, diff --git a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt index 2300b41e..144b79bb 100644 --- a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt +++ b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt @@ -1,5 +1,6 @@ package io.privkey.keep +import android.app.KeyguardManager import android.content.Context import android.os.Bundle import android.util.Log @@ -154,10 +155,46 @@ class MainActivity : FragmentActivity() { val unlockTitle = stringResource(R.string.main_unlock_title) val unlockSubtitle = stringResource(R.string.main_unlock_subtitle) + val removeTitle = stringResource(R.string.pin_unlock_unreadable_remove) + val removeSubtitle = stringResource(R.string.pin_unlock_unreadable_auth_subtitle) if (pinStoreForUnlock != null) { PinUnlockScreen( pinStore = pinStoreForUnlock, onUnlocked = { isPinUnlocked = true }, + // Removing an unreadable PIN takes away an authentication factor + // before anyone has authenticated, so it asks for the strongest + // factor the device can offer right now. A biometric first. If + // none is usable, which includes a sensor that is only + // temporarily unavailable, the device's own lock-screen + // credential. Only a device with no secure lock at all runs it + // unasked, because then there is nothing to ask for and refusing + // would leave no way into the app. The secure-lock check is made + // here against the platform rather than through the helper, so a + // missing helper on a secure device refuses instead of falling + // through to the unasked branch. + authorizePinRemoval = { + val deviceSecure = + getSystemService(KeyguardManager::class.java)?.isDeviceSecure == true + when { + biometricAvailable -> { + val ok = biometricHelper?.authenticate( + title = removeTitle, + subtitle = removeSubtitle, + forcePrompt = true + ) ?: false + // A biometric here is as good as the one the lock + // screen below would ask for, so it is not asked twice. + if (ok) isBiometricUnlocked = true + ok + } + deviceSecure -> + biometricHelper?.authenticateWithDeviceCredential( + removeTitle, + removeSubtitle + ) ?: false + else -> true + } + }, onBiometricAuth = if (biometricAvailable) { { biometricHelper?.authenticate( diff --git a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt index de70c1d0..ae7c3d54 100644 --- a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt +++ b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt @@ -26,7 +26,14 @@ fun PinUnlockScreen( pinStore: PinStore, onUnlocked: () -> Unit, onBiometricAuth: (suspend () -> Boolean)? = null, - onBiometricSuccess: () -> Unit = {} + onBiometricSuccess: () -> Unit = {}, + /** + * Authorizes removing a PIN record that cannot be read. Supplied by the caller rather + * than decided here, because which factor to ask for depends on what the device can + * offer at that moment, which this screen cannot see. Required, with no default, so + * no caller can get the removal without having decided how it is gated. + */ + authorizePinRemoval: suspend () -> Boolean ) { var pinInput by remember { mutableStateOf("") } var error by remember { mutableStateOf(null) } @@ -147,22 +154,52 @@ fun PinUnlockScreen( ) } Spacer(modifier = Modifier.height(24.dp)) - Button( + // A biometric already unlocks this screen in its PIN-entry state, so it is + // offered here first as the way in that destroys nothing. The record stays + // as it is, and if the fault was transient it can read again later. It + // does not refresh the session, which writes to the file that just failed + // to read; signing stays blocked until the record is repaired, as the body + // text says. + if (onBiometricAuth != null) { + Button( + onClick = { + coroutineScope.launch { + if (onBiometricAuth()) { + onBiometricSuccess() + onUnlocked() + } + } + }, + modifier = Modifier.fillMaxWidth() + ) { + Text(stringResource(R.string.pin_unlock_use_biometrics)) + } + Spacer(modifier = Modifier.height(8.dp)) + } + OutlinedButton( onClick = { coroutineScope.launch { - // Behind biometrics when the device has them, since this - // removes an authentication factor and the person asking - // has not authenticated. With no factor enrolled there is - // nothing to ask for, and refusing would leave the app with - // no way in at all, which is the same convention - // requireAuthThen follows elsewhere. - val authed = onBiometricAuth?.invoke() ?: true - if (!authed) return@launch - removeFailed = !pinStore.clearPin() - if (!removeFailed) { + removeFailed = false + if (!authorizePinRemoval()) return@launch + if (pinStore.clearPin()) { pinRecord = pinStore.pinRecord() pinStore.refreshSession() onUnlocked() + return@launch + } + // The clear refuses a record it can read, so a refusal can mean + // the record recovered between the screen being drawn and the + // tap. Read everything again rather than staying in this state, + // whose only button would then refuse forever. + pinRecord = pinStore.pinRecord() + when (pinRecord) { + PinRecord.UNREADABLE -> removeFailed = true + PinRecord.NOT_SET -> onUnlocked() + PinRecord.SET -> { + isLockedOut = pinStore.isLockedOut() + lockoutRemaining = pinStore.getLockoutRemainingMs() + biometricResetRequired = pinStore.requiresBiometricReset() + } } } }, diff --git a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt index 1859bcf1..dd062122 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt @@ -96,10 +96,33 @@ class PinStore(private val context: Context) { * [clearPin] exists and why the unlock screen offers it. */ @Synchronized - fun pinRecord(): PinRecord = when (val read = prefs.readBoolean(KEY_PIN_ENABLED)) { - is PrefRead.Found -> if (read.value) PinRecord.SET else PinRecord.NOT_SET - PrefRead.Absent -> PinRecord.NOT_SET - PrefRead.Unreadable -> PinRecord.UNREADABLE + fun pinRecord(): PinRecord = classify(prefs) + + /** + * Decided by the flag and the hash together, because a record is only usable when + * the flag says a PIN is set and the hash and salt can be read to verify one. + * + * The flag alone left two cases wrong. A readable flag beside an unreadable hash read + * as SET, so every PIN was refused as incorrect, no attempt was spent, and the + * removal was never offered: a lockout with no way out. A broken flag beside a usable + * hash read as UNREADABLE and offered removal of a PIN that could still verify. + * disablePin removes the hash and salt, so a usable hash means a PIN was set and + * never disabled, which is what makes SET the right answer for the second case. + */ + private fun classify(record: ReadablePrefs): PinRecord { + val flag = record.readBoolean(KEY_PIN_ENABLED) + val hash = record.readString(KEY_PIN_HASH) + val salt = record.readString(KEY_PIN_SALT) + val verifierUnreadable = hash == PrefRead.Unreadable || salt == PrefRead.Unreadable + val verifierUsable = hash is PrefRead.Found && salt is PrefRead.Found + return when (flag) { + is PrefRead.Found -> + if (!flag.value) PinRecord.NOT_SET + else if (verifierUnreadable) PinRecord.UNREADABLE + else PinRecord.SET + PrefRead.Absent -> PinRecord.NOT_SET + PrefRead.Unreadable -> if (verifierUsable) PinRecord.SET else PinRecord.UNREADABLE + } } /** True when a PIN may be set, which includes a record that cannot be read. */ @@ -118,40 +141,27 @@ class PinStore(private val context: Context) { * file's own HMAC key: resolving a name to remove it decrypts that key, so a * targeted remove raises instead of answering, while a clear resolves no names and * takes the damaged key material with it. + * + * The record is read a second time through a fresh instance before anything is + * deleted, which repeats the Keystore round trip rather than trusting this + * instance's earlier answer. [PrefRead.Unreadable] says a failed decrypt is + * permanent, but [KeystoreEncryptedPrefs] notes one can be transient, and StrongBox + * keys here have been seen to generate cleanly and then fail on use. A fault that + * clears between the screen being drawn and the tap must not cost a PIN that was + * fine. If the second read cannot even be attempted, nothing is deleted. */ @Synchronized fun clearPin(): Boolean { - if (pinRecord() != PinRecord.UNREADABLE) return false + if (classify(prefs) != PinRecord.UNREADABLE) return false + val fresh = runCatching { KeystoreEncryptedPrefs.create(context, PREFS_NAME) }.getOrNull() + ?: return false + if (classify(fresh) != PinRecord.UNREADABLE) return false return runCatching { prefs.edit().clear().commit() }.getOrDefault(false) } @Synchronized fun requiresAuthentication(): Boolean = isPinEnabled() && !isSessionValid() - // Both answers are named at every call site below. The bug these replace was that - // the getters gave one answer for both, and for roughly half of this file's keys - // the absent answer is the permissive one. - private fun longAt(key: String, ifAbsent: Long, ifUnreadable: Long): Long = - when (val read = prefs.readLong(key)) { - is PrefRead.Found -> read.value - PrefRead.Absent -> ifAbsent - PrefRead.Unreadable -> ifUnreadable - } - - private fun intAt(key: String, ifAbsent: Int, ifUnreadable: Int): Int = - when (val read = prefs.readInt(key)) { - is PrefRead.Found -> read.value - PrefRead.Absent -> ifAbsent - PrefRead.Unreadable -> ifUnreadable - } - - private fun boolAt(key: String, ifAbsent: Boolean, ifUnreadable: Boolean): Boolean = - when (val read = prefs.readBoolean(key)) { - is PrefRead.Found -> read.value - PrefRead.Absent -> ifAbsent - PrefRead.Unreadable -> ifUnreadable - } - @Synchronized fun isWeakPin(pin: String): Boolean { if (!pin.all { it.isDigit() }) return true @@ -293,8 +303,8 @@ class PinStore(private val context: Context) { if (!isPinEnabled()) return true // An unreadable timestamp answers 0, which is the no-session value, so an // unreadable session is not a valid one. - val sessionStartedAtRealtime = longAt(KEY_SESSION_STARTED_AT_REALTIME, ifAbsent = 0, ifUnreadable = 0) - val sessionStartedAtWall = longAt(KEY_SESSION_STARTED_AT_WALL, ifAbsent = 0, ifUnreadable = 0) + val sessionStartedAtRealtime = prefs.readLong(KEY_SESSION_STARTED_AT_REALTIME).resolve(ifAbsent = 0, ifUnreadable = 0) + val sessionStartedAtWall = prefs.readLong(KEY_SESSION_STARTED_AT_WALL).resolve(ifAbsent = 0, ifUnreadable = 0) if (sessionStartedAtRealtime == 0L || sessionStartedAtWall == 0L) return false val timeout = getSessionTimeout() if (timeout == 0L) return false @@ -314,12 +324,17 @@ class PinStore(private val context: Context) { return elapsedRealtime in 0..timeout && elapsedWall in 0..timeout } + // The default, not 0. Nothing in the app calls setSessionTimeout, and neither setPin + // nor disablePin writes this key, so an unreadable value here could never be + // rewritten: 0 would make every session invalid for good and refuse all background + // signing while a PIN is set, with no way back. The default is what every install + // runs with, so it lifts nothing below that. A whole-file fault does not rely on + // this: the session timestamps then read as 0, which already invalidates the session. @Synchronized - // 0 is a configured option here, meaning authenticate every time, so it is the - // strictest answer available rather than an invented one. An unreadable timeout - // must not grant the longest session anyone had set. - fun getSessionTimeout(): Long = - longAt(KEY_SESSION_TIMEOUT, ifAbsent = DEFAULT_SESSION_TIMEOUT_MS, ifUnreadable = 0) + fun getSessionTimeout(): Long = prefs.readLong(KEY_SESSION_TIMEOUT).resolve( + ifAbsent = DEFAULT_SESSION_TIMEOUT_MS, + ifUnreadable = DEFAULT_SESSION_TIMEOUT_MS + ) @Synchronized fun setSessionTimeout(timeoutMs: Long): Boolean { @@ -357,8 +372,7 @@ class PinStore(private val context: Context) { // its own state, which offers the reset. if (pinRecord() == PinRecord.UNREADABLE) return LOCKOUT_DURATIONS_MS.last() - val lockoutDuration = longAt( - KEY_LOCKOUT_DURATION, + val lockoutDuration = prefs.readLong(KEY_LOCKOUT_DURATION).resolve( ifAbsent = 0, ifUnreadable = LOCKOUT_DURATIONS_MS.last() ) @@ -412,7 +426,6 @@ class PinStore(private val context: Context) { return remainingByElapsed } - @Synchronized /** * Starts [duration] over from now, for a lockout whose anchor could not be read. * @@ -430,8 +443,9 @@ class PinStore(private val context: Context) { } // A reset that is owed and cannot be read is still owed. + @Synchronized fun requiresBiometricReset(): Boolean = - boolAt(KEY_BIOMETRIC_RESET_REQUIRED, ifAbsent = false, ifUnreadable = true) + prefs.readBoolean(KEY_BIOMETRIC_RESET_REQUIRED).resolve(ifAbsent = false, ifUnreadable = true) @Synchronized fun clearBiometricResetRequirement(): Boolean { @@ -454,10 +468,10 @@ class PinStore(private val context: Context) { // Decay only ever lowers the escalation level, so an unreadable level or // timestamp must not drive it: both answer "nothing to decay" and leave the // escalation where it is. - val currentLevel = intAt(KEY_LOCKOUT_LEVEL, ifAbsent = 0, ifUnreadable = 0) + val currentLevel = prefs.readInt(KEY_LOCKOUT_LEVEL).resolve(ifAbsent = 0, ifUnreadable = 0) if (currentLevel == 0) return - val lastCleared = longAt(KEY_LAST_LOCKOUT_CLEARED, ifAbsent = 0, ifUnreadable = 0) + val lastCleared = prefs.readLong(KEY_LAST_LOCKOUT_CLEARED).resolve(ifAbsent = 0, ifUnreadable = 0) if (lastCleared == 0L) return val now = System.currentTimeMillis() @@ -499,7 +513,7 @@ class PinStore(private val context: Context) { // An unreadable counter reports the budget spent, so getRemainingAttempts answers 0 // rather than a full allowance. fun getFailedAttempts(): Int = - intAt(KEY_FAILED_ATTEMPTS, ifAbsent = 0, ifUnreadable = MAX_FAILED_ATTEMPTS) + prefs.readInt(KEY_FAILED_ATTEMPTS).resolve(ifAbsent = 0, ifUnreadable = MAX_FAILED_ATTEMPTS) @Synchronized fun getRemainingAttempts(): Int = MAX_FAILED_ATTEMPTS - getFailedAttempts() @@ -511,8 +525,7 @@ class PinStore(private val context: Context) { if (attempts >= MAX_FAILED_ATTEMPTS) { // An unreadable level takes the top of the ladder, not the bottom: reading // it as 0 would hand out the shortest lockout and restart the escalation. - val currentLevel = intAt( - KEY_LOCKOUT_LEVEL, + val currentLevel = prefs.readInt(KEY_LOCKOUT_LEVEL).resolve( ifAbsent = 0, ifUnreadable = LOCKOUT_DURATIONS_MS.size - 1 ) @@ -526,8 +539,7 @@ class PinStore(private val context: Context) { // An unreadable epoch counts as having reached the threshold, so the // biometric reset below is required rather than postponed. - val epoch = intAt( - KEY_LOCKOUT_EPOCH, + val epoch = prefs.readInt(KEY_LOCKOUT_EPOCH).resolve( ifAbsent = 0, ifUnreadable = LOCKOUT_EPOCHS_BEFORE_BIOMETRIC ) + 1 @@ -546,7 +558,7 @@ class PinStore(private val context: Context) { // 0 is the strict answer here, unlike the other level reads: it only decides // whether to stamp the decay clock, and not stamping leaves the escalation // where it is. Don't align this with the others. - val currentLevel = intAt(KEY_LOCKOUT_LEVEL, ifAbsent = 0, ifUnreadable = 0) + val currentLevel = prefs.readInt(KEY_LOCKOUT_LEVEL).resolve(ifAbsent = 0, ifUnreadable = 0) val editor = prefs.edit() .putInt(KEY_FAILED_ATTEMPTS, 0) .putInt(KEY_LOCKOUT_EPOCH, 0) diff --git a/app/src/main/kotlin/io/privkey/keep/storage/PrefRead.kt b/app/src/main/kotlin/io/privkey/keep/storage/PrefRead.kt index cb4b4972..13ab6d0f 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/PrefRead.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/PrefRead.kt @@ -53,6 +53,19 @@ sealed interface PrefRead { object Unreadable : PrefRead } +/** + * The value, or the answer the caller names for each of the two ways there is none. + * + * Both are required on purpose. A caller whose value is a restriction usually wants + * different answers for the two, and the bug this type exists to prevent is a single + * default standing in for both. + */ +fun PrefRead.resolve(ifAbsent: T, ifUnreadable: T): T = when (this) { + is PrefRead.Found -> value + PrefRead.Absent -> ifAbsent + PrefRead.Unreadable -> ifUnreadable +} + /** * Preferences that will say why a read produced no value. * diff --git a/app/src/main/res/values/strings_pin.xml b/app/src/main/res/values/strings_pin.xml index 0256a1cf..bf5d2476 100644 --- a/app/src/main/res/values/strings_pin.xml +++ b/app/src/main/res/values/strings_pin.xml @@ -23,7 +23,8 @@ Locked out Try again in %1$d seconds PIN cannot be read - The stored PIN is damaged or its encryption key is gone, so no PIN can unlock the app and signing stays blocked. Removing it clears the PIN and the lockout; your keys and accounts are untouched. You can set a new PIN afterwards. + The app\'s PIN record is damaged or its encryption key is gone, so a PIN may be set that can no longer be checked, and signing stays blocked. Removing the record clears the PIN and the lockout; your keys and accounts are untouched. You can set a new PIN afterwards. + Confirm it\'s you to remove the stored PIN Remove PIN The PIN could not be removed. It is still stored on this device. Too many attempts. Try again later. From af102bb5a487aa5c84ae714604900e0fc9ac4b4b Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 10:29:06 -0400 Subject: [PATCH 04/10] Gate PIN removal on a biometric or the device credential and drop the unlock that could not hold --- .../keep/PinUnlockScreenUnreadableTest.kt | 46 +++++--------- .../nip55/RelayAuthWhitelistScreenTest.kt | 11 +++- .../keep/storage/EncryptedPrefsTestSupport.kt | 24 ++++++++ .../keep/storage/PinStoreUnreadableTest.kt | 39 +++++++----- .../kotlin/io/privkey/keep/BiometricHelper.kt | 24 +++++--- .../kotlin/io/privkey/keep/MainActivity.kt | 61 +++++++++---------- .../kotlin/io/privkey/keep/PinUnlockScreen.kt | 32 +++------- .../io/privkey/keep/storage/PinStore.kt | 53 +++++++++------- 8 files changed, 159 insertions(+), 131 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt index d8ea5fb8..590026fc 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt @@ -87,7 +87,10 @@ class PinUnlockScreenUnreadableTest { } } - private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = 5_000) { + // 15s rather than 5s, for the reason RelayAuthWhitelistScreenTest gives: the state + // appears after a Keystore decrypt fails off the main thread, which runs well past + // its local time on the CI emulator. waitUntil returns as soon as it holds. + private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } @@ -129,7 +132,7 @@ class PinUnlockScreenUnreadableTest { awaitText(unreadableTitle) compose.onNodeWithText(removeLabel).performClick() - compose.waitUntil(timeoutMillis = 5_000) { unlocked } + compose.waitUntil(timeoutMillis = AWAIT_MS) { unlocked } assertTrue("removal has to unlock, or the screen has no exit", unlocked) assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) assertFalse(PinStore(context).requiresAuthentication()) @@ -155,40 +158,18 @@ class PinUnlockScreenUnreadableTest { } /** - * With a biometric available the screen offers a way in that destroys nothing. The - * record has to survive it, since a transient fault may clear, and removing a PIN is - * the step that cannot be taken back. + * No biometric unlock is offered in this state, even with a biometric available. + * + * One that left the record alone could not hold: the activity re-derives the unlock + * from the session on every resume, the session cannot be refreshed against a file + * that will not decrypt, and dismissing the prompt resumes the activity, so the lock + * screen would come straight back. Pinned so it is not added back as a convenience. */ @Test - fun aBiometricUnlocksWithoutRemovingTheRecord() { + fun theUnreadableStateOffersNoUnlockThatCannotHold() { PinStore(context).setPin(GOOD_PIN) breakNameResolution(context, PinStore.PREFS_NAME) - var unlocked = false - var removalAsked = false - showScreen( - onBiometricAuth = { true }, - authorizePinRemoval = { removalAsked = true; true }, - onUnlocked = { unlocked = true } - ) - - awaitText(unreadableTitle) - compose.onNodeWithText(context.getString(R.string.pin_unlock_use_biometrics)).performClick() - - compose.waitUntil(timeoutMillis = 5_000) { unlocked } - assertFalse("the biometric path must not go through removal", removalAsked) - assertEquals( - "and the record must be left as it was", - PinRecord.UNREADABLE, - PinStore(context).pinRecord() - ) - } - - // Without a biometric there is no non-destructive way in, so it is not offered. - @Test - fun noBiometricMeansNoBiometricButton() { - PinStore(context).setPin(GOOD_PIN) - breakNameResolution(context, PinStore.PREFS_NAME) - showScreen(onBiometricAuth = null) + showScreen(onBiometricAuth = { true }) awaitText(unreadableTitle) compose.onNodeWithText(context.getString(R.string.pin_unlock_use_biometrics)) @@ -244,5 +225,6 @@ class PinUnlockScreenUnreadableTest { private companion object { const val GOOD_PIN = "4917" + const val AWAIT_MS = 15_000L } } diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index bab9161e..46295436 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -125,7 +125,12 @@ class RelayAuthWhitelistScreenTest { } } - private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = 5_000) { + // 15s, not 5s. The unreadable state appears only after a Keystore decrypt fails on an + // IO dispatcher, which takes about 1s here and about 2s on the CI emulator, and on CI + // it has also run past 5s: the same wait then passed in one test and timed out in two + // others of the same run. waitUntil returns as soon as the text appears, so the larger + // bound costs nothing on a pass, and a screen that never shows it still fails. + private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } @@ -205,4 +210,8 @@ class RelayAuthWhitelistScreenTest { forgetWriteProbe(context, RelayAuthWhitelistStore.PREFS_NAME) } + + private companion object { + const val AWAIT_MS = 15_000L + } } diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/EncryptedPrefsTestSupport.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/EncryptedPrefsTestSupport.kt index 7ef350fa..b5791b1b 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/EncryptedPrefsTestSupport.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/EncryptedPrefsTestSupport.kt @@ -50,6 +50,30 @@ internal fun corruptStoredValue(context: Context, prefsName: String, storedName: .commit() } +/** + * Breaks the stored value of [plainKey], which must already be written. + * + * [writeAndFindStoredName] finds a name by what a write adds, which does not work for a + * key already present. Here the name is found the only other way available from outside: + * removing the key and seeing which entry goes. [restore] then writes it back, which lands + * on the same name because the file's HMAC key is unchanged, and that value is broken. + */ +internal fun corruptExistingValue( + context: Context, + prefsName: String, + plainKey: String, + restore: (android.content.SharedPreferences.Editor) -> Unit +) { + val before = rawPrefs(context, prefsName).all.keys.toSet() + KeystoreEncryptedPrefs.create(context, prefsName).edit().remove(plainKey).commit() + val removed = before - rawPrefs(context, prefsName).all.keys.toSet() + assertEquals("expected exactly one entry to go with $plainKey: $removed", 1, removed.size) + val editor = KeystoreEncryptedPrefs.create(context, prefsName).edit() + restore(editor) + editor.commit() + corruptStoredValue(context, prefsName, removed.single()) +} + /** * Breaks name resolution for a whole file, the other half of the fault a lost Keystore * alias produces. diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt index e251cbac..f90fc8e6 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/PinStoreUnreadableTest.kt @@ -168,15 +168,15 @@ class PinStoreUnreadableTest { */ @Test fun an_unreadable_lockout_anchor_runs_once_and_then_counts_down() { + // A real PIN, so the record is SET on the strength of a usable hash, and a lockout + // in force whose start time is the one value broken. + assertTrue(PinStore(context).setPin(GOOD_PIN)) KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit() - .putBoolean("pin_enabled", true) .putLong("lockout_duration", LONGEST_LOCKOUT_MS) .commit() - val storedName = writeAndFindStoredName(context, PinStore.PREFS_NAME) { - KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME) - .edit().putLong("lockout_set_at_elapsed", 1L).commit() + corruptExistingValue(context, PinStore.PREFS_NAME, "lockout_set_at_elapsed") { + it.putLong("lockout_set_at_elapsed", 1L) } - corruptStoredValue(context, PinStore.PREFS_NAME, storedName) assertEquals(PinRecord.SET, PinStore(context).pinRecord()) val first = PinStore(context).getLockoutRemainingMs() @@ -188,7 +188,6 @@ class PinStoreUnreadableTest { "the lockout has to count down rather than restart on every read: $first then $second", first - second >= COUNTDOWN_PROBE_MS - COUNTDOWN_SLACK_MS ) - forgetWriteProbe(context, PinStore.PREFS_NAME) } /** @@ -215,6 +214,23 @@ class PinStoreUnreadableTest { assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) } + /** + * A flag saying SET with no hash at all. No PIN can ever verify against it, and as + * SET it showed the ordinary PIN field, refusing every entry with no way out. It is + * the same uncheckable state as a hash that will not decrypt, so it gets the same exit. + */ + @Test + fun a_set_flag_with_no_hash_is_unreadable() { + KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit() + .putBoolean("pin_enabled", true) + .commit() + + val store = PinStore(context) + assertEquals(PinRecord.UNREADABLE, store.pinRecord()) + assertTrue("and the way out is offered for it", store.clearPin()) + assertEquals(PinRecord.NOT_SET, PinStore(context).pinRecord()) + } + /** * The reverse: a broken flag beside a hash and salt that still read. disablePin removes * the hash and salt, so a usable pair means a PIN was set and never disabled. Reading @@ -223,14 +239,9 @@ class PinStoreUnreadableTest { @Test fun a_broken_flag_beside_a_usable_verifier_is_still_set() { assertTrue(PinStore(context).setPin(GOOD_PIN)) - // The stored name is an HMAC of the key under the file's own random key, so it is - // found by removing the flag and seeing which entry goes, then writing it back, - // which lands on the same name. - val before = rawPrefs(context, PinStore.PREFS_NAME).all.keys.toSet() - KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit().remove("pin_enabled").commit() - val flagName = (before - rawPrefs(context, PinStore.PREFS_NAME).all.keys.toSet()).single() - KeystoreEncryptedPrefs.create(context, PinStore.PREFS_NAME).edit().putBoolean("pin_enabled", true).commit() - corruptStoredValue(context, PinStore.PREFS_NAME, flagName) + corruptExistingValue(context, PinStore.PREFS_NAME, "pin_enabled") { + it.putBoolean("pin_enabled", true) + } val store = PinStore(context) assertEquals(PinRecord.SET, store.pinRecord()) diff --git a/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt b/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt index 8443eae7..793121f8 100644 --- a/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt +++ b/app/src/main/kotlin/io/privkey/keep/BiometricHelper.kt @@ -116,28 +116,36 @@ class BiometricHelper private constructor( } /** - * Prompts for the device's own lock-screen credential, for an action that has to be - * authorized when no biometric can be used. + * Prompts for a strong biometric or the device's own lock-screen credential, whichever + * the person uses, for an action that has to be authorized now. + * + * Both are allowed in one prompt so that a biometric in lockout, or a sensor that is + * unavailable, offers the credential instead of failing outright. Returns the + * [BiometricPrompt.AuthenticationResult.getAuthenticationType] of the success, so the + * caller can tell a biometric from a credential, or null if nothing was satisfied. * * A prompt that allows DEVICE_CREDENTIAL must not set a negative button, which is why * this does not go through [resolvePromptInfo]: the builder rejects the combination. - * Never short-circuited by the biometric timeout window, since the callers are - * removing an authentication factor and need a credential entered now. + * Never short-circuited by the biometric timeout window, and never opens one, since + * the callers are removing an authentication factor and need it presented now. */ - suspend fun authenticateWithDeviceCredential(title: String, subtitle: String): Boolean { + suspend fun authenticateStrongOrDeviceCredential(title: String, subtitle: String): Int? { val promptInfo = BiometricPrompt.PromptInfo.Builder() .setTitle(title) .setSubtitle(subtitle) - .setAllowedAuthenticators(BiometricManager.Authenticators.DEVICE_CREDENTIAL) + .setAllowedAuthenticators( + BiometricManager.Authenticators.BIOMETRIC_STRONG or + BiometricManager.Authenticators.DEVICE_CREDENTIAL + ) .build() return suspendCoroutine { continuation -> val callback = object : BiometricPrompt.AuthenticationCallback() { override fun onAuthenticationSucceeded(result: BiometricPrompt.AuthenticationResult) { - continuation.resume(true) + continuation.resume(result.authenticationType) } override fun onAuthenticationError(errorCode: Int, errString: CharSequence) { - continuation.resume(false) + continuation.resume(null) } override fun onAuthenticationFailed() {} diff --git a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt index 144b79bb..111a7a46 100644 --- a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt +++ b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt @@ -6,6 +6,7 @@ import android.os.Bundle import android.util.Log import android.widget.Toast import androidx.activity.compose.setContent +import androidx.biometric.BiometricPrompt import androidx.compose.foundation.Image import androidx.compose.foundation.layout.* import androidx.compose.foundation.rememberScrollState @@ -162,38 +163,36 @@ class MainActivity : FragmentActivity() { pinStore = pinStoreForUnlock, onUnlocked = { isPinUnlocked = true }, // Removing an unreadable PIN takes away an authentication factor - // before anyone has authenticated, so it asks for the strongest - // factor the device can offer right now. A biometric first. If - // none is usable, which includes a sensor that is only - // temporarily unavailable, the device's own lock-screen - // credential. Only a device with no secure lock at all runs it - // unasked, because then there is nothing to ask for and refusing - // would leave no way into the app. The secure-lock check is made - // here against the platform rather than through the helper, so a - // missing helper on a secure device refuses instead of falling - // through to the unasked branch. - authorizePinRemoval = { - val deviceSecure = - getSystemService(KeyguardManager::class.java)?.isDeviceSecure == true - when { - biometricAvailable -> { - val ok = biometricHelper?.authenticate( - title = removeTitle, - subtitle = removeSubtitle, - forcePrompt = true - ) ?: false - // A biometric here is as good as the one the lock - // screen below would ask for, so it is not asked twice. - if (ok) isBiometricUnlocked = true - ok - } - deviceSecure -> - biometricHelper?.authenticateWithDeviceCredential( - removeTitle, - removeSubtitle - ) ?: false - else -> true + // before anyone has authenticated, so on a device with a secure + // lock it asks for a strong biometric or the lock-screen + // credential in one prompt. Allowing both means a biometric in + // lockout, or a sensor that is unavailable, offers the credential + // rather than refusing, and enrolling a biometric requires a + // secure lock, so this covers every device that has one. Only a + // device with no secure lock at all runs it unasked, because then + // there is nothing to ask for and refusing would leave no way + // into the app. + // + // Both platform answers fail closed. The secure-lock check is made + // against the platform rather than through the helper, so a + // missing helper on a secure device refuses; and a missing + // KeyguardManager refuses instead of counting as insecure, which + // would be the one input that reached the unasked branch. + authorizePinRemoval = authorize@{ + val keyguard = getSystemService(KeyguardManager::class.java) + ?: return@authorize false + if (!keyguard.isDeviceSecure) return@authorize true + val type = biometricHelper?.authenticateStrongOrDeviceCredential( + removeTitle, + removeSubtitle + ) ?: return@authorize false + // A biometric here is as good as the one the lock screen below + // would ask for, so it is not asked twice. A credential is not, + // so that screen still asks. + if (type == BiometricPrompt.AUTHENTICATION_RESULT_TYPE_BIOMETRIC) { + isBiometricUnlocked = true } + true }, onBiometricAuth = if (biometricAvailable) { { diff --git a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt index ae7c3d54..1f8735d4 100644 --- a/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt +++ b/app/src/main/kotlin/io/privkey/keep/PinUnlockScreen.kt @@ -154,29 +154,15 @@ fun PinUnlockScreen( ) } Spacer(modifier = Modifier.height(24.dp)) - // A biometric already unlocks this screen in its PIN-entry state, so it is - // offered here first as the way in that destroys nothing. The record stays - // as it is, and if the fault was transient it can read again later. It - // does not refresh the session, which writes to the file that just failed - // to read; signing stays blocked until the record is repaired, as the body - // text says. - if (onBiometricAuth != null) { - Button( - onClick = { - coroutineScope.launch { - if (onBiometricAuth()) { - onBiometricSuccess() - onUnlocked() - } - } - }, - modifier = Modifier.fillMaxWidth() - ) { - Text(stringResource(R.string.pin_unlock_use_biometrics)) - } - Spacer(modifier = Modifier.height(8.dp)) - } - OutlinedButton( + // Removal is the only way in from here, not a biometric unlock that leaves + // the record alone. Such an unlock cannot hold: the activity re-derives the + // unlock from isSessionValid on every resume, the session cannot be + // refreshed against a file that will not decrypt, and dismissing the + // prompt resumes the activity, so the lock screen comes straight back. + // Making it hold would mean overriding the session check on resume, which + // is a hole in the relock rather than a feature. A transient fault is + // covered instead by clearPin reading the record again before deleting. + Button( onClick = { coroutineScope.launch { removeFailed = false diff --git a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt index dd062122..7b6e1ea5 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/PinStore.kt @@ -99,30 +99,30 @@ class PinStore(private val context: Context) { fun pinRecord(): PinRecord = classify(prefs) /** - * Decided by the flag and the hash together, because a record is only usable when - * the flag says a PIN is set and the hash and salt can be read to verify one. + * Decided by the flag and the hash together. A PIN is SET only when one can actually + * be verified, which takes a readable hash and salt; anything that leaves a PIN + * possibly set but uncheckable is UNREADABLE, the state the unlock screen offers a + * way out of. * - * The flag alone left two cases wrong. A readable flag beside an unreadable hash read - * as SET, so every PIN was refused as incorrect, no attempt was spent, and the - * removal was never offered: a lockout with no way out. A broken flag beside a usable - * hash read as UNREADABLE and offered removal of a PIN that could still verify. - * disablePin removes the hash and salt, so a usable hash means a PIN was set and - * never disabled, which is what makes SET the right answer for the second case. + * The flag alone left three cases wrong. A flag saying SET beside a hash that would + * not decrypt, or that was missing, read as SET, so every PIN was refused as + * incorrect, no attempt was spent, and the removal was never offered: a lockout with + * no way out. And a broken flag beside a usable hash read as UNREADABLE, offering to + * delete a PIN that could still verify. disablePin removes the hash and salt, so a + * usable pair means a PIN was set and never disabled, which makes SET right there. + * + * The hash and salt are read only when the flag leaves the answer open, so a store + * with no PIN costs one Keystore decrypt rather than three. This runs on the + * background provider path, which any installed app can drive. */ private fun classify(record: ReadablePrefs): PinRecord { val flag = record.readBoolean(KEY_PIN_ENABLED) - val hash = record.readString(KEY_PIN_HASH) - val salt = record.readString(KEY_PIN_SALT) - val verifierUnreadable = hash == PrefRead.Unreadable || salt == PrefRead.Unreadable - val verifierUsable = hash is PrefRead.Found && salt is PrefRead.Found - return when (flag) { - is PrefRead.Found -> - if (!flag.value) PinRecord.NOT_SET - else if (verifierUnreadable) PinRecord.UNREADABLE - else PinRecord.SET - PrefRead.Absent -> PinRecord.NOT_SET - PrefRead.Unreadable -> if (verifierUsable) PinRecord.SET else PinRecord.UNREADABLE + if (flag == PrefRead.Absent || (flag is PrefRead.Found && !flag.value)) { + return PinRecord.NOT_SET } + val verifierUsable = record.readString(KEY_PIN_HASH) is PrefRead.Found && + record.readString(KEY_PIN_SALT) is PrefRead.Found + return if (verifierUsable) PinRecord.SET else PinRecord.UNREADABLE } /** True when a PIN may be set, which includes a record that cannot be read. */ @@ -160,7 +160,10 @@ class PinStore(private val context: Context) { } @Synchronized - fun requiresAuthentication(): Boolean = isPinEnabled() && !isSessionValid() + // One classification per call. Calling isPinEnabled and then isSessionValid, which + // asks again, decrypted the record twice on a path any installed app can drive + // through the provider, ahead of its rate limit. + fun requiresAuthentication(): Boolean = isPinEnabled() && !sessionOpen() @Synchronized fun isWeakPin(pin: String): Boolean { @@ -299,8 +302,14 @@ class PinStore(private val context: Context) { } @Synchronized - fun isSessionValid(): Boolean { - if (!isPinEnabled()) return true + fun isSessionValid(): Boolean = !isPinEnabled() || sessionOpen() + + /** + * Whether a session opened by a verified unlock is still open, without first asking + * whether a PIN is set. Split out so [requiresAuthentication] classifies the record + * once rather than twice. + */ + private fun sessionOpen(): Boolean { // An unreadable timestamp answers 0, which is the no-session value, so an // unreadable session is not a valid one. val sessionStartedAtRealtime = prefs.readLong(KEY_SESSION_STARTED_AT_REALTIME).resolve(ifAbsent = 0, ifUnreadable = 0) From 75b57957fc26c77481dd510240573e997e8c39c1 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 12:21:10 -0400 Subject: [PATCH 05/10] Report what the whitelist screen and its store saw when the unreadable state never appears --- .../keep/PinUnlockScreenUnreadableTest.kt | 5 +- .../nip55/RelayAuthWhitelistScreenTest.kt | 107 +++++++++++++++--- 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt index 590026fc..1917d562 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt @@ -87,9 +87,8 @@ class PinUnlockScreenUnreadableTest { } } - // 15s rather than 5s, for the reason RelayAuthWhitelistScreenTest gives: the state - // appears after a Keystore decrypt fails off the main thread, which runs well past - // its local time on the CI emulator. waitUntil returns as soon as it holds. + // A generous bound, since the CI emulator runs these at about twice local speed, and + // waitUntil returns as soon as the condition holds, so it costs nothing on a pass. private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index 46295436..94147bd8 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -2,11 +2,14 @@ package io.privkey.keep.nip55 import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.ui.test.ComposeTimeoutException import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.onRoot import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.printToString import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import io.privkey.keep.R @@ -14,6 +17,7 @@ import io.privkey.keep.storage.PrefRead import io.privkey.keep.storage.RelayAuthWhitelistStore import io.privkey.keep.storage.corruptStoredValue import io.privkey.keep.storage.forgetWriteProbe +import io.privkey.keep.storage.rawPrefs import io.privkey.keep.storage.writeAndFindStoredName import io.privkey.keep.ui.theme.KeepAndroidTheme import kotlinx.coroutines.launch @@ -70,11 +74,16 @@ class RelayAuthWhitelistScreenTest { context.deleteSharedPreferences(RelayAuthWhitelistStore.PREFS_NAME) } - private fun stageUnreadableList() { + /** What was staged, kept so a timeout can say whether it is still on disk. */ + private class Staged(val storedName: String, val corruptedValue: String) + + private fun stageUnreadableList(): Staged { val storedName = writeAndFindStoredName(context, RelayAuthWhitelistStore.PREFS_NAME) { - RelayAuthWhitelistStore(context).add("wss://relay.example.com") + RelayAuthWhitelistStore(context).add("wss://$STAGED_HOST") } corruptStoredValue(context, RelayAuthWhitelistStore.PREFS_NAME, storedName) + val corrupted = rawPrefs(context, RelayAuthWhitelistStore.PREFS_NAME).getString(storedName, null) + return Staged(storedName, checkNotNull(corrupted) { "the staged entry vanished as it was written" }) } /** @@ -87,10 +96,10 @@ class RelayAuthWhitelistScreenTest { */ @Test fun theResetDoesNotHappenWithoutTheAuthGate() { - stageUnreadableList() + val staged = stageUnreadableList() showScreen(gateReset = { /* never runs the action */ }) - awaitText(unreadableText) + awaitUnreadable(staged) compose.onNodeWithText(resetText).performClick() val confirm = context.getString(R.string.connections_nip55_relay_auth_whitelist_reset_confirm) awaitText(confirm) @@ -107,12 +116,17 @@ class RelayAuthWhitelistScreenTest { * was satisfied, so the cases below assert what the screen does after the gate * rather than what the gate does. Production supplies `requireAuthThen`. */ + /** The store the screen under test holds, so a timeout can ask it directly. */ + private var screenStore: RelayAuthWhitelistStore? = null + private fun showScreen(gateReset: ((suspend () -> Unit) -> Unit)? = null) { compose.setContent { // Remembered, not rebuilt per recomposition: production hands this screen // the application's single store, and a fresh one each pass would give the // launched read and the button handlers different per-instance caches. - val store = remember { RelayAuthWhitelistStore(context) } + // Kept for the timeout report only. Created where it always was, inside the + // composition, so holding a reference does not change when it is built. + val store = remember { RelayAuthWhitelistStore(context).also { screenStore = it } } val scope = rememberCoroutineScope() val satisfiedGate: (suspend () -> Unit) -> Unit = { action -> scope.launch { action() } } KeepAndroidTheme { @@ -125,21 +139,80 @@ class RelayAuthWhitelistScreenTest { } } - // 15s, not 5s. The unreadable state appears only after a Keystore decrypt fails on an - // IO dispatcher, which takes about 1s here and about 2s on the CI emulator, and on CI - // it has also run past 5s: the same wait then passed in one test and timed out in two - // others of the same run. waitUntil returns as soon as the text appears, so the larger - // bound costs nothing on a pass, and a screen that never shows it still fails. + // A generous bound, since the CI emulator runs these at about twice local speed and + // waitUntil returns as soon as the text appears, so it costs nothing on a pass. It is + // not what decides a failure: the CI timeouts these tests have hit ran out the full + // 15s, so the state was not arriving late, it was not arriving. awaitUnreadable says + // what was there instead. private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } + /** + * Waits for the unreadable state, and on a timeout fails with what was actually there. + * + * These tests have timed out on CI while passing every local run, including full + * suites, so a bare timeout gives nothing to act on. The report separates the layers + * that can fail: whether the damaged entry is still damaged in the process's copy of + * the file, what the screen's own store and a fresh store each answer now, and which + * state the screen is showing. The two stores disagreeing about one file is a fault + * in how a store instance comes to see it; both reporting the fault behind a screen + * showing something else is the screen; a rewritten entry is something else writing + * the file. It reads the copy the process holds in memory, not the physical file. + */ + private fun awaitUnreadable(staged: Staged) { + try { + awaitText(unreadableText) + } catch (e: ComposeTimeoutException) { + throw AssertionError(describe(staged), e) + } + } + + /** + * One store's answer for the list, in words. The screen's own store and a fresh one + * are both asked: if they disagree about the same file, the fault is in how a store + * instance comes to see the file, not in the file or the screen. + */ + private fun describeRead(store: RelayAuthWhitelistStore): String = + runCatching { store.readHosts() }.fold( + onSuccess = { read -> + when (read) { + is PrefRead.Found -> "Found with ${read.value.size} host(s)" + PrefRead.Absent -> "Absent" + PrefRead.Unreadable -> "Unreadable" + } + }, + onFailure = { "a raised ${it::class.simpleName}" } + ) + + private fun describe(staged: Staged): String { + val raw = rawPrefs(context, RelayAuthWhitelistStore.PREFS_NAME) + val onDisk = raw.getString(staged.storedName, null) + val diskState = when (onDisk) { + null -> "The damaged entry is gone from the process's copy of the file" + staged.corruptedValue -> "The damaged entry is still in the process's copy of the file, unchanged" + else -> "The damaged entry was rewritten in the process's copy of the file" + } + val screenRead = screenStore?.let(::describeRead) ?: "nothing, it was never created" + val freshRead = describeRead(RelayAuthWhitelistStore(context)) + val shown = listOf("unreadable" to unreadableText, "empty" to emptyText, "a host row" to STAGED_HOST) + .filter { (_, text) -> compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } + .map { it.first } + .ifEmpty { listOf("none of the known states") } + val tree = runCatching { compose.onRoot().printToString() } + .getOrElse { "unavailable: ${it::class.simpleName}" } + return "The unreadable state never appeared. $diskState, with ${raw.all.size} raw " + + "entries in that copy; the screen's own store answers $screenRead; a fresh store answers " + + "$freshRead; the screen shows " + + "${shown.joinToString()}.\n$tree" + } + @Test fun anUnreadableListIsReportedRatherThanShownAsEmpty() { - stageUnreadableList() + val staged = stageUnreadableList() showScreen() - awaitText(unreadableText) + awaitUnreadable(staged) compose.onNodeWithText(unreadableText).assertIsDisplayed() compose.onNodeWithText(emptyText).assertDoesNotExist() // The input is gone too: every add in this state fails, and it fails with the @@ -167,10 +240,10 @@ class RelayAuthWhitelistScreenTest { // deleted on either answer would pass every other case here. @Test fun decliningTheResetLeavesTheListAlone() { - stageUnreadableList() + val staged = stageUnreadableList() showScreen() - awaitText(unreadableText) + awaitUnreadable(staged) compose.onNodeWithText(resetText).performClick() val cancel = context.getString(R.string.common_cancel) awaitText(cancel) @@ -187,10 +260,10 @@ class RelayAuthWhitelistScreenTest { @Test fun resettingAnUnreadableListClearsItAndTheScreenRecovers() { - stageUnreadableList() + val staged = stageUnreadableList() showScreen() - awaitText(unreadableText) + awaitUnreadable(staged) compose.onNodeWithText(resetText).performClick() // Confirmed rather than done on the first tap: an unreadable list can be a @@ -213,5 +286,7 @@ class RelayAuthWhitelistScreenTest { private companion object { const val AWAIT_MS = 15_000L + // Normalized form, which is what the store keeps and the screen renders. + const val STAGED_HOST = "relay.example.com" } } From 26a083c9b6c8ebc04a7c85538e241a13c09120ec Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 14:28:08 -0400 Subject: [PATCH 06/10] Report where the whitelist screen's read is when the unreadable state never appears --- .../nip55/RelayAuthWhitelistScreenTest.kt | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index 94147bd8..4a2f2f1c 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -204,7 +204,30 @@ class RelayAuthWhitelistScreenTest { return "The unreadable state never appeared. $diskState, with ${raw.all.size} raw " + "entries in that copy; the screen's own store answers $screenRead; a fresh store answers " + "$freshRead; the screen shows " + - "${shown.joinToString()}.\n$tree" + "${shown.joinToString()}.\n${describeThreads()}\n$tree" + } + + /** + * Every other thread inside app or Keystore code, with its stack. + * + * The screen shows its initial empty state, which is also what it shows while its one + * read is still in flight, so the question is whether that read is stuck and where. + * The store answering this thread afterwards does not settle it: a read that fails for + * any reason, including a busy Keystore, also answers Unreadable. + */ + private fun describeThreads(): String { + val current = Thread.currentThread() + val busy = Thread.getAllStackTraces().filter { (thread, stack) -> + thread !== current && stack.any { + it.className.startsWith("io.privkey.keep") || it.className.startsWith("android.security") + } + } + val workers = Thread.getAllStackTraces().keys.filter { it.name.startsWith("DefaultDispatcher-worker") } + val header = "Dispatcher workers: ${workers.size}, " + + workers.groupingBy { it.state }.eachCount() + ". Threads in app or Keystore code: ${busy.size}." + return busy.entries.joinToString("\n", prefix = "$header\n") { (thread, stack) -> + "--- ${thread.name} (${thread.state})\n" + stack.take(40).joinToString("\n") { " at $it" } + } } @Test From 259da5c2f4bfd2fb1442ea4f446648978214cd2e Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Sun, 18 Oct 2026 22:25:47 -0400 Subject: [PATCH 07/10] Report whether the whitelist screen's state arrives once pending writes are announced --- .../keep/nip55/RelayAuthWhitelistScreenTest.kt | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index 4a2f2f1c..b7bae532 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -2,6 +2,7 @@ package io.privkey.keep.nip55 import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope +import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.ui.test.ComposeTimeoutException import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createComposeRule @@ -164,10 +165,25 @@ class RelayAuthWhitelistScreenTest { try { awaitText(unreadableText) } catch (e: ComposeTimeoutException) { - throw AssertionError(describe(staged), e) + val report = describe(staged) + throw AssertionError("$report\n${afterForcedApply()}", e) } } + /** + * Whether the state arrives once pending snapshot writes are announced and the clock + * moves. If it does, the read's write happened and recomposition was never told; if + * not, the write itself never ran. + */ + private fun afterForcedApply(): String { + Snapshot.sendApplyNotifications() + compose.mainClock.advanceTimeBy(1_000) + compose.waitForIdle() + val appeared = compose.onAllNodesWithText(unreadableText).fetchSemanticsNodes().isNotEmpty() + return "After announcing pending snapshot writes and advancing the clock, the unreadable " + + "state ${if (appeared) "appeared" else "still did not appear"}." + } + /** * One store's answer for the list, in words. The screen's own store and a fresh one * are both asked: if they disagree about the same file, the fault is in how a store From 3de7633ba4fa4bff4616cb7e7510178228564820 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Mon, 19 Oct 2026 01:44:07 -0400 Subject: [PATCH 08/10] Run the whitelist screen tests on the v2 compose rule so the screen's read resumes on the main thread --- .../nip55/RelayAuthWhitelistScreenTest.kt | 62 ++++--------------- 1 file changed, 11 insertions(+), 51 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index b7bae532..a56e635f 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -2,10 +2,9 @@ package io.privkey.keep.nip55 import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope -import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.ui.test.ComposeTimeoutException import androidx.compose.ui.test.assertIsDisplayed -import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.junit4.v2.createComposeRule import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.onRoot @@ -46,9 +45,12 @@ import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class RelayAuthWhitelistScreenTest { - // createComposeRule() is deprecated in favor of the v2 API; the classic rule - // is intentional here, and the project builds with -Werror. - @Suppress("DEPRECATION") + // The v2 rule, not the classic one. The screen loads its list in a LaunchedEffect + // with an IO hop, and the classic rule resumes such a continuation on the IO thread, + // so the screen's state was written off the main thread while the main thread could + // still be composing it. Now and then the update never showed: the read finished with + // the right answer and the screen stayed on its initial empty state. The v2 rule + // resumes on the main thread, as the app does. @get:Rule val compose = createComposeRule() @@ -141,10 +143,7 @@ class RelayAuthWhitelistScreenTest { } // A generous bound, since the CI emulator runs these at about twice local speed and - // waitUntil returns as soon as the text appears, so it costs nothing on a pass. It is - // not what decides a failure: the CI timeouts these tests have hit ran out the full - // 15s, so the state was not arriving late, it was not arriving. awaitUnreadable says - // what was there instead. + // waitUntil returns as soon as the text appears, so it costs nothing on a pass. private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() } @@ -152,8 +151,7 @@ class RelayAuthWhitelistScreenTest { /** * Waits for the unreadable state, and on a timeout fails with what was actually there. * - * These tests have timed out on CI while passing every local run, including full - * suites, so a bare timeout gives nothing to act on. The report separates the layers + * A bare timeout gives nothing to act on. The report separates the layers * that can fail: whether the damaged entry is still damaged in the process's copy of * the file, what the screen's own store and a fresh store each answer now, and which * state the screen is showing. The two stores disagreeing about one file is a fault @@ -165,25 +163,10 @@ class RelayAuthWhitelistScreenTest { try { awaitText(unreadableText) } catch (e: ComposeTimeoutException) { - val report = describe(staged) - throw AssertionError("$report\n${afterForcedApply()}", e) + throw AssertionError(describe(staged), e) } } - /** - * Whether the state arrives once pending snapshot writes are announced and the clock - * moves. If it does, the read's write happened and recomposition was never told; if - * not, the write itself never ran. - */ - private fun afterForcedApply(): String { - Snapshot.sendApplyNotifications() - compose.mainClock.advanceTimeBy(1_000) - compose.waitForIdle() - val appeared = compose.onAllNodesWithText(unreadableText).fetchSemanticsNodes().isNotEmpty() - return "After announcing pending snapshot writes and advancing the clock, the unreadable " + - "state ${if (appeared) "appeared" else "still did not appear"}." - } - /** * One store's answer for the list, in words. The screen's own store and a fresh one * are both asked: if they disagree about the same file, the fault is in how a store @@ -220,30 +203,7 @@ class RelayAuthWhitelistScreenTest { return "The unreadable state never appeared. $diskState, with ${raw.all.size} raw " + "entries in that copy; the screen's own store answers $screenRead; a fresh store answers " + "$freshRead; the screen shows " + - "${shown.joinToString()}.\n${describeThreads()}\n$tree" - } - - /** - * Every other thread inside app or Keystore code, with its stack. - * - * The screen shows its initial empty state, which is also what it shows while its one - * read is still in flight, so the question is whether that read is stuck and where. - * The store answering this thread afterwards does not settle it: a read that fails for - * any reason, including a busy Keystore, also answers Unreadable. - */ - private fun describeThreads(): String { - val current = Thread.currentThread() - val busy = Thread.getAllStackTraces().filter { (thread, stack) -> - thread !== current && stack.any { - it.className.startsWith("io.privkey.keep") || it.className.startsWith("android.security") - } - } - val workers = Thread.getAllStackTraces().keys.filter { it.name.startsWith("DefaultDispatcher-worker") } - val header = "Dispatcher workers: ${workers.size}, " + - workers.groupingBy { it.state }.eachCount() + ". Threads in app or Keystore code: ${busy.size}." - return busy.entries.joinToString("\n", prefix = "$header\n") { (thread, stack) -> - "--- ${thread.name} (${thread.state})\n" + stack.take(40).joinToString("\n") { " at $it" } - } + "${shown.joinToString()}.\n$tree" } @Test From 3dcc06d65e6a9ba02fb370d84feae12ec9567efa Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Mon, 19 Oct 2026 02:12:22 -0400 Subject: [PATCH 09/10] Add a readable whitelist control that only passes once the screen's read has landed --- .../keep/nip55/RelayAuthWhitelistScreenTest.kt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index a56e635f..c6dc8940 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -235,6 +235,21 @@ class RelayAuthWhitelistScreenTest { compose.onNodeWithText(resetText).assertDoesNotExist() } + // The empty case above also matches the screen before its read lands, since it opens + // on an empty list. A listed host appears only once the read has landed, so this is + // the readable control that cannot pass on the initial state. + @Test + fun aReadableListShowsItsHostsRatherThanAFault() { + RelayAuthWhitelistStore(context).add("wss://$STAGED_HOST") + showScreen() + + awaitText(STAGED_HOST) + compose.onNodeWithText(STAGED_HOST).assertIsDisplayed() + compose.onNodeWithText(unreadableText).assertDoesNotExist() + compose.onNodeWithText(emptyText).assertDoesNotExist() + compose.onNodeWithText(resetText).assertDoesNotExist() + } + // Declining has to leave the list where it is. Without this, a confirmation that // deleted on either answer would pass every other case here. @Test From 1824ab8816225b006b08ff996aaaab5cd5e0c623 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Mon, 19 Oct 2026 09:09:17 -0400 Subject: [PATCH 10/10] Correct the whitelist and PIN screen test comments to match what they show --- .../keep/PinUnlockScreenUnreadableTest.kt | 2 +- .../nip55/RelayAuthWhitelistScreenTest.kt | 46 ++++++++----------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt index 1917d562..faf4860a 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/PinUnlockScreenUnreadableTest.kt @@ -87,7 +87,7 @@ class PinUnlockScreenUnreadableTest { } } - // A generous bound, since the CI emulator runs these at about twice local speed, and + // A generous bound, since the CI emulator runs these at about half local speed, and // waitUntil returns as soon as the condition holds, so it costs nothing on a pass. private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt index c6dc8940..cb04bf0e 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthWhitelistScreenTest.kt @@ -47,10 +47,10 @@ class RelayAuthWhitelistScreenTest { // The v2 rule, not the classic one. The screen loads its list in a LaunchedEffect // with an IO hop, and the classic rule resumes such a continuation on the IO thread, - // so the screen's state was written off the main thread while the main thread could - // still be composing it. Now and then the update never showed: the read finished with - // the right answer and the screen stayed on its initial empty state. The v2 rule - // resumes on the main thread, as the app does. + // writing the screen's state off the main thread. Under it the update now and then + // never showed: the read finished with the right answer and the screen stayed on its + // initial empty state. The v2 rule resumes on the main thread, as the app does, and + // the failure has not recurred under it. @get:Rule val compose = createComposeRule() @@ -114,21 +114,19 @@ class RelayAuthWhitelistScreenTest { forgetWriteProbe(context, RelayAuthWhitelistStore.PREFS_NAME) } + /** The store the screen under test holds, so a timeout can ask it directly. */ + private var screenStore: RelayAuthWhitelistStore? = null + /** * [gateReset] defaults to running the action, standing in for an auth factor that * was satisfied, so the cases below assert what the screen does after the gate * rather than what the gate does. Production supplies `requireAuthThen`. */ - /** The store the screen under test holds, so a timeout can ask it directly. */ - private var screenStore: RelayAuthWhitelistStore? = null - private fun showScreen(gateReset: ((suspend () -> Unit) -> Unit)? = null) { compose.setContent { // Remembered, not rebuilt per recomposition: production hands this screen // the application's single store, and a fresh one each pass would give the // launched read and the button handlers different per-instance caches. - // Kept for the timeout report only. Created where it always was, inside the - // composition, so holding a reference does not change when it is built. val store = remember { RelayAuthWhitelistStore(context).also { screenStore = it } } val scope = rememberCoroutineScope() val satisfiedGate: (suspend () -> Unit) -> Unit = { action -> scope.launch { action() } } @@ -142,7 +140,7 @@ class RelayAuthWhitelistScreenTest { } } - // A generous bound, since the CI emulator runs these at about twice local speed and + // A generous bound, since the CI emulator runs these at about half local speed and // waitUntil returns as soon as the text appears, so it costs nothing on a pass. private fun awaitText(text: String) = compose.waitUntil(timeoutMillis = AWAIT_MS) { compose.onAllNodesWithText(text).fetchSemanticsNodes().isNotEmpty() @@ -151,13 +149,10 @@ class RelayAuthWhitelistScreenTest { /** * Waits for the unreadable state, and on a timeout fails with what was actually there. * - * A bare timeout gives nothing to act on. The report separates the layers - * that can fail: whether the damaged entry is still damaged in the process's copy of - * the file, what the screen's own store and a fresh store each answer now, and which - * state the screen is showing. The two stores disagreeing about one file is a fault - * in how a store instance comes to see it; both reporting the fault behind a screen - * showing something else is the screen; a rewritten entry is something else writing - * the file. It reads the copy the process holds in memory, not the physical file. + * The report says whether the damaged entry is still damaged in the process's copy + * of the file, what the screen's own store and a fresh store each answer now, and + * which state the screen is showing, so a failure points at the file, the store or + * the screen. */ private fun awaitUnreadable(staged: Staged) { try { @@ -167,11 +162,7 @@ class RelayAuthWhitelistScreenTest { } } - /** - * One store's answer for the list, in words. The screen's own store and a fresh one - * are both asked: if they disagree about the same file, the fault is in how a store - * instance comes to see the file, not in the file or the screen. - */ + /** One store's answer for the list, in words. */ private fun describeRead(store: RelayAuthWhitelistStore): String = runCatching { store.readHosts() }.fold( onSuccess = { read -> @@ -223,8 +214,9 @@ class RelayAuthWhitelistScreenTest { forgetWriteProbe(context, RelayAuthWhitelistStore.PREFS_NAME) } - // The readable case, so the assertions above cannot pass on a screen that reports a - // fault whatever it reads. + // An empty list is reported as empty, not as a fault. This also matches the screen + // before its read lands, since it opens on an empty list, so the listed-host case + // below is the control that proves the read. @Test fun anEmptyListIsStillReportedAsEmpty() { showScreen() @@ -235,9 +227,9 @@ class RelayAuthWhitelistScreenTest { compose.onNodeWithText(resetText).assertDoesNotExist() } - // The empty case above also matches the screen before its read lands, since it opens - // on an empty list. A listed host appears only once the read has landed, so this is - // the readable control that cannot pass on the initial state. + // The readable control. A listed host appears only once the read has landed, so the + // unreadable assertions above cannot pass on a screen that reports a fault whatever + // it reads. @Test fun aReadableListShowsItsHostsRatherThanAFault() { RelayAuthWhitelistStore(context).add("wss://$STAGED_HOST")