From eb9e97968264e0d5e9e629e6bd0925458659c170 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 17 Oct 2026 23:35:15 +0200 Subject: [PATCH 1/3] fix: stop importing okhttp's internal OkHostnameVerifier RestModule wrapped okhttp3.internal.tls.OkHostnameVerifier, an internal class without compatibility guarantees that a later okhttp release may move or remove. Read the same default verifier through the public OkHttpClient.hostnameVerifier() instead; in okhttp 5 it is OkHostnameVerifier, so hostname verification is unchanged. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger --- .../java/com/nextcloud/talk/dagger/modules/RestModule.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 c70bafc1bd..b44f7f7a10 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)) { From 6b8c9852fb5514f2727db12a2f66dfce29dc6379 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 17 Oct 2026 23:35:16 +0200 Subject: [PATCH 2/3] test: make the insurance request never() checks observe the IO thread The insurance request reaches the network mock from the pull flow on Dispatchers.IO, which runCurrent() does not wait for. A never() check right after runCurrent() therefore passed even when a request had been started too early, as long as the IO thread had not reached the mock yet. Verify with after(), so the check watches for a while before concluding that nothing was sent. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger --- .../data/network/OfflineFirstChatRepositoryTest.kt | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) 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 9accbc8549..c6396692ef 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) From 1a0e9b1298deedbbf0d5b4cc23281a8d8f05c346 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 17 Oct 2026 23:58:47 +0200 Subject: [PATCH 3/3] test: pin the hostname verification of the REST client TrustManagerTest covers our verifier wrapper with a mocked default verifier only. Build the client through RestModule.provideHttpClient and run real certificates through its hostname verifier: a certificate for another host, a subdomain or with the host only as common name is rejected, and a mismatching one is only accepted once the user trusted it. This fails if hostname verification is ever weakened, whether by an okhttp default or by the client setup. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Andy Scherzinger --- .../RestModuleHostnameVerificationTest.kt | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/dagger/modules/RestModuleHostnameVerificationTest.kt 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 0000000000..619f0453e6 --- /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 + } +}