From 307d5a0a3e65d61a82dcbc17197c4d25c68eabc7 Mon Sep 17 00:00:00 2001 From: 2ro <17595647+2ro@users.noreply.github.com> Date: Thu, 9 Jul 2026 20:32:31 -0400 Subject: [PATCH] admission: strict-lowercase gift-wrap recipient + reject extraneous tags Two more kind-1059-scoped hardening items from the security review, on top of the existing GiftWrapRetention guard: - The recipient p-tag check used case-insensitive is_hex, but the recipient-only read gate (server.rs allowed_to_send) does a case-sensitive string compare against the NIP-42 auth pubkey. An uppercase/mixed-case gift-wrap recipient would be admitted onto the relay but could never be read back by its own recipient: a permanently undeliverable payment. Switch to the existing is_lower_hex check so only strict lowercase 64-char hex is accepted. - A NIP-59 gift wrap legitimately carries exactly one tag (the single p). Reject any kind-1059 event that carries additional tags instead of silently allowing them through. Also verified and closed a related gap in the NIP-09 deletion path: kind-5 deletion hides events using an author-only check, and a gift wrap's "author" is the throwaway ephemeral key the sender briefly holds while sealing it. That let a sender recall/grief their own in-flight payment envelope by deleting it right after publish. Exclude kind 1059 from NIP-09 deletion entirely (both directions: hiding on delete arrival and hiding on gift-wrap arrival when a deletion already exists), mirrored in both the sqlite and postgres repo backends. Every other kind's deletion behavior is unchanged. New unit tests cover both the admission-layer tag/case checks and the sqlite write path's deletion guard (self-delete blocked, delete-first ordering blocked, non-1059 kinds still delete normally). --- src/admission.rs | 106 +++++++++++++++++++++++++-- src/repo/postgres.rs | 14 +++- src/repo/sqlite.rs | 166 ++++++++++++++++++++++++++++++++++++++++++- src/server.rs | 2 +- 4 files changed, 276 insertions(+), 12 deletions(-) diff --git a/src/admission.rs b/src/admission.rs index 2090dab..8ea5383 100644 --- a/src/admission.rs +++ b/src/admission.rs @@ -185,10 +185,16 @@ impl AdmissionPolicy for RestrictedKindAuthors { /// floonet-strfry's write-policy plugin (commit 55231da): this relay's /// database writer honors NIP-40 `expiration` tags as an automatic deletion /// trigger, so a Grin payment gift wrap must never carry one. Also requires -/// exactly one well-formed `p` tag (32-byte hex recipient) so a malformed -/// gift wrap the recipient's client cannot route never lands on the relay -/// either. Every other kind is unaffected. Fails closed on malformed tags, -/// matching every other policy here. +/// exactly one well-formed `p` tag: a strict-lowercase 64-char hex recipient +/// (uppercase/mixed-case is rejected, not just normalized away) because the +/// recipient-only read gate elsewhere in the relay does a case-sensitive +/// string compare against the NIP-42 auth pubkey (which nostr keys always +/// carry as lowercase hex) -- an uppercase/mixed-case `p` would be accepted +/// onto the relay but could never be read back by its recipient, silently +/// burning the payment it carries. On top of that, a NIP-59 gift wrap +/// legitimately carries exactly one tag (the single `p`), so any additional +/// tag is also rejected outright. Every other kind is unaffected. Fails +/// closed on malformed tags, matching every other policy here. pub struct GiftWrapRetention; impl AdmissionPolicy for GiftWrapRetention { @@ -209,13 +215,23 @@ impl AdmissionPolicy for GiftWrapRetention { .filter(|tag| tag.len() >= 2 && tag[0] == "p") .map(|tag| tag[1].as_str()) .collect(); + // Strict lowercase hex: an uppercase/mixed-case recipient would pass + // a case-insensitive hex check but can never match the + // case-sensitive recipient comparison the read-gate performs, so it + // must be rejected here rather than merely normalized. let recipient_ok = match p_pubkeys.as_slice() { - [pubkey] => pubkey.len() == 64 && crate::utils::is_hex(pubkey), + [pubkey] => pubkey.len() == 64 && crate::utils::is_lower_hex(pubkey), _ => false, }; if !recipient_ok { return Decision::deny("gift wrap missing recipient"); } + // A gift wrap carries only the one `p` tag; anything else riding + // along (an extra `e`, `t`, etc.) isn't part of NIP-59 and is + // rejected rather than silently allowed through. + if event.tags.len() != 1 { + return Decision::deny("gift wrap has extraneous tags"); + } Decision::Allow } } @@ -627,6 +643,86 @@ mod tests { assert_eq!(admission.check(&event, None), Decision::Allow); } + #[test] + fn giftwrap_minimal_valid_wrap_accepted() { + // The minimal legitimate NIP-59 gift wrap: exactly one well-formed, + // strictly-lowercase-hex `p` tag and nothing else. + let admission = Admission::from_settings(&floonet_settings()); + let recipient = "df".repeat(32); + let event = giftwrap_with_tags(vec![tag(&["p", &recipient])]); + assert_eq!(admission.check(&event, None), Decision::Allow); + } + + #[test] + fn giftwrap_valid_lowercase_p_accepted() { + let admission = Admission::from_settings(&floonet_settings()); + let recipient = "0123456789abcdef".repeat(4); + assert_eq!(recipient.len(), 64); + let event = giftwrap_with_tags(vec![tag(&["p", &recipient])]); + assert_eq!(admission.check(&event, None), Decision::Allow); + } + + #[test] + fn giftwrap_uppercase_p_rejected() { + // Recipient hex is well-formed but uppercase. The relay's + // recipient-only read gate does a case-sensitive compare against + // the lowercase-hex NIP-42 auth pubkey, so an uppercase recipient + // here would be an undeliverable, permanently stuck payment. + let admission = Admission::from_settings(&floonet_settings()); + let recipient = "AA".repeat(32); + let event = giftwrap_with_tags(vec![tag(&["p", &recipient])]); + match admission.check(&event, None) { + Decision::Deny { reason, .. } => { + assert!(reason.contains("missing recipient"), "{reason}"); + } + Decision::Allow => panic!("uppercase-hex recipient must be rejected"), + } + } + + #[test] + fn giftwrap_mixedcase_p_rejected() { + let admission = Admission::from_settings(&floonet_settings()); + let mut recipient = "aa".repeat(32); + recipient.replace_range(0..1, "A"); + assert_eq!(recipient.len(), 64); + let event = giftwrap_with_tags(vec![tag(&["p", &recipient])]); + match admission.check(&event, None) { + Decision::Deny { reason, .. } => { + assert!(reason.contains("missing recipient"), "{reason}"); + } + Decision::Allow => panic!("mixed-case recipient must be rejected"), + } + } + + #[test] + fn giftwrap_extraneous_tags_rejected() { + // A NIP-59 gift wrap carries only the single `p` tag; anything else + // riding along is not part of the spec and must be rejected. + let admission = Admission::from_settings(&floonet_settings()); + let recipient = "aa".repeat(32); + let event = giftwrap_with_tags(vec![tag(&["p", &recipient]), tag(&["t", "spam"])]); + match admission.check(&event, None) { + Decision::Deny { reason, .. } => { + assert!(reason.contains("extraneous tags"), "{reason}"); + } + Decision::Allow => panic!("gift wrap with an extra tag must be rejected"), + } + } + + #[test] + fn non_giftwrap_unaffected_by_case_and_tag_count() { + // Kind 0 (profile) is not the gift-wrap kind, so mixed-case/extra + // `p` tags and additional tags are all untouched by this guard. + let admission = Admission::from_settings(&floonet_settings()); + let mut event = event_of_kind(0); + event.tags = vec![ + tag(&["p", &"AA".repeat(32)]), + tag(&["p", &"bb".repeat(32)]), + tag(&["t", "hello"]), + ]; + assert_eq!(admission.check(&event, None), Decision::Allow); + } + #[test] fn non_giftwrap_with_expiration_unaffected() { // Expiration tags are fine on every other kind; only 1059 is guarded. diff --git a/src/repo/postgres.rs b/src/repo/postgres.rs index 15f87f6..3655c3a 100644 --- a/src/repo/postgres.rs +++ b/src/repo/postgres.rs @@ -251,8 +251,14 @@ ON CONFLICT (id) DO NOTHING"#, .filter_map(|x| hex::decode(x).ok()) .collect(); + // Kind 1059 (NIP-59 gift wrap) is excluded from NIP-09 deletion + // entirely: the outer gift-wrap event is signed by a throwaway + // ephemeral key the sender briefly holds, so "author-only" + // deletion here would let the sender recall/grief an in-flight + // Grin payment envelope out from under its recipient. Every + // other kind keeps ordinary author-authorized deletion. let mut builder = QueryBuilder::new( - "UPDATE \"event\" SET hidden = 1::bit(1) WHERE kind != 5 AND pub_key = ", + "UPDATE \"event\" SET hidden = 1::bit(1) WHERE kind != 5 AND kind != 1059 AND pub_key = ", ); builder.push_bind(hex::decode(&e.pubkey).ok()); builder.push(" AND id IN ("); @@ -269,9 +275,11 @@ ON CONFLICT (id) DO NOTHING"#, update_count, e.get_author_prefix() ); - } else { + } else if e.kind != 1059 { // check if a deletion has already been recorded for this event. - // Only relevant for non-deletion events + // Only relevant for non-deletion, non-gift-wrap events: a gift + // wrap must never be retroactively hidden by an earlier kind-5, + // for the same payment-reliability reason as above. let del_count = sqlx::query( "SELECT e.id FROM \"event\" e \ LEFT JOIN tag t ON e.id = t.event_id \ diff --git a/src/repo/sqlite.rs b/src/repo/sqlite.rs index 3b9c0b7..f94b4da 100644 --- a/src/repo/sqlite.rs +++ b/src/repo/sqlite.rs @@ -208,8 +208,14 @@ impl SqliteRepo { .filter(|x| is_hex(x) && x.len() == 64) .filter_map(|x| hex::decode(x).ok()) .for_each(|x| params.push(Box::new(x))); + // Kind 1059 (NIP-59 gift wrap) is excluded from NIP-09 deletion + // entirely: the outer gift-wrap event is signed by a throwaway + // ephemeral key the sender briefly holds, so "author-only" + // deletion here would let the sender recall/grief an in-flight + // Grin payment envelope out from under its recipient. Every + // other kind keeps ordinary author-authorized deletion. let query = format!( - "UPDATE event SET hidden=TRUE WHERE kind!=5 AND author=? AND event_hash IN ({})", + "UPDATE event SET hidden=TRUE WHERE kind!=5 AND kind!=1059 AND author=? AND event_hash IN ({})", repeat_vars(params.len() - 1) ); let mut stmt = tx.prepare(&query)?; @@ -219,9 +225,11 @@ impl SqliteRepo { update_count, e.get_author_prefix() ); - } else { + } else if e.kind != 1059 { // check if a deletion has already been recorded for this event. - // Only relevant for non-deletion events + // Only relevant for non-deletion, non-gift-wrap events: a gift + // wrap must never be retroactively hidden by an earlier kind-5, + // for the same payment-reliability reason as above. let del_count = tx.query_row( "SELECT e.id FROM event e WHERE e.author=? AND e.id IN (SELECT t.event_id FROM tag t WHERE t.name='e' AND t.kind=5 AND t.value=?) LIMIT 1;", params![pubkey_blob, e.id], |row| row.get::(0)); @@ -1611,4 +1619,156 @@ mod tests { "Should have 2 subqueries for 2 different tag keys" ); } + + // --- Kind-1059 (NIP-59 gift wrap) deletion guard (payment reliability) --- + // + // These exercise the real write path against a temp-file-backed + // SQLite database (not the shared in-memory URI, so concurrently + // running tests don't collide on the same underlying db). + + fn unique_test_settings(label: &str) -> Settings { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::SeqCst); + let dir = std::env::temp_dir().join(format!( + "floonet-rs-giftwrap-delete-test-{}-{}-{}", + std::process::id(), + label, + n + )); + std::fs::create_dir_all(&dir).expect("create test db dir"); + let mut settings = Settings::default(); + settings.database.data_directory = dir.to_string_lossy().into_owned(); + settings.database.in_memory = false; + settings + } + + fn test_event(kind: u64, id: &str, pubkey: &str, tags: Vec>) -> Event { + let mut e = Event::simple_event(); + e.kind = kind; + e.id = id.to_owned(); + e.pubkey = pubkey.to_owned(); + e.tags = tags; + e + } + + async fn hidden_flag(repo: &SqliteRepo, event_id_hex: &str) -> bool { + let pool = repo.read_pool.clone(); + let id = event_id_hex.to_owned(); + task::spawn_blocking(move || { + let conn = pool.get().unwrap(); + conn.query_row( + "SELECT hidden FROM event WHERE event_hash=?", + params![hex::decode(&id).unwrap()], + |row| row.get::(0), + ) + .unwrap() + }) + .await + .unwrap() + } + + #[tokio::test] + async fn kind5_delete_of_giftwrap_is_blocked_even_from_same_ephemeral_author() { + let settings = unique_test_settings("same-author"); + let (_registry, metrics) = crate::server::create_metrics(); + let repo = SqliteRepo::new(&settings, metrics); + repo.migrate_up().await.unwrap(); + + let ephemeral_author = "ab".repeat(32); + let recipient = "cd".repeat(32); + let giftwrap_id = "11".repeat(32); + let delete_id = "22".repeat(32); + + let giftwrap = test_event( + 1059, + &giftwrap_id, + &ephemeral_author, + vec![vec!["p".to_owned(), recipient]], + ); + repo.write_event(&giftwrap).await.unwrap(); + + // The sender still holds the ephemeral gift-wrap key right after + // publishing and tries to recall the payment, targeting the gift + // wrap's own event id from that same (non-recipient) key. + let delete = test_event( + 5, + &delete_id, + &ephemeral_author, + vec![vec!["e".to_owned(), giftwrap_id.clone()]], + ); + repo.write_event(&delete).await.unwrap(); + + assert!( + !hidden_flag(&repo, &giftwrap_id).await, + "a kind-5 from the gift wrap's own (ephemeral, non-recipient) author must not hide the payment envelope" + ); + } + + #[tokio::test] + async fn kind5_predating_giftwrap_does_not_hide_it_on_arrival() { + // Same scenario but the deletion happens to land before the gift + // wrap does; the reverse ("already deleted") check must also spare + // kind 1059. + let settings = unique_test_settings("delete-first"); + let (_registry, metrics) = crate::server::create_metrics(); + let repo = SqliteRepo::new(&settings, metrics); + repo.migrate_up().await.unwrap(); + + let ephemeral_author = "ab".repeat(32); + let recipient = "cd".repeat(32); + let giftwrap_id = "33".repeat(32); + let delete_id = "44".repeat(32); + + let delete = test_event( + 5, + &delete_id, + &ephemeral_author, + vec![vec!["e".to_owned(), giftwrap_id.clone()]], + ); + repo.write_event(&delete).await.unwrap(); + + let giftwrap = test_event( + 1059, + &giftwrap_id, + &ephemeral_author, + vec![vec!["p".to_owned(), recipient]], + ); + repo.write_event(&giftwrap).await.unwrap(); + + assert!( + !hidden_flag(&repo, &giftwrap_id).await, + "a pre-existing kind-5 must not retroactively hide a gift wrap on arrival" + ); + } + + #[tokio::test] + async fn kind5_delete_still_works_for_non_giftwrap_kinds() { + // Regression check: the guard is scoped to kind 1059 only; ordinary + // NIP-09 same-author deletion of every other kind is unaffected. + let settings = unique_test_settings("control"); + let (_registry, metrics) = crate::server::create_metrics(); + let repo = SqliteRepo::new(&settings, metrics); + repo.migrate_up().await.unwrap(); + + let author = "ab".repeat(32); + let note_id = "55".repeat(32); + let delete_id = "66".repeat(32); + + let note = test_event(7, ¬e_id, &author, vec![]); + repo.write_event(¬e).await.unwrap(); + + let delete = test_event( + 5, + &delete_id, + &author, + vec![vec!["e".to_owned(), note_id.clone()]], + ); + repo.write_event(&delete).await.unwrap(); + + assert!( + hidden_flag(&repo, ¬e_id).await, + "a same-author kind-5 must still hide ordinary (non-1059) kinds" + ); + } } diff --git a/src/server.rs b/src/server.rs index 8c16014..835761a 100644 --- a/src/server.rs +++ b/src/server.rs @@ -773,7 +773,7 @@ async fn ctrl_c_or_signal(mut shutdown_signal: Receiver<()>) { } } -fn create_metrics() -> (Registry, NostrMetrics) { +pub(crate) fn create_metrics() -> (Registry, NostrMetrics) { // setup prometheus registry let registry = Registry::new();