fix(marmot): reject past/future-epoch commits instead of partially applying

After a group creator restarts the app, their own relay-echoed kind:445 commit events (add-member, updateGroupExtensions, etc.) stop being filtered by `inboundProcessor.processedEventIds` — that in-memory dedup set doesn't survive process death. The echo then: 1. Outer-decrypts via a retained epoch key (we still hold those). 2. Gets parsed as a `PublicMessage` with `contentType = COMMIT`. 3. Flows into `applyCommit` → `groupManager.processCommit` → `group.processCommit`. `MlsGroup.processCommit` is not atomic: it runs proposal application, transcript-hash recomputation, epoch advance, key-schedule derivation, and SecretTree rebuild **in place** before reaching the final confirmation-tag check. If that check fails — which it always does for an already-applied commit, because the tag was computed against the original epoch's confirmation key and we're now several epochs past — the exception fires after the mutations, leaving the group context at a bogus "epoch + 1" with a completely new exporter secret. Net result on the wire: the creator's next outbound kind:445 is encrypted with this rogue exporter secret. Every other member is at the real epoch, so they all log "Outer decryption failed with current and N retained epoch key(s)" and no one receives the message. The creator's own self-echo still decrypts because his sentKeys / self-state are internally consistent; only external observers are cut off. Fix: in `MarmotInboundProcessor.applyCommit`, after parsing the PublicMessage header but **before** calling `processCommit`, compare `pubMsg.epoch` with the current local `group.epoch`. - `pubMsg.epoch < current` → commit is from the past (either our own already-applied echo or a stale relay replay). Return `GroupEventResult.Duplicate` and don't touch local state. - `pubMsg.epoch > current` → we're behind; the inbound pipeline upstream needs to feed us the intervening commits in order. Return an error describing the gap without touching local state. - Otherwise → proceed to `processCommit` as before. Making `processCommit` itself atomic would be nicer, but is a much larger refactor touching tree mutations, key-schedule state, and per-sender ratchets. The epoch check catches the production scenario at the right boundary — we never call into `processCommit` with bytes that we know won't apply cleanly. Regression test: - `testCreatorDoesNotDoubleApplyOwnCommitAfterRestart`: David creates a group, adds Eden and Fred, persists. Simulates David's app restart by rebuilding the `MlsGroupManager` from the store. Then feeds David's own add-Eden commit back through the inbound pipeline — exactly what a relay echo would deliver. Asserts the result is `Duplicate`, and that David's epoch and exporter secret are unchanged afterwards. Before the fix, his epoch advances by one and his exporter secret drifts, which is exactly what the production logs showed. - `testCreatorCanSendMessageAfterRestart`: save/restore round-trip sanity check that the restored manager's exporter secret matches what the members still at the same epoch hold, and that the first post-restart message from the creator is decryptable by an un-restarted member. https://claude.ai/code/session_014zfdNeeKAfU1zGGyFw4bUL
2026-04-21 02:31:12 +00:00
parent 621fe9c8c5
commit 7ee5f64fd8
2 changed files with 227 additions and 11 deletions
@@ -467,17 +467,44 @@ class MarmotInboundProcessor(
                WireFormat.PUBLIC_MESSAGE -> {
                    val pubMsg = PublicMessage.decodeTls(TlsReader(mlsMessage.payload))
                    val tag = pubMsg.confirmationTag
-                    if (tag == null) {
-                        GroupEventResult.Error(groupId, "PublicMessage commit missing confirmation_tag")
-                    } else {
-                        groupManager.processCommit(
-                            nostrGroupId = groupId,
-                            commitBytes = pubMsg.content,
-                            senderLeafIndex = pubMsg.sender.leafIndex,
-                            confirmationTag = tag,
-                        )
-                        val group = groupManager.getGroup(groupId)
-                        GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0)
+                    val currentEpoch = groupManager.getGroup(groupId)?.epoch
+                    when {
+                        tag == null -> {
+                            GroupEventResult.Error(groupId, "PublicMessage commit missing confirmation_tag")
+                        }
+
+                        // Reject commits that are not for our current epoch.
+                        // Happens most commonly when our own already-applied
+                        // commit is echoed back from the relay after an app
+                        // restart (the in-memory dedup set is cleared), and
+                        // the outer layer decrypts via a retained epoch key.
+                        // Calling `processCommit` on a past-epoch commit
+                        // partially mutates tree / groupContext / epochSecrets
+                        // before throwing on the confirmation-tag check,
+                        // leaving the local state diverged from every other
+                        // member's — they then can't decrypt anything we
+                        // send next.
+                        currentEpoch != null && pubMsg.epoch < currentEpoch -> {
+                            GroupEventResult.Duplicate(groupId)
+                        }
+
+                        currentEpoch != null && pubMsg.epoch > currentEpoch -> {
+                            GroupEventResult.Error(
+                                groupId,
+                                "Commit epoch ${pubMsg.epoch} is ahead of local epoch $currentEpoch; ignoring",
+                            )
+                        }
+
+                        else -> {
+                            groupManager.processCommit(
+                                nostrGroupId = groupId,
+                                commitBytes = pubMsg.content,
+                                senderLeafIndex = pubMsg.sender.leafIndex,
+                                confirmationTag = tag,
+                            )
+                            val group = groupManager.getGroup(groupId)
+                            GroupEventResult.CommitProcessed(groupId, group?.epoch ?: 0)
+                        }
                    }
                }

@@ -428,6 +428,195 @@ class MarmotPipelineTest {
        }
    }

