fix: MEDIUM/LOW bugs - validation, unread tracking, TLS bounds, KeyPackage checks

- H17: Add unread count tracking to MarmotGroupChatroom - M8: Add MAX_OPAQUE_SIZE bounds check to TLS deserialization - M13: Add version/ciphersuite validation on KeyPackage deserialization - M24: Add logging before deleting corrupted group state in restoreAll - L1: Add size limit to sentKeys map in MlsGroup - Additional UI fixes: leave group cleanup, error handling improvements - Fix MarmotSubscriptionManagerTest for updated API https://claude.ai/code/session_018gVkmmYgMFtBH7G31pCk9N
2026-04-07 23:04:09 +00:00
parent 8c8ab4bb2c
commit 4526beb4be
10 changed files with 204 additions and 98 deletions
@@ -31,6 +31,11 @@ class TlsReader(
    private var position: Int = 0,
    private val limit: Int = data.size,
 ) {
+    companion object {
+        /** Maximum allowed size for a single opaque field (1 MB) */
+        const val MAX_OPAQUE_SIZE = 1_048_576
+    }
+
    val remaining: Int get() = limit - position

    val hasRemaining: Boolean get() = position < limit
@@ -87,12 +92,18 @@ class TlsReader(
    /** Read a variable-length opaque with 2-byte length prefix */
    fun readOpaque2(): ByteArray {
        val length = readUint16()
+        require(length <= MAX_OPAQUE_SIZE) {
+            "Opaque2 length $length exceeds maximum allowed size $MAX_OPAQUE_SIZE"
+        }
        return readBytes(length)
    }

    /** Read a variable-length opaque with 4-byte length prefix */
    fun readOpaque4(): ByteArray {
        val length = readUint32().toInt()
+        require(length <= MAX_OPAQUE_SIZE) {
+            "Opaque4 length $length exceeds maximum allowed size $MAX_OPAQUE_SIZE"
+        }
        return readBytes(length)
    }

@@ -130,6 +141,9 @@ class TlsReader(
    /** Read a variable-length opaque with QUIC-style VarInt length prefix */
    fun readOpaqueVarInt(): ByteArray {
        val length = readVarInt()
+        require(length <= MAX_OPAQUE_SIZE) {
+            "OpaqueVarInt length $length exceeds maximum allowed size $MAX_OPAQUE_SIZE"
+        }
        return readBytes(length)
    }

@@ -153,6 +153,7 @@ class MlsGroup private constructor(
            senderDataSecret = epochSecrets.senderDataSecret,
            encryptionSecret = epochSecrets.encryptionSecret,
            leafCount = tree.leafCount,
+            exporterSecret = epochSecrets.exporterSecret,
        )

    val memberCount: Int
@@ -25,8 +25,10 @@ import com.vitorpamplona.quartz.marmot.mls.codec.TlsWriter
 import com.vitorpamplona.quartz.marmot.mls.crypto.MlsCryptoProvider
 import com.vitorpamplona.quartz.marmot.mls.messages.CommitResult
 import com.vitorpamplona.quartz.marmot.mls.messages.KeyPackageBundle
+import com.vitorpamplona.quartz.marmot.mls.schedule.KeySchedule
 import com.vitorpamplona.quartz.marmot.mls.schedule.SecretTree
 import com.vitorpamplona.quartz.nip01Core.core.HexKey
+import com.vitorpamplona.quartz.utils.Log
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock

@@ -402,6 +404,33 @@ class MlsGroupManager(
            32,
        )

+    /**
+     * Return exporter secrets from retained epochs for a group.
+     *
+     * Used by the inbound processor to attempt outer decryption with
+     * previous epoch keys when the current epoch's key fails (e.g.,
+     * after a commit has advanced the epoch but late-arriving messages
+     * still use the old exporter key).
+     *
+     * @param nostrGroupId hex-encoded Nostr group ID
+     * @return list of retained exporter secrets (most recent first), each
+     *         derived via MLS-Exporter("marmot", "group-event", 32)
+     */
+    fun retainedExporterSecrets(nostrGroupId: HexKey): List<ByteArray> {
+        val retained = retainedEpochs[nostrGroupId] ?: return emptyList()
+        return retained
+            .filter { it.exporterSecret.isNotEmpty() }
+            .sortedByDescending { it.epoch }
+            .map { epochSecrets ->
+                KeySchedule.mlsExporter(
+                    epochSecrets.exporterSecret,
+                    "marmot",
+                    "group-event".encodeToByteArray(),
+                    32,
+                )
+            }
+    }
+
    // --- Private Helpers ---

    private fun requireGroup(nostrGroupId: HexKey): MlsGroup =
@@ -118,15 +118,20 @@ data class MlsKeyPackage(
    }

    companion object {
-        fun decodeTls(reader: TlsReader): MlsKeyPackage =
-            MlsKeyPackage(
-                version = reader.readUint16(),
-                cipherSuite = reader.readUint16(),
+        fun decodeTls(reader: TlsReader): MlsKeyPackage {
+            val version = reader.readUint16()
+            require(version == 1) { "Unsupported MLS version: $version" }
+            val cipherSuite = reader.readUint16()
+            require(cipherSuite == 1) { "Unsupported ciphersuite: $cipherSuite" }
+            return MlsKeyPackage(
+                version = version,
+                cipherSuite = cipherSuite,
                initKey = reader.readOpaqueVarInt(),
                leafNode = LeafNode.decodeTls(reader),
                extensions = reader.readVectorVarInt { Extension.decodeTls(it) },
                signature = reader.readOpaqueVarInt(),
            )
+        }
    }
 }

@@ -22,6 +22,7 @@ package com.vitorpamplona.quartz.marmot

 import com.vitorpamplona.quartz.marmot.mip03GroupMessages.GroupEvent
 import com.vitorpamplona.quartz.nip59Giftwrap.wraps.GiftWrapEvent
+import kotlinx.coroutines.test.runTest
 import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
@@ -38,65 +39,70 @@ class MarmotSubscriptionManagerTest {
    private val groupId2 = "c".repeat(64)

    @Test
-    fun testSubscribeGroup() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testSubscribeGroup() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
+            manager.subscribeGroup(groupId1)

-        assertTrue(manager.isSubscribed(groupId1))
-        assertEquals(setOf(groupId1), manager.activeGroupIds())
-    }
+            assertTrue(manager.isSubscribed(groupId1))
+            assertEquals(setOf(groupId1), manager.activeGroupIds())
+        }

    @Test
-    fun testSubscribeGroupWithSince() {
-        val manager = MarmotSubscriptionManager(userPubKey)
-        val since = 1700000000L
+    fun testSubscribeGroupWithSince() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)
+            val since = 1700000000L

-        manager.subscribeGroup(groupId1, since)
+            manager.subscribeGroup(groupId1, since)

-        assertTrue(manager.isSubscribed(groupId1))
+            assertTrue(manager.isSubscribed(groupId1))

-        val filters = manager.activeGroupFilters()
-        assertEquals(1, filters.size)
-        assertEquals(since, filters[0].since)
-    }
+            val filters = manager.activeGroupFilters()
+            assertEquals(1, filters.size)
+            assertEquals(since, filters[0].since)
+        }

    @Test
