diff --git a/app/src/main/java/com/nextcloud/talk/dagger/modules/RestModule.java b/app/src/main/java/com/nextcloud/talk/dagger/modules/RestModule.java index c70bafc1bd6..b44f7f7a101 100644 --- a/app/src/main/java/com/nextcloud/talk/dagger/modules/RestModule.java +++ b/app/src/main/java/com/nextcloud/talk/dagger/modules/RestModule.java @@ -55,7 +55,6 @@ import okhttp3.Request; import okhttp3.Response; import okhttp3.Route; -import okhttp3.internal.tls.OkHostnameVerifier; import retrofit2.Retrofit; import retrofit2.adapter.rxjava2.RxJava2CallAdapterFactory; @@ -198,7 +197,8 @@ OkHttpClient provideHttpClient(Proxy proxy, AppPreferences appPreferences, // Trust own CA and all self-signed certs httpClient.sslSocketFactory(sslSocketFactoryCompat, trustManager); httpClient.retryOnConnectionFailure(true); - httpClient.hostnameVerifier(trustManager.getHostnameVerifier(OkHostnameVerifier.INSTANCE)); + // okhttp's default verifier (OkHostnameVerifier), read through public API instead of okhttp3.internal + httpClient.hostnameVerifier(trustManager.getHostnameVerifier(new OkHttpClient().hostnameVerifier())); httpClient.dispatcher(dispatcher); if (!Proxy.NO_PROXY.equals(proxy)) { diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.kt index 9accbc85499..c6396692ef7 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepositoryTest.kt @@ -55,6 +55,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +import org.mockito.kotlin.after import org.mockito.kotlin.any import org.mockito.kotlin.argumentCaptor import org.mockito.kotlin.atLeastOnce @@ -259,7 +260,9 @@ class OfflineFirstChatRepositoryTest { advanceTimeBy(INSURANCE_DELAY_MS * 5 + 1) runCurrent() - verifyBlocking(network, never()) { pullChatMessages(any(), any(), any()) } + verifyBlocking(network, after(LONG_POLLING_OBSERVATION_MILLIS).never()) { + pullChatMessages(any(), any(), any()) + } polling.cancelAndJoin() } @@ -272,13 +275,17 @@ class OfflineFirstChatRepositoryTest { val polling = launch { repository.initInsuranceRequests() } advanceTimeBy(INSURANCE_DELAY_MS * 3 + 1) runCurrent() - verifyBlocking(network, never()) { pullChatMessages(any(), any(), any()) } + verifyBlocking(network, after(LONG_POLLING_OBSERVATION_MILLIS).never()) { + pullChatMessages(any(), any(), any()) + } repository.handleOnResume() // the loop keeps its normal step: nothing is sent before the next tick ... advanceTimeBy(INSURANCE_DELAY_MS / 2) runCurrent() - verifyBlocking(network, never()) { pullChatMessages(any(), any(), any()) } + verifyBlocking(network, after(LONG_POLLING_OBSERVATION_MILLIS).never()) { + pullChatMessages(any(), any(), any()) + } // ... and the request is sent on it, from the pull flow on Dispatchers.IO that runCurrent doesn't wait for advanceTimeBy(INSURANCE_DELAY_MS / 2) diff --git a/app/src/test/java/com/nextcloud/talk/dagger/modules/RestModuleHostnameVerificationTest.kt b/app/src/test/java/com/nextcloud/talk/dagger/modules/RestModuleHostnameVerificationTest.kt new file mode 100644 index 00000000000..619f0453e63 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/dagger/modules/RestModuleHostnameVerificationTest.kt @@ -0,0 +1,153 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.dagger.modules + +import android.app.Application +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.nextcloud.talk.api.LoggingHttpInterceptor +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.utils.AccountCookieInterceptor +import com.nextcloud.talk.utils.preferences.AppPreferences +import com.nextcloud.talk.utils.ssl.SSLSocketFactoryCompat +import com.nextcloud.talk.utils.ssl.TrustManager +import okhttp3.Cache +import okhttp3.Dispatcher +import okhttp3.tls.HeldCertificate +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.robolectric.util.ReflectionHelpers +import java.net.CookieManager +import java.net.Proxy +import java.security.cert.X509Certificate +import javax.net.ssl.HostnameVerifier +import javax.net.ssl.SSLSession + +/** + * Checks the hostname verification of the client [RestModule] actually builds, with real certificates. + * + * [com.nextcloud.talk.utils.ssl.TrustManagerTest] covers our wrapper with a mocked default verifier; this test + * pins the default the wrapper is given (okhttp's own) and fails if hostname verification is ever weakened, + * whether by okhttp or by the way the client is set up. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class RestModuleHostnameVerificationTest { + + @get:Rule + val temporaryFolder = TemporaryFolder() + + private lateinit var trustManager: TrustManager + private lateinit var hostnameVerifier: HostnameVerifier + + @Before + fun setUp() { + // TrustManager() needs NextcloudTalkApplication.sharedApplication for Context.getDir(), see TrustManagerTest + val fakeApplication = NextcloudTalkApplication() + ReflectionHelpers.callInstanceMethod( + fakeApplication, + "attachBaseContext", + ReflectionHelpers.ClassParameter.from(Context::class.java, ApplicationProvider.getApplicationContext()) + ) + setSharedApplication(fakeApplication) + trustManager = TrustManager() + + val client = RestModule(ApplicationProvider.getApplicationContext()).provideHttpClient( + Proxy.NO_PROXY, + mock(), + trustManager, + SSLSocketFactoryCompat(null, trustManager), + Cache(temporaryFolder.newFolder(), CACHE_SIZE), + CookieManager(), + Dispatcher(), + mock(), + AccountCookieInterceptor() + ) + hostnameVerifier = client.hostnameVerifier + } + + @After + fun tearDown() { + setSharedApplication(null) + } + + private fun setSharedApplication(application: NextcloudTalkApplication?) { + ReflectionHelpers.callInstanceMethod( + NextcloudTalkApplication.Companion, + "setSharedApplication", + ReflectionHelpers.ClassParameter.from(NextcloudTalkApplication::class.java, application) + ) + } + + @Test + fun `a certificate for the requested host is accepted`() { + val certificate = certificateFor(SERVER_HOST) + + assertTrue(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + } + + @Test + fun `a certificate for another host is rejected`() { + val certificate = certificateFor(OTHER_HOST) + + assertFalse(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + } + + @Test + fun `a certificate for a subdomain of the requested host is rejected`() { + val certificate = certificateFor("sub.$SERVER_HOST") + + assertFalse(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + } + + @Test + fun `a certificate whose host only appears as common name is rejected`() { + // since RFC 2818 the common name is not used for hostname verification, only subject alternative names + val certificate = HeldCertificate.Builder().commonName(SERVER_HOST).build().certificate + + assertFalse(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + } + + @Test + fun `a certificate for another host is only accepted once the user trusted it`() { + val certificate = certificateFor(OTHER_HOST) + assertFalse(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + + trustManager.addCertInTrustStore(certificate) + + assertTrue(hostnameVerifier.verify(SERVER_HOST, sessionWith(certificate))) + } + + private fun certificateFor(host: String): X509Certificate = + HeldCertificate.Builder() + .commonName(host) + .addSubjectAlternativeName(host) + .build() + .certificate + + private fun sessionWith(certificate: X509Certificate): SSLSession { + val sslSession = mock() + whenever(sslSession.peerCertificates).thenReturn(arrayOf(certificate)) + return sslSession + } + + companion object { + private const val SERVER_HOST = "cloud.example.com" + private const val OTHER_HOST = "evil.example.org" + private const val CACHE_SIZE = 1024L + } +}