makiolaj
89c3a5f66e
The cleartext gate sits on the emission, but the 401 branch cached the Basic challenge before reaching it. So after refusing a plain-HTTP challenge the handler still believed Basic was in play: the preemptive block is skipped because it requires both caches empty, the refusal repeats, and the 401 after that logs "Basic credentials didn't work last time" about a credential that never reached the wire.
250 lines
12 KiB
Markdown
250 lines
12 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.
|
||
|
||
## 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.
|