diff --git a/Cargo.lock b/Cargo.lock index d5cad9b..df41e19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2624,6 +2624,7 @@ dependencies = [ "thiserror 1.0.69", "tokio", "tracing", + "tracing-subscriber", "zeroize", ] diff --git a/crates/nuncio-store/Cargo.toml b/crates/nuncio-store/Cargo.toml index d60e957..6d8df27 100644 --- a/crates/nuncio-store/Cargo.toml +++ b/crates/nuncio-store/Cargo.toml @@ -28,3 +28,6 @@ hex = "0.4" hmac.workspace = true sha2.workspace = true chrono.workspace = true + +[dev-dependencies] +tracing-subscriber.workspace = true diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 8b52b76..d42a051 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -1589,14 +1589,29 @@ impl DatabaseEngine { let mut rules = Vec::new(); for (id, name, priority, enabled, nsql_text, created_at, updated_at) in rows { - if let Ok(mut parsed) = - nuncio_filter::NsqlParser::parse_rule(&name, priority as i32, &nsql_text) - { - parsed.id = id; - parsed.enabled = enabled != 0; - parsed.created_at = created_at; - parsed.updated_at = updated_at; - rules.push(parsed); + match nuncio_filter::NsqlParser::parse_rule(&name, priority as i32, &nsql_text) { + Ok(mut parsed) => { + parsed.id = id; + parsed.enabled = enabled != 0; + parsed.created_at = created_at; + parsed.updated_at = updated_at; + rules.push(parsed); + } + Err(err) => { + // A stored rule that no longer parses (e.g. the NSQL grammar + // changed, or the row was hand-edited) must not vanish from + // enforcement without a trace: that silently disables the + // rule's protection with no operator-visible signal. Warn + // loudly with the rule id and skip it rather than either + // dropping it unnoticed or failing the whole listing over + // one bad row. + tracing::warn!( + rule_id = %id, + rule_name = %name, + error = %err, + "stored filter rule failed to parse and will not be enforced" + ); + } } } Ok(rules) @@ -3180,6 +3195,84 @@ mod tests { assert_eq!(rules_after.len(), 0); } + /// A rule row that no longer re-parses (e.g. hand-edited NSQL, or a grammar + /// change that invalidates previously-stored text) must not vanish from + /// `list_filter_rules` invisibly. This asserts the observable policy chosen + /// for that case: the rule is excluded from the returned (enforced) set, but + /// a `WARN`-level log naming the specific rule id is emitted so an operator + /// can see that a rule stopped being enforced -- silent disappearance would + /// look identical to "this rule was never uploaded", which is the failure + /// mode this test guards against. + #[tokio::test] + async fn list_filter_rules_warns_on_unparseable_row_instead_of_dropping_silently() { + use std::sync::{Arc, Mutex}; + use tracing_subscriber::fmt::MakeWriter; + + #[derive(Clone, Default)] + struct CapturedLogs(Arc>>); + + impl std::io::Write for CapturedLogs { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0 + .lock() + .expect("log buffer lock") + .extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl<'a> MakeWriter<'a> for CapturedLogs { + type Writer = CapturedLogs; + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } + } + + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + // Build a structurally valid rule (so the store-level id/name/priority + // bookkeeping is untouched), then corrupt only the persisted NSQL text + // to simulate a row that no longer parses under the current grammar. + let nsql = "SELECT * FROM emails WHERE subject CONTAINS 'Spam' ACTION DELETE"; + let mut rule = nuncio_filter::NsqlParser::parse_rule("Spam Filter", 1, nsql).unwrap(); + rule.nsql_text = "THIS IS NOT VALID NSQL !!!".to_string(); + engine + .save_filter_rule(&rule) + .await + .expect("save filter rule"); + + let logs = CapturedLogs::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(logs.clone()) + .with_ansi(false) + .finish(); + + // `set_default` returns a guard that stays active across `.await` + // points as long as this test stays on the single `#[tokio::test]` + // current-thread executor, which is the default runtime flavor and + // exactly what is used here. + let guard = tracing::subscriber::set_default(subscriber); + let rules = engine.list_filter_rules().await.expect("list filter rules"); + drop(guard); + + // The unparseable row must not appear in the enforced rule set. + assert!(rules.is_empty()); + + let captured = String::from_utf8(logs.0.lock().expect("log buffer lock").clone()) + .expect("captured log is valid utf8"); + assert!( + captured.contains(&rule.id), + "expected the specific rule id to be named in the WARN log, got: {captured}" + ); + assert!( + captured.to_ascii_uppercase().contains("WARN"), + "expected a WARN-level log for the unparseable rule, got: {captured}" + ); + } + #[tokio::test] async fn test_pending_mutations_and_keyset_chunking() { let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); diff --git a/crates/nunciod/src/lib.rs b/crates/nunciod/src/lib.rs index 94daff7..7b3413b 100644 --- a/crates/nunciod/src/lib.rs +++ b/crates/nunciod/src/lib.rs @@ -35,6 +35,27 @@ pub fn default_db_path() -> std::path::PathBuf { } } +/// Loads the persisted filter rule set at daemon boot. +/// +/// A load failure here is a genuine database problem, not "no rules are +/// configured": an empty database legitimately has zero rules, but a query +/// error does not mean zero rules, it means the true rule set is unknown. +/// Collapsing that error into an empty `Vec` would silently disable all +/// filtering with nothing in the logs to explain why. This surfaces the +/// failure loudly and propagates it instead, so daemon startup fails +/// visibly rather than starting with an unknown -- possibly wide open -- +/// filtering posture. +pub async fn load_initial_filter_rules( + db: &nuncio_store::DatabaseEngine, +) -> Result, nuncio_store::DatabaseError> { + db.list_filter_rules().await.inspect_err(|err| { + tracing::error!( + error = %err, + "failed to load filter rules from database at startup; refusing to start with an unknown rule set" + ); + }) +} + /// Environment variable that opts `nunciod` in to its autonomous /// background auto-update-check loop. /// @@ -92,6 +113,38 @@ mod tests { assert!(!auto_update_task_enabled()); } + #[tokio::test] + async fn load_initial_filter_rules_propagates_db_errors_instead_of_returning_empty() { + let (engine, _dir) = nuncio_store::DatabaseEngine::connect_ephemeral() + .await + .unwrap(); + + // A pool that has been closed makes every subsequent query fail + // deterministically, standing in for a genuine boot-time database + // problem (as opposed to a healthy, merely empty, rule table). + engine.close().await; + + let result = load_initial_filter_rules(&engine).await; + assert!( + result.is_err(), + "a genuine database error must propagate as Err, not be silently \ + mapped to an empty rule set (which would look identical to a \ + database with zero rules configured)" + ); + } + + #[tokio::test] + async fn load_initial_filter_rules_returns_empty_vec_for_a_healthy_empty_database() { + let (engine, _dir) = nuncio_store::DatabaseEngine::connect_ephemeral() + .await + .unwrap(); + + let rules = load_initial_filter_rules(&engine) + .await + .expect("a healthy, empty database must load successfully"); + assert!(rules.is_empty()); + } + #[test] fn default_db_path_is_persistent_under_dot_nuncio_directory() { // Deliberately does not mutate `USERPROFILE`/`HOME` (real env vars diff --git a/crates/nunciod/src/main.rs b/crates/nunciod/src/main.rs index 21b07bd..c98aa72 100644 --- a/crates/nunciod/src/main.rs +++ b/crates/nunciod/src/main.rs @@ -36,8 +36,10 @@ async fn main() -> Result<(), Box> { // wrapper instances. let account_secrets = Arc::new(nuncio_store::vault::SecretManager::production()); - // Load active rules from SQLite - let initial_rules = db.list_filter_rules().await.unwrap_or_default(); + // Load active rules from SQLite. See `load_initial_filter_rules` for why + // a load failure must fail startup rather than silently become an empty + // rule set. + let initial_rules = nunciod::load_initial_filter_rules(&db).await?; let filter_engine = Arc::new(FilterEngine::new(initial_rules)?); // Real inbound-sync `CoreCommand` consumer. This is the ONE place that