diff --git a/amethyst/plans/2026-07-20-v1.13.0-release-qa.md b/amethyst/plans/2026-07-20-v1.13.0-release-qa.md index 7ebfd52193..e71cc638ab 100644 --- a/amethyst/plans/2026-07-20-v1.13.0-release-qa.md +++ b/amethyst/plans/2026-07-20-v1.13.0-release-qa.md @@ -102,17 +102,19 @@ pinned messages were never exercised. cannot ban a peer admin)", restated as step 3 of §5. Only §4, the section that defines the Banlist, omits the rank half — and both independent implementations read §4 in isolation and made the same mistake. Spec: (`04.md`). - So the fold change is spec-*mandated*, not a unilateral divergence. It is still - consensus-affecting (we would ignore bans Armada honors until they ship), so it wants - coordination rather than a race. Full write-up to share upstream: - `docs/concord-banlist-rank-conformance.md`. - Amethyst currently refuses only to *author* such a ban (`Account.concordBanTarget` and the Members - roster both route through `canActOn`), which restricts what we write, never what we accept. Three - `@Ignore`-d tests in `AuthorityResolverTest` record the fold-level invariant; un-ignore them when - we enforce. - Also reproduced, and covered in the write-up: a banned member holding BAN can lift their own ban - (the gate reads role-derived permissions, so bans do not stick against any BAN holder), and a - forked ban survives an unban that does not chain onto it. + **FIXED and shipping** — `AuthorityResolver` now enforces §3 as a *delta rule* (an edition may only + add/remove npubs its signer strictly outranks; the owner is never a valid target; unpermitted + entries are ignored rather than rejecting the edition, so a bulk-ban survives). The UI and the + ban/unban write path route through it too — `ConcordModeration.currentBanned` now reads the + *honored* banlist via the resolver instead of decoding the raw head, which also closes a + laundering path where our own next ban would re-publish an unauthorized entry under our signature. + **Known consequence: Armada has not shipped this, so banlists can differ between clients** — + we ignore a ban Armada honors when the signer did not outrank the target. Deliberate. + Write-up to send upstream: `docs/concord-banlist-rank-conformance.md`. + Still open, both covered in the write-up: a banned member holding BAN can lift their own ban (the + gate reads role-derived permissions, so bans do not stick against any BAN holder — this one is a + genuine fixpoint-ordering question and needs a spec ruling), and a forked ban survives an unban + that does not chain onto it. - Notification cards whose target note isn't in `LocalCache` render "Event is loading or can't be found in your relay list" (seen on old zaps). `tagsAnEventByUser` needs the reacted-to note loaded, so deep history stays partially unresolved. Cosmetic, pre-existing. diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordModeration.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordModeration.kt index cd879c549d..3e3e9b1de0 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordModeration.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/actions/ConcordModeration.kt @@ -22,6 +22,7 @@ package com.vitorpamplona.amethyst.commons.actions import com.vitorpamplona.quartz.concord.cord02Community.ConcordCommunityState import com.vitorpamplona.quartz.concord.cord04Roles.AuthorityCitation +import com.vitorpamplona.quartz.concord.cord04Roles.AuthorityResolver import com.vitorpamplona.quartz.concord.cord04Roles.ChannelEntity import com.vitorpamplona.quartz.concord.cord04Roles.ConcordJson import com.vitorpamplona.quartz.concord.cord04Roles.ControlEdition @@ -213,16 +214,20 @@ object ConcordModeration { owner: HexKey, ): Event = setBanlist(actor, controlPlane, communityId, currentBanned(current, communityId, owner) - member.lowercase(), current, createdAt, citation, owner) - /** The current banlist union across the head editions (lowercase hex). */ + /** + * The current banlist union across the head editions (lowercase hex). + * + * Read through the [AuthorityResolver] rather than by decoding the head's content directly, so + * this is the *honored* banlist: the resolver heals concurrent forks into the union (CORD-04 §4 + * re-heal) and drops entries whose signer did not outrank them (§3's rank rule, enforced as a + * delta rule). Decoding the raw head instead would make every ban/unban we author re-publish + * entries our own fold refuses — laundering an unauthorized ban into a list signed by us. + */ fun currentBanned( current: List, communityId: ByteArray, owner: HexKey, - ): Set { - val entityId = ConcordKeyDerivation.banlistCoordinate(communityId) - val head = headOf(current, entityId, owner) - return head?.let { ConcordJson.decodeBanlist(it.content) }?.mapTo(HashSet()) { it.lowercase() } ?: emptySet() - } + ): Set = AuthorityResolver.resolve(current, owner).bannedMembers() private suspend fun setBanlist( actor: NostrSigner, diff --git a/docs/concord-banlist-rank-conformance.md b/docs/concord-banlist-rank-conformance.md index a980e4e391..afbe98fdbd 100644 --- a/docs/concord-banlist-rank-conformance.md +++ b/docs/concord-banlist-rank-conformance.md @@ -1,6 +1,7 @@ # Concord: the Banlist is not rank-gated in any implementation (CORD-04 conformance) -**Status:** conformance bug, reproduced in Amethyst, present by inspection in Armada. +**Status:** conformance bug. Reproduced in Amethyst and **fixed there** (see §6); present by +inspection in Armada. **Severity:** privilege escalation. Any `BAN` holder can neutralise every authority above them, including the owner. **Reported by:** Amethyst (MIT), 2026-07-20. Findings verified by unit test; see "Evidence" below. @@ -193,29 +194,34 @@ Separately, please rule on **#3**: whether a banned npub's Banlist edition is ho §4 ("drops every event from a banned npub — message, reaction, edit, or authority action") is that it must not be, but the fixpoint ordering needs to be stated for that to be implementable consistently. -## 6. A note on rollout +## 6. Rollout status -This is a consensus-affecting change: a client that enforces the rank rule will ignore bans that a -client which does not will honor, so the Banlist can diverge between implementations until both -sides ship. We would rather coordinate that than race it. +**Amethyst enforces the rule described in §5 as of this report** — both on the fold +(`AuthorityResolver`) and on what it will author (UI + the ban/unban write path, which now reads the +*honored* banlist so an unauthorized entry can never be laundered into a list we sign). -Amethyst has therefore **not** changed its fold. We have only stopped our UI from *authoring* a ban -the actor does not outrank — which restricts what we write, never what we accept, and so cannot -diverge anyone's view. We are ready to enforce on the fold as soon as there is agreement on the rule -above and a rough sense of timing. +We're flagging the consequence plainly: this is consensus-affecting. Until Armada ships the same +rule, the two clients can show different banlists — Amethyst will ignore a ban that Armada honors +whenever the signer did not outrank the target. We judged shipping the spec-conformant behaviour +better than continuing to honor an escalation, but we recognise that is a decision with a cost for +your users as well as ours, and we're happy to discuss timing, or to adjust if you read §3/§5 +differently than we do. + +If it is useful, our implementation is MIT and the delta rule is about 40 lines in +`AuthorityResolver.resolve`; you're welcome to lift the approach outright. ## 7. Evidence Reproductions live in Amethyst's `AuthorityResolverTest` -(`quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/`), currently `@Ignore`d -with this issue as the reason so they document the invariant without failing the build: +(`quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/`). Each failed before +the change and passes after it: - `aBanHolderCannotBanAMemberItDoesNotOutrank` - `aBanHolderCannotBanTheOwner` -- `anUnrankedBanIsDroppedWithoutOrphaningTheRestOfTheList` (asserts the "ignore, don't reject" - semantics proposed above) +- `anUnrankedBanIsDroppedWithoutOrphaningTheRestOfTheList` (pins the "ignore, don't reject" + semantics of §5) -Two companion tests, which pass today, pin the behaviour a fix must preserve: +Two companions pin the behaviour the fix had to preserve, and passed throughout: - `aBanHolderStillBansThoseItOutranks` - `theOwnerBansAnyone` 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 f9ccfe1e74..8f67a24b5a 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 @@ -256,13 +256,74 @@ class AuthorityResolver private constructor( fun banGate(e: ControlEdition): Boolean = e.author.lowercase() == ownerLower || effectivePermissionsOf(e.author.lowercase()).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 + // admin cannot ban a peer admin"); §5 step 3 restates it. Only §4, which defines the + // Banlist, states the BAN-bit half alone — which is why every implementation (ours and + // Armada's) shipped the bit check without the rank check, letting the most junior BAN + // holder ban the admins above them and the owner. See + // docs/concord-banlist-rank-conformance.md. + // + // The rule is stated per TARGET, but the Banlist is one whole-list document, so it is + // enforced as a DELTA rule: an edition may only add or remove npubs its signer strictly + // outranks. Entries it may not act on are ignored and the rest of the edition applies — + // rejecting the whole edition would discard the bulk-ban §4 recommends as the collision + // remedy, and would let a rogue grief the list by forcing rejections. + fun canBanTarget( + author: String, + target: String, + ): Boolean { + // Position 0 is "supreme and unremovable" (§2) and nothing may outrank it, so the + // owner is never a valid target — not even for themselves. + if (target == ownerLower) return false + if (author == ownerLower) return true + if (!effectivePermissionsOf(author).has(ConcordPermissions.BAN)) return false + val authorRank = rankOf(author) ?: return false + val targetRank = rankOf(target) ?: Long.MAX_VALUE // no roles ⇒ lowest authority + return authorRank < targetRank + } + + val byHash = allBanlist.associateBy { it.hashHex } + val effective = HashMap>() + + // The list an edition actually establishes: its parent's effective list, plus only the + // additions its signer may make and minus only the removals its signer may make. Walks + // the parent chain, so it is memoized; `visiting` also terminates a prevHash cycle. + fun effectiveList( + edition: ControlEdition, + visiting: MutableSet, + ): Set { + effective[edition.hashHex]?.let { return it } + if (!visiting.add(edition.hashHex)) return emptySet() + + val parent = edition.prevHash?.toHexKey()?.let { byHash[it] } + val base = parent?.let { effectiveList(it, visiting) } ?: emptySet() + val author = edition.author.lowercase() + val claimed = ConcordJson.decodeBanlist(edition.content)?.mapTo(HashSet()) { it.lowercase() } + + // A malformed body changes nothing rather than clearing the list. + val result = + if (claimed == null) { + base + } else { + val out = HashSet(base) + for (added in claimed - base) if (canBanTarget(author, added)) out.add(added) + for (removed in base - claimed) if (canBanTarget(author, removed)) out.remove(removed) + out + } + + visiting.remove(edition.hashHex) + effective[edition.hashHex] = result + return result + } + val banned = HashSet() // Candidate-then-gate, like roles and grants: an unauthorized banlist edition in the // middle of the chain must not orphan the authorized ones chained above it (which, on // a banlist, would silently resurrect every ban a later unban had cleared). val banHead = EditionFold.foldEntityGated(allBanlist, gate = ::banGate) if (banHead != null) { - ConcordJson.decodeBanlist(banHead.content)?.forEach { banned.add(it.lowercase()) } + banned.addAll(effectiveList(banHead, HashSet())) // Ancestry is a STRUCTURAL fact, so it is walked over the full pool: an unauthorized // edition on the head's back-chain still supersedes what is beneath it, and walking // only the authorized subset would stop there and mis-read those genuine ancestors as @@ -270,7 +331,7 @@ class AuthorityResolver private constructor( val ancestry = banlistAncestry(banHead, allBanlist) for (edition in authorizedBanlist) { if (edition.hashHex !in ancestry) { - ConcordJson.decodeBanlist(edition.content)?.forEach { banned.add(it.lowercase()) } + banned.addAll(effectiveList(edition, HashSet())) } } } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolverTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolverTest.kt index 1890050f03..ec6ecae8c6 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolverTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/AuthorityResolverTest.kt @@ -23,18 +23,12 @@ package com.vitorpamplona.quartz.concord.cord04Roles import com.vitorpamplona.quartz.concord.cord04Roles.ConcordPermissions.Companion.BAN import com.vitorpamplona.quartz.concord.cord04Roles.ConcordPermissions.Companion.KICK import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray -import kotlin.test.Ignore import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue -private const val KNOWN_GAP = - "CORD-04 does not rank-gate BANLIST contents and Armada's banlistGate is rank-blind too; " + - "enforcing it here would diverge from every other client. Amethyst refuses to AUTHOR such " + - "a ban instead. Un-ignore when the spec closes this." - class AuthorityResolverTest { private val owner = "0f".repeat(32) private val alice = "a1".repeat(32) @@ -514,17 +508,17 @@ class AuthorityResolverTest { // ---- Rank gating on the banlist (CORD-04 "equal cannot act on equal") ---- // - // CORD-04 rank-gates ROLE and GRANT editions, but a BANLIST edition is a single whole-list - // entity, so no client rank-checks the *contents* of the list — the gate is the author's BAN - // bit alone. Armada does exactly the same: its `banlistGate` calls the rank-blind - // `isAuthorized(.., Permissions.BAN)`, even though its role path uses the rank-aware - // `canActOnPosition`. Enforcing rank on the fold unilaterally would make us ignore bans every - // other client honors, so the three @Ignore-d tests below record the gap instead of asserting - // a fix. Amethyst restricts what it will *author* (Account.concordBanTarget and the Members - // roster both route through canActOn); closing it for real needs a spec change. + // CORD-04 §3 binds "every action" to hold the required bit AND strictly outrank its target, and + // picks banning as its example ("an admin cannot ban a peer admin"); §5 step 3 restates it. Only + // §4, which defines the Banlist, states the bit half alone — which is why both this client and + // Armada shipped a rank-blind gate and let the most junior BAN holder ban the admins above them, + // and the owner. Because the Banlist is one whole-list document, the per-target rule is enforced + // as a delta rule: only additions and removals the signer outranks take effect, and the entries + // it may not act on are ignored rather than rejecting the edition wholesale. + // See docs/concord-banlist-rank-conformance.md — Armada has not shipped this yet, so banlists + // may differ between clients until it does. - // A moderator that holds BAN but sits BELOW an admin. The banlist is a single whole-list - // entity, so nothing about the *content* of the list is rank-checked — only the author's bit. + // A moderator that holds BAN but sits BELOW an admin. private val modWithBanJson = """{"name":"Mod","position":5,"permissions":"24"}""" // KICK|BAN private fun rankedBanScenario(vararg extra: ControlEdition) = @@ -539,11 +533,10 @@ class AuthorityResolverTest { ) @Test - @Ignore(KNOWN_GAP) fun aBanHolderCannotBanAMemberItDoesNotOutrank() { - // FAILS TODAY, deliberately unfixed. A rank-5 moderator bans the rank-1 admin above them and - // the fold accepts it — privilege escalation, not a no-op: the admin then loses every - // permission, because hasPermission() is `!isBanned && ..`. + // A rank-5 moderator bans the rank-1 admin above them. Before the delta rule the fold + // accepted this — privilege escalation, not a no-op: the admin then lost every permission, + // because hasPermission() is `!isBanned && ..`. val r = rankedBanScenario(banlistBy(bob, "mod-bans-admin", alice)) assertFalse(r.canActOn(bob, alice, BAN), "the rule itself: a moderator cannot act on an admin") @@ -551,7 +544,6 @@ class AuthorityResolverTest { } @Test - @Ignore(KNOWN_GAP) fun aBanHolderCannotBanTheOwner() { // The owner is unremovable (canActOn refuses them as a target), but the banlist is just a // list of keys. Banning the owner does not cost them fold authority (banGate/authorizedHeads @@ -581,7 +573,6 @@ class AuthorityResolverTest { } @Test - @Ignore(KNOWN_GAP) fun anUnrankedBanIsDroppedWithoutOrphaningTheRestOfTheList() { // The moderator bans the admin AND a plain member in one edition. The edition is the head of // the chain, so rejecting it wholesale would also lose the legitimate ban of carol. Only the