mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-11 08:47:33 +00:00
fix: address audit findings on the MLS unread/restore changes
- Clamp the seeded kind:445 subscription since at wall-clock now: the inner createdAt is sender-controlled, so a single future-dated message could push since past the present and silently skip genuinely new events on every restart. Covered by a new regression test. - Drop the remember() around the group-list unread count: the chatroom's message set can shrink without newestMessage or lastReadTime changing (pruning, kind:5 deletion of an older message), which left the cached count stale. The set is pruned to ~100 entries, so counting per recomposition is cheap. - Extract marmotGroupLastReadRoute(): the "MarmotGroup/<id>" last-read key was inlined at three call sites; a prefix drift between the mark-as-read side and the unread checks would silently reintroduce the bug this branch fixes. - Derive GROUP_EVENT_REFETCH_OVERLAP_SEC from TimeUtils.ONE_DAY instead of re-deriving 24*60*60.
This commit is contained in:
+1
-1
@@ -129,7 +129,7 @@ fun MarmotGroupChatView(
|
||||
feedContentState = feedViewModel.feedState,
|
||||
accountViewModel = accountViewModel,
|
||||
nav = nav,
|
||||
routeForLastRead = "MarmotGroup/$nostrGroupId",
|
||||
routeForLastRead = marmotGroupLastReadRoute(nostrGroupId),
|
||||
onWantsToReply = { note -> newMessageModel.reply(note) },
|
||||
onWantsToEditDraft = { },
|
||||
)
|
||||
|
||||
+31
@@ -0,0 +1,31 @@
|
||||
/*
|
||||
* Copyright (c) 2025 Vitor Pamplona
|
||||
*
|
||||
* Permission is hereby granted, free of charge, to any person obtaining a copy of
|
||||
* this software and associated documentation files (the "Software"), to deal in
|
||||
* the Software without restriction, including without limitation the rights to use,
|
||||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
|
||||
* Software, and to permit persons to whom the Software is furnished to do so,
|
||||
* subject to the following conditions:
|
||||
*
|
||||
* The above copyright notice and this permission notice shall be included in all
|
||||
* copies or substantial portions of the Software.
|
||||
*
|
||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
||||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
|
||||
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
|
||||
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
|
||||
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
|
||||
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.marmotGroup
|
||||
|
||||
import com.vitorpamplona.quartz.nip01Core.core.HexKey
|
||||
|
||||
/**
|
||||
* Key into the account's persisted last-read map (`lastReadPerRoute`) for a
|
||||
* Marmot MLS group. The mark-as-read side (the open chat's feed) and the
|
||||
* unread indicators (Messages screen row, group list row) must use the
|
||||
* identical key, or read state silently stops persisting for one of them.
|
||||
*/
|
||||
fun marmotGroupLastReadRoute(nostrGroupId: HexKey): String = "MarmotGroup/$nostrGroupId"
|
||||
+6
-5
@@ -226,11 +226,12 @@ fun MarmotGroupListItem(
|
||||
val memberPubkeys = remember(members) { members.map { it.pubkey } }
|
||||
val newestMessage = chatroom.newestMessage
|
||||
|
||||
val lastReadTime by accountViewModel.account.loadLastReadFlow("MarmotGroup/$groupId").collectAsStateWithLifecycle()
|
||||
val unread =
|
||||
remember(newestMessage, lastReadTime) {
|
||||
chatroom.messages.count { (it.createdAt() ?: Long.MIN_VALUE) > lastReadTime }
|
||||
}
|
||||
val lastReadTime by accountViewModel.account.loadLastReadFlow(marmotGroupLastReadRoute(groupId)).collectAsStateWithLifecycle()
|
||||
// Not remembered: chatroom.messages can shrink without newestMessage or
|
||||
// lastReadTime changing (pruning, kind:5 deletion of an older message),
|
||||
// so caching on those keys would serve a stale count. The set is pruned
|
||||
// to ~100 entries, so counting per recomposition is cheap.
|
||||
val unread = chatroom.messages.count { (it.createdAt() ?: Long.MIN_VALUE) > lastReadTime }
|
||||
|
||||
Row(
|
||||
modifier =
|
||||
|
||||
+2
-1
@@ -63,6 +63,7 @@ import com.vitorpamplona.amethyst.ui.note.ObserveDraftEvent
|
||||
import com.vitorpamplona.amethyst.ui.note.elements.TimeAgoStyle
|
||||
import com.vitorpamplona.amethyst.ui.note.elements.ToggleableTimeAgoText
|
||||
import com.vitorpamplona.amethyst.ui.screen.loggedIn.AccountViewModel
|
||||
import com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.marmotGroup.marmotGroupLastReadRoute
|
||||
import com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.privateDM.header.RoomNameDisplay
|
||||
import com.vitorpamplona.amethyst.ui.screen.loggedIn.chats.publicChannels.ephemChat.LoadEphemeralChatChannel
|
||||
import com.vitorpamplona.amethyst.ui.stringRes
|
||||
@@ -273,7 +274,7 @@ private fun MarmotGroupRoomCompose(
|
||||
stringRes(R.string.marmot_group_no_messages_yet)
|
||||
}
|
||||
|
||||
val lastReadTime by accountViewModel.account.loadLastReadFlow("MarmotGroup/${chatroom.nostrGroupId}").collectAsStateWithLifecycle()
|
||||
val lastReadTime by accountViewModel.account.loadLastReadFlow(marmotGroupLastReadRoute(chatroom.nostrGroupId)).collectAsStateWithLifecycle()
|
||||
|
||||
ChannelName(
|
||||
channelIdHex = chatroom.nostrGroupId,
|
||||
|
||||
+8
-2
@@ -51,6 +51,7 @@ import com.vitorpamplona.quartz.nip01Core.tags.people.pTags
|
||||
import com.vitorpamplona.quartz.nip18Reposts.quotes.QEventTag
|
||||
import com.vitorpamplona.quartz.nip18Reposts.quotes.quote
|
||||
import com.vitorpamplona.quartz.utils.Log
|
||||
import com.vitorpamplona.quartz.utils.TimeUtils
|
||||
import kotlin.io.encoding.Base64
|
||||
import kotlin.io.encoding.ExperimentalEncodingApi
|
||||
|
||||
@@ -124,7 +125,7 @@ class MarmotManager(
|
||||
if (messageStore == null) return emptyMap()
|
||||
val result = mutableMapOf<HexKey, Long>()
|
||||
for (groupId in groupIds) {
|
||||
val newest =
|
||||
val newestStored =
|
||||
loadStoredMessages(groupId).maxOfOrNull { json ->
|
||||
try {
|
||||
Event.fromJson(json).createdAt
|
||||
@@ -133,6 +134,11 @@ class MarmotManager(
|
||||
0L
|
||||
}
|
||||
} ?: continue
|
||||
// Clamp at wall-clock now: inner createdAt is sender-controlled,
|
||||
// and a single future-dated message must not push `since` past
|
||||
// the present — that would skip genuinely new events on every
|
||||
// restart until a fresher message arrives.
|
||||
val newest = minOf(newestStored, TimeUtils.now())
|
||||
if (newest > GROUP_EVENT_REFETCH_OVERLAP_SEC) {
|
||||
result[groupId] = newest - GROUP_EVENT_REFETCH_OVERLAP_SEC
|
||||
}
|
||||
@@ -755,7 +761,7 @@ class MarmotManager(
|
||||
* are timestamped at the same send, so the window only needs to
|
||||
* absorb relay/system clock skew and out-of-order publishes.
|
||||
*/
|
||||
internal const val GROUP_EVENT_REFETCH_OVERLAP_SEC = 24L * 60 * 60
|
||||
internal val GROUP_EVENT_REFETCH_OVERLAP_SEC: Long = TimeUtils.ONE_DAY.toLong()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+47
@@ -27,10 +27,13 @@ import com.vitorpamplona.quartz.marmot.mls.group.MlsGroupStateStore
|
||||
import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair
|
||||
import com.vitorpamplona.quartz.nip01Core.relay.normalizer.RelayUrlNormalizer
|
||||
import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal
|
||||
import com.vitorpamplona.quartz.utils.TimeUtils
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertNotNull
|
||||
import kotlin.test.assertNull
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* Restart behavior of [MarmotManager.restoreAll].
|
||||
@@ -84,6 +87,50 @@ class MarmotManagerRestoreTest {
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testRestoreAllClampsFutureDatedMessagesAtNow() {
|
||||
runBlocking {
|
||||
val signer = NostrSignerInternal(KeyPair())
|
||||
val mlsStore = InMemoryStateStore()
|
||||
val messageStore = InMemoryMessageStore()
|
||||
val kpStore = InMemoryBundleStore()
|
||||
|
||||
val manager = MarmotManager(signer, mlsStore, messageStore, kpStore)
|
||||
manager.createGroup(
|
||||
nostrGroupId,
|
||||
MarmotGroupData(
|
||||
nostrGroupId = nostrGroupId,
|
||||
name = "restore-clamp",
|
||||
relays = listOf(relay.url),
|
||||
),
|
||||
)
|
||||
// The inner createdAt is sender-controlled: a single future-dated
|
||||
// message must not push the seeded `since` past wall-clock now,
|
||||
// or the filter would skip genuinely new events after restart.
|
||||
val futureCreatedAt = TimeUtils.now() + 30L * 24 * 60 * 60
|
||||
val futureJson =
|
||||
"""{"id":"${"c".repeat(64)}","pubkey":"${"d".repeat(64)}","created_at":$futureCreatedAt,"kind":9,"tags":[],"content":"from the future","sig":""}"""
|
||||
manager.persistDecryptedMessage(nostrGroupId, futureJson)
|
||||
|
||||
val beforeRestore = TimeUtils.now()
|
||||
val restarted = MarmotManager(signer, mlsStore, messageStore, kpStore)
|
||||
restarted.restoreAll()
|
||||
val afterRestore = TimeUtils.now()
|
||||
|
||||
val since =
|
||||
restarted.subscriptionManager
|
||||
.activeGroupFilters()
|
||||
.single()
|
||||
.since
|
||||
assertNotNull(since, "a future-dated message must still seed a clamped since")
|
||||
assertTrue(
|
||||
since >= beforeRestore - MarmotManager.GROUP_EVENT_REFETCH_OVERLAP_SEC &&
|
||||
since <= afterRestore - MarmotManager.GROUP_EVENT_REFETCH_OVERLAP_SEC,
|
||||
"since must be clamped to now - overlap, was $since",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testRestoreAllLeavesSinceUnsetWithoutStoredMessages() {
|
||||
runBlocking {
|
||||
|
||||
Reference in New Issue
Block a user