mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(relay): address pass-1 review findings R1-R3
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 <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
co-authored by
Will Pfleger
parent
4a7ea477ed
commit
985c20f885
@@ -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)"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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() {
|
||||
|
||||
@@ -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<Value> = 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<String> = 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<Value> = 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<String> = 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<String> = 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<String> = 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:?}"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user