fix(recovery): stop the two ways a returning user loses their history
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 <noreply@anthropic.com>
This commit is contained in:
parent
5151698d56
commit
7ab80013df
|
|
@ -123,6 +123,23 @@ class RecoveryKeyManager @Inject constructor() {
|
||||||
}
|
}
|
||||||
|
|
||||||
companion object {
|
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 PHRASE_WORD_COUNT = 10
|
||||||
private const val SALT_BYTES = 16
|
private const val SALT_BYTES = 16
|
||||||
private const val KEY_BYTES = 32
|
private const val KEY_BYTES = 32
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel
|
||||||
import kotlinx.coroutines.flow.MutableStateFlow
|
import kotlinx.coroutines.flow.MutableStateFlow
|
||||||
import kotlinx.coroutines.flow.StateFlow
|
import kotlinx.coroutines.flow.StateFlow
|
||||||
import kotlinx.coroutines.flow.asStateFlow
|
import kotlinx.coroutines.flow.asStateFlow
|
||||||
|
import kotlinx.coroutines.delay
|
||||||
import kotlinx.coroutines.flow.update
|
import kotlinx.coroutines.flow.update
|
||||||
import kotlinx.coroutines.launch
|
import kotlinx.coroutines.launch
|
||||||
import javax.inject.Inject
|
import javax.inject.Inject
|
||||||
|
|
@ -35,13 +36,8 @@ class OnboardingViewModel @Inject constructor(
|
||||||
is AuthState.Loading -> _uiState.update { it.copy(isCheckingAuth = true) }
|
is AuthState.Loading -> _uiState.update { it.copy(isCheckingAuth = true) }
|
||||||
is AuthState.Unauthenticated -> _uiState.update { it.copy(isCheckingAuth = false, navigateTo = null) }
|
is AuthState.Unauthenticated -> _uiState.update { it.copy(isCheckingAuth = false, navigateTo = null) }
|
||||||
is AuthState.Authenticated -> {
|
is AuthState.Authenticated -> {
|
||||||
val user = runCatching { userRepository.getUser(authState.userId) }
|
_uiState.update { it.copy(isCheckingAuth = true) }
|
||||||
.onFailure { Log.w(TAG, "Could not load user profile during onboarding", it) }
|
val destination = resolveDestination(authState.userId)
|
||||||
.getOrNull()
|
|
||||||
val destination = when {
|
|
||||||
user == null || user.displayName.isBlank() || user.sex.isBlank() -> "create_profile"
|
|
||||||
else -> "home"
|
|
||||||
}
|
|
||||||
_uiState.update { it.copy(isCheckingAuth = false, navigateTo = destination) }
|
_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) }
|
fun onNavigated() = _uiState.update { it.copy(navigateTo = null) }
|
||||||
|
|
||||||
companion object {
|
companion object {
|
||||||
private const val TAG = "OnboardingViewModel"
|
private const val TAG = "OnboardingViewModel"
|
||||||
|
private const val PROFILE_READ_ATTEMPTS = 3
|
||||||
|
private const val RETRY_DELAY_MS = 400L
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -36,6 +36,7 @@ import androidx.compose.ui.Modifier
|
||||||
import androidx.compose.ui.graphics.Color
|
import androidx.compose.ui.graphics.Color
|
||||||
import androidx.compose.ui.text.font.FontWeight
|
import androidx.compose.ui.text.font.FontWeight
|
||||||
import androidx.compose.ui.text.input.ImeAction
|
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.text.style.TextAlign
|
||||||
import androidx.compose.ui.unit.dp
|
import androidx.compose.ui.unit.dp
|
||||||
import androidx.hilt.navigation.compose.hiltViewModel
|
import androidx.hilt.navigation.compose.hiltViewModel
|
||||||
|
|
@ -125,7 +126,13 @@ fun RecoveryScreen(
|
||||||
unfocusedLabelColor = SettingsMuted,
|
unfocusedLabelColor = SettingsMuted,
|
||||||
cursorColor = SettingsPrimaryDeep
|
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))
|
Spacer(Modifier.height(24.dp))
|
||||||
|
|
@ -149,12 +156,20 @@ fun RecoveryScreen(
|
||||||
|
|
||||||
Spacer(Modifier.height(16.dp))
|
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,
|
onClick = onPartnerRestore,
|
||||||
modifier = Modifier.fillMaxWidth()
|
enabled = !state.isLoading,
|
||||||
|
modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp),
|
||||||
|
shape = RoundedCornerShape(16.dp)
|
||||||
) {
|
) {
|
||||||
Text(
|
Text(
|
||||||
"No phrase? Ask your partner to restore this device",
|
"I don't have the phrase — ask my partner",
|
||||||
style = MaterialTheme.typography.labelLarge,
|
style = MaterialTheme.typography.labelLarge,
|
||||||
color = SettingsPrimaryDeep,
|
color = SettingsPrimaryDeep,
|
||||||
textAlign = TextAlign.Center
|
textAlign = TextAlign.Center
|
||||||
|
|
@ -164,7 +179,7 @@ fun RecoveryScreen(
|
||||||
Spacer(Modifier.height(12.dp))
|
Spacer(Modifier.height(12.dp))
|
||||||
|
|
||||||
Text(
|
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,
|
style = MaterialTheme.typography.bodyMedium,
|
||||||
color = SettingsInk,
|
color = SettingsInk,
|
||||||
textAlign = TextAlign.Center
|
textAlign = TextAlign.Center
|
||||||
|
|
|
||||||
|
|
@ -34,7 +34,10 @@ class RecoveryViewModel @Inject constructor(
|
||||||
fun onPhraseChanged(phrase: String) = _uiState.update { it.copy(phrase = phrase, error = null) }
|
fun onPhraseChanged(phrase: String) = _uiState.update { it.copy(phrase = phrase, error = null) }
|
||||||
|
|
||||||
fun recover() {
|
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()) {
|
if (phrase.isBlank()) {
|
||||||
_uiState.update { it.copy(error = "Enter your recovery phrase.") }
|
_uiState.update { it.copy(error = "Enter your recovery phrase.") }
|
||||||
return
|
return
|
||||||
|
|
|
||||||
|
|
@ -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 "))
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in New Issue