Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fail earlier on cache corruptions #6495

Merged
merged 6 commits into from Jan 9, 2021
Merged

Fail earlier on cache corruptions #6495

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8177daf
Fail earlier on cache corruptions
yschimke Dec 30, 2020
dea3db4
log warning
yschimke Dec 30, 2020
4a85eef
fix imports
yschimke Dec 30, 2020
41c67d8
Fix comment
yschimke Dec 30, 2020
9b70d19
Fix tests
yschimke Dec 30, 2020
4b8efb6
Review comments
yschimke Jan 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 34 additions & 21 deletions okhttp/src/main/kotlin/okhttp3/Cache.kt
View file
Open in desktop
Expand Up @@ -15,16 +15,7 @@
*/
package okhttp3

import java.io.Closeable
import java.io.File
import java.io.Flushable
import java.io.IOException
import java.security.cert.Certificate
import java.security.cert.CertificateEncodingException
import java.security.cert.CertificateException
import java.security.cert.CertificateFactory
import java.util.NoSuchElementException
import java.util.TreeSet
import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
import okhttp3.MediaType.Companion.toMediaTypeOrNull
import okhttp3.internal.EMPTY_HEADERS
import okhttp3.internal.cache.CacheRequest
Expand All @@ -36,6 +27,7 @@ import okhttp3.internal.http.HttpMethod
import okhttp3.internal.http.StatusLine
import okhttp3.internal.io.FileSystem
import okhttp3.internal.platform.Platform
import okhttp3.internal.platform.Platform.Companion.WARN
import okhttp3.internal.toLongOrDefault
import okio.Buffer
import okio.BufferedSink
Expand All @@ -48,6 +40,24 @@ import okio.ForwardingSource
import okio.Sink
import okio.Source
import okio.buffer
import java.io.Closeable
import java.io.File
import java.io.Flushable
import java.io.IOException
import java.security.cert.Certificate
import java.security.cert.CertificateEncodingException
import java.security.cert.CertificateException
import java.security.cert.CertificateFactory
import java.util.NoSuchElementException
import java.util.TreeSet
import kotlin.collections.ArrayList
import kotlin.collections.List
import kotlin.collections.MutableIterator
import kotlin.collections.MutableSet
import kotlin.collections.Set
import kotlin.collections.emptyList
import kotlin.collections.emptySet
import kotlin.collections.none

