diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 71b85924579d..c4076307e52a 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -628,9 +628,9 @@ class Cache internal constructor( .writeDecimalLong(receivedResponseMillis) .writeByte('\n'.code) - if (url.isHttps) { + if (url.isHttps && handshake != null) { sink.writeByte('\n'.code) - sink.writeUtf8(handshake!!.cipherSuite.javaName).writeByte('\n'.code) + sink.writeUtf8(handshake.cipherSuite.javaName).writeByte('\n'.code) writeCertList(sink, handshake.peerCertificates) writeCertList(sink, handshake.localCertificates) sink.writeUtf8(handshake.tlsVersion.javaName).writeByte('\n'.code) diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 573f085b8642..238a02499fef 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -422,6 +422,134 @@ class CacheTest { .close() } + /** + * 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. + * + * https://github.com/square/okhttp/issues/8962 + */ + @Test + fun httpsResponseWithNullHandshakeDoesNotCrashWriteTo() { + server.useHttps(handshakeCertificates.sslSocketFactory()) + server.enqueue( + MockResponse + .Builder() + .body("secure content") + .addHeader("Cache-Control", "max-age=3600") + .build(), + ) + + client = + client + .newBuilder() + .sslSocketFactory( + handshakeCertificates.sslSocketFactory(), + handshakeCertificates.trustManager, + ).hostnameVerifier(NULL_HOSTNAME_VERIFIER) + .addNetworkInterceptor { chain -> + chain + .proceed(chain.request()) + .newBuilder() + .handshake(null) + .build() + }.build() + + val response = client.newCall(Request(server.url("/"))).execute() + assertThat(response.code).isEqualTo(200) + assertThat(response.body.string()).isEqualTo("secure content") + } + + /** + * Verifies the null-handshake fix holds across multiple sequential cache writes, confirming + * it is not a one-time race condition. + * + * https://github.com/square/okhttp/issues/8962 + */ + @Test + fun multipleHttpsRequestsWithNullHandshakeAllSucceed() { + server.useHttps(handshakeCertificates.sslSocketFactory()) + repeat(3) { + server.enqueue( + MockResponse + .Builder() + .body("response $it") + .addHeader("Cache-Control", "max-age=3600") + .build(), + ) + } + + client = + client + .newBuilder() + .sslSocketFactory( + handshakeCertificates.sslSocketFactory(), + handshakeCertificates.trustManager, + ).hostnameVerifier(NULL_HOSTNAME_VERIFIER) + .addNetworkInterceptor { chain -> + chain + .proceed(chain.request()) + .newBuilder() + .handshake(null) + .build() + }.build() + + repeat(3) { i -> + val response = client.newCall(Request(server.url("/path$i"))).execute() + assertThat(response.code).isEqualTo(200) + assertThat(response.body.string()).isEqualTo("response $i") + } + } + + /** + * 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. + * + * https://github.com/square/okhttp/issues/8962 + */ + @Test + fun httpsResponseWithNullHandshakeIsNotServedFromCache() { + server.useHttps(handshakeCertificates.sslSocketFactory()) + server.enqueue( + MockResponse + .Builder() + .body("first") + .addHeader("Cache-Control", "max-age=3600") + .build(), + ) + server.enqueue( + MockResponse + .Builder() + .body("second") + .addHeader("Cache-Control", "max-age=3600") + .build(), + ) + + client = + client + .newBuilder() + .sslSocketFactory( + handshakeCertificates.sslSocketFactory(), + handshakeCertificates.trustManager, + ).hostnameVerifier(NULL_HOSTNAME_VERIFIER) + .addNetworkInterceptor { chain -> + chain + .proceed(chain.request()) + .newBuilder() + .handshake(null) + .build() + }.build() + + 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 + val response2 = client.newCall(Request(server.url("/"))).execute() + assertThat(response2.body.string()).isEqualTo("second") + assertThat(response2.cacheResponse).isNull() + } + @Test fun responseCachingAndRedirects() { server.enqueue(