From 3c7a1abbe78c6aa1638f4458c196cb1324549d85 Mon Sep 17 00:00:00 2001 From: Schabi Date: Tue, 10 Nov 2020 11:25:33 +0100 Subject: [PATCH] fix wrong handling of redirect to unsecure connection --- .../lib/resources/status/StatusRequester.kt | 21 ++++++---- .../android/lib/StatusRequestorTest.kt | 39 ++++++++++++++++--- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt index 2dbb3653..da13fa5c 100644 --- a/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt +++ b/owncloudComLibrary/src/main/java/com/owncloud/android/lib/resources/status/StatusRequester.kt @@ -28,6 +28,7 @@ import com.owncloud.android.lib.common.OwnCloudClient import com.owncloud.android.lib.common.http.HttpConstants import com.owncloud.android.lib.common.http.methods.nonwebdav.GetMethod import com.owncloud.android.lib.common.operations.RemoteOperationResult + import com.owncloud.android.lib.resources.status.HttpScheme.HTTPS_SCHEME import com.owncloud.android.lib.resources.status.HttpScheme.HTTP_SCHEME import org.json.JSONObject @@ -36,14 +37,20 @@ import java.util.concurrent.TimeUnit internal class StatusRequester { - private fun checkIfConnectionIsRedirectedToNoneSecure( - isConnectionSecure: Boolean, + /** + * This function is ment to detect if a redirect from a secure to an unsecure connection + * was made. If only connections from unsecure connections to unsecure connections were made + * this function should not return true, because if the whole redirect chain was unsecure + * we assume it was a debug setup. + */ + fun isRedirectedToNonSecureConnection( + redirectedToUnsecureLocationBefore: Boolean, baseUrl: String, redirectedUrl: String - ): Boolean { - return isConnectionSecure || - (baseUrl.startsWith(HTTPS_SCHEME) && redirectedUrl.startsWith(HTTP_SCHEME)) - } + ) = redirectedToUnsecureLocationBefore + || (baseUrl.startsWith(HTTPS_SCHEME) + && (!redirectedUrl.startsWith(HTTPS_SCHEME)) + && redirectedUrl.startsWith(HTTP_SCHEME)) fun updateLocationWithRedirectPath(oldLocation: String, redirectedLocation: String): String { if (!redirectedLocation.startsWith("/")) @@ -84,7 +91,7 @@ internal class StatusRequester { } else { val nextLocation = updateLocationWithRedirectPath(currentLocation, result.redirectedLocation) redirectedToUnsecureLocation = - checkIfConnectionIsRedirectedToNoneSecure( + isRedirectedToNonSecureConnection( redirectedToUnsecureLocation, currentLocation, nextLocation diff --git a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt b/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt index a4e3de0e..adf4d3ea 100644 --- a/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt +++ b/owncloudComLibrary/src/test/java/com/owncloud/android/lib/StatusRequestorTest.kt @@ -26,39 +26,68 @@ package com.owncloud.android.lib import com.owncloud.android.lib.resources.status.StatusRequester import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test class StatusRequestorTest { - private val requestor = StatusRequester() + private val requester = StatusRequester() @Test fun `update location - ok - absolute path`() { - val newLocation = requestor.updateLocationWithRedirectPath(TEST_DOMAIN, "$TEST_DOMAIN$SUB_PATH") + val newLocation = requester.updateLocationWithRedirectPath(TEST_DOMAIN, "$TEST_DOMAIN$SUB_PATH") assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } @Test fun `update location - ok - smaller absolute path`() { - val newLocation = requestor.updateLocationWithRedirectPath("$TEST_DOMAIN$SUB_PATH", TEST_DOMAIN) + val newLocation = requester.updateLocationWithRedirectPath("$TEST_DOMAIN$SUB_PATH", TEST_DOMAIN) assertEquals(TEST_DOMAIN, newLocation) } @Test fun `update location - ok - relative path`() { - val newLocation = requestor.updateLocationWithRedirectPath(TEST_DOMAIN, SUB_PATH) + val newLocation = requester.updateLocationWithRedirectPath(TEST_DOMAIN, SUB_PATH) assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } @Test fun `update location - ok - replace relative path`() { - val newLocation = requestor.updateLocationWithRedirectPath( + val newLocation = requester.updateLocationWithRedirectPath( "$TEST_DOMAIN/some/other/subdir", SUB_PATH ) assertEquals("$TEST_DOMAIN$SUB_PATH", newLocation) } + @Test + fun `check redirect to unsecure connection - ok - redirect to http`() { + assertTrue(requester.isRedirectedToNonSecureConnection( + false, SECURE_DOMAIN, UNSECURE_DOMAIN)) + } + + @Test + fun `check redirect to unsecure connection - ko - redirect to https from http`() { + assertFalse(requester.isRedirectedToNonSecureConnection( + false, UNSECURE_DOMAIN, SECURE_DOMAIN)) + } + + @Test + fun `check redirect to unsecure connection - ko - from https to https`() { + assertFalse(requester.isRedirectedToNonSecureConnection( + false, SECURE_DOMAIN, SECURE_DOMAIN)) + } + + @Test + fun `check redirect to unsecure connection - ok - from https to https with previous http`() { + assertTrue(requester.isRedirectedToNonSecureConnection( + true, SECURE_DOMAIN, SECURE_DOMAIN)) + } + companion object { const val TEST_DOMAIN = "https://cloud.somewhere.com" const val SUB_PATH = "/subdir" + + const val SECURE_DOMAIN = "https://cloud.somewhere.com" + const val UNSECURE_DOMAIN = "http://somewhereelse.org" } }