makiolaj/agendula
1
0
Fork 0
Code Issues Pull Requests Actions Releases 7 Activity

sync: follow the redirects the JSON calls never followed, and keep partial fetches

Browse Source 
Every client here is built with followRedirects(false), because DavResource
requires it — so the DAV calls follow by hand and the two OCS/JSON ones
followed not at all. A Nextcloud that canonicalises host or path with a
301 (apex to 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 flow row deleted,
and the app-password revocation into the "attempted, and did nothing"
outcome its own KDoc exists to prevent. All three go through one
follower now, which re-issues the method and body — both routes answer
405 to a GET — bounds the hops, and applies the same downgrade rule
DavResource does.

The home set was resolved against the URL the principal PROPFIND was
aimed at rather than where it landed, though followRedirects rewrites
that location in place and probe() twenty lines above reads it back for
exactly this reason. A permanently moved principal answers a relative
href, which resolved against the old base names a path that 404s — and a
perfectly good account reports that it holds no calendars.

And a failing multiget batch discarded every batch before it: the
runCatching wrapped the whole loop rather than one request, so a 500 on
batch seven of ten threw away two hundred parsed resources and answered
Result.failure, and the collection re-downloaded everything next run. It
now keeps what it has and lets the unasked hrefs fall out as missing,
which the engine already counts rather than acting on. A run that got
nothing at all still fails.
This commit is contained in:
makiolaj
2026-09-09 12:11:12 +02:00
parent e4500e01b8
commit 1a0041f8fa
9 changed files with 260 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt
+6 -6
View File
@@ -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
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt
+10 -2
View File
@@ -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
// `<href>calendars/alice/</href>`, 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(
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt
+31 -9
View File
@@ -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<HttpUrl>, batchSize: Int): Result<FetchResult> =
runCatching {
val resources = mutableListOf<RemoteResource>()
val unsolicited = mutableListOf<HttpUrl>()
val failed = mutableListOf<FetchFailure>()
val seen = mutableSetOf<HttpUrl>()
internal fun fetch(hrefs: List<HttpUrl>, batchSize: Int): Result<FetchResult> {
val resources = mutableListOf<RemoteResource>()
val unsolicited = mutableListOf<HttpUrl>()
val failed = mutableListOf<FetchFailure>()
val seen = mutableSetOf<HttpUrl>()
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: *`.
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt
+2 -2
View File
@@ -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),
caldav/src/main/kotlin/de/jeanlucmakiola/caldav/Redirects.kt
+67
View File
@@ -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}")
}
}
caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt
+20
View File
@@ -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.
caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt
+24
View File
@@ -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/", "<collection/>")))
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
caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt
+36
View File
@@ -368,6 +368,42 @@ END:VCALENDAR</C:calendar-data>
assertThat(server.requestCount).isEqualTo(2)
}
@Test fun `a batch that fails keeps the batches that already worked`() {
server.enqueue(
multistatus(
"""
<response>
<href>/dav/tasks/1.ics</href>
<propstat><prop>
<getetag>"e1"</getetag>
<C:calendar-data xmlns:C="urn:ietf:params:xml:ns:caldav">BEGIN:VCALENDAR
END:VCALENDAR</C:calendar-data>
</prop><status>HTTP/1.1 200 OK</status></propstat>
</response>
""",
),
)
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`() {
caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt
+64
View File
@@ -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()