From 4e99aafb59f5f081f5fe6f13924abcc15ee38f8e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 15:47:41 +0000 Subject: [PATCH] fix(quartz): honor the banlist against the Control Plane itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B2 in docs/concord-soft-ban-audit.md, plus B4's bound and the audit's status pass. hasPermission was ban-aware; the resolver's own ROLE/GRANT/BANLIST gates were not, and could not be as written — the roles/grants fixpoint settled before `banned` was computed at all. So half the Control Plane honored a ban and half was blind to it, and a banned staffer still holding control_root kept the whole roster: banning everyone beneath them, revoking the surviving moderators, retiring the roles under them, and minting a fresh un-banned npub that passed every ban-aware gate and finished the job. resolve() is now a bounded two-pass where authority only ever shrinks. Pass A resolves as before and yields a candidate banlist; pass B re-resolves with every author on it treated as holding no authority. Two passes always, so it terminates by construction, and mutual bans cannot oscillate because the rank rule makes them unreachable — only someone who strictly outranks you may ban you, and you cannot outrank them back. A chain-local rule would not have worked: forking the banlist at genesis means no parent ever mentions the ban and §4's re-heal union carries it in regardless, so the rule is a whole-pass mask rather than a per-edition check. This cascades, deliberately: every edition a banned member ever authored is dropped, grants included, so banning an admin also demotes everyone that admin promoted. That is the literal reading of CORD-04 §4 and it is what kills the sockpuppet, but a legitimate promotion by a later-banned admin vanishes with it and has to be re-issued. Both the cascade and its blast radius are pinned, and the trade-off is written up in the Armada report as the answer to its own open row 3 — which also widens the divergence recorded there: we now drop editions they honor wherever a privileged member was banned. B4: the Refounding recipient set is capped. allMembers() is the Guestbook ∪ observedAuthors ∪ the roster, and the first two are unbounded and attacker-writable, so each throwaway npub someone posts from became one more mandatory blob in the next Refounding — the attack inflating the cost of its own remedy. The owner-rooted roster is kept first and anything dropped is logged, never silently truncated, because a dropped member is stranded. The nine escalation reproductions now assert the fixed behaviour. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DrJhpFhhLjuDJQNkGvYMGj --- .../amethyst/model/AccountConcordActions.kt | 48 ++++++- docs/concord-banlist-rank-conformance.md | 35 ++++- docs/concord-soft-ban-audit.md | 134 ++++++++++++------ .../concord/cord04Roles/AuthorityResolver.kt | 52 ++++++- .../cord04Roles/BannedStaffEscalationTest.kt | 111 ++++++++++----- 5 files changed, 299 insertions(+), 81 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 c403e08986..5b5c8fe0db 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/AccountConcordActions.kt @@ -33,6 +33,7 @@ import com.vitorpamplona.quartz.concord.cord02Community.ConcordCommunityListEven import com.vitorpamplona.quartz.concord.cord02Community.HeldRoot import com.vitorpamplona.quartz.concord.cord02Community.ImagePointer import com.vitorpamplona.quartz.concord.cord03Channels.ChannelChat +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.ConcordPermissions @@ -76,6 +77,15 @@ private const val CONCORD_ADMIN_ROLE = "Admin" */ private const val RECOVERY_CHECK_INTERVAL_MS = 15 * 60 * 1000L +/** + * How many recipients one Refounding will re-key. See `AccountConcordActions.boundRecipients`. + * + * 120 blobs ride in each kind-3303 chunk, so this is ~42 published events and ~5k NIP-44 + * encryptions at the ceiling — heavy but survivable on a phone, and far above any real community. + * Raising it raises the cost of the attack it exists to bound, not the safety. + */ +private const val MAX_REFOUNDING_RECIPIENTS = 5_000 + /** * Concord (encrypted communities) orchestration for an [Account]: join/create/ * invite flows, channel messages/reactions/edits/typing, roles and moderation, @@ -810,7 +820,7 @@ class AccountConcordActions( .apply { removeAll(removedLower) removeAll(authority.bannedMembers()) - }.toList() + }.let { candidates -> boundRecipients(candidates, authority) } // 3. Build the refounding: new root, compacted Control Plane, per-recipient rekey blobs. val entry = session.entry @@ -853,6 +863,42 @@ class AccountConcordActions( return true } + /** + * Caps the Refounding recipient set, keeping the members whose standing we can actually vouch + * for when there are too many. + * + * `allMembers()` is the Guestbook ∪ `observedAuthors` ∪ the roster, and the first two are + * unbounded and attacker-writable: a Guestbook Join is self-signed by any key at all, and every + * author we decrypt is folded in by design (CORD-02 §5, "observably present"). So each throwaway + * npub someone posts from, or simply announces, becomes one more mandatory blob in the next + * Refounding — meaning the attack inflates the cost of its own remedy, and the remedy is the only + * hard removal Concord has. See B4 in `docs/concord-soft-ban-audit.md`. + * + * The roster and the owner are kept unconditionally: they are owner-rooted, so they cannot be + * padded from outside. The remainder fills the budget, and anything dropped is **logged rather + * than silently truncated** — a dropped member is stranded on the dead epoch and their only way + * back is a recovery path that needs to know it happened. + */ + private fun boundRecipients( + candidates: Set, + authority: AuthorityResolver, + ): List { + if (candidates.size <= MAX_REFOUNDING_RECIPIENTS) return candidates.toList() + + val vouched = authority.roleHolders() + authority.staffMembers() + val kept = LinkedHashSet(MAX_REFOUNDING_RECIPIENTS) + candidates.filterTo(kept) { it in vouched } + for (candidate in candidates) { + if (kept.size >= MAX_REFOUNDING_RECIPIENTS) break + kept.add(candidate) + } + Log.w("Concord") { + "Refounding recipient set capped at $MAX_REFOUNDING_RECIPIENTS of ${candidates.size}: " + + "${candidates.size - kept.size} member(s) will be stranded on the prior epoch" + } + return kept.toList() + } + // Rotations we've already adopted ("communityId:epoch"), so a base-rekey wrap still buffered // in the pre-rebuild window (the session rebuild off `liveCommunities` is async) is not // adopted — and re-published — twice on successive revision ticks. diff --git a/docs/concord-banlist-rank-conformance.md b/docs/concord-banlist-rank-conformance.md index afbe98fdbd..04e92c9de7 100644 --- a/docs/concord-banlist-rank-conformance.md +++ b/docs/concord-banlist-rank-conformance.md @@ -1,7 +1,8 @@ # Concord: the Banlist is not rank-gated in any implementation (CORD-04 conformance) **Status:** conformance bug. Reproduced in Amethyst and **fixed there** (see §6); present by -inspection in Armada. +inspection in Armada. Finding #3, left open in §4 as a fixpoint-ordering question, is now also +implemented in Amethyst — see the 2026-08-09 update before §Rollout status. **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. @@ -190,6 +191,38 @@ We'd also suggest **§4 restating the rank half inline**, the way §2 does for G does for Kicks. Both independent implementations read §4 in isolation and both got it wrong the same way; that is strong evidence the section is the problem, not the readers. +### Update, 2026-08-09: we have now implemented #3 + +Amethyst now answers the ordering question rather than leaving it open, because #3 turned out to be +the doorway to a full community takeover and not merely an inconsistency — a banned staffer who kept +`control_root` kept the entire roster, and could mint a fresh un-banned npub that passed every +ban-aware gate. The write-up is `docs/concord-soft-ban-audit.md` (B2). + +The rule we shipped: **authority only ever shrinks, over two passes.** Pass A resolves exactly as +before and yields a candidate banlist; pass B re-resolves with every author on that list treated as +holding no authority at all, for roles, grants and the Banlist alike. Two passes, always, so it +terminates by construction. It cannot oscillate on mutual bans either, because the rank rule makes +them unreachable: only a member who strictly outranks you may ban you, and you cannot outrank them +back. + +Note what it costs, because it is not obvious and you would hit it too: this **cascades**. Every +edition a banned member ever authored is dropped, grants included, so banning an admin also demotes +everyone that admin promoted. We think that is the literal reading of §4 and it is what kills the +sockpuppet — but a legitimate promotion by a later-banned admin vanishes with it, and the owner has +to re-issue it. If you read §4 as scoping only to editions authored *after* the ban, say so; that is +implementable too, but it needs the spec to define an ordering between an edition and a Banlist +entry, which today it does not. + +A chain-local rule is not enough, and this is the trap worth flagging: "the author must not be banned +by the state their edition chains from" is bypassed by forking the Banlist at genesis, where no +parent ever mentions the ban and §4's re-heal union carries it in anyway. The rule has to bind the +union, which is why ours is a whole-pass mask rather than a per-edition check. + +This widens the divergence in §6: we now drop editions you honor in any community where a privileged +member was banned, not only where a signer failed to outrank their target. + +--- + Separately, please rule on **#3**: whether a banned npub's Banlist edition is honored. Our reading of §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. diff --git a/docs/concord-soft-ban-audit.md b/docs/concord-soft-ban-audit.md index 58429394ca..1da43ed4d0 100644 --- a/docs/concord-soft-ban-audit.md +++ b/docs/concord-soft-ban-audit.md @@ -1,7 +1,9 @@ # Concord: soft-ban and Control Plane audit **Scope:** what a removed member — or a moderator who turns — can still do to a Concord community. -**Date:** 2026-08-09. **Status:** findings only, nothing fixed yet. +**Date:** 2026-08-09. **Status:** A1–A4, B1, B2 and B4 are **fixed** on this branch; the rest are +accepted, deferred to the spec, or belong to other people's relays. Each section carries its own +status line. **Companion:** `docs/concord-banlist-rank-conformance.md` (the rank half of CORD-04 §4, already reported to Armada and fixed here). @@ -47,32 +49,32 @@ Two structural causes account for most of both halves: ### Part A — reachable from stock Amethyst (our bugs) -| # | Finding | Severity | Was | -|---|---------|----------|-----| -| [A1](#a1) | Any member — banned included — mints a working invite in one tap | **Critical** | new | -| [A2](#a2) | Stranded recovery runs on a timer and never checks the banlist | **Critical** | V10 | -| [A3](#a3) | The action layer has no permission checks; the UI's are ban-blind | High | new | -| [A4](#a4) | A banned member keeps broadcasting "typing", and we keep showing it | Low | V12 | -| [A5](#a5) | A banned member's own client keeps reading and rendering everything | Medium | V8 | +| # | Finding | Severity | Status | +|---|---------|----------|--------| +| [A1](#a1) | Any member — banned included — mints a working invite in one tap | **Critical** | **Fixed** | +| [A2](#a2) | Stranded recovery runs on a timer and never checks the banlist | **Critical** | **Fixed** (security half; liveness half open) | +| [A3](#a3) | The action layer has no permission checks; the UI's are ban-blind | High | **Fixed** | +| [A4](#a4) | A banned member keeps broadcasting "typing", and we keep showing it | Low | **Fixed** | +| [A5](#a5) | A banned member's own client keeps reading and rendering everything | Medium | Inherent — product decision | ### Part B — requires a malicious client -| # | Finding | Severity | Needs a ban? | Recoverable? | Was | -|---|---------|----------|--------------|--------------|-----| -| [B1](#b1) | One edition at `version = Long.MAX_VALUE` pins an entity forever | **Critical** | No — any bit-holder | **No** | V1 | -| [B2](#b2) | A banned staffer keeps Role/Grant/Banlist authority | **Critical** | Yes | Yes (Refounding) | V2 | -| [B3](#b3) | A rogue rotator compacts the banlist away | High | Via B2 | Partly | V3 | -| [B4](#b4) | The Refounding recipient set is attacker-inflatable | High | No | Yes | V4 | -| [B5](#b5) | The ban is per-pubkey; the channel key is not revoked | High | Yes | Yes (Refounding) | V5 | -| [B6](#b6) | Channel history is deletable on a naive third-party relay | High | Yes | **No** (history) | V6 | -| [B7](#b7) | The base-rekey plane is writable by every member | Low | Yes | Yes | V9 | +| # | Finding | Severity | Needs a ban? | Status | +|---|---------|----------|--------------|--------| +| [B1](#b1) | One edition at `version = Long.MAX_VALUE` pins an entity forever | **Critical** | No — any bit-holder | **Fixed** | +| [B2](#b2) | A banned staffer keeps Role/Grant/Banlist authority | **Critical** | Yes | **Fixed** (consensus-affecting) | +| [B3](#b3) | A rogue rotator compacts the banlist away | High | Via B2 | **Mitigated** by B2 | +| [B4](#b4) | The Refounding recipient set is attacker-inflatable | High | No | **Fixed** (bounded) | +| [B5](#b5) | The ban is per-pubkey; the channel key is not revoked | High | Yes | Inherent — Refounding is the answer | +| [B6](#b6) | Channel history is deletable on a naive third-party relay | High | Yes | Correct here; external relays at risk | +| [B7](#b7) | The base-rekey plane is writable by every member | Low | Yes | Accepted | ### Part C — interop and not-yet-shipped -| # | Finding | Severity | Was | -|---|---------|----------|-----| -| [C1](#c1) | Banlist rank rule diverges from Armada | Medium | V7 | -| [C2](#c2) | CORD-07 voice rooms are key-gated, not roster-gated | Design | V11 | +| # | Finding | Severity | Status | +|---|---------|----------|--------| +| [C1](#c1) | Banlist rank rule diverges from Armada | Medium | Reported; B2 widens the divergence | +| [C2](#c2) | CORD-07 voice rooms are key-gated, not roster-gated | Design | Note for whoever ships voice | --- @@ -82,6 +84,9 @@ No custom tooling. A banned user with the shipping app, or our own background sw ## A1 — Any member, banned included, mints a working invite in one tap +**Status: fixed.** `mintConcordInvite` and its button now require `CREATE_INVITE` (or ownership). + + **Critical. The single most likely thing an irritated banned user actually does.** *Read:* `AccountConcordActions.mintConcordInvite`, `ConcordChannelListScreen` (the `PersonAdd` `IconButton`). @@ -106,6 +111,12 @@ button on the same. This is contained, uncontroversial, and closes the realistic ## A2 — Stranded recovery runs on a timer and never checks the banlist +**Status: security half fixed; liveness half open.** `isStranded` / `mergeForward` now take +`bannedAtCurrentEpoch` as a *required* argument, so a removed member is no longer walked back in. +Whether anything should re-mint at a stable coordinate — without which legitimate recovery never +fires for anyone — still needs a spec answer and is untouched. + + **Critical, and it forks.** *Read:* `ConcordStrandedRecovery`, `AccountConcordActions.recoverStrandedConcordCommunities`, `AccountConcordActions.mintConcordInvite`. @@ -137,6 +148,10 @@ evicted owners another route. ## A3 — The action layer has no permission checks; the UI's are ban-blind +**Status: fixed.** Authority now lives in `AccountConcordActions.isAuthorizedFor`, which every +moderation verb funnels through, and every authorization test uses the ban-aware `hasPermission`. + + **High (defense in depth).** *Read:* `AccountConcordActions` (`banConcordMember`, `unbanConcordMember`, `editConcordMetadata`, `deleteConcordChannel`, `refoundConcordCommunity`), `ConcordMembersScreen`, `ConcordChannelListScreen`. @@ -169,6 +184,9 @@ their roles say", independent of standing. ## A4 — A banned member keeps broadcasting "typing", and we keep showing it +**Status: fixed on both ends.** + + **Low, both halves ours.** *Read:* `AccountConcordActions.sendConcordTyping`, `ConcordCommunitySession.ingestTyping`. @@ -180,6 +198,11 @@ and it directly contradicts what a ban promises the user. ## A5 — A banned member's own client keeps reading and rendering everything +**Status: inherent; no code change.** The cryptography cannot be fixed without a Refounding, so what +is left is a product decision about how "Ban" and "Remove from community" are presented. Left for a +design pass rather than guessed at here. + + **Medium, partly inherent.** *Read:* CORD-02/05, `ConcordCommunitySession`. Until a Refounding, a ban stops honest clients from *showing* the banned member's posts; it does not @@ -201,6 +224,12 @@ what they publish. ## B1 — One edition at `Long.MAX_VALUE` pins an entity forever +**Status: fixed.** The compaction arm tries the floor-anchored chain first and bounds the bootstrap +jump at `EditionFold.MAX_COMPACTION_VERSION_JUMP`; `compactControlPlane` picks the chain head rather +than raw max version. The three reproductions now assert the fixed behaviour, and +`aGenuineCompactionJumpIsStillFollowed` pins the CORD-06 §3 tolerance the bound must not break. + + **Critical. Does not require a banned user, a sockpuppet, or the owner's absence. Unrecoverable.** *Verified:* `quartz/…/cord04Roles/ControlPlaneVersionExhaustionTest.kt` (3 tests). @@ -246,6 +275,14 @@ The first is the smallest change and closes the unrecoverability; the third shou ## B2 — A banned staffer keeps Role, Grant and Banlist authority +**Status: fixed — and consensus-affecting.** `AuthorityResolver.resolve` is now a bounded two-pass +where authority only shrinks. Note the deliberate cascade it brings: every edition a banned member +ever authored is dropped, so banning an admin also demotes everyone that admin promoted. That is the +literal reading of CORD-04 §4 and it is what kills the sockpuppet, but a legitimate promotion by a +later-banned admin vanishes with it and has to be re-issued. Until Armada ships the same rule the two +clients can disagree about any community where a privileged member was banned. + + **Critical.** *Verified:* `quartz/…/cord04Roles/BannedStaffEscalationTest.kt` (13 tests). `hasPermission` is ban-aware; the resolver's internal gates are not, and structurally cannot be as @@ -277,6 +314,11 @@ Armada ships the same rule, we will drop editions they honor. ## B3 — A rogue rotator compacts the banlist away +**Status: mitigated by B2.** The rotator this needed was the sockpuppet, which can no longer be +minted. A *legitimately* privileged rotator can still omit the banlist, and `EntityFloor` remains the +only defense for clients that already folded it — unchanged, and still worth a spec fix. + + **High.** *Verified:* `aRogueRotatorCompactsTheBanAwayForEveryClientWithoutAFloor`. A CORD-06 §3 compaction re-wraps one edition per entity and the *rotator* picks it, so a rotator can @@ -292,6 +334,10 @@ protect people who were already there. ## B4 — The Refounding recipient set is attacker-inflatable +**Status: fixed (bounded).** The recipient set is capped, the owner-rooted roster is kept first, and +anything dropped is logged rather than silently truncated. + + **High.** *Read:* `ConcordCommunitySession.allMembers()` / `emitChannelRumors`; `AccountConcordActions.refoundConcordCommunity` step 2; `ConcordRefounding.buildBaseRekeyWraps`. @@ -311,6 +357,9 @@ the recipient set, prefer recent/attested members when over the cap, and surface ## B5 — The ban is a per-pubkey display rule and the channel key is not revoked +**Status: inherent.** No client-side fix exists; a Refounding is the answer, which is why B4 mattered. + + **High.** *Read:* `Account.consumeConcordRumorGated` (`isBanned(rumor.pubKey)`), `Account.isAcceptable`. Writing to a channel needs the channel key, which the ban does not take away; the seal author is @@ -324,6 +373,10 @@ correct design, which is why B4 matters so much. ## B6 — Channel history is deletable on a naive third-party relay +**Status: correct on our relay and pinned; external relays remain exposed.** Needs a CORD-01 spec note +and relay-selection guidance, not code. + + **High, external.** *Verified (that we are safe):* `geode/…/ConcordPlaneKeyDeletionTest.kt` (3 tests). @@ -346,6 +399,9 @@ Worth a note in the CORD-01 spec and a line in the relay-selection guidance. ## B7 — The base-rekey plane is writable by every member +**Status: accepted.** Bounded work per wrap, no correctness impact. + + **Low.** *Read:* `ConcordKeyDerivation.baseRekeyAddress`, `AccountConcordActions.drainConcordRekeys`. The base-rekey address derives from `community_root`, so any member — banned included — can mint @@ -406,25 +462,19 @@ Not looked at at all: - Unread counts and notification triggers, media/upload references from messages, the NIP-53 nests overlap, and the desktop client's Concord paths. -## Suggested order +## What is left -**Part A first.** It is the whole of the realistic threat — a banned user with the app already -installed — and none of it needs coordination with anyone. - -1. **A1** — one guard on `mintConcordInvite` plus one on its button. Smallest fix on the list and it - closes the attack a banned user will actually reach for. -2. **A3** — move authority into the action layer and replace `effectivePermissions` with - `hasPermission` everywhere it is used as an authorization test. This is also the cheapest partial - mitigation for B2: it shrinks what a banned staffer can do *without* writing their own client. -3. **A2** — needs the semantics decided before any code. Raise it with the spec. -4. **A4 / A5** — small, user-visible, and they make the product honest about what a ban is. - -**Then Part B**, hardest first because the ceiling is highest: - -5. **B4** — cheap, not consensus-affecting, and it protects the remedy every other fix depends on. -6. **B1** — worst blast radius, the only unrecoverable one, and the bar is a single ordinary - permission bit. -7. **B2 (+B3, +C1's open row)** — one two-pass change closes all three. Coordinate with Armada - first; this one splits consensus. -8. **B6** — spec note plus relay-selection guidance; our own behaviour is already correct and pinned. -9. **B5 / B7** — accept, or bound. +1. **A2's liveness half** — decide whether a community re-mints its invite bundle at a stable + coordinate. Today nothing does, so stranded recovery never fires for anyone, and an owner evicted + by a rogue admin has no route back. Needs a spec answer before code. +2. **C1 / B2 interop** — tell Armada about the two-pass rule, as with the rank rule before it. The + divergence is now wider: we drop editions they honor whenever a privileged member is banned. +3. **B6** — a CORD-01 note that a plane's wraps must stay owned by a key nobody holds, plus guidance + that a relay authorizing NIP-09/62 by `pubkey` hands every ex-member a wipe button. +4. **B3's residue** — a legitimately privileged rotator can still omit an entity during compaction. + `EntityFloor` catches it for clients that were present; fresh joiners have nothing. +5. **A5** — a design pass on how "Ban" and "Remove from community" are presented, since they promise + very different things. +6. **The unexamined surfaces below**, particularly private channels — there appears to be no + channel-scoped rekey receive path at all, which would mean the full-community Refounding is the + only removal Amethyst can perform. 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 039a63e7ca..ff85147ea6 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 @@ -135,9 +135,54 @@ data class AuthorityResolver private constructor( /** The owner's rank — supreme and unremovable. No Role may claim it. */ const val OWNER_RANK = 0L + /** + * The owner-rooted authority state of a community, with the banlist honored **against the + * Control Plane itself** (CORD-04 §4: a reader "drops every event from a banned npub — + * message, reaction, edit, or authority action"). + * + * This is a bounded two-pass, because the rule is circular as stated: you cannot know who is + * banned until you fold the Banlist, and you cannot decide who may write the Banlist without + * knowing who is banned. `docs/concord-banlist-rank-conformance.md` §4 row 3 flagged that to + * the spec authors and left it open. We resolve it by making authority only ever **shrink**: + * + * - **Pass A** resolves exactly as before, ban-blind, and yields a candidate banlist. + * - **Pass B** re-resolves with every author in that banlist treated as unauthorized, for + * roles, grants and the banlist alike. + * + * Two passes, always, so it terminates by construction — pass B never feeds back. It cannot + * oscillate on mutual bans either, because the rank rule makes them unreachable: only a + * member who strictly outranks you may ban you, and you cannot outrank them back. + * + * **This cascades, deliberately.** Every edition a banned member ever authored is dropped, + * including grants they made while in good standing — so banning an admin also demotes + * everyone that admin promoted. That is the literal reading of §4, and it is the point: the + * escalation in `docs/concord-soft-ban-audit.md` B2 was a banned staffer minting a fresh, + * un-banned npub and acting through it, and dropping the grant is what kills the puppet. The + * cost is that a legitimate promotion by a later-banned admin vanishes too, and the owner has + * to re-issue it. + * + * **Consensus-affecting.** Armada gates the Control Plane on role-derived permissions alone, + * so until it ships the same rule the two clients can disagree about any community where a + * privileged member was banned. + */ fun resolve( editions: Collection, ownerPubKey: String, + ): AuthorityResolver { + val passA = resolveOnce(editions, ownerPubKey, bannedAuthors = emptySet()) + if (passA.banned.isEmpty()) return passA + return resolveOnce(editions, ownerPubKey, bannedAuthors = passA.banned) + } + + /** + * One resolution pass. [bannedAuthors] are treated as holding no authority at all — their + * role, grant and banlist editions are dropped rather than merely being unable to act on + * others. Empty on pass A; pass A's banlist on pass B. See [resolve]. + */ + private fun resolveOnce( + editions: Collection, + ownerPubKey: String, + bannedAuthors: Set, ): AuthorityResolver { val ownerLower = ownerPubKey.lowercase() @@ -191,6 +236,7 @@ data class AuthorityResolver private constructor( ): Boolean { val author = e.author.lowercase() if (author == ownerLower) return true + if (author in bannedAuthors) return false if (!holdsManageRoles(author)) return false val authorRank = rankOf(author) ?: return false val r = ConcordJson.decodeOrNull(e.content) ?: return false @@ -223,6 +269,7 @@ data class AuthorityResolver private constructor( fun grantGate(e: ControlEdition): Boolean { val granter = e.author.lowercase() if (granter == ownerLower) return true + if (granter in bannedAuthors) return false if (!holdsManageRoles(granter)) return false val granterRank = rankOf(granter) ?: return false val g = ConcordJson.decodeOrNull(e.content) ?: return false @@ -269,7 +316,9 @@ 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 || effectivePermissionsOf(e.author.lowercase()).has(ConcordPermissions.BAN) + fun banGate(e: ControlEdition): Boolean = + e.author.lowercase() == ownerLower || + (e.author.lowercase() !in bannedAuthors && 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 @@ -292,6 +341,7 @@ data class AuthorityResolver private constructor( // owner is never a valid target — not even for themselves. if (target == ownerLower) return false if (author == ownerLower) return true + if (author in bannedAuthors) return false 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 diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/BannedStaffEscalationTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/BannedStaffEscalationTest.kt index aedadd885a..5b6c0fc64d 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/BannedStaffEscalationTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/concord/cord04Roles/BannedStaffEscalationTest.kt @@ -28,27 +28,31 @@ import kotlin.test.assertFalse import kotlin.test.assertTrue /** - * What a **soft-banned staffer** can still do to a community — the reproduction behind - * `docs/concord-banlist-rank-conformance.md` §4 row 3, which the report left open as "a genuine - * fixpoint-ordering question, not a plain oversight". + * **B2 in `docs/concord-soft-ban-audit.md` — regression guard.** What a soft-banned staffer used to + * be able to do to a community, and can no longer. Every test here failed before the two-pass rule + * in [AuthorityResolver.resolve] and passes after it. * - * The asymmetry these tests pin: [ConcordCommunityState.fold] gates METADATA / CHANNEL / INVITE - * through `authority.hasPermission`, which is `!isBanned && …`, but ROLE, GRANT and BANLIST are - * gated *inside* [AuthorityResolver.resolve] by `holdsManageRoles` / `bitsOf` / - * `effectivePermissionsOf` — none of which consult the banlist. Nor could they as written: the - * roles/grants fixpoint runs before `banned` is computed at all. So half the Control Plane honors - * a ban and half is structurally blind to it, and a banned member who still holds `control_root` - * keeps full authority over the roster. + * The asymmetry they were written to pin: [ConcordCommunityState.fold] gates METADATA / CHANNEL / + * INVITE through `authority.hasPermission`, which is `!isBanned && …`, but ROLE, GRANT and BANLIST + * were gated *inside* [AuthorityResolver.resolve] by `holdsManageRoles` / `bitsOf` / + * `effectivePermissionsOf` — none of which consulted the banlist, nor could they as written, since + * the roles/grants fixpoint ran before `banned` was computed at all. Half the Control Plane honored + * a ban and half was structurally blind to it, so a banned member still holding `control_root` kept + * full authority over the roster: they banned everyone beneath them, revoked the surviving + * moderators, retired the roles under them, and minted a fresh un-banned npub that passed every + * ban-aware gate and finished the job. * - * **These tests assert the CURRENT, VULNERABLE behaviour**, so the escalation cannot regress - * silently or be "fixed" by accident without someone noticing. Every `ESCALATION:` assertion here - * must be INVERTED — not deleted — when the ordering rule lands. [selfUnbanIsStillRefused] and - * [aJuniorPuppetCannotLiftASeniorsBan] are the opposite: they pin behaviour the fix must preserve. + * The fix resolves the ordering by making authority only ever shrink across two passes — see + * [AuthorityResolver.resolve]. Note that a chain-local rule ("the author must not be banned by the + * state their edition chains from") would NOT have been enough: + * [aBannedAdminForksTheBanlistAtGenesisRatherThanChainingOntoTheirOwnBan] forks at genesis so no + * parent ever mentions the ban, and CORD-04 §4's re-heal union would carry it in anyway. The rule + * had to bind the union too, which is why it is expressed as a whole-pass mask. * - * Note for whoever writes that fix: a chain-local rule ("the author must not be banned by the state - * their edition chains from") is NOT sufficient — see - * [aBannedAdminForksTheBanlistAtGenesisRatherThanChainingOntoTheirOwnBan]. The rule has to bind - * CORD-04 §4's re-heal union too. + * Three tests pin behaviour the fix had to *preserve* rather than change: + * [selfUnbanIsStillRefused], [aJuniorPuppetCannotLiftASeniorsBan], and + * [aBanByOneAdminDoesNotDropTheGrantsOfAnother]. One pins the cost it deliberately accepts: + * [banningAnAdminAlsoDemotesEveryoneThatAdminPromoted]. */ class BannedStaffEscalationTest { private val owner = "0f".repeat(32) @@ -167,10 +171,12 @@ class BannedStaffEscalationTest { assertTrue(r.isBanned(alice), "the owner's ban lands") assertFalse(r.hasPermission(alice, ConcordPermissions.MANAGE_ROLES), "the ban-aware check refuses her") - // ...but this is the one every ROLE/GRANT/BANLIST gate inside resolve() actually consults. + // effectivePermissions still reports what her ROLES say — that is its job, and the members + // screen reads it to label her. What changed is that the resolver's own ROLE/GRANT/BANLIST + // gates no longer consult it for a banned author; they drop the edition outright. assertTrue( r.effectivePermissions(alice).has(ConcordPermissions.MANAGE_ROLES), - "ESCALATION: a banned staffer keeps the permissions the resolver's own gates read", + "the role-derived view is unchanged — only what it authorizes is", ) } @@ -178,11 +184,11 @@ class BannedStaffEscalationTest { fun aBannedAdminPromotesAFreshSockpuppetToAdmin() { val r = AuthorityResolver.resolve(community() + ownerBansAlice + aliceMintsAPuppet(), owner) - assertEquals(2, r.rank(puppet), "ESCALATION: the banned admin's role edition is honored") - assertFalse(r.isBanned(puppet), "the puppet is a clean npub — nothing to filter it on") - assertTrue( + assertEquals(null, r.rank(puppet), "the banned admin's role and grant editions are both dropped") + assertFalse(r.isBanned(puppet), "the puppet itself is a clean npub — it is never banned, just powerless") + assertFalse( r.hasPermission(puppet, ConcordPermissions.MANAGE_CHANNELS), - "ESCALATION: a banned member minted a live admin with the ban-aware check passing", + "a banned member cannot mint authority it no longer has to give", ) } @@ -196,8 +202,8 @@ class BannedStaffEscalationTest { val state = ConcordCommunityState.fold(editions, owner) - assertEquals(0, state.channels.size, "ESCALATION: the community's channels are irrecoverably tombstoned") - assertEquals("Owned by the guy you banned", state.metadata?.name, "ESCALATION: and its identity rewritten") + assertEquals(1, state.channels.size, "the puppet holds nothing, so its tombstone is inert") + assertEquals("My Community", state.metadata?.name, "and the community keeps its identity") } @Test @@ -206,8 +212,8 @@ class BannedStaffEscalationTest { val r = AuthorityResolver.resolve(editions, owner) - assertTrue(r.isBanned(bob), "ESCALATION: the surviving moderator is silenced, losing all authority with it") - assertTrue(r.isBanned(carol), "ESCALATION: and the plain members with them") + assertFalse(r.isBanned(bob), "the puppet's banlist edition is unauthorized, so the moderator stands") + assertFalse(r.isBanned(carol), "and so do the plain members") } @Test @@ -216,8 +222,9 @@ class BannedStaffEscalationTest { val r = AuthorityResolver.resolve(editions, owner) - assertTrue(r.isBanned(bob), "ESCALATION: banGate reads effectivePermissionsOf, which ignores her own ban") - assertTrue(r.isBanned(carol), "ESCALATION: same") + assertTrue(r.isBanned(alice), "her own ban stands — it was the owner's") + assertFalse(r.isBanned(bob), "banGate now drops a banned author's edition outright") + assertFalse(r.isBanned(carol), "same") } @Test @@ -231,8 +238,8 @@ class BannedStaffEscalationTest { val r = AuthorityResolver.resolve(editions, owner) assertTrue(r.isBanned(alice), "the owner's ban survives the fork — the union is down-only") - assertTrue(r.isBanned(bob), "ESCALATION: and so does the banned admin's, healed in as a concurrent ban") - assertTrue(r.isBanned(carol), "ESCALATION: same") + assertFalse(r.isBanned(bob), "the fix binds the UNION too: her fork is dropped before it can be healed in") + assertFalse(r.isBanned(carol), "same") } @Test @@ -241,8 +248,8 @@ class BannedStaffEscalationTest { val r = AuthorityResolver.resolve(editions, owner) - assertEquals(null, r.rank(bob), "ESCALATION: a banned admin stripped a live moderator's roles") - assertFalse(r.hasPermission(bob, ConcordPermissions.BAN), "ESCALATION: leaving nobody but the owner able to act") + assertEquals(5, r.rank(bob), "a banned admin's revoke is dropped, so the moderator keeps their role") + assertTrue(r.hasPermission(bob, ConcordPermissions.BAN), "and keeps the authority that comes with it") } @Test @@ -251,8 +258,8 @@ class BannedStaffEscalationTest { val r = AuthorityResolver.resolve(community() + ownerBansAlice + tombstone, owner) - assertEquals(null, r.roles()[modRole], "ESCALATION: a banned admin retired a role beneath them") - assertEquals(null, r.rank(bob), "ESCALATION: every holder of it silently loses their standing") + assertEquals(5, r.roles()[modRole]?.position, "a banned admin's tombstone is dropped, so the role survives") + assertEquals(5, r.rank(bob), "and its holders keep their standing") } @Test @@ -323,4 +330,36 @@ class BannedStaffEscalationTest { "a client that already folded the ban must refuse the rollback", ) } + + @Test + fun banningAnAdminAlsoDemotesEveryoneThatAdminPromoted() { + // The deliberate cascade, pinned because it is surprising and because it is the whole point. + // CORD-04 §4 drops every event from a banned npub, authority actions included, so a grant + // they made while in good standing goes too. That is what kills a sockpuppet minted moments + // before the ban — and the same rule costs the owner a legitimate promotion, which they have + // to re-issue. See B2 in docs/concord-soft-ban-audit.md. + val promoted = grant("36".repeat(32), carol, listOf(modRole), author = alice) + + val before = AuthorityResolver.resolve(community() + promoted, owner) + assertEquals(5, before.rank(carol), "while alice is in good standing, her grant stands") + + val after = AuthorityResolver.resolve(community() + promoted + ownerBansAlice, owner) + assertEquals(null, after.rank(carol), "banning alice retroactively drops the grant she authored") + } + + @Test + fun aBanByOneAdminDoesNotDropTheGrantsOfAnother() { + // The cascade must follow the banned author, not spread. Bob is untouched by alice's ban, so + // everything he authored keeps standing. + val carolByBob = grant("37".repeat(32), carol, listOf(modRole), author = bob) + // bob is a Mod at position 5 and the role he hands out is that same position, so the grant is + // only honored when authored by someone who outranks it — the owner does, bob does not. + val carolByOwner = grant("38".repeat(32), carol, listOf(modRole), author = owner) + + val r = AuthorityResolver.resolve(community() + ownerBansAlice + carolByBob + carolByOwner, owner) + + assertTrue(r.isBanned(alice), "alice is the only one banned") + assertEquals(5, r.rank(bob), "bob is untouched") + assertEquals(5, r.rank(carol), "and the owner's grant of carol stands") + } }