feat(http): don't fail a message when given a 200 OK reply even with a non-parsable response.

.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 }}

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

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)
 

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<RecyclerView.ViewHolder>(
+              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<RecyclerView.ViewHolder>(
+              hasDescendant(withText(textResource)),
+              click(),
+          ),
+      )
   BaristaEditTextInteractions.writeTo(android.R.id.edit, value)
   clickDialogPositiveButton()
 }

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<PreferencesActivity>() {
     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<PreferencesActivity>() {
     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)

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)
             }
           }
         }

project/app/src/test/java/org/owntracks/android/net/http/HttpMessageProcessorEndpointTest.kt

@@ -4,23 +4,31 @@ 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
 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
 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,16 +47,21 @@ 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 ""
       on { username } doReturn ""
       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)
+  }
 }