sync: what the on-device round found
Everything here came from running the account flow against a real Nextcloud rather than from reading the code. Discovery - A typed bare origin now gets the RFC 6764 well-known probe. It was returned as the only candidate, so `https://cloud.example.com` — what people actually type — was PROPFIND'd against the web UI, answered 405, and a working Nextcloud reported as "not a CalDAV server". - A same-host HTTPS→HTTP redirect is put back on TLS instead of refused (`dav` change 7). A Nextcloud behind a TLS-terminating proxy without `overwriteprotocol` builds every redirect with http://, including the /.well-known/caldav hop discovery depends on. Cross-host still throws. - Outcomes carry a `Cause` the UI translates, not the server's own words. "HTTP 405 Method Not Allowed" told someone entering an address nothing, in a language they may not read, from outside strings.xml. - An IPv6 origin keeps its brackets: `HttpUrl.host` returns "fd00::1", so the rebuilt origin did not parse and a homelab address came back as "not an address". Login Flow v2 - The poll response's scheme is coerced, never refused. Nextcloud returns the app password exactly once, so throwing there burned a live credential and left it dangling in the user's device list. The host mismatch already worked this way; the scheme now matches it. Accounts - The accounts screen observes Room and the sign-in state instead of taking a snapshot, so a sync landing — or a 401 stopping an account — reaches a screen that is already open. - A per-account detail screen, and provider identity (`CalDavProvider`) shared with the quirk table so one list drives both the icon and the warning. - The password field masks: floret-kit's `InlineTextField` gained a visual transformation, since `KeyboardType.Password` only tells the IME to drop suggestions.
This commit is contained in:
@@ -155,3 +155,28 @@ exists in this version:
|
||||
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/`.
|
||||
|
||||
@@ -33,4 +33,5 @@ dependencies {
|
||||
// vendoring safe, which is the same call provider/PROVENANCE.md made.
|
||||
testImplementation(libs.junit4)
|
||||
testImplementation(libs.okhttp.mockwebserver)
|
||||
testImplementation(libs.okhttp.tls)
|
||||
}
|
||||
|
||||
@@ -635,12 +635,34 @@ open class DavResource @JvmOverloads constructor(
|
||||
if (response.isRedirect)
|
||||
// handle 3xx Redirection
|
||||
response.use {
|
||||
val target = it.header("Location")?.let { location.resolve(it) }
|
||||
var target = it.header("Location")?.let { location.resolve(it) }
|
||||
if (target != null) {
|
||||
log.fine("Redirected, new location = $target")
|
||||
|
||||
if (location.isHttps && !target.isHttps)
|
||||
throw DavException("Received redirect from HTTPS to HTTP")
|
||||
if (location.isHttps && !target.isHttps) {
|
||||
// ⚠️ A downgrade to the *same host* is a server
|
||||
// misconfiguration, not an attack, and it is the
|
||||
// single most common one in this space: a Nextcloud
|
||||
// behind a TLS-terminating proxy without
|
||||
// `overwriteprotocol` (or `X-Forwarded-Proto`) builds
|
||||
// every redirect with http://, including the
|
||||
// /.well-known/caldav hop that discovery depends on.
|
||||
// Refusing it outright makes a perfectly good server
|
||||
// undiscoverable, and the user cannot tell why.
|
||||
//
|
||||
// Re-issuing the same host and path over TLS is
|
||||
// strictly safer than what we were asked to do, and
|
||||
// preserves the invariant that actually matters:
|
||||
// credentials never travel in the clear. A downgrade
|
||||
// pointing at a *different* host has no innocent
|
||||
// reading, so that still fails.
|
||||
if (target.host == location.host) {
|
||||
target = target.newBuilder().scheme("https").build()
|
||||
log.fine("Upgraded same-host downgrade back to $target")
|
||||
} else {
|
||||
throw DavException("Received redirect from HTTPS to HTTP")
|
||||
}
|
||||
}
|
||||
|
||||
if (chainStillPermanent && (it.code == HTTP_MOVED_PERM || it.code == HTTP_PERM_REDIRECT))
|
||||
permanentLocation = target
|
||||
|
||||
@@ -10,10 +10,13 @@ import at.bitfire.dav4jvm.property.CurrentUserPrincipal
|
||||
import okhttp3.OkHttpClient
|
||||
import okhttp3.mockwebserver.MockResponse
|
||||
import okhttp3.mockwebserver.MockWebServer
|
||||
import okhttp3.tls.HandshakeCertificates
|
||||
import okhttp3.tls.HeldCertificate
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertNull
|
||||
import org.junit.Assert.assertThrows
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
@@ -242,4 +245,73 @@ class LocalChangesTest {
|
||||
TimeZone.setDefault(zone)
|
||||
}
|
||||
}
|
||||
|
||||
// --- change 7: a same-host downgrade is upgraded, a cross-host one is not ---
|
||||
|
||||
/**
|
||||
* A TLS MockWebServer, because the branch under test only exists on HTTPS.
|
||||
*
|
||||
* Same trap as change 2's: a test that runs against plain HTTP here would
|
||||
* pass whatever the code did, since `location.isHttps` gates the whole
|
||||
* decision.
|
||||
*/
|
||||
private fun httpsServer(): Pair<MockWebServer, OkHttpClient> {
|
||||
val certificate = HeldCertificate.Builder()
|
||||
.addSubjectAlternativeName("localhost")
|
||||
.build()
|
||||
val serverCertificates = HandshakeCertificates.Builder()
|
||||
.heldCertificate(certificate)
|
||||
.build()
|
||||
val clientCertificates = HandshakeCertificates.Builder()
|
||||
.addTrustedCertificate(certificate.certificate)
|
||||
.build()
|
||||
|
||||
val tlsServer = MockWebServer().apply {
|
||||
useHttps(serverCertificates.sslSocketFactory(), false)
|
||||
start()
|
||||
}
|
||||
val client = OkHttpClient.Builder()
|
||||
.followRedirects(false)
|
||||
.sslSocketFactory(
|
||||
clientCertificates.sslSocketFactory(),
|
||||
clientCertificates.trustManager,
|
||||
)
|
||||
.build()
|
||||
return tlsServer to client
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a same-host redirect to http is retried over https`() {
|
||||
// ⚠️ Exactly what a Nextcloud behind a proxy without `overwriteprotocol`
|
||||
// sends for /.well-known/caldav. Refusing it makes a working CalDAV
|
||||
// server report as "not a CalDAV server".
|
||||
val (tls, client) = httpsServer()
|
||||
val resource = DavResource(client, tls.url("/.well-known/caldav"))
|
||||
val downgrade = "http://${tls.hostName}:${tls.port}/remote.php/dav/"
|
||||
|
||||
tls.enqueue(MockResponse().setResponseCode(301).setHeader("Location", downgrade))
|
||||
tls.enqueue(MockResponse().setResponseCode(200))
|
||||
|
||||
resource.head { }
|
||||
|
||||
tls.takeRequest()
|
||||
val followed = tls.takeRequest()
|
||||
assertEquals("/remote.php/dav/", followed.path)
|
||||
// The scheme was put back; the host and path are the server's own.
|
||||
assertEquals("https", resource.location.scheme)
|
||||
assertEquals("/remote.php/dav/", resource.location.encodedPath)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cross-host redirect to http is still refused`() {
|
||||
val (tls, client) = httpsServer()
|
||||
val resource = DavResource(client, tls.url("/dav/"))
|
||||
tls.enqueue(
|
||||
MockResponse().setResponseCode(301)
|
||||
.setHeader("Location", "http://elsewhere.example.com/dav/"),
|
||||
)
|
||||
|
||||
// No innocent reading of this one, so it stays fatal.
|
||||
assertThrows(at.bitfire.dav4jvm.exception.DavException::class.java) { resource.head { } }
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user