diff --git a/docs/explanation/purgatory-design.md b/docs/explanation/purgatory-design.md index 8e7d75c..f4bf6df 100644 --- a/docs/explanation/purgatory-design.md +++ b/docs/explanation/purgatory-design.md @@ -102,6 +102,8 @@ 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. @@ -227,10 +229,15 @@ 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). +**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. ### Purgatory Stores @@ -733,9 +740,33 @@ 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) + /// Add a PR placeholder (git-data-first scenario, standard endpoint) 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 4476765..974cfb3 100644 --- a/src/grasp06/receive.rs +++ b/src/grasp06/receive.rs @@ -575,19 +575,16 @@ 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 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 - ); - } + 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 + ); } } NostrRefPreValidation::Unknown => { diff --git a/src/purgatory/mod.rs b/src/purgatory/mod.rs index 962b9d9..acec8fb 100644 --- a/src/purgatory/mod.rs +++ b/src/purgatory/mod.rs @@ -536,6 +536,46 @@ 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 @@ -3240,13 +3280,41 @@ fn add_prs_pr_placeholder_overwrites_un_scoped_placeholder() { } #[test] -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. +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. let purgatory = Purgatory::new(PathBuf::new()); let submitter_a = Keys::generate(); let submitter_b = Keys::generate(); @@ -3254,6 +3322,8 @@ fn add_prs_pr_placeholder_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(), @@ -3261,19 +3331,62 @@ fn add_prs_pr_placeholder_does_not_overwrite_existing_scoped_placeholder() { "repo-a".to_string(), ); - // 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(), + // 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, commit_b.clone(), submitter_b.public_key(), "repo-b".to_string(), ); + assert!(!upgraded, "should return false when entry already scoped"); - // The entry was replaced (gate bypass scenario — illustrates why - // post_push_validate checks prs_scope.is_none() before upgrading). + // Original scope is preserved. let entry = purgatory.find_pr(&event_id).unwrap(); let scope = entry.prs_scope.unwrap(); - assert_eq!(scope.submitter, submitter_b.public_key()); - assert_eq!(scope.identifier, "repo-b"); + 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"); }