From 0ae6bc6698431c1b4bf1f40a333a1e104be1981d Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Mon, 20 Jul 2026 11:54:28 -0400 Subject: [PATCH] fix(concord): rank-gate the Ban and Remove affordances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ban/Remove were offered to any BAN holder against any non-owner, ignoring rank — unlike the role picker, which routes through `canActOn`. Both the Members roster and the message-level path (`Account.concordBanTarget`, the chokepoint for the quick-action menu, the note dropdown, the note action sections and the chat action sheet) now require `canActOn(me, target, BAN)`. This is NOT the no-op it first looked like. The premise that the fold would drop such a ban is wrong, and a test proves it: BANLIST is a single whole-list entity, so `authorizedHeads`/`banGate` gate on the author's BAN bit alone and never rank-check the list's *contents*. A rank-5 moderator's ban of a rank-1 admin is therefore ACCEPTED by every client, and the admin then loses every permission, since `hasPermission` is `!isBanned && ..`. It is privilege escalation, not a silent no-op. The fold is deliberately left alone. Armada has the identical gap — its `banlistGate` calls the rank-blind `isAuthorized(.., Permissions.BAN)` while its role path uses the rank-aware `canActOnPosition` — so rank-gating our fold would make us ignore bans every other client honors, splitting the banlist across clients. Closing it needs a spec change, like CORD-05. Refusing to AUTHOR such a ban restricts only what we write, never what we accept, so it cannot diverge consensus. Three `@Ignore`-d tests in AuthorityResolverTest state the fold-level invariant and currently fail by design; two companions assert the gate does not over-correct (a moderator still bans a plain member; the owner still bans anyone). Un-ignore the first three when the spec closes the gap. The owner short-circuits the check rather than going through `canActOn`, which begins at `hasPermission` and is false while banned — since a rogue BAN holder *can* currently banlist the owner, routing them through it would let them be locked out of moderating their own community. Device-verified on Amethyst QA Concord as Dr. Edo (QA Lead, rank 2): Bob (Admin, rank 1) now offers only the disabled "Roles… / You don't outrank this member" where Ban and Remove used to be enabled, while the Helper (rank 5) still offers Roles…, Ban and Remove. Co-Authored-By: Claude Opus 4.8 --- .../plans/2026-07-20-v1.13.0-release-qa.md | 17 ++-- .../vitorpamplona/amethyst/model/Account.kt | 23 +++-- .../concord/ConcordMembersScreen.kt | 17 +++- .../cord04Roles/AuthorityResolverTest.kt | 86 +++++++++++++++++++ 4 files changed, 129 insertions(+), 14 deletions(-) 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 30e4dde0f4..7c33bb23b2 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 @@ -89,11 +89,18 @@ pinned messages were never exercised. - **CORD-05: `community_id` does not commit to `community_root`**, so a crafted invite can carry a real community's identity with an attacker's root. **Armada has the identical gap** — this needs a spec conversation, not a unilateral fix. -- Concord Members roster: `canBan`/`canRemove` check only `viewerCanBan && !isOwnerTarget && - !isSelf` — never rank — while the ban fold enforces `canActOn`. So Ban and Remove are offered on - members who outrank the viewer and are then silently dropped on fold: exactly the no-op-control - trap the new "Roles…" item avoids. Fix is to route both through `canActOn`. Also affects the - message-level ban path, which is why it wasn't folded into the role-grant change. +- **CORD-04: the BANLIST is not rank-gated, so any BAN holder can ban anyone — including the owner.** + Role/grant editions are rank-gated (`canActOn`), but a banlist edition is a single *whole-list* + entity, so no client rank-checks its contents; the gate is the author's BAN bit alone. A rank-5 + moderator's ban of a rank-1 admin is therefore **accepted** by the fold, and the admin then loses + every permission (`hasPermission` is `!isBanned && …`). **Armada has the identical gap** — its + `banlistGate` calls the rank-blind `isAuthorized(.., Permissions.BAN)` while its role path uses + the rank-aware `canActOnPosition` — so enforcing rank on the fold unilaterally would make us + ignore bans every other client honors. Needs a spec conversation, like CORD-05. + Amethyst now refuses to *author* such a ban (`Account.concordBanTarget` and the Members roster + both route through `canActOn`), which restricts only what we write, never what we accept. Three + `@Ignore`-d tests in `AuthorityResolverTest` record the fold-level invariant; un-ignore them when + the spec closes 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/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index 76f667ebdc..9ffa1c6f1c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -2454,10 +2454,19 @@ class Account( /** * If [note] is a Concord channel message whose author this account is allowed to - * ban — the actor is the owner or holds the BAN permission, and the target is - * neither the owner nor the actor — returns `(communityId, memberHex)`. Null - * otherwise, so the UI shows the Ban action only when it would actually take - * effect on fold. + * ban — the actor outranks the target and holds the BAN permission, and the target + * is neither the owner nor the actor — returns `(communityId, memberHex)`. Null + * otherwise, so the UI offers Ban only where we are willing to act. + * + * The rank half is ours alone. CORD-04 rank-gates role grants (`canActOn`) but the + * BANLIST is a single whole-list entity, so neither this client's fold nor Armada's + * rank-checks the *contents* of a banlist edition — both gate only on the author's + * BAN bit (Armada: `banlistGate` → `isAuthorized(.., Permissions.BAN)`, while its + * role path uses the rank-aware `canActOnPosition`). A moderator's ban of an admin + * above them is therefore *accepted* by every client today. Since we cannot refuse + * such a ban without diverging from Armada, we at least refuse to author one — this + * restricts what we write, never what we accept, so it cannot split consensus. + * Enforcing it on the fold needs a spec change; see the QA plan's open findings. */ fun concordBanTarget(note: Note): Pair? { val channel = note.inGatherers?.firstNotNullOfOrNull { it as? ConcordChannel } ?: return null @@ -2471,7 +2480,11 @@ class Account( ?.value ?.authority ?: return null if (authority.isOwner(author)) return null - val canBan = authority.isOwner(signer.pubKey) || authority.effectivePermissions(signer.pubKey).has(ConcordPermissions.BAN) + // The owner short-circuits rather than going through canActOn: canActOn starts at + // hasPermission, which is false while banned, and a rogue BAN holder *can* currently put + // the owner on the banlist (see the KDoc) — routing the owner through it would let them be + // locked out of moderating their own community. + val canBan = authority.isOwner(signer.pubKey) || authority.canActOn(signer.pubKey, author, ConcordPermissions.BAN) return if (canBan) communityId to author else null } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/publicChannels/concord/ConcordMembersScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/publicChannels/concord/ConcordMembersScreen.kt index b443ffd3fa..d2a7b390be 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/publicChannels/concord/ConcordMembersScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/chats/publicChannels/concord/ConcordMembersScreen.kt @@ -188,6 +188,13 @@ fun ConcordMembersScreen( isSelf = entry.pubkey.equals(myPubKey, ignoreCase = true), viewerIsOwner = iAmOwner, viewerCanBan = iCanBan, + // Ban/Remove are rank-gated the same way Roles… is. The owner short-circuits + // because canActOn begins at hasPermission, which is false while banned, and + // a rogue BAN holder can currently banlist the owner — see the note on + // Account.concordBanTarget. + canBanTarget = + iAmOwner || + state?.authority?.canActOn(myPubKey, entry.pubkey, ConcordPermissions.BAN) == true, viewerCanManageRoles = iCanManageRoles, // canActOn folds the whole rank rule for us: we hold MANAGE_ROLES, we're not // banned, the target isn't the owner (unremovable), and we strictly outrank @@ -211,6 +218,7 @@ private fun ConcordMemberRow( isSelf: Boolean, viewerIsOwner: Boolean, viewerCanBan: Boolean, + canBanTarget: Boolean, viewerCanManageRoles: Boolean, canManageRolesOnTarget: Boolean, assignableRoles: List, @@ -222,12 +230,13 @@ private fun ConcordMemberRow( val isBanned = entry.membership == ConcordMembership.BANNED val isAdmin = entry.membership == ConcordMembership.ADMIN - // Owner can promote/demote anyone but the owner; ban is available to owner + BAN holders, - // never against the owner or yourself. A banned user only offers "unban". + // Owner can promote/demote anyone but the owner; ban is available to owner + BAN holders that + // strictly outrank the target, never against the owner or yourself. A banned user only offers + // "unban" — and unban is rank-gated too, so whoever cannot ban you cannot lift your ban either. val canToggleAdmin = viewerIsOwner && !isOwnerTarget && !isBanned && !isSelf - val canBan = viewerCanBan && !isOwnerTarget && !isSelf + val canBan = viewerCanBan && canBanTarget && !isOwnerTarget && !isSelf // Hard removal (CORD-06 Refounding) rotates the community key; same authority as ban. - val canRemove = viewerCanBan && !isOwnerTarget && !isSelf + val canRemove = viewerCanBan && canBanTarget && !isOwnerTarget && !isSelf // Shown to any MANAGE_ROLES holder, but disabled with a reason when this particular // member (or every defined role) is out of our reach — a grant we don't outrank // publishes fine and is then dropped by every client's fold, so a silently no-op 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 d5718451e2..1890050f03 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,12 +23,18 @@ 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) @@ -505,4 +511,84 @@ class AuthorityResolverTest { assertEquals(setOf(adminRole), above.rolesOf(alice)) assertEquals(1L, above.rank(alice)) } + + // ---- 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. + + // 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. + private val modWithBanJson = """{"name":"Mod","position":5,"permissions":"24"}""" // KICK|BAN + + private fun rankedBanScenario(vararg extra: ControlEdition) = + AuthorityResolver.resolve( + listOf( + role(adminRole, adminJson), // position 1 + role(modRole, modWithBanJson), // position 5, holds BAN + grant("31".repeat(32), alice, listOf(adminRole), granter = owner), + grant("32".repeat(32), bob, listOf(modRole), granter = owner), + ) + extra, + owner, + ) + + @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 && ..`. + 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") + assertFalse(r.isBanned(alice), "so the fold must not honor the moderator's ban of the admin") + } + + @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 + // short-circuit on isOwner), yet hasPermission() is `!isBanned && ...`, so canActOn(owner, ..) + // goes false and the owner loses every rank-gated action. + val r = rankedBanScenario(banlistBy(bob, "mod-bans-owner", owner)) + + assertFalse(r.isBanned(owner), "the owner must never be bannable") + assertTrue(r.canActOn(owner, bob, BAN), "and must keep authority over everyone") + } + + @Test + fun aBanHolderStillBansThoseItOutranks() { + // The gate must not over-correct: carol holds no role at all (rank = lowest), so the + // moderator outranks her and the ban must land. + val r = rankedBanScenario(banlistBy(bob, "mod-bans-plain-member", carol)) + + assertTrue(r.canActOn(bob, carol, BAN)) + assertTrue(r.isBanned(carol), "a moderator must still ban a plain member") + } + + @Test + fun theOwnerBansAnyone() { + val r = rankedBanScenario(banlistBy(owner, "owner-bans-admin", alice)) + + assertTrue(r.isBanned(alice), "the owner outranks everyone") + } + + @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 + // entries the author does not outrank may be dropped. + val r = rankedBanScenario(banlistBy(bob, "mod-bans-both", alice, carol)) + + assertFalse(r.isBanned(alice), "the part it does not outrank is dropped") + assertTrue(r.isBanned(carol), "the part it does outrank still lands") + } }