sync: close four holes in the transport and the change log
changes() dropped every per-resource status that was neither 2xx nor 404/410, so a per-object ACL answering 403 read as "unchanged" and the row kept whatever it held until the next full reconciliation noticed the ETag differed. The same reasoning error as fetch's, and the same fix shape now exists next door: it is carried as a change with no validator, which forces the multiget, and the multiget already grades a refusal. An SRV target is not required 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. Since the credential is scoped to the typed address's registrable domain, an out-of-domain target could only ever answer 401 anyway. Refusing it leaves the well-known ladder on the typed domain, which is where a correctly delegated install answers. CalDavHttp, three of them. Accept-Encoding: identity takes OkHttp's transparent gunzip out of the loop, so a proxy that gzips regardless handed raw deflate to the iCalendar parser and a good resource was quarantined as unreadable; the header the response carries is now honoured. Prefer was set rather than appended, so a caller's own preference was silently dropped on every write. And there was no call-level ceiling: readTimeout is per-read, so a server trickling a byte every 119 seconds held a sequential sync for the whole WorkManager window and every later collection was skipped. Three minutes is well above a full multiget batch on a slow link and well below the window — c2166b9's note said the opposite, and the revocation still sets its own budget because it needs seconds, not minutes. The KDoc claimed the handler caches which scheme worked "so the challenge is paid once", while authenticated() builds a fresh handler per call. It says what it actually does now.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<Candidate>, origin: String, path: String, label: String) {
|
||||
val url = (origin.trimEnd('/') + "/" + path.trimStart('/')).toHttpUrlOrNull() ?: return
|
||||
if (into.none { it.url == url }) into += Candidate(url, label)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
"""
|
||||
<response>
|
||||
<href>/dav/tasks/refused.ics</href>
|
||||
<status>HTTP/1.1 403 Forbidden</status>
|
||||
</response>
|
||||
<sync-token>urn:x:2</sync-token>
|
||||
""",
|
||||
),
|
||||
)
|
||||
|
||||
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("<sync-token>urn:x:2</sync-token>"))
|
||||
collection.changes(null)
|
||||
|
||||
@@ -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</C:calendar-data>
|
||||
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`() {
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user