Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
128 changes: 128 additions & 0 deletions okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading