Skip to content

Refuse opening a private channel that has other members - #9307

Draft
loganj wants to merge 2 commits into
mainfrom
loganj/refuse-private-to-open
Draft

loganj wants to merge 2 commits into
mainfrom
loganj/refuse-private-to-open

Conversation

@loganj

@loganj loganj commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Turning a private channel open changes who can read and join it for everyone who joined while it was private. The relay now refuses that change when the channel has, or once had, any other member.
  • Removed members count too, because their posts stay in the history that opening would make public.
  • Opening is still allowed when every member, current or removed, is the person making the change or one of that person's agents. A Solo chat that holds only its owner and the owner's agents is the main case.
  • Changing an open channel to private is unchanged.
  • A refused change stores nothing. The sender gets: invalid: a private channel with other members cannot be made open; create a new open channel instead.

Details

  • The check is in the channel metadata edit path (kind:9002), for every channel type. An agent counts as the sender's own only when its owner is recorded for it through an owner attestation (NIP-OA). A member who has never signed in, or an agent with no recorded owner, counts as another member.
  • The check runs under the membership lock that a visibility change already takes. Members cannot be added between the check and the commit.
  • Disclosure levels for agents do not exist yet, so all of the sender's own agents may stay when the sender opens a channel. When levels land, restricted writers will need to be excluded.
  • update_channel in the store does not apply this rule. Only tests call it. User edits go through kind:9002. Both doc comments say so.
  • Old desktop builds can still send an opening edit. They now get the refusal as an error.
  • NOSTR.md documents the refusal on the kind:9002 row.

Related issue

None found. The client UX is in block/buzz-app#3145.

Testing

  • New Postgres tests in edit_metadata_postgres_tests.rs cover: 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.
  • Each refused case checks that the channel stays private and nothing is stored. The tests fail when the check is removed, when it skips removed members, or when the user join is made inner.

🤖 Generated with Claude Code

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>
@loganj

loganj commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 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: has_member_besides_sender_and_agents reads channel_members with no removed_at filter, and private_to_open_needs_no_other_members covers it with the case "a removed person still counts: their posts stay". No change needed; noting it so reviewers know the former-member rule is intended.

@loganj
loganj marked this pull request as ready for review November 9, 2026 23:00
@loganj
loganj requested a review from a team as a code owner November 9, 2026 23:00
@loganj
loganj deployed to codex-review November 9, 2026 23:00 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 9, 2026
@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated, security-focused review generated by Codex.
Use it as a supplement to human review; false positives are possible.

Scope

  • Exact PR diff: 44b3c4823f0cf1f7715f85dbf5566cb64e848162...2482ad871418b8475dd899b7e55939cb5eb8f727
  • Model: gpt-6.1-sol

💡 Click "edited" above to see earlier reviews for this PR.


Review Summary

Overall Risk: NONE

No concrete security, correctness, or reliability findings in the authorized PR range.

Findings

No concrete security, correctness, or reliability findings were identified.

Notes

  • Review used read-only source and history inspection. Builds and tests were not run, as requested.

Generated by Codex Security Review |
Requested by: @loganj |
Workflow run

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 :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>
@github-actions github-actions Bot removed the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 10, 2026
@loganj
loganj deployed to codex-review November 10, 2026 13:04 — with GitHub Actions Active
@github-actions github-actions Bot added the codex-security-review-current The posted Codex security review matches its recorded range. label Nov 10, 2026

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 :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.

@loganj
loganj marked this pull request as draft November 10, 2026 15:08

This branch was successfully deployed

1 active deployment
codex-review — 2482ad87 Deployed Nov 10, 2026 by loganj via Run Codex Security Review #14225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codex-security-review-current The posted Codex security review matches its recorded range.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants