fix(marmot): align MIP compliance tests with held-back v2 and signed commits

Two drifts surfaced as 7 failing tests on the jvmTest run: 1. MarmotGroupData CURRENT_VERSION is held at 2 for mdk-core interop (see the const-doc block), but three tests still assumed v3: - marmotGroupData_defaultVersionIsThree asserted the constant directly. - marmotGroupData_roundTripWithDisappearingSecs and buildGroupEvent_appendsExpirationTagWhenDisappearingConfigured relied on the default version emitting the v3-only disappearing_message_secs field, which the encoder correctly skips for v2. Rename the first to `currentVersionIsHeldAtTwoForMdkInterop` with a spec-cross-reference comment and an extra assertion that v3 is still in SUPPORTED_VERSIONS. Pin version=3 explicitly in the other two so they exercise the v3 path regardless of the interop hold-back. 2. MlsGroup.processCommit now requires a non-empty FramedContentTBS signature on non-external commits (per RFC 9420 §6.1, introduced in 0c970945). Four pipeline tests still called the raw processCommit overload with signature=ByteArray(0), so they all failed with "FramedContentTBS signature missing on commit from leaf 0". Add a `processFramedCommit` wrapper on MlsGroupManager that mirrors MlsGroup.processFramedCommit + the retention/persistence bookkeeping from processCommit, and switch the four tests to feed framedCommitBytes through it. MarmotInboundProcessor is unaffected (it already has the decoded PublicMessage fields). All 7 originally-failing Marmot tests now pass; the 4 unrelated NostrClient network tests remain independently flaky. https://claude.ai/code/session_01XQNAmwn1y87GAoK94QWgyn
2026-04-22 16:04:59 +00:00
parent 491c58dbfb
commit 64ab67a6fc
4 changed files with 62 additions and 39 deletions
@@ -307,6 +307,32 @@ class MlsGroupManager(
        persistGroup(nostrGroupId)
    }