-    fun testUnsubscribeGroup() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testUnsubscribeGroup() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
-        manager.unsubscribeGroup(groupId1)
+            manager.subscribeGroup(groupId1)
+            manager.unsubscribeGroup(groupId1)

-        assertFalse(manager.isSubscribed(groupId1))
-        assertTrue(manager.activeGroupIds().isEmpty())
-    }
+            assertFalse(manager.isSubscribed(groupId1))
+            assertTrue(manager.activeGroupIds().isEmpty())
+        }

    @Test
-    fun testMultipleGroups() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testMultipleGroups() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
-        manager.subscribeGroup(groupId2)
+            manager.subscribeGroup(groupId1)
+            manager.subscribeGroup(groupId2)

-        assertEquals(setOf(groupId1, groupId2), manager.activeGroupIds())
+            assertEquals(setOf(groupId1, groupId2), manager.activeGroupIds())

-        val filters = manager.activeGroupFilters()
-        assertEquals(2, filters.size)
-    }
+            val filters = manager.activeGroupFilters()
+            assertEquals(2, filters.size)
+        }

    @Test
-    fun testUpdateGroupSince() {
-        val manager = MarmotSubscriptionManager(userPubKey)
-        val newSince = 1700000000L
+    fun testUpdateGroupSince() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)
+            val newSince = 1700000000L

