From 7ab80013dfe8e3f0cf4d90a95d7da40749d2fdda Mon Sep 17 00:00:00 2001 From: null Date: Wed, 15 Jul 2026 00:25:20 -0500 Subject: [PATCH] fix(recovery): stop the two ways a returning user loses their history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recovering on a new phone was walked end to end (a fixture was wiped, then restored through the app's own flow). The Recovery screen itself is good — the app detects the missing key and routes there by itself, and the copy is honest. Everything around it had two holes, one of them destructive. 1. Sign-in could send a returning user into new-user profile setup (P2, data loss). OnboardingViewModel treated "couldn't read the profile" and "has no profile" as the same thing, so a slow read right after sign-in — a real race, hit live — routed them to CREATE_PROFILE, which asks "What should your partner call you?" over a `🔒 Couldn't unlock on this device` value. Anyone tapping through it re-encrypts and overwrites the name we had just failed to read, and never reaches Recovery. The repository already distinguishes the cases (missing doc = success(null), failed read throws), so only a successful read may now route to profile setup; a read that never succeeds retries and then goes Home, which is non-destructive and routes to Recovery on its own. 2. A correct phrase was reported as wrong. It is Argon2id key material, so it is byte-exact, and only .trim() was applied — while the field allowed the keyboard to capitalise the first word and autocorrect the wordlist. Every generated phrase is lowercase a-z single-spaced (all 248 WORDLIST entries checked), so folding typed input to that canonical form is lossless and cannot weaken the KDF: it only removes failures that were never about the phrase being wrong. "That phrase doesn't match" now means it actually doesn't. Also sets KeyboardCapitalization.None + autoCorrectEnabled = false. 3. The escape hatch was styled as a footnote. Most people arriving here are on a new phone and never saved the phrase, so "ask my partner" — not the field — is their real way through, yet it was a bare TextButton under the one control they can't use. It is now a full-width OutlinedButton with plainer copy ("I don't have the phrase — ask my partner"). It stays below the field rather than replacing it: someone who does have the phrase pasted from a message is unlocked instantly and offline, while this path waits on the partner. Adds RecoveryPhraseNormalizationTest (7 cases): auto-capitalisation, shouting, double/tab/newline spacing and chat-pasted text all fold to the canonical phrase, while a genuinely wrong phrase still differs. Unit suite green. Co-Authored-By: Claude Opus 4.8 --- .../app/closer/crypto/RecoveryKeyManager.kt | 17 +++++ .../ui/onboarding/OnboardingViewModel.kt | 48 ++++++++++++-- .../app/closer/ui/pairing/RecoveryScreen.kt | 25 ++++++-- .../closer/ui/pairing/RecoveryViewModel.kt | 5 +- .../crypto/RecoveryPhraseNormalizationTest.kt | 64 +++++++++++++++++++ 5 files changed, 146 insertions(+), 13 deletions(-) create mode 100644 app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt diff --git a/app/src/main/java/app/closer/crypto/RecoveryKeyManager.kt b/app/src/main/java/app/closer/crypto/RecoveryKeyManager.kt index ac5e7a3a..2dfc0738 100644 --- a/app/src/main/java/app/closer/crypto/RecoveryKeyManager.kt +++ b/app/src/main/java/app/closer/crypto/RecoveryKeyManager.kt @@ -123,6 +123,23 @@ class RecoveryKeyManager @Inject constructor() { } companion object { + + /** + * The canonical form of a typed recovery phrase. + * + * [generateRecoveryPhrase] only ever emits lowercase a-z words joined by single spaces (every + * word in WORDLIST is plain lowercase), and the phrase is fed straight into Argon2id as key + * material — so it is byte-exact: "Hunt down gear" or a stray double space derives a different + * key and fails as if it were the wrong phrase entirely. That is the worst possible lie to tell + * someone on the one screen standing between them and permanently unreadable history. + * + * Folding input to the canonical form is lossless (no generated phrase can contain uppercase, + * punctuation or repeated spaces) and cannot weaken the KDF: it only removes failures that were + * never about the phrase being wrong. Typed input must go through here before deriving. + */ + fun normalizePhrase(raw: String): String = + raw.trim().lowercase().split(Regex("\\s+")).filter { it.isNotBlank() }.joinToString(" ") + private const val PHRASE_WORD_COUNT = 10 private const val SALT_BYTES = 16 private const val KEY_BYTES = 32 diff --git a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt index 18ecdecd..c9e0c833 100644 --- a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt +++ b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt @@ -10,6 +10,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import javax.inject.Inject @@ -35,13 +36,8 @@ class OnboardingViewModel @Inject constructor( is AuthState.Loading -> _uiState.update { it.copy(isCheckingAuth = true) } is AuthState.Unauthenticated -> _uiState.update { it.copy(isCheckingAuth = false, navigateTo = null) } is AuthState.Authenticated -> { - val user = runCatching { userRepository.getUser(authState.userId) } - .onFailure { Log.w(TAG, "Could not load user profile during onboarding", it) } - .getOrNull() - val destination = when { - user == null || user.displayName.isBlank() || user.sex.isBlank() -> "create_profile" - else -> "home" - } + _uiState.update { it.copy(isCheckingAuth = true) } + val destination = resolveDestination(authState.userId) _uiState.update { it.copy(isCheckingAuth = false, navigateTo = destination) } } } @@ -49,9 +45,47 @@ class OnboardingViewModel @Inject constructor( } } + /** + * Where a signed-in user belongs: the profile step only if they genuinely have no profile yet. + * + * "Couldn't read the profile" and "has no profile" are NOT the same thing, and treating them the + * same was a data-loss trap. A returning user whose profile read failed (slow network, token not + * propagated yet — a real race seen right after sign-in) was sent to CREATE_PROFILE, which + * greets them with "What should your partner call you?" over a `🔒 Couldn't unlock on this + * device` value. Tapping through it re-encrypts and overwrites the very name we had just failed + * to read — and they never reach the Recovery screen that would have fixed them. + * + * The repository distinguishes the two cleanly: a missing document is `success(null)`, while a + * failed read throws. So only a *successful* read may route to the profile step. If the read + * never succeeds we send them Home, which is non-destructive and has its own offline/error + * handling — and which routes to Recovery by itself when the couple key is missing. + */ + private suspend fun resolveDestination(uid: String): String { + repeat(PROFILE_READ_ATTEMPTS) { attempt -> + val result = runCatching { userRepository.getUser(uid) } + if (result.isSuccess) { + val user = result.getOrNull() + val needsProfile = user == null || user.displayName.isBlank() || user.sex.isBlank() + return if (needsProfile) "create_profile" else "home" + } + Log.w( + TAG, + "Could not load user profile during onboarding (attempt ${attempt + 1})", + result.exceptionOrNull() + ) + if (attempt < PROFILE_READ_ATTEMPTS - 1) delay(RETRY_DELAY_MS * (attempt + 1)) + } + // Never read it. They are signed in, so they are not a new user: send them somewhere safe + // rather than inviting them to overwrite a profile we simply couldn't see. + Log.w(TAG, "Profile unreadable after $PROFILE_READ_ATTEMPTS attempts; routing Home, not to profile setup") + return "home" + } + fun onNavigated() = _uiState.update { it.copy(navigateTo = null) } companion object { private const val TAG = "OnboardingViewModel" + private const val PROFILE_READ_ATTEMPTS = 3 + private const val RETRY_DELAY_MS = 400L } } diff --git a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt index eecede8c..75ac1db5 100644 --- a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt +++ b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt @@ -36,6 +36,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.Color import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.input.ImeAction +import androidx.compose.ui.text.input.KeyboardCapitalization import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.unit.dp import androidx.hilt.navigation.compose.hiltViewModel @@ -125,7 +126,13 @@ fun RecoveryScreen( unfocusedLabelColor = SettingsMuted, cursorColor = SettingsPrimaryDeep ), - keyboardOptions = KeyboardOptions(imeAction = ImeAction.Done) + // The phrase is byte-exact key material and is always lowercase: let the keyboard + // neither capitalise the first word nor "correct" the wordlist out from under them. + keyboardOptions = KeyboardOptions( + imeAction = ImeAction.Done, + capitalization = KeyboardCapitalization.None, + autoCorrectEnabled = false + ) ) Spacer(Modifier.height(24.dp)) @@ -149,12 +156,20 @@ fun RecoveryScreen( Spacer(Modifier.height(16.dp)) - androidx.compose.material3.TextButton( + // A real button, not a text link. Most people arriving here are on a new phone and never + // saved the phrase, so this — not the field above — is their actual way through; it was + // styled as the weakest thing on the screen, under the one control they can't use. + // It stays *below* the field rather than replacing it: someone who does have the phrase + // (pasted from their partner's message) is unlocked instantly and offline, whereas this + // path has to wait on the partner tapping consent. + androidx.compose.material3.OutlinedButton( onClick = onPartnerRestore, - modifier = Modifier.fillMaxWidth() + enabled = !state.isLoading, + modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp), + shape = RoundedCornerShape(16.dp) ) { Text( - "No phrase? Ask your partner to restore this device", + "I don't have the phrase — ask my partner", style = MaterialTheme.typography.labelLarge, color = SettingsPrimaryDeep, textAlign = TextAlign.Center @@ -164,7 +179,7 @@ fun RecoveryScreen( Spacer(Modifier.height(12.dp)) Text( - "Your partner can restore your history for you — no phrase needed. They were also shown the same phrase and can reveal it any time in Settings → Security.", + "They just tap to approve — you don't need the phrase. (They can also read it to you: Settings → Security.)", style = MaterialTheme.typography.bodyMedium, color = SettingsInk, textAlign = TextAlign.Center diff --git a/app/src/main/java/app/closer/ui/pairing/RecoveryViewModel.kt b/app/src/main/java/app/closer/ui/pairing/RecoveryViewModel.kt index d52a4eff..c059ff0f 100644 --- a/app/src/main/java/app/closer/ui/pairing/RecoveryViewModel.kt +++ b/app/src/main/java/app/closer/ui/pairing/RecoveryViewModel.kt @@ -34,7 +34,10 @@ class RecoveryViewModel @Inject constructor( fun onPhraseChanged(phrase: String) = _uiState.update { it.copy(phrase = phrase, error = null) } fun recover() { - val phrase = _uiState.value.phrase.trim() + // Fold to the canonical form the phrase was generated in — a capitalised first word (which + // is exactly what a phone keyboard offers) is otherwise a different Argon2id input, and the + // user is told their correct phrase is wrong. + val phrase = RecoveryKeyManager.normalizePhrase(_uiState.value.phrase) if (phrase.isBlank()) { _uiState.update { it.copy(error = "Enter your recovery phrase.") } return diff --git a/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt new file mode 100644 index 00000000..7cab821d --- /dev/null +++ b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt @@ -0,0 +1,64 @@ +package app.closer.crypto + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * The recovery phrase is Argon2id key material, so it is byte-exact — and it is typed by a person + * on a phone, reading it off their partner's screen or hearing it down a phone line. Those two + * facts fight each other: a capitalised first word (what a keyboard offers by default) or a double + * space derives a different key and reports "That phrase doesn't match", which is a lie, on the one + * screen standing between someone and permanently unreadable history. + * + * These pin the fold to the canonical generated form. Every case here is a *correct* phrase that + * used to fail. + */ +class RecoveryPhraseNormalizationTest { + + private val canonical = "hunt down gear east lead over drop live more baby" + + private fun norm(raw: String) = RecoveryKeyManager.normalizePhrase(raw) + + @Test + fun canonicalPhraseIsUnchanged() { + assertEquals(canonical, norm(canonical)) + } + + @Test + fun keyboardAutoCapitalizationIsForgiven() { + // What a phone keyboard actually produces when you start typing. + assertEquals(canonical, norm("Hunt down gear east lead over drop live more baby")) + } + + @Test + fun shoutingIsForgiven() { + assertEquals(canonical, norm("HUNT DOWN GEAR EAST LEAD OVER DROP LIVE MORE BABY")) + assertEquals(canonical, norm("Hunt Down Gear East Lead Over Drop Live More Baby")) + } + + @Test + fun strayWhitespaceIsForgiven() { + // Double spaces, tabs and newlines: reading it aloud, or pasting from a message. + assertEquals(canonical, norm("hunt down gear east lead over drop live more baby")) + assertEquals(canonical, norm(" hunt down gear east lead over drop live more baby ")) + assertEquals(canonical, norm("hunt\tdown gear east lead over drop\nlive more baby")) + } + + @Test + fun pastedFromChatIsForgiven() { + assertEquals(canonical, norm("Hunt Down\tgear east\nlead over drop live more baby ")) + } + + @Test + fun aGenuinelyWrongPhraseStillDiffers() { + // Normalisation must not make wrong phrases pass: only the *presentation* is folded. + assert(norm("hunt down gear east lead over drop live more cat") != canonical) + assert(norm("hunt down gear east lead over drop live more") != canonical) + } + + @Test + fun blankStaysBlank() { + assertEquals("", norm("")) + assertEquals("", norm(" \n\t ")) + } +}