Skip to content

Ktor 8345: Make VaryHeader check lenient #4816

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Ktor 8345: Make VaryHeader check lenient #4816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
86f93d3
Remove vary header size check in storage
StefMa Apr 29, 2025
d07d61b
Check varyKeys size in HttpCache
StefMa Apr 29, 2025
bd6e2d0
Adjust test
StefMa Apr 29, 2025
5ed9cce
Fix for nullable cache
StefMa Apr 29, 2025
6a65e42
Fix logger call
StefMa Apr 29, 2025
9b49d6b
Fix return statement
StefMa Apr 29, 2025
5eda066
Remove check in CacheStorages. Fix duplicate cache responses
marychatte May 27, 2025
e364c82
Merge branch 'main' into KTOR-8345-lenient-vary-header-check
marychatte May 27, 2025
69d2e61
Fix HttpCacheLegacy, fix codestyle
marychatte May 28, 2025
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
17 changes: 13 additions & 4 deletions ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,17 @@ public class HttpCache private constructor(
plugin.findAndRefresh(response.call.request, response) ?: throw InvalidCacheStateException(
response.call.request.url
)
if (responseFromCache.varyKeys().size != response.varyKeys().size) {
LOGGER.warn(
"Vary header mismatch on cached response for ${response.call.request.url}. " +
"Received 304 Not Modified with Vary: ${response.varyKeys()} " +
"but cached response has Vary: ${responseFromCache.varyKeys()}. " +
"According to RFC 7232 §4.1 and RFC 9111 §4.1, " +
"the server must include the full Vary header in 304 responses. " +
"Falling back to missing cache logic. " +
"Consider reporting this issue to the server maintainers."
)
}

scope.monitor.raise(HttpResponseFromCache, responseFromCache)
proceedWith(responseFromCache)
Expand Down Expand Up @@ -329,10 +340,8 @@ public class HttpCache private constructor(
else -> publicStorageNew
}

val varyKeysFrom304 = response.varyKeys()
val cache = findResponse(storage, varyKeysFrom304, url, request) ?: return null
val newVaryKeys = varyKeysFrom304.ifEmpty { cache.varyKeys }
storage.store(request.url, cache.copy(newVaryKeys, response.cacheExpires(isSharedClient)))
val cache = findResponse(storage, response.varyKeys(), url, request) ?: return null
storage.store(request.url, cache.copy(cache.varyKeys, response.cacheExpires(isSharedClient)))
return cache.createResponse(request.call.client, request, response.coroutineContext)
}

