diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt index 8be9da7..19096a2 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt @@ -99,11 +99,11 @@ object AppPassword { timeout: Duration = REVOCATION_TIMEOUT, ): Boolean = try { val url = ocsRoot.newBuilder().addPathSegments(PATH).build() - httpClient.newBuilder() - // Shares the pool and dispatcher, so this costs nothing. - .callTimeout(timeout.toJavaDuration()) - .build() - .newCall( + Redirects.follow( + httpClient.newBuilder() + // Shares the pool and dispatcher, so this costs nothing. + .callTimeout(timeout.toJavaDuration()) + .build(), Request.Builder() .url(url) .delete() @@ -113,7 +113,7 @@ object AppPassword { .header("OCS-APIRequest", "true") .header("Accept", "application/json") .build(), - ).execute().use { it.isSuccessful } + ).use { it.isSuccessful } } catch (_: IOException) { // callTimeout throws InterruptedIOException, which lands here. false diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 3bb6a26..eae2321 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -245,8 +245,16 @@ class CalDavDiscovery( // ⚠️ Iterate ALL hrefs. Multiple home sets are normative (RFC 4791 // §6.2.1's own example) and iCloud depends on it: the principal is // on caldav.icloud.com and the home set on pNN-caldav.icloud.com. + // ⚠️ Against where the PROPFIND *landed*, not where it was + // aimed. `followRedirects` rewrites `location` in place, and the + // probe above already reads it back for exactly this reason. A + // principal that has permanently moved answers a relative + // `calendars/alice/`, which resolved against the + // pre-redirect URL names a path on the old host — the home-set + // PROPFIND 404s and a perfectly good account reports that it + // holds no calendars. response[CalendarHomeSet::class.java]?.hrefs?.forEach { raw -> - principal.resolve(raw)?.let(homeSets::add) + principalResource.location.resolve(raw)?.let(homeSets::add) } } } @@ -262,7 +270,7 @@ class CalDavDiscovery( // Cross-host is legal and required, but never over plain HTTP: the // credentials follow the home set, and a downgrade would send them in the // clear. The caller surfaces the host change to the user. - val crossHost = homeSets.filter { it.host != principal.host } + val crossHost = homeSets.filter { it.host != principalResource.location.host } val insecure = homeSets.filter { principal.isHttps && !it.isHttps } if (insecure.isNotEmpty()) { return Outcome.Failed( diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index 8cf9297..116322e 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -225,14 +225,21 @@ class CalendarCollection( answered.host.equals(collection.host, ignoreCase = true) /** [batchSize] is a seam for tests; production always uses [MULTIGET_BATCH]. */ - internal fun fetch(hrefs: List, batchSize: Int): Result = - runCatching { - val resources = mutableListOf() - val unsolicited = mutableListOf() - val failed = mutableListOf() - val seen = mutableSetOf() + internal fun fetch(hrefs: List, batchSize: Int): Result { + val resources = mutableListOf() + val unsolicited = mutableListOf() + val failed = mutableListOf() + val seen = mutableSetOf() - hrefs.chunked(batchSize).forEach { batch -> + // ⚠️ Per batch, not around the whole loop. A 500 on batch seven of ten + // used to discard the two hundred resources already parsed and answer + // `Result.failure`, so the caller recorded a collection failure and + // re-downloaded everything next run — on a flaky link, for ever. A batch + // that could not be asked is exactly what `missing` means, and the + // per-resource verdicts the engine already applies say the rest. + val batches = hrefs.chunked(batchSize) + for (batch in batches) { + val attempt = runCatching { val byExactPath = batch.associateBy { it.encodedPath } // ⚠️ Only identities that name exactly one request href. Two // spellings of one path — `/my%40dav.ics` and `/my@dav.ics` — @@ -282,7 +289,21 @@ class CalendarCollection( ) } } + // ⚠️ The first batch to fail ends the fetch, but keeps what came + // before it. Carrying on would re-ask a server that has just said it + // cannot answer, and the hrefs left unasked fall out as `missing` — + // which the engine grades as "listed but not returned" and counts, + // rather than acting on as a deletion. A whole run that fails on the + // first batch still answers `failure`, exactly as before. + if (attempt.isFailure) { + if (resources.isEmpty() && failed.isEmpty()) { + return Result.failure(attempt.exceptionOrNull()!!) + } + break + } + } + return Result.success( FetchResult( resources = resources, missing = hrefs.filterNot { it in seen }, @@ -293,8 +314,9 @@ class CalendarCollection( .filterNot { failure -> resources.any { it.href == failure.href } } .distinctBy { it.href }, unsolicited = unsolicited, - ) - } + ), + ) + } /** * Creates a resource at [name] with `If-None-Match: *`. diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt index a6988ab..eb66279 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt @@ -105,7 +105,7 @@ class NextcloudLoginFlow( .post(FormBody.Builder().build()) .build() - httpClient.newCall(request).execute().use { response -> + Redirects.follow(httpClient, request).use { response -> if (!response.isSuccessful) error("login flow init failed: HTTP ${response.code}") val body = response.body?.string().orEmpty() val root = json.parseToJsonElement(body).jsonObject @@ -155,7 +155,7 @@ class NextcloudLoginFlow( .build() return runCatching { - httpClient.newCall(request).execute().use { response -> + Redirects.follow(httpClient, request).use { response -> when { // ⚠️ 404 means pending, and *only* 404 does. "Anything that // isn't 200 is pending" swallows 429 (brute-force protection), diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/Redirects.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/Redirects.kt new file mode 100644 index 0000000..ff761dc --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/Redirects.kt @@ -0,0 +1,67 @@ +package de.jeanlucmakiola.caldav + +import okhttp3.HttpUrl +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response +import java.io.IOException + +/** + * Redirect following for the plain-HTTP corners of this module. + * + * ⚠️ Every client here is built with `followRedirects(false)`, because + * `DavResource` requires it and asserts on it — so the DAV calls follow by hand + * and the two OCS/JSON ones did not follow at all. A Nextcloud that + * canonicalises host or path with a 301 (apex→www, a trailing slash, a proxy) + * therefore turned the login flow's `start` into a bare failure, its `poll` into + * an unexplained SERVER_ERROR *after* the password was minted, and the app + * password revocation into the "attempted, and did nothing" outcome its own + * KDoc exists to prevent. + * + * ⚠️ The method and body are **re-issued as they were**, which is 307/308 + * behaviour rather than the classic 301/302 POST→GET rewrite. Both endpoints + * answer 405 to a GET, so rewriting the method would turn one silent failure + * into another; and the bodies here are in-memory forms, so replaying costs + * nothing. + */ +internal object Redirects { + + /** The same ceiling `DavResource` uses, for the same reason. */ + const val MAX_HOPS = 5 + + @Throws(IOException::class) + fun follow(client: OkHttpClient, request: Request): Response { + var current = request + repeat(MAX_HOPS) { + val response = client.newCall(current).execute() + if (!response.isRedirect) return response + val target = response.header("Location")?.let { current.url.resolve(it) } + response.close() + if (target == null) throw IOException("redirect with no usable Location from ${current.url}") + current = current.newBuilder().url(secure(current.url, target)).build() + } + throw IOException("more than $MAX_HOPS redirects from ${request.url}") + } + + /** + * The downgrade rule `DavResource` already applies, stated once more here. + * + * A downgrade to the *same host* is the single most common misconfiguration + * in this space — a Nextcloud behind a TLS-terminating proxy with no + * `overwriteprotocol` builds every redirect with `http://` — and re-issuing + * over TLS is strictly safer than what we were asked to do. A downgrade to a + * different host has no innocent reading. + * + * ⚠️ The port goes with the scheme: OkHttp only drops a *default* port + * across a scheme change, so a redirect to `http://host:8080` would + * otherwise be re-issued as `https://host:8080`, which almost certainly + * speaks cleartext. The port we were already talking to is the one known to + * answer. + */ + private fun secure(from: HttpUrl, to: HttpUrl): HttpUrl = when { + !from.isHttps || to.isHttps -> to + to.host.equals(from.host, ignoreCase = true) -> + to.newBuilder().scheme("https").port(from.port).build() + else -> throw IOException("refusing a redirect from HTTPS to HTTP at ${to.host}") + } +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt index 74d8254..54bbb2e 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt @@ -22,6 +22,26 @@ class AppPasswordTest { @After fun stop() = server.shutdown() + @Test fun `a redirected revocation is followed`() { + // Production builds its clients with followRedirects(false), so a server + // that canonicalises host or path leaves the password dangling — + // attempted, and doing nothing. + val noRedirects = OkHttpClient.Builder().followRedirects(false).build() + server.enqueue( + MockResponse().setResponseCode(301) + .setHeader("Location", server.url("/nc/ocs/v2.php/core/apppassword").toString()), + ) + server.enqueue(MockResponse().setResponseCode(200)) + + val revoked = AppPassword.revokeAt(noRedirects, server.url("/")) + + assertThat(revoked).isTrue() + server.takeRequest() + val followed = server.takeRequest() + assertThat(followed.method).isEqualTo("DELETE") + assertThat(followed.getHeader("OCS-APIRequest")).isEqualTo("true") + } + @Test fun `the OCS root is derived from a principal, not appended to it`() { // ⚠️ Appending to the principal produces a path that 404s on every // server, silently — the revocation looks attempted and does nothing. diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt index c26acce..65c0e6c 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt @@ -97,6 +97,30 @@ class CalDavDiscoveryTest { // ------------------------------------------------------------ principal + @Test + fun `a home set is resolved against where the principal actually landed`() { + // ⚠️ followRedirects rewrites the resource's location in place, and a + // relative home-set href means "relative to the answer". Resolved + // against the URL we aimed at, a permanently moved principal names a + // path under the old prefix: the home-set PROPFIND 404s and a perfectly + // good account reports that it holds no calendars. + server.enqueue( + MockResponse().setResponseCode(301) + .setHeader("Location", server.url("/nc/principals/me/").toString()), + ) + server.enqueue(homeSetResponse("calendars/me/")) + server.enqueue(listing(collection("/nc/principals/me/calendars/me/tasks/", ""))) + + val outcome = discovery.fromPrincipal(server.url("/principals/me/")) + + assertThat(outcome).isInstanceOf(CalDavDiscovery.Outcome.Found::class.java) + // Under the prefix the redirect landed on, not the one we aimed at — + // which would have been /principals/me/calendars/me/, a 404. + assertThat((outcome as CalDavDiscovery.Outcome.Found).homeSets.single().encodedPath) + .isEqualTo("/nc/principals/me/calendars/me/") + } + + @Test fun `a 200 carrying unauthenticated is a failed login, not an empty result`() { // RFC 5397 section 3. Without this check a rejected credential looks like diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt index 73e6a75..5f53406 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -368,6 +368,42 @@ END:VCALENDAR assertThat(server.requestCount).isEqualTo(2) } + @Test fun `a batch that fails keeps the batches that already worked`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/1.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + server.enqueue(MockResponse().setResponseCode(500)) + + val result = collection.fetch((1..3).map { href("$it.ics") }, batchSize = 1).getOrThrow() + + // ⚠️ Wrapping the whole loop threw away everything parsed so far and + // answered failure, so the engine recorded a collection failure and + // re-downloaded the lot next run — on a flaky link, for ever. + assertThat(result.resources.map { it.href.encodedPath }) + .containsExactly("/dav/tasks/1.ics") + // The rest were never asked, which is exactly what missing means; the + // engine counts those rather than acting on them as deletions. + assertThat(result.missing.map { it.encodedPath }) + .containsExactly("/dav/tasks/2.ics", "/dav/tasks/3.ics") + } + + @Test fun `a fetch that never got anything still fails`() { + server.enqueue(MockResponse().setResponseCode(500)) + + assertThat(collection.fetch(listOf(href("one.ics"))).isFailure).isTrue() + } + // ---------------------------------------------------------------- create @Test fun `create with a strong etag is stored`() { diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt index f4a99d1..4ef8058 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt @@ -38,6 +38,70 @@ class NextcloudLoginFlowTest { return flow.start(server.url("/"), now = 0).getOrThrow() } + /** As production builds it: DavResource requires no automatic redirects. */ + private fun noRedirectFlow() = NextcloudLoginFlow( + OkHttpClient.Builder().followRedirects(false).build(), + userAgent = "Agendula/1.0 (Pixel 8)", + ) + + @Test + fun `a redirected init is followed, and stays a POST`() { + // ⚠️ Apex to www, a trailing slash, a proxy — Nextcloud canonicalises + // with a 301 and the client cannot follow on its own. The method has to + // survive: this route answers 405 to a GET. + val redirecting = noRedirectFlow() + server.enqueue( + MockResponse().setResponseCode(301) + .setHeader("Location", server.url("/nc/index.php/login/v2").toString()), + ) + server.enqueue( + json( + """ + {"poll":{"token":"tok-123","endpoint":"${server.url("/nc/index.php/login/v2/poll")}"}, + "login":"${server.url("/nc/index.php/login/v2/flow/abc")}"} + """.trimIndent(), + ), + ) + + val started = redirecting.start(server.url("/"), now = 0).getOrThrow() + + assertThat(started.pollToken).isEqualTo("tok-123") + server.takeRequest() + val followed = server.takeRequest() + assertThat(followed.path).isEqualTo("/nc/index.php/login/v2") + assertThat(followed.method).isEqualTo("POST") + } + + @Test + fun `a redirected poll is followed rather than read as a server error`() { + // ⚠️ After approval. Reporting SERVER_ERROR here abandons a password the + // server has already minted and can never hand back again. + val redirecting = noRedirectFlow() + val started = NextcloudLoginFlow.Flow( + loginUrl = server.url("/index.php/login/v2/flow/abc"), + pollEndpoint = server.url("/index.php/login/v2/poll"), + pollToken = "tok-123", + deadlineEpochSeconds = 1_200, + ) + server.enqueue( + MockResponse().setResponseCode(308) + .setHeader("Location", server.url("/nc/login/v2/poll").toString()), + ) + server.enqueue( + json("""{"server":"${server.url("/")}","loginName":"me","appPassword":"secret-app-pw"}"""), + ) + + val result = redirecting.poll(started, now = 1) + + assertThat(result).isInstanceOf(NextcloudLoginFlow.PollResult.Approved::class.java) + assertThat((result as NextcloudLoginFlow.PollResult.Approved).credentials.appPassword) + .isEqualTo("secret-app-pw") + server.takeRequest() + val followed = server.takeRequest() + assertThat(followed.method).isEqualTo("POST") + assertThat(followed.body.readUtf8()).contains("token=tok-123") + } + @Test fun `init posts, and carries a User-Agent the user can recognise`() { startFlow()