-        manager.subscribeGroup(groupId1)
-        manager.updateGroupSince(groupId1, newSince)
+            manager.subscribeGroup(groupId1)
+            manager.updateGroupSince(groupId1, newSince)

-        val filters = manager.activeGroupFilters()
-        assertEquals(1, filters.size)
-        assertEquals(newSince, filters[0].since)
-    }
+            val filters = manager.activeGroupFilters()
+            assertEquals(1, filters.size)
+            assertEquals(newSince, filters[0].since)
+        }

    @Test
    fun testGiftWrapFilter() {
@@ -110,39 +116,42 @@ class MarmotSubscriptionManagerTest {
    }

    @Test
-    fun testGiftWrapFilterWithSince() {
-        val manager = MarmotSubscriptionManager(userPubKey)
-        val since = 1700000000L
+    fun testGiftWrapFilterWithSince() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)
+            val since = 1700000000L

-        manager.updateGiftWrapSince(since)
-        val filter = manager.giftWrapFilter()
+            manager.updateGiftWrapSince(since)
+            val filter = manager.giftWrapFilter()

-        assertEquals(since, filter.since)
-    }
+            assertEquals(since, filter.since)
+        }

    @Test
-    fun testActiveGroupFiltersContainCorrectKind() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testActiveGroupFiltersContainCorrectKind() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
-        val filters = manager.activeGroupFilters()
+            manager.subscribeGroup(groupId1)
+            val filters = manager.activeGroupFilters()

-        assertEquals(1, filters.size)
-        assertEquals(listOf(GroupEvent.KIND), filters[0].kinds)
-        assertNotNull(filters[0].tags)
-        assertEquals(listOf(groupId1), filters[0].tags!!["h"])
-    }
+            assertEquals(1, filters.size)
+            assertEquals(listOf(GroupEvent.KIND), filters[0].kinds)
+            assertNotNull(filters[0].tags)
+            assertEquals(listOf(groupId1), filters[0].tags!!["h"])
+        }

    @Test
-    fun testBuildFiltersIncludesBothTypes() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testBuildFiltersIncludesBothTypes() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
-        val allFilters = manager.buildFilters()
+            manager.subscribeGroup(groupId1)
+            val allFilters = manager.buildFilters()

-        // Should have 1 group filter + 1 gift wrap filter
-        assertEquals(2, allFilters.size)
-    }
+            // Should have 1 group filter + 1 gift wrap filter
+            assertEquals(2, allFilters.size)
+        }

    @Test
    fun testBuildFiltersWithNoGroupsHasGiftWrapOnly() {
@@ -164,31 +173,33 @@ class MarmotSubscriptionManagerTest {
    }

    @Test
-    fun testSyncWithGroupManager() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testSyncWithGroupManager() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        // Start with one group
-        manager.subscribeGroup(groupId1)
+            // Start with one group
+            manager.subscribeGroup(groupId1)

-        // Sync with group manager that has different groups
-        manager.syncWithGroupManager(setOf(groupId2))
+            // Sync with group manager that has different groups
+            manager.syncWithGroupManager(setOf(groupId2))

-        // groupId1 should be removed, groupId2 added
-        assertFalse(manager.isSubscribed(groupId1))
-        assertTrue(manager.isSubscribed(groupId2))
-    }
+            // groupId1 should be removed, groupId2 added
+            assertFalse(manager.isSubscribed(groupId1))
+            assertTrue(manager.isSubscribed(groupId2))
+        }

    @Test
-    fun testClear() {
-        val manager = MarmotSubscriptionManager(userPubKey)
+    fun testClear() =
+        runTest {
+            val manager = MarmotSubscriptionManager(userPubKey)

-        manager.subscribeGroup(groupId1)
-        manager.subscribeGroup(groupId2)
-        manager.updateGiftWrapSince(1700000000L)
+            manager.subscribeGroup(groupId1)
+            manager.subscribeGroup(groupId2)
+            manager.updateGiftWrapSince(1700000000L)

-        manager.clear()
+            manager.clear()

-        assertTrue(manager.activeGroupIds().isEmpty())
-        assertNull(manager.giftWrapFilter().since)
-    }
+            assertTrue(manager.activeGroupIds().isEmpty())
+            assertNull(manager.giftWrapFilter().since)
+        }
 }