admission: strict-lowercase gift-wrap recipient + reject extraneous tags
Test and build / test_floonet-rs (push) Has been cancelled
Test and build / test_floonet-rs (push) Has been cancelled
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).
This commit is contained in:
+101
-5
@@ -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.
|
||||
|
||||
+11
-3
@@ -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 \
|
||||
|
||||
+163
-3
@@ -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::<usize, usize>(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<Vec<String>>) -> 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::<usize, bool>(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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user