Don't send Authorization after redirect

Fix #1467
ktorio · Dec 5, 2019 · 0c10815 · 0c10815
1 parent 5077046
commit 0c10815
Show file tree

Hide file tree

Showing 6 changed files with 90 additions and 21 deletions.
diff --git a/gradle.properties b/gradle.properties
@@ -11,7 +11,6 @@ org.gradle.parallel=true
 # gradle
 org.gradle.daemon=true
 org.gradle.jvmargs=-Xmx2g -XX:MaxPermSize=2048m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8
-org.jetbrains.kotlin.native.jvmArgs=-XX:TieredStopAtLevel=1
 
 # kotlin
 kotlin_version=1.3.61

diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/HttpClient.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/HttpClient.kt
@@ -123,11 +123,15 @@ class HttpClient(
                 config.install("DefaultTransformers") { defaultTransformers() }
             }
 
-            if (expectSuccess) config.addDefaultResponseValidation()
+            if (expectSuccess) {
+                config.addDefaultResponseValidation()
+            }
 
             config.install(HttpSend)
 
-            if (followRedirects) config.install(HttpRedirect)
+            if (followRedirects) {
+                config.install(HttpRedirect)
+            }
 
             config += this
             config.install(this@HttpClient)

diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/features/HttpRedirect.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/features/HttpRedirect.kt
@@ -7,22 +7,39 @@ package io.ktor.client.features
 import io.ktor.client.*
 import io.ktor.client.call.*
 import io.ktor.client.request.*
-import io.ktor.client.statement.*
 import io.ktor.http.*
 import io.ktor.util.*
-import kotlinx.coroutines.*
+import kotlin.jvm.*
+import kotlin.native.concurrent.*
+
+@ThreadLocal
+private val ALLOWED_FOR_REDIRECT: Set<HttpMethod> = setOf(HttpMethod.Get, HttpMethod.Head)
 
 /**
  * [HttpClient] feature that handles http redirect
  */
 class HttpRedirect {
+    /**
+     * Check if the HTTP method is allowed for redirect.
+     * Only [HttpMethod.Get] and [HttpMethod.Head] is allowed for implicit redirect.
+     *
+     * Please note: changing this flag could lead to security issues, consider changing the request URL instead.
+     */
+    @KtorExperimentalAPI
+    @Volatile
+    var checkHttpMethod: Boolean = true
+
     companion object Feature : HttpClientFeature<Unit, HttpRedirect> {
         override val key: AttributeKey<HttpRedirect> = AttributeKey("HttpRedirect")
 
         override fun prepare(block: Unit.() -> Unit): HttpRedirect = HttpRedirect()
 
         override fun install(feature: HttpRedirect, scope: HttpClient) {
             scope.feature(HttpSend)!!.intercept { origin ->
+                if (feature.checkHttpMethod && origin.request.method !in ALLOWED_FOR_REDIRECT) {
+                    return@intercept origin
+                }
+
                 handleCall(origin)
             }
         }
@@ -31,13 +48,26 @@ class HttpRedirect {
             if (!origin.response.status.isRedirect()) return origin
 
             var call = origin
+            val originProtocol = origin.request.url.protocol
+            val originAuthority = origin.request.url.authority
             while (true) {
                 val location = call.response.headers[HttpHeaders.Location]
 
                 val requestBuilder = HttpRequestBuilder().apply {
                     takeFrom(origin.request)
                     url.parameters.clear()
 
+                    /**
+                     * Disallow redirect with a security downgrade.
+                     */
+                    if (originProtocol.isSecure() && !url.protocol.isSecure()) {
+                        return call
+                    }
+
+                    if (originAuthority != url.authority) {
+                        headers.remove(HttpHeaders.Authorization)
+                    }
+
                     location?.let { url.takeFrom(it) }
                 }
 

diff --git a/ktor-client/ktor-client-tests/build.gradle.kts b/ktor-client/ktor-client-tests/build.gradle.kts
@@ -48,6 +48,7 @@ kotlin.sourceSets {
     commonTest {
         dependencies {
             api(project(":ktor-client:ktor-client-features:ktor-client-logging"))
+            api(project(":ktor-client:ktor-client-features:ktor-client-auth"))
         }
     }
     jvmMain {

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt
@@ -16,7 +16,7 @@ class HttpRedirectTest : ClientLoader() {
     private val TEST_URL_BASE = "$TEST_SERVER/redirect"
 
     @Test
-    fun redirectTest() = clientTests {
+    fun testRedirect() = clientTests {
         config {
             install(HttpRedirect)
         }
@@ -30,7 +30,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun infinityRedirectTest() = clientTests {
+    fun testInfinityRedirect() = clientTests {
         config {
             install(HttpRedirect)
         }
@@ -43,7 +43,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun redirectWithCookiesTest() = clientTests(listOf("js")) {
+    fun testRedirectWithCookies() = clientTests(listOf("js")) {
         config {
             install(HttpCookies)
             install(HttpRedirect)
@@ -59,7 +59,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun customUrlsTest() = clientTests {
+    fun testCustomUrls() = clientTests {
         val urls = listOf(
             "https://files.forgecdn.net/files/2574/880/BiblioCraft[v2.4.5][MC1.12.2].jar",
             "https://files.forgecdn.net/files/2611/560/Botania r1.10-356.jar",
@@ -81,16 +81,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun httpStatsTest() = clientTests {
-        test { client ->
-            client.get<HttpStatement>("https://httpstat.us/301").execute { response ->
-                assertEquals(HttpStatusCode.OK, response.status)
-            }
-        }
-    }
-
-    @Test
-    fun redirectRelative() = clientTests {
+    fun testRedirectRelative() = clientTests {
         test { client ->
             client.get<HttpStatement>("$TEST_URL_BASE/directory/redirectFile").execute {
                 assertEquals("targetFile", it.readText())
@@ -99,7 +90,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun redirectAbsolute() = clientTests {
+    fun testRedirectAbsolute() = clientTests {
         test { client ->
             client.get<HttpStatement>("$TEST_URL_BASE/directory/absoluteRedirectFile").execute {
                 assertEquals("absoluteTargetFile", it.readText())
@@ -108,7 +99,7 @@ class HttpRedirectTest : ClientLoader() {
     }
 
     @Test
-    fun redirectHostAbsolute() = clientTests(listOf("js")) {
+    fun testRedirectHostAbsolute() = clientTests(listOf("js")) {
         test { client ->
             client.get<HttpStatement>("$TEST_URL_BASE/directory/hostAbsoluteRedirect").execute {
                 assertEquals("200 OK", it.readText())

diff --git a/ktor-http/common/src/io/ktor/http/URLBuilder.kt b/ktor-http/common/src/io/ktor/http/URLBuilder.kt
@@ -160,3 +160,47 @@ data class Url(
 
     companion object
 }
+
+/**
+ * [Url] authority.
+ */
+val Url.authority: String
+    get() = buildString {
+        user?.let {
+            append(it)
+        }
+        password?.let {
+            append(':')
+            append(it)
+        }
+
+        if (isNotEmpty()) {
+            append('@')
+        }
+
+        append(host)
+        append(':')
+        append(port)
+    }
+
+/**
+ * [URLBuilder] authority.
+ */
+val URLBuilder.authority: String
+    get() = buildString {
+        user?.let {
+            append(it)
+        }
+        password?.let {
+            append(':')
+            append(it)
+        }
+
+        if (isNotEmpty()) {
+            append('@')
+        }
+
+        append(host)
+        append(':')
+        append(port)
+    }