diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt index 4fd00a7..8be9da7 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt @@ -70,9 +70,9 @@ object AppPassword { * authenticator's retry, plus up to 120s of read timeout. Minutes, for a * step whose own doc says it must not block the removal. * - * Not set on the shared client on purpose: `callTimeout` bounds the whole - * exchange including the body, and a multiget of a large list over a slow - * link legitimately runs long. + * The shared client has a ceiling of its own, but it is sized for a + * multiget of a full batch over a slow link — minutes, where a courtesy + * revocation the user is waiting behind gets seconds. * * @return true when the server confirmed the revocation. False means the * credential may still exist server-side — the caller carries on regardless. diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index 93c750a..0ff1ad1 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -4,6 +4,10 @@ import at.bitfire.dav4jvm.BasicDigestAuthHandler import okhttp3.HttpUrl import okhttp3.Interceptor import okhttp3.OkHttpClient +import okhttp3.Response +import okhttp3.ResponseBody.Companion.asResponseBody +import okio.GzipSource +import okio.buffer import java.util.concurrent.TimeUnit /** @@ -30,6 +34,13 @@ object CalDavHttp { .connectTimeout(30, TimeUnit.SECONDS) .readTimeout(120, TimeUnit.SECONDS) .writeTimeout(120, TimeUnit.SECONDS) + // ⚠️ A whole-call ceiling, because the three above are per-attempt: + // a server trickling one byte every 119 seconds satisfies the read + // timeout for ever, and holds a sequential sync for the entire + // WorkManager window while every later collection is skipped. Wide + // enough for a large multiget on a slow homelab link, narrow enough + // that one stalled request cannot eat the run. + .callTimeout(3, TimeUnit.MINUTES) .build() } @@ -53,7 +64,11 @@ object CalDavHttp { * set needs: iCloud puts the principal on `caldav.icloud.com` and the home * set on `pNN-caldav.icloud.com`, and an exact-host allowlist refuses the * second one. - * - It caches which scheme worked, so the challenge is paid once. + * - **It caches which scheme worked for the lifetime of the client**, so a + * sync that reuses one client pays the 401 challenge once. ⚠️ The cache + * lives on the handler, and this builds a fresh one per call — `discover`, + * `revokeAppPassword` and `sync` each get their own — so a Digest-only + * server pays one challenge per client, not one per process. */ fun authenticated( userAgent: String, @@ -122,9 +137,40 @@ object CalDavHttp { private val calendarHeaders = Interceptor { chain -> val request = chain.request() val builder = request.newBuilder().header("Accept-Encoding", "identity") - if (request.method in WRITE_METHODS) builder.header("Prefer", "handling=strict") - chain.proceed(builder.build()) + if (request.method in WRITE_METHODS) { + // ⚠️ Appended, not replaced. A caller that set its own `Prefer` — + // `return=minimal`, a `depth-noroot` — would otherwise have it + // silently dropped on every write. + val existing = request.header("Prefer")?.takeIf { it.isNotBlank() } + builder.header("Prefer", listOfNotNull(existing, STRICT_HANDLING).joinToString(", ")) + } + gunzip(chain.proceed(builder.build())) + } + + /** + * Undoes a compression we asked not to have. + * + * Setting `Accept-Encoding` by hand takes OkHttp's transparent gunzip out of + * the loop — it only decompresses what it asked for itself. ⚠️ A proxy that + * gzips regardless (a misconfigured nginx, an ISP middlebox) then hands raw + * deflate bytes to the iCalendar parser, which reports it as an unreadable + * body and quarantines a perfectly good resource. Honouring the header the + * response actually carries costs nothing when nobody compressed anything. + */ + private fun gunzip(response: Response): Response { + if (!"gzip".equals(response.header("Content-Encoding"), ignoreCase = true)) return response + val body = response.body ?: return response + val source = GzipSource(body.source()).buffer() + return response.newBuilder() + .removeHeader("Content-Encoding") + // Dropped with it: the length describes the compressed bytes and is + // a lie about what the caller now reads. + .removeHeader("Content-Length") + .body(source.asResponseBody(body.contentType(), contentLength = -1)) + .build() } private val WRITE_METHODS = setOf("PUT", "POST", "PATCH") + + private const val STRICT_HANDLING = "handling=strict" } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index ba0ae42..8cf9297 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -136,7 +136,15 @@ class CalendarCollection( response.isSuccess() -> changed += RemoteRef(response.href, ETag.from(response[GetETag::class.java])) - else -> Unit + // ⚠️ Neither a success nor a removal — a per-object ACL + // answering 403, a server failing on its own object with 5xx. + // Dropping it reports the resource as *unchanged*, so the row + // keeps whatever it holds until the next full reconciliation + // notices the ETag differs. Carried as changed with no + // validator instead: the multiget then produces a real verdict + // for it, and `fetch` already grades one — the same reasoning + // error, and the same fix, as its twin next door. + else -> changed += RemoteRef(response.href, eTag = null) } } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt index 7450ca9..d25125d 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt @@ -117,6 +117,16 @@ object ServiceDiscovery { // DNS names are case-insensitive and dnsjava returns the wire case, so // "Calendar.Google.com." would otherwise walk straight past the guard. .filterNot { it.target.trimEnd('.').lowercase() in SRV_DEAD_ENDS } + // ⚠️ RFC 2782 does not require a target to live inside the domain it + // was queried for, and this ladder is walked over plain UDP DNS with + // no DNSSEC — then walked *again* after a 401, with the + // authenticated client. An out-of-domain target is also one the + // credential would never be offered to, since CalDavHttp scopes it + // to the typed address's registrable domain, so following it can + // only end in a 401 the user cannot act on. Refusing leaves the + // well-known ladder on the typed domain, which is where a correctly + // delegated install answers anyway. + .filter { sharesDomain(it.target, domain) } .sortedWith(compareBy({ it.priority }, { -it.weight })) // The TXT path applies to the SRV target, and to the bare domain when @@ -149,6 +159,24 @@ object ServiceDiscovery { return candidates } + /** + * Whether an SRV target may stand in for [domain] — the same registrable + * domain, or the exact host where there is none (an IP literal, a + * single-label name, a host that *is* a public suffix). + */ + private fun sharesDomain(target: String, domain: String): Boolean { + val host = target.trimEnd('.').lowercase() + val scope = scopeOf(domain) ?: return host.equals(domain, ignoreCase = true) + return scopeOf(host)?.equals(scope, ignoreCase = true) == true + } + + /** The boundary `CalDavHttp` scopes a credential by, derived the same way. */ + private fun scopeOf(host: String): String? { + val literal = if (':' in host) "[$host]" else host + val url = "https://$literal".toHttpUrlOrNull() ?: return null + return url.topPrivateDomain() ?: url.host + } + private fun add(into: MutableList, origin: String, path: String, label: String) { val url = (origin.trimEnd('/') + "/" + path.trimStart('/')).toHttpUrlOrNull() ?: return if (into.none { it.url == url }) into += Candidate(url, label) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt index eb362a8..95d8398 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt @@ -203,11 +203,14 @@ class CalDavHttpTest { .build() @Test - fun `the shared client has no call timeout`() { - // ⚠️ Deliberate. A callTimeout bounds the whole exchange including the - // body, and a multiget of a large list over a slow link legitimately runs - // long. The one call that needs a budget sets its own. - assertThat(CalDavHttp.anonymous("Agendula").callTimeoutMillis).isEqualTo(0) + fun `the shared client bounds a whole call, not just each read`() { + // ⚠️ readTimeout is per-read, so a server trickling a byte every 119 + // seconds satisfies it for ever and holds a sequential sync for the + // entire WorkManager window. The ceiling is wide enough for a multiget + // of a full batch over a slow homelab link and far below that window; + // the revocation, which must return in seconds, still sets its own. + assertThat(CalDavHttp.anonymous("Agendula").callTimeoutMillis) + .isEqualTo(3 * 60 * 1000) } @Test diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarChangesTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarChangesTest.kt index 94cd604..4ad1871 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarChangesTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarChangesTest.kt @@ -51,6 +51,30 @@ class CalendarChangesTest { assertThat(page.truncated).isFalse() } + @Test fun `a status that is neither success nor removal is carried as a change`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/refused.ics + HTTP/1.1 403 Forbidden + + urn:x:2 + """, + ), + ) + + val page = collection.changes("urn:x:1") as ChangeSet.Page + + // Dropping it would report the resource as unchanged and leave the row + // stale until the next full reconciliation. With no validator on it the + // multiget is forced, and that is what grades the refusal. + assertThat(page.changed.map { it.href.encodedPath }) + .containsExactly("/dav/tasks/refused.ics") + assertThat(page.changed.single().eTag).isNull() + assertThat(page.removed).isEmpty() + } + @Test fun `no DAV limit is ever sent`() { server.enqueue(multistatus("urn:x:2")) collection.changes(null) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt index 583afc6..73e6a75 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -5,7 +5,9 @@ import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse +import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.mockwebserver.MockWebServer +import okio.buffer import org.junit.After import org.junit.Before import org.junit.Test @@ -480,6 +482,43 @@ END:VCALENDAR assertThat(request.getHeader("Accept-Encoding")).isEqualTo("identity") } + @Test fun `a caller's own Prefer survives beside ours`() { + val client = CalDavHttp.anonymous("Agendula test") + server.enqueue(MockResponse().setResponseCode(200)) + + client.newCall( + okhttp3.Request.Builder() + .url(server.url("/dav/tasks/one.ics")) + .header("Prefer", "return=minimal") + .put("x".toRequestBody()) + .build(), + ).execute().close() + + // Replacing it would silently drop whatever the caller asked for. + assertThat(server.takeRequest().getHeader("Prefer")) + .isEqualTo("return=minimal, handling=strict") + } + + @Test fun `a body gzipped despite identity is still readable`() { + val client = CalDavHttp.anonymous("Agendula test") + val body = okio.Buffer() + okio.GzipSink(body).buffer().use { it.writeUtf8("BEGIN:VCALENDAR") } + server.enqueue( + MockResponse().setResponseCode(200) + .setHeader("Content-Encoding", "gzip") + .setBody(body), + ) + + val response = client.newCall( + okhttp3.Request.Builder().url(server.url("/dav/tasks/one.ics")).build(), + ).execute() + + // ⚠️ Asking for identity takes OkHttp's transparent gunzip out of the + // loop, so a proxy that compresses anyway hands raw deflate to the + // iCalendar parser and a good resource is quarantined as unreadable. + assertThat(response.use { it.body!!.string() }).isEqualTo("BEGIN:VCALENDAR") + } + // ----------------------------------------------------------------- state @Test fun `state re-reads read-only and shared`() { diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt index c9a582e..0a18c19 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt @@ -92,6 +92,35 @@ class ServiceDiscoveryTest { assertThat(urls("me@gmail.com", dns)).doesNotContain("https://calendar.google.com/") } + @Test + fun `an SRV target outside the queried domain is refused`() { + // Plain UDP DNS, no DNSSEC, and the ladder is walked again after a 401 + // with the authenticated client. The credential is scoped to the typed + // address's registrable domain anyway, so this target could only ever + // answer 401. + val dns = FakeDns( + srv = mapOf( + "_caldavs._tcp.example.com" to listOf(SrvRecord(0, 0, 443, "dav.attacker.test.")), + ), + ) + val candidates = urls("me@example.com", dns) + + assertThat(candidates).doesNotContain("https://dav.attacker.test/.well-known/caldav") + // The typed domain's own ladder still runs. + assertThat(candidates).contains("https://example.com/.well-known/caldav") + } + + @Test + fun `a subdomain SRV target is still followed`() { + val dns = FakeDns( + srv = mapOf( + "_caldavs._tcp.example.com" to listOf(SrvRecord(0, 0, 443, "caldav.example.com.")), + ), + ) + assertThat(urls("me@example.com", dns)) + .contains("https://caldav.example.com/.well-known/caldav") + } + @Test fun `SRV priority wins, then weight`() { val dns = FakeDns(