/**
* Caches HTTP and HTTPS responses to the filesystem so they may be reused, saving time and
Expand Down Expand Up @@ -425,7 +435,7 @@ class Cache internal constructor(
}

private class Entry {
private val url: String
private val url: HttpUrl
private val varyHeaders: Headers
private val requestMethod: String
private val protocol: Protocol
Expand All @@ -436,7 +446,7 @@ class Cache internal constructor(
private val sentRequestMillis: Long
private val receivedResponseMillis: Long

private val isHttps: Boolean get() = url.startsWith("https://")
private val isHttps: Boolean get() = url.scheme == "https"

/**
* Reads an entry from an input stream. A typical entry looks like this:
Expand Down Expand Up @@ -490,9 +500,14 @@ class Cache internal constructor(
* optional. If present, it contains the TLS version.
*/
@Throws(IOException::class) constructor(rawSource: Source) {
try {
rawSource.use {
val source = rawSource.buffer()
url = source.readUtf8LineStrict()
val urlLine = source.readUtf8LineStrict()
// Choice here is between failing with a correct RuntimeException
// or mostly silently with an IOException
url = urlLine.toHttpUrlOrNull() ?: throw IOException("Cache Failure for $urlLine").also {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 options here

  1. runtime exception - fatal...
  2. io exception only
  3. io exception and log a warning

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change cache failure to cache corruption ?

Platform.get().log("cache corruption", WARN, it)
}
requestMethod = source.readUtf8LineStrict()
val varyHeadersBuilder = Headers.Builder()
val varyRequestHeaderLineCount = readInt(source)
Expand Down Expand Up @@ -536,13 +551,11 @@ class Cache internal constructor(
} else {
handshake = null
}
} finally {
rawSource.close()
}
}

constructor(response: Response) {
this.url = response.request.url.toString()
this.url = response.request.url
this.varyHeaders = response.varyHeaders()
this.requestMethod = response.request.method
this.protocol = response.protocol
Expand All @@ -557,7 +570,7 @@ class Cache internal constructor(
@Throws(IOException::class)
fun writeTo(editor: DiskLruCache.Editor) {
editor.newSink(ENTRY_METADATA).buffer().use { sink ->
sink.writeUtf8(url).writeByte('\n'.toInt())
sink.writeUtf8(url.toString()).writeByte('\n'.toInt())
sink.writeUtf8(requestMethod).writeByte('\n'.toInt())
sink.writeDecimalLong(varyHeaders.size.toLong()).writeByte('\n'.toInt())
for (i in 0 until varyHeaders.size) {
Expand Down Expand Up @@ -618,8 +631,8 @@ class Cache internal constructor(
private fun writeCertList(sink: BufferedSink, certificates: List<Certificate>) {
try {
sink.writeDecimalLong(certificates.size.toLong()).writeByte('\n'.toInt())
for (i in 0 until certificates.size) {
val bytes = certificates[i].encoded
for (element in certificates) {
val bytes = element.encoded
val line = bytes.toByteString().base64()
sink.writeUtf8(line).writeByte('\n'.toInt())
}
Expand All @@ -629,7 +642,7 @@ class Cache internal constructor(
}

fun matches(request: Request, response: Response): Boolean {
return url == request.url.toString() &&
return url == request.url &&
requestMethod == request.method &&
varyMatches(response, varyHeaders, request)
}
Expand Down
20 changes: 10 additions & 10 deletions okhttp/src/main/kotlin/okhttp3/HttpUrl.kt
View file
Open in desktop
Expand Up @@ -15,15 +15,6 @@
*/
package okhttp3

import java.net.InetAddress
import java.net.MalformedURLException
import java.net.URI
import java.net.URISyntaxException
import java.net.URL
import java.nio.charset.Charset
import java.nio.charset.StandardCharsets.UTF_8
import java.util.Collections
import java.util.LinkedHashSet
import okhttp3.HttpUrl.Companion.toHttpUrl
import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
import okhttp3.internal.canParseAsIpAddress
Expand All @@ -34,6 +25,15 @@ import okhttp3.internal.parseHexDigit
import okhttp3.internal.publicsuffix.PublicSuffixDatabase
import okhttp3.internal.toCanonicalHost
import okio.Buffer
import java.net.InetAddress
import java.net.MalformedURLException
import java.net.URI
import java.net.URISyntaxException
import java.net.URL
import java.nio.charset.Charset
import java.nio.charset.StandardCharsets.UTF_8
import java.util.Collections
import java.util.LinkedHashSet

/**
* A uniform resource locator (URL) with a scheme of either `http` or `https`. Use this class to
Expand Down Expand Up @@ -1258,7 +1258,7 @@ class HttpUrl internal constructor(
this.scheme = base.scheme
} else {
throw IllegalArgumentException(
"Expected URL scheme 'http' or 'https' but no colon was found")
"Expected URL scheme 'http' or 'https' but no scheme was found for $input")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to drop this for PII reasons if there is concern, although as a corrupted url, the debug value seems higher than normal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log up to 6 characters? Like www.ex... ?

}

// Authority.
Expand Down
38 changes: 26 additions & 12 deletions okhttp/src/test/java/okhttp3/CacheCorruptionTest.kt
View file
Open in desktop
Expand Up @@ -15,17 +15,6 @@
*/
package okhttp3

import java.io.File
import java.net.CookieManager
import java.net.ResponseCache
import java.text.DateFormat
import java.text.SimpleDateFormat
import java.util.Date
import java.util.Locale
import java.util.TimeZone
import java.util.concurrent.TimeUnit
import javax.net.ssl.HostnameVerifier
import javax.net.ssl.SSLSession
import mockwebserver3.MockResponse
import mockwebserver3.MockWebServer
import okhttp3.internal.buildCache
Expand All @@ -37,6 +26,17 @@ import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.extension.RegisterExtension
import java.io.File
import java.net.CookieManager
import java.net.ResponseCache
import java.text.DateFormat
import java.text.SimpleDateFormat
import java.util.Date
import java.util.Locale
import java.util.TimeZone
import java.util.concurrent.TimeUnit
import javax.net.ssl.HostnameVerifier
import javax.net.ssl.SSLSession

class CacheCorruptionTest(
var server: MockWebServer
Expand All @@ -49,7 +49,7 @@ class CacheCorruptionTest(
private lateinit var client: OkHttpClient
private lateinit var cache: Cache
private val NULL_HOSTNAME_VERIFIER =
HostnameVerifier { name: String?, session: SSLSession? -> true }
HostnameVerifier { _: String?, _: SSLSession? -> true }
private val cookieManager = CookieManager()

@BeforeEach fun setUp() {
Expand Down Expand Up @@ -100,6 +100,20 @@ class CacheCorruptionTest(
assertThat(cache.hitCount()).isEqualTo(0)
}

@Test fun corruptedUrl() {
val response = testCorruptingCache {
corruptMetadata {
// strip https scheme
it.substring(5)
}
}

assertThat(response.body!!.string()).isEqualTo("ABC.2") // not cached
assertThat(cache.requestCount()).isEqualTo(2)
assertThat(cache.networkCount()).isEqualTo(2)
assertThat(cache.hitCount()).isEqualTo(0)
}

private fun corruptMetadata(corruptor: (String) -> String) {
val metadataFile = fileSystem.files.keys.find { it.name.endsWith(".0") }
val metadataBuffer = fileSystem.files[metadataFile]
Expand Down
12 changes: 6 additions & 6 deletions okhttp/src/test/java/okhttp3/HttpUrlTest.java
View file
Open in desktop
Expand Up @@ -137,7 +137,7 @@ public void parseDoesNotTrimOtherWhitespaceCharacters() throws Exception {

assertInvalid("image640://480.png", "Expected URL scheme 'http' or 'https' but was 'image640'");
assertInvalid("httpp://host/", "Expected URL scheme 'http' or 'https' but was 'httpp'");
assertInvalid("0ttp://host/", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("0ttp://host/", "Expected URL scheme 'http' or 'https' but no scheme was found for 0ttp://host/");
assertInvalid("ht+tp://host/", "Expected URL scheme 'http' or 'https' but was 'ht+tp'");
assertInvalid("ht.tp://host/", "Expected URL scheme 'http' or 'https' but was 'ht.tp'");
assertInvalid("ht-tp://host/", "Expected URL scheme 'http' or 'https' but was 'ht-tp'");
Expand All @@ -147,11 +147,11 @@ public void parseDoesNotTrimOtherWhitespaceCharacters() throws Exception {

@Test
public void parseNoScheme() throws Exception {
assertInvalid("//host", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("/path", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("path", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("?query", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("#fragment", "Expected URL scheme 'http' or 'https' but no colon was found");
assertInvalid("//host", "Expected URL scheme 'http' or 'https' but no scheme was found for //host");
assertInvalid("/path", "Expected URL scheme 'http' or 'https' but no scheme was found for /path");
assertInvalid("path", "Expected URL scheme 'http' or 'https' but no scheme was found for path");
assertInvalid("?query", "Expected URL scheme 'http' or 'https' but no scheme was found for ?query");
assertInvalid("#fragment", "Expected URL scheme 'http' or 'https' but no scheme was found for #fragment");
}

@Test
Expand Down