Scope relay runtime state by community#1658
Conversation
wesbillman
left a comment
There was a problem hiding this comment.
Review on behalf of Wes (posted by Pinky, agent).
The relay fix itself (f4acc0e9e) — excellent, would approve alone
- Real cross-tenant confidentiality bug: an attacker who learns a victim huddle UUID (kind:48100 event) can recreate it in their own community and land in the victim's live room. Re-keying
AudioRoomManagerto(CommunityId, Uuid)and threadingtenant.community()throughget_or_createand everycleanup_if_emptypath is the minimal causal fix, sourced from the already-resolved tenant — no new trust surface. observer_owner_cachere-key to(CommunityId, agent, owner)covers all three touchpoints (event-path get/insert, NIP-OA prewarm). I grepped for unscoped stragglers; found none.- Both vulns get real regression tests (same-UUID two-community room isolation; cached allow in A not bypassing B's deny, asserting the restricted OK response). Nice work — this should land fast.
Requesting changes: 2 of 3 commits don't belong in this PR
-
a80920229(CLI inbox/TUI + ATTACK_PLAN.md, +4,076 lines) is dead code.inbox.rsandtui.rsare not declared incommands/mod.rs, and they importratatui/crosstermwhich aren't inbuzz-cli's Cargo.toml — they never compile, so CI green tells us nothing about them.ATTACK_PLAN.mdat repo root reads as session scratch from the security sweep. Please drop this commit. -
58a2849d8(ACP author gate diagnostics, +935 lines) is live behavior change to a different subsystem. TheAuthorGateDecisionrefactor + reason logging looks genuinely useful, and the base-prompt "Work Intake" section is plausibly valuable prompt-injection hardening — but base-prompt changes affect every deployed agent and deserve review on their own merits, not riding under a relay-tenancy title. Please move to its own PR. -
FOUND_VULNS.mdat repo root is a deliberate policy call — a public vuln write-up with repro shape in-tree vs. an advisory/internal doc. Fine if intentional, but worth deciding explicitly rather than by drive-by.
Suggested path: rebase this PR to just f4acc0e9e (it merges clean and is CI-covered), and open a separate PR for the ACP gate diagnostics. Happy to re-review immediately — the fix itself is ready.
f4acc0e to
00a7698
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Re-review on behalf of Wes (posted by Pinky, agent).
All requested changes addressed: the PR is now the single relay-tenancy commit touching only crates/buzz-relay (+173/−24) — dead CLI code, lab scripts, ATTACK_PLAN.md, FOUND_VULNS.md, and the ACP gate-diagnostics work are all gone.
I verified the new head's relay diff is line-for-line identical to the previously reviewed fix commit (f4acc0e9e), which passed full CI. The rekeying of AudioRoomManager to (CommunityId, Uuid) and observer_owner_cache to (CommunityId, agent, owner) covers every lookup/insert/cleanup site, with regression tests for both vulns. Approving — merge once the fresh CI run is green. Thanks for the fast turnaround; this is an important fix.
What changed
This changes the relay's in-memory huddle room registry and observer owner cache to include
community_idin their keys.AudioRoomManagernow keys rooms by(community_id, channel_id), and the audio connection handler passes the resolved tenant community through every room lookup and cleanup path. Two communities that reuse the same channel UUID no longer share peer lists, protocol pins, or audio frames.The observer owner cache now keys
(community_id, agent_pubkey, owner_pubkey)for both event-path lookups and NIP-OA prewarming. A cached allow in one community can no longer authorize observer traffic in another community for the same agent/owner pair.Why
Both structures were process-global while their previous keys only named community-local identifiers. That meant a local two-tenant collision could cross the host-derived community boundary after authentication had already succeeded.
An attacker needs two things:
The attack is:
Safety and tradeoffs
This does not change wire formats or database schema. The only runtime cost is carrying one additional
CommunityIdin the room and cache keys, which is required to preserve tenant isolation.Testing
cargo test -p buzz-relaycargo fmt -p buzz-relay --checkcargo clippy -p buzz-relay --all-targets -- -D warningsgit diff --check