diff --git a/CHANGELOG.md b/CHANGELOG.md index 811b48b..0c81410 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Correct kind `30618` repository-state authority for a view explicitly + selected through a former maintainer's coordinate. Previously, only that + selected view could continue accepting state signed by the former + maintainer after the lead stopped assigning them; this did not restore the + former maintainer's authority over views selected through the lead or any + other maintainer. Selected views now resolve authority through valid active + `M` records to the terminal maintainer. Ended selected authors, malformed + role histories, missing or ambiguous lead targets, and lead cycles fail + closed. + ## [3.0.0] - 2026-08-20 ### Summary diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index c08ee9f..f0f9542 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -296,16 +296,18 @@ pub struct RepositoryState { ... } Announcements carry maintainer listings in two formats: - **Indexed role tags** (`M` lead, `m` co-maintainer). A tag may record - history as alternating start/end timestamps; it is currently active when - it has fewer than four elements or an odd number of elements. Ended - entries are ignored entirely - the history is only used to conclude a - role has *ended*, never to grant authority for a past period. The lead / - co-maintainer distinction carries no meaning for this service. A pubkey - may appear in multiple role tags: one of each letter records a role - transition, and out-of-spec duplicates under the same letter are - tolerated. Histories are consolidated - the pubkey is a maintainer while - any `M`/`m` entry is active. An author who appears in no role tag is - implicitly a maintainer for the repository's entire history. + history as alternating numeric start/end timestamps; `defer` is valid only + as the final end boundary. It is currently active when it has no boundaries + or its final valid boundary is a start. Ended, deferred, and malformed + entries grant no authority - the history is only used to conclude a role + has *ended*, never to grant authority for a past period. Both `M` and `m` + grant maintainer authority, while active `M` records also form the lead + path that roots the selected repository view. A pubkey may appear in + multiple role tags: one of each letter records a role transition, and + out-of-spec duplicates under the same letter are tolerated. Histories are + consolidated - the pubkey is a maintainer while any valid `M`/`m` entry is + active. An author who appears in no role tag is implicitly a maintainer for + the repository's entire history. - **Deprecated `maintainers` tag** (fallback). Ignored when `M`/`m` tags are present. Without any listing tags the author is the sole maintainer. A `u` (subordinate fork) tag has no effect on maintainership. @@ -331,9 +333,12 @@ Authorization follows a reciprocal membership rule, computed per owner by author: an ended `M`/`m` self-entry means the pubkey has left, which takes precedence over assignments in other announcements. An author who appears in no role tag implicitly asserts maintainership. -- The owner is always a confirmed maintainer of their own repository: - announcing a repository in their namespace is what creates it on this - service. +- A valid active `M` path is followed to a terminal self-`M` before the + reciprocal fixpoint is seeded. Without an active `M`, the selected author + roots a legacy or deliberately leadless view while their own role is active. + Missing, ambiguous, and cyclic explicit paths grant no state authority, and + a former maintainer's forwarding coordinate does not restore that signer to + the confirmed set. #### [`policy/state.rs`](src/nostr/policy/state.rs) - State Event Authorization diff --git a/docs/explanation/decisions.md b/docs/explanation/decisions.md index d6874f2..b39e8b7 100644 --- a/docs/explanation/decisions.md +++ b/docs/explanation/decisions.md @@ -215,9 +215,11 @@ NIP-34 maintainers model refined in nips commit `781590b`). until its own announcement for the same identifier lists back an existing confirmed maintainer. - Only confirmed maintainers publish authoritative repository state. -- The owner is always a confirmed maintainer of their own repository: - announcing a repository in their namespace is what creates it on this - service. +- Authority is rooted at the terminal lead reached through valid active `M` + records. Without an active `M`, the selected author roots a legacy or + deliberately leadless view only while their own role is active. Once an + explicit path is followed, a missing target, multiple active targets, or a + cycle grants no repository-state authority. ### Implementation choices @@ -240,15 +242,16 @@ NIP-34 maintainers model refined in nips commit `781590b`). - `M` (lead) and `m` (co-maintainer) tags are the primary maintainer listing (per the model clarified in nips commit `986edd1`); when present the deprecated `maintainers` tag is ignored per NIP-34 (and parsed as - empty). The role distinction carries no meaning for this service: both - collapse into one maintainer set. -- Role tags may record history as alternating start/end timestamps; a tag - is currently active when it has fewer than four elements or an odd number - of elements. Ended entries are ignored entirely: role history is only - used to conclude that a pubkey is *no longer* a maintainer, never to - grant time-scoped retroactive authority over historic events. The - NIP's owner-first precedence for conflicting past-role records is - therefore unused. + empty). Both grant equal maintainer authority, while active `M` records + additionally define the lead path used to root that authority. +- Role tags may record history as alternating numeric start/end timestamps; + `defer` is valid only as the final end boundary. A valid tag is currently + active when it has no boundaries or its final boundary is a start. Ended, + deferred, and malformed entries grant no authority: role history is only + used to conclude that a pubkey is *no longer* a maintainer, never to grant + time-scoped retroactive authority over historic events. The NIP's + owner-first precedence for conflicting past-role records is therefore + unused. - A pubkey may appear in multiple role tags: one of each letter records a role transition per the NIP, and out-of-spec duplicates under the same letter are consolidated rather than rejected - rejecting them would drop @@ -284,4 +287,3 @@ the `M`/`m` assignments they might carry (the NIP says role combinations beyond self-plus-lead SHOULD be avoided unless the author is `M`), and moderator membership gets no reciprocal-confirmation treatment. Both only matter if status-event authority is ever enforced. - diff --git a/src/git/authorization.rs b/src/git/authorization.rs index 43c40b9..bc16e47 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -316,7 +316,8 @@ pub fn pubkey_authorised_for_repo_owners( #[derive(Debug, Default)] pub struct RepoMembership { /// Pubkeys whose repository state events are authoritative for this - /// owner's repository: the owner plus every confirmed maintainer. + /// owner's selected repository view: the resolved lead or selected + /// leadless/legacy root plus every confirmed maintainer. pub state_maintainers: HashSet, /// Pubkeys currently listed as maintainers whose own announcement does /// not yet list back a confirmed maintainer. Their announcements must @@ -337,9 +338,12 @@ pub struct RepoMembership { /// assignments in other announcements. An author who appears in no role tag /// implicitly asserts maintainership for the repository's entire history. /// -/// The owner is always a confirmed maintainer of their own repository: -/// announcing a repository in their namespace is what creates it on this -/// service. +/// An explicit active `M` path is resolved before the fixpoint is seeded. A +/// self-`M` terminates at the lead; an announcement without an active `M` +/// seeds its author only when resolution has not already followed an explicit +/// path. Missing, ambiguous, or cyclic explicit paths grant no authority. +/// This prevents a removed maintainer's forwarding coordinate from restoring +/// that signer to the confirmed set. pub fn compute_membership( announcements: &[RepositoryAnnouncement], owner: &str, @@ -350,11 +354,35 @@ pub fn compute_membership( .iter() .find(|a| a.event.pubkey.to_hex() == pubkey && a.identifier == identifier) }; - if find(owner).is_none() { - return RepoMembership::default(); + // Resolve the selected coordinate's explicit lead path before seeding + // reciprocal membership. With no active M on the selected announcement, + // this is a legacy or deliberately leadless view rooted at the selected + // author. Once an explicit path is followed, only an active self-M may + // terminate it; incomplete, ambiguous, and cyclic paths fail closed. + let mut root = owner.to_string(); + let mut followed_explicit_lead = false; + let mut visited = HashSet::new(); + loop { + if !visited.insert(root.clone()) { + return RepoMembership::default(); + } + let Some(announcement) = find(&root) else { + return RepoMembership::default(); + }; + let active_leads = announcement.active_leads(); + match active_leads.as_slice() { + [] if !followed_explicit_lead && announcement.author_role_active() => break, + [] => return RepoMembership::default(), + [lead] if lead == &root && announcement.author_role_active() => break, + [lead] if lead != &root => { + root = lead.clone(); + followed_explicit_lead = true; + } + _ => return RepoMembership::default(), + } } - let mut confirmed: HashSet = HashSet::from([owner.to_string()]); + let mut confirmed: HashSet = HashSet::from([root]); // Fixpoint: keep confirming listed pubkeys whose own announcement lists // back an already-confirmed maintainer. The confirmed set only grows, so // the loop terminates. @@ -1456,6 +1484,131 @@ mod tests { let membership = compute_membership(&announcements, &hex(&alice), identifier); assert!(membership.state_maintainers.contains(&hex(&bob))); + + let selected_from_bob = compute_membership(&announcements, &hex(&bob), identifier); + assert_eq!( + selected_from_bob.state_maintainers, + membership.state_maintainers + ); + } + + #[test] + fn test_ended_selected_author_has_no_state_authority() { + let alice = create_test_keys(); + let identifier = "test-repo"; + + let announcements = vec![parse(create_role_announcement( + &alice, + identifier, + &[("m", vec![hex(&alice), "100".to_string(), "200".to_string()])], + ))]; + + let membership = compute_membership(&announcements, &hex(&alice), identifier); + assert!(membership.state_maintainers.is_empty()); + assert!(membership.invited.is_empty()); + } + + #[test] + fn test_removed_selected_maintainer_resolves_lead_without_regaining_authority() { + let alice = create_test_keys(); + let bob = create_test_keys(); + let identifier = "test-repo"; + + // Alice no longer assigns Bob. Bob's still-active acceptance and M + // redirect keep the coordinate attached to Alice, but do not seed Bob + // as a maintainer of a separate same-identifier repository. + let announcements = vec![ + parse(create_role_announcement( + &alice, + identifier, + &[("M", vec![hex(&alice)])], + )), + parse(create_role_announcement( + &bob, + identifier, + &[("M", vec![hex(&alice)]), ("m", vec![hex(&bob)])], + )), + ]; + + let membership = compute_membership(&announcements, &hex(&bob), identifier); + assert_eq!(membership.state_maintainers, HashSet::from([hex(&alice)])); + + let repo_data = RepositoryData { + announcements, + states: Vec::new(), + }; + assert!(pubkey_authorised_for_repo_owners(&bob.public_key(), &repo_data).is_empty()); + assert_eq!( + pubkey_authorised_for_repo_owners(&alice.public_key(), &repo_data) + .into_iter() + .collect::>(), + HashSet::from([hex(&alice), hex(&bob)]) + ); + } + + #[test] + fn test_incomplete_ambiguous_or_cyclic_lead_path_grants_no_state_authority() { + let alice = create_test_keys(); + let bob = create_test_keys(); + let identifier = "test-repo"; + + let bob_announcement = parse(create_role_announcement( + &bob, + identifier, + &[("M", vec![hex(&alice)]), ("m", vec![hex(&bob)])], + )); + let missing_lead = compute_membership( + std::slice::from_ref(&bob_announcement), + &hex(&bob), + identifier, + ); + assert!(missing_lead.state_maintainers.is_empty()); + + let ambiguous = parse(create_role_announcement( + &alice, + identifier, + &[("M", vec![hex(&alice)]), ("M", vec![hex(&bob)])], + )); + let ambiguous_lead = compute_membership(&[ambiguous], &hex(&alice), identifier); + assert!(ambiguous_lead.state_maintainers.is_empty()); + + let alice_announcement = parse(create_role_announcement( + &alice, + identifier, + &[("M", vec![hex(&bob)]), ("m", vec![hex(&alice)])], + )); + let cycle = compute_membership( + &[alice_announcement, bob_announcement], + &hex(&alice), + identifier, + ); + assert!(cycle.state_maintainers.is_empty()); + } + + #[test] + fn test_explicit_leadless_roles_still_root_selected_authority() { + let alice = create_test_keys(); + let bob = create_test_keys(); + let identifier = "test-repo"; + + let announcements = vec![ + parse(create_role_announcement( + &alice, + identifier, + &[("m", vec![hex(&alice)]), ("m", vec![hex(&bob)])], + )), + parse(create_role_announcement( + &bob, + identifier, + &[("m", vec![hex(&bob)]), ("m", vec![hex(&alice)])], + )), + ]; + + let membership = compute_membership(&announcements, &hex(&alice), identifier); + assert_eq!( + membership.state_maintainers, + HashSet::from([hex(&alice), hex(&bob)]) + ); } #[test] diff --git a/src/nostr/events.rs b/src/nostr/events.rs index 4e35448..8981a53 100644 --- a/src/nostr/events.rs +++ b/src/nostr/events.rs @@ -7,7 +7,32 @@ /// - NIP-34: https://nips.nostr.com/34 /// - GRASP-01: https://gitworkshop.dev/danconwaydev.com/grasp/01.md use anyhow::{anyhow, Result}; -use nostr_sdk::prelude::{Event, Kind, ToBech32}; +use nostr_sdk::prelude::{Event, Kind, PublicKey, ToBech32}; + +/// Whether a NIP-34 indexed role record is syntactically valid and active. +/// +/// Values after the pubkey alternate between numeric start and end +/// boundaries. `defer` is valid only as the final end boundary and is never +/// active. Malformed history must not grant repository authority. +fn role_entry_is_active(slice: &[String]) -> bool { + if slice.len() < 2 { + return false; + } + + let boundaries = &slice[2..]; + for (index, value) in boundaries.iter().enumerate() { + if value == "defer" { + let is_final_end = index % 2 == 1 && index + 1 == boundaries.len(); + if !is_final_end { + return false; + } + } else if value.parse::().is_err() { + return false; + } + } + + boundaries.is_empty() || boundaries.len() % 2 == 1 +} // NOTE: Using rust-nostr Kind variants instead of hardcoded constants: // - KIND_REPOSITORY_ANNOUNCEMENT -> Kind::GitRepoAnnouncement (30617) @@ -34,11 +59,11 @@ pub struct RepositoryAnnouncement { /// `None` when the announcement contains no role tags. Moderator (`o`) /// entries never contribute: moderators are not maintainers. /// - /// The lead (`M`) / co-maintainer (`m`) distinction carries no meaning - /// for this service, so both collapse into one maintainer set. Entries - /// whose role history shows the role has ended are excluded entirely: an - /// ended role means the pubkey is no longer a maintainer, and historic - /// listing never grants authority for a past period. + /// Both `M` and `m` grant maintainer authority, so they collapse into one + /// maintainer set. Active `M` entries additionally drive lead resolution + /// through [`Self::active_leads`]. Entries whose role history is malformed + /// or ended are excluded entirely: historic listing never grants + /// authority for a past period. pub role_maintainers: Option>, /// Whether any role tag names the author, active or ended. Per NIP-34 an /// author without a self-entry is implicitly a maintainer for the @@ -127,13 +152,14 @@ impl RepositoryAnnouncement { // Extract maintainer pubkeys from NIP-34 `M`/`m`/`o` role tags // Format: ["M"|"m"|"o", "", "", "", ...] // A role tag lists a pubkey followed by optional alternating start/end - // history timestamps; it is currently active when it has fewer than - // four elements or an odd number of elements. Ended entries are - // ignored entirely. A pubkey may appear in multiple role tags: one - // tag of each letter records transitions between roles, and - // out-of-spec duplicates under the same letter are tolerated. The - // histories are consolidated - the pubkey is a maintainer while any - // of its `M`/`m` entries is active. + // history timestamps; it is currently active when it has no history + // or its final valid boundary is a start. `defer` is accepted only as + // the final end boundary. Ended and malformed entries are ignored for + // authority. A pubkey may appear in multiple role tags: one tag of + // each letter records transitions between roles, and out-of-spec + // duplicates under the same letter are tolerated. The histories are + // consolidated - the pubkey is a maintainer while any of its valid + // `M`/`m` entries is active. // // The moderator role (`o`) grants no maintainership: moderators are // only empowered to have status events (kinds 1630-1633) treated as @@ -144,7 +170,6 @@ impl RepositoryAnnouncement { // counts as a role tag: its presence suppresses the deprecated // `maintainers` fallback, and a self-`o` entry is a self-role, so // the author is not implicitly a maintainer. - let author_hex = event.pubkey.to_hex(); let mut role_tags_present = false; let mut author_has_role_entry = false; let mut role_active: Vec = Vec::new(); @@ -158,15 +183,21 @@ impl RepositoryAnnouncement { let Some(pubkey) = slice.get(1).filter(|value| !value.is_empty()) else { continue; }; - if *pubkey == author_hex { + let parsed_pubkey = PublicKey::from_hex(pubkey).ok(); + if parsed_pubkey.as_ref() == Some(&event.pubkey) { author_has_role_entry = true; } if kind == "o" { continue; } - let active = slice.len() < 4 || slice.len() % 2 == 1; - if active && !role_active.contains(pubkey) { - role_active.push(pubkey.clone()); + if role_entry_is_active(slice) { + let Some(parsed_pubkey) = parsed_pubkey else { + continue; + }; + let pubkey = parsed_pubkey.to_hex(); + if !role_active.contains(&pubkey) { + role_active.push(pubkey); + } } } let role_maintainers = role_tags_present.then_some(role_active); @@ -226,6 +257,32 @@ impl RepositoryAnnouncement { listed } + /// Distinct targets of syntactically valid, currently-active `M` records. + /// + /// An active `M` is both a maintainer edge and an explicit lead pointer. + /// Duplicate records for one target are consolidated; distinct targets + /// remain visible so authority resolution can fail closed on ambiguity. + pub fn active_leads(&self) -> Vec { + let mut leads = Vec::new(); + for tag in self.event.tags.iter() { + let slice = tag.as_slice(); + if slice.first().map(String::as_str) != Some("M") || !role_entry_is_active(slice) { + continue; + } + let Some(pubkey) = slice + .get(1) + .and_then(|value| PublicKey::from_hex(value).ok()) + else { + continue; + }; + let pubkey = pubkey.to_hex(); + if !leads.contains(&pubkey) { + leads.push(pubkey); + } + } + leads + } + /// Whether this announcement asserts an active maintainer role for its /// own author. /// @@ -928,20 +985,22 @@ mod tests { } #[test] - fn test_role_tag_activeness_by_element_count() { + fn test_role_tag_activeness_requires_valid_history() { let keys = create_test_keys(); let author = keys.public_key().to_hex(); let other = create_test_keys().public_key().to_hex(); // (history values after the pubkey, expected active) - // Tag element counts: 2 -> active, 3 -> active, 4 -> ended, - // 5 -> active, 6 -> ended. let cases: Vec<(Vec<&str>, bool)> = vec![ (vec![], true), (vec!["100"], true), (vec!["100", "200"], false), (vec!["100", "200", "300"], true), (vec!["100", "200", "300", "400"], false), + (vec!["100", "defer"], false), + (vec!["not-a-timestamp"], false), + (vec!["defer"], false), + (vec!["100", "defer", "300"], false), ]; for (history, expected_active) in cases { @@ -957,6 +1016,28 @@ mod tests { } } + #[test] + fn test_active_leads_include_only_valid_distinct_targets() { + let keys = create_test_keys(); + let first = create_test_keys().public_key().to_hex(); + let second = create_test_keys().public_key().to_hex(); + let ended = create_test_keys().public_key().to_hex(); + let malformed = create_test_keys().public_key().to_hex(); + + let announcement = role_announcement( + &keys, + vec![ + ("M", vec![first.clone()]), + ("M", vec![first.clone(), "100".to_string()]), + ("M", vec![second.clone()]), + ("M", vec![ended, "100".to_string(), "200".to_string()]), + ("M", vec![malformed, "not-a-timestamp".to_string()]), + ], + ); + + assert_eq!(announcement.active_leads(), vec![first, second]); + } + #[test] fn test_lead_and_co_maintainer_roles_are_equivalent() { let keys = create_test_keys(); diff --git a/tests/state_authorization.rs b/tests/state_authorization.rs index f6abffd..47fdb9f 100644 --- a/tests/state_authorization.rs +++ b/tests/state_authorization.rs @@ -297,6 +297,81 @@ async fn state_event_rejected_as_unauthorized(client: &Client, keys: &Keys) -> b } } +#[tokio::test] +async fn test_reject_state_from_announcement_author_with_ended_self_role() { + let relay = TestRelay::start().await; + let owner_keys = Keys::generate(); + + let announcement = EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags([ + Tag::custom("d", ["test-repo"]), + Tag::custom("clone", [format!("https://{}/test.git", relay.domain())]), + Tag::custom("relays", [relay.url()]), + Tag::custom( + "m", + [ + owner_keys.public_key().to_hex(), + "0".to_string(), + "1700000000".to_string(), + ], + ), + ]) + .finalize(&owner_keys) + .unwrap(); + + let client = Client::default(); + client.add_relay(relay.url()).await.unwrap(); + client.connect().await; + client.send_event(&announcement).await.unwrap(); + + assert!( + state_event_rejected_as_unauthorized(&client, &owner_keys).await, + "an ended selected author's state event must be rejected as unauthorized" + ); + + relay.stop().await; +} + +#[tokio::test] +async fn test_reject_state_from_unassigned_forwarding_author() { + let relay = TestRelay::start().await; + let lead_keys = Keys::generate(); + let former_keys = Keys::generate(); + + let lead_announcement = EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags([ + Tag::custom("d", ["test-repo"]), + Tag::custom("clone", [format!("https://{}/test.git", relay.domain())]), + Tag::custom("relays", [relay.url()]), + Tag::custom("M", [lead_keys.public_key().to_hex()]), + ]) + .finalize(&lead_keys) + .unwrap(); + let former_announcement = EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags([ + Tag::custom("d", ["test-repo"]), + Tag::custom("clone", [format!("https://{}/test.git", relay.domain())]), + Tag::custom("relays", [relay.url()]), + Tag::custom("M", [lead_keys.public_key().to_hex()]), + Tag::custom("m", [former_keys.public_key().to_hex()]), + ]) + .finalize(&former_keys) + .unwrap(); + + let client = Client::default(); + client.add_relay(relay.url()).await.unwrap(); + client.connect().await; + client.send_event(&lead_announcement).await.unwrap(); + client.send_event(&former_announcement).await.unwrap(); + + assert!( + state_event_rejected_as_unauthorized(&client, &former_keys).await, + "a forwarding signer not assigned by the lead must not regain state authority" + ); + + relay.stop().await; +} + #[tokio::test] async fn test_reject_state_from_invited_maintainer() { let relay = TestRelay::start().await;