From 9ac71fd35d39b0a33fe979b1a0bdfbf90bd843f2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 15 Oct 2026 12:40:26 +0200 Subject: [PATCH] Fix room id parsing from notification tags Thread notification tags have the form "roomId|threadId". clearReadRoomsNotifications was building a RoomId from the whole tag, which is only validated in debug builds, so in release an invalid room id reached RoomListService.room(). Add NotificationCreator.getMessageNotificationRoomId(), the inverse of messageTag(), which only keeps the part before '|' and checks it is a valid room id. Also make RoomListServiceInterface.roomOrNull take a RoomId instead of a String. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../libraries/matrix/impl/RustMatrixClient.kt | 4 +- .../matrix/impl/room/RoomContentForwarder.kt | 2 +- .../matrix/impl/room/RustRoomFactory.kt | 4 +- .../impl/roomlist/RoomListExtensions.kt | 5 ++- .../impl/roomlist/RoomSummaryListProcessor.kt | 7 ++-- .../DefaultNotificationDrawerManager.kt | 3 +- .../factories/NotificationCreator.kt | 23 +++++++++-- .../DefaultNotificationDrawerManagerTest.kt | 39 ++++++++++++++++++- .../factories/NotificationCreatorTest.kt | 36 +++++++++++++++++ 9 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreatorTest.kt diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt index 43b339e06c..6c1c31539f 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt @@ -820,7 +820,7 @@ class RustMatrixClient( override suspend fun getRoomInfo(roomId: RoomId): Result = withContext(sessionDispatcher) { runCatchingExceptions { - innerRoomListService.roomOrNull(roomId.value)?.use { room -> + innerRoomListService.roomOrNull(roomId)?.use { room -> roomInfoMapper.map(room.roomInfo()) } } @@ -828,7 +828,7 @@ class RustMatrixClient( override fun getRoomInfoFlow(roomId: RoomId): Flow> { return mxCallbackFlow { - val roomNotFound = innerRoomListService.roomOrNull(roomId.value).use { it == null } + val roomNotFound = innerRoomListService.roomOrNull(roomId).use { it == null } if (roomNotFound) { channel.send(Optional.empty()) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomContentForwarder.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomContentForwarder.kt index 8191082806..4392eb8bee 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomContentForwarder.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RoomContentForwarder.kt @@ -49,7 +49,7 @@ class RoomContentForwarder( val message = content.content.kind as? MsgLikeKind.Message ?: throw ForwardEventException(toRoomIds) contentWithoutRelationFromMessage(message.content) } - val targetRooms = toRoomIds.toSet().mapNotNull { roomId -> roomListService.roomOrNull(roomId.value) } + val targetRooms = toRoomIds.toSet().mapNotNull { roomId -> roomListService.roomOrNull(roomId) } val failedForwardingTo = mutableSetOf() targetRooms.parallelMap { room -> room.use { targetRoom -> diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomFactory.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomFactory.kt index 960509b503..6d20a23f26 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomFactory.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustRoomFactory.kt @@ -187,11 +187,11 @@ class RustRoomFactory( * Get the Rust room for a room, retrying after the room list is loaded if necessary. */ private suspend fun awaitRoomInRoomList(roomId: RoomId): Room? { - var sdkRoom = innerRoomListService.roomOrNull(roomId.value) + var sdkRoom = innerRoomListService.roomOrNull(roomId) if (sdkRoom == null) { // ... otherwise, lets wait for the SS to load all rooms and check again. roomListService.allRooms.awaitLoaded() - sdkRoom = innerRoomListService.roomOrNull(roomId.value) + sdkRoom = innerRoomListService.roomOrNull(roomId) } if (sdkRoom == null) { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListExtensions.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListExtensions.kt index 2352f82c3b..5aacf89bc2 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListExtensions.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListExtensions.kt @@ -9,6 +9,7 @@ package io.element.android.libraries.matrix.impl.roomlist import io.element.android.libraries.core.data.tryOrNull +import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.impl.util.cancelAndDestroy import io.element.android.libraries.matrix.impl.util.mxCallbackFlow import kotlinx.coroutines.channels.Channel @@ -109,10 +110,10 @@ internal fun RoomListServiceInterface.syncIndicator(): Flow - val result = buildRoomSummaryForIdentifier(summary.roomId.value) + val result = buildRoomSummaryForIdentifier(summary.roomId) if (result != null) { this[i] = result } @@ -103,8 +104,8 @@ class RoomSummaryListProcessor( return entry.use { roomSummaryFactory.create(room = it) } } - private suspend fun buildRoomSummaryForIdentifier(identifier: String): RoomSummary? { - return roomListService.roomOrNull(identifier)?.let { room -> + private suspend fun buildRoomSummaryForIdentifier(roomId: RoomId): RoomSummary? { + return roomListService.roomOrNull(roomId)?.let { room -> buildSummaryForRoomListEntry(room) } } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt index 12109381df..8e5b19505d 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt @@ -12,7 +12,6 @@ import dev.zacsweers.metro.AppScope import dev.zacsweers.metro.ContributesBinding import dev.zacsweers.metro.SingleIn import io.element.android.features.lockscreen.api.LockScreenService -import io.element.android.libraries.core.data.tryOrNull import io.element.android.libraries.di.annotations.AppCoroutineScope import io.element.android.libraries.matrix.api.MatrixClientProvider import io.element.android.libraries.matrix.api.core.EventId @@ -191,7 +190,7 @@ class DefaultNotificationDrawerManager( override fun clearReadRoomsNotifications(sessionId: SessionId) { val notifications = activeNotificationsProvider.getNotificationsForSession(sessionId) val notificationsGroupedByRoomId = notifications.mapNotNull { - tryOrNull { RoomId(it.tag) }?.let { roomId -> roomId to it } + it.tag?.let(NotificationCreator::getMessageNotificationRoomId)?.let { roomId -> roomId to it } } if (notificationsGroupedByRoomId.isEmpty()) return diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt index 26412abebc..3394a2730c 100755 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt @@ -24,6 +24,7 @@ import io.element.android.libraries.designsystem.components.avatar.AvatarData import io.element.android.libraries.designsystem.components.avatar.AvatarSize import io.element.android.libraries.designsystem.utils.CommonDrawables import io.element.android.libraries.di.annotations.ApplicationContext +import io.element.android.libraries.matrix.api.core.MatrixPatterns import io.element.android.libraries.matrix.api.core.RoomId import io.element.android.libraries.matrix.api.core.ThreadId import io.element.android.libraries.matrix.api.timeline.item.event.EventType @@ -102,11 +103,25 @@ interface NotificationCreator { /** * Creates a tag for a message notification given its [roomId] and optional [threadId]. */ - fun messageTag(roomId: RoomId, threadId: ThreadId?): String = if (threadId != null) { - "$roomId|$threadId" - } else { - roomId.value + fun messageTag(roomId: RoomId, threadId: ThreadId?): String = buildString { + append(roomId) + threadId?.let { + append(SEPARATOR) + append(it) + } } + + /** + * Extracts the [RoomId] from a message notification [tag] created by [messageTag]. + * Returns null if the tag does not start with a valid room id. + */ + fun getMessageNotificationRoomId(tag: String): RoomId? { + return tag.substringBefore(SEPARATOR) + .takeIf(MatrixPatterns::isRoomId) + ?.let(::RoomId) + } + + private const val SEPARATOR = "|" } } diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManagerTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManagerTest.kt index ce7690ed0a..0cb6997ffa 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManagerTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManagerTest.kt @@ -636,6 +636,43 @@ class DefaultNotificationDrawerManagerTest { cancelNotificationResult.assertions().isNeverCalled() } + @Test + fun `clearReadRoomsNotifications cancels thread notifications of rooms without unread counts`() = runTest { + val cancelNotificationResult = lambdaRecorder { _, _ -> } + val threadTag = "$A_ROOM_ID|$A_THREAD_ID" + val sut = createClearReadRoomsManager( + cancelNotificationResult = cancelNotificationResult, + notifications = listOf( + mockk { + every { id } returns 1 + every { tag } returns threadTag + }, + ), + roomInfo = aRoomInfo(id = A_ROOM_ID), + ) + sut.clearReadRoomsNotifications(A_SESSION_ID) + runCurrent() + cancelNotificationResult.assertions().isCalledOnce().with(value(threadTag), value(1)) + } + + @Test + fun `clearReadRoomsNotifications ignores notifications whose tag is not a room id`() = runTest { + val cancelNotificationResult = lambdaRecorder { _, _ -> } + val sut = createClearReadRoomsManager( + cancelNotificationResult = cancelNotificationResult, + notifications = listOf( + mockk { + every { id } returns 1 + every { tag } returns AN_EVENT_ID.value + }, + ), + roomInfo = aRoomInfo(id = A_ROOM_ID), + ) + sut.clearReadRoomsNotifications(A_SESSION_ID) + runCurrent() + cancelNotificationResult.assertions().isNeverCalled() + } + @Test fun `clearReadRoomsNotifications clears the summary when it is the only notification left`() = runTest { val cancelNotificationResult = lambdaRecorder { _, _ -> } @@ -681,7 +718,7 @@ class DefaultNotificationDrawerManagerTest { count: Int = notifications.size, ): DefaultNotificationDrawerManager { val matrixClient = FakeMatrixClient().apply { - getRoomInfoLambda = { Result.success(roomInfo) } + getRoomInfoLambda = { roomId -> Result.success(roomInfo?.takeIf { it.id == roomId }) } } return createDefaultNotificationDrawerManager( notificationDisplayer = FakeNotificationDisplayer(cancelNotificationResult = cancelNotificationResult), diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreatorTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreatorTest.kt new file mode 100644 index 0000000000..1c4c6df895 --- /dev/null +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreatorTest.kt @@ -0,0 +1,36 @@ +/* + * Copyright (c) 2026 Element Creations Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.libraries.push.impl.notifications.factories + +import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.matrix.test.AN_EVENT_ID +import io.element.android.libraries.matrix.test.A_ROOM_ID +import io.element.android.libraries.matrix.test.A_THREAD_ID +import org.junit.Test + +class NotificationCreatorTest { + @Test + fun `getMessageNotificationRoomId returns the room id of a room message tag`() { + val tag = NotificationCreator.messageTag(A_ROOM_ID, null) + assertThat(tag).isEqualTo(A_ROOM_ID.value) + assertThat(NotificationCreator.getMessageNotificationRoomId(tag)).isEqualTo(A_ROOM_ID) + } + + @Test + fun `getMessageNotificationRoomId returns the room id of a thread message tag`() { + val tag = NotificationCreator.messageTag(A_ROOM_ID, A_THREAD_ID) + assertThat(tag).isEqualTo("${A_ROOM_ID.value}|${A_THREAD_ID.value}") + assertThat(NotificationCreator.getMessageNotificationRoomId(tag)).isEqualTo(A_ROOM_ID) + } + + @Test + fun `getMessageNotificationRoomId returns null for a tag which is not a room id`() { + assertThat(NotificationCreator.getMessageNotificationRoomId(AN_EVENT_ID.value)).isNull() + assertThat(NotificationCreator.getMessageNotificationRoomId("")).isNull() + } +}