From 985c20f88588f0653335569121167c88adfe9583 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Sun, 12 Jul 2026 14:46:59 -0400 Subject: [PATCH] fix(relay): address pass-1 review findings R1-R3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1 (IMPORTANT): Extract should_dispatch_workflow helper from inline if condition in dispatch_persistent_event_inner. The Q7 test previously duplicated the production predicate as a local closure; deleting the AUTHOR_ONLY_KINDS guard from the real if block would leave it green. Now dispatch_persistent_event_inner calls should_dispatch_workflow, and the test calls the same function — no copy-paste divergence is possible. Test gains kind:30300 + is_relay_workflow_msg=true controls. R2 (IMPORTANT): Add test_reminder_target_reaction_oracle_closed e2e to prove the author-only mask in derive_reaction_channel generalizes to kind:30300 reminders (not only kind:31234 drafts). Corrects the overclaiming comment at ingest.rs:4149-4154 which asserted the unit tests exercised actor_can_reference_target directly; they only assert constant membership, and the comment now says so explicitly with a pointer to the e2e tests that provide behavioral coverage. R3 (MINOR): Move draft timestamp to base+3 in test_channel_window_cursor_boundary_excludes_draft so the draft is inside the raw first page (raw DESC: msg2@+4, draft@+3, msg1@+2, msg0@+0 — a filter-after-pagination regression would cursor off draft). Replace the optional if-let cursor branch with cursor.expect(), forcing the test to fail if has_more=false or no cursor is returned. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- crates/buzz-relay/src/handlers/event.rs | 110 +++++---- crates/buzz-relay/src/handlers/ingest.rs | 15 +- .../buzz-test-client/tests/e2e_nip37_draft.rs | 225 +++++++++++++----- 3 files changed, 236 insertions(+), 114 deletions(-) diff --git a/crates/buzz-relay/src/handlers/event.rs b/crates/buzz-relay/src/handlers/event.rs index 90b93cd26..cf93b5729 100644 --- a/crates/buzz-relay/src/handlers/event.rs +++ b/crates/buzz-relay/src/handlers/event.rs @@ -370,6 +370,31 @@ pub(crate) async fn dispatch_persistent_event( 0 } +/// Returns `true` when an event of `kind_u32` should trigger workspace-level +/// workflow dispatch. +/// +/// This is the single canonical gate that `dispatch_persistent_event_inner` +/// uses. Extracting it as a named function lets tests call the real predicate +/// rather than maintaining a parallel closure that can silently diverge. +/// +/// Excluded from dispatch: +/// - Workflow-execution kinds (avoid re-triggering the engine on its own output) +/// - Command kinds (internal relay commands) +/// - Relay-signed workflow messages (tagged `buzz:workflow`) +/// - `KIND_GIFT_WRAP` (encrypted payloads — content is opaque) +/// - `AUTHOR_ONLY_KINDS` (NIP-37 draft wraps, NIP-ER reminders — private +/// per-user state that must never reach workspace-level automations) +pub(crate) fn should_dispatch_workflow(kind_u32: u32, is_relay_workflow_msg: bool) -> bool { + !buzz_core::kind::is_workflow_execution_kind(kind_u32) + && !buzz_core::kind::is_command_kind(kind_u32) + && !is_relay_workflow_msg + && kind_u32 != KIND_GIFT_WRAP + // Author-only kinds (NIP-ER reminders, NIP-37 draft wraps) are private + // per-user state that must not trigger workspace-level workflows. + // AUTHOR_ONLY_KINDS.contains is the permanent guard at this seam. + && !AUTHOR_ONLY_KINDS.contains(&kind_u32) +} + /// Run post-commit delivery/side effects for a stored event. async fn dispatch_persistent_event_inner( tenant: &TenantContext, @@ -503,15 +528,7 @@ async fn dispatch_persistent_event_inner( .iter() .any(|t| t.as_slice().first().map(|s| s.as_str()) == Some("buzz:workflow")); - if !buzz_core::kind::is_workflow_execution_kind(kind_u32) - && !buzz_core::kind::is_command_kind(kind_u32) - && !is_relay_workflow_msg - && kind_u32 != KIND_GIFT_WRAP - // Author-only kinds (NIP-ER reminders, NIP-37 draft wraps) are private - // per-user state that must not trigger workspace-level workflows. - // AUTHOR_ONLY_KINDS.contains is the permanent guard at this seam. - && !AUTHOR_ONLY_KINDS.contains(&kind_u32) - { + if should_dispatch_workflow(kind_u32, is_relay_workflow_msg) { let workflow_engine = Arc::clone(&state.workflow_engine); let workflow_event = stored_event.clone(); let trigger_kind = kind_u32.to_string(); @@ -2427,58 +2444,51 @@ mod tests { ); } - /// Tripwire: kind:31234 (NIP-37 draft wrap) MUST appear in - /// `AUTHOR_ONLY_KINDS` so the workflow-dispatch guard at - /// `event.rs:514` (`!AUTHOR_ONLY_KINDS.contains(&kind_u32)`) - /// permanently suppresses workflow triggers for draft events. + /// Dispatch-predicate tripwire: kind:31234 (NIP-37 draft wrap) and + /// kind:30300 (NIP-ER reminder) are in `AUTHOR_ONLY_KINDS` so + /// `should_dispatch_workflow` returns `false` for them, permanently + /// suppressing workflow triggers for author-only events. /// - /// A draft must never arrive in the workflow engine, regardless - /// of future refactoring in the dispatch path. - /// - /// The test exercises the **actual dispatch predicate** used in - /// `dispatch_persistent_event_inner` (the `if` condition that - /// gates the workflow-spawn block) — not just membership in the - /// constant. It compares a kind:31234 draft against a kind:9 - /// channel message to prove the gate is live: the predicate must - /// evaluate to `false` for the draft and `true` for the message. + /// The test calls `should_dispatch_workflow` — the same function + /// that `dispatch_persistent_event_inner` calls — so deleting or + /// changing the guard in the production path reds this test. + /// A kind:9 positive control proves the predicate is not trivially + /// always-false. #[test] fn draft_kind_is_excluded_from_workflow_dispatch_by_author_only_guard() { - // Step 1 — membership check: AUTHOR_ONLY_KINDS must contain KIND_DRAFT. - // Changing the constant without updating this test turns it red. + // kind:31234 must be excluded from workflow dispatch. + // is_relay_workflow_msg=false for any user-submitted event. assert!( - buzz_core::kind::AUTHOR_ONLY_KINDS.contains(&buzz_core::kind::KIND_DRAFT), - "KIND_DRAFT must be in AUTHOR_ONLY_KINDS — the workflow-dispatch guard \ - at event.rs:514 (`!AUTHOR_ONLY_KINDS.contains(&kind_u32)`) relies on \ - this to suppress draft events from reaching the workflow engine" - ); - - // Step 2 — dispatch-predicate evaluation for a normal user event - // (is_relay_workflow_msg = false for any user-submitted event, so - // that branch does not affect the predicate). This mirrors the - // exact `if` condition that gates the workflow spawn in - // `dispatch_persistent_event_inner`. - let should_trigger_workflow = |kind_u32: u32| -> bool { - // is_relay_workflow_msg = false for user-submitted events. - !buzz_core::kind::is_workflow_execution_kind(kind_u32) - && !buzz_core::kind::is_command_kind(kind_u32) - && kind_u32 != buzz_core::kind::KIND_GIFT_WRAP - && !buzz_core::kind::AUTHOR_ONLY_KINDS.contains(&kind_u32) - }; - - assert!( - !should_trigger_workflow(buzz_core::kind::KIND_DRAFT), - "workflow dispatch predicate must return false for kind:31234 — \ + !super::super::should_dispatch_workflow(buzz_core::kind::KIND_DRAFT, false), + "should_dispatch_workflow must return false for kind:31234 — \ a draft wrap must NEVER reach the workflow engine" ); - // Positive control: a plain channel message (kind:9) must pass the - // same predicate so we know the gate is not just trivially always-false. + // kind:30300 (NIP-ER reminder) is also AUTHOR_ONLY and must be excluded. + assert!( + !super::super::should_dispatch_workflow( + buzz_core::kind::KIND_EVENT_REMINDER, + false + ), + "should_dispatch_workflow must return false for kind:30300 — \ + reminders are author-only and must not trigger workspace workflows" + ); + + // Positive control: a plain channel message (kind:9) must return true + // so we know the predicate is not trivially always-false. let kind_channel_message: u32 = 9; assert!( - should_trigger_workflow(kind_channel_message), - "workflow dispatch predicate must return true for kind:9 channel messages \ + super::super::should_dispatch_workflow(kind_channel_message, false), + "should_dispatch_workflow must return true for kind:9 channel messages \ (positive control) — the gate must be live, not trivially always-false" ); + + // is_relay_workflow_msg=true must suppress dispatch for any kind. + assert!( + !super::super::should_dispatch_workflow(kind_channel_message, true), + "should_dispatch_workflow must return false when is_relay_workflow_msg=true \ + (relay-signed workflow messages must not re-trigger the engine)" + ); } } } diff --git a/crates/buzz-relay/src/handlers/ingest.rs b/crates/buzz-relay/src/handlers/ingest.rs index 708fece4f..f8c3bb06e 100644 --- a/crates/buzz-relay/src/handlers/ingest.rs +++ b/crates/buzz-relay/src/handlers/ingest.rs @@ -4153,11 +4153,16 @@ mod tests { } // ── actor_can_reference_target: author-only mask ─────────────────────────── - // These unit tests verify that `actor_can_reference_target` masks kind:31234 - // (draft wraps) and kind:30300 (reminders) — all `AUTHOR_ONLY_KINDS` — without - // requiring a live Postgres. The helper is the single seam that closes the - // write-path id-oracle on reaction, thread-parent, stream-edit, forum-vote, - // and kind:5 e-tag deletion paths. + // These unit tests verify AUTHOR_ONLY_KINDS membership for kind:31234 and + // kind:30300 — the constant that `actor_can_reference_target` (and the + // inline guards in `derive_reaction_channel` / `resolve_nip10_thread_meta`) + // derive their mask from. They do NOT exercise the async helper or any + // call site directly (no live Postgres is available here). + // + // Behavioral coverage — that the guard actually fires in the live relay for + // both draft and reminder targets — is provided by the e2e tests: + // - e2e_nip37_draft.rs: test_draft_target_reaction_oracle_closed (kind:31234) + // - e2e_nip37_draft.rs: test_reminder_target_reaction_oracle_closed (kind:30300) #[test] fn author_only_kinds_covers_draft_and_reminder() { diff --git a/crates/buzz-test-client/tests/e2e_nip37_draft.rs b/crates/buzz-test-client/tests/e2e_nip37_draft.rs index a61afddaa..dc1088872 100644 --- a/crates/buzz-test-client/tests/e2e_nip37_draft.rs +++ b/crates/buzz-test-client/tests/e2e_nip37_draft.rs @@ -2895,9 +2895,11 @@ async fn test_channel_window_cursor_boundary_excludes_draft() { add_member_http(&client, &author, &ch_id, &attacker).await; // Post 3 public kind:9 messages with strictly increasing timestamps. - // Then post a draft between message 1 and 2 (same second as msg-1 so it - // sits in the middle of the window) to make it straddle the cursor - // boundary when limit=2. + // The draft is placed at base+3 — inside the raw first page of limit=2 + // (raw DESC order: msg2@+4, draft@+3, msg1@+2, msg0@+0). A regression + // that filters drafts AFTER computing the cursor would pick draft as the + // cursor candidate and leak it. Correct SQL-layer exclusion computes + // the cursor from the filtered set, giving msg1@+2 as the page-1 cursor. let base_ts = nostr::Timestamp::now().as_secs() - 10; let mut msg_ids = Vec::new(); for i in 0..3u64 { @@ -2912,10 +2914,11 @@ async fn test_channel_window_cursor_boundary_excludes_draft() { assert!(ok, "kind:9 message {i} must be accepted: {err}"); } - // Post a draft with a timestamp between msg-0 and msg-1 so it lands in - // the middle of the window (created_at = base_ts + 1). + // Draft at base+3: sits between msg2@+4 and msg1@+2, inside the raw + // first page. A filter-after-pagination regression would use it as + // the cursor (leaking the draft id via 39006 next_cursor). let d = uuid::Uuid::new_v4().to_string(); - let draft_ts = nostr::Timestamp::from(base_ts + 1); + let draft_ts = nostr::Timestamp::from(base_ts + 3); let draft = nostr::EventBuilder::new(nostr::Kind::Custom(KIND_DRAFT), fake_nip44_v2()) .tags([ nostr::Tag::parse(["d", &d]).unwrap(), @@ -2996,62 +2999,65 @@ async fn test_channel_window_cursor_boundary_excludes_draft() { Some((cursor_ts, cursor_id)) }); - // If the relay returned has_more=true and a cursor, paginate to page 2 - // and verify the second page also contains no draft id. - if let Some((cursor_ts, cursor_id)) = cursor { - // The cursor must not be the draft id. - assert_ne!( - cursor_id, draft_id, - "next_cursor.id must not be the draft id — cursor must be computed on the \ - draft-excluded row set" - ); + // With 3 public messages and limit=2, has_more MUST be true and a cursor + // MUST be present — the test is structured to force pagination. If the + // relay returned has_more=false, the SQL-layer exclusion likely mis-counted. + let (cursor_ts, cursor_id) = cursor.expect( + "page 1 must have has_more=true and a next_cursor — 3 public messages + limit=2 \ + guarantees pagination; if this fails the draft may have been counted or the \ + cursor may have been computed from the unfiltered row set", + ); - let page2_filter = serde_json::json!({ - "kinds": [9, KIND_DRAFT], - "#h": [ch_id], - "top_level": true, - "limit": 2, - "until": cursor_ts, - "before_id": cursor_id, - "include_aux": true, - "include_summaries": false, - }); - let resp2 = client - .post(format!("{}/query", relay_http_url())) - .header("X-Pubkey", &attacker.public_key().to_hex()) - .header("Content-Type", "application/json") - .body(serde_json::to_string(&serde_json::json!([page2_filter])).unwrap()) - .send() - .await - .expect("page 2 window query"); - assert!(resp2.status().is_success(), "page 2 must succeed"); - let page2_events: Vec = resp2.json().await.expect("page 2 parse"); + // The cursor must not be the draft id. + assert_ne!( + cursor_id, draft_id, + "next_cursor.id must not be the draft id — cursor must be computed on the \ + draft-excluded row set" + ); - let page2_ids: Vec = page2_events - .iter() - .filter_map(|e| e["id"].as_str().map(|s| s.to_string())) - .collect(); - assert!( - !page2_ids.iter().any(|id| id == &draft_id), - "draft id must not appear in page 2: draft_id={draft_id}, page2={page2_events:?}" - ); + let page2_filter = serde_json::json!({ + "kinds": [9, KIND_DRAFT], + "#h": [ch_id], + "top_level": true, + "limit": 2, + "until": cursor_ts, + "before_id": cursor_id, + "include_aux": true, + "include_summaries": false, + }); + let resp2 = client + .post(format!("{}/query", relay_http_url())) + .header("X-Pubkey", &attacker.public_key().to_hex()) + .header("Content-Type", "application/json") + .body(serde_json::to_string(&serde_json::json!([page2_filter])).unwrap()) + .send() + .await + .expect("page 2 window query"); + assert!(resp2.status().is_success(), "page 2 must succeed"); + let page2_events: Vec = resp2.json().await.expect("page 2 parse"); - // Gapless invariant: the two pages together cover all 3 public messages. - let all_msg_ids: std::collections::HashSet = page1_ids - .iter() - .chain(page2_ids.iter()) - .filter(|id| msg_ids.contains(id)) - .cloned() - .collect(); - assert_eq!( - all_msg_ids.len(), - 3, - "all 3 public messages must appear across both pages with no gaps; \ - got: page1={page1_ids:?}, page2={page2_ids:?}" - ); - } - // If has_more=false, all 3 messages fit in one page — cursor test skipped, - // but the no-draft assertion above already passed. + let page2_ids: Vec = page2_events + .iter() + .filter_map(|e| e["id"].as_str().map(|s| s.to_string())) + .collect(); + assert!( + !page2_ids.iter().any(|id| id == &draft_id), + "draft id must not appear in page 2: draft_id={draft_id}, page2={page2_events:?}" + ); + + // Gapless invariant: the two pages together cover all 3 public messages. + let all_msg_ids: std::collections::HashSet = page1_ids + .iter() + .chain(page2_ids.iter()) + .filter(|id| msg_ids.contains(id)) + .cloned() + .collect(); + assert_eq!( + all_msg_ids.len(), + 3, + "all 3 public messages must appear across both pages with no gaps; \ + got: page1={page1_ids:?}, page2={page2_ids:?}" + ); } // ─── Q15 oracle-closure e2e — write-path id-oracle guards ──────────────────── @@ -3450,3 +3456,104 @@ async fn test_draft_target_kind5_oracle_closed() { "draft head must be the original draft — attacker's kind:5 must not have altered it" ); } + +#[tokio::test] +#[ignore] +async fn test_reminder_target_reaction_oracle_closed() { + // Behavioral companion to test_draft_target_reaction_oracle_closed for + // kind:30300 (NIP-ER reminders). This proves the author-only mask in + // derive_reaction_channel generalizes over ALL AUTHOR_ONLY_KINDS, not + // just kind:31234 drafts. + // + // Attacker reacts (kind:7) to: + // (A) a real kind:30300 reminder event id authored by `author` + // (B) a random 64-hex id that definitely does not exist + // + // Both must return the SAME byte-identical error string. After each + // rejected submission, the attacker queries for kind:7 events that + // e-tag the target id and asserts zero are stored. + // + // Reminders are global (no channel association), so the test does not + // need a channel and the relay must fire the author-only guard before + // any channel-derivation logic. + let client = http_client(); + let author = Keys::generate(); + let attacker = Keys::generate(); + + // Author publishes a kind:30300 reminder. Minimal valid shape: d tag + alt tag. + let d = uuid::Uuid::new_v4().to_string(); + let reminder = + nostr::EventBuilder::new(nostr::Kind::Custom(30300), "nip44-ciphertext-placeholder") + .tags([ + nostr::Tag::parse(["d", &d]).unwrap(), + nostr::Tag::parse(["alt", "Encrypted reminder"]).unwrap(), + ]) + .sign_with_keys(&author) + .unwrap(); + let reminder_id_hex = reminder.id.to_hex(); + let (ok_r, err_r) = submit_event_http(&client, &author, &reminder).await; + assert!(ok_r, "reminder must be accepted: {err_r}"); + + let random_id_hex = "3".repeat(64); + + let build_reaction = |target_hex: &str| { + nostr::EventBuilder::new(nostr::Kind::Custom(7), "+") + .tags([nostr::Tag::parse(["e", target_hex]).unwrap()]) + .sign_with_keys(&attacker) + .unwrap() + }; + + // (A) Attacker reacts to the real reminder id. + let reaction_a = build_reaction(&reminder_id_hex); + let (accepted_a, msg_a) = submit_event_http(&client, &attacker, &reaction_a).await; + assert!( + !accepted_a, + "reaction to a real reminder id must be rejected; relay said: {msg_a}" + ); + + // (B) Attacker reacts to the random (nonexistent) id. + let reaction_b = build_reaction(&random_id_hex); + let (accepted_b, msg_b) = submit_event_http(&client, &attacker, &reaction_b).await; + assert!( + !accepted_b, + "reaction to a nonexistent id must be rejected; relay said: {msg_b}" + ); + + // Oracle-closure: error strings must be byte-identical. + assert_eq!( + msg_a, msg_b, + "reaction rejection for real-reminder-id vs random-id must be BYTE-IDENTICAL; \ + real_reminder='{msg_a}', random='{msg_b}'" + ); + assert_eq!( + msg_a, "invalid: reaction target event not found", + "expected byte-exact masking error for reminder target; got: '{msg_a}'" + ); + + // Post-rejection storage check: no kind:7 events e-tagging the reminder id + // must be stored. + let kind7_filter = nostr::Filter::new() + .kind(nostr::Kind::Custom(7)) + .custom_tag( + nostr::SingleLetterTag::lowercase(nostr::Alphabet::E), + reminder_id_hex.as_str(), + ); + let kind7_as_attacker = query_events_http( + &client, + &attacker.public_key().to_hex(), + vec![kind7_filter.clone()], + ) + .await; + assert!( + kind7_as_attacker.is_empty(), + "zero kind:7 events referencing the reminder id must be stored after attacker's \ + rejected reaction attempt; found: {kind7_as_attacker:?}" + ); + let kind7_as_author = + query_events_http(&client, &author.public_key().to_hex(), vec![kind7_filter]).await; + assert!( + kind7_as_author.is_empty(), + "author-side query must also return zero kind:7 events referencing the reminder id; \ + found: {kind7_as_author:?}" + ); +}