KTOR-3159 Allow secure cookie with unencrypted response (#3932)

ktorio · Mar 1, 2024 · 722950d · 722950d
1 parent 515472f
commit 722950d
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 32 deletions.
diff --git a/ktor-server/ktor-server-core/jvmAndNix/src/io/ktor/server/response/ResponseCookies.kt b/ktor-server/ktor-server-core/jvmAndNix/src/io/ktor/server/response/ResponseCookies.kt
@@ -13,7 +13,6 @@ import io.ktor.util.date.*
  */
 public class ResponseCookies(
     private val response: ApplicationResponse,
-    private val secureTransport: Boolean
 ) {
     /**
      * Gets a cookie from a response's `Set-Cookie` header.
@@ -27,9 +26,6 @@ public class ResponseCookies(
      * Appends a cookie [item] using the `Set-Cookie` response header.
      */
     public fun append(item: Cookie) {
-        if (item.secure && !secureTransport) {
-            throw IllegalArgumentException("You should set secure cookie only via secure transport (HTTPS)")
-        }
         response.headers.append("Set-Cookie", renderSetCookieHeader(item))
     }
 

diff --git a/...er/ktor-server-host-common/jvmAndNix/src/io/ktor/server/engine/BaseApplicationResponse.kt b/...er/ktor-server-host-common/jvmAndNix/src/io/ktor/server/engine/BaseApplicationResponse.kt
@@ -30,7 +30,7 @@ public abstract class BaseApplicationResponse(
         private set
 
     override val cookies: ResponseCookies by lazy {
-        ResponseCookies(this, call.request.origin.scheme == "https" || call.request.origin.scheme == "wss")
+        ResponseCookies(this)
     }
 
     override fun status(): HttpStatusCode? = _status

diff --git a/ktor-server/ktor-server-tests/jvmAndNix/test/io/ktor/tests/server/plugins/CookiesTest.kt b/ktor-server/ktor-server-tests/jvmAndNix/test/io/ktor/tests/server/plugins/CookiesTest.kt
@@ -4,6 +4,7 @@
 
 package io.ktor.tests.server.plugins
 
+import io.ktor.client.request.*
 import io.ktor.http.*
 import io.ktor.server.application.*
 import io.ktor.server.response.*
@@ -120,36 +121,21 @@ class CookiesTest {
         }
     }
 
+    /*
+     * We do not enforce that Secure flag matches protocol, because we could be behind a proxy.  There are other ways
+     * to ensure that secure cookies are not vulnerable to man-in-the-middle attacks, but we leave this to the
+     * developer.
+     */
     @Test
-    fun testSecureCookieHttp() {
-        withTestApplication {
-            application.routing {
-                get("/*") {
-                    call.response.cookies.append("S", "secret", secure = true)
-                    call.respond("ok")
-                }
-            }
-
-            assertFails {
-                handleRequest(HttpMethod.Get, "/1")
-            }
-        }
-    }
-
-    @Test
-    fun testSecureCookieHttps() {
-        withTestApplication {
-            application.routing {
-                get("/*") {
-                    call.response.cookies.append("S", "secret", secure = true)
-                    call.respond("ok")
-                }
-            }
-
-            handleRequest(HttpMethod.Get, "/2") {
-                protocol = "https"
+    fun testSecureCookie() = testApplication {
+        routing {
+            get("/*") {
+                call.response.cookies.append("S", "secret", secure = true)
+                call.respond("ok")
             }
         }
+        val response = client.get("/cookie-monster")
+        assertEquals("S=secret; Secure", response.headers["Set-Cookie"]?.cutSetCookieHeader())
     }
 
     private fun testSetCookies(expectedHeaderContent: String, block: ApplicationResponse.() -> Unit) {