laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-10-05 19:28:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(marmot): five defects from the pre-merge audit

Browse Source 
A review of the full branch diff turned up five, all verified against the code
before changing anything.

**A departure gate nothing ever cleared.** `leaveGroup` raises a durable
`LEAVING` gate and the only `clearGate` calls were inside
`resolveDisbandRequest`, so a member who left and was invited back held a gate
against a membership that no longer existed. Harmless until this branch, where
gates began blocking outbound work AND surviving restarts — which turned it
into a group that reads fine and can never be written to again, permanently. An
authenticated re-join now clears it, which is the rule `REMOVED` already
stated.

**An SSRF hole in IPv6 avatar hosts.** `isNonRoutableIpv6` compared TEXT, so
`::1` was caught and `0:0:0:0:0:0:0:1` — the same address, expanded — was not,
and `::ffff:127.0.0.1` shares no prefix with anything it looked for. A group
avatar URL could make every member fetch from their own machine. Addresses are
now parsed to their 16 bytes and judged numerically, with IPv4-mapped and
-compatible forms delegated to the existing IPv4 rules and an unparseable
literal refused rather than waved through.

**A message on a branch the group then adopted was never rendered.** An app
payload that decrypted only on a candidate branch had its id recorded as
processed, so a later redelivery hit the `Duplicate` early-return — even though
that result is itself a witness FOR the branch, which convergence may go on to
select. It is now retryable like `UndecryptableOuterLayer`. Safe to
re-process: witnesses are a set keyed by sender, so a resent payload adds
nothing to a branch's standing.

**One dropped socket wedged a group until app restart.** An unconfirmed publish
pins `PendingPublish`, and the only caller of `retryPendingPublishObligations`
was `restoreAll`. A blocked commit now retries that group's obligations on its
way through, so the next attempt is the recovery.

`MarmotPublishBeforeApplyTest` measured "no replacement commit" by counting
sends, which the retry breaks without violating anything: the re-send carries
the SAME event id, and a fork means a second DIFFERENT commit for the held
epoch. It now counts distinct ids, which is the property it always meant.

**`forget()` left two of the gate's three copies behind.** It dropped the map
but not the snapshot non-suspending readers see, nor the record on disk, so
`restore()` resurrected a gate for a forgotten group. Latent — no production
caller yet.

11,001 tests green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kCuA6tc4JQzHPCDd39GHq
This commit is contained in:
Claude
2026-09-11 02:53:37 +00:00
parent 105d5bd217
commit 025116acc8
8 changed files with 297 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotManager.kt
+20 -2
View File
@@ -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)})"
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotDisbandTest.kt
+32
View File
@@ -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<IllegalStateException>("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 {
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishBeforeApplyTest.kt
+16 -1
View File
@@ -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. */
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/marmot/MarmotPublishDurabilityTest.kt
+50
View File
@@ -207,6 +207,56 @@ class MarmotPublishDurabilityTest {
assertTrue(recordedBytes.isNotEmpty())
}
@Test
fun aWedgedGroupRecoversOnTheNextCommitWithoutARestart() =
runBlocking<Unit> {
// `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<Unit> {
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/MarmotInboundProcessor.kt
+24 -8
View File
@@ -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
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/appComponents/MarmotWebUrl.kt
+114 -5
View File
@@ -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<Int>()
val tail = mutableListOf<Int>()
// The embedded-IPv4 form is only legal as the last element, and it
// contributes two groups rather than one.
fun push(
into: MutableList<Int>,
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. */
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/marmot/protocolCore/MarmotPublishGate.kt
+7
View File
@@ -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()
}
}
}
quartz/src/jvmAndroidTest/kotlin/com/vitorpamplona/quartz/marmot/appComponents/GroupAvatarUrlV1Test.kt
+34
View File
@@ -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