## Summary
`ingest_event`'s durable write-path restriction gate exempts NIP-43
relay-admin kinds **9030–9033**, so that a *timed-out* admin keeps
administrative capability. That exemption was ban-blind, and
`handle_relay_admin_event` performed no restriction check of its own. A
**banned** admin or owner could still add members, remove members,
change member roles, and set the workspace icon by posting a signed
NIP-98 request to `POST /events`. No open WebSocket required.
Reported externally by **Bilal Syed** (also filed publicly as #3020
before he read `SECURITY.md`). Verified true, reproduced live, and found
slightly worse than reported.
Same class as BUZZ-SEC-007, which PR #1915 closed for moderation command
kinds 9040–9044. That fix was never extended to the 9030 range.
## Why it worked
- `handlers/ingest.rs:1639` skipped the restriction check when
`is_relay_admin_kind(kind)` was true.
- `handlers/relay_admin.rs` did a freshness check and a role lookup only
— zero restriction reads in the file.
- A ban does not remove the role: `ban_member`
(`buzz-db/src/moderation.rs:314`) writes only `community_bans`, so the
`relay_members` admin row survives.
- The HTTP path never consulted ban state — `enforce_relay_membership`
is a bare `SELECT 1 FROM relay_members`.
- The ban was enforced only at the NIP-42 auth seam, which an HTTP
request never crosses.
**Worse than reported:** the report covered remove (9031) and icon
(9033). Add (**9030**) works too, so a banned admin can *plant* new
members. That matters because `moderation_authz.rs:163-170` derives "an
admin cannot ban an owner or fellow admin" from `relay_members` — the
very table 9030/9031 mutate. A banned admin could seed accomplices into
the roster the ban was meant to stop them touching.
Also of note: `moderation_authz.rs:158-165` already asserts in a comment
that *"The command handler separately rejects a banned actor on every
transport."* `relay_admin.rs` was the one command handler not holding
that invariant.
## The fix
Enforce the durable ban **inside `handle_relay_admin_event`** — the
reporter's own suggested shape, and the `moderation_commands.rs:99-108`
precedent.
Deliberately **not** the one-token alternative of dropping `&&
!is_relay_admin_kind(kind_u32)` at `ingest.rs:1639`: that would also
start blocking *timed-out* admins, silently changing policy. Bans are
refused; timeouts still administer, which is the entire reason the
exemption exists.
`handle_relay_admin_event` becomes a thin admission wrapper around an
unchanged `execute_relay_admin_command` body, so no future early return
inside that body can precede the check. The check therefore also
necessarily precedes the freshness check.
**The refusal category is part of the security contract**, so this
returns a typed `RelayAdminError` rather than a string. A `blocked:`
string would have kept the right wire text but returned **400** instead
of **403** (`api/bridge.rs:845` vs `:858`), and would have reported a
restriction-DB outage as a client error:
| Variant | Ingest | Wire | HTTP |
|---|---|---|---|
| `Banned` | `AuthFailed` | `blocked: you are banned from this
community` | **403** |
| `Rejected(..)` | `Rejected` | `invalid: …` | 400 (unchanged) |
| `Internal(..)` | `Internal` | `error: …` (sanitized) | **500** |
## Verification
Live over real HTTP against an isolated relay, all four exempt kinds
refused, DB checked after each for non-mutation:
```
[banned] 9031 remove -> 403 blocked: you are banned from this community
[banned] 9030 add -> 403 blocked: you are banned from this community
[banned] 9032 change role -> 403 blocked: you are banned from this community
[banned] 9033 set icon -> 403 blocked: you are banned from this community
```
Victim still `member`, planted key absent, role target unchanged, icon
still NULL. 9032 required a banned **owner** to be a real test, since it
is owner-only.
- **Mutation-tested.** The admission decision is the pure
`admits_relay_admin_command(&RestrictionState)`, covered by the
*default* suite. Neutering it fails
`banned_actor_is_not_admitted_to_a_relay_admin_command`. The first
version of this patch would have stayed green if someone deleted the
check — that gap is closed. The unit test does not prove handler
*wiring*; the `#[ignore]`d live E2E is what checks linkage.
- **Fail-closed proven empirically**, by manual fault injection rather
than assertion: renaming `community_bans.banned` out from under the
running relay yields 500, no mutation, and no schema detail leaked to
the client.
- Negative/positive controls: timed-out admin still administers *and* is
still content-write-blocked; clean admin unaffected with mutation
confirmed; non-admin still gets `invalid:`/400.
- Reviewed iteratively by **@Mari** over three rounds; final approval at
9/10+ on minimalness, elegance, and correctness. She also ran an
independent deep regression pass on an isolated stack (odd port 44391)
covering channel lifecycle, membership,
messages/replies/search/edit/delete, reactions, canvas, DMs, and
moderation transitions — no regressions.
- `cargo fmt --all --check`, `cargo clippy -p buzz-relay --all-targets
-D warnings`, `buzz-core` 229/229, `buzz-cli` 250/250, `run-tests.sh
unit` all five packages green.
- `buzz-relay --lib`: **756 passed / 1 failed**. The sole failure
`api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo` (504
vs 200) is **pre-existing** — reproduced identically in a detached
worktree at merge base `00ecf2c`.
## Notes for the reviewer
- Merged `origin/main` in as a merge commit rather than rebasing, per
instruction. No conflicts; the eight incoming commits touch none of the
three files here. Closest neighbour is `00ecf2c` (kind:9000 NIP-29
*channel* role authz) — disjoint from this NIP-43 *relay-admin* fix.
- **This does not close the class.** Two separate items remain open,
deliberately excluded to keep an externally-known security fix
reviewable:
1. **Command kinds dispatch before the gate.** `is_command_kind` fires
at `ingest.rs:1561`, ~80 lines *before* the restriction gate, and
`command_executor.rs` has no restriction read. Measured live: a banned
member can still open a DM (41010 → 200). 41011/41012/30620/46030/46031
unprobed. Needs per-kind semantics enumerated first (reports allowed
while banned; moderation commands allow timeouts but reject bans;
ordinary writes reject both).
2. **`moderation_commands.rs` maps its own restriction-DB failure to
400, not 500**, and leaks the raw Postgres message to the client.
- One correction for the public issue: its repro step 1 says
`kind:9041`, which is **unban**. The ban is **9040**
(`KIND_MODERATION_BAN`, `buzz-core/src/kind.rs:298`). Following the
steps verbatim yields a false negative.
Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
---------
Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
Co-authored-by: npub1jmc9dt2lyvzu3h0kxlwxt5zg4fxp9476awyxw6gwxn72g6cw7exqs64whm <96f056ad5f2305c8ddf637dc65d048aa4c12d7daeb8867690e34fca46b0ef64c@buzz.block.builderlab.xyz>
Co-authored-by: Tyler Longwell <tlongwell@block.xyz>
## Summary
NIP-29 `kind:9000` (PUT_USER) role changes were only authorized when the
**new** role was elevated. Demotions were unauthorized, so any
authenticated user could strip a channel owner to `member` with a single
event — and the demotion was unrecoverable, since the ex-owner then
lacked the privilege to restore themselves.
Reported by @Tyler in `#buzz-security`. Verified true, plus two adjacent
defects the report flagged and one it did not.
## The defects
1. **Demotion unauthorized.** The actor check only fired when the
*requested* role was elevated. Lowering someone's role skipped it
entirely.
2. **Open channels skipped the actor check.** It was nested under
`visibility == "private"`.
3. **`add_member` had no last-owner guard** while `remove_member` did —
so a channel could be left with zero owners.
4. **(Not in the report.)** An absent `role` tag defaulted to `Member`,
so a bare self-targeted PUT_USER silently demoted the sender. No
attacker required.
## The fix
**`crates/buzz-db/src/channel.rs`** — the authority, because it also
covers the desktop/admin callers that bypass the relay validator:
- Changing an **active** member's role requires an elevated actor **in
both directions**. Re-adding at the same role stays unguarded and
idempotent (the huddle bot-add and `kind:9021` join paths depend on
this).
- Last-owner guard in `add_member`, mirroring `remove_member`.
- Keyed on the **active** role (`removed_at IS NULL`). A soft-removed
row's role is history, not live authority — otherwise soft-deleted
ownership becomes a resurrection token: a kicked owner self-rejoins via
`9021` and silently regains ownership.
- New `pg_advisory_xact_lock` on a channel-membership namespace, taken
as the first statement in both `add_member` and `remove_member`. Both
read an owner `COUNT` and then write a *different* row, so READ
COMMITTED alone lets two concurrent demotions each observe 2 owners and
together leave 0.
- `remove_member`'s `is_agent_owner` lookup moved before the transaction
opens — it borrows a second pool connection, and issuing it while
holding the lock could self-deadlock on a small pool. Safe because
`agent_owner_pubkey` is immutable (first-mint-wins).
**`crates/buzz-relay/src/handlers/side_effects.rs`**:
- Role tag is now `Option` — absent means "no role change requested"
rather than defaulting to `Member`.
- Actor-role lookup hoisted out of the `visibility == "private"` block,
so open channels are covered.
- Role-change and last-owner guards on every visibility. Rejecting here
*as well as* in the DB means clients get a real error instead of an `OK`
whose side effect then fails silently.
## Verification
**Mutation tested — every guard stubbed individually to confirm a test
actually dies.** Three of eight guards were originally uncovered and
survived being disabled with the suite fully green:
| Guard | Dying test |
|---|---|
| DB actor-auth | *survived* → **new**
`unprivileged_member_cannot_demote_a_co_owner` |
| DB last-owner | `owner_can_still_manage_roles_after_demotion_guard` |
| DB active-role (soft-remove) |
`kicked_owner_rejoins_as_member_not_owner` + 3 |
| `add_member` advisory lock |
`membership_writes_serialize_on_the_shared_channel_lock` |
| `remove_member` advisory lock | +
`remove_member_rejects_an_actor_demoted_while_it_waited` |
| relay no-role-tag preservation |
`test_nip29_put_user_without_role_tag_preserves_role` |
| relay actor-auth | *survived* → **new**
`test_nip29_relay_rejects_role_change_by_unprivileged_actor` |
| relay last-owner | *survived* → **new**
`test_nip29_relay_rejects_last_owner_self_demotion` |
The three gaps shared one cause: every existing test asserts resulting
**state** ("the role did not change"), and the DB guards enforce that
state, masking every layer above them. With a relay guard stubbed the
relay answers `accepted:true` and logs `Side effect failed: access
denied: ...` while the state assertion still passes — the entire relay
validator could be deleted unnoticed. The new relay tests assert
`accepted == false` instead, the one observable only the validator
controls. Each new test is verified in both directions: green against
the real fix, failing with its intended message when its guard alone is
stubbed.
**Test runs** (at `9461eedb`):
- `buzz-db`, serial: **210 passed / 3 failed** — the same 3 failures as
clean `main` (202/3), which are pre-existing and unrelated
(`concurrent_same_owner_create…`,
`create_community_with_owner_is_atomic…`,
`test_usage_metrics_lock_has_single_owner…`). +8 = the new tests.
- `e2e_relay --ignored`: **40 passed / 3 failed**. Clean `main` on the
same relay is 35/6 — the same 3 infra failures
(`test_invite_mint_and_claim…`, `test_subscription_limit_enforced`,
`test_unarchive_emits_member_added_notification`) plus the 3 security
tests that fail unpatched and pass here.
- `cargo fmt`, `clippy`, `git diff --check` all clean.
**Live manual drive** against a locally running relay, using raw
`nak`-signed events (the `buzz` CLI refuses malformed `kind:9000`, so
the guards have to be exercised directly):
- *Rejected:* member demotes owner; member demotes admin; self-promote
to owner; self-promote to admin; admin demotes the last owner; sole
owner self-demote; demoted ex-owner demotes last owner; private-channel
member demotes owner; non-member demotes owner in private.
- *Allowed:* bare PUT_USER with no role tag (owner keeps role);
idempotent re-add at same role; owner promotes admin→owner, then owner2
legitimately demotes owner1.
- *Resurrection defeated:* owner promotes attacker to admin → kicks them
(`9001`) → attacker self-rejoins (`9021`) → returns as **member**, not
admin, and cannot demote the owner.
- Normal ops unaffected throughout: channel creation, messaging, member
listing, and legitimate governance all work.
## Behavior change to be aware of
Huddle bot-add sends `role="bot"`. If the target is **already an active
member at a different role**, that is now a role change and requires an
elevated actor. Previously it silently re-roled them — the same privesc
primitive through a different door, so narrowing it is intended.
This does not break the huddle flow in the path that matters: the
ephemeral channel add (the one that fails hard) is performed by the
host, who *created* that channel and is therefore its owner — verified
live. The parent-channel add is already explicitly best-effort,
capturing the error into `parent_error` with a comment anticipating "may
already be member"; adding a non-member agent there still works.
Flagging it rather than burying it.
## Notes
- Commit is **signoff-only, not cryptographically signed** — `-S` fails
in this environment (git tries to load the agent npub as an SSH key
file). DCO trailers are present and correct.
- Branch was merged with `origin/main` via `--no-ff` (not rebased).
Upstream had 17 commits, none touching these files, no migration
changes.
---------
Signed-off-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com>
Co-authored-by: Dawn (sprout agent) <c6237ef84fa537c78dcee78efd2d4e59f728859c7f194da42ac51ededfa0be05@sprout-oss.stage.blox.sqprod.co>
Kind 30175 persona sync events carry plaintext `system_prompt` and
`respond_to_allowlist`. This PR adds **author-only-unless-shared read
semantics**: events without `["shared","true"]` are visible only to the
author; events with that tag are community-readable.
## What changed
### New read class (kind 30175)
Kind 30175 gets per-event gating at every relay read surface. The
`shared` marker is a **tag**, not a content field, so content bytes
(which double as the `source_version` drift basis) are not affected when
toggling share state.
### `event_visible_to_reader` helper (`handlers/req.rs`)
Centralizes the three per-event access predicates —
`is_author_only_event`, `is_unshared_persona_event`,
`reader_authorized_for_event` — into one `pub(crate)` fn callable from
both WS and HTTP adapters. All result-visibility sites now call this
single helper.
### NIP-98 HTTP bridge (`api/bridge.rs`)
- `POST /query` catchall: replaced the two-step author-only +
result-gated checks with `event_visible_to_reader` (now also covers the
persona shared-gate).
- `POST /count`: added `needs_persona_filtering` to the fast-path guard
(forces per-event fallback when filter can match `kind:30175`) and
replaced both fallback loops' individual checks with
`event_visible_to_reader`.
- FTS `/search` bridge helper: replaced `is_author_only_event` with
`event_visible_to_reader` as defense-in-depth (30175 is not in the FTS
allowlist today; comment at site explains the future-proofing intent).
### Ingest validation (`handlers/ingest.rs`)
`validate_persona_envelope` rejects malformed `shared` tags: wrong
value, missing value, duplicates. Accepts exactly `["shared","true"]`
and tag-absent.
### Kind helpers (`buzz-core/src/kind.rs`)
`is_persona_shared_kind`, `is_unshared_persona_event`,
`filter_can_match_persona_shared_kinds`.
### Tests (`e2e_persona.rs`)
8 unit tests in `kind.rs`, 6 in `ingest.rs`, 8 e2e tests total:
- AC-1–6 covering the gated surfaces
- `test_persona_live_fanout_shared_gate`: reworked with explicit
monotonic `created_at` timestamps (t0 < t1 < t2) and per-step head
assertions, eliminating the NIP-33 event-id tie-break race. Also asserts
foreign live subscription receives nothing on shared→unshared
transition.
- `test_persona_ingest_shared_tag_validation`: added `shared=x` and
missing-value wire-level rejection cases.
- `test_persona_mixed_kind_filter_does_not_leak`: publishes a kind-9
event and asserts it IS returned; absence-only assertion no longer
sufficient.
- `test_persona_http_query_cross_author_gate`: NIP-98 `/query`
cross-author gate (authors filter, kindless `ids` — both blocked; shared
`ids` — passes).
- `test_persona_http_count_cross_author_gate`: NIP-98 `/count`
cross-author gate (foreign sees 1/shared, author sees all, wildcard
checked).
### NIP-AP.md
Replaced aspirational "every relay read chokepoint" wording with an
enumerated list of gated surfaces including NIP-98 `/query`, `/count`,
and FTS/search with their enforcement mechanism named. Added
**Non-goal** note for side-band existence oracles
(reaction/report/deletion target resolution).
## Existing tests
All pre-existing `e2e_persona` tests use `{ids:[event_id]}` or
`{authors:[self]}` filters — author self-reads bypass the gate and are
unaffected.
## Gates
`just check` ✅ | `just test-unit` ✅ | `cargo test -p buzz-relay` ✅ (749
passed, 1 pre-existing failure in
`demo_join_forwarded_arm_round_trips_echo` — flaky on `main`, unrelated
to this PR, verified red at `origin/main` before this branch)
---------
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Co-authored-by: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
