fix(concord)!: enforce CORD-04 rank gating on the Banlist fold

Closes the privilege escalation: any BAN holder could ban the authorities above them — including the owner — because the Banlist gate checked only the BAN bit. Once banned, a member loses all authority (`hasPermission` is `!isBanned && ..`) and honest clients drop their events, so a single edition from the most junior moderator permanently silenced every admin above them. CORD-04 §3 requires the rank half: "One hard rule binds every action: the actor must hold the required bit and strictly outrank its target — equal cannot act on equal (an admin cannot ban a peer admin)", restated as §5 step 3. Only §4, which defines the Banlist, states the bit half alone — which is why both this client and Armada shipped the same rank-blind gate. §3 is stated per TARGET while 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, judged against the roster settled behind it; the owner is never a valid target (position 0 is "supreme and unremovable"); and entries the signer may not act on are IGNORED rather than rejecting the edition, so one bad entry cannot discard the bulk-ban §4 recommends as the collision remedy, and a rogue cannot grief the list by forcing rejections. ConcordModeration.currentBanned now reads the honored banlist through the resolver instead of decoding the raw head. Besides picking up the fork healing it was missing, this closes a laundering path: our own next ban/unban would otherwise re-publish an entry our fold refuses, under our signature. BREAKING (consensus): Armada has not shipped this rule, so banlists can differ between clients until it does — we now ignore a ban Armada honors whenever the signer did not outrank the target. Shipping the spec-conformant behaviour was judged better than continuing to honor an escalation. Write-up to send upstream is docs/concord-banlist-rank-conformance.md. The three tests added in 0ae6bc6698 as @Ignore-d documentation now pass and are un-ignored; two companions (a moderator still bans a plain member, the owner still bans anyone) passed throughout and pin what the fix had to preserve. Full :quartz:jvmTest and :commons:jvmTest suites green. Still open and documented, not addressed here: a banned BAN holder can lift their own ban (a fixpoint-ordering question that needs a spec ruling), and a forked ban survives an unban that does not chain onto it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-10-05 19:28:25 +00:00 · 2026-07-20 12:53:11 -04:00
parent ea6762b137
commit 8913af7a79
5 changed files with 120 additions and 55 deletions
@@ -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: <https://github.com/concord-protocol/concord> (`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.
@@ -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<ControlEdition>,
        communityId: ByteArray,
        owner: HexKey,
-    ): Set<HexKey> {
-        val entityId = ConcordKeyDerivation.banlistCoordinate(communityId)
-        val head = headOf(current, entityId, owner)
-        return head?.let { ConcordJson.decodeBanlist(it.content) }?.mapTo(HashSet()) { it.lowercase() } ?: emptySet()
-    }
+    ): Set<HexKey> = AuthorityResolver.resolve(current, owner).bannedMembers()

    private suspend fun setBanlist(
        actor: NostrSigner,
@@ -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`
@@ -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<String, Set<String>>()
+
+            // 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<String>,
+            ): Set<String> {
+                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<String>()
            // 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()))
                    }
                }
            }
@@ -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