Files
agendula/dav/PROVENANCE.md
T
makiolaj 80133d5cc8 sync: three defects in the vendored Digest handler
The Basic arm returned out of the challenge loop as soon as it found a
Basic challenge it had already tried. The handler is a network
interceptor, so Basic is always primed preemptively over HTTPS — the
abort therefore fired on the first 401, and a Digest challenge later in
the same header was never read. A Baikal or Apache front end offering
both and rejecting Basic at the app layer got no Digest answer at all,
and the account was marked as needing sign-in for good. Every challenge
is read before anything is decided now; giving up still happens, just
after the whole header has been looked at.

clientNonce and nonceCount sat on the companion object while SyncEngine
builds one handler per account, so two accounts syncing at once
interleaved their nc values. They are instance state now, and a new
server nonce restarts the count — RFC 7616 3.4.1 counts requests sent
with that nonce, starting at 1, and Apache's AuthDigestNcCheck answers
401 for a carried-over count.

qop values were split on "," and compared untrimmed, so
qop="auth, auth-int" quietly downgraded to auth and qop=" auth" matched
nothing — falling into the RFC 2069 branch, which emits no qop, nc or
cnonce and which an RFC 7616 server rejects outright.

Documented as changes 10-12 in PROVENANCE. The four static assignments
in upstream's digest tests now address the handler; nothing else in that
file changes.
2026-09-09 11:22:34 +02:00

15 KiB
Raw Blame History

:dav — vendored dav4jvm

Upstream: bitfireAT/dav4jvm, tag 2.2.1, commit f434c9d19b322228916c106beaebd8634b85ddb1. Licence: MPL-2.0 (dav/LICENSE, verbatim). Every file keeps its upstream header, as §3.4 requires. Agendula's own code is MIT and unaffected — MPL is file-level copyleft, which is why this lives in its own module rather than inside :app.

62 source files, ~4,200 lines, plus upstream's 15 test classes.


Why vendored, and why this version

docs/SYNC-PLAN.md decision 1 settled vendor rather than depend. dav4jvm is published on JitPack only, which conflicts with our FAIL_ON_PROJECT_REPOS policy, does not sign artifacts, and rebuilds on demand — so a coordinate is not immutable. Upstream also shipped two breaking majors nineteen days apart (3.0.0 on 2026-07-08, 4.0.0 on 2026-07-27).

2.2.1 is the last OkHttp release. 3.0.0 deleted the OkHttp package for Ktor, and 4.x additionally requires Java 21 bytecode while we target 17 across :app and all of floret-kit. Taking 4.x would mean Ktor (~2.45 MB), the wrong guava flavour, and a Java-target migration — and it would invalidate the whole auth design in docs/SYNC.md, which is written in OkHttp terms throughout (preemptive Basic via an Interceptor, OkHttp stripping Authorization on cross-host redirects, BasicDigestAuthHandler because OkHttp has no Digest).

Vendoring at 2.2.1 costs us upstream's later work and makes us responsible for this tree. What it buys: our own Java target, no JitPack, no Ktor tail, no xpp3 in the APK, and the freedom to fix the defects below rather than route around them.

This is a plain JVM module, not an Android library. The upstream tree has zero Android imports and must not gain any: docs/SYNC.md earmarks this layer for floret-kit's core-dav, and Calendula needs the same primitives, so extraction should stay a file move.


Changes from upstream

Upstream's own test suite is vendored with the code and passes unmodified — that is what makes these changes safe to make. Our additions live in LocalChangesTest; every other test file is upstream's, untouched.

1. commons-lang3 removed

HttpUtils.kt imported org.apache.commons.lang3.time.DateUtils for exactly one call — DateUtils.parseDate(str, locale, patterns…), a loop over format strings. Replaced with that loop. The dependency is gone; the format list is byte-for-byte upstream's, comments included.

⚠️ The loop is not the whole of what DateUtils did. It parsed with a ParsePosition and rejected a pattern unless the entire string was consumed; SimpleDateFormat.parse(String) accepts a prefix. Pattern 1 ends in the quoted literal 'GMT', so "Wed, 21 Oct 2015 07:28:00 GMT+02:00" matches it as a prefix and the offset is silently discarded — a two-hour error, and precisely the failure change 2 exists to remove. The replacement requires full consumption.

2. ⚠️ HTTP dates were parsed and formatted in the device's local zone

An upstream defect, not a porting artefact. httpDateFormatStr is "EEE, dd MMM yyyy HH:mm:ss 'GMT'" — the GMT is a quoted literal, so SimpleDateFormat neither reads nor writes a zone from it, and httpDateFormat never had timeZone set. So formatDate emitted local time labelled GMT, and parseDate read 07:28:00 GMT as 07:28 local — every getlastmodified out by the device's UTC offset, in whichever direction the user happens to live.

