Repository navigation
Conversation
A kind:9002 that changes a private channel to open changes the admission rules under everyone who joined it while it was private. The relay now refuses that change unless every member, current or removed, is the sender or an agent whose NIP-OA owner is the sender, for example a Solo chat that holds only its owner and the owner's agents. Removed members count because their posts stay in the history that opening would publish. Open to private stays allowed. The check runs under the membership lock that a visibility change already takes, so the member set it reads stays the same until commit. A refused edit stores nothing. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
|
🤖 From the information-flow design review (#9290): the private-to-open exception must count former members as well as current ones. Otherwise an admin could remove another person and then open the channel, publishing that person's posts, which stay in its history. This PR already does that: |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent — APPROVE
Reviewed: 18f3e35413c6fd9b5ebc9254aca4981c6ca7fd5c..9e28cfbdadb83fb03ecb054ac053766b442ec03e (exact head, clean review worktree). Risk: high — opening a private channel exposes its historical messages to new readers.
Integrated review: The kind:9002 metadata edit checks historical membership under the per-channel membership lock, inside the same transaction as the event and visibility update (crates/buzz-db/src/store/channel.rs:1647-1667,1724-1739; crates/buzz-relay/src/handlers/ingest.rs:3677-3692). Removed members and users without a profile still count; only the sender and agents with a same-community NIP-OA owner link to that sender are exempt. The Desktop settings flow already displays the resulting invalid: refusal inline without closing on failure (desktop/src/features/channels/ui/ChannelManagementSheet.tsx:338-340,588-592). The new Postgres regression exercises allowed and refused transitions, removed/unknown/other-owned members, agent senders and historical open→private→open (crates/buzz-relay/src/handlers/edit_metadata_postgres_tests.rs:877-1050). No blocking author-actionable finding in the systems or product/adversarial review.
Validation: At exact head, reviewer cargo fmt --all --check and git diff --check passed. GitHub run 42713900367, job 129496415666, reports the PostgreSQL-backed test step successful at this SHA: private_to_open_needs_no_other_members PASS, 1,490/1,490 tests passed (2,371 skipped by lane selection). Relay E2E, backend integration and desktop E2E integration checks also succeeded at this head. This is not a claim that the reviewer ran a local Postgres or native session.
Author action: none. Optional only: the comment near edit_metadata_postgres_tests.rs:871 incorrectly says removed members do not count; behavior and assertions do count them. Verification owner: reviewer/tooling for an isolated live kind:9002 refusal-and-allowed journey if further manual confidence is wanted. Residual risk: local Postgres/live-relay and native UI observation were unavailable (Docker socket unavailable); CI's real Postgres regression covers the principal storage/ingest boundary. The public update_channel DB helper deliberately does not enforce this sender-dependent rule and must remain an internal trusted boundary (channel.rs:1102-1136).
The test asserts that removed members block opening; its doc comment said the opposite. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
jedwards27
left a comment
There was a problem hiding this comment.
🤖 :bot: Jude’s code review agent — APPROVE
Reviewed: 18f3e35413c6fd9b5ebc9254aca4981c6ca7fd5c..2482ad871418b8475dd899b7e55939cb5eb8f727 (exact head, clean checkout). Risk: high: opening private-channel history can disclose old posts. The only change since the prior reviewed head 9e28cfbdadb83fb03ecb054ac053766b442ec03e is a corrected test comment at crates/buzz-relay/src/handlers/edit_metadata_postgres_tests.rs:871–873: removed members do count. The policy implementation is unchanged.
Integrated findings: No blocking author-actionable defect. The kind:9002 path checks current and removed membership under the admission lock before committing visibility or the edit event (crates/buzz-db/src/store/channel.rs:1221–1293,1642–1667,1680–1794; crates/buzz-relay/src/handlers/ingest.rs:3677–3692). Members without a user/owner record count as outsiders; only the sender and agents with a recorded same-community owner link to that sender are exempt. The searched production call sites in crates/buzz-{relay,db}/src showed no alternate user-facing private→open writer; Db::update_channel is an explicitly unguarded internal trust boundary (channel.rs:1102–1136). The regression table exercises refused and permitted edits, including removed members and agent senders (edit_metadata_postgres_tests.rs:871–1050).
Product/accessibility follow-up, non-blocking: The unchanged legacy Desktop settings view displays all mutation errors as a plain paragraph rather than announcing them as an alert (desktop/src/features/channels/ui/ChannelManagementSheet.tsx:335–354,588–592), so a screen-reader user may miss this new refusal while focus stays on Save. This generic client gap predates the relay change; this PR has no client diff, and the active Desktop policy UI is separately proposed in block/buzz-app#3145. Give the client owner that accessibility follow-up rather than expanding a relay-only PR into legacy-client remediation. No other product/UI finding was established.
Validation: At clean exact head, reviewer cargo fmt --all --check and git diff --check 9e28cfb..HEAD passed; independent systems review also passed scripts/test-postgres-test-discovery.sh. GitHub run 42821161302 is successful at this exact head; its PostgreSQL job logs handlers::edit_metadata_postgres_tests::private_to_open_needs_no_other_members PASS. Relay E2E, backend integration, desktop E2E integration, Rust lint/unit and macOS build checks also succeeded at this head. These CI results are not a claim of local Postgres/native execution.
Author action: none for this PR. Verification owner: reviewer/tooling for an isolated live-relay permitted-open/refused-open journey if further manual confidence is required; active Desktop/client owner for the screen-reader error-announcement follow-up. Manual/native evidence: none at this head. Residual risk: no independent native assistive-technology or live-relay observation; the exact-head PostgreSQL regression and integration CI cover the storage/ingest boundary, not screen-reader announcement or a packaged client.
Summary
invalid: a private channel with other members cannot be made open; create a new open channel instead.Details
update_channelin the store does not apply this rule. Only tests call it. User edits go through kind:9002. Both doc comments say so.NOSTR.mddocuments the refusal on the kind:9002 row.Related issue
None found. The client UX is in block/buzz-app#3145.
Testing
edit_metadata_postgres_tests.rscover: only the owner, the owner with their agent, the owner's removed agent, a removed person, another person, a member with no user record, someone else's agent, an agent sending with its owner as a member, and a stranger who joined while the channel was open.🤖 Generated with Claude Code