Skip to content

Commit 21ad1aa

Browse files
iVamsiyschimke
andauthored
cache: guard null fields in Entry.writeTo() to fix NPE (#8962) (#9345)
Co-authored-by: Vamsi <iVamsi@users.noreply.github.com> Co-authored-by: Yuri Schimke <yuri@schimke.ee>
1 parent bbe4f22 commit 21ad1aa

2 files changed

Lines changed: 130 additions & 2 deletions

File tree

okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -628,9 +628,9 @@ class Cache internal constructor(
628628
.writeDecimalLong(receivedResponseMillis)
629629
.writeByte('\n'.code)
630630

631-
if (url.isHttps) {
631+
if (url.isHttps && handshake != null) {
632632
sink.writeByte('\n'.code)
633-
sink.writeUtf8(handshake!!.cipherSuite.javaName).writeByte('\n'.code)
633+
sink.writeUtf8(handshake.cipherSuite.javaName).writeByte('\n'.code)
634634
writeCertList(sink, handshake.peerCertificates)
635635
writeCertList(sink, handshake.localCertificates)
636636
sink.writeUtf8(handshake.tlsVersion.javaName).writeByte('\n'.code)

okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,134 @@ class CacheTest {
422422
.close()
423423
}
424424

425+
/**
426+
* A network interceptor strips the handshake from a real HTTPS response before the
427+
* CacheInterceptor writes it to disk. This creates the bug condition: url.isHttps=true
428+
* but handshake=null. Before the fix, `handshake!!` in writeTo() threw NPE.
429+
*
430+
* https://github.com/square/okhttp/issues/8962
431+
*/
432+
@Test
433+
fun httpsResponseWithNullHandshakeDoesNotCrashWriteTo() {
434+
server.useHttps(handshakeCertificates.sslSocketFactory())
435+
server.enqueue(
436+
MockResponse
437+
.Builder()
438+
.body("secure content")
439+
.addHeader("Cache-Control", "max-age=3600")
440+
.build(),
441+
)
442+
443+
client =
444+
client
445+
.newBuilder()
446+
.sslSocketFactory(
447+
handshakeCertificates.sslSocketFactory(),
448+
handshakeCertificates.trustManager,
449+
).hostnameVerifier(NULL_HOSTNAME_VERIFIER)
450+
.addNetworkInterceptor { chain ->
451+
chain
452+
.proceed(chain.request())
453+
.newBuilder()
454+
.handshake(null)
455+
.build()
456+
}.build()
457+
458+
val response = client.newCall(Request(server.url("/"))).execute()
459+
assertThat(response.code).isEqualTo(200)
460+
assertThat(response.body.string()).isEqualTo("secure content")
461+
}
462+
463+
/**
464+
* Verifies the null-handshake fix holds across multiple sequential cache writes, confirming
465+
* it is not a one-time race condition.
466+
*
467+
* https://github.com/square/okhttp/issues/8962
468+
*/
469+
@Test
470+
fun multipleHttpsRequestsWithNullHandshakeAllSucceed() {
471+
server.useHttps(handshakeCertificates.sslSocketFactory())
472+
repeat(3) {
473+
server.enqueue(
474+
MockResponse
475+
.Builder()
476+
.body("response $it")
477+
.addHeader("Cache-Control", "max-age=3600")
478+
.build(),
479+
)
480+
}
481+
482+
client =
483+
client
484+
.newBuilder()
485+
.sslSocketFactory(
486+
handshakeCertificates.sslSocketFactory(),
487+
handshakeCertificates.trustManager,
488+
).hostnameVerifier(NULL_HOSTNAME_VERIFIER)
489+
.addNetworkInterceptor { chain ->
490+
chain
491+
.proceed(chain.request())
492+
.newBuilder()
493+
.handshake(null)
494+
.build()
495+
}.build()
496+
497+
repeat(3) { i ->
498+
val response = client.newCall(Request(server.url("/path$i"))).execute()
499+
assertThat(response.code).isEqualTo(200)
500+
assertThat(response.body.string()).isEqualTo("response $i")
501+
}
502+
}
503+
504+
/**
505+
* When handshake is null for an HTTPS URL, the TLS block is skipped making the entry
506+
* unreadable on re-read. The response should still succeed but won't be served from cache
507+
* on subsequent requests.
508+
*
509+
* https://github.com/square/okhttp/issues/8962
510+
*/
511+
@Test
512+
fun httpsResponseWithNullHandshakeIsNotServedFromCache() {
513+
server.useHttps(handshakeCertificates.sslSocketFactory())
514+
server.enqueue(
515+
MockResponse
516+
.Builder()
517+
.body("first")
518+
.addHeader("Cache-Control", "max-age=3600")
519+
.build(),
520+
)
521+
server.enqueue(
522+
MockResponse
523+
.Builder()
524+
.body("second")
525+
.addHeader("Cache-Control", "max-age=3600")
526+
.build(),
527+
)
528+
529+
client =
530+
client
531+
.newBuilder()
532+
.sslSocketFactory(
533+
handshakeCertificates.sslSocketFactory(),
534+
handshakeCertificates.trustManager,
535+
).hostnameVerifier(NULL_HOSTNAME_VERIFIER)
536+
.addNetworkInterceptor { chain ->
537+
chain
538+
.proceed(chain.request())
539+
.newBuilder()
540+
.handshake(null)
541+
.build()
542+
}.build()
543+
544+
val response1 = client.newCall(Request(server.url("/"))).execute()
545+
assertThat(response1.body.string()).isEqualTo("first")
546+
547+
// Second request hits the network again because the first entry was not cacheable
548+
val response2 = client.newCall(Request(server.url("/"))).execute()
549+
assertThat(response2.body.string()).isEqualTo("second")
550+
assertThat(response2.cacheResponse).isNull()
551+
}
552+
425553
@Test
426554
fun responseCachingAndRedirects() {
427555
server.enqueue(

0 commit comments

Comments
 (0)