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,47 +473,38 @@ impl Config { } } - /// Get parsed archive configuration with computed read-only mode + /// Validate configuration and return fatal errors /// - /// 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)?; - let archive_enabled = self.archive_all || !whitelist.is_empty(); + /// 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")?; - 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 + // 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" + )); } - Some(false) => false, - None => { - // Default: true if archive mode enabled, false otherwise - archive_enabled - } - }; + } - Ok(ArchiveConfig { - archive_all: self.archive_all, - whitelist, - read_only, - }) - } + // Validate repository whitelist configuration + let repository_whitelist = WhitelistEntry::parse_whitelist(&self.repository_whitelist); - /// 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 { + // 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, \ @@ -514,7 +514,39 @@ impl Config { } } - Ok(RepositoryConfig { whitelist }) + Ok(()) + } + + /// Get parsed archive configuration with computed read-only mode + /// + /// Read-only mode defaults to true if archive mode is enabled, false otherwise. + /// 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) => true, // Already validated in validate() + Some(false) => false, + None => { + // Default: true if archive mode enabled, false otherwise + archive_enabled + } + }; + + ArchiveConfig { + archive_all: self.archive_all, + whitelist, + read_only, + } + } + + /// Get parsed repository whitelist configuration + /// + /// 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);