From b7e5031777475d4f89cfa2a7a3c9c7c2b4a8cf54 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 7 Sep 2026 23:08:04 +0200 Subject: [PATCH] sync: hand the password back on the retry path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 5dfbe0f discarded on start over, the back arrow and leaving the screen — but after a post-approval failure the screen shows the address field with an error and a *Continue* button, and there is no start over to press. So the retry the user actually makes ran a second login flow and overwrote the first password without handing it back, which is the leak the commit was written to close. Its test called onStartOver, an affordance that state never offers, so it passed against a path nobody can take. onServerSubmitted now discards, since it begins a fresh attempt. Three smaller ones from the same review. The revoke runs without a catch on a scope that has no exception handler, so anything escaping OkHttp outside AppPassword's own try would take the app down for a courtesy call whose failures are deliberately silent. An unparseable host counted as out-of-scope, and since a stranger is now fatal, an IPv6 literal — which OkHttp hands back unbracketed — would strand a homelab at [::1]; it brackets first and reads unparseable as in-scope, so the diagnostic fails quiet rather than into a dead end. And the one-shot 200 is recorded uncancellably: a cancellation delivered between the server deleting the flow row and our writing the credential down spends it with nothing left holding it. --- .../ui/accounts/AddAccountViewModel.kt | 55 ++++++++++++++----- .../ui/accounts/AddAccountViewModelTest.kt | 29 +++++++++- 2 files changed, 70 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt index f83702a..dc59e41 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt @@ -13,6 +13,8 @@ import de.jeanlucmakiola.caldav.ServiceDiscovery import de.jeanlucmakiola.caldav.TaskCollection import de.jeanlucmakiola.agendula.data.di.ApplicationScope import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow @@ -131,6 +133,11 @@ class AddAccountViewModel @Inject constructor( fun onServerSubmitted() { val input = (_state.value.step as? AddAccountStep.EnterServer)?.input?.trim().orEmpty() if (input.isEmpty()) return + // ⚠️ The retry path, and the one that actually leaks. A post-approval + // failure sends the user back here with an error, and the screen offers + // *Continue*, not start over — so a second attempt would mint a second + // password on top of the first without this. + discardMintedPassword() typedInput = input val quirk = ServerQuirk.forInput(input) @@ -242,15 +249,23 @@ class AddAccountViewModel @Inject constructor( delay(POLL_INTERVAL_MILLIS) when (val result = gateway.pollLoginFlow(flow)) { is NextcloudLoginFlow.PollResult.Approved -> { - username = result.credentials.loginName - appPassword = result.credentials.appPassword - serverRoot = result.credentials.server - // Ours to hand back until an account takes it on. - minted = CalDavGateway.Credentials( - username, - appPassword, - result.credentials.server, - ) + // ⚠️ Uncancellable. The 200 that carried this password is + // the only one there will ever be — the server deletes the + // flow row before answering — so a cancellation delivered + // between that answer and the assignment below spends the + // credential with nothing left holding it. Recording it is + // what makes handing it back possible. + withContext(NonCancellable) { + username = result.credentials.loginName + appPassword = result.credentials.appPassword + serverRoot = result.credentials.server + // Ours to hand back until an account takes it on. + minted = CalDavGateway.Credentials( + username, + appPassword, + result.credentials.server, + ) + } // The server contradicted itself about where it lives. // We recovered, but the setting behind it is the user's // to fix, so say so rather than silently coercing. @@ -371,10 +386,17 @@ class AddAccountViewModel @Inject constructor( } when (outcome) { is AccountRepository.Outcome.Created -> { - // Ownership passes to the account, which revokes on removal. + // Ownership passes to the account, which revokes on removal — + // but only if this is the password it was saved with. Every + // route from a minted password to a typed one already + // discards on the way, so the else branch should be dead; + // it is here because the failure it guards is silent and + // delayed. The account syncs once and then 401s for ever, + // long after anyone would connect it to adding it. + // // Cleared before the state update: an exception there must not // leave a saved account's own credential queued for revoking. - minted = null + if (minted?.password == appPassword) minted = null else discardMintedPassword() _state.update { it.copy(step = AddAccountStep.Done) } } @@ -429,7 +451,9 @@ class AddAccountViewModel @Inject constructor( private fun discardMintedPassword() { val credentials = minted ?: return minted = null - appScope.launch { gateway.revokeIssuedAppPassword(credentials) } + // The scope has no exception handler, so anything escaping here takes the + // app down — for a courtesy call whose failures are deliberately silent. + appScope.launch { runCatching { gateway.revokeIssuedAppPassword(credentials) } } } // ------------------------------------------------------------- internals @@ -520,7 +544,12 @@ class AddAccountViewModel @Inject constructor( val root = serverRoot ?: return null val scope = root.topPrivateDomain() ?: root.host return hosts.firstOrNull { host -> - val candidate = "https://$host".toHttpUrlOrNull() ?: return@firstOrNull true + // OkHttp hands back IPv6 literals unbracketed, which will not parse + // again. Unparseable reads as in-scope: the cost of a diagnostic that + // stays quiet is nothing, and the caller now treats out-of-scope as + // fatal — so failing the other way strands a homelab on [::1]. + val literal = if (':' in host) "[$host]" else host + val candidate = "https://$literal".toHttpUrlOrNull() ?: return@firstOrNull false !(candidate.topPrivateDomain() ?: candidate.host).equals(scope, ignoreCase = true) } } diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt index 135b228..e98424b 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt @@ -291,7 +291,15 @@ class AddAccountViewModelTest { ), ) - vm.onStartOver() + // ⚠️ The path the user actually has. After a post-approval failure + // the screen shows the address with an error and a *Continue* button + // — there is no start-over here, so a test that used one would pass + // against an affordance nobody can reach. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ) + vm.onServerSubmitted() advanceUntilIdle() // Nextcloud hands it over exactly once. Walking away leaves it valid @@ -345,6 +353,25 @@ class AddAccountViewModelTest { assertThat(gateway.revoked).isEmpty() } + @Test + fun `a typed password after an approval hands the minted one back`() = + runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.NeedsAuthentication(listOf("cloud.example.com")), + ) + // Retry, this time with no login flow on offer, so the user types + // their own password instead. + gateway.loginFlow = null + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + vm.onServerSubmitted() + advanceUntilIdle() + + // The retry itself hands back the first password. + assertThat(gateway.revoked.map { it.password }).containsExactly("app-pw") + } + @Test fun `a revoke that fails is never surfaced`() = runTest(dispatcher) { gateway.revokeSucceeds = false