+    @Test
+    fun testCreatorDoesNotDoubleApplyOwnCommitAfterRestart() {
+        // Root cause: after David restarts, his own add-member kind:445 that
+        // got echoed back from the relay is no longer marked as "already
+        // processed" (the dedup set lives in memory and isn't persisted).
+        // The inbound path then outer-decrypts it via a retained epoch key
+        // and feeds the commit into group.processCommit — which is NOT
+        // atomic, so it partially advances David's local state before failing
+        // with "Confirmation tag verification failed" (the tag was computed
+        // at the original epoch; David is already two epochs past).
+        //
+        // After this partial apply, David's epoch / tree / exporter_secret
+        // drift forward by one epoch while Eden and Fred stay put. Any
+        // subsequent message David sends uses a key nobody else has, and
+        // they log "Outer decryption failed with current and N retained
+        // epoch key(s)".
+        runBlocking {
+            val davidStore = TestGroupStateStore()
+            val davidMgr = MlsGroupManager(davidStore)
+            val edenMgr = MlsGroupManager(TestGroupStateStore())
+            val fredMgr = MlsGroupManager(TestGroupStateStore())
+            val davidId = ByteArray(32) { 0xD1.toByte() }
+            val edenId = ByteArray(32) { 0xE2.toByte() }
+            val fredId = ByteArray(32) { 0xF3.toByte() }
+
+            davidMgr.createGroup(groupId, davidId)
+            davidMgr.updateGroupExtensions(
+                groupId,
+                listOf(
+                    com.vitorpamplona.quartz.marmot.mip01Groups
+                        .MarmotGroupData(
+                            nostrGroupId = groupId,
+                            adminPubkeys = listOf(davidId.toHexKey()),
+                        ).toExtension(),
+                ),
+            )
+            val davidGroup = davidMgr.getGroup(groupId)!!
+            val edenBundle = davidGroup.createKeyPackage(edenId, ByteArray(0))
+            val fredBundle = davidGroup.createKeyPackage(fredId, ByteArray(0))
+
+            val addEden = davidMgr.addMember(groupId, edenBundle.keyPackage.toTlsBytes())
+            edenMgr.processWelcome(addEden.welcomeBytes!!, edenBundle)
+            val addFred = davidMgr.addMember(groupId, fredBundle.keyPackage.toTlsBytes())
+            fredMgr.processWelcome(addFred.welcomeBytes!!, fredBundle)
+            edenMgr.processCommit(
+                groupId,
+                addFred.commitBytes,
+                davidMgr.getGroup(groupId)!!.leafIndex,
+                ByteArray(0),
+            )
+
+            val preRestartEpoch = davidMgr.getGroup(groupId)!!.epoch
+            val preRestartExporter = davidMgr.exporterSecret(groupId)
+
+            // Simulate David's app restart.
+            val davidMgr2 = MlsGroupManager(davidStore)
+            davidMgr2.restoreAll()
+
+            // Now replay David's own echoed add-Eden commit that's still on
+            // the relay. After restart, the inbound dedup set is empty so
+            // the inbound pipeline does the full outer-decrypt via retained
+            // keys + processCommit on an already-applied commit.
+            val inbound =
+                com.vitorpamplona.quartz.marmot
+                    .MarmotInboundProcessor(
+                        groupManager = davidMgr2,
+                        keyPackageRotationManager =
+                            com.vitorpamplona.quartz.marmot.mip00KeyPackages
+                                .KeyPackageRotationManager(),
+                    )
+            val outbound = MarmotOutboundProcessor(davidMgr2)
+
+            // Re-encrypt the add-Eden framed commit with the pre-commit key
+            // so it looks exactly like what the relay would re-deliver.
+            val echoed =
+                outbound.buildCommitEvent(
+                    nostrGroupId = groupId,
+                    commitBytes = addEden.framedCommitBytes,
+                    exporterKey = addEden.preCommitExporterSecret,
+                )
+            val result = inbound.processGroupEvent(echoed.signedEvent)
+            assertIs<GroupEventResult.Duplicate>(
+                result,
+                "echoed already-applied commit must be treated as a no-op Duplicate",
+            )
+
+            val postEchoEpoch = davidMgr2.getGroup(groupId)!!.epoch
+            val postEchoExporter = davidMgr2.exporterSecret(groupId)
+
+            assertEquals(
+                preRestartEpoch,
+                postEchoEpoch,
+                "processing our own already-applied commit echo must NOT advance the local epoch",
+            )
+            kotlin.test.assertContentEquals(
+                preRestartExporter,
+                postEchoExporter,
+                "processing our own already-applied commit echo must NOT change the exporter secret",
+            )
+        }
+    }
+
+    @Test
+    fun testCreatorCanSendMessageAfterRestart() {
+        // Reproduces production symptom: David creates a group, adds Eden and
+        // Fred, sends messages — all fine. After David restarts the app
+        // (simulated here by save-state + fresh MlsGroupManager + restoreAll),
+        // his outbound kind:445 outer ChaCha20 layer suddenly uses a
+        // different key than Eden/Fred, and they fail with
+        // "Outer decryption failed with current and N retained epoch key(s)".
+        runBlocking {
+            val davidStore = TestGroupStateStore()
+            val davidMgr = MlsGroupManager(davidStore)
+            val edenMgr = MlsGroupManager(TestGroupStateStore())
+            val fredMgr = MlsGroupManager(TestGroupStateStore())
+
+            val davidId = ByteArray(32) { 0xD1.toByte() }
+            val edenId = ByteArray(32) { 0xE2.toByte() }
+            val fredId = ByteArray(32) { 0xF3.toByte() }
+
+            davidMgr.createGroup(groupId, davidId)
+            davidMgr.updateGroupExtensions(
+                nostrGroupId = groupId,
+                extensions =
+                    listOf(
+                        com.vitorpamplona.quartz.marmot.mip01Groups
+                            .MarmotGroupData(
+                                nostrGroupId = groupId,
+                                adminPubkeys = listOf(davidId.toHexKey()),
+                            ).toExtension(),
+                    ),
+            )
+            val davidGroup = davidMgr.getGroup(groupId)!!
+            val edenBundle = davidGroup.createKeyPackage(edenId, ByteArray(0))
+            val fredBundle = davidGroup.createKeyPackage(fredId, ByteArray(0))
+
+            // David adds Eden.
+            val addEden = davidMgr.addMember(groupId, edenBundle.keyPackage.toTlsBytes())
+            edenMgr.processWelcome(addEden.welcomeBytes!!, edenBundle)
+
+            // David adds Fred; Eden processes Alice's add-Fred commit.
+            val addFred = davidMgr.addMember(groupId, fredBundle.keyPackage.toTlsBytes())
+            fredMgr.processWelcome(addFred.welcomeBytes!!, fredBundle)
+            edenMgr.processCommit(
+                nostrGroupId = groupId,
+                commitBytes = addFred.commitBytes,
+                senderLeafIndex = davidMgr.getGroup(groupId)!!.leafIndex,
+                confirmationTag = ByteArray(0),
+            )
+
+            // Pre-restart sanity: all three share the same outer exporter key.
+            val preRestartDavidKey = davidMgr.exporterSecret(groupId)
+            kotlin.test.assertContentEquals(
+                preRestartDavidKey,
+                edenMgr.exporterSecret(groupId),
+                "Eden's exporter key should match David's before restart",
+            )
+            kotlin.test.assertContentEquals(
+                preRestartDavidKey,
+                fredMgr.exporterSecret(groupId),
+                "Fred's exporter key should match David's before restart",
+            )
+
+            // Simulate David's app restart: drop his in-memory manager and
+            // rebuild it from the persisted store.
+            val davidMgr2 = MlsGroupManager(davidStore)
+            davidMgr2.restoreAll()
+
+            val postRestartDavidKey = davidMgr2.exporterSecret(groupId)
+            kotlin.test.assertContentEquals(
+                preRestartDavidKey,
+                postRestartDavidKey,
+                "David's exporter key must survive a save+restore round-trip; " +
+                    "otherwise Eden/Fred can't decrypt his next outer layer.",
+            )
+
+            // David sends a message post-restart; Eden and Fred must decrypt.
+            val outbound = MarmotOutboundProcessor(davidMgr2)
+            val msg = "hi after restart"
+            val postRestartEvent =
+                outbound.buildGroupEventFromBytes(groupId, msg.encodeToByteArray())
+
+            val edenKey = edenMgr.exporterSecret(groupId)
+            val mlsBytesEden = GroupEventEncryption.decrypt(postRestartEvent.signedEvent.content, edenKey)
+            val edenDecrypted = edenMgr.decrypt(groupId, mlsBytesEden)
+            assertEquals(msg, edenDecrypted.content.decodeToString())
+        }
+    }
+
    @Test
    fun testFredEncryptsAfterJoiningThreeMemberGroup() {
        // Reproduces the production StackOverflowError: the last-joined member