From 64ab67a6fcba444c0d5c3d836ae4aad460dacbac Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Apr 2026 16:04:59 +0000 Subject: [PATCH] fix(marmot): align MIP compliance tests with held-back v2 and signed commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../marmot/mls/group/MlsGroupManager.kt | 26 +++++++++ .../quartz/marmot/MarmotMipComplianceTest.kt | 18 ++++++- .../quartz/marmot/MarmotMipBehaviorTest.kt | 4 ++ .../quartz/marmot/MarmotPipelineTest.kt | 53 ++++++------------- 4 files changed, 62 insertions(+), 39 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt index 565db9b15..ca5f592c5 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/mls/group/MlsGroupManager.kt @@ -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 --- /** diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipComplianceTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipComplianceTest.kt index 201f79e63..552d5275c 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipComplianceTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipComplianceTest.kt @@ -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/"), diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt index cedb62ea3..174de21f7 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotMipBehaviorTest.kt @@ -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, diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt index a819ad36e..ebfa6775d 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/MarmotPipelineTest.kt @@ -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)