KTOR-7828 Revert removed content-length check (#4505)

* KTOR-7828 Revert removed content-length check
ktorio · Dec 2, 2024 · f4bf505 · f4bf505
1 parent 561cd8d
commit f4bf505
Show file tree

Hide file tree

Showing 15 changed files with 109 additions and 26 deletions.
diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/call/SavedCall.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/call/SavedCall.kt
@@ -10,8 +10,8 @@ import io.ktor.client.statement.*
 import io.ktor.http.*
 import io.ktor.util.date.*
 import io.ktor.utils.io.*
-import kotlinx.io.*
-import kotlin.coroutines.*
+import kotlinx.io.readByteArray
+import kotlin.coroutines.CoroutineContext
 
 internal class SavedHttpCall(
     client: HttpClient,
@@ -23,6 +23,8 @@ internal class SavedHttpCall(
     init {
         this.request = SavedHttpRequest(this, request)
         this.response = SavedHttpResponse(this, responseBody, response)
+
+        checkContentLength(response.contentLength(), responseBody.size.toLong(), request.method)
     }
 
     /**

diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/call/utils.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/call/utils.kt
@@ -14,3 +14,13 @@ public class UnsupportedContentTypeException(content: OutgoingContent) :
 public class UnsupportedUpgradeProtocolException(
     url: Url
 ) : IllegalArgumentException("Unsupported upgrade protocol exception: $url")
+
+internal fun checkContentLength(contentLength: Long?, bodySize: Long, method: HttpMethod) {
+    if (contentLength == null || contentLength < 0 || method == HttpMethod.Head) return
+
+    if (contentLength != bodySize) {
+        throw IllegalStateException(
+            "Content-Length mismatch: expected $contentLength bytes, but received $bodySize bytes"
+        )
+    }
+}
diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultTransform.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultTransform.kt
@@ -79,6 +79,12 @@ public fun HttpClient.defaultTransformers() {
 
             ByteArray::class -> {
                 val bytes = body.toByteArray()
+                val contentLength = context.response.contentLength()
+
+                if (context.request.method != HttpMethod.Head) {
+                    checkContentLength(contentLength, bytes.size.toLong())
+                }
+
                 proceedWith(HttpResponseContainer(info, bytes))
             }
 
@@ -124,6 +130,12 @@ public fun HttpClient.defaultTransformers() {
     platformResponseDefaultTransformers()
 }
 
+private fun checkContentLength(contentLength: Long?, bytes: Long) {
+    check(contentLength == null || contentLength == bytes) {
+        "Content-Length mismatch: expected $contentLength bytes, but received $bytes bytes"
+    }
+}
+
 internal expect fun platformRequestDefaultTransform(
     contentType: ContentType?,
     context: HttpRequestBuilder,

diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
@@ -21,7 +21,7 @@ import io.ktor.util.date.*
 import io.ktor.util.logging.*
 import io.ktor.util.pipeline.*
 import io.ktor.utils.io.*
-import kotlin.coroutines.*
+import kotlin.coroutines.CoroutineContext
 
 internal object CacheControl {
     internal val NO_STORE = HeaderValue("no-store")
@@ -208,8 +208,11 @@ public class HttpCache private constructor(
                     LOGGER.trace("Caching response for ${response.call.request.url}")
                     val cachedData = plugin.cacheResponse(response)
                     if (cachedData != null) {
-                        val reusableResponse =
-                            cachedData.createResponse(scope, response.request, response.coroutineContext)
+                        val reusableResponse = cachedData.createResponse(
+                            scope,
+                            response.request,
+                            response.coroutineContext
+                        )
                         proceedWith(reusableResponse)
                         return@intercept
                     }

diff --git a/ktor-client/ktor-client-curl/desktop/src/io/ktor/client/engine/curl/CurlClientEngine.kt b/ktor-client/ktor-client-curl/desktop/src/io/ktor/client/engine/curl/CurlClientEngine.kt
@@ -11,7 +11,6 @@ import io.ktor.client.plugins.sse.*
 import io.ktor.client.request.*
 import io.ktor.http.*
 import io.ktor.http.cio.*
-import io.ktor.util.*
 import io.ktor.util.date.*
 import io.ktor.utils.io.*
 import kotlinx.coroutines.*
@@ -42,7 +41,7 @@ internal class CurlClientEngine(
 
             val status = HttpStatusCode.fromValue(status)
 
-            val headers = HeadersImpl(rawHeaders.toMap())
+            val headers = filterCurlHeaders(rawHeaders)
             rawHeaders.release()
 
             val responseBody: Any = data.attributes.getOrNull(ResponseAdapterAttributeKey)
@@ -66,6 +65,23 @@ internal class CurlClientEngine(
     }
 }
 
+/**
+ * Curl provides raw response headers while performing request decoding.
+ * This can lead to an invalid content-length header or trigger the content encoding plugin wrongly.
+ *
+ * We need to filter out the headers that are no longer valid.
+ */
+internal fun filterCurlHeaders(raw: HttpHeadersMap): Headers {
+    val builder = raw.toBuilder()
+
+    if (builder.contains(HttpHeaders.ContentEncoding)) {
+        builder.remove(HttpHeaders.ContentEncoding)
+        builder.remove(HttpHeaders.ContentLength)
+    }
+
+    return builder.build()
+}
+
 @Deprecated("This exception will be removed in a future release in favor of a better error handling.")
 public class CurlIllegalStateException(cause: String) : IllegalStateException(cause)
 

diff --git a/ktor-client/ktor-client-curl/desktop/src/io/ktor/client/engine/curl/ResponseUtils.kt b/ktor-client/ktor-client-curl/desktop/src/io/ktor/client/engine/curl/ResponseUtils.kt
@@ -4,19 +4,18 @@
 
 package io.ktor.client.engine.curl
 
+import io.ktor.http.HeadersBuilder
 import io.ktor.http.cio.*
 
-internal fun HttpHeadersMap.toMap(): Map<String, List<String>> {
-    val result = mutableMapOf<String, MutableList<String>>()
+internal fun HttpHeadersMap.toBuilder(): HeadersBuilder {
+    val builder = HeadersBuilder()
 
     for (index in 0 until size) {
         val key = nameAt(index).toString()
         val value = valueAt(index).toString()
 
-        if (result[key]?.add(value) == null) {
-            result[key] = mutableListOf(value)
-        }
+        builder.append(key, value)
     }
 
-    return result
+    return builder
 }
diff --git a/ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockEngine.kt b/ktor-client/ktor-client-mock/common/src/io/ktor/client/engine/mock/MockEngine.kt
@@ -31,7 +31,7 @@ public class MockEngine(override val config: MockEngineConfig) : HttpClientEngin
     private var invocationCount: Int = 0
 
     init {
-        check(config.requestHandlers.size > 0) {
+        check(config.requestHandlers.isNotEmpty()) {
             "No request handler provided in [MockEngineConfig], please provide at least one."
         }
     }

diff --git a/ktor-client/ktor-client-plugins/ktor-client-encoding/common/src/ContentEncoding.kt b/ktor-client/ktor-client-plugins/ktor-client-encoding/common/src/ContentEncoding.kt
@@ -15,7 +15,7 @@ import io.ktor.util.*
 import io.ktor.util.logging.*
 import io.ktor.util.pipeline.*
 import io.ktor.utils.io.*
-import kotlinx.coroutines.*
+import kotlinx.coroutines.CoroutineScope
 
 private val LOGGER = KtorSimpleLogger("io.ktor.client.plugins.compression.ContentEncoding")
 
@@ -130,7 +130,11 @@ public val ContentEncoding: ClientPlugin<ContentEncodingConfig> =
 
             val headers = headers {
                 response.headers.forEach { name, values ->
-                    if (name.equals(HttpHeaders.ContentEncoding, ignoreCase = true)) return@forEach
+                    if (name.equals(HttpHeaders.ContentEncoding, ignoreCase = true) ||
+                        name.equals(HttpHeaders.ContentLength, ignoreCase = true)
+                    ) {
+                        return@forEach
+                    }
                     appendAll(name, values)
                 }
                 val remainingEncodings = encodings.filter { !encodings.contains(it) }

diff --git a/ktor-client/ktor-client-plugins/ktor-client-encoding/common/test/ContentEncodingTest.kt b/ktor-client/ktor-client-plugins/ktor-client-encoding/common/test/ContentEncodingTest.kt
@@ -96,7 +96,7 @@ class ContentEncodingTest : ClientLoader() {
     }
 
     @Test
-    fun testDisableDecompression() = clientTests(listOf("OkHttp")) {
+    fun testDisableDecompression() = clientTests(listOf("OkHttp", "Js")) {
         config {
             ContentEncoding(mode = ContentEncodingConfig.Mode.CompressRequest) {
                 gzip()

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ContentTest.kt
@@ -145,7 +145,7 @@ class ContentTest : ClientLoader(5 * 60) {
     }
 
     @Test
-    fun testString() = clientTests(listOf("Darwin", "CIO", "DarwinLegacy")) {
+    fun testString() = clientTests(listOf("Darwin", "CIO", "DarwinLegacy"), retries = 10) {
         test { client ->
             testStrings.forEach { content ->
                 val requestWithBody = client.echo<String>(content)

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/DefaultTransformTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/DefaultTransformTest.kt
@@ -9,21 +9,20 @@ import io.ktor.client.call.*
 import io.ktor.client.engine.mock.*
 import io.ktor.client.request.*
 import io.ktor.http.*
-import io.ktor.test.dispatcher.*
+import kotlinx.coroutines.test.runTest
 import kotlin.test.*
 
 class DefaultTransformTest {
 
     @Test
-    fun testReadingHeadResponseAsByteArray() = testSuspend {
-        val httpClient = HttpClient(MockEngine) {
+    fun testReadingHeadResponseAsByteArray() = runTest {
+        val client = HttpClient(MockEngine) {
             engine {
                 addHandler { _ ->
                     respond("", headers = headersOf(HttpHeaders.ContentLength, "123"))
                 }
             }
         }
-
-        httpClient.head("http://host/path").body<ByteArray>()
+        client.head("http://host/path").body<ByteArray>()
     }
 }
diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/LoggingTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/LoggingTest.kt
@@ -498,7 +498,7 @@ class LoggingTest : ClientLoader() {
     data class User(val name: String)
 
     @Test
-    fun testLogPostBodyWithJson() = clientTests {
+    fun testLogPostBodyWithJson() = clientTests(retries = 5) {
         val testLogger = TestLogger(
             "REQUEST: http://127.0.0.1:8080/content/echo",
             "METHOD: HttpMethod(value=POST)",

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/MockedTests.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/MockedTests.kt
@@ -100,6 +100,44 @@ class MockedTests {
                 .body<Unit>()
         }
     }
+
+    @Test
+    fun testContentLengthIsCheckedForByteArray() = testWithEngine(MockEngine) {
+        config {
+            engine {
+                addHandler { request ->
+                    respond("hello", headers = headersOf(HttpHeaders.ContentLength, "123"))
+                }
+            }
+        }
+
+        test { client ->
+            assertFailsWith<IllegalStateException> {
+                client.prepareGet(Url("http://host")) {
+                    url.path("path")
+                }.execute { response ->
+                    response.body<ByteArray>()
+                }
+            }
+        }
+    }
+
+    @Test
+    fun testContentLengthIsChecked() = testWithEngine(MockEngine) {
+        config {
+            engine {
+                addHandler { request ->
+                    respond("hello", headers = headersOf(HttpHeaders.ContentLength, "123"))
+                }
+            }
+        }
+
+        test { client ->
+            assertFailsWith<IllegalStateException> {
+                client.get("https://host/path").body<String>()
+            }
+        }
+    }
 }
 
 @Serializable

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/PluginsTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/PluginsTest.kt
@@ -17,7 +17,7 @@ class PluginsTest : ClientLoader() {
     private val testSize = listOf(0, 1, 1024, 4 * 1024, 16 * 1024, 16 * 1024 * 1024)
 
     @Test
-    fun testIgnoreBody() = clientTests {
+    fun testIgnoreBody() = clientTests(retries = 10) {
         test { client ->
             testSize.forEach {
                 client.getIgnoringBody(it)

diff --git a/...ent/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CookiesIntegrationTests.kt b/...ent/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CookiesIntegrationTests.kt
@@ -186,7 +186,7 @@ class CookiesIntegrationTests : ClientLoader() {
     }
 
     @Test
-    fun testCookiesWithWrongValue() = clientTests(listOf("Js", "Darwin", "DarwinLegacy", "WinHttp", "Java")) {
+    fun testCookiesWithWrongValue() = clientTests(listOf("Js", "Darwin", "DarwinLegacy", "WinHttp", "Java", "Curl")) {
         config {
             install(HttpCookies)
         }