From 1948312d40f34fca868d1ef6d6d94e165c09738c Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 12 Jan 2026 21:20:00 +0000 Subject: refactor(config): validate eagerly at startup and remove Result from runtime config methods Refactors configuration validation to fail fast on fatal errors at startup while gracefully handling recoverable issues (e.g., malformed whitelist entries). Changes: - Add Config::validate() for eager validation called immediately after load - Remove Result<> from archive_config() and repository_config() methods - WhitelistEntry::parse_whitelist() skips invalid entries with warnings - Validate relay_owner_nsec format in Config::validate() - Update all call sites to remove Result handling from config getters Benefits: - Fatal config errors (incompatible settings) fail at startup, not runtime - Recoverable errors (bad whitelist entries) logged as warnings and skipped - No Result handling scattered throughout runtime code after validation - Config methods safe to call without error handling after validate() Testing: - Add 7 new tests for validation edge cases and error handling - Total config tests: 40 (up from 33) - All 320 library tests passing Breaking change: Config users must call config.validate() after Config::load() to ensure configuration is valid. This is enforced in main.rs. --- src/config.rs | 261 +++++++++++++++++++++++++++++++++++++-------------- src/http/nip11.rs | 33 ++----- src/main.rs | 9 +- src/nostr/builder.rs | 34 ++++--- src/nostr/events.rs | 12 +-- 5 files changed, 230 insertions(+), 119 deletions(-) (limited to 'src') diff --git a/src/config.rs b/src/config.rs index 8a9eb4d..37b1c1e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -85,16 +85,25 @@ impl WhitelistEntry { } /// Parse whitelist from comma-separated string - pub fn parse_whitelist(input: &str) -> Result> { + /// + /// Skips invalid entries with warnings instead of failing. + /// This allows the config to load even if some whitelist entries are malformed. + pub fn parse_whitelist(input: &str) -> Vec { if input.trim().is_empty() { - return Ok(Vec::new()); + return Vec::new(); } input .split(',') .map(|s| s.trim()) .filter(|s| !s.is_empty()) - .map(Self::parse) + .filter_map(|s| match Self::parse(s) { + Ok(entry) => Some(entry), + Err(e) => { + tracing::warn!("Skipping invalid whitelist entry '{}': {}", s, e); + None + } + }) .collect() } } @@ -464,23 +473,60 @@ impl Config { } } + /// Validate configuration and return fatal errors + /// + /// This should be called immediately after Config::load() to fail fast on config errors. + /// Recoverable issues (e.g., malformed whitelist entries) are logged as warnings and skipped. + pub fn validate(&self) -> Result<()> { + // Validate relay owner nsec (should always be set by Config::load()) + let nsec = self + .relay_owner_nsec + .as_ref() + .context("relay_owner_nsec not set (should be set by Config::load())")?; + Keys::parse(nsec).context("Invalid relay_owner_nsec format")?; + + // Validate archive configuration + let archive_whitelist = WhitelistEntry::parse_whitelist(&self.archive_whitelist); + let archive_enabled = self.archive_all || !archive_whitelist.is_empty(); + + // Fatal error: archive_read_only=true without archive mode enabled + if let Some(true) = self.archive_read_only { + if !archive_enabled { + return Err(anyhow!( + "NGIT_ARCHIVE_READ_ONLY=true requires either NGIT_ARCHIVE_ALL=true or NGIT_ARCHIVE_WHITELIST to be set" + )); + } + } + + // Validate repository whitelist configuration + let repository_whitelist = WhitelistEntry::parse_whitelist(&self.repository_whitelist); + + // Fatal error: repository_whitelist with archive_read_only=true (incompatible) + if !repository_whitelist.is_empty() { + let read_only = self.archive_read_only.unwrap_or(archive_enabled); + if read_only { + return Err(anyhow!( + "NGIT_REPOSITORY_WHITELIST cannot be used with NGIT_ARCHIVE_READ_ONLY=true. \ + Archive read-only mode rejects announcements that don't match the archive whitelist, \ + regardless of service listing. Either set NGIT_ARCHIVE_READ_ONLY=false or use \ + NGIT_ARCHIVE_WHITELIST instead of NGIT_REPOSITORY_WHITELIST." + )); + } + } + + Ok(()) + } + /// Get parsed archive configuration with computed read-only mode /// /// Read-only mode defaults to true if archive mode is enabled, false otherwise. - /// Throws error if explicitly set to true without archive mode enabled. - pub fn archive_config(&self) -> Result { - let whitelist = WhitelistEntry::parse_whitelist(&self.archive_whitelist)?; + /// This method assumes config has been validated - call Config::validate() first! + pub fn archive_config(&self) -> ArchiveConfig { + let whitelist = WhitelistEntry::parse_whitelist(&self.archive_whitelist); let archive_enabled = self.archive_all || !whitelist.is_empty(); let read_only = match self.archive_read_only { - Some(true) => { - if !archive_enabled { - return Err(anyhow!( - "NGIT_ARCHIVE_READ_ONLY=true requires either NGIT_ARCHIVE_ALL=true or NGIT_ARCHIVE_WHITELIST to be set" - )); - } - true - } + Some(true) => true, // Already validated in validate() Some(false) => false, None => { // Default: true if archive mode enabled, false otherwise @@ -488,33 +534,19 @@ impl Config { } }; - Ok(ArchiveConfig { + ArchiveConfig { archive_all: self.archive_all, whitelist, read_only, - }) + } } /// Get parsed repository whitelist configuration /// - /// Throws error if repository_whitelist is set together with archive_read_only=true - pub fn repository_config(&self) -> Result { - let whitelist = WhitelistEntry::parse_whitelist(&self.repository_whitelist)?; - - // Validate incompatible configurations - if !whitelist.is_empty() { - let archive_config = self.archive_config()?; - if archive_config.read_only { - return Err(anyhow!( - "NGIT_REPOSITORY_WHITELIST cannot be used with NGIT_ARCHIVE_READ_ONLY=true. \ - Archive read-only mode rejects announcements that don't match the archive whitelist, \ - regardless of service listing. Either set NGIT_ARCHIVE_READ_ONLY=false or use \ - NGIT_ARCHIVE_WHITELIST instead of NGIT_REPOSITORY_WHITELIST." - )); - } - } - - Ok(RepositoryConfig { whitelist }) + /// This method assumes config has been validated - call Config::validate() first! + pub fn repository_config(&self) -> RepositoryConfig { + let whitelist = WhitelistEntry::parse_whitelist(&self.repository_whitelist); + RepositoryConfig { whitelist } } /// Create config for testing @@ -807,10 +839,10 @@ mod tests { #[test] fn test_parse_whitelist_empty() { - let whitelist = WhitelistEntry::parse_whitelist("").unwrap(); + let whitelist = WhitelistEntry::parse_whitelist(""); assert!(whitelist.is_empty()); - let whitelist = WhitelistEntry::parse_whitelist(" ").unwrap(); + let whitelist = WhitelistEntry::parse_whitelist(" "); assert!(whitelist.is_empty()); } @@ -823,11 +855,21 @@ mod tests { let whitelist = WhitelistEntry::parse_whitelist(&format!( "{},bitcoin-core,{}/linux", test_npub1, test_npub2 - )) - .unwrap(); + )); assert_eq!(whitelist.len(), 3); } + #[test] + fn test_parse_whitelist_invalid_npub_skipped() { + // Invalid entries should be skipped with warnings, not fail + let whitelist = WhitelistEntry::parse_whitelist("npub1invalid,bitcoin-core"); + assert_eq!(whitelist.len(), 1); // Only bitcoin-core should be parsed + assert!(matches!( + &whitelist[0], + WhitelistEntry::Identifier(id) if id == "bitcoin-core" + )); + } + #[test] fn test_archive_config_parsing() { let keys = Keys::generate(); @@ -836,31 +878,22 @@ mod tests { archive_whitelist: format!("{},bitcoin-core", test_npub), ..Config::for_testing() }; - let archive_config = config.archive_config().unwrap(); + let archive_config = config.archive_config(); assert_eq!(archive_config.whitelist.len(), 2); } - #[test] - fn test_archive_config_invalid_npub() { - let config = Config { - archive_whitelist: "npub1invalid".to_string(), - ..Config::for_testing() - }; - assert!(config.archive_config().is_err()); - } - #[test] fn test_archive_read_only_defaults() { // Default: false when no archive mode let config = Config::for_testing(); - assert_eq!(config.archive_config().unwrap().read_only, false); + assert_eq!(config.archive_config().read_only, false); // Default: true when archive_all is set let config = Config { archive_all: true, ..Config::for_testing() }; - assert_eq!(config.archive_config().unwrap().read_only, true); + assert_eq!(config.archive_config().read_only, true); // Default: true when archive_whitelist is set let keys = Keys::generate(); @@ -869,7 +902,7 @@ mod tests { archive_whitelist: test_npub, ..Config::for_testing() }; - assert_eq!(config.archive_config().unwrap().read_only, true); + assert_eq!(config.archive_config().read_only, true); } #[test] @@ -880,7 +913,7 @@ mod tests { archive_read_only: Some(true), ..Config::for_testing() }; - assert_eq!(config.archive_config().unwrap().read_only, true); + assert_eq!(config.archive_config().read_only, true); // Explicit false with archive_all (unusual but allowed) let config = Config { @@ -888,29 +921,26 @@ mod tests { archive_read_only: Some(false), ..Config::for_testing() }; - assert_eq!(config.archive_config().unwrap().read_only, false); + assert_eq!(config.archive_config().read_only, false); // Explicit false without archive mode let config = Config { archive_read_only: Some(false), ..Config::for_testing() }; - assert_eq!(config.archive_config().unwrap().read_only, false); + assert_eq!(config.archive_config().read_only, false); } #[test] - fn test_archive_read_only_error() { - // Error: true without archive mode + fn test_archive_read_only_validation_error() { + // Error: true without archive mode should fail validation let config = Config { archive_read_only: Some(true), ..Config::for_testing() }; - assert!(config.archive_config().is_err()); - assert!(config - .archive_config() - .unwrap_err() - .to_string() - .contains("requires either")); + let result = config.validate(); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("requires either")); } #[test] @@ -921,7 +951,7 @@ mod tests { repository_whitelist: format!("{},bitcoin-core", test_npub), ..Config::for_testing() }; - let repo_config = config.repository_config().unwrap(); + let repo_config = config.repository_config(); assert_eq!(repo_config.whitelist.len(), 2); assert!(repo_config.enabled()); } @@ -929,13 +959,13 @@ mod tests { #[test] fn test_repository_whitelist_empty() { let config = Config::for_testing(); - let repo_config = config.repository_config().unwrap(); + let repo_config = config.repository_config(); assert!(repo_config.whitelist.is_empty()); assert!(!repo_config.enabled()); } #[test] - fn test_repository_whitelist_incompatible_with_archive_read_only() { + fn test_repository_whitelist_validation_incompatible_with_archive_read_only() { let keys = Keys::generate(); let test_npub = keys.public_key().to_bech32().unwrap(); let config = Config { @@ -944,7 +974,7 @@ mod tests { repository_whitelist: test_npub, ..Config::for_testing() }; - let result = config.repository_config(); + let result = config.validate(); assert!(result.is_err()); let err = result.unwrap_err().to_string(); assert!(err.contains("cannot be used with")); @@ -961,8 +991,8 @@ mod tests { repository_whitelist: test_npub, ..Config::for_testing() }; - // Should not error - assert!(config.repository_config().is_ok()); + // Should not error on validation + assert!(config.validate().is_ok()); } #[test] @@ -980,4 +1010,99 @@ mod tests { assert!(config.matches("npub1bob", "bitcoin-core")); assert!(!config.matches("npub1bob", "other-repo")); } + + #[test] + fn test_validate_success_with_valid_config() { + // Valid config should pass validation + let keys = Keys::generate(); + let test_npub = keys.public_key().to_bech32().unwrap(); + let config = Config { + archive_whitelist: format!("{},bitcoin-core", test_npub), + archive_read_only: Some(false), + repository_whitelist: "rust".to_string(), + ..Config::for_testing() + }; + assert!(config.validate().is_ok()); + } + + #[test] + fn test_validate_with_all_invalid_whitelist_entries() { + // All invalid entries should be skipped with warnings, but validation should succeed + let config = Config { + archive_whitelist: "npub1invalid,npub1bad,npub1wrong".to_string(), + ..Config::for_testing() + }; + assert!(config.validate().is_ok()); + // All entries should be skipped + let archive_config = config.archive_config(); + assert_eq!(archive_config.whitelist.len(), 0); + assert!(!archive_config.enabled()); + } + + #[test] + fn test_validate_with_mixed_valid_invalid_entries() { + let keys = Keys::generate(); + let test_npub = keys.public_key().to_bech32().unwrap(); + // Mixed valid and invalid entries - should keep valid ones + let config = Config { + repository_whitelist: format!("npub1invalid,{},bitcoin-core,npub1bad", test_npub), + ..Config::for_testing() + }; + assert!(config.validate().is_ok()); + let repo_config = config.repository_config(); + // Should have 2 valid entries: the test_npub and bitcoin-core + assert_eq!(repo_config.whitelist.len(), 2); + } + + #[test] + fn test_whitelist_entry_with_extra_whitespace() { + let keys = Keys::generate(); + let test_npub = keys.public_key().to_bech32().unwrap(); + // Whitespace should be trimmed + let whitelist = + WhitelistEntry::parse_whitelist(&format!(" {} , bitcoin-core , rust ", test_npub)); + assert_eq!(whitelist.len(), 3); + } + + #[test] + fn test_archive_config_with_all_invalid_entries_not_enabled() { + // If all whitelist entries are invalid, archive mode should not be enabled + let config = Config { + archive_whitelist: "npub1invalid,npub1bad".to_string(), + ..Config::for_testing() + }; + let archive_config = config.archive_config(); + assert!(!archive_config.enabled()); + assert_eq!(archive_config.whitelist.len(), 0); + } + + #[test] + fn test_validate_detects_invalid_relay_owner_nsec() { + // Invalid nsec should fail validation + let config = Config { + relay_owner_nsec: Some("nsec1invalid".to_string()), + ..Config::for_testing() + }; + let result = config.validate(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("Invalid relay_owner_nsec")); + } + + #[test] + fn test_validate_requires_relay_owner_nsec() { + // Missing nsec should fail validation + let config = Config { + relay_owner_nsec: None, + ..Config::for_testing() + }; + let result = config.validate(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("relay_owner_nsec not set")); + } } diff --git a/src/http/nip11.rs b/src/http/nip11.rs index ff7b8df..7c58175 100644 --- a/src/http/nip11.rs +++ b/src/http/nip11.rs @@ -56,16 +56,10 @@ pub struct RelayInformationDocument { impl RelayInformationDocument { /// Create NIP-11 relay information document from configuration pub fn from_config(config: &Config) -> Self { - // Determine if archive mode is enabled - let archive_config = config.archive_config().ok(); - let archive_enabled = archive_config - .as_ref() - .map(|ac| ac.enabled()) - .unwrap_or(false); - let archive_read_only = archive_config - .as_ref() - .map(|ac| ac.read_only) - .unwrap_or(false); + // Get validated configuration (config.validate() must be called at startup) + let archive_config = config.archive_config(); + let archive_enabled = archive_config.enabled(); + let archive_read_only = archive_config.read_only; // Build supported_grasps list let mut supported_grasps = vec!["GRASP-01".to_string()]; @@ -75,22 +69,15 @@ impl RelayInformationDocument { supported_grasps.push("GRASP-02".to_string()); // Build curation field for archive read-only mode or repository whitelist - let repository_config = config.repository_config().ok(); - let repository_whitelist_enabled = repository_config - .as_ref() - .map(|rc| rc.enabled()) - .unwrap_or(false); + let repository_config = config.repository_config(); + let repository_whitelist_enabled = repository_config.enabled(); let curation = if archive_read_only { // Archive read-only mode (GRASP-05 only) - if let Some(ref ac) = archive_config { - if ac.archive_all { - Some("Read-only sync of all repositories found on network".to_string()) - } else if !ac.whitelist.is_empty() { - Some("Read-only sync of whitelisted repositories and maintainers".to_string()) - } else { - None - } + if archive_config.archive_all { + Some("Read-only sync of all repositories found on network".to_string()) + } else if !archive_config.whitelist.is_empty() { + Some("Read-only sync of whitelisted repositories and maintainers".to_string()) } else { None } diff --git a/src/main.rs b/src/main.rs index 8b959a6..a6f1d9d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,7 +28,14 @@ async fn main() -> Result<()> { // Load configuration (priority: CLI flags > env vars > .env file > defaults) let config = Config::load()?; - info!("Configuration loaded: {}", config.bind_address); + // Validate configuration and fail fast on fatal errors + // Recoverable issues (e.g., malformed whitelist entries) are logged as warnings + config.validate()?; + + info!( + "Configuration loaded and validated: {}", + config.bind_address + ); info!("Domain: {}", config.domain); info!("Relay name: {}", config.relay_name()); info!("Git data directory: {}", config.effective_git_data_path()); diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index 10f7648..9819e37 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -567,26 +567,24 @@ pub async fn create_relay( // Clone Arc for the write policy so both relay and policy can access the database let git_data_path = config.effective_git_data_path(); - // Parse and log archive configuration - if let Ok(archive_config) = config.archive_config() { - if archive_config.enabled() { - tracing::info!( - "GRASP-05 archive mode enabled: archive_all={}, whitelist_entries={}, read_only={}", - archive_config.archive_all, - archive_config.whitelist.len(), - archive_config.read_only - ); - } + // Log archive configuration (config.validate() must be called at startup) + let archive_config = config.archive_config(); + if archive_config.enabled() { + tracing::info!( + "GRASP-05 archive mode enabled: archive_all={}, whitelist_entries={}, read_only={}", + archive_config.archive_all, + archive_config.whitelist.len(), + archive_config.read_only + ); } - // Parse and log repository configuration - if let Ok(repository_config) = config.repository_config() { - if repository_config.enabled() { - tracing::info!( - "Repository whitelist enabled: whitelist_entries={}", - repository_config.whitelist.len() - ); - } + // Log repository configuration + let repository_config = config.repository_config(); + if repository_config.enabled() { + tracing::info!( + "Repository whitelist enabled: whitelist_entries={}", + repository_config.whitelist.len() + ); } // Create write policy with purgatory integration diff --git a/src/nostr/events.rs b/src/nostr/events.rs index 3ec075d..3b4ef25 100644 --- a/src/nostr/events.rs +++ b/src/nostr/events.rs @@ -400,15 +400,9 @@ pub fn validate_announcement( Err(e) => return AnnouncementResult::Reject(format!("Invalid announcement: {}", e)), }; - // Get archive and repository configs (fail-secure: reject on config errors) - let archive_config = match config.archive_config() { - Ok(c) => c, - Err(e) => return AnnouncementResult::Reject(format!("Config error: {}", e)), - }; - let repository_config = match config.repository_config() { - Ok(c) => c, - Err(e) => return AnnouncementResult::Reject(format!("Config error: {}", e)), - }; + // Get validated configs (config.validate() must be called at startup) + let archive_config = config.archive_config(); + let repository_config = config.repository_config(); let npub = announcement.owner_npub(); let lists_service = announcement.lists_service(&config.domain); -- cgit v1.2.3