makiolaj
ede4205b7f
The auth handler decided which hosts may receive the password by comparing the last two labels of their names. A server at cloud.example.co.uk therefore scoped the app password to co.uk, one at myhome.duckdns.org to duckdns.org, and a self-hoster at 192.168.1.10 to "any address ending .1.10". The handler sends Basic preemptively, before any challenge, so those hosts get the password unprompted on the first HTTPS request. It is reachable: ServiceDiscovery accepts an SRV target outside the domain it queried, over plain UDP DNS. Scoped correctly, an on-path attacker needs a certificate for a name inside the victim's own registrable domain. Scoped to co.uk, they need one for a domain they already own. Now topPrivateDomain(), from the list OkHttp bundles, falling back to the exact host where there is none -- null means no restriction here, so an IP literal or localhost must not pass one through. The handler had to change with the caller: it re-derives the domain per request, so fixing only the caller withholds the credential from everything. Still trusted: two hosts under one registrable domain share an owner. That is what iCloud's caldav/pNN-caldav split needs, and narrowing further costs it.
217 lines
11 KiB
Markdown
217 lines
11 KiB
Markdown
# `:dav` — vendored dav4jvm
|
||
|
||
**Upstream:** [bitfireAT/dav4jvm](https://github.com/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.
|