diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index c4076307e52a..a6bbdab08f47 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -238,6 +238,17 @@ class Cache internal constructor( return null } + if (response.request.url.isHttps && response.handshake == null) { + // An HTTPS response without a handshake cannot be serialized to the cache format because the + // Entry reader expects TLS session data for HTTPS URLs. This can happen when a network + // interceptor strips the handshake or when cacheUrlOverride rewrites an HTTP URL to HTTPS. + Platform.get().log( + "HTTPS response has no handshake; skipping cache write for ${response.request.url.redact()}", + WARN, + ) + return null + } + val entry = Entry(response) var editor: DiskLruCache.Editor? = null try { @@ -259,6 +270,14 @@ class Cache internal constructor( cached: Response, network: Response, ) { + // HTTPS responses without a handshake cannot be serialized (see put() for details). + if (network.request.url.isHttps && network.handshake == null) { + Platform.get().log( + "HTTPS response has no handshake; skipping cache update for ${network.request.url.redact()}", + WARN, + ) + return + } val entry = Entry(network) val snapshot = (cached.body as CacheResponseBody).snapshot var editor: DiskLruCache.Editor? = null diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 238a02499fef..758a2bd7aa4f 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -423,14 +423,13 @@ class CacheTest { } /** - * A network interceptor strips the handshake from a real HTTPS response before the - * CacheInterceptor writes it to disk. This creates the bug condition: url.isHttps=true - * but handshake=null. Before the fix, `handshake!!` in writeTo() threw NPE. + * An HTTPS response with a null handshake is rejected by Cache.put() rather than writing a + * malformed cache entry. The response still succeeds but nothing is written to disk. * * https://github.com/square/okhttp/issues/8962 */ @Test - fun httpsResponseWithNullHandshakeDoesNotCrashWriteTo() { + fun httpsResponseWithNullHandshakeIsNotCached() { server.useHttps(handshakeCertificates.sslSocketFactory()) server.enqueue( MockResponse @@ -458,11 +457,11 @@ class CacheTest { val response = client.newCall(Request(server.url("/"))).execute() assertThat(response.code).isEqualTo(200) assertThat(response.body.string()).isEqualTo("secure content") + assertThat(cache.writeSuccessCount()).isEqualTo(0) } /** - * Verifies the null-handshake fix holds across multiple sequential cache writes, confirming - * it is not a one-time race condition. + * Multiple HTTPS requests with null handshake all succeed without writing to cache. * * https://github.com/square/okhttp/issues/8962 */ @@ -499,12 +498,12 @@ class CacheTest { assertThat(response.code).isEqualTo(200) assertThat(response.body.string()).isEqualTo("response $i") } + assertThat(cache.writeSuccessCount()).isEqualTo(0) } /** - * When handshake is null for an HTTPS URL, the TLS block is skipped making the entry - * unreadable on re-read. The response should still succeed but won't be served from cache - * on subsequent requests. + * When handshake is null for an HTTPS URL, Cache.put() rejects the entry so nothing is written + * to disk. Subsequent requests hit the network. * * https://github.com/square/okhttp/issues/8962 */ @@ -544,10 +543,11 @@ class CacheTest { val response1 = client.newCall(Request(server.url("/"))).execute() assertThat(response1.body.string()).isEqualTo("first") - // Second request hits the network again because the first entry was not cacheable + // Second request hits the network because the first was not written to cache val response2 = client.newCall(Request(server.url("/"))).execute() assertThat(response2.body.string()).isEqualTo("second") assertThat(response2.cacheResponse).isNull() + assertThat(cache.writeSuccessCount()).isEqualTo(0) } @Test