do not add tracing ids to verification events

Author: BillCarsonFr

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/EventType.kt

@@ -16,6 +16,8 @@
 
 package org.matrix.android.sdk.api.session.events.model
 
+import org.matrix.android.sdk.api.session.room.model.message.MessageType.MSGTYPE_VERIFICATION_REQUEST
+
 /**
  * Constants defining known event types from Matrix specifications.
  */
@@ -126,6 +128,7 @@ object EventType {
 
     fun isVerificationEvent(type: String): Boolean {
         return when (type) {
+            MSGTYPE_VERIFICATION_REQUEST,
             KEY_VERIFICATION_START,
             KEY_VERIFICATION_ACCEPT,
             KEY_VERIFICATION_KEY,

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/tasks/SendToDeviceTask.kt

@@ -18,6 +18,7 @@ package org.matrix.android.sdk.internal.crypto.tasks
 
 import org.matrix.android.sdk.api.session.crypto.model.MXUsersDevicesMap
 import org.matrix.android.sdk.api.session.events.model.Event
+import org.matrix.android.sdk.api.session.events.model.EventType
 import org.matrix.android.sdk.api.session.events.model.toContent
 import org.matrix.android.sdk.internal.crypto.api.CryptoApi
 import org.matrix.android.sdk.internal.crypto.model.rest.SendToDeviceBody
@@ -39,7 +40,9 @@ internal interface SendToDeviceTask : Task<SendToDeviceTask.Params, Unit> {
             // the content to send. Map from user_id to device_id to content dictionary.
             val contentMap: MXUsersDevicesMap<Any>,
             // the transactionId. If not provided, a transactionId will be created by the task
-            val transactionId: String? = null
+            val transactionId: String? = null,
+            // add tracing id, notice that to device events that do signature on content might be broken by it
+            val addTracingIds: Boolean = !EventType.isVerificationEvent(eventType),
     )
 }
 
@@ -55,7 +58,12 @@ internal class DefaultSendToDeviceTask @Inject constructor(
         val txnId = params.transactionId ?: createUniqueTxnId()
 
         // add id tracing to debug
-        val decorated = decorateWithToDeviceTracingIds(params)
+        val decorated = if (params.addTracingIds) {
+            decorateWithToDeviceTracingIds(params)
+        } else {
+            params.contentMap.map to emptyList()
+        }
+
         val sendToDeviceBody = SendToDeviceBody(
                 messages = decorated.first
         )

matrix-sdk-android/src/test/java/org/matrix/android/sdk/internal/crypto/DefaultSendToDeviceTaskTest.kt

@@ -25,6 +25,7 @@ import org.matrix.android.sdk.api.session.crypto.model.DeviceInfo
 import org.matrix.android.sdk.api.session.crypto.model.DevicesListResponse
 import org.matrix.android.sdk.api.session.crypto.model.MXUsersDevicesMap
 import org.matrix.android.sdk.api.session.events.model.EventType
+import org.matrix.android.sdk.api.session.room.model.message.MessageType
 import org.matrix.android.sdk.internal.crypto.api.CryptoApi
 import org.matrix.android.sdk.internal.crypto.model.rest.DeleteDeviceParams
 import org.matrix.android.sdk.internal.crypto.model.rest.DeleteDevicesParams
@@ -60,8 +61,28 @@ class DefaultSendToDeviceTaskTest {
             )
     )
 
+    private val fakeStartVerificationContent = mapOf(
+            "method" to "m.sas.v1",
+            "from_device" to "MNQHVEISFQ",
+            "key_agreement_protocols" to listOf(
+                    "curve25519-hkdf-sha256",
+                    "curve25519"
+            ),
+            "hashes" to listOf("sha256"),
+            "message_authentication_codes" to listOf(
+                    "org.matrix.msc3783.hkdf-hmac-sha256",
+                    "hkdf-hmac-sha256",
+                    "hmac-sha256"
+            ),
+            "short_authentication_string" to listOf(
+                    "decimal",
+                    "emoji"
+            ),
+            "transaction_id" to "4wNOpkHGwGZPXjkZToooCDWfb8hsf7vW"
+    )
+
     @Test
-    fun `tracing id should be added to all to_device contents`() {
+    fun `tracing id should be added to to_device contents`() {
         val fakeCryptoAPi = FakeCryptoApi()
 
         val sendToDeviceTask = DefaultSendToDeviceTask(
@@ -107,6 +128,80 @@ class DefaultSendToDeviceTaskTest {
         println("modified content ${fakeCryptoAPi.body}")
     }
 
+    @Test
+    fun `tracing id should not be added to verification start to_device contents`() {
+        val fakeCryptoAPi = FakeCryptoApi()
+
+        val sendToDeviceTask = DefaultSendToDeviceTask(
+                cryptoApi = fakeCryptoAPi,
+                globalErrorReceiver = mockk(relaxed = true)
+        )
+        val contentMap = MXUsersDevicesMap<Any>()
+        contentMap.setObject("@alice:example.com", "MNQHVEISFQ", fakeStartVerificationContent)
+
+        val params = SendToDeviceTask.Params(
+                eventType = EventType.KEY_VERIFICATION_START,
+                contentMap = contentMap
+        )
+
+        runBlocking {
+            sendToDeviceTask.execute(params)
+        }
+
+        val modifiedContent = fakeCryptoAPi.body!!.messages!!["@alice:example.com"]!!["MNQHVEISFQ"] as Map<*, *>
+        Assert.assertNull("Tracing id should not have been added", modifiedContent["org.matrix.msgid"])
+
+        // try to force
+        runBlocking {
+            sendToDeviceTask.execute(
+                    SendToDeviceTask.Params(
+                            eventType = EventType.KEY_VERIFICATION_START,
+                            contentMap = contentMap,
+                            addTracingIds = true
+                    )
+            )
+        }
+
+        val modifiedContentForced = fakeCryptoAPi.body!!.messages!!["@alice:example.com"]!!["MNQHVEISFQ"] as Map<*, *>
+        Assert.assertNotNull("Tracing id should have been added", modifiedContentForced["org.matrix.msgid"])
+    }
+
+    @Test
+    fun `tracing id should not be added to all verification to_device contents`() {
+        val fakeCryptoAPi = FakeCryptoApi()
+
+        val sendToDeviceTask = DefaultSendToDeviceTask(
+                cryptoApi = fakeCryptoAPi,
+                globalErrorReceiver = mockk(relaxed = true)
+        )
+        val contentMap = MXUsersDevicesMap<Any>()
+        contentMap.setObject("@alice:example.com", "MNQHVEISFQ", emptyMap<String, Any>())
+
+        val verificationEvents = listOf(
+                MessageType.MSGTYPE_VERIFICATION_REQUEST,
+                EventType.KEY_VERIFICATION_START,
+                EventType.KEY_VERIFICATION_ACCEPT,
+                EventType.KEY_VERIFICATION_KEY,
+                EventType.KEY_VERIFICATION_MAC,
+                EventType.KEY_VERIFICATION_CANCEL,
+                EventType.KEY_VERIFICATION_DONE,
+                EventType.KEY_VERIFICATION_READY
+        )
+
+        for (type in verificationEvents) {
+            val params = SendToDeviceTask.Params(
+                    eventType = type,
+                    contentMap = contentMap
+            )
+            runBlocking {
+                sendToDeviceTask.execute(params)
+            }
+
+            val modifiedContent = fakeCryptoAPi.body!!.messages!!["@alice:example.com"]!!["MNQHVEISFQ"] as Map<*, *>
+            Assert.assertNull("Tracing id should not have been added", modifiedContent["org.matrix.msgid"])
+        }
+    }
+
     internal class FakeCryptoApi : CryptoApi {
         override suspend fun getDevices(): DevicesListResponse {
             throw java.lang.AssertionError("Should not be called")