Expand Down
20 changes: 16 additions & 4 deletions ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ internal suspend fun PipelineContext<HttpResponse, Unit>.interceptReceiveLegacy(
if (response.status == HttpStatusCode.NotModified) {
val responseFromCache = plugin.findAndRefresh(response.call.request, response)
?: throw InvalidCacheStateException(response.call.request.url)
if (responseFromCache.varyKeys().size != response.varyKeys().size) {
LOGGER.warn(
"Vary header mismatch on cached response for ${response.call.request.url}. " +
"Received 304 Not Modified with Vary: ${response.varyKeys()} " +
"but cached response has Vary: ${responseFromCache.varyKeys()}. " +
"According to RFC 7232 §4.1 and RFC 9111 §4.1, " +
"the server must include the full Vary header in 304 responses. " +
"Falling back to missing cache logic. " +
"Consider reporting this issue to the server maintainers."
)
}
Comment on lines +69 to +79
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inconsistent warning message about fallback behavior.

The warning message states "Falling back to missing cache logic" but the code actually continues to use the cached response (proceedWith(responseFromCache) on line 82). This is misleading as no fallback occurs - the implementation proceeds with the cached response despite the Vary header mismatch.

Consider updating the warning message to accurately reflect the actual behavior:

-                    "Falling back to missing cache logic. " +
+                    "Proceeding with cached response despite mismatch. " +

The RFC references and overall warning structure are excellent and provide clear guidance to developers.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (responseFromCache.varyKeys().size != response.varyKeys().size) {
LOGGER.warn(
"Vary header mismatch on cached response for ${response.call.request.url}. " +
"Received 304 Not Modified with Vary: ${response.varyKeys()} " +
"but cached response has Vary: ${responseFromCache.varyKeys()}. " +
"According to RFC 7232 §4.1 and RFC 9111 §4.1, " +
"the server must include the full Vary header in 304 responses. " +
"Falling back to missing cache logic. " +
"Consider reporting this issue to the server maintainers."
)
}
if (responseFromCache.varyKeys().size != response.varyKeys().size) {
LOGGER.warn(
"Vary header mismatch on cached response for ${response.call.request.url}. " +
"Received 304 Not Modified with Vary: ${response.varyKeys()} " +
"but cached response has Vary: ${responseFromCache.varyKeys()}. " +
"According to RFC 7232 §4.1 and RFC 9111 §4.1, " +
"the server must include the full Vary header in 304 responses. " +
"Proceeding with cached response despite mismatch. " +
"Consider reporting this issue to the server maintainers."
)
}
🤖 Prompt for AI Agents 
In
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt
around lines 69 to 79, the warning message incorrectly states "Falling back to
missing cache logic" while the code actually proceeds with the cached response
despite the Vary header mismatch. Update the warning message to accurately
reflect that the cached response is still being used despite the mismatch,
removing the misleading fallback statement to avoid confusion.


scope.monitor.raise(HttpCache.HttpResponseFromCache, responseFromCache)
proceedWith(responseFromCache)
Expand Down Expand Up @@ -116,10 +127,11 @@ private fun HttpCache.findAndRefresh(request: HttpRequest, response: HttpRespons

val storage = if (CacheControl.PRIVATE in cacheControl) privateStorage else publicStorage

val varyKeysFrom304 = response.varyKeys()
val cache = findResponse(storage, varyKeysFrom304, url, request) ?: return null
val newVaryKeys = varyKeysFrom304.ifEmpty { cache.varyKeys }
storage.store(url, HttpCacheEntry(response.cacheExpires(isSharedClient), newVaryKeys, cache.response, cache.body))
val cache = findResponse(storage, response.varyKeys(), url, request) ?: return null
storage.store(
url,
HttpCacheEntry(response.cacheExpires(isSharedClient), cache.varyKeys, cache.response, cache.body)
)
return cache.produceResponse()
}

Expand Down
4 changes: 2 additions & 2 deletions ...ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal class UnlimitedCacheStorage : HttpCacheStorage() {
override fun find(url: Url, varyKeys: Map<String, String>): HttpCacheEntry? {
val data = store.computeIfAbsent(url) { ConcurrentSet() }
return data.find {
varyKeys.all { (key, value) -> it.varyKeys[key] == value } && varyKeys.size == it.varyKeys.size
varyKeys.all { (key, value) -> it.varyKeys[key] == value }
}
}

Expand All @@ -45,7 +45,7 @@ internal class UnlimitedStorage : CacheStorage {
override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? {
val data = store.computeIfAbsent(url) { ConcurrentSet() }
return data.find {
varyKeys.all { (key, value) -> it.varyKeys[key] == value } && varyKeys.size == it.varyKeys.size
varyKeys.all { (key, value) -> it.varyKeys[key] == value }
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ internal class CachingCacheStorage(
}
val data = store.getValue(url)
return data.find {
varyKeys.all { (key, value) -> it.varyKeys[key] == value } && varyKeys.size == it.varyKeys.size
varyKeys.all { (key, value) -> it.varyKeys[key] == value }
}
}

Expand Down Expand Up @@ -83,7 +83,7 @@ private class FileCacheStorage(
override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? {
val data = readCache(key(url))
return data.find {
varyKeys.all { (key, value) -> it.varyKeys[key] == value } && varyKeys.size == it.varyKeys.size
varyKeys.all { (key, value) -> it.varyKeys[key] == value }
}
}

Expand Down
2 changes: 1 addition & 1 deletion ktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ private class InMemoryCacheStorage : CacheStorage {
findCalledCount++
val cache = store.computeIfAbsent(url) { mutableSetOf() }
return cache.find {
varyKeys.all { (key, value) -> it.varyKeys[key] == value } && varyKeys.size == it.varyKeys.size
varyKeys.all { (key, value) -> it.varyKeys[key] == value }
}
}

Expand Down
13 changes: 6 additions & 7 deletions ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,15 +846,14 @@ class CacheTest : ClientLoader() {
}

test { client ->
client.get("$TEST_SERVER/cache/different-vary") {
val url = "$TEST_SERVER/cache/different-vary"
val first = client.get(url) {
header("200", "true")
header("Set-Vary", "X-Requested-With,Accept-Encoding")
}
assertFailsWith<InvalidCacheStateException> {
client.get("$TEST_SERVER/cache/different-vary") {
header("Set-Vary", "X-Requested-With")
}
}
val second = client.get(url)

assertEquals(first.bodyAsText(), second.bodyAsText())
assertEquals(storage.findAll(Url("$TEST_SERVER/cache/different-vary")).size, 1)
}
}

Expand Down
4 changes: 2 additions & 2 deletions ktor-test-server/src/main/kotlin/test/server/tests/Cache.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ internal fun Application.cacheTestServer() {
}
get("/different-vary") {
if (call.request.headers.contains("200")) {
call.response.header("Vary", "X-Requested-With,Accept-Encoding")
call.respond(HttpStatusCode.OK)
call.response.header("Vary", "X-Requested-With, Accept-Encoding")
call.respondText { "Ok" }
} else {
call.response.header("Vary", "X-Requested-With")
call.respond(HttpStatusCode.NotModified)
Expand Down