diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt index e5928616a3..586f4cd0a4 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt @@ -244,8 +244,8 @@ class MarmotManager( * the safe direction: it blocks new local commits until the group actually * knows what happened to this one. */ - suspend fun retryPendingPublishObligations() { - val pending = publishGate.allPending() + suspend fun retryPendingPublishObligations(onlyGroupId: HexKey? = null) { + val pending = publishGate.allPending().filter { onlyGroupId == null || it.groupId == onlyGroupId } if (pending.isEmpty()) return Log.d("MarmotManager") { "retryPendingPublishObligations(): ${pending.size} unresolved" } for (obligation in pending) { @@ -433,6 +433,14 @@ class MarmotManager( val result = inboundProcessor.processWelcome(welcomeEvent, hintNostrGroupId) if (result is WelcomeResult.Joined) { + // An authenticated re-join is what clears a departure gate — the + // rule `LocalOutboundGate.REMOVED` states, and `LEAVING` needs it + // just as much: a member who left and was invited back holds a gate + // raised against a membership that no longer exists. Nothing else + // clears it, so without this the rejoined group is readable and + // permanently unsendable, and the gate's durability makes that + // survive every restart. + publishGate.clearGate(result.nostrGroupId) subscriptionManager.subscribeGroup(result.nostrGroupId) Log.d("MarmotManager") { "Joined group ${result.nostrGroupId}" } } @@ -1052,6 +1060,16 @@ class MarmotManager( stage: suspend () -> MlsGroupManager.StagedCommit, ): CommitPublication { requireOutboundAllowed(nostrGroupId, "commit a group-state change", ignoringGate) + // A commit whose publish went unconfirmed leaves the group in + // `PendingPublish`, which correctly refuses new commits — but the only + // thing that ever resolved it was `restoreAll`, so one dropped socket + // wedged the group until the app was restarted. Retrying this group's + // obligations here makes the next attempt the recovery: republishing + // the same event is safe (a peer deduplicates it by id) and a retry + // that still fails leaves the group held exactly as before. + if (publishGate.lifecycle(nostrGroupId) == GroupLifecycleState.PENDING_PUBLISH) { + retryPendingPublishObligations(onlyGroupId = nostrGroupId) + } check(publishGate.canPrepareLocalCommit(nostrGroupId, ignoringGate)) { "Group $nostrGroupId cannot prepare a local commit " + "(lifecycle=${publishGate.lifecycle(nostrGroupId)}, gate=${publishGate.outboundGate(nostrGroupId)})" diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotDisbandTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotDisbandTest.kt index 4ddf1943f6..547ec25c2b 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotDisbandTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotDisbandTest.kt @@ -260,6 +260,38 @@ class MarmotDisbandTest { assertEquals(LocalOutboundGate.DISBANDING, chatroom.outboundGate.value) } + @Test + fun `rejoining a group we left clears the departure gate`() = + runBlocking { + // Leaving raises a durable `Leaving` gate, and nothing but a + // re-join clears it. That was harmless while gates blocked nothing; + // now that they stop outbound work — and survive restarts — a group + // you left and were invited back to would be readable and + // permanently unsendable. + val alice = Fixture() + val bob = Fixture() + alice.createCurrentProfile() + + val kp = bob.manager.generateKeyPackageEvent(relays = emptyList()) + val (_, welcome) = alice.manager.addMember(nostrGroupId, kp, emptyList()) + bob.manager.ingest(welcome!!.giftWrapEvent) + + bob.manager.leaveGroup(nostrGroupId) + assertFailsWith("a leaving member may not send") { + bob.manager.buildTextMessage(nostrGroupId, "one more thing") + } + + // Invited back: a fresh KeyPackage, a fresh Welcome. + val rejoinKp = bob.manager.generateKeyPackageEvent(relays = emptyList()) + val (_, rejoinWelcome) = alice.manager.addMember(nostrGroupId, rejoinKp, emptyList()) + bob.manager.ingest(rejoinWelcome!!.giftWrapEvent) + + // The group has to be usable again — that is the whole point of + // being invited back. + bob.manager.buildTextMessage(nostrGroupId, "back again") + Unit + } + @Test fun `a pending disband request outlives a restart`() = runBlocking { diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishBeforeApplyTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishBeforeApplyTest.kt index 1d36ed7931..d84c56ca69 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishBeforeApplyTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishBeforeApplyTest.kt @@ -413,7 +413,22 @@ class MarmotPublishBeforeApplyTest { relays = listOf(relay), ) } - assertEquals(1, fx.publisher.published.size, "no replacement commit was offered") + // The invariant is that no REPLACEMENT COMMIT was minted for the + // held epoch — not that nothing went out. A blocked attempt now + // retries the stuck obligation on its way through, so the relay may + // legitimately see the same event twice; what it must never see is + // a second, different commit for the same epoch, because that is + // the fork publish-before-apply exists to prevent. Counting + // distinct ids says that, where counting sends only said it by + // accident. + assertEquals( + 1, + fx.publisher.published + .map { it.id } + .toSet() + .size, + "no replacement commit was offered; a re-send of the same event is not one", + ) } /** The group's own relay list is the default recipient scope. */ diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishDurabilityTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishDurabilityTest.kt index b168f05cdb..e85afc40ca 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishDurabilityTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishDurabilityTest.kt @@ -207,6 +207,56 @@ class MarmotPublishDurabilityTest { assertTrue(recordedBytes.isNotEmpty()) } + @Test + fun aWedgedGroupRecoversOnTheNextCommitWithoutARestart() = + runBlocking { + // `PendingPublish` correctly refuses new commits, but the only + // thing that ever resolved it was `restoreAll` — so a single + // dropped socket left the group unable to commit anything until the + // app was restarted. The next attempt has to BE the recovery. + val signer = NostrSignerInternal(KeyPair()) + val store = MemoryStateStore() + val obligations = MemoryObligationStore() + val publisher = SwitchablePublisher(accepts = false) + val groupId = "e".repeat(64) + + val manager = manager(signer, store, obligations, publisher) + manager.createGroup( + groupId, + MarmotGroupData( + nostrGroupId = groupId, + adminPubkeys = listOf(signer.pubKey), + relays = listOf(relay.url), + ), + ) + val founder = KeyPair() + manager.addMember( + nostrGroupId = groupId, + memberPubKey = founder.pubKey.toHexKey(), + keyPackageBytes = + manager.groupManager + .getGroup(groupId)!! + .createKeyPackage(founder.pubKey, ByteArray(0)) + .keyPackage + .toTlsBytes(), + keyPackageEventId = "f".repeat(64), + relays = listOf(relay), + ) + + // A commit the relay never acknowledged: the group is held. + runCatching { manager.setGroupProfile(groupId, "first try", "", listOf(relay)) } + assertEquals(GroupLifecycleState.PENDING_PUBLISH, manager.lifecycle(groupId)) + + // The relay is back. Without a restart, the next commit must clear + // the stuck obligation and then land. + publisher.accepts = true + manager.setGroupProfile(groupId, "second try", "", listOf(relay)) + + assertEquals(GroupLifecycleState.STABLE, manager.lifecycle(groupId)) + assertTrue(obligations.entries.isEmpty(), "the stuck obligation resolved on the way through") + assertEquals("second try", manager.groupView(groupId)?.name) + } + @Test fun aRetryThatFailsAgainKeepsTheGroupHeld() = runBlocking { diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt index c7635b6dc4..698be085ec 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt @@ -334,15 +334,31 @@ class MarmotInboundProcessor( GroupEventResult.Error(groupId, "Failed to process GroupEvent: ${e.message}", e) } - // Track processed events for dedup — except UndecryptableOuterLayer, - // which must stay retryable. These events are typically future-epoch - // arrivals buffered by the handler and replayed after a - // CommitProcessed advances our epoch; marking them processed here - // would cause the retry to hit the Duplicate early-return above and - // skip MLS decryption entirely. DoS is already bounded by the - // handler's per-group pending buffer. + // Track processed events for dedup — except the two results that must + // stay RETRYABLE, because for both of them "we saw this" is not the + // same as "we are done with this". + // + // UndecryptableOuterLayer is typically a future-epoch arrival buffered + // by the handler and replayed once a CommitProcessed advances our + // epoch; marking it processed would send the retry into the Duplicate + // early-return above and skip MLS decryption entirely. + // + // AppMessageOnCandidateBranch is the same shape one level up: the + // payload decrypted on a branch that was losing AT THE TIME, and + // convergence may still select that branch — this result is itself a + // witness FOR it. Remembering the id would mean a message that landed + // on the branch the group went on to adopt is dropped as a duplicate + // and never rendered, which is precisely backwards. Re-processing is + // safe: witnesses are a set keyed by sender account, so a resent + // payload adds nothing to a branch's standing and is admitted as + // ordinary rather than selection-relevant. + // + // DoS is bounded for both by the handler's per-group pending buffer. val idToRemember = messageId - if (idToRemember != null && result !is GroupEventResult.UndecryptableOuterLayer) { + if (idToRemember != null && + result !is GroupEventResult.UndecryptableOuterLayer && + result !is GroupEventResult.AppMessageOnCandidateBranch + ) { processedIdsMutex.withLock { processedMessageIds.add(idToRemember) // Trim the set if it exceeds the max size diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/appComponents/MarmotWebUrl.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/appComponents/MarmotWebUrl.kt index b0b4e71bc0..6200ad2ce8 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/appComponents/MarmotWebUrl.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/appComponents/MarmotWebUrl.kt @@ -224,12 +224,121 @@ object MarmotWebUrl { a >= 224 } + /** + * Decide an IPv6 literal on its BYTES, never on how it was spelled. + * + * Matching text was the bug: `::1` is one of many spellings of loopback, + * and `0:0:0:0:0:0:0:1` — the same address, fully expanded — matched + * nothing and read as routable. `::ffff:127.0.0.1` is worse still, because + * it is IPv4 loopback wearing an IPv6 coat and shares no prefix with any + * of the strings above. Both made a group avatar URL a way to have every + * member fetch from their own machine. + * + * An address this cannot parse is refused rather than allowed: "we could + * not tell" must not mean "go ahead", which is the same rule the IPv4 side + * applies to shapes like `0x7f.1`. + */ private fun isNonRoutableIpv6(addr: String): Boolean { - val a = addr.lowercase() - if (a == "::1" || a == "::") return true - // Unique-local (fc00::/7) and link-local (fe80::/10). - return a.startsWith("fc") || a.startsWith("fd") || a.startsWith("fe8") || - a.startsWith("fe9") || a.startsWith("fea") || a.startsWith("feb") + val bytes = parseIpv6(addr) ?: return true + + // An IPv4-mapped (::ffff:a.b.c.d) or IPv4-compatible (::a.b.c.d) + // address is really that IPv4 address, so it gets the IPv4 rules. + val v4Prefix = bytes.take(10).all { it.toInt() == 0 } + if (v4Prefix) { + val mapped = bytes[10].toInt() and 0xFF + val mapped2 = bytes[11].toInt() and 0xFF + if ((mapped == 0xFF && mapped2 == 0xFF) || (mapped == 0 && mapped2 == 0)) { + val packed = + ((bytes[12].toLong() and 0xFF) shl 24) or + ((bytes[13].toLong() and 0xFF) shl 16) or + ((bytes[14].toLong() and 0xFF) shl 8) or + (bytes[15].toLong() and 0xFF) + // `::` and `::1` land here too, and both are non-routable under + // the IPv4 rules (0.0.0.0 and 0.0.0.1 are in 0.0.0.0/8). + return isNonRoutableIpv4(packed) + } + } + + val first = bytes[0].toInt() and 0xFF + val second = bytes[1].toInt() and 0xFF + return when { + // Unique-local fc00::/7. + first == 0xFC || first == 0xFD -> true + // Link-local fe80::/10 — the top two bits of the second byte. + first == 0xFE && (second and 0xC0) == 0x80 -> true + // Multicast ff00::/8. + first == 0xFF -> true + else -> false + } + } + + /** + * The 16 bytes of an IPv6 literal, or null when it is not one. + * + * Handles `::` compression once, a trailing embedded IPv4 dotted quad, and + * a `%zone` suffix (dropped — a zone never makes an address more routable). + */ + private fun parseIpv6(addr: String): ByteArray? { + val text = addr.lowercase().substringBefore('%') + if (text.isEmpty()) return null + + val doubleColon = text.indexOf("::") + if (doubleColon != text.lastIndexOf("::")) return null + + val headText = if (doubleColon >= 0) text.substring(0, doubleColon) else text + val tailText = if (doubleColon >= 0) text.substring(doubleColon + 2) else "" + + val head = mutableListOf() + val tail = mutableListOf() + + // The embedded-IPv4 form is only legal as the last element, and it + // contributes two groups rather than one. + fun push( + into: MutableList, + piece: String, + isLast: Boolean, + ): Boolean { + if (piece.contains('.')) { + if (!isLast) return false + val quad = packIpv4(piece) ?: return false + if (piece.count { it == '.' } != 3) return false + into.add(((quad shr 16) and 0xFFFF).toInt()) + into.add((quad and 0xFFFF).toInt()) + return true + } + if (piece.isEmpty() || piece.length > 4) return false + val value = piece.toIntOrNull(16) ?: return false + into.add(value) + return true + } + + if (headText.isNotEmpty()) { + val pieces = headText.split(':') + pieces.forEachIndexed { i, piece -> + if (!push(head, piece, i == pieces.lastIndex && doubleColon < 0)) return null + } + } + if (tailText.isNotEmpty()) { + val pieces = tailText.split(':') + pieces.forEachIndexed { i, piece -> + if (!push(tail, piece, i == pieces.lastIndex)) return null + } + } + + val groups = + when { + doubleColon < 0 -> if (head.size == 8) head else return null + head.size + tail.size > 7 -> return null + else -> head + List(8 - head.size - tail.size) { 0 } + tail + } + if (groups.size != 8) return null + + val bytes = ByteArray(16) + groups.forEachIndexed { i, group -> + bytes[i * 2] = ((group shr 8) and 0xFF).toByte() + bytes[i * 2 + 1] = (group and 0xFF).toByte() + } + return bytes } /** Splits `host:port`, keeping an IPv6 literal's brackets on the host. */ diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/protocolCore/MarmotPublishGate.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/protocolCore/MarmotPublishGate.kt index 91bd6b769d..369233a9c1 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/protocolCore/MarmotPublishGate.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/protocolCore/MarmotPublishGate.kt @@ -434,10 +434,17 @@ class MarmotPublishGate( suspend fun forget(groupId: HexKey) { val ids = mutex.withLock { pending.values.filter { it.groupId == groupId }.map { it.obligationId } } ids.forEach { store.delete(it) } + // The gate has three copies now — the map, the snapshot every + // non-suspending reader sees, and the record on disk. Dropping only the + // map would leave `outboundGateNow` still reporting a gate for a group + // this client has forgotten, and `restore()` would bring it back on the + // next start. + store.deleteGate(groupId) mutex.withLock { ids.forEach { pending.remove(it) } lifecycles.remove(groupId) gates.remove(groupId) + gateSnapshot.value = gates.toMap() } } } diff --git a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/appComponents/GroupAvatarUrlV1Test.kt b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/appComponents/GroupAvatarUrlV1Test.kt index a488d12136..dab9c56995 100644 --- a/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/appComponents/GroupAvatarUrlV1Test.kt +++ b/quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/appComponents/GroupAvatarUrlV1Test.kt @@ -160,6 +160,40 @@ class GroupAvatarUrlV1Test { assertTrue(MarmotWebUrl.isSafeToContact("https://8.8.8.8/a.png")) } + @Test + fun anIpv6LiteralIsJudgedOnItsBytesNotItsSpelling() { + // Matching text let the same address through under another name. + // Loopback, in four spellings that are all ::1: + assertTrue("::1", !MarmotWebUrl.isSafeToContact("https://[::1]/a.png")) + assertTrue("expanded", !MarmotWebUrl.isSafeToContact("https://[0:0:0:0:0:0:0:1]/a.png")) + assertTrue("padded", !MarmotWebUrl.isSafeToContact("https://[0000:0000:0000:0000:0000:0000:0000:0001]/a.png")) + assertTrue("mixed case", !MarmotWebUrl.isSafeToContact("https://[::0001]/a.png")) + + // IPv4 loopback wearing an IPv6 coat — shares no prefix with any of + // the strings the old check compared against. + assertTrue("v4-mapped loopback", !MarmotWebUrl.isSafeToContact("https://[::ffff:127.0.0.1]/a.png")) + assertTrue("v4-compatible loopback", !MarmotWebUrl.isSafeToContact("https://[::127.0.0.1]/a.png")) + assertTrue("v4-mapped private", !MarmotWebUrl.isSafeToContact("https://[::ffff:10.0.0.1]/a.png")) + assertTrue("v4-mapped link-local", !MarmotWebUrl.isSafeToContact("https://[::ffff:169.254.169.254]/a.png")) + + assertTrue("unspecified", !MarmotWebUrl.isSafeToContact("https://[::]/a.png")) + assertTrue("unique local", !MarmotWebUrl.isSafeToContact("https://[fd00::1]/a.png")) + assertTrue("unique local fc", !MarmotWebUrl.isSafeToContact("https://[fc00::1]/a.png")) + assertTrue("link local", !MarmotWebUrl.isSafeToContact("https://[fe80::1]/a.png")) + // fe80::/10 is the top TEN bits, so febf is in range and fec0 is not — + // the old prefix test got this right by accident and wrong in general. + assertTrue("link local top of range", !MarmotWebUrl.isSafeToContact("https://[febf::1]/a.png")) + assertTrue("multicast", !MarmotWebUrl.isSafeToContact("https://[ff02::1]/a.png")) + // A zone index never makes an address more routable. + assertTrue("zoned link local", !MarmotWebUrl.isSafeToContact("https://[fe80::1%25eth0]/a.png")) + // Unparseable is refused, not waved through. + assertTrue("garbage", !MarmotWebUrl.isSafeToContact("https://[1:2:3::4::5]/a.png")) + + // A real, routable v6 address still works. + assertTrue(MarmotWebUrl.isSafeToContact("https://[2001:4860:4860::8888]/a.png")) + assertTrue(MarmotWebUrl.isSafeToContact("https://[2606:4700:4700::1111]/a.png")) + } + @Test fun aHostThatWouldNeedIdnaIsRefusedRatherThanGuessedAt() { // We do not implement IDNA/punycode, so we cannot produce the encoded