diff --git a/.github/workflows/espresso.yaml b/.github/workflows/espresso.yaml index 17b202b1ed..6c92aa96a5 100644 --- a/.github/workflows/espresso.yaml +++ b/.github/workflows/espresso.yaml @@ -25,7 +25,7 @@ jobs: - name: Set Matrix id: set-matrix run: | - jq -cn --argjson count "${{ inputs.shards }}" '[range(1; $count + 1) | {"shard":.}]' > matrix.json + jq -cn --argjson count "${{ inputs.shards }}" '[range(1; $count + 1)]' > matrix.json echo "matrix=$(cat matrix.json)" >> "$GITHUB_OUTPUT" espresso-test: needs: [generate-shards-matrix] @@ -39,7 +39,7 @@ jobs: matrix: android-api: [31] flavour: [Gms, Oss] - include: ${{ fromJSON(needs.generate-shards-matrix.outputs.matrix) }} + shard: ${{ fromJSON(needs.generate-shards-matrix.outputs.matrix) }} steps: - name: Enable KVM group perms run: | @@ -112,7 +112,7 @@ jobs: timeout-minutes: 30 env: FLAVOUR: ${{ matrix.flavour }} - SHARD_COUNT: 6 + SHARD_COUNT: ${{ inputs.shards }} SHARD_INDEX: ${{ matrix.shard }} GRADLE_OPTS: "-Dorg.gradle.daemon=true -Dorg.gradle.configuration-cache=true -Dorg.gradle.parallel=true -Dorg.gradle.caching=true -Dorg.gradle.jvmargs='-Xmx3096M -Dkotlin.daemon.jvm.options=-Xmx2048M -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC'" TEST_ANNOTATION: ${{ inputs.test-annotation }} diff --git a/CHANGELOG.md b/CHANGELOG.md index abfbf7852e..66ad8602c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ This release addresses a security advisory covering several intent-handling vuln ### Bug fixes - DEBUG and VERBOSE log messages are no longer emitted to the system Logcat in release builds, preventing potential PII (e.g. coordinates) leakage via `TimberInMemoryLogTree` (CWE-532) +- HTTP mode no longer treats an unparsable or empty response body as a send failure. A `200 OK` response is sufficient to confirm a message was delivered successfully; response body parse errors are logged as warnings and ignored (#2242) ## Version 2.5.10 diff --git a/project/app/build.gradle.kts b/project/app/build.gradle.kts index 78e7ef8d8d..56fa854d4b 100644 --- a/project/app/build.gradle.kts +++ b/project/app/build.gradle.kts @@ -259,6 +259,7 @@ dependencies { testImplementation(libs.mockito.kotlin) testImplementation(libs.androidx.core.testing) testImplementation(libs.kotlin.coroutines.test) + testImplementation(libs.okhttp.mockwebserver) androidTestImplementation(libs.bundles.androidx.test) diff --git a/project/app/src/androidTest/kotlin/org/owntracks/android/testutils/Helpers.kt b/project/app/src/androidTest/kotlin/org/owntracks/android/testutils/Helpers.kt index 85d055fbae..d336b63f3c 100644 --- a/project/app/src/androidTest/kotlin/org/owntracks/android/testutils/Helpers.kt +++ b/project/app/src/androidTest/kotlin/org/owntracks/android/testutils/Helpers.kt @@ -68,9 +68,24 @@ fun scrollToPreferenceWithText(textResource: Int) { ) } +fun clickOnPreference(textResource: Int) { + onView(withId(androidx.preference.R.id.recycler_view)) + .perform( + RecyclerViewActions.actionOnItem( + hasDescendant(withText(textResource)), + click(), + ), + ) +} + fun writeToPreference(textResource: Int, value: String) { - scrollToPreferenceWithText(textResource) - clickOn(textResource) + onView(withId(androidx.preference.R.id.recycler_view)) + .perform( + RecyclerViewActions.actionOnItem( + hasDescendant(withText(textResource)), + click(), + ), + ) BaristaEditTextInteractions.writeTo(android.R.id.edit, value) clickDialogPositiveButton() } diff --git a/project/app/src/androidTest/kotlin/org/owntracks/android/ui/PreferencesActivityTests.kt b/project/app/src/androidTest/kotlin/org/owntracks/android/ui/PreferencesActivityTests.kt index 80400248b6..9f1245be14 100644 --- a/project/app/src/androidTest/kotlin/org/owntracks/android/ui/PreferencesActivityTests.kt +++ b/project/app/src/androidTest/kotlin/org/owntracks/android/ui/PreferencesActivityTests.kt @@ -17,6 +17,7 @@ import org.owntracks.android.preferences.SharedPreferencesStore import org.owntracks.android.preferences.types.ReverseGeocodeProvider import org.owntracks.android.test.SimpleIdlingResource import org.owntracks.android.testutils.TestWithAnActivity +import org.owntracks.android.testutils.clickOnPreference import org.owntracks.android.testutils.scrollToPreferenceWithText import org.owntracks.android.testutils.writeToPreference import org.owntracks.android.ui.preferences.PreferencesActivity @@ -45,16 +46,14 @@ class PreferencesActivityTests : TestWithAnActivity() { writeToPreference( R.string.preferencesClientId, "test-clientId") // This hyphen will get squelched - scrollToPreferenceWithText(R.string.preferencesWebsocket) - clickOn(R.string.preferencesWebsocket) + clickOnPreference(R.string.preferencesWebsocket) writeToPreference(R.string.preferencesUsername, "testUsername") writeToPreference(R.string.preferencesBrokerPassword, "testPassword") writeToPreference(R.string.preferencesDeviceName, "testDeviceId") writeToPreference(R.string.preferencesTrackerId, "t5") - scrollToPreferenceWithText(R.string.preferencesCleanSessionEnabled) - clickOn(R.string.preferencesCleanSessionEnabled) + clickOnPreference(R.string.preferencesCleanSessionEnabled) writeToPreference(R.string.preferencesKeepalive, "1570") @@ -126,14 +125,11 @@ class PreferencesActivityTests : TestWithAnActivity() { writeToPreference(R.string.preferencesLocatorDisplacement, "567") writeToPreference(R.string.preferencesMoveModeLocatorInterval, "5") - scrollToPreferenceWithText(R.string.preferencesPegLocatorFastestIntervalToInterval) - clickOn(R.string.preferencesPegLocatorFastestIntervalToInterval) + clickOnPreference(R.string.preferencesPegLocatorFastestIntervalToInterval) - scrollToPreferenceWithText(R.string.preferencesAutostart) - clickOn(R.string.preferencesAutostart) + clickOnPreference(R.string.preferencesAutostart) - scrollToPreferenceWithText(R.string.preferencesReverseGeocodeProvider) - clickOn(R.string.preferencesReverseGeocodeProvider) + clickOnPreference(R.string.preferencesReverseGeocodeProvider) clickOn("OpenCage") clickOn(android.R.id.button1) diff --git a/project/app/src/main/java/org/owntracks/android/net/http/HttpMessageProcessorEndpoint.kt b/project/app/src/main/java/org/owntracks/android/net/http/HttpMessageProcessorEndpoint.kt index bc6d9baa2b..a759bb0945 100644 --- a/project/app/src/main/java/org/owntracks/android/net/http/HttpMessageProcessorEndpoint.kt +++ b/project/app/src/main/java/org/owntracks/android/net/http/HttpMessageProcessorEndpoint.kt @@ -48,7 +48,7 @@ class HttpMessageProcessorEndpoint( @CoroutineScopes.IoDispatcher private val ioDispatcher: CoroutineDispatcher ) : MessageProcessorEndpoint(messageProcessor), Preferences.OnPreferenceChangeListener { override val modeId: ConnectionMode = ConnectionMode.HTTP - private var httpClientAndConfiguration: HttpClientAndConfiguration? = null + internal var httpClientAndConfiguration: HttpClientAndConfiguration? = null override fun activate() { Timber.v("HTTP Activate") @@ -154,13 +154,22 @@ class HttpMessageProcessorEndpoint( } return Result.success(Unit) } catch (e: IOException) { - Timber.e(e, "HTTP Delivery failed") - endpointStateRepo.setState(EndpointState.ERROR.withError(e)) - messageProcessor.onMessageDeliveryFailed(message) - return Result.failure(OutgoingMessageSendingException(e)) + Timber.w(e, "HTTP response body could not be parsed, ignoring") + endpointStateRepo.setState( + EndpointState.IDLE.withMessage( + String.format( + Locale.ROOT, "Response %d (response not parseable)", response.code), + ), + ) + return Result.success(Unit) } } else { - Result.failure(OutgoingMessageSendingException(null)) + endpointStateRepo.setState( + EndpointState.IDLE.withMessage( + String.format(Locale.ROOT, "Response %d", response.code), + ), + ) + Result.success(Unit) } } } diff --git a/project/app/src/test/java/org/owntracks/android/net/http/HttpMessageProcessorEndpointTest.kt b/project/app/src/test/java/org/owntracks/android/net/http/HttpMessageProcessorEndpointTest.kt index f7aa0b05b0..b3022e02e8 100644 --- a/project/app/src/test/java/org/owntracks/android/net/http/HttpMessageProcessorEndpointTest.kt +++ b/project/app/src/test/java/org/owntracks/android/net/http/HttpMessageProcessorEndpointTest.kt @@ -4,9 +4,14 @@ import android.app.Application import androidx.arch.core.executor.testing.InstantTaskExecutorRule import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.runTest +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test @@ -14,6 +19,8 @@ import org.mockito.Mock import org.mockito.Mockito.`when` import org.mockito.kotlin.doReturn import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify import org.owntracks.android.data.repos.EndpointStateRepo import org.owntracks.android.model.EncryptionProvider import org.owntracks.android.model.Parser @@ -21,6 +28,7 @@ import org.owntracks.android.model.messages.MessageCard import org.owntracks.android.model.messages.MessageCmd import org.owntracks.android.model.messages.MessageLocation import org.owntracks.android.preferences.Preferences +import org.owntracks.android.preferences.types.MqttQos import org.owntracks.android.services.MessageProcessor import org.owntracks.android.support.interfaces.ConfigurationIncompleteException @@ -39,9 +47,11 @@ class HttpMessageProcessorEndpointTest { @Mock private lateinit var parser: Parser private lateinit var messageLocation: MessageLocation + private val mockWebServer = MockWebServer() @Before fun setupPreferences() { + mockWebServer.start() testPreferences = mock { on { encryptionKey } doReturn "testEncryptionKey" on { tlsClientCrt } doReturn "" @@ -49,6 +59,9 @@ class HttpMessageProcessorEndpointTest { on { deviceId } doReturn "" on { password } doReturn "" on { url } doReturn "http://example.com/owntracks/test" + on { pubTopicLocations } doReturn "owntracks/test/phone" + on { pubQosLocations } doReturn MqttQos.Zero + on { pubRetainLocations } doReturn false } encryptionProvider = mock { on { isPayloadEncryptionEnabled } doReturn false } messageProcessor = mock {} @@ -66,6 +79,11 @@ class HttpMessageProcessorEndpointTest { application = mock {} } + @After + fun tearDown() { + mockWebServer.shutdown() + } + @Test fun `Given a simple request, the auth headers are not set`() = runTest { val httpMessageProcessorEndpoint = @@ -257,4 +275,53 @@ class HttpMessageProcessorEndpointTest { httpMessageProcessorEndpoint.onMessageReceived(messageCmd) assertEquals("NOKEY", messageCmd.getContactId()) } + + @Test + fun `Given a 200 OK response with an unparsable body, sendMessage returns success and does not fail delivery`() = + runTest { + mockWebServer.enqueue(MockResponse().setResponseCode(200).setBody("this is not valid json")) + val dispatcher = StandardTestDispatcher(testScheduler) + val endpoint = + HttpMessageProcessorEndpoint( + messageProcessor, + parser, + testPreferences, + application, + endpointStateRepo, + mock {}, + this, + dispatcher) + endpoint.httpClientAndConfiguration = + HttpMessageProcessorEndpoint.HttpClientAndConfiguration( + OkHttpClient(), + HttpConfiguration(mockWebServer.url("/owntracks/test").toString(), "", "", ""), + ) + val result = endpoint.sendMessage(messageLocation) + assertTrue(result.isSuccess) + verify(messageProcessor, never()).onMessageDeliveryFailed(messageLocation) + } + + @Test + fun `Given a 200 OK response with no body, sendMessage returns success`() = runTest { + mockWebServer.enqueue(MockResponse().setResponseCode(200)) + val dispatcher = StandardTestDispatcher(testScheduler) + val endpoint = + HttpMessageProcessorEndpoint( + messageProcessor, + parser, + testPreferences, + application, + endpointStateRepo, + mock {}, + this, + dispatcher) + endpoint.httpClientAndConfiguration = + HttpMessageProcessorEndpoint.HttpClientAndConfiguration( + OkHttpClient(), + HttpConfiguration(mockWebServer.url("/owntracks/test").toString(), "", "", ""), + ) + val result = endpoint.sendMessage(messageLocation) + assertTrue(result.isSuccess) + verify(messageProcessor, never()).onMessageDeliveryFailed(messageLocation) + } }