Upstream's HttpUtilsTest covers only fileName() and never touches dates, which is why this survived. Fixed by forcing GMT on the formatter and on every parse attempt; patterns carrying a real z still take the zone from the input, as they must. Covered by LocalChangesTest.

3. Permanent redirects now reach the caller (dav4jvm#209)

followRedirects mutated location in place for every 3xx and told the caller nothing, so a caller could not distinguish "this resource has moved, store the new URL" from "follow this once". DAVx5 consequently never rewrites a stored collection URL after a 301 and re-follows it on every sync; docs/SYNC.md names persisting the new URL ourselves as the fix.

Added DavResource.permanentLocation, set only along an unbroken chain of 301 / 308. A temporary hop ends the chain — 301 → 302 means the resource moved to the 301's target and is being served elsewhere for now, so persisting the 302's target would be wrong. location still moves for every redirect, unchanged.

It is cleared at the start of every request, so it describes the request just made and never one made earlier through the same object. DavResource instances are reused, and a stale value would have the caller persist a URL that a later move() already superseded.

4. xpp3 is compile-time only

Upstream declares org.ogce:xpp3 as api. Android ships org.xmlpull.v1 in the framework, so the 371 KB jar is compileOnly here and never reaches the APK. The unit tests run on a plain JVM, which has no framework, so they get the real implementation via testImplementation.

5. httpDateFormat is no longer a shared mutable formatter

Upstream exposed a single public SimpleDateFormat. It is mutable and not thread-safe: two workers formatting a header concurrently corrupt each other through its Calendar, and any caller could setTimeZone on it and undo change 2 for everyone else. It is now private and built per call. Nothing else in the tree referenced it.

6. <D:unauthenticated/> is parsed instead of inferred

CurrentUserPrincipal.Factory read only the <href> child, so an unauthenticated body (RFC 5397 §3 — a 200 whose content means the request was not authenticated) arrived as "property present, href null" — identical to a conformant-but-empty element, and to a server that omits the property entirely. A caller inferring rejection from the null href therefore also fires on merely non-conformant servers, and on a request that carried no credential at all.

The factory now makes one pass over both children (XmlUtils.processTag consumes to the end tag and so cannot be called twice) and reports unauthenticated explicitly. Without it, a rejected credential is indistinguishable from a successful discovery that found nothing.


Build integration

  • :dav's tests are a plain JVM test task. CI runs testDebugUnitTest, which exists only on Android variants, so .forgejo/workflows/ci.yaml names :dav:test explicitly. Without that the vendored suite is compiled by nobody and run by nobody, and the safety argument above is void.
  • :app declares testImplementation(libs.xpp3). compileOnly is not transitive, :app's unit tests run on a plain JVM with no framework, and isReturnDefaultValues = true makes android.jar's stub factory return null — so anything touching XmlUtils would fail with an unrelated-looking NPE.

What was not a defect

docs/SYNC.md lists two dav4jvm defects we would inherit. Only one of them exists in this version:

  • "Handles 301/302/307/308 but not 303" — does not hold for 2.2.1. followRedirects gates on OkHttp's Response.isRedirect, which includes HTTP_SEE_OTHER, and it re-sends the same method, which is what RFC 6764 §5 asks for during discovery. Pinned by a test so a future resync cannot lose it silently.
  • dav4jvm#209 — real, and fixed above as change 3.

Resyncing

Fetch the new tag, diff against f434c9d, reapply changes 1–4, run ./gradlew :dav:test. If upstream's suite fails, the port is wrong — that is the entire reason it is vendored alongside the code.

Change 7 — a same-host HTTPS→HTTP redirect is upgraded, not refused

DavResource.followRedirects threw DavException("Received redirect from HTTPS to HTTP") for any downgrade. That is right for a redirect to a different host, which has no innocent reading. It is wrong for the same host, and the same host is the case that actually occurs.

⚠️ A Nextcloud behind a TLS-terminating reverse proxy without overwriteprotocol — or without proxy_set_header X-Forwarded-Proto $scheme — builds every redirect with http://. That includes the /.well-known/caldav hop RFC 6764 discovery depends on. The server is entirely functional: /remote.php/dav/ answers 401 over HTTPS exactly as it should. But discovery refuses the downgrade, falls back to a PROPFIND on the web root, gets the 405 an ordinary web server returns, and reports "not a CalDAV server" about a working CalDAV server.

Now: when the redirect target's host matches the current one, the scheme is put back to https and the hop continues. Re-issuing the same host and path over TLS is strictly safer than obeying the redirect as sent, and it preserves the invariant that matters — credentials never travel in cleartext. A cross-host downgrade still throws.

Found against a real server, not by reading: cloud.jeanlucmakiola.de returns 301 → http://cloud.jeanlucmakiola.de/remote.php/dav/.

Change 8 — the credential is scoped by the public-suffix list, not by a label split

BasicDigestAuthHandler gated every request on domain.equals(UrlUtils.hostToDomain(request host)), and hostToDomain is a pure last-two-labels split with no public-suffix knowledge. So a server at cloud.example.co.uk scoped the credential to co.uk, one at myhome.duckdns.org to duckdns.org, and a self-hoster at 192.168.1.10 to 1.10.

⚠️ The handler adds Authorization: Basic preemptively, before any challenge, to the first HTTPS request to any host that passes that gate. So the scope is not merely recorded — it is the set of hosts that receive the app password unprompted.

That is reachable. ServiceDiscovery builds candidate origins from SRV targets without requiring the target to lie inside the queried domain, over plain UDP DNS with no DNSSEC. Correct scoping forces an on-path attacker to obtain a certificate for a name inside the victim's own registrable domain, which is infeasible; co.uk scoping lets them point the SRV at a domain they own and hold a legitimate certificate for.

Now: request.url.topPrivateDomain() ?: request.url.host. OkHttp bundles the public-suffix list including its private section, so the dynamic-DNS providers self-hosters actually use are covered. topPrivateDomain() is null for an IP literal, a single-label host, and a host that is a public suffix — and null means no restriction to this handler, so it falls back to the exact host.

The caller must derive the scope the same way, which is why CalDavHttp.authenticated changed with it: a mismatch withholds the credential from every request rather than leaking it.

UrlUtils.hostToDomain and its test are left alone — after this it has no production callers, and keeping it keeps the resync diff small.

Change 9 — Basic is refused over cleartext even when the server asks for it

insecurePreemptive gated only the preemptive branch. A plain-HTTP server answering 401 with a Basic challenge still got Authorization: Basic <user:password> in the clear — the flag's name was accurate and its coverage was not.

The gate now sits on the Basic emission, so both paths are covered by one condition, and the flag is renamed insecureBasic to say what it actually permits. Digest is deliberately untouched: it never puts the password on the wire, and refusing it would break a LAN server the day the documented per-account cleartext opt-in ships.

Not currently reachable in the app — network_security_config.xml sets cleartextTrafficPermitted="false", so OkHttp throws before the request is written, and nothing passes allowCleartext = true. Fixed anyway: :caldav is a plain JVM module earmarked for reuse, where the Android policy does not apply, and the mitigation would evaporate silently the day that opt-in is wired up.

A refused challenge is also not cached. Recording one we never answer leaves the handler believing Basic is in play, so the preemptive block is skipped, the refusal repeats, and the 401 after that reports "Basic credentials didn't work last time" about a credential that never reached the wire.

⚠️ Upstream's BasicDigestAuthHandlerTest.testBasic was amended — it asserted exactly this behaviour, using http://example.com with a Basic challenge and expecting the header. Its URL is now https://, and the cleartext cases it used to cover are pinned explicitly by cleartextBasicIsRefusedEvenWhenChallenged, cleartextBasicIsSentWhenExplicitlyAllowed and cleartextDigestIsStillAnswered. This is the one upstream test this port deliberately changes rather than inherits.

Change 10 — a Basic challenge no longer hides the Digest one beside it

The 401 branch scanned response.challenges() in a loop and did a non-local return null the moment it saw a Basic challenge it had already tried. Because the handler is installed as a network interceptor, basicAuth is always primed preemptively over HTTPS — so that fired on the first 401, and a Digest challenge later in the same header was never read at all.

A Baïkal or Apache front end that advertises both and rejects Basic at the application layer therefore never got a Digest answer, and SyncEngine marked the account as needing sign-in permanently.

The loop now reads every challenge before anything is decided; a scheme already known not to work is simply not re-offered. Giving up is still the outcome when nothing usable is left — it happens after the whole header has been looked at rather than in the middle of it.

Change 11 — the digest counter is per handler, and per nonce

clientNonce and nonceCount lived on the companion object while SyncEngine builds one handler per account, so two accounts syncing at once interleaved their nc values against each other's nonces. They are instance state now.

The count was also never reset. RFC 7616 §3.4.1 defines nc as the count of requests sent with that nonce, starting at 1, so a server that enforces it (Apache AuthDigestNcCheck On, several NAS stacks) answers 401 for a rotated nonce that arrives with a carried-over count. A new server nonce now restarts it. The client nonce stays put: it is ours, one per handler, and pairing it with a restarted count is what the RFC describes.

The tell upstream left behind is that every digest test has to reset the counter by hand — those four assignments now address the handler rather than the class, which is the only change to that file.

Change 12 — qop list values are trimmed

paramValue.split(",") with no trim(). HTTP list syntax allows space around the separator, so qop="auth, auth-int" silently downgraded to auth, and a single spaced value (qop=" auth") matched nothing at all — dropping into the RFC 2069 legacy branch, which emits a response with no qop, nc or cnonce. An RFC 7616 server rejects that outright: a permanent 401 against a server behaving perfectly legally. Values are trimmed and compared case-insensitively.