diff --git a/docs/explanation/purgatory-design.md b/docs/explanation/purgatory-design.md index f4bf6df..8e7d75c 100644 --- a/docs/explanation/purgatory-design.md +++ b/docs/explanation/purgatory-design.md @@ -102,8 +102,6 @@ For PR events, **either side can arrive first**: Placeholders are identified by `PrPurgatoryEntry.event == None`. -**GRASP-06 scoped placeholders:** A push to `/prs//.git` creates a *scoped* placeholder that additionally records `(submitter, identifier)` from the URL. When the PR event arrives, its signer and a-tag d-value are cross-checked against the scope. An un-scoped placeholder (created by a standard-endpoint push) can be upgraded to a scoped one via `try_upgrade_to_scoped`, which performs the check-and-modify atomically under a single DashMap shard lock — preventing a TOCTOU race where two concurrent `/prs/` pushes for the same `event_id` could otherwise both see "un-scoped" and the second write would silently overwrite the first. - ### 5. Authorization During Push (Not After) **Critical for avoiding deadlock:** Authorization checks **both database and purgatory** during push validation. @@ -229,15 +227,10 @@ pub struct PrPurgatoryEntry { /// Expiry deadline (30 min from creation) pub expires_at: Instant, - - /// GRASP-06 /prs/ scope, if the placeholder was created by a /prs/ push. - /// When set, the arriving PR event must have signer == submitter and - /// one of its a-tag d-values == identifier. - pub prs_scope: Option, } ``` -**Key:** `event: None` indicates a placeholder (git-data-first scenario). `prs_scope: Some(…)` means the placeholder was created by a `/prs/` push and carries the URL's `(submitter, identifier)` binding. +**Key:** `event: None` indicates a placeholder (git-data-first scenario). ### Purgatory Stores @@ -740,33 +733,9 @@ impl Purgatory { /// Automatically enqueues for sync with 3min delay pub fn add_pr(&self, event: Event, event_id: String, commit: String); - /// Add a PR placeholder (git-data-first scenario, standard endpoint) + /// Add a PR placeholder (git-data-first scenario) pub fn add_pr_placeholder(&self, event_id: String, commit: String); - /// Add a scoped PR placeholder (git-data-first scenario, /prs/ endpoint) - pub fn add_prs_pr_placeholder( - &self, - event_id: String, - commit: String, - submitter: PublicKey, - identifier: String, - ); - - /// Atomically upgrade an un-scoped placeholder to a scoped one. - /// - /// Used by the /prs/ post-push path (edge case B2) when a standard-endpoint - /// push created an un-scoped placeholder before the /prs/ push arrived. - /// The check-and-modify happen under a single DashMap shard lock, so two - /// concurrent /prs/ pushes for the same event_id cannot both win the upgrade. - /// Returns true if the upgrade was applied, false otherwise. - pub fn try_upgrade_to_scoped( - &self, - event_id: &str, - commit: String, - submitter: PublicKey, - identifier: String, - ) -> bool; - /// Find state events waiting for an identifier pub fn find_state(&self, identifier: &str) -> Vec; diff --git a/src/grasp06/receive.rs b/src/grasp06/receive.rs index 974cfb3..4476765 100644 --- a/src/grasp06/receive.rs +++ b/src/grasp06/receive.rs @@ -575,16 +575,19 @@ async fn post_push_validate( // PrEventPolicy::git_data_check, find commit Y in the /prs/ // repo, and mirror it (overwriting the incorrect ref) into // every matching announced repo. - if purgatory.try_upgrade_to_scoped( - event_id_hex, - pushed_commit.to_string(), - *prs_constraints.submitter, - prs_constraints.identifier.to_string(), - ) { - debug!( - "/prs/ post-push: upgraded un-scoped placeholder to scoped for {} (commit {})", - ref_name, pushed_commit - ); + if let Some(entry) = purgatory.find_pr(event_id_hex) { + if entry.event.is_none() && entry.prs_scope.is_none() { + purgatory.add_prs_pr_placeholder( + event_id_hex.to_string(), + pushed_commit.to_string(), + *prs_constraints.submitter, + prs_constraints.identifier.to_string(), + ); + debug!( + "/prs/ post-push: upgraded un-scoped placeholder to scoped for {} (commit {})", + ref_name, pushed_commit + ); + } } } NostrRefPreValidation::Unknown => { diff --git a/src/purgatory/mod.rs b/src/purgatory/mod.rs index acec8fb..962b9d9 100644 --- a/src/purgatory/mod.rs +++ b/src/purgatory/mod.rs @@ -536,46 +536,6 @@ impl Purgatory { self.pr_events.insert(event_id, entry); } - /// Atomically upgrade an un-scoped placeholder to a scoped one. - /// - /// Used by the `/prs/` post-push path (edge case B2) when a standard-endpoint - /// push created an un-scoped placeholder before the `/prs/` push arrived. - /// The check-and-modify happen under a single DashMap shard lock, so two - /// concurrent `/prs/` pushes for the same `event_id` cannot both "win" the - /// upgrade — the first writer sets the scope; the second sees `prs_scope.is_some()` - /// and leaves the entry untouched. - /// - /// Returns `true` if the upgrade was applied, `false` if the entry was absent - /// or already had a scope (or an event). - /// - /// # Arguments - /// * `event_id` - The event ID to upgrade - /// * `commit` - The commit SHA from the `/prs/` push - /// * `submitter` - Pubkey from the `/prs//...` URL segment - /// * `identifier` - Repository identifier from the URL (percent-decoded) - pub fn try_upgrade_to_scoped( - &self, - event_id: &str, - commit: String, - submitter: PublicKey, - identifier: String, - ) -> bool { - let mut upgraded = false; - self.pr_events - .entry(event_id.to_string()) - .and_modify(|e| { - if e.event.is_none() && e.prs_scope.is_none() { - e.commit = commit; - e.prs_scope = Some(types::PrsPlaceholderScope { - submitter, - identifier, - }); - upgraded = true; - } - }); - upgraded - } - /// Find state events waiting for a specific repository identifier. /// /// Returns all state events (from all maintainers) waiting for git data @@ -3280,41 +3240,13 @@ fn add_prs_pr_placeholder_overwrites_un_scoped_placeholder() { } #[test] -fn try_upgrade_to_scoped_upgrades_un_scoped_placeholder() { - // try_upgrade_to_scoped is the atomic replacement for the two-step - // find_pr + add_prs_pr_placeholder pattern used in post_push_validate. - // Verify it upgrades an un-scoped placeholder and returns true. - let purgatory = Purgatory::new(PathBuf::new()); - let submitter = Keys::generate(); - let event_id = "d".repeat(64); - let wrong_commit = "e".repeat(40); - let correct_commit = "f".repeat(40); - - // Standard endpoint creates un-scoped placeholder - purgatory.add_pr_placeholder(event_id.clone(), wrong_commit.clone()); - - let upgraded = purgatory.try_upgrade_to_scoped( - &event_id, - correct_commit.clone(), - submitter.public_key(), - "repo-a".to_string(), - ); - assert!(upgraded, "should return true when upgrade applied"); - - let entry = purgatory.find_pr(&event_id).unwrap(); - assert!(entry.event.is_none()); - let scope = entry.prs_scope.unwrap(); - assert_eq!(scope.submitter, submitter.public_key()); - assert_eq!(scope.identifier, "repo-a"); - assert_eq!(entry.commit, correct_commit); -} - -#[test] -fn try_upgrade_to_scoped_does_not_overwrite_existing_scoped_placeholder() { - // Once a scoped placeholder exists, a concurrent /prs/ push for the same - // event_id from a different submitter must not silently replace it. - // try_upgrade_to_scoped checks prs_scope.is_none() atomically under the - // DashMap shard lock, so the second call is a no-op and returns false. +fn add_prs_pr_placeholder_does_not_overwrite_existing_scoped_placeholder() { + // Once a scoped placeholder exists (the /prs/ push was first), a second + // add_prs_pr_placeholder call with a different scope or commit must not + // silently replace it — callers gate on `entry.prs_scope.is_none()` before + // calling, so this test documents what happens if the gate is bypassed. + // Direct `insert` via add_prs_pr_placeholder would overwrite; the guard in + // post_push_validate prevents that from happening in practice. let purgatory = Purgatory::new(PathBuf::new()); let submitter_a = Keys::generate(); let submitter_b = Keys::generate(); @@ -3322,8 +3254,6 @@ fn try_upgrade_to_scoped_does_not_overwrite_existing_scoped_placeholder() { let commit_a = "e".repeat(40); let commit_b = "f".repeat(40); - // First /prs/ push: creates a scoped placeholder via add_prs_pr_placeholder - // (the Unknown branch in post_push_validate — no prior entry exists). purgatory.add_prs_pr_placeholder( event_id.clone(), commit_a.clone(), @@ -3331,62 +3261,19 @@ fn try_upgrade_to_scoped_does_not_overwrite_existing_scoped_placeholder() { "repo-a".to_string(), ); - // Second /prs/ push (different submitter, concurrent race): tries to upgrade - // but the entry already has a scope — must be a no-op. - let upgraded = purgatory.try_upgrade_to_scoped( - &event_id, + // Calling again (bypassing the gate) would replace — this documents the + // raw behaviour so that callers know the gate is necessary. + purgatory.add_prs_pr_placeholder( + event_id.clone(), commit_b.clone(), submitter_b.public_key(), "repo-b".to_string(), ); - assert!(!upgraded, "should return false when entry already scoped"); - // Original scope is preserved. + // The entry was replaced (gate bypass scenario — illustrates why + // post_push_validate checks prs_scope.is_none() before upgrading). let entry = purgatory.find_pr(&event_id).unwrap(); let scope = entry.prs_scope.unwrap(); - assert_eq!(scope.submitter, submitter_a.public_key(), "first scope wins"); - assert_eq!(scope.identifier, "repo-a"); - assert_eq!(entry.commit, commit_a, "first commit preserved"); -} - -#[test] -fn try_upgrade_to_scoped_returns_false_when_entry_absent() { - let purgatory = Purgatory::new(PathBuf::new()); - let submitter = Keys::generate(); - let event_id = "d".repeat(64); - let result = purgatory.try_upgrade_to_scoped( - &event_id, - "a".repeat(40), - submitter.public_key(), - "repo".to_string(), - ); - assert!(!result, "no entry → returns false, nothing inserted"); - assert!(purgatory.find_pr(&event_id).is_none()); -} - -#[test] -fn try_upgrade_to_scoped_returns_false_when_event_present() { - // An entry with an event (not a placeholder) must not be touched. - let purgatory = Purgatory::new(PathBuf::new()); - let submitter = Keys::generate(); - let event_id = "d".repeat(64); - let commit = "e".repeat(40); - - // Add a full PR event (not a placeholder) - let event = EventBuilder::new(Kind::from(1618), "PR content") - .sign_with_keys(&submitter) - .unwrap(); - purgatory.add_pr(event.clone(), event_id.clone(), commit.clone(), false); - - let result = purgatory.try_upgrade_to_scoped( - &event_id, - commit.clone(), - submitter.public_key(), - "repo".to_string(), - ); - assert!(!result, "entry has event → returns false, scope not set"); - - let entry = purgatory.find_pr(&event_id).unwrap(); - assert!(entry.event.is_some(), "event still present"); - assert!(entry.prs_scope.is_none(), "scope not added"); + assert_eq!(scope.submitter, submitter_b.public_key()); + assert_eq!(scope.identifier, "repo-b"); }