Fix redirect handling in KtorImageFetcher (#859)

gradle/libs.versions.toml

@@ -84,6 +84,7 @@ ktor-okhttp = { group = "io.ktor", name = "ktor-client-okhttp", version.ref = "k
 ktor-engine-darwin = { module = "io.ktor:ktor-client-darwin", version.ref = "ktor" }
 ktor-engine-js = { module = "io.ktor:ktor-client-js", version.ref = "ktor" }
 ktor-engine-cio = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" }
+ktor-mock = { group = "io.ktor", name = "ktor-client-mock", version.ref = "ktor" }
 okio = { group = "com.squareup.okio", name = "okio", version.ref = "okio" }
 atomicfu = { group = "org.jetbrains.kotlinx", name = "atomicfu", version.ref = "atomicfu" }
 kotlinx-serialization-json = { group = "org.jetbrains.kotlinx", name = "kotlinx-serialization-json", version.ref = "kotlinxSerializationJson" }

landscapist-core/build.gradle.kts

@@ -70,6 +70,7 @@ kotlin {
       dependencies {
         implementation(kotlin("test"))
         implementation(libs.kotlinx.coroutines.test)
+        implementation(libs.ktor.mock)
       }
     }
 

landscapist-core/src/commonMain/kotlin/com/skydoves/landscapist/core/network/KtorImageFetcher.kt

@@ -19,8 +19,10 @@ import com.skydoves.landscapist.core.ImageRequest
 import com.skydoves.landscapist.core.NetworkConfig
 import com.skydoves.landscapist.core.model.DataSource
 import io.ktor.client.HttpClient
+import io.ktor.client.HttpClientConfig
 import io.ktor.client.plugins.HttpSend
 import io.ktor.client.plugins.HttpTimeout
+import io.ktor.client.plugins.cookies.HttpCookies
 import io.ktor.client.request.get
 import io.ktor.client.request.headers
 import io.ktor.client.statement.bodyAsBytes
@@ -113,20 +115,44 @@ public class KtorImageFetcher(
      */
     public fun create(networkConfig: NetworkConfig = NetworkConfig()): KtorImageFetcher {
       val httpClient = HttpClient {
-        install(HttpTimeout) {
-          connectTimeoutMillis = networkConfig.connectTimeout.inWholeMilliseconds
-          requestTimeoutMillis = networkConfig.readTimeout.inWholeMilliseconds
-        }
-        install(HttpSend) {
-          maxSendCount = networkConfig.maxRedirects
-        }
-        followRedirects = networkConfig.followRedirects
+        configureForImageLoading(networkConfig)
       }
       return KtorImageFetcher(httpClient, networkConfig)
     }
   }
 }
 
+/**
+ * Applies the shared [HttpClient] configuration used for image loading.
+ *
+ * Kept as a standalone extension so both [KtorImageFetcher.create] and tests configure the client
+ * identically, regardless of the underlying engine.
+ *
+ * @param networkConfig Network configuration settings.
+ */
+internal fun HttpClientConfig<*>.configureForImageLoading(networkConfig: NetworkConfig) {
+  install(HttpTimeout) {
+    connectTimeoutMillis = networkConfig.connectTimeout.inWholeMilliseconds
+    requestTimeoutMillis = networkConfig.readTimeout.inWholeMilliseconds
+  }
+
+  // Persist cookies across redirect hops. Some CDNs set a cookie on the first response and
+  // redirect to a target that requires it; without a cookie store the target keeps redirecting,
+  // producing an endless loop that only ends when the send-count limit is hit. Browsers avoid this
+  // because they retain cookies automatically.
+  // See https://github.com/skydoves/landscapist/issues/859
+  install(HttpCookies)
+
+  install(HttpSend) {
+    // maxSendCount is the total number of times a request may be dispatched (the original request
+    // plus every redirect and retry), not the number of redirects. Allow the original send plus
+    // maxRedirects redirect hops so a request that legitimately redirects never trips the limit.
+    maxSendCount = networkConfig.maxRedirects + 1
+  }
+
+  followRedirects = networkConfig.followRedirects
+}
+
 /**
  * Exception for HTTP errors.
  *

landscapist-core/src/commonTest/kotlin/com/skydoves/landscapist/core/network/KtorImageFetcherTest.kt

@@ -0,0 +1,125 @@
+/*
+ * Designed and developed by 2020-2023 skydoves (Jaewoong Eum)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.skydoves.landscapist.core.network
+
+import com.skydoves.landscapist.core.ImageRequest
+import com.skydoves.landscapist.core.NetworkConfig
+import io.ktor.client.HttpClient
+import io.ktor.client.engine.mock.MockEngine
+import io.ktor.client.engine.mock.respond
+import io.ktor.client.plugins.SendCountExceedException
+import io.ktor.http.HttpHeaders
+import io.ktor.http.HttpStatusCode
+import io.ktor.http.headersOf
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.test.runTest
+import kotlinx.coroutines.withContext
+import kotlin.test.Test
+import kotlin.test.assertContentEquals
+import kotlin.test.assertTrue
+
+class KtorImageFetcherTest {
+
+  /**
+   * Fetches on a real dispatcher so Ktor's [io.ktor.client.plugins.HttpTimeout] uses wall-clock
+   * time. Under runTest's virtual clock the timeout delay would otherwise be advanced and fire
+   * before the (instant) mock response arrives.
+   */
+  private suspend fun fetchImage(
+    engine: MockEngine,
+    config: NetworkConfig,
+    url: String,
+  ): FetchResult = withContext(Dispatchers.Default) {
+    val client = HttpClient(engine) { configureForImageLoading(config) }
+    KtorImageFetcher(client, config).fetch(ImageRequest(model = url))
+  }
+
+  /**
+   * Regression test for https://github.com/skydoves/landscapist/issues/859.
+   *
+   * The endpoint only serves the image once a cookie set by its own redirect is present. Without a
+   * cookie store the redirect target keeps redirecting, exhausting the send-count limit and failing
+   * with SendCountExceedException. Installing [io.ktor.client.plugins.cookies.HttpCookies] retains
+   * the cookie across the redirect hop, matching browser behavior, so the image resolves.
+   */
+  @Test
+  fun cookieGatedRedirectResolvesInsteadOfLooping() = runTest {
+    val imageBytes = byteArrayOf(0x1, 0x2, 0x3, 0x4)
+    val engine = MockEngine { request ->
+      if (request.headers[HttpHeaders.Cookie]?.contains("session=granted") == true) {
+        respond(
+          content = imageBytes,
+          status = HttpStatusCode.OK,
+          headers = headersOf(HttpHeaders.ContentType, "image/png"),
+        )
+      } else {
+        respond(
+          content = "",
+          status = HttpStatusCode.Found,
+          headers = headersOf(
+            HttpHeaders.Location to listOf("https://cdn.example.com/image.png"),
+            HttpHeaders.SetCookie to listOf("session=granted; Path=/"),
+          ),
+        )
+      }
+    }
+
+    val result = fetchImage(engine, NetworkConfig(), "https://cdn.example.com/image.png")
+
+    assertTrue(result is FetchResult.Success, "expected success but was $result")
+    assertContentEquals(imageBytes, result.data)
+  }
+
+  /** A redirect chain no longer than [NetworkConfig.maxRedirects] hops is followed to success. */
+  @Test
+  fun redirectChainWithinMaxRedirectsSucceeds() = runTest {
+    val imageBytes = byteArrayOf(0x5, 0x6)
+    // maxRedirects = 2 permits: /1 -> /2 -> /ok (two redirect hops).
+    val engine = MockEngine { request ->
+      when (request.url.encodedPath) {
+        "/1" -> respond("", HttpStatusCode.Found, headersOf(HttpHeaders.Location, "https://h/2"))
+        "/2" -> respond("", HttpStatusCode.Found, headersOf(HttpHeaders.Location, "https://h/ok"))
+        else -> respond(
+          content = imageBytes,
+          status = HttpStatusCode.OK,
+          headers = headersOf(HttpHeaders.ContentType, "image/png"),
+        )
+      }
+    }
+
+    val result = fetchImage(engine, NetworkConfig(maxRedirects = 2), "https://h/1")
+
+    assertTrue(result is FetchResult.Success, "expected success but was $result")
+    assertContentEquals(imageBytes, result.data)
+  }
+
+  /** A redirect chain longer than [NetworkConfig.maxRedirects] fails instead of looping forever. */
+  @Test
+  fun redirectChainBeyondMaxRedirectsFails() = runTest {
+    // Always redirect: the send-count limit must stop it rather than looping indefinitely.
+    val engine = MockEngine {
+      respond("", HttpStatusCode.Found, headersOf(HttpHeaders.Location, "https://h/loop"))
+    }
+
+    val result = fetchImage(engine, NetworkConfig(maxRedirects = 2), "https://h/start")
+
+    assertTrue(result is FetchResult.Error, "expected error but was $result")
+    assertTrue(
+      result.throwable is SendCountExceedException,
+      "expected SendCountExceedException but was ${result.throwable}",
+    )
+  }
+}