From 45d8f69d336e335f27fc49dc81c8228496ab8cca Mon Sep 17 00:00:00 2001
From: James Maes <jmaes@dmdbrands.com>
Date: Fri, 31 Jul 2026 11:29:46 -0500
Subject: [PATCH] fix(filter): use unique rule ids to prevent silent overwrite

CreateRule derived a rule's id deterministically from
sha256(name + nsql), so two rules with identical name and NSQL text
produced the same id. save_filter_rule persists via INSERT OR REPLACE
with no existence check, so the second create silently overwrote the
first rule with no warning.

Generate each rule's id from a CSPRNG (the same OsRng nuncio-store
already uses to source key material) instead of deriving it from the
rule's content, so identical name+NSQL always yields distinct ids.
UpdateRule and list_filter_rules already reassign the id from the
persisted row after parsing, so they are unaffected.

Refs #249
---
 Cargo.lock                         |  1 +
 crates/nuncio-filter/Cargo.toml    |  1 +
 crates/nuncio-filter/src/parser.rs | 20 ++++++----
 crates/nunciod/src/grpc.rs         | 59 ++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+), 7 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index a9c3920..d5cad9b 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2544,6 +2544,7 @@ dependencies = [
 name = "nuncio-filter"
 version = "0.1.0"
 dependencies = [
+ "aes-gcm",
  "arc-swap",
  "async-trait",
  "chrono",
diff --git a/crates/nuncio-filter/Cargo.toml b/crates/nuncio-filter/Cargo.toml
index 31c98f8..3eb1acb 100644
--- a/crates/nuncio-filter/Cargo.toml
+++ b/crates/nuncio-filter/Cargo.toml
@@ -26,6 +26,7 @@ chrono.workspace = true
 reqwest.workspace = true
 hex.workspace = true
 zeroize.workspace = true
+aes-gcm.workspace = true
 
 [dev-dependencies]
 tempfile.workspace = true
diff --git a/crates/nuncio-filter/src/parser.rs b/crates/nuncio-filter/src/parser.rs
index c9b7f13..b52ab2b 100644
--- a/crates/nuncio-filter/src/parser.rs
+++ b/crates/nuncio-filter/src/parser.rs
@@ -65,7 +65,7 @@ impl NsqlParser {
 
         let now = chrono::Utc::now().timestamp();
         Ok(FilterRule {
-            id: format!("rule-{}", uuid_or_simple_hash(&name_str, nsql)),
+            id: format!("rule-{}", generate_rule_id()),
             name: name_str,
             target_account,
             priority,
@@ -610,12 +610,18 @@ fn split_comma_outside_quotes(text: &str) -> Vec<String> {
     result
 }
 
-fn uuid_or_simple_hash(name: &str, nsql: &str) -> String {
-    use sha2::{Digest, Sha256};
-    let mut hasher = Sha256::new();
-    hasher.update(name.as_bytes());
-    hasher.update(nsql.as_bytes());
-    hex::encode(&hasher.finalize()[..8])
+/// Generates a fresh, collision-resistant rule id from the same CSPRNG
+/// (`aes-gcm`'s `OsRng`) that `nuncio-store` uses to source key material,
+/// rather than deriving the id from the rule's name/NSQL text. Two rules
+/// created with identical name and NSQL text MUST still get distinct ids --
+/// deriving the id from their content would make the second `CreateRule`
+/// silently overwrite the first via `save_filter_rule`'s `INSERT OR
+/// REPLACE`.
+fn generate_rule_id() -> String {
+    use aes_gcm::aead::{rand_core::RngCore, OsRng};
+    let mut bytes = [0u8; 16];
+    OsRng.fill_bytes(&mut bytes);
+    hex::encode(bytes)
 }
 
 #[cfg(test)]
diff --git a/crates/nunciod/src/grpc.rs b/crates/nunciod/src/grpc.rs
index 3d11220..b820424 100644
--- a/crates/nunciod/src/grpc.rs
+++ b/crates/nunciod/src/grpc.rs
@@ -4333,6 +4333,65 @@ mod tests {
             .is_empty());
     }
 
+    /// Two `CreateRule` calls with the SAME name AND the SAME NSQL text
+    /// must persist as two distinct rules rather than the second silently
+    /// overwriting the first. Rule ids used to be derived deterministically
+    /// from `sha256(name + nsql)`, so identical name+NSQL produced identical
+    /// ids and `save_filter_rule`'s `INSERT OR REPLACE` clobbered the first
+    /// rule with no warning.
+    #[tokio::test]
+    async fn create_rule_with_duplicate_name_and_nsql_does_not_overwrite_the_first() {
+        let (addr, _handle, _db, _engine, _dir) = spawn_filters_test_server("correct-token").await;
+
+        let mut client = FiltersClient::connect(format!("http://{addr}"))
+            .await
+            .expect("client connects");
+
+        let first = client
+            .create_rule(authed_bearer_request(CreateRuleRequest {
+                name: "Duplicate Rule".to_string(),
+                nsql: SAMPLE_RULE_NSQL.to_string(),
+                priority: 1,
+            }))
+            .await
+            .expect("first create_rule succeeds")
+            .into_inner()
+            .rule
+            .expect("response carries the rule");
+
+        let second = client
+            .create_rule(authed_bearer_request(CreateRuleRequest {
+                name: "Duplicate Rule".to_string(),
+                nsql: SAMPLE_RULE_NSQL.to_string(),
+                priority: 1,
+            }))
+            .await
+            .expect("second create_rule succeeds")
+            .into_inner()
+            .rule
+            .expect("response carries the rule");
+
+        assert_ne!(
+            first.id, second.id,
+            "identical name+NSQL must still get distinct ids"
+        );
+
+        let list_response = client
+            .list_rules(authed_bearer_request(ListRulesRequest {}))
+            .await
+            .expect("list_rules succeeds")
+            .into_inner();
+        assert_eq!(
+            list_response.rules.len(),
+            2,
+            "both rules must persist, neither overwritten"
+        );
+        let ids: std::collections::HashSet<_> =
+            list_response.rules.iter().map(|r| r.id.clone()).collect();
+        assert!(ids.contains(&first.id));
+        assert!(ids.contains(&second.id));
+    }
+
     #[tokio::test]
     async fn delete_rule_rejects_empty_id() {
         let (addr, _handle, _db, _engine, _dir) = spawn_filters_test_server("correct-token").await;
-- 
2.54.0