mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
fix TOCTOU in /prs/ placeholder upgrade via atomic try_upgrade_to_scoped
The two-step find_pr + add_prs_pr_placeholder pattern in post_push_validate had a TOCTOU window: two concurrent /prs/ pushes for the same event_id could both see an un-scoped placeholder and both call add_prs_pr_placeholder, with the second write silently overwriting the first scope. Replace with Purgatory::try_upgrade_to_scoped, which performs the is_none() check and the field update atomically under a single DashMap shard lock via entry().and_modify(). The first writer sets the scope; any subsequent call sees prs_scope.is_some() and is a no-op (returns false). - Add try_upgrade_to_scoped to src/purgatory/mod.rs - Update post_push_validate in src/grasp06/receive.rs to use it - Replace the old gate-bypass documentation test with four focused tests covering: successful upgrade, no-overwrite of existing scope, absent entry, and entry-with-event cases - Update purgatory-design.md: PR entry struct, API section, and principle 4 (bidirectional waiting) to document the atomic upgrade
This commit is contained in:
@@ -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/<npub>/<id>.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<PrsPlaceholderScope>,
|
||||
}
|
||||
```
|
||||
|
||||
**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<StatePurgatoryEntry>;
|
||||
|
||||
|
||||
+10
-13
@@ -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 => {
|
||||
|
||||
+128
-15
@@ -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/<npub>/...` 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");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user