From 56889bfc1df28282e1edfa229ee0029584fc9353 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 23 Sep 2026 15:07:19 +0000 Subject: [PATCH] fix(nostr): reject PR events missing commit metadata as invalid Production sync repeatedly retried immutable PR events without a c tag because the Git validation error was classified as a retryable server failure. Validate required commit metadata before Git access and return an invalid rejection for PR and PR-update events. Sync's existing terminal accounting then stops hydration retries. Reuse the metadata extraction in the Git policy. Only absent commit values are reclassified. This assumes signed metadata cannot be repaired by fetching another copy; unavailable Git objects still enter purgatory and actual Git/database faults remain retryable. Commit hash syntax and other PR metadata validation are deliberately unchanged. Validation: the regression covers both event kinds, missing and valueless tags, synced and direct delivery, and valid metadata awaiting Git data. All 924 library tests and the PR hosting and purgatory integration suites passed, along with all-target Clippy with warnings denied and formatting. Assisted-by: GPT-6 --- CHANGELOG.md | 3 ++ docs/explanation/architecture.md | 6 +++ src/nostr/builder.rs | 82 ++++++++++++++++++++++++++++++++ src/nostr/policy/pr_event.rs | 20 ++++---- 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19dd18a..b11eaef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Reject PR and PR-update events missing commit metadata as terminal invalid + events, avoiding repeated recovery attempts for immutable malformed data. + - Retire incomplete historic batches when transient requests time out or close before EOSE, and bound SDK subscription retention with an auto-close deadline. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index c2a71d4..34884d3 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -398,6 +398,12 @@ The purgatory system solves two related problems: **Design Document**: See [`purgatory-design.md`](purgatory-design.md) for complete design specifications. +PR and PR-update events must include a commit value in their signed `c` tag. +Missing metadata is rejected as `invalid` before Git checks, so sync treats the +event as terminally accounted rather than repeatedly retrying an apparent +server failure. Events with commit metadata but unavailable Git objects remain +eligible for purgatory; Git and database failures remain retryable. + #### Architecture ```rust diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index 7a40df6..80568c4 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -551,6 +551,11 @@ impl Nip34WritePolicy { /// * `event` - The PR event to validate /// * `is_synced` - True if this event came from proactive sync (vs user-submitted) async fn handle_pr_event(&self, event: &Event, is_synced: bool) -> WritePolicyResult { + // Missing signed metadata cannot be repaired by fetching Git data or + // retrying another relay. Keep actual Git/database faults retryable. + if PrEventPolicy::commit(event).is_none() { + return reject_invalid(format!("PR event {} has no 'c' tag", event.id)); + } let event_id_str = event.id.to_bech32().unwrap_or_else(|_| event.id.to_hex()); // duplicate check in purgatory @@ -1176,3 +1181,80 @@ pub async fn create_relay( }, }) } + +#[cfg(test)] +mod pr_metadata_tests { + use super::*; + use crate::grasp06::receive::RepoInitLocks; + use std::path::PathBuf; + + #[tokio::test] + async fn missing_pr_commit_is_invalid_but_missing_git_data_enters_purgatory() { + let directory = tempfile::tempdir().unwrap(); + let mut config = Config::for_testing(); + config.grasp06_enable = true; + config.git_data_path = directory.path().join("git").to_string_lossy().into_owned(); + config.relay_data_path = directory + .path() + .join("relay") + .to_string_lossy() + .into_owned(); + let purgatory = Arc::new(crate::purgatory::Purgatory::new(PathBuf::from( + &config.git_data_path, + ))); + let runtime = create_relay(&config, purgatory, RepoInitLocks::default(), None) + .await + .unwrap(); + let keys = Keys::generate(); + let tags = vec![ + Tag::custom("a", [format!("30617:{}:test", keys.public_key().to_hex())]), + Tag::custom( + "clone", + [format!( + "https://{}/prs/{}/test.git", + config.service_address(), + keys.public_key().to_bech32().unwrap() + )], + ), + ]; + for kind in [Kind::GitPullRequest, Kind::GitPullRequestUpdate] { + for empty_tag in [false, true] { + let mut event_tags = tags.clone(); + if empty_tag { + event_tags.push(Tag::custom("c", Vec::::new())); + } + let event = EventBuilder::new(kind, "missing commit") + .tags(event_tags) + .finalize(&keys) + .unwrap(); + for synced in [false, true] { + let result = runtime.write_policy.handle_pr_event(&event, synced).await; + assert!( + matches!( + result, + WritePolicyResult::Reject { + prefix: MachineReadablePrefix::Invalid, + status: false, + .. + } + ), + "missing immutable metadata must be a terminal validation failure" + ); + } + } + let event = EventBuilder::new(kind, "awaiting Git data") + .tags(tags.clone()) + .tag(Tag::custom( + "c", + ["1111111111111111111111111111111111111111"], + )) + .finalize(&keys) + .unwrap(); + let result = runtime.write_policy.handle_pr_event(&event, true).await; + assert!( + matches!(result, WritePolicyResult::Reject { status: true, .. }), + "valid metadata with unavailable Git objects must remain eligible for purgatory" + ); + } + } +} diff --git a/src/nostr/policy/pr_event.rs b/src/nostr/policy/pr_event.rs index 9a78806..449b85d 100644 --- a/src/nostr/policy/pr_event.rs +++ b/src/nostr/policy/pr_event.rs @@ -30,6 +30,16 @@ impl PrEventPolicy { } } + /// Extract required commit metadata separately from fallible Git I/O. + pub(crate) fn commit(event: &Event) -> Option { + event.tags.iter().find_map(|tag| { + let values = tag.as_slice(); + (values.first().map(String::as_str) == Some("c")) + .then(|| values.get(1).cloned()) + .flatten() + }) + } + /// Check if git data exists for a PR event /// /// This unified method checks for git data existence and handles: @@ -46,15 +56,7 @@ impl PrEventPolicy { pub async fn git_data_check(&self, event: &Event) -> Result { let event_id = event.id.to_hex(); - // Extract the `c` tag (commit hash) from the PR event - let commit = event.tags.iter().find_map(|tag| { - let tag_vec = tag.clone().to_vec(); - if tag_vec.len() >= 2 && tag_vec[0] == "c" { - Some(tag_vec[1].clone()) - } else { - None - } - }); + let commit = Self::commit(event); let commit = match commit { Some(c) => c,