diff --git a/.claude/skills/resolving-issues/SKILL.md b/.claude/skills/resolving-issues/SKILL.md index 201b43a2..e69f8634 100644 --- a/.claude/skills/resolving-issues/SKILL.md +++ b/.claude/skills/resolving-issues/SKILL.md @@ -181,11 +181,32 @@ Coordinator owns this check (metadata work, allowed under "coordinator never cod ### Waiting for implementer commits without polling - Implementer agent runs in the background. Coordinator gets a notification when the agent's tool call completes — but the agent's *commit* may land seconds before that, and the harness's stop hook fires on uncommitted changes during cargo gates. - **Don't sleep, don't poll, don't read the agent's output file.** The system explicitly blocks long sleeps and warns against reading sub-agent transcripts (context overflow). -- **Use `Monitor` with an `until` loop watching for HEAD to advance** past the prior known SHA. One notification fires when the loop exits; the coordinator stays idle in between. Pattern: +- **Capture the full pre-dispatch SHA via `git log -1 --format='%H' ` BEFORE arming the wait.** Use that exact 40-char string in the until-condition. Never type/synthesize the hash from a short prefix (`31c7851` ≠ `31c7851bf8c6...` — the loop will exit immediately on the mismatched comparison and you'll spuriously conclude the agent committed). Pattern: capture into a shell variable, interpolate into the loop. +- **Use `Monitor` (or `Bash` background with an `until` loop) watching for HEAD to advance** past the captured SHA. One notification fires when the loop exits; the coordinator stays idle in between. Pattern: ```bash - cd /home/user/willow && until [ "$(git log -1 --format='%H')" != "" ]; do sleep 5; done; echo "agent committed: $(git log -1 --oneline)" + PRIOR=$(git log -1 --format='%H' ) + until [ "$(git log -1 --format='%H' )" != "$PRIOR" ]; do sleep 10; done + git log -1 --oneline ``` - Set `timeout_ms` to ~5–10 minutes for typical implementer runs (longer for cross-crate gates). The coordinator can do GitHub-API metadata work (filing follow-up issues, drafting PR body) while waiting; just don't touch files in the implementer's scope. + Set `timeout` to ~10–15 minutes for typical implementer runs (longer for cross-crate gates). The coordinator can do GitHub-API metadata work (filing follow-up issues, drafting PR body) while waiting; just don't touch files in the implementer's scope. +- **Two independent signals: SHA-advance + agent-completion notification.** They arrive on their own schedules — agent-completion can lag the commit by minutes (or vice versa, agent can complete WITHOUT committing — see "Implementer cuts off mid-flight" below). Trust whichever signal answers your immediate question: + - "Did the implementer's work land on master?" → check git ref / wait for SHA-advance. + - "Did the implementer agent terminate?" → wait for agent-completion notification. + Don't treat them as redundant — and don't preemptively conclude the implementer crashed just because one arrived first. + +### Implementer cuts off mid-flight (uncommitted edits, agent terminated) +- A surprisingly common failure mode: the implementer agent gets cut off (token budget, time slice, harness limit) AFTER making the substantive edits but BEFORE running the local merge gate or committing. Symptoms: + - Agent-completion notification arrives with a truncated/incomplete summary (e.g. "Spec doc looks good. Let me check the test result." mid-thought). + - `git status` shows uncommitted changes matching the issue's expected scope. + - Stop hook fires complaining about uncommitted changes. +- **Don't conclude the work is wrong** — inspect the diff. If it matches the issue's intent, the edits are usually correct; the agent just ran out of runway before the gate-and-commit step. +- **Don't restart the issue from scratch.** Dispatch a thin **finalize-implementer** agent: brief it on the existing uncommitted state, hand it the gate list + commit-and-push instructions verbatim, no need to re-do brainstorming or TDD-red. The previous implementer's substance stands; the finalize agent's job is the mechanical close-out (verify gates, commit, push, report SHA). +- **Brief the finalize agent on what's already done** — paste a `git diff --stat` summary so it knows the scope and doesn't redo the work or panic at the size. Cite the previous agent's brainstorm decision (Option A/B etc.) so it carries the same reasoning into the commit body. +- **Stop-hook noise is normal mid-implementer.** A `git status` showing uncommitted changes during a cargo gate run does NOT mean the implementer crashed. Only suspect crash-before-commit if (a) sufficient time has passed for the gates to finish (typically 5–10 min for a small fix, longer for `just check` workspace-wide), AND (b) the agent-completion notification has arrived AND no commit was made. Don't preemptively dispatch a finalize agent based on stop-hook chatter alone. +- **Two false-alarm patterns to avoid:** + 1. *Wrong-SHA wait exits immediately, you assume the implementer never started.* (See "Capture the full pre-dispatch SHA" above.) Result: redundant finalize dispatch on a tree the original agent already cleanly committed. + 2. *Agent-completion notification lags the commit.* By the time you see the agent's summary, the work has been on origin for minutes. The redundant finalize agent will report "no changes to commit, gates green" — harmless but wasteful (~5 min cargo lock contention). + Both are mitigated by: capture full SHA → arm SHA-watch → wait for either signal → on signal arrival, `git status` first to disambiguate (clean tree = real commit landed; uncommitted edits matching scope = mid-flight, dispatch finalize). ### Implementer-flagged out-of-scope rot - When the implementer surfaces pre-existing rot it intentionally doesn't fix (e.g. unrelated wasm break under `--all-features`, dead-code warnings in untouched files), the coordinator files a follow-up GH issue using `mcp__github__issue_write` (metadata work, allowed under the Coordinator-never-codes rule). Cite the discovery context (which dispatch surfaced it, which commit + gate step) so the next run has full provenance. diff --git a/crates/network/src/lib.rs b/crates/network/src/lib.rs index 35a3640d..406a82c1 100644 --- a/crates/network/src/lib.rs +++ b/crates/network/src/lib.rs @@ -19,7 +19,7 @@ pub mod iroh; pub mod topics; pub mod traits; -#[cfg(any(test, feature = "test-utils"))] +#[cfg(all(not(target_arch = "wasm32"), any(test, feature = "test-utils")))] pub mod mem; // Re-export key types for convenience. diff --git a/crates/state/src/materialize.rs b/crates/state/src/materialize.rs index 4f057a69..c0552f60 100644 --- a/crates/state/src/materialize.rs +++ b/crates/state/src/materialize.rs @@ -301,16 +301,23 @@ fn required_permission(kind: &EventKind) -> Option { // RevokePermission, // RenameServer, // SetServerDescription — admin-only, checked in the admin block above - // SetProfile — unrestricted (any member) - // UpdateProfile — unrestricted (any member; self-authorship - // is the only identity check) + // SetProfile — any current member; membership gate + // lives in apply_mutation (issue #177) + // UpdateProfile — any current member; membership gate + // lives in apply_mutation. Self- + // authorship is enforced structurally + // (only the author's own profile is + // mutated). See issue #177. // PinMessage, - // UnpinMessage — unrestricted (any member) + // UnpinMessage — any current member; membership gate + // lives in apply_mutation (issue #177) // ChannelRevive — membership check lives in apply_mutation // (does not require SendMessages so a muted // member can still un-archive) // MuteChannel, // MuteGrove — per-identity preference, never gated + // (preferences are not server state and + // survive a kick) // // If a new EventKind variant is added and is NOT listed here or // in an arm above, it will silently get no permission check. @@ -539,6 +546,15 @@ fn apply_mutation(state: &mut ServerState, event: &Event) -> ApplyResult { } EventKind::SetProfile { display_name } => { + // Defense-in-depth: reject if the author isn't a member of + // the server. `required_permission()` returns `None` for + // SetProfile (it's "any current member"), so the membership + // gate must live here. Without it, late-arriving events + // from a kicked or never-joined signer would silently + // mutate `state.profiles`. See issue #177. + if !state.members.contains_key(&event.author) { + return ApplyResult::Rejected(format!("author '{}' is not a member", event.author)); + } if display_name.chars().count() > 64 { return ApplyResult::Rejected(format!( "display name exceeds 64 chars ({} chars)", @@ -556,6 +572,17 @@ fn apply_mutation(state: &mut ServerState, event: &Event) -> ApplyResult { } EventKind::UpdateProfile(delta) => { + // Defense-in-depth: reject if the author isn't a member of + // the server. `required_permission()` returns `None` for + // UpdateProfile (it's "any current member"; self-authorship + // is already structurally enforced by mutating only the + // author's own profile). The membership gate must live + // here so late-arriving events from a kicked or never- + // joined signer cannot silently mutate `state.profiles`. + // See issue #177. + if !state.members.contains_key(&event.author) { + return ApplyResult::Rejected(format!("author '{}' is not a member", event.author)); + } let crate::types::ProfileDelta { display_name, pronouns, @@ -662,6 +689,15 @@ fn apply_mutation(state: &mut ServerState, event: &Event) -> ApplyResult { channel_id, message_id, } => { + // Defense-in-depth: reject if the author isn't a member of + // the server. `required_permission()` returns `None` for + // PinMessage (it's "any current member"), so the membership + // gate must live here. Without it, late-arriving events + // from a kicked or never-joined signer would silently + // mutate `pinned_messages`. See issue #177. + if !state.members.contains_key(&event.author) { + return ApplyResult::Rejected(format!("author '{}' is not a member", event.author)); + } if let Some(ch) = state.channels.get_mut(channel_id) { ch.pinned_messages.insert(*message_id); } @@ -671,6 +707,15 @@ fn apply_mutation(state: &mut ServerState, event: &Event) -> ApplyResult { channel_id, message_id, } => { + // Defense-in-depth: reject if the author isn't a member of + // the server. `required_permission()` returns `None` for + // UnpinMessage (it's "any current member"), so the + // membership gate must live here. Without it, late- + // arriving events from a kicked or never-joined signer + // would silently mutate `pinned_messages`. See issue #177. + if !state.members.contains_key(&event.author) { + return ApplyResult::Rejected(format!("author '{}' is not a member", event.author)); + } if let Some(ch) = state.channels.get_mut(channel_id) { ch.pinned_messages.remove(message_id); } diff --git a/crates/state/src/sync.rs b/crates/state/src/sync.rs index 233e14bc..bae4624f 100644 --- a/crates/state/src/sync.rs +++ b/crates/state/src/sync.rs @@ -881,7 +881,11 @@ mod tests { dag.insert(e).unwrap(); } - // Grant permissions to multiple peers. + // Grant permissions to multiple peers, recording each grant + // hash so peer SetProfile events can causally depend on it. + // (SetProfile requires membership — issue #177 — so the grant + // must topologically precede the SetProfile.) + let mut grant_hashes: std::collections::BTreeMap<_, _> = std::collections::BTreeMap::new(); for peer in [&peer_a, &peer_b, &peer_c] { let e = dag.create_event( &owner, @@ -892,17 +896,20 @@ mod tests { vec![], 0, ); + grant_hashes.insert(peer.endpoint_id(), e.hash); dag.insert(e).unwrap(); } - // Set profiles from different peers. + // Set profiles from different peers, each depending on the + // grant that made them a member. for (peer, name) in [(&peer_a, "Alice"), (&peer_b, "Bob"), (&peer_c, "Carol")] { + let dep = grant_hashes[&peer.endpoint_id()]; let e = dag.create_event( peer, EventKind::SetProfile { display_name: name.into(), }, - vec![], + vec![dep], 0, ); dag.insert(e).unwrap(); diff --git a/crates/state/src/tests/dag.rs b/crates/state/src/tests/dag.rs index 4d6e4f7c..10a7e332 100644 --- a/crates/state/src/tests/dag.rs +++ b/crates/state/src/tests/dag.rs @@ -422,12 +422,28 @@ fn managed_dag_insert_remote_event_applies_to_state() { // Simulate a remote event from a different peer. let peer = Identity::generate(); + // Grant peer membership first — `SetProfile` is gated on + // membership (issue #177); the grant adds peer to `state.members`. + let grant = managed.dag().create_event( + &owner, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: crate::event::Permission::SendMessages, + }, + vec![], + 50, + ); + let grant_hash = grant.hash; + managed.insert_and_apply(grant).unwrap(); + + // Causally link the SetProfile to the grant so topological sort + // applies the grant first. let event = managed.dag().create_event( &peer, EventKind::SetProfile { display_name: "Alice".into(), }, - vec![], + vec![grant_hash], 100, ); let outcome = managed.insert_and_apply(event).unwrap(); @@ -450,13 +466,28 @@ fn managed_dag_buffers_gap_events_and_resolves() { let peer = Identity::generate(); let mut managed = ManagedDag::new(&owner, "Test", 5000).unwrap(); - // Create peer's seq=1 event. + // Grant peer membership first — `SetProfile` is gated on + // membership (issue #177); the grant adds peer to `state.members`. + let grant = managed.dag().create_event( + &owner, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: crate::event::Permission::SendMessages, + }, + vec![], + 0, + ); + let grant_hash = grant.hash; + managed.insert_and_apply(grant).unwrap(); + + // Create peer's seq=1 event, depending on the grant so topological + // sort guarantees the grant applies first. let e1 = managed.dag().create_event( &peer, EventKind::SetProfile { display_name: "first".into(), }, - vec![], + vec![grant_hash], 0, ); diff --git a/crates/state/src/tests/permissions.rs b/crates/state/src/tests/permissions.rs index e33b40db..76f1bf76 100644 --- a/crates/state/src/tests/permissions.rs +++ b/crates/state/src/tests/permissions.rs @@ -1085,3 +1085,759 @@ fn set_permission_legacy_unknown_string_drops_silently() { "unknown legacy permission must apply as a no-op" ); } + +// ───── Membership gate for "unrestricted" event kinds ───────────────────── +// +// Issue #177 / SEC: `SetProfile`, `UpdateProfile`, `PinMessage`, +// `UnpinMessage` are documented as "any member can execute" but the +// handlers in `apply_mutation` did not actually enforce the membership +// predicate. Late-arriving events from a kicked or never-joined signer +// were silently applied, allowing post-kick state mutation. +// +// These tests pin down the membership gate at the apply boundary using +// the same pattern as `RotateChannelKey` (defense-in-depth even when +// `required_permission()` returns `None`). + +/// Helper: set up a server where `peer` was a member then was kicked. +/// +/// Returns a [`ManagedDag`] whose state has `peer` removed from +/// `members`, plus the admin and peer identities. +fn setup_kicked_peer() -> ( + crate::managed::ManagedDag, + Identity, + Identity, + EventHash, /* kick_proposal_hash */ +) { + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + // Promote peer to a member by granting SendMessages. + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 10, + ); + managed.insert_and_apply(grant).unwrap(); + assert!(managed.state().members.contains_key(&peer.endpoint_id())); + + // Admin is sole admin, so a Majority threshold (default) auto-applies + // their own self-vote of 1/1 > 0.5. + let kick = managed.dag().create_event( + &admin, + EventKind::Propose { + action: ProposedAction::KickMember { + peer_id: peer.endpoint_id(), + }, + }, + vec![], + 20, + ); + let kick_hash = kick.hash; + managed.insert_and_apply(kick).unwrap(); + assert!( + !managed.state().members.contains_key(&peer.endpoint_id()), + "peer must be kicked", + ); + + (managed, admin, peer, kick_hash) +} + +// SetProfile ----------------------------------------------------------------- + +#[test] +fn set_profile_by_member_is_applied() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 10, + ); + managed.insert_and_apply(grant).unwrap(); + + let set = managed.dag().create_event( + &peer, + EventKind::SetProfile { + display_name: "Peer".into(), + }, + vec![], + 20, + ); + let outcome = managed.insert_and_apply(set).unwrap(); + assert!( + matches!(outcome.apply_result, Some(ApplyResult::Applied)), + "member SetProfile must apply: {:?}", + outcome.apply_result, + ); + assert_eq!( + managed + .state() + .profiles + .get(&peer.endpoint_id()) + .map(|p| p.display_name.as_str()), + Some("Peer"), + ); +} + +#[test] +fn set_profile_by_kicked_member_is_rejected() { + use crate::materialize::ApplyResult; + + let (mut managed, _admin, peer, _kick) = setup_kicked_peer(); + + let set = managed.dag().create_event( + &peer, + EventKind::SetProfile { + display_name: "Sneaky".into(), + }, + vec![], + 30, + ); + let outcome = managed.insert_and_apply(set).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("kicked-member SetProfile must be rejected: {other:?}"), + } + // State must not reflect the kicked peer's profile change. + let dn = managed + .state() + .profiles + .get(&peer.endpoint_id()) + .map(|p| p.display_name.clone()) + .unwrap_or_default(); + assert_ne!(dn, "Sneaky", "kicked peer must not mutate profile state"); +} + +#[test] +fn set_profile_by_non_member_is_rejected() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let stranger = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let set = managed.dag().create_event( + &stranger, + EventKind::SetProfile { + display_name: "Stranger".into(), + }, + vec![], + 10, + ); + let outcome = managed.insert_and_apply(set).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("non-member SetProfile must be rejected: {other:?}"), + } + assert!( + !managed + .state() + .profiles + .contains_key(&stranger.endpoint_id()), + "non-member must not create profile entry", + ); +} + +// UpdateProfile -------------------------------------------------------------- + +#[test] +fn update_profile_by_member_is_applied() { + use crate::materialize::ApplyResult; + use crate::types::ProfileDelta; + + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 10, + ); + managed.insert_and_apply(grant).unwrap(); + + let upd = managed.dag().create_event( + &peer, + EventKind::UpdateProfile(Box::new(ProfileDelta { + display_name: None, + pronouns: Some(Some("they/them".into())), + bio: None, + tagline: None, + crest_pattern: None, + crest_color: None, + pinned: None, + elsewhere: None, + since: None, + })), + vec![], + 20, + ); + let outcome = managed.insert_and_apply(upd).unwrap(); + assert!( + matches!(outcome.apply_result, Some(ApplyResult::Applied)), + "member UpdateProfile must apply: {:?}", + outcome.apply_result, + ); + assert_eq!( + managed + .state() + .profiles + .get(&peer.endpoint_id()) + .and_then(|p| p.pronouns.as_deref()), + Some("they/them"), + ); +} + +#[test] +fn update_profile_by_kicked_member_is_rejected() { + use crate::materialize::ApplyResult; + use crate::types::ProfileDelta; + + let (mut managed, _admin, peer, _kick) = setup_kicked_peer(); + + let upd = managed.dag().create_event( + &peer, + EventKind::UpdateProfile(Box::new(ProfileDelta { + display_name: None, + pronouns: Some(Some("rogue".into())), + bio: None, + tagline: None, + crest_pattern: None, + crest_color: None, + pinned: None, + elsewhere: None, + since: None, + })), + vec![], + 30, + ); + let outcome = managed.insert_and_apply(upd).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("kicked-member UpdateProfile must be rejected: {other:?}"), + } + assert_ne!( + managed + .state() + .profiles + .get(&peer.endpoint_id()) + .and_then(|p| p.pronouns.as_deref()), + Some("rogue"), + "kicked peer must not mutate profile state", + ); +} + +#[test] +fn update_profile_by_non_member_is_rejected() { + use crate::materialize::ApplyResult; + use crate::types::ProfileDelta; + + let admin = Identity::generate(); + let stranger = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let upd = managed.dag().create_event( + &stranger, + EventKind::UpdateProfile(Box::new(ProfileDelta { + display_name: None, + pronouns: Some(Some("nope".into())), + bio: None, + tagline: None, + crest_pattern: None, + crest_color: None, + pinned: None, + elsewhere: None, + since: None, + })), + vec![], + 10, + ); + let outcome = managed.insert_and_apply(upd).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("non-member UpdateProfile must be rejected: {other:?}"), + } + assert!( + !managed + .state() + .profiles + .contains_key(&stranger.endpoint_id()), + "non-member must not create profile entry", + ); +} + +// PinMessage / UnpinMessage -------------------------------------------------- + +/// Helper: build a server with one channel and one message authored by +/// `peer` (a member). Returns the message hash, useful for Pin/Unpin +/// scenarios. +fn setup_channel_with_peer_message( + managed: &mut crate::managed::ManagedDag, + admin: &Identity, + peer: &Identity, + channel_id: &str, +) -> EventHash { + let create = managed.dag().create_event( + admin, + EventKind::CreateChannel { + name: "general".into(), + channel_id: channel_id.into(), + kind: crate::types::ChannelKind::Text, + ephemeral: None, + }, + vec![], + 5, + ); + managed.insert_and_apply(create).unwrap(); + + let msg = managed.dag().create_event( + peer, + EventKind::Message { + channel_id: channel_id.into(), + body: "hi".into(), + reply_to: None, + }, + vec![], + 15, + ); + let h = msg.hash; + managed.insert_and_apply(msg).unwrap(); + h +} + +#[test] +fn pin_message_by_member_is_applied() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 1, + ); + managed.insert_and_apply(grant).unwrap(); + + let msg_hash = setup_channel_with_peer_message(&mut managed, &admin, &peer, "ch1"); + + let pin = managed.dag().create_event( + &peer, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 20, + ); + let outcome = managed.insert_and_apply(pin).unwrap(); + assert!( + matches!(outcome.apply_result, Some(ApplyResult::Applied)), + "member PinMessage must apply: {:?}", + outcome.apply_result, + ); + assert!(managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash)); +} + +#[test] +fn pin_message_by_kicked_member_is_rejected() { + use crate::materialize::ApplyResult; + + // Set up: peer is member, posts a message, then is kicked. Late + // PinMessage from peer must be rejected. + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 1, + ); + managed.insert_and_apply(grant).unwrap(); + + let msg_hash = setup_channel_with_peer_message(&mut managed, &admin, &peer, "ch1"); + + let kick = managed.dag().create_event( + &admin, + EventKind::Propose { + action: ProposedAction::KickMember { + peer_id: peer.endpoint_id(), + }, + }, + vec![], + 25, + ); + managed.insert_and_apply(kick).unwrap(); + assert!(!managed.state().members.contains_key(&peer.endpoint_id())); + + let pin = managed.dag().create_event( + &peer, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 30, + ); + let outcome = managed.insert_and_apply(pin).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("kicked-member PinMessage must be rejected: {other:?}"), + } + assert!( + !managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash), + "kicked peer must not mutate pinned_messages", + ); +} + +#[test] +fn pin_message_by_non_member_is_rejected() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let stranger = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let create = managed.dag().create_event( + &admin, + EventKind::CreateChannel { + name: "general".into(), + channel_id: "ch1".into(), + kind: crate::types::ChannelKind::Text, + ephemeral: None, + }, + vec![], + 5, + ); + managed.insert_and_apply(create).unwrap(); + + // Use admin's own message as the target — stranger never joined, + // so they have no events of their own. Authority gate should fire + // before message-existence is even checked. + let msg = managed.dag().create_event( + &admin, + EventKind::Message { + channel_id: "ch1".into(), + body: "hello".into(), + reply_to: None, + }, + vec![], + 10, + ); + let msg_hash = msg.hash; + managed.insert_and_apply(msg).unwrap(); + + let pin = managed.dag().create_event( + &stranger, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 20, + ); + let outcome = managed.insert_and_apply(pin).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("non-member PinMessage must be rejected: {other:?}"), + } + assert!( + !managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash), + "non-member must not mutate pinned_messages", + ); +} + +#[test] +fn unpin_message_by_member_is_applied() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 1, + ); + managed.insert_and_apply(grant).unwrap(); + + let msg_hash = setup_channel_with_peer_message(&mut managed, &admin, &peer, "ch1"); + + // Admin pins, then peer unpins. + let pin = managed.dag().create_event( + &admin, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 20, + ); + managed.insert_and_apply(pin).unwrap(); + + let unpin = managed.dag().create_event( + &peer, + EventKind::UnpinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 30, + ); + let outcome = managed.insert_and_apply(unpin).unwrap(); + assert!( + matches!(outcome.apply_result, Some(ApplyResult::Applied)), + "member UnpinMessage must apply: {:?}", + outcome.apply_result, + ); + assert!(!managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash)); +} + +#[test] +fn unpin_message_by_kicked_member_is_rejected() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let peer = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let grant = managed.dag().create_event( + &admin, + EventKind::GrantPermission { + peer_id: peer.endpoint_id(), + permission: Permission::SendMessages, + }, + vec![], + 1, + ); + managed.insert_and_apply(grant).unwrap(); + + let msg_hash = setup_channel_with_peer_message(&mut managed, &admin, &peer, "ch1"); + + // Admin pins. + let pin = managed.dag().create_event( + &admin, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 20, + ); + managed.insert_and_apply(pin).unwrap(); + assert!(managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash)); + + // Kick peer. + let kick = managed.dag().create_event( + &admin, + EventKind::Propose { + action: ProposedAction::KickMember { + peer_id: peer.endpoint_id(), + }, + }, + vec![], + 25, + ); + managed.insert_and_apply(kick).unwrap(); + assert!(!managed.state().members.contains_key(&peer.endpoint_id())); + + // Late UnpinMessage from kicked peer must be rejected. + let unpin = managed.dag().create_event( + &peer, + EventKind::UnpinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 30, + ); + let outcome = managed.insert_and_apply(unpin).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("kicked-member UnpinMessage must be rejected: {other:?}"), + } + assert!( + managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash), + "kicked peer must not unpin", + ); +} + +#[test] +fn unpin_message_by_non_member_is_rejected() { + use crate::materialize::ApplyResult; + + let admin = Identity::generate(); + let stranger = Identity::generate(); + let mut managed = crate::managed::ManagedDag::new(&admin, "S", 5000).unwrap(); + + let create = managed.dag().create_event( + &admin, + EventKind::CreateChannel { + name: "general".into(), + channel_id: "ch1".into(), + kind: crate::types::ChannelKind::Text, + ephemeral: None, + }, + vec![], + 5, + ); + managed.insert_and_apply(create).unwrap(); + + let msg = managed.dag().create_event( + &admin, + EventKind::Message { + channel_id: "ch1".into(), + body: "hello".into(), + reply_to: None, + }, + vec![], + 10, + ); + let msg_hash = msg.hash; + managed.insert_and_apply(msg).unwrap(); + + // Admin pins. + let pin = managed.dag().create_event( + &admin, + EventKind::PinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 15, + ); + managed.insert_and_apply(pin).unwrap(); + + let unpin = managed.dag().create_event( + &stranger, + EventKind::UnpinMessage { + channel_id: "ch1".into(), + message_id: msg_hash, + }, + vec![], + 20, + ); + let outcome = managed.insert_and_apply(unpin).unwrap(); + match &outcome.apply_result { + Some(ApplyResult::Rejected(reason)) => { + assert!( + reason.contains("is not a member"), + "rejection reason must mention non-membership: {reason}", + ); + } + other => panic!("non-member UnpinMessage must be rejected: {other:?}"), + } + assert!( + managed + .state() + .channels + .get("ch1") + .unwrap() + .pinned_messages + .contains(&msg_hash), + "non-member must not unpin", + ); +} diff --git a/crates/state/src/tests/stress.rs b/crates/state/src/tests/stress.rs index 04a08355..be4676c0 100644 --- a/crates/state/src/tests/stress.rs +++ b/crates/state/src/tests/stress.rs @@ -273,8 +273,28 @@ fn stress_100_authors_deterministic_profiles() { let authors: Vec = (0..9).map(|_| Identity::generate()).collect(); + // Grant SendMessages to every author so they become members. + // SetProfile is gated on membership (issue #177); each SetProfile + // must causally depend on the author's own grant so topological + // sort applies the grant first. + let mut grant_hashes: std::collections::BTreeMap<_, _> = std::collections::BTreeMap::new(); for author in &authors { - let deps = vec![*dag.head(&admin.endpoint_id()).unwrap()]; + let e = dag.create_event( + &admin, + EventKind::GrantPermission { + peer_id: author.endpoint_id(), + permission: crate::event::Permission::SendMessages, + }, + vec![], + 0, + ); + grant_hashes.insert(author.endpoint_id(), e.hash); + dag.insert(e).unwrap(); + } + + for author in &authors { + let mut deps = vec![*dag.head(&admin.endpoint_id()).unwrap()]; + deps.push(grant_hashes[&author.endpoint_id()]); let e = dag.create_event( author, EventKind::SetProfile { diff --git a/docs/specs/2026-04-12-state-authority-and-mutations.md b/docs/specs/2026-04-12-state-authority-and-mutations.md index d53989cc..1f6d90f7 100644 --- a/docs/specs/2026-04-12-state-authority-and-mutations.md +++ b/docs/specs/2026-04-12-state-authority-and-mutations.md @@ -76,7 +76,8 @@ cases where a remote event is rejected: | **Governance (vote)** | `Propose`, `Vote` | `is_admin()` — only admins may propose or vote. Actions auto-apply when vote threshold is met. | | **Admin-only** | `GrantPermission`, `RevokePermission`, `RenameServer`, `SetServerDescription` | `is_admin()` — any single admin can execute these directly. | | **Permission-gated** | `Message`, `EditMessage`, `DeleteMessage`, `Reaction` → `SendMessages`; `CreateChannel`, `DeleteChannel`, `RenameChannel`, `RotateChannelKey` → `ManageChannels`; `CreateRole`, `DeleteRole`, `SetPermission`, `AssignRole` → `ManageRoles` | `has_permission()` — admins pass implicitly; non-admins need an explicit grant. | -| **Unrestricted** | `SetProfile`, `UpdateProfile`, `PinMessage`, `UnpinMessage`, `MuteChannel`, `MuteGrove` | No check — any member can execute. | +| **Member-only (server state)** | `SetProfile`, `UpdateProfile`, `PinMessage`, `UnpinMessage` | `state.members.contains_key(&author)` — any current member can execute. `required_permission()` returns `None`, so the membership gate lives in each handler in `apply_mutation` (defense-in-depth, see issue #177). Note: this is "any current member" — distinct from "any signer" with no gate at all. | +| **Per-identity preference (no gate)** | `MuteChannel`, `MuteGrove` | No check — these are personal preferences, not shared server state. Preferences persist across kicks. | | **Genesis** | `CreateServer` | No-op on replay; the genesis author becomes the sole initial admin. | Admin status is tracked in `ServerState.admins` and is **not** a variant @@ -102,10 +103,23 @@ comment so reviewers notice when a new variant is missing: admin block - `RenameServer`, `SetServerDescription` — admin-only, checked in the admin block -- `SetProfile` — intentionally unrestricted -- `UpdateProfile` — intentionally unrestricted (any member; self-authorship enforced structurally) -- `PinMessage`, `UnpinMessage` — intentionally unrestricted +- `SetProfile` — any current member; membership gate enforced in + `apply_mutation` (not in `required_permission()`). See issue #177. +- `UpdateProfile` — any current member; membership gate enforced in + `apply_mutation`. Self-authorship is enforced structurally (only the + author's own profile is mutated). See issue #177. +- `PinMessage`, `UnpinMessage` — any current member; membership gate + enforced in `apply_mutation` (not in `required_permission()`). See + issue #177. - `MuteChannel`, `MuteGrove` — per-identity preference, never gated + (preferences are not shared server state and survive a kick) + +**"Intentionally unrestricted" still requires membership.** The +membership gate (`state.members.contains_key(&event.author)`) lives in +the per-handler block inside `apply_mutation`, not in +`required_permission()`. This is defense-in-depth: even if a future +refactor changes the permission table, the handler-local gate keeps +non-members from mutating server state. If a variant is not in this list and not in a `required_permission()` arm, it is a bug.