+    /**
+     * Process a fully-framed `MlsMessage(PublicMessage(Commit))` — the wire shape
+     * returned in [CommitResult.framedCommitBytes]. Delegates to
+     * [MlsGroup.processFramedCommit] which extracts the sender leaf index,
+     * confirmation_tag, and FramedContentTBS signature from the envelope, then
+     * calls [MlsGroup.processCommit] with all fields properly populated.
+     *
+     * Intended for callers that hold the framed bytes directly (tests driving
+     * peer managers, CLI interop harnesses). Production inbound goes through
+     * `MarmotInboundProcessor`, which unwraps the outer ChaCha20 layer first
+     * and then calls [processCommit] with the already-decoded PublicMessage
+     * fields.
+     */
+    suspend fun processFramedCommit(
+        nostrGroupId: HexKey,
+        framedCommitBytes: ByteArray,
+    ) = mutex.withLock {
+        val group = requireGroup(nostrGroupId)
+        val retainedBefore = group.retainedSecrets()
+
+        group.processFramedCommit(framedCommitBytes)
+
+        pushRetainedEpoch(nostrGroupId, retainedBefore)
+        persistGroup(nostrGroupId)
+    }
+
    // --- Message Encryption/Decryption ---

    /**
@@ -60,8 +60,17 @@ class MarmotMipComplianceTest {
    // ---------------------------------------------------------------------- MIP-01

    @Test
-    fun marmotGroupData_defaultVersionIsThree() {
-        assertEquals(3, MarmotGroupData.CURRENT_VERSION)
+    fun marmotGroupData_currentVersionIsHeldAtTwoForMdkInterop() {
+        // MIP-01 spec targets v3, but mdk-core (whitenoise-rs' MLS engine)
+        // rejects v3 NostrGroupData payloads with `ExtensionFormatError`
+        // ("Trailing bytes in NostrGroupDataExtension"), violating the spec's
+        // forward-compat rule. Until mdk-core ships the fix, Amethyst holds
+        // CURRENT_VERSION at 2 for cross-client interop; v3 stays parseable
+        // (see disappearing_message_secs round-trip test below). Bump this
+        // assertion back to 3 in lockstep with the MarmotGroupData constant
+        // once mdk publishes the parser fix.
+        assertEquals(2, MarmotGroupData.CURRENT_VERSION)
+        assertTrue(3 in MarmotGroupData.SUPPORTED_VERSIONS, "v3 MUST remain parseable for forward compat")
    }

    @Test
@@ -89,8 +98,13 @@ class MarmotMipComplianceTest {

    @Test
    fun marmotGroupData_roundTripWithDisappearingSecs() {
+        // disappearing_message_secs is a v3+ field. CURRENT_VERSION is held
+        // at 2 for MDK interop (see marmotGroupData_currentVersionIsHeldAtTwoForMdkInterop),
+        // so this test pins v3 explicitly to exercise the v3 encoder/decoder
+        // path end-to-end.
        val original =
            MarmotGroupData(
+                version = 3,
                nostrGroupId = groupId32,
                adminPubkeys = listOf(adminPubkey),
                relays = listOf("wss://relay.example/"),
@@ -202,8 +202,12 @@ class MarmotMipBehaviorTest {
            val manager = createGroupManager()
            manager.createGroup(groupId, aliceId.hexToByteArray())

+            // disappearing_message_secs is a v3+ field. CURRENT_VERSION is held at 2 for
+            // MDK interop, so the encoder only emits the field for version ≥ 3 — pin
+            // version=3 here so the outbound processor actually sees the setting.
            val configured =
                MarmotGroupData(
+                    version = 3,
                    nostrGroupId = groupId,
                    adminPubkeys = listOf(aliceId),
                    disappearingMessageSecs = 3600UL,
@@ -472,12 +472,10 @@ class MarmotPipelineTest {
            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),
-            )
+            // Feed the framed PublicMessage envelope so MlsGroup.processCommit
+            // can verify the RFC 9420 §6.1 FramedContentTBS signature (now
+            // mandatory on non-external commits).
+            edenMgr.processFramedCommit(groupId, addFred.framedCommitBytes)

            val preRestartEpoch = davidMgr.getGroup(groupId)!!.epoch
            val preRestartExporter = davidMgr.exporterSecret(groupId)
@@ -571,12 +569,10 @@ class MarmotPipelineTest {
            // 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),
-            )
+            // Feed the framed PublicMessage envelope so MlsGroup.processCommit
+            // can verify the RFC 9420 §6.1 FramedContentTBS signature (now
+            // mandatory on non-external commits).
+            edenMgr.processFramedCommit(groupId, addFred.framedCommitBytes)

            // Pre-restart sanity: all three share the same outer exporter key.
            val preRestartDavidKey = davidMgr.exporterSecret(groupId)
@@ -655,13 +651,10 @@ class MarmotPipelineTest {
            // Bob (existing member at the pre-add-Fred epoch) receives and
            // applies Alice's add-Fred commit to reach the same epoch as
            // Alice + Fred. Without this, Bob can't decrypt Fred's subsequent
-            // application messages.
-            bobMgr.processCommit(
-                nostrGroupId = groupId,
-                commitBytes = addFred.commitBytes,
-                senderLeafIndex = aliceMgr.getGroup(groupId)!!.leafIndex,
-                confirmationTag = ByteArray(0),
-            )
+            // application messages. Feed the framed PublicMessage envelope so
+            // MlsGroup.processCommit can verify the mandatory RFC 9420 §6.1
+            // FramedContentTBS signature.
+            bobMgr.processFramedCommit(groupId, addFred.framedCommitBytes)

            // Sanity: Fred is at leafIndex=2, leafCount=3.
            assertEquals(2, fredMgr.getGroup(groupId)!!.leafIndex)
@@ -809,24 +802,10 @@ class MarmotPipelineTest {
                    addCarolCommitEvent.signedEvent.content,
                    bobExporterAtE1,
                )
-            val mlsMessage =
-                com.vitorpamplona.quartz.marmot.mls.framing.MlsMessage
-                    .decodeTls(
-                        com.vitorpamplona.quartz.marmot.mls.codec
-                            .TlsReader(decryptedMlsBytes),
-                    )
-            val publicMessage =
-                com.vitorpamplona.quartz.marmot.mls.framing.PublicMessage
-                    .decodeTls(
-                        com.vitorpamplona.quartz.marmot.mls.codec
-                            .TlsReader(mlsMessage.payload),
-                    )
-            bobMgr.processCommit(
-                nostrGroupId = groupId,
-                commitBytes = publicMessage.content,
-                senderLeafIndex = publicMessage.sender.leafIndex,
-                confirmationTag = publicMessage.confirmationTag!!,
-            )
+            // Feed the already-decoded MlsMessage(PublicMessage(Commit)) envelope
+            // through processFramedCommit so MlsGroup can verify the RFC 9420 §6.1
+            // FramedContentTBS signature from the sender's PublicMessage.
+            bobMgr.processFramedCommit(groupId, decryptedMlsBytes)

            val epochAfterAddCarol = aliceMgr.getGroup(groupId)!!.epoch
            assertEquals(epochAfterAddBob + 1, epochAfterAddCarol)