From f1241e891bb46ae9bbcdbfb48a3a1a23f653f894 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 17:11:45 +0000 Subject: [PATCH] fix(quartz): compaction must carry the authority-gated head, not the chain head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from an independent review of this branch (a different model, per CONTRIBUTING-WITH-AI.md). Two were real, and the first is a regression this branch introduced. compactControlPlane picked its per-entity head with a bare structural chain walk, which is worse than the raw max-version it replaced. With no floor, foldEntity anchors at the lowest-version edition carrying no `prev` — and after a PRIOR compaction the genuine head's `prev` dangles into a trimmed epoch by design. So a forged `version = 1, prev = null` decoy outranks a real v50→v52 chain, and because nothing in this path checks a signature it became the entity's entire carried-forward state. A forged empty banlist would have erased every ban at the next Refounding. Reproduced, then fixed by selecting the owner-rooted authority-gated head — the same edition ConcordCommunityState.fold would seat, so the new epoch starts where the old one left off, and an unprivileged author cannot influence the choice at all. recoverStrandedConcordCommunities derived its new ban gate with `?.isBanned(..) == true`, which reads "not banned" when the session does not exist yet or its first fold has not landed. The sweep runs on the revision tick, so a banned member's own client would have hit that window on cold start and recovered itself — the exact bypass the gate exists to stop. It now fails closed and retries on the next sweep. Also from the review: resolve() now warns when the ban fixpoint exhausts its pass cap without settling, instead of silently returning a roster folded under a mask that no longer matches its banlist; and banGate stops lowercasing the same author three times. Two review findings are accepted rather than fixed, and recorded on the PR: the anchor tie-break picks the lowest rumor id before testing whether that candidate connects (pre-existing, and changing it is consensus-affecting), and non-owner moderators now need a resolved roster before a verb succeeds, which is the intended fail-closed trade. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DrJhpFhhLjuDJQNkGvYMGj --- .../amethyst/model/AccountConcordActions.kt | 16 +++- .../commons/actions/ConcordActions.kt | 2 + .../commons/actions/ConcordActionsTest.kt | 1 + .../model/concord/ConcordRollbackFloorTest.kt | 6 +- .../concord/cord04Roles/AuthorityResolver.kt | 18 ++++- .../concord/cord06Rekey/ConcordRefounding.kt | 38 ++++++---- .../cord06Rekey/ConcordRefoundingTest.kt | 76 +++++++++++++++++++ .../cord06Rekey/ControlRootRotationTest.kt | 2 + 8 files changed, 138 insertions(+), 21 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt index fc812f1bb8..f659e38564 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt @@ -852,6 +852,7 @@ class AccountConcordActions( recipientsXOnly = recipients, staffXOnly = staff, createdAt = TimeUtils.now(), + ownerPubKey = entry.owner, ) // 4. Publish the compacted Control Plane (the new epoch's state) then the rekey blobs @@ -1123,13 +1124,24 @@ class AccountConcordActions( // walks them straight back into the epoch they were rotated out of — see A2 in // docs/concord-soft-ban-audit.md. Read off the epoch we are LEAVING, which is the last // one whose Control Plane we can still fold. - val bannedHere = + // + // Fails CLOSED. `?.isBanned(..) == true` reads "not banned" for a session that does not + // exist yet or whose first fold has not landed, and this sweep runs on the revision tick + // — so a banned member's own client would have hit that window on cold start and + // recovered itself, which is precisely the bypass this gate exists to stop. No verdict + // means no recovery; the next sweep retries once the roster is known. + val authority = account.concordSessions .sessionFor(entry.id) ?.state ?.value ?.authority - ?.isBanned(account.signer.pubKey) == true + if (authority == null) { + Log.i("Concord") { "Stranded-recovery check deferred for ${entry.id}: control plane not folded yet" } + lastConcordRecoveryCheck.remove(entry.id) + continue + } + val bannedHere = authority.isBanned(account.signer.pubKey) val merged = ConcordActions.recoverStranded(entry, bundle, bannedHere) ?: continue if (!adoptedConcordRotations.add("${entry.id}:${merged.rootEpoch}")) continue Log.i("Concord", "Stranded recovery: ${entry.id} ${entry.rootEpoch} -> ${merged.rootEpoch}") diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt index dff1143c45..e70fe18364 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActions.kt @@ -545,6 +545,7 @@ object ConcordActions { recipientsXOnly: List, staffXOnly: Set, createdAt: Long, + ownerPubKey: HexKey, ): RefoundingBuild = ConcordRefounding.build( rotatorSigner = rotatorSigner, @@ -558,6 +559,7 @@ object ConcordActions { recipientsXOnly = recipientsXOnly, staffXOnly = staffXOnly, createdAt = createdAt, + ownerPubKey = ownerPubKey, ) /** diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActionsTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActionsTest.kt index a5d8ff772d..82169a7708 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActionsTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordActionsTest.kt @@ -123,6 +123,7 @@ class ConcordActionsTest { // Only the owner is staff, so only the owner's blob carries the secret. staffXOnly = setOf(owner.pubKey), createdAt = 5L, + ownerPubKey = owner.pubKey, ) val baseRekey = ConcordActions.nextBaseRekeyPlane(community.communityRoot, community.communityId, community.rootEpoch) diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/concord/ConcordRollbackFloorTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/concord/ConcordRollbackFloorTest.kt index d3c1a50a75..ae06a84891 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/concord/ConcordRollbackFloorTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/model/concord/ConcordRollbackFloorTest.kt @@ -78,7 +78,7 @@ class ConcordRollbackFloorTest { // new epoch's plane is split and addressed by the derived signer, not the root. val newControlRoot = ByteArray(32) { 0x44 } val newControl = ControlPlaneKeys.forStaff(newRoot, community.communityId, newEpoch, newControlRoot) - val rolledBack = ConcordRefounding.compactControlPlane(community.genesisWraps, community.controlPlane, newControl) + val rolledBack = ConcordRefounding.compactControlPlane(community.genesisWraps, community.controlPlane, newControl, community.ownerPubKey) val entry = ConcordCommunityListEntry( @@ -145,7 +145,7 @@ class ConcordRollbackFloorTest { val newControlRoot = ByteArray(32) { 0x44 } val newControl = ControlPlaneKeys.forStaff(newRoot, community.communityId, newEpoch, newControlRoot) // Honest: compacted from the FULL prior plane, so each entity's head (metadata v1) survives. - val honest = ConcordRefounding.compactControlPlane(epoch0Wraps, community.controlPlane, newControl) + val honest = ConcordRefounding.compactControlPlane(epoch0Wraps, community.controlPlane, newControl, community.ownerPubKey) val entry = ConcordCommunityListEntry( @@ -186,7 +186,7 @@ class ConcordRollbackFloorTest { // new epoch's plane is split and addressed by the derived signer, not the root. val newControlRoot = ByteArray(32) { 0x44 } val newControl = ControlPlaneKeys.forStaff(newRoot, community.communityId, newEpoch, newControlRoot) - val compacted = ConcordRefounding.compactControlPlane(community.genesisWraps, community.controlPlane, newControl) + val compacted = ConcordRefounding.compactControlPlane(community.genesisWraps, community.controlPlane, newControl, community.ownerPubKey) val entry = ConcordCommunityListEntry( diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolver.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolver.kt index 126eaade9b..5677e56244 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolver.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolver.kt @@ -21,6 +21,7 @@ package com.vitorpamplona.quartz.concord.cord04Roles import com.vitorpamplona.quartz.nip01Core.core.toHexKey +import com.vitorpamplona.quartz.utils.Log /** * Resolves the owner-rooted authority state of a Concord community from its @@ -132,6 +133,8 @@ data class AuthorityResolver private constructor( } companion object { + private const val TAG = "ConcordAuthorityResolver" + /** The owner's rank — supreme and unremovable. No Role may claim it. */ const val OWNER_RANK = 0L @@ -205,6 +208,14 @@ data class AuthorityResolver private constructor( if (result.banned == mask) return result mask = result.banned } + // Exhausted the cap without settling. The returned roster was folded under a mask that is + // no longer the banlist beside it, so this is reported rather than swallowed — the same + // reasoning as EditionFold.LOG_GAP: an unsettled fold is either an adversarial edition set + // or a rule of ours that does not converge, and both are things a reader wants to know. + Log.w(TAG) { + "Banlist resolution did not settle in $MAX_BAN_RESOLUTION_PASSES passes for owner $ownerPubKey " + + "(${editions.size} editions, ${result.banned.size} banned): keeping the last pass" + } return result } @@ -350,9 +361,10 @@ data class AuthorityResolver private constructor( // a concurrent ban is never lost, while an on-chain unban still takes effect. val allBanlist = editions.filter { it.entityKind == ControlEntityKind.BANLIST } - fun banGate(e: ControlEdition): Boolean = - e.author.lowercase() == ownerLower || - (e.author.lowercase() !in bannedAuthors && effectivePermissionsOf(e.author.lowercase()).has(ConcordPermissions.BAN)) + fun banGate(e: ControlEdition): Boolean { + val author = e.author.lowercase() + return author == ownerLower || (author !in bannedAuthors && effectivePermissionsOf(author).has(ConcordPermissions.BAN)) + } val authorizedBanlist = allBanlist.filter(::banGate) // CORD-04 §3's rank rule binds "every action", and it names banning as its example ("an diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefounding.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefounding.kt index 7dc80de9cb..74fb73b594 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefounding.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefounding.kt @@ -20,8 +20,8 @@ */ package com.vitorpamplona.quartz.concord.cord06Rekey +import com.vitorpamplona.quartz.concord.cord02Community.ConcordCommunityState import com.vitorpamplona.quartz.concord.cord04Roles.ControlEdition -import com.vitorpamplona.quartz.concord.cord04Roles.EditionFold import com.vitorpamplona.quartz.concord.crypto.ConcordKeyDerivation import com.vitorpamplona.quartz.concord.crypto.ControlPlaneKeys import com.vitorpamplona.quartz.concord.crypto.GroupKey @@ -124,11 +124,12 @@ object ConcordRefounding { recipientsXOnly: List, staffXOnly: Set, createdAt: Long, + ownerPubKey: HexKey, ): RefoundingBuild { val newEpoch = rootEpoch + 1 val newControlKeys = ControlPlaneKeys.forStaff(newRoot, communityId, newEpoch, newControlRoot) - val controlWraps = compactControlPlane(priorControlWraps, priorControlKeys, newControlKeys) + val controlWraps = compactControlPlane(priorControlWraps, priorControlKeys, newControlKeys, ownerPubKey) val baseRekeyKey = ConcordKeyDerivation.baseRekeyAddress(priorRoot, communityId, newEpoch) val prevCommit = ConcordKeyDerivation.epochKeyCommitment(rootEpoch, priorRoot).toHexKey() @@ -167,6 +168,7 @@ object ConcordRefounding { priorWraps: List, priorControlKeys: ControlPlaneKeys, newControlKeys: ControlPlaneKeys, + ownerPubKey: HexKey, ): List { // entity coordinate -> every edition we can open, paired with its verified seal. val byCoordinate = HashMap>>() @@ -177,17 +179,27 @@ object ConcordRefounding { byCoordinate.getOrPut(coord) { ArrayList() }.add(edition to opened.seal) } - // The head is the CHAIN head, not the highest version. Picking by raw version made an honest - // rotator the delivery mechanism for a disconnected stray: an edition minted at an arbitrary - // version never joins the chain, but it won this comparison and was then re-wrapped into the - // new epoch as that entity's whole history — where a fresh joiner, holding no floor, anchors - // on it as their baseline. See B1 in `docs/concord-soft-ban-audit.md`. foldEntity walks from - // genesis and keeps the fresh-joiner fallback for a head whose own `prev` dangles into an - // epoch this rotator no longer holds, which is the ordinary shape after a prior compaction. - val out = ArrayList(byCoordinate.size) - for ((_, entries) in byCoordinate) { - val head = EditionFold.foldEntity(entries.map { it.first }) ?: continue - val seal = entries.firstOrNull { it.first.rumorId == head.rumorId }?.second ?: continue + // The head to carry forward is the one every READER honors — the authority-gated head — not + // the highest version and not the bare structural chain head. + // + // Raw highest version made an honest rotator the delivery mechanism for a disconnected stray: + // an edition minted at an arbitrary version never joins the chain, but it won that comparison + // and was re-wrapped into the new epoch as the entity's whole history (B1 in + // `docs/concord-soft-ban-audit.md`). The bare chain walk is *worse*, and this is the trap: + // with no floor it anchors at the lowest-version edition carrying no `prev`, and after a prior + // compaction the real head's `prev` dangles by design — so a forged `version = 1, prev = null` + // decoy outranks a genuine v50→v52 chain and, because nothing here checks signatures, becomes + // the entity's entire carried-forward state. A forged empty banlist would erase every ban. + // + // Gating on the owner-rooted roster is the only selection that cannot be gamed by an + // unprivileged author, and it is exactly what ConcordCommunityState.fold would seat, so the + // compacted epoch starts where the previous one left off. + val editions = byCoordinate.values.flatten() + val honored = ConcordCommunityState.authorizedHeads(editions.map { it.first }, ownerPubKey) + val out = ArrayList(honored.size) + for ((_, floor) in honored) { + val head = floor.known ?: continue + val seal = editions.firstOrNull { it.first.rumorId == head.rumorId }?.second ?: continue out.add(ConcordStreamEnvelope.wrapSeal(seal, newControlKeys, createdAt = seal.createdAt)) } return out diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefoundingTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefoundingTest.kt index 06778c4c71..dbc6e9c81d 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefoundingTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ConcordRefoundingTest.kt @@ -74,6 +74,7 @@ class ConcordRefoundingTest { recipientsXOnly = listOf(alice.pubKey, bob.pubKey), staffXOnly = setOf(owner.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) assertEquals(community.rootEpoch + 1, build.newEpoch) @@ -113,6 +114,7 @@ class ConcordRefoundingTest { recipientsXOnly = listOf(alice.pubKey), staffXOnly = setOf(owner.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) val newControl = build.newControlKeys @@ -184,6 +186,7 @@ class ConcordRefoundingTest { recipientsXOnly = listOf(alice.pubKey), staffXOnly = setOf(owner.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) val newControl = build.newControlKeys @@ -223,6 +226,7 @@ class ConcordRefoundingTest { recipientsXOnly = listOf(alice.pubKey), staffXOnly = setOf(owner.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) val baseRekeyKey = ConcordKeyDerivation.baseRekeyAddress(community.communityRoot, community.communityId, build.newEpoch) @@ -230,4 +234,76 @@ class ConcordRefoundingTest { val wrongRoot = ByteArray(32) { 0x11 } assertNull(ConcordRefounding.findNewRoot(build.rekeyWraps, baseRekeyKey, alice, community.communityId, wrongRoot, community.rootEpoch)) } + + @Test + fun compactionRefusesAForgedGenesisAndCarriesTheAuthorizedHead() = + runTest { + // A compaction re-wraps ONE edition per entity and nothing downstream re-checks the + // choice, so how that edition is picked is a security decision, not a detail. + // + // Raw highest-version lets a stray at an arbitrary version through. But the bare + // structural chain walk is worse: with no floor it anchors at the lowest-version edition + // carrying no `prev`, and after a PRIOR compaction the real head's `prev` dangles into a + // trimmed epoch by design — so a forged `version = 1, prev = null` decoy outranks a + // genuine v50→v52 chain, and becomes the entity's entire carried-forward state. A forged + // empty banlist would erase every ban that way. Only the owner-rooted gate is safe. + val community = ConcordCommunityFactory.create(owner, "Test", now) + val communityId = community.communityId + val control = community.controlPlane + + // The metadata entity, already compacted once: its head chains from an epoch we no longer hold. + val danglingPrev = ByteArray(32) { 0x7F } + val realHead = + ConcordStreamEnvelope.wrap( + ControlEditionBuilder.rumor( + owner.pubKey, + ControlEntityKind.METADATA, + communityId, + 50, + danglingPrev, + ConcordJson.instance.encodeToString(MetadataEntity.serializer(), MetadataEntity(name = "Real")), + now, + null, + ), + control, + owner, + encrypted = false, + createdAt = now, + ) + + // carol holds nothing at all and mints a genesis-shaped decoy at version 1. + val forged = + ConcordStreamEnvelope.wrap( + ControlEditionBuilder.rumor( + carol.pubKey, + ControlEntityKind.METADATA, + communityId, + 1, + null, + ConcordJson.instance.encodeToString(MetadataEntity.serializer(), MetadataEntity(name = "PWNED")), + now, + null, + ), + control, + carol, + encrypted = false, + createdAt = now, + ) + + val newEpoch = community.rootEpoch + 1 + val newControl = + com.vitorpamplona.quartz.concord.crypto.ControlPlaneKeys + .forStaff(newRoot, communityId, newEpoch, newControlRoot) + val compacted = ConcordRefounding.compactControlPlane(listOf(realHead, forged), control, newControl, owner.pubKey) + + val carried = + compacted + .mapNotNull { ConcordStreamEnvelope.openOrNull(it, newControl) } + .mapNotNull { ControlEdition.fromRumor(it.rumor) } + .filter { it.entityKind == ControlEntityKind.METADATA } + + assertEquals(1, carried.size, "one metadata edition carried forward") + assertEquals(50, carried.single().version, "the owner's real head, not the forged genesis") + assertEquals("Real", ConcordJson.decodeOrNull(carried.single().content)?.name) + } } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ControlRootRotationTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ControlRootRotationTest.kt index bcd5eec1e5..7a4440e1a0 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ControlRootRotationTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord06Rekey/ControlRootRotationTest.kt @@ -109,6 +109,7 @@ class ControlRootRotationTest { recipientsXOnly = listOf(owner.pubKey, moderator.pubKey, member.pubKey), staffXOnly = setOf(owner.pubKey, moderator.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) val baseRekey = ConcordKeyDerivation.baseRekeyAddress(community.communityRoot, community.communityId, build.newEpoch) @@ -149,6 +150,7 @@ class ControlRootRotationTest { recipientsXOnly = listOf(owner.pubKey, member.pubKey), staffXOnly = setOf(owner.pubKey), createdAt = now, + ownerPubKey = owner.pubKey, ) // The rotator's own view writes; a member's view of the same epoch only reads.