sync: do not cache a challenge we refuse to answer
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.
This commit is contained in:
@@ -234,6 +234,11 @@ written, and nothing passes `allowCleartext = true`. Fixed anyway: `:caldav` is
|
|||||||
a plain JVM module earmarked for reuse, where the Android policy does not apply,
|
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.
|
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
|
⚠️ **Upstream's `BasicDigestAuthHandlerTest.testBasic` was amended** — it
|
||||||
asserted exactly this behaviour, using `http://example.com` with a Basic
|
asserted exactly this behaviour, using `http://example.com` with a Basic
|
||||||
challenge and expecting the header. Its URL is now `https://`, and the
|
challenge and expecting the header. Its URL is now `https://`, and the
|
||||||
|
|||||||
@@ -116,7 +116,12 @@ class BasicDigestAuthHandler(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
basicAuth = newBasicAuth
|
// ⚠️ Not cached if we would refuse to answer it. Caching a challenge
|
||||||
|
// we never answer leaves the handler believing Basic is in play: 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 was never put on the wire.
|
||||||
|
basicAuth = newBasicAuth?.takeIf { request.isHttps || insecureBasic }
|
||||||
digestAuth = newDigestAuth
|
digestAuth = newDigestAuth
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -60,6 +60,23 @@ class BasicDigestAuthHandlerTest {
|
|||||||
assertNull(authenticator.authenticateRequest(original, response))
|
assertNull(authenticator.authenticateRequest(original, response))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aRefusedCleartextChallengeIsNotCached() {
|
||||||
|
val authenticator = BasicDigestAuthHandler(null, "user", "password")
|
||||||
|
val original = Request.Builder().url("http://example.com").build()
|
||||||
|
val response = Builder()
|
||||||
|
.request(original)
|
||||||
|
.protocol(Protocol.HTTP_1_1)
|
||||||
|
.code(401).message("Authentication required")
|
||||||
|
.header("WWW-Authenticate", "Basic realm=\"WallyWorld\"")
|
||||||
|
.build()
|
||||||
|
|
||||||
|
assertNull(authenticator.authenticateRequest(original, response))
|
||||||
|
// Caching it would make the *next* 401 report that the credentials did
|
||||||
|
// not work, about a credential that was never sent.
|
||||||
|
assertNull(authenticator.authenticateRequest(original, response))
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun cleartextBasicIsSentWhenExplicitlyAllowed() {
|
fun cleartextBasicIsSentWhenExplicitlyAllowed() {
|
||||||
val authenticator = BasicDigestAuthHandler(null, "user", "password", insecureBasic = true)
|
val authenticator = BasicDigestAuthHandler(null, "user", "password", insecureBasic = true)
|
||||||
|
|||||||
Reference in New Issue
Block a user