diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c170d3..3baa42f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,12 @@ - Reject invalid APNs `environment` configuration values at startup instead of silently routing pushes to the sandbox gateway; only `production` and `sandbox` are accepted ([#143](https://github.com/marmot-protocol/transponder/pull/143)). - Limit each notification event to at most 100 encrypted tokens before base64 decoding, preventing oversized events from forcing unbounded token blob allocation and rate-limit work ([#38](https://github.com/marmot-protocol/transponder/pull/38)). +- Validate configuration at load time so misconfiguration fails loudly at startup with a named-field error instead of silently coercing or detonating later: `server.max_dedup_cache_size`, `server.max_rate_limit_cache_size`, and `server.max_tokens_per_event` now reject `0`; `health.bind_address` must parse as a socket address; `logging.format` accepts only `json`/`pretty` (unknown values, including `off`, are rejected — silence console logs with `logging.level = "off"`); and `relays.onion` entries must be `ws://`/`wss://` URLs with a `.onion` host. Change-detection for the kind-10050 inbox relay list now normalizes URLs through `RelayUrl` and selects the newest event by `created_at`, avoiding spurious republishes. ### Changed - Updated MIP-05 token handling for the expanded 1084-byte encrypted token format and variable-length APNs/FCM device tokens introduced in [marmot-protocol/mdk#254](https://github.com/marmot-protocol/mdk/pull/254) ([#40](https://github.com/marmot-protocol/transponder/pull/40)). +- Centralized configuration defaults on the serde `default_*` functions as the single source of truth, removing the parallel `set_default` ladder that could silently drift out of sync. ### Security @@ -23,3 +25,5 @@ - Changed the default health server bind address to localhost and documented internal-only exposure for the unauthenticated health, readiness, and metrics endpoints ([#39](https://github.com/marmot-protocol/transponder/pull/39)). - Redacted FCM service account private keys from debug output and zeroized the service account JSON buffer after loading [#35](https://github.com/marmot-protocol/transponder/pull/35) - Zeroized the server Nostr private key in config state and shortened the lifetime of resolved key material during startup ([#37](https://github.com/marmot-protocol/transponder/pull/37)). +- Refused to start when the server `private_key_file` grants group or other read access (mode `& 0o077`), mirroring the `0600` the `generate-keys` write path enforces, so an accidentally world-readable key file is rejected instead of loaded silently. +- Kept the inline server private key (`server.private_key` / `TRANSPONDER_SERVER_PRIVATE_KEY`) out of the `config` crate's un-zeroized `Value` tree by resolving it through a dedicated `Zeroizing` path before the rest of the configuration is parsed. diff --git a/README.md b/README.md index 3f9eb5d..808bd00 100644 --- a/README.md +++ b/README.md @@ -49,11 +49,16 @@ Transponder uses TOML configuration files with environment variable overrides. T ```toml [server] -# Server's Nostr private key in hex format (64 hex characters) -# Prefer private_key_file for production secret handling. +# Server's Nostr private key in hex format (64 hex characters). +# Prefer private_key_file for production: it keeps the secret out of the config +# file and lets the server enforce file permissions on it. An inline value here +# (or in TRANSPONDER_SERVER_PRIVATE_KEY) is read through a dedicated zeroizing +# path that never stores it in the config parser's un-zeroized buffers. private_key = "" # Alternative: path to a file containing the private key. +# The file must be mode 0600 (not group/world readable) or the server refuses +# to start, mirroring the 0600 that generate-keys writes. # Generate with: transponder generate-keys --output /path/to/server.key # private_key_file = "/run/secrets/transponder_private_key" @@ -61,8 +66,8 @@ private_key = "" shutdown_timeout_secs = 10 # Volatile event deduplication cache size when durable replay state is disabled -# (default: 100000). With dedup_state_path set, all terminal event IDs inside -# dedup_retention_secs are retained for the full NIP-59 subscription lookback. +# (default: 100000; must be >= 1). With dedup_state_path set, all terminal event +# IDs inside dedup_retention_secs are retained for the full NIP-59 lookback. # max_dedup_cache_size = 100000 # Optional durable replay state for processed gift-wrap event IDs. @@ -75,7 +80,9 @@ shutdown_timeout_secs = 10 # max_notification_age_secs = 3600 # max_notification_future_skew_secs = 300 -# Rate limiting to prevent spam and replay attacks +# Rate limiting to prevent spam and replay attacks. Size fields must be >= 1; +# a value of 0 is rejected at startup (it previously either silently swapped in +# the default or rejected every event). # max_rate_limit_cache_size = 100000 # Tracked keys per limiter, not total timestamps # max_tokens_per_event = 100 # Per notification event # encrypted_token_rate_limit_per_minute = 240 # Per encrypted token (replay protection) @@ -100,7 +107,10 @@ clearnet = [ allow_unencrypted_clearnet_relays = false # Tor/onion relays (optional) -# Requires a build with `--features tor` and a host that can support Tor traffic +# Requires a build with `--features tor` and a host that can support Tor traffic. +# Each entry must be a ws:// or wss:// URL with a .onion host; a clearnet or +# malformed entry here is rejected at startup instead of silently degrading to +# clearnet. onion = [] # Reconnection settings @@ -147,23 +157,29 @@ service_account_path = "" project_id = "" [health] -# Enable the health check HTTP server +# Enable the /health and /ready endpoints enabled = true -# Address and port to bind the health server to -# Keep this on localhost unless an internal proxy, VPN, or load balancer needs it +# Address and port to bind the health/metrics listener to. Must be a valid +# IP:port socket address (a hostname like "localhost:8080" or an out-of-range +# port is rejected at startup). Keep this on localhost unless an internal proxy, +# VPN, or load balancer needs it. bind_address = "127.0.0.1:8080" [metrics] -# Whether Prometheus metrics are enabled -# Metrics are exposed at /metrics on the health server port +# Whether Prometheus metrics are enabled. +# /metrics is served on health.bind_address whenever metrics are enabled, even +# if the health endpoints (health.enabled) are disabled. enabled = true [logging] -# Log level: "trace", "debug", "info", "warn", "error", "off" +# Log level: "trace", "debug", "info", "warn", "error", "off". +# level = "off" silences all console output while keeping the subscriber active. level = "info" -# Log format: "json" (structured, for production) or "pretty" (human-readable) +# Log format: "json" (structured, for production) or "pretty" (human-readable). +# Only "json" or "pretty" are accepted; any other value (including "off") is +# rejected at startup. To silence logs, set level = "off". format = "json" [glitchtip] diff --git a/config/default.toml b/config/default.toml index 13e7724..4eb8717 100644 --- a/config/default.toml +++ b/config/default.toml @@ -2,12 +2,20 @@ # MIP-05 Push Notification Server [server] -# Server's Nostr private key (hex format) -# Prefer private_key_file for production secret handling. +# Server's Nostr private key (hex format). +# +# Prefer private_key_file for production. An inline private_key here (or in +# TRANSPONDER_SERVER_PRIVATE_KEY) is read through a dedicated zeroizing path so +# it never lands in the config parser's un-zeroized buffers, but a file keeps +# the secret out of the config file entirely and lets the loader enforce +# permissions on it. private_key = "" # Alternative: path to a file containing the private key. # Useful with Docker secrets or other mounted secret files. +# The file must NOT be group- or world-readable (mode 0600): the server refuses +# to start if it grants group/other access, mirroring the 0600 that +# generate-keys writes. # Generate with: transponder generate-keys --output /run/secrets/transponder_private_key # private_key_file = "/run/secrets/transponder_private_key" @@ -20,6 +28,8 @@ shutdown_timeout_secs = 10 # Applies only when dedup_state_path is unset. Durable replay state retains all # terminal event IDs inside dedup_retention_secs so restart/reconnect replay # suppression covers the full NIP-59 subscription lookback window. +# Must be >= 1; a value of 0 is rejected at startup (it was previously swapped +# for the default silently). # Default: 100000 # max_dedup_cache_size = 100000 @@ -47,10 +57,14 @@ shutdown_timeout_secs = 10 # Transponder has two such limiters. # At capacity, new keys evict stale or below-limit entries when possible. # If no safe eviction candidate exists, the new key is rate limited. +# Must be >= 1; a value of 0 is rejected at startup (it was previously swapped +# for the 100000 default silently, per limiter). # Default: 100000 entries per cache # max_rate_limit_cache_size = 100000 # Maximum encrypted tokens accepted in a single notification event. +# Must be >= 1; a value of 0 is rejected at startup (it would reject every +# notification event — a total, silent push outage). # Default: 100 # max_tokens_per_event = 100 @@ -105,6 +119,9 @@ allow_unencrypted_clearnet_relays = false # Tor/onion relays (optional) # Requires a binary built with: cargo build --features tor +# Each entry must be a ws:// or wss:// URL with a .onion host; a plaintext +# clearnet URL or a malformed .onion entry here is rejected at startup rather +# than silently degrading to clearnet. onion = [] # Reconnection settings. reconnect_interval_secs must be between 1 and 300 seconds. @@ -156,6 +173,9 @@ project_id = "" enabled = true # Address to bind the health/metrics listener to. +# Must be a valid IP:port socket address (e.g. "127.0.0.1:8080"); a hostname +# like "localhost:8080" or an out-of-range port is rejected at startup so a +# typo cannot silently disable /health, /ready, and /metrics. # Keep this on localhost unless an internal proxy, VPN, or load balancer needs # access to the endpoint. bind_address = "127.0.0.1:8080" @@ -167,10 +187,14 @@ bind_address = "127.0.0.1:8080" enabled = true [logging] -# Log level: "trace", "debug", "info", "warn", "error", "off" +# Log level: "trace", "debug", "info", "warn", "error", "off". +# Use level = "off" to silence all console output; it keeps the tracing +# subscriber installed so error reporting and runtime filter changes still work. level = "info" -# Log format: "json" (for production) or "pretty" (for development) +# Log format: "json" (for production) or "pretty" (for development). +# Only "json" and "pretty" are accepted; any other value (including "off") is +# rejected at startup. To silence logs, use logging.level = "off". format = "json" [glitchtip] diff --git a/config/production.toml.example b/config/production.toml.example index 0661fa0..3d91438 100644 --- a/config/production.toml.example +++ b/config/production.toml.example @@ -2,7 +2,9 @@ # Copy this to config/production.toml and adjust for your deployment. [server] -# Recommended for production: supply the key via +# Recommended for production: supply the key via a permission-scoped file +# (mode 0600). The server refuses to start if the key file is group/world +# readable. # TRANSPONDER_SERVER_PRIVATE_KEY_FILE=/run/secrets/transponder_private_key # Generate with: transponder generate-keys --output /run/secrets/transponder_private_key private_key = "" @@ -50,6 +52,8 @@ allow_unencrypted_clearnet_relays = false # Add onion relays only if you intend to run a Tor-enabled build and can spare # additional memory/CPU for Tor circuits. +# Each entry must be a ws:// or wss:// URL with a .onion host; malformed or +# clearnet entries here are rejected at startup. # Build with: docker build --build-arg CARGO_FEATURES='--features tor' -t transponder:local . onion = [] @@ -75,6 +79,7 @@ project_id = "" [health] enabled = true +# Must be a valid IP:port socket address (rejected at startup otherwise). # Keep native deployments bound to localhost by default. For Docker Compose, # compose.prod.yml overrides this to 0.0.0.0 inside the container while # publishing the host port on 127.0.0.1 only. @@ -84,7 +89,10 @@ bind_address = "127.0.0.1:8080" enabled = true [logging] +# level = "off" silences all console output (subscriber stays installed). level = "info" +# Only "json" or "pretty"; any other value (including "off") is rejected at +# startup. Silence logs with level = "off", not format. format = "json" [glitchtip] diff --git a/src/config.rs b/src/config.rs index ef073a4..1b1358c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -12,12 +12,27 @@ //! ignored rather than treated as errors, so ambient service-discovery //! variables injected by Kubernetes and Docker (e.g. `TRANSPONDER_SERVICE_HOST`, //! `TRANSPONDER_PORT_8080_TCP`) do not abort startup. - -use config::{Config, ConfigBuilder, File, builder::DefaultState}; +//! +//! Defaults live in exactly one place: the serde `default_*` functions on the +//! section structs (backed by the shared `DEFAULT_*` consts). The `config` +//! crate builder only seeds each section as an empty table so `try_deserialize` +//! reaches those serde defaults even when a section is absent from every +//! source. +//! +//! The server private key is special-cased for secret hygiene: an inline +//! `server.private_key` (TOML) or `TRANSPONDER_SERVER_PRIVATE_KEY` (env) value +//! is extracted through a dedicated [`Zeroizing`] path *before* the remaining +//! configuration is handed to the `config` crate, so the secret never sits in +//! the crate's un-zeroized `Value` tree. Prefer `server.private_key_file` in +//! production; the file must not be group/other readable. + +use config::{Config, ConfigBuilder, File, FileFormat, builder::DefaultState}; use serde::{Deserialize, Deserializer}; use std::{ env, ffi::OsString, + fs, + net::SocketAddr, path::{Path, PathBuf}, }; use tokio::sync::Semaphore; @@ -124,7 +139,13 @@ fn default_rate_limit_per_hour() -> u32 { #[derive(Clone, Deserialize)] pub struct ServerConfig { /// Server's Nostr private key (hex or nsec format). - #[serde(deserialize_with = "deserialize_zeroizing_string")] + /// + /// Never deserialized from the `config` crate's `Value` tree: the inline + /// TOML value and the `TRANSPONDER_SERVER_PRIVATE_KEY` env override are + /// extracted through a dedicated [`Zeroizing`] path in the load functions + /// and assigned here afterwards, so the secret never sits in the crate's + /// un-zeroized intermediate buffers. Prefer `private_key_file`. + #[serde(skip_deserializing, default = "default_private_key")] pub private_key: Zeroizing, /// Path to a file containing the server's Nostr private key. @@ -300,13 +321,8 @@ fn default_shutdown_timeout() -> u64 { 10 } -fn deserialize_zeroizing_string<'de, D>( - deserializer: D, -) -> std::result::Result, D::Error> -where - D: Deserializer<'de>, -{ - String::deserialize(deserializer).map(Zeroizing::new) +fn default_private_key() -> Zeroizing { + Zeroizing::new(String::new()) } /// Relay connection configuration. @@ -539,17 +555,61 @@ pub struct LoggingConfig { #[serde(default = "default_log_level")] pub level: String, - /// Log format: "json" or "pretty". + /// Log format: [`LogFormat::Json`] or [`LogFormat::Pretty`]. #[serde(default = "default_log_format")] - pub format: String, + pub format: LogFormat, +} + +/// Console log output format. +/// +/// Unknown values — including `"off"` — are rejected at config load. Console +/// logging is silenced with `logging.level = "off"`, which keeps a tracing +/// subscriber installed (so runtime filter changes and error reporting keep +/// working) instead of dropping all output unrecoverably. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum LogFormat { + /// Structured JSON output for production log aggregation. + #[default] + Json, + + /// Human-readable output for development. + Pretty, +} + +impl<'de> Deserialize<'de> for LogFormat { + // Hand-written instead of derived so the error for the plausible-but-wrong + // `format = "off"` (and any typo) names the field and points at + // `logging.level = "off"`, the supported way to silence console logs. + fn deserialize(deserializer: D) -> std::result::Result + where + D: Deserializer<'de>, + { + let value = String::deserialize(deserializer)?; + match value.as_str() { + "json" => Ok(Self::Json), + "pretty" => Ok(Self::Pretty), + other => Err(serde::de::Error::custom(format!( + "logging.format must be \"json\" or \"pretty\", got \"{other}\"; to silence console logs set logging.level = \"off\"" + ))), + } + } +} + +impl std::fmt::Display for LogFormat { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Json => f.write_str("json"), + Self::Pretty => f.write_str("pretty"), + } + } } fn default_log_level() -> String { "info".to_string() } -fn default_log_format() -> String { - "json".to_string() +fn default_log_format() -> LogFormat { + LogFormat::Json } /// GlitchTip (Sentry-compatible) error-reporting configuration. @@ -597,98 +657,50 @@ fn default_glitchtip_environment() -> String { "production".to_string() } +/// Top-level configuration sections. +/// +/// Seeded as empty tables so `try_deserialize` reaches every field's serde +/// `default_*` fn even when a section is absent from all sources. +const CONFIG_SECTIONS: &[&str] = &[ + "server", + "relays", + "apns", + "fcm", + "health", + "metrics", + "logging", + "glitchtip", +]; + +/// Build the base config with each section seeded as an empty table. +/// +/// Deliberately sets **no default values**: the serde `default_*` fns on the +/// section structs are the single source of defaults (see the module docs). +/// Without these structural seeds, a config source that omits a whole section +/// would fail deserialization with `missing field` instead of using defaults. fn base_config_builder() -> Result> { - Ok(Config::builder() - // Start with default values - .set_default("server.private_key", "")? - .set_default("server.private_key_file", "")? - .set_default("server.shutdown_timeout_secs", 10)? - .set_default( - "server.max_dedup_cache_size", - DEFAULT_MAX_DEDUP_CACHE_SIZE as i64, - )? - .set_default("server.dedup_state_path", "")? - .set_default( - "server.dedup_retention_secs", - DEFAULT_DEDUP_RETENTION_SECS as i64, - )? - .set_default( - "server.max_notification_age_secs", - DEFAULT_MAX_NOTIFICATION_AGE_SECS as i64, - )? - .set_default( - "server.max_notification_future_skew_secs", - DEFAULT_MAX_NOTIFICATION_FUTURE_SKEW_SECS as i64, - )? - .set_default( - "server.max_rate_limit_cache_size", - DEFAULT_MAX_RATE_LIMIT_CACHE_SIZE as i64, - )? - .set_default( - "server.max_tokens_per_event", - DEFAULT_MAX_TOKENS_PER_EVENT as i64, - )? - .set_default( - "server.encrypted_token_rate_limit_per_minute", - DEFAULT_RATE_LIMIT_PER_MINUTE as i64, - )? - .set_default( - "server.encrypted_token_rate_limit_per_hour", - DEFAULT_RATE_LIMIT_PER_HOUR as i64, - )? - .set_default( - "server.device_token_rate_limit_per_minute", - DEFAULT_RATE_LIMIT_PER_MINUTE as i64, - )? - .set_default( - "server.device_token_rate_limit_per_hour", - DEFAULT_RATE_LIMIT_PER_HOUR as i64, - )? - .set_default( - "server.max_concurrent_event_processing", - DEFAULT_MAX_CONCURRENT_EVENT_PROCESSING as i64, - )? - .set_default( - "server.global_unwrap_rate_limit_per_minute", - DEFAULT_GLOBAL_UNWRAP_LIMIT_PER_MINUTE as i64, - )? - .set_default( - "server.global_unwrap_rate_limit_per_hour", - DEFAULT_GLOBAL_UNWRAP_LIMIT_PER_HOUR as i64, - )? - .set_default("relays.clearnet", Vec::::new())? - .set_default("relays.allow_unencrypted_clearnet_relays", false)? - .set_default("relays.onion", Vec::::new())? - .set_default("relays.reconnect_interval_secs", 5)? - .set_default("relays.max_reconnect_attempts", 10)? - .set_default("relays.connection_timeout_secs", 30)? - .set_default("apns.enabled", false)? - .set_default("apns.key_id", "")? - .set_default("apns.team_id", "")? - .set_default("apns.private_key_path", "")? - .set_default("apns.environment", "production")? - .set_default("apns.bundle_id", "")? - .set_default("apns.payload_mode", "silent")? - .set_default("fcm.enabled", false)? - .set_default("fcm.service_account_path", "")? - .set_default("fcm.project_id", "")? - .set_default("health.enabled", true)? - .set_default("health.bind_address", DEFAULT_HEALTH_BIND_ADDRESS)? - .set_default("metrics.enabled", true)? - .set_default("logging.level", "info")? - .set_default("logging.format", "json")? - .set_default("glitchtip.dsn", "")? - .set_default("glitchtip.environment", "production")? - .set_default("glitchtip.traces_sample_rate", 0.0)?) + let mut builder = Config::builder(); + for section in CONFIG_SECTIONS { + builder = builder.set_default(*section, config::Map::::new())?; + } + Ok(builder) } +/// Apply `TRANSPONDER_*` environment overrides to the builder. +/// +/// Returns the updated builder plus the raw `TRANSPONDER_SERVER_PRIVATE_KEY` +/// value, if present. The private key is intercepted here — never stored in +/// the builder — so the secret stays in a [`Zeroizing`] buffer instead of the +/// `config` crate's un-zeroized `Value` tree. fn apply_env_overrides( mut builder: ConfigBuilder, env_iter: I, -) -> Result> +) -> Result<(ConfigBuilder, Option>)> where I: IntoIterator, { + let mut private_key: Option> = None; + for (key, value) in env_iter { let Some((env_key, config_key)) = env_var_to_config_key(key)? else { continue; @@ -700,6 +712,11 @@ where )) })?; + if config_key == PRIVATE_KEY_CONFIG_KEY { + private_key = Some(Zeroizing::new(value)); + continue; + } + builder = if is_string_list_key(&config_key) { builder.set_override(&config_key, parse_string_list_env(&env_key, &value)?)? } else { @@ -707,7 +724,33 @@ where }; } - Ok(builder) + Ok((builder, private_key)) +} + +const PRIVATE_KEY_CONFIG_KEY: &str = "server.private_key"; + +/// Remove an inline `server.private_key` from a parsed TOML document, moving +/// the secret into a [`Zeroizing`] buffer. +/// +/// Called before the document is handed to the `config` crate so the secret +/// never enters the crate's `Value` tree. The extracted `String` is moved (not +/// copied) out of the TOML value, so no additional plaintext copy is left +/// behind in the document. +fn extract_inline_private_key( + doc: &mut toml::Table, +) -> std::result::Result>, config::ConfigError> { + let Some(server) = doc.get_mut("server").and_then(toml::Value::as_table_mut) else { + return Ok(None); + }; + + match server.remove("private_key") { + None => Ok(None), + Some(toml::Value::String(key)) => Ok(Some(Zeroizing::new(key))), + Some(other) => Err(config::ConfigError::Message(format!( + "server.private_key must be a string, got a TOML {}", + other.type_str() + ))), + } } // Preserve underscores in field names by splitting only once after the section. @@ -836,10 +879,40 @@ impl AppConfig { P: AsRef, I: IntoIterator, { - let builder = base_config_builder()?.add_source(File::from(path.as_ref())); - let config = apply_env_overrides(builder, env_iter)?.build()?; + let path = path.as_ref(); + + // Read and parse the TOML ourselves so an inline `server.private_key` + // can be moved into a `Zeroizing` buffer before the document reaches + // the `config` crate's un-zeroized `Value` tree. The raw file contents + // (which may contain the secret) are zeroized on drop. + let raw = Zeroizing::new(fs::read_to_string(path).map_err(|error| { + config::ConfigError::Message(format!( + "failed to read config file {}: {error}", + path.display() + )) + })?); + let mut doc: toml::Table = toml::from_str(&raw).map_err(|error| { + config::ConfigError::Message(format!( + "failed to parse config file {}: {error}", + path.display() + )) + })?; + let file_private_key = extract_inline_private_key(&mut doc)?; + let sanitized = toml::to_string(&doc).map_err(|error| { + config::ConfigError::Message(format!( + "failed to re-encode config file {}: {error}", + path.display() + )) + })?; + + let builder = + base_config_builder()?.add_source(File::from_str(&sanitized, FileFormat::Toml)); + let (builder, env_private_key) = apply_env_overrides(builder, env_iter)?; + let config = builder.build()?; - let config: Self = config.try_deserialize()?; + let mut config: Self = config.try_deserialize()?; + // Environment overrides file config, matching every other key. + config.server.private_key = env_private_key.or(file_private_key).unwrap_or_default(); config.validate()?; Ok(config) } @@ -848,9 +921,11 @@ impl AppConfig { where I: IntoIterator, { - let config = apply_env_overrides(base_config_builder()?, env_iter)?.build()?; + let (builder, env_private_key) = apply_env_overrides(base_config_builder()?, env_iter)?; + let config = builder.build()?; - let config: Self = config.try_deserialize()?; + let mut config: Self = config.try_deserialize()?; + config.server.private_key = env_private_key.unwrap_or_default(); config.validate()?; Ok(config) } @@ -858,15 +933,18 @@ impl AppConfig { /// Validates configuration values that cannot be expressed by the type /// system or `serde` defaults. /// - /// Rejects `0` for the rate-limit count fields and the rate-limit cache - /// size. A `0` count would either block every request for that limiter - /// dimension (a silent push outage) or be silently swapped for the default, - /// so an explicit error at load time surfaces the misconfiguration instead - /// of letting it reach runtime. Duration fields are also range-checked so a - /// degenerate value cannot defeat graceful shutdown or hang startup. + /// Rejects `0` for the rate-limit count fields and the cache sizes. A `0` + /// count would either block every request for that limiter dimension (a + /// silent push outage) or be silently swapped for the default, so an + /// explicit error at load time surfaces the misconfiguration instead of + /// letting it reach runtime. Duration fields are also range-checked so a + /// degenerate value cannot defeat graceful shutdown or hang startup, and + /// `health.bind_address` must parse as a socket address so a typo cannot + /// silently disable the health/readiness/metrics endpoints. fn validate(&self) -> Result<()> { self.server.validate()?; self.relays.validate()?; + self.health.validate()?; self.glitchtip.validate()?; Ok(()) } @@ -891,18 +969,22 @@ impl AppConfig { } impl ServerConfig { - /// Rejects rate-limit count fields and the rate-limit cache size set to `0`, - /// `shutdown_timeout_secs` set to `0`, and - /// `max_concurrent_event_processing` above [`MAX_CONCURRENT_EVENT_PROCESSING`]. + /// Rejects rate-limit count fields, the cache sizes, and + /// `max_tokens_per_event` set to `0`, `shutdown_timeout_secs` set to `0`, + /// and `max_concurrent_event_processing` above + /// [`MAX_CONCURRENT_EVENT_PROCESSING`]. /// /// A `0` per-minute/per-hour limit blocks every request for that limiter - /// dimension, and a `0` cache size is silently replaced by the default - /// rather than honoured. A `0` `shutdown_timeout_secs` makes the graceful - /// drain time out immediately, abandoning in-flight push notifications, so - /// it is rejected rather than silently skipping the drain. An oversized - /// `max_concurrent_event_processing` would panic in `Semaphore::new` at - /// startup, so it is range-checked here. Each error names the offending - /// config field. + /// dimension, a `0` `max_tokens_per_event` rejects every notification + /// event (a total, silent push outage), and a `0` cache size used to be + /// silently replaced by the default rather than honoured — the downstream + /// constructors now take `NonZeroUsize`, so the zero is rejected here with + /// a named-field error instead. A `0` `shutdown_timeout_secs` makes the + /// graceful drain time out immediately, abandoning in-flight push + /// notifications, so it is rejected rather than silently skipping the + /// drain. An oversized `max_concurrent_event_processing` would panic in + /// `Semaphore::new` at startup, so it is range-checked here. Each error + /// names the offending config field. fn validate(&self) -> std::result::Result<(), config::ConfigError> { if self.shutdown_timeout_secs == 0 { return Err(config::ConfigError::Message( @@ -939,6 +1021,14 @@ impl ServerConfig { "server.max_rate_limit_cache_size", self.max_rate_limit_cache_size as u64, ), + ( + "server.max_dedup_cache_size", + self.max_dedup_cache_size as u64, + ), + ( + "server.max_tokens_per_event", + self.max_tokens_per_event as u64, + ), ("server.dedup_retention_secs", self.dedup_retention_secs), ]; @@ -999,6 +1089,27 @@ impl RelayConfig { } } +impl HealthConfig { + /// Rejects a `bind_address` that does not parse as a socket address. + /// + /// The health server binds this address at startup; without a load-time + /// check, a typo (e.g. a hostname like `localhost:8080`, or an out-of-range + /// port) surfaces only as a runtime bind failure. Validated even when the + /// health server is disabled so a dormant misconfiguration cannot hide + /// until the endpoint is re-enabled. + fn validate(&self) -> std::result::Result<(), config::ConfigError> { + self.bind_address + .parse::() + .map_err(|error| { + config::ConfigError::Message(format!( + "health.bind_address \"{}\" is not a valid socket address (expected IP:port, e.g. \"127.0.0.1:8080\"): {error}", + self.bind_address + )) + }) + .map(|_| ()) + } +} + impl GlitchtipConfig { /// Rejects a `traces_sample_rate` outside `0.0..=1.0`. /// @@ -1260,7 +1371,7 @@ mod tests { assert_eq!(config.health.bind_address, "127.0.0.1:9090"); assert!(config.metrics.enabled); assert_eq!(config.logging.level, "debug"); - assert_eq!(config.logging.format, "pretty"); + assert_eq!(config.logging.format, LogFormat::Pretty); } #[test] @@ -1302,7 +1413,7 @@ mod tests { assert_eq!(config.health.bind_address, "127.0.0.1:8080"); assert!(config.metrics.enabled); assert_eq!(config.logging.level, "info"); - assert_eq!(config.logging.format, "json"); + assert_eq!(config.logging.format, LogFormat::Json); } #[test] @@ -1345,7 +1456,7 @@ mod tests { assert!(config.health.enabled); assert_eq!(config.health.bind_address, "127.0.0.1:8080"); assert_eq!(config.logging.level, "info"); - assert_eq!(config.logging.format, "json"); + assert_eq!(config.logging.format, LogFormat::Json); } #[test] @@ -1646,7 +1757,7 @@ mod tests { #[test] fn test_logging_config_defaults() { assert_eq!(default_log_level(), "info"); - assert_eq!(default_log_format(), "json"); + assert_eq!(default_log_format(), LogFormat::Json); } #[test] @@ -2222,4 +2333,343 @@ mod tests { MAX_CONCURRENT_EVENT_PROCESSING ); } + + // ---- #171: single source of defaults ---- + + #[test] + fn test_empty_toml_loads_all_serde_defaults() { + // The `set_default` ladder is gone; an empty config file (no sections at + // all) must still load, driven entirely by the serde `default_*` fns. + // This is the load path that previously relied on the config-crate + // defaults, so it is the regression guard for #171. + let file = create_temp_config(""); + let config = load_with_test_env(file.path(), &[]).unwrap(); + + assert_eq!( + config.server.shutdown_timeout_secs, + default_shutdown_timeout() + ); + assert_eq!( + config.server.max_dedup_cache_size, + default_max_dedup_cache_size() + ); + assert_eq!( + config.server.max_rate_limit_cache_size, + default_max_rate_limit_cache_size() + ); + assert_eq!( + config.server.max_tokens_per_event, + default_max_tokens_per_event() + ); + assert_eq!( + config.relays.reconnect_interval_secs, + default_reconnect_interval() + ); + assert_eq!( + config.relays.connection_timeout_secs, + default_connection_timeout() + ); + assert_eq!(config.apns.environment, default_apns_environment()); + assert!(config.health.enabled); + assert_eq!(config.health.bind_address, DEFAULT_HEALTH_BIND_ADDRESS); + assert!(config.metrics.enabled); + assert_eq!(config.logging.level, default_log_level()); + assert_eq!(config.logging.format, default_log_format()); + assert_eq!( + config.glitchtip.environment, + default_glitchtip_environment() + ); + // No private key set anywhere. + assert_eq!(config.server.private_key.as_str(), ""); + } + + #[test] + fn test_completely_empty_toml_table_loads_defaults() { + // Even a document that only declares empty section tables must succeed. + let file = create_temp_config( + "[server]\n[relays]\n[apns]\n[fcm]\n[health]\n[metrics]\n[logging]\n[glitchtip]\n", + ); + let config = load_with_test_env(file.path(), &[]).unwrap(); + assert_eq!(config.server.shutdown_timeout_secs, 10); + assert_eq!(config.logging.format, LogFormat::Json); + } + + // ---- #149 / #166: reject-zero cache/size fields ---- + + #[test] + fn test_zero_max_dedup_cache_size_rejected_from_file() { + let file = create_temp_config( + r#" + [server] + private_key = "test" + max_dedup_cache_size = 0 + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("server.max_dedup_cache_size"), "{message}"); + assert!(message.contains("must be greater than 0"), "{message}"); + } + + #[test] + fn test_zero_max_dedup_cache_size_rejected_from_env() { + let error = from_test_env(&[("TRANSPONDER_SERVER_MAX_DEDUP_CACHE_SIZE", "0")]).unwrap_err(); + assert!( + error.to_string().contains("server.max_dedup_cache_size"), + "{error}" + ); + } + + #[test] + fn test_zero_max_tokens_per_event_rejected_from_file() { + // A zero here caused a total silent push outage (#149): every event + // failed token parsing. It must fail at load instead. + let file = create_temp_config( + r#" + [server] + private_key = "test" + max_tokens_per_event = 0 + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("server.max_tokens_per_event"), "{message}"); + assert!(message.contains("must be greater than 0"), "{message}"); + } + + #[test] + fn test_zero_max_tokens_per_event_rejected_from_env() { + let error = from_test_env(&[("TRANSPONDER_SERVER_MAX_TOKENS_PER_EVENT", "0")]).unwrap_err(); + assert!( + error.to_string().contains("server.max_tokens_per_event"), + "{error}" + ); + } + + #[test] + fn test_zero_max_rate_limit_cache_size_rejected() { + // #166: previously coerced to 100k inside RateLimiter::new. + let error = + from_test_env(&[("TRANSPONDER_SERVER_MAX_RATE_LIMIT_CACHE_SIZE", "0")]).unwrap_err(); + assert!( + error + .to_string() + .contains("server.max_rate_limit_cache_size"), + "{error}" + ); + } + + #[test] + fn test_minimal_cache_and_token_sizes_accepted() { + let config = from_test_env(&[ + ("TRANSPONDER_SERVER_MAX_DEDUP_CACHE_SIZE", "1"), + ("TRANSPONDER_SERVER_MAX_TOKENS_PER_EVENT", "1"), + ("TRANSPONDER_SERVER_MAX_RATE_LIMIT_CACHE_SIZE", "1"), + ]) + .unwrap(); + assert_eq!(config.server.max_dedup_cache_size, 1); + assert_eq!(config.server.max_tokens_per_event, 1); + assert_eq!(config.server.max_rate_limit_cache_size, 1); + } + + // ---- #150 / #142: LogFormat enum ---- + + #[test] + fn test_log_format_parses_json_and_pretty() { + for (value, expected) in [("json", LogFormat::Json), ("pretty", LogFormat::Pretty)] { + let config_content = format!( + r#" + [server] + private_key = "test" + + [logging] + format = "{value}" + "# + ); + let file = create_temp_config(&config_content); + let config = load_with_test_env(file.path(), &[]).unwrap(); + assert_eq!(config.logging.format, expected); + } + } + + #[test] + fn test_log_format_off_is_rejected() { + // #150: `format = "off"` used to silently disable ALL logging. It must + // now fail at load with a message pointing to `logging.level = "off"`. + let file = create_temp_config( + r#" + [server] + private_key = "test" + + [logging] + format = "off" + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("logging.format"), "{message}"); + assert!(message.contains("off"), "{message}"); + assert!(message.contains("level"), "{message}"); + } + + #[test] + fn test_log_format_unknown_value_is_rejected() { + // #142: a typo like "jsno" must not silently fall back to a default. + let file = create_temp_config( + r#" + [server] + private_key = "test" + + [logging] + format = "jsno" + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("logging.format"), "{message}"); + assert!(message.contains("jsno"), "{message}"); + } + + #[test] + fn test_log_format_rejected_from_env() { + let error = from_test_env(&[("TRANSPONDER_LOGGING_FORMAT", "off")]).unwrap_err(); + assert!(error.to_string().contains("logging.format"), "{error}"); + } + + #[test] + fn test_log_format_display_round_trips() { + assert_eq!(LogFormat::Json.to_string(), "json"); + assert_eq!(LogFormat::Pretty.to_string(), "pretty"); + } + + // ---- #167: health.bind_address SocketAddr validation ---- + + #[test] + fn test_health_bind_address_hostname_rejected() { + // A hostname is not a SocketAddr; the health server would fail to bind + // at runtime and silently take down /health, /ready, /metrics. + let file = create_temp_config( + r#" + [server] + private_key = "test" + + [health] + bind_address = "localhost:8080" + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("health.bind_address"), "{message}"); + assert!(message.contains("localhost:8080"), "{message}"); + } + + #[test] + fn test_health_bind_address_out_of_range_port_rejected() { + let error = + from_test_env(&[("TRANSPONDER_HEALTH_BIND_ADDRESS", "127.0.0.1:99999")]).unwrap_err(); + assert!(error.to_string().contains("health.bind_address"), "{error}"); + } + + #[test] + fn test_health_bind_address_validated_even_when_disabled() { + // A dormant misconfiguration must still be caught so it cannot hide + // until the health server is re-enabled. + let file = create_temp_config( + r#" + [server] + private_key = "test" + + [health] + enabled = false + bind_address = "not-an-address" + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + assert!(error.to_string().contains("health.bind_address"), "{error}"); + } + + #[test] + fn test_health_bind_address_valid_accepted() { + let config = from_test_env(&[("TRANSPONDER_HEALTH_BIND_ADDRESS", "0.0.0.0:9100")]).unwrap(); + assert_eq!(config.health.bind_address, "0.0.0.0:9100"); + } + + // ---- #156: private key never enters the config Value tree ---- + + #[test] + fn test_inline_private_key_resolved_from_file() { + let file = create_temp_config( + r#" + [server] + private_key = "inline-secret-key" + "#, + ); + let config = load_with_test_env(file.path(), &[]).unwrap(); + assert_eq!(config.server.private_key.as_str(), "inline-secret-key"); + } + + #[test] + fn test_env_private_key_overrides_inline_file_value() { + // Precedence must match every other key: env beats file. + let file = create_temp_config( + r#" + [server] + private_key = "file-secret-key" + "#, + ); + let config = load_with_test_env( + file.path(), + &[("TRANSPONDER_SERVER_PRIVATE_KEY", "env-secret-key")], + ) + .unwrap(); + assert_eq!(config.server.private_key.as_str(), "env-secret-key"); + } + + #[test] + fn test_non_string_inline_private_key_rejected() { + // A non-string inline value is a misconfiguration and must fail with a + // named-field error rather than being silently ignored. + let file = create_temp_config( + r#" + [server] + private_key = 12345 + "#, + ); + let error = load_with_test_env(file.path(), &[]).unwrap_err(); + assert!(error.to_string().contains("server.private_key"), "{error}"); + } + + #[test] + fn test_extract_inline_private_key_removes_from_doc() { + // Direct unit check that the secret is moved out of the parsed TOML doc + // (so it never reaches the config crate's Value tree). + let mut doc: toml::Table = toml::from_str( + r#" + [server] + private_key = "top-secret" + private_key_file = "/some/path" + "#, + ) + .unwrap(); + + let extracted = extract_inline_private_key(&mut doc).unwrap(); + assert_eq!(extracted.as_deref().map(|z| z.as_str()), Some("top-secret")); + + let server = doc.get("server").unwrap().as_table().unwrap(); + assert!(!server.contains_key("private_key")); + // Non-secret sibling keys are untouched. + assert!(server.contains_key("private_key_file")); + } + + #[test] + fn test_extract_inline_private_key_absent_is_none() { + let mut doc: toml::Table = toml::from_str( + r#" + [server] + private_key_file = "/some/path" + "#, + ) + .unwrap(); + assert!(extract_inline_private_key(&mut doc).unwrap().is_none()); + } } diff --git a/src/main.rs b/src/main.rs index 8a0ddcf..d43eee2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,7 @@ //! to APNs and FCM. use std::io::Write; +use std::num::NonZeroUsize; use std::sync::Arc; use std::time::Duration; use std::{ @@ -134,10 +135,26 @@ fn validate_startup_config(server_private_key: &str, relays: &config::RelayConfi Ok(()) } +/// Convert a size field that `AppConfig::validate` already rejected as zero. +/// +/// The downstream cache/limiter configs take [`NonZeroUsize`] so the old +/// silent zero-to-default constructor coercions are unrepresentable; this is +/// the single, loud conversion point from the load-validated `usize` fields. +fn validated_non_zero(value: usize, field: &str) -> NonZeroUsize { + NonZeroUsize::new(value) + .unwrap_or_else(|| panic!("{field} is validated as non-zero at config load")) +} + fn build_rate_limit_config(server: &config::ServerConfig) -> nostr::events::TokenRateLimitConfig { nostr::events::TokenRateLimitConfig { - max_cache_size: server.max_rate_limit_cache_size, - max_tokens_per_event: server.max_tokens_per_event, + max_cache_size: validated_non_zero( + server.max_rate_limit_cache_size, + "server.max_rate_limit_cache_size", + ), + max_tokens_per_event: validated_non_zero( + server.max_tokens_per_event, + "server.max_tokens_per_event", + ), encrypted_token_per_minute: server.encrypted_token_rate_limit_per_minute, encrypted_token_per_hour: server.encrypted_token_rate_limit_per_hour, device_token_per_minute: server.device_token_rate_limit_per_minute, @@ -157,7 +174,10 @@ fn build_replay_protection_config( }; nostr::events::ReplayProtectionConfig { - max_dedup_cache_size: server.max_dedup_cache_size, + max_dedup_cache_size: validated_non_zero( + server.max_dedup_cache_size, + "server.max_dedup_cache_size", + ), dedup_state_path, dedup_retention: Duration::from_secs(server.dedup_retention_secs), max_notification_age: Duration::from_secs(server.max_notification_age_secs), @@ -457,6 +477,13 @@ async fn main() -> Result<()> { /// `telemetry::init`, `init_logging`) occur before the subscriber and client /// exist, so they surface only on stderr, not in GlitchTip. async fn run(mut config: AppConfig) -> Result<()> { + // Note: the `metrics.enabled && !health.enabled` case is no longer a + // silent-loss footgun. The health server now binds its listener and serves + // `/metrics` whenever a metrics collector exists — independent of + // `health.enabled` — and emits its own targeted warning at bind time (see + // `server::health::HealthServer::bind`). The #196 rider's structural fix + // landed there, so no separate load-time warning is needed here. + // Initialize metrics let metrics = if config.metrics.enabled { match Metrics::new() { @@ -835,6 +862,34 @@ fn generate_keys(output: Option<&Path>, show_private_key: bool) -> Result<()> { Ok(()) } +/// Refuse to load a private key file whose permissions grant group/other access. +/// +/// Mirrors the `0600` mode enforced by the `generate-keys` write path (and the +/// ssh/gpg convention): the server private key decrypts every notification +/// token, so a group/world-readable key file is a standing compromise. Failing +/// startup makes the exposure visible instead of silently loading the key. +#[cfg(unix)] +fn verify_private_key_file_permissions(path: &Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + + // A missing or unreadable file falls through to the read below, which + // reports the canonical "Failed to read server private key file" error. + let Ok(metadata) = fs::metadata(path) else { + return Ok(()); + }; + + let mode = metadata.permissions().mode() & 0o777; + if mode & 0o077 != 0 { + anyhow::bail!( + "Refusing to load server private key file {path}: permissions {mode:03o} allow group/other access; restrict with `chmod 600 {path}`", + path = path.display(), + mode = mode + ); + } + + Ok(()) +} + /// Resolve the server private key from config or a mounted secret file. fn resolve_server_private_key(config: &mut config::ServerConfig) -> Result> { let private_key = config.private_key.trim(); @@ -854,6 +909,10 @@ fn resolve_server_private_key(config: &mut config::ServerConfig) -> Result Result<()> { let filter = logging_filter(config)?; - // Build the console layer for the configured format (`None` disables console - // logging). The `EnvFilter` is attached per-layer to the fmt layer only, so it - // never gates the GlitchTip layer: error reporting stays independent of console - // verbosity — a tightened `RUST_LOG`, or `format = "off"`, does not silence it. - // The GlitchTip layer carries its own ERROR-level filter (see `telemetry`). - let console_layer: Option + Send + Sync>> = - match config.format.as_str() { - "json" => Some(fmt::layer().json().with_filter(filter).boxed()), - "pretty" => Some(fmt::layer().pretty().with_filter(filter).boxed()), - "off" => None, - _ => Some(fmt::layer().with_filter(filter).boxed()), + // `logging.format` is a validated enum, so there is no silent fallthrough + // for typos and no undocumented "off" blackout arm: silencing console + // output is `logging.level = "off"`, which keeps the subscriber installed. + // The `EnvFilter` is attached per-layer to the fmt layer only, so it never + // gates the GlitchTip layer: error reporting stays independent of console + // verbosity — a tightened `RUST_LOG` or `level = "off"` does not silence + // it. The GlitchTip layer carries its own ERROR-level filter (see + // `telemetry`). + let console_layer: Box + Send + Sync> = + match config.format { + config::LogFormat::Json => fmt::layer().json().with_filter(filter).boxed(), + config::LogFormat::Pretty => fmt::layer().pretty().with_filter(filter).boxed(), }; tracing_subscriber::registry() @@ -1464,7 +1524,7 @@ mod tests { fn test_logging_config(level: &str) -> config::LoggingConfig { config::LoggingConfig { level: level.to_string(), - format: "json".to_string(), + format: config::LogFormat::Json, } } @@ -1851,8 +1911,8 @@ mod tests { let rate_limit_config = build_rate_limit_config(&server); - assert_eq!(rate_limit_config.max_cache_size, 1234); - assert_eq!(rate_limit_config.max_tokens_per_event, 25); + assert_eq!(rate_limit_config.max_cache_size.get(), 1234); + assert_eq!(rate_limit_config.max_tokens_per_event.get(), 25); assert_eq!(rate_limit_config.encrypted_token_per_minute, 111); assert_eq!(rate_limit_config.encrypted_token_per_hour, 2222); assert_eq!(rate_limit_config.device_token_per_minute, 333); @@ -1886,7 +1946,7 @@ mod tests { let replay_config = build_replay_protection_config(&server); - assert_eq!(replay_config.max_dedup_cache_size, 77); + assert_eq!(replay_config.max_dedup_cache_size.get(), 77); assert_eq!(replay_config.dedup_state_path, Some(state_path)); assert_eq!(replay_config.dedup_retention, Duration::from_secs(88)); assert_eq!(replay_config.max_notification_age, Duration::from_secs(99)); @@ -1922,4 +1982,122 @@ mod tests { assert_eq!(replay_config.dedup_state_path, None); } + + // ---- #146: private key file permission check (unix) ---- + + #[cfg(unix)] + fn write_key_file_with_mode(mode: u32) -> NamedTempFile { + use std::os::unix::fs::PermissionsExt; + let file = NamedTempFile::new().unwrap(); + std::fs::write(file.path(), "abc123\n").unwrap(); + let mut perms = std::fs::metadata(file.path()).unwrap().permissions(); + perms.set_mode(mode); + std::fs::set_permissions(file.path(), perms).unwrap(); + file + } + + #[cfg(unix)] + #[test] + fn verify_private_key_file_permissions_accepts_0600() { + let file = write_key_file_with_mode(0o600); + assert!(verify_private_key_file_permissions(file.path()).is_ok()); + } + + #[cfg(unix)] + #[test] + fn verify_private_key_file_permissions_rejects_group_readable() { + let file = write_key_file_with_mode(0o640); + let error = verify_private_key_file_permissions(file.path()) + .expect_err("group-readable key file must be rejected"); + assert!(error.to_string().contains("group/other access"), "{error}"); + } + + #[cfg(unix)] + #[test] + fn verify_private_key_file_permissions_rejects_world_readable() { + let file = write_key_file_with_mode(0o644); + let error = verify_private_key_file_permissions(file.path()) + .expect_err("world-readable key file must be rejected"); + assert!(error.to_string().contains("group/other access"), "{error}"); + } + + #[cfg(unix)] + #[test] + fn resolve_server_private_key_rejects_permissive_key_file() { + // End-to-end: a permissive key file must fail the whole resolve path, + // not just the standalone permission check. + let file = write_key_file_with_mode(0o644); + let mut config = config::ServerConfig { + private_key: Zeroizing::new(String::new()), + private_key_file: file.path().display().to_string(), + shutdown_timeout_secs: 10, + max_dedup_cache_size: 100_000, + dedup_state_path: PathBuf::new(), + dedup_retention_secs: crate::nostr::events::DEFAULT_DEDUP_RETENTION_SECS, + max_notification_age_secs: crate::nostr::events::DEFAULT_MAX_NOTIFICATION_AGE_SECS, + max_notification_future_skew_secs: + crate::nostr::events::DEFAULT_MAX_NOTIFICATION_FUTURE_SKEW_SECS, + max_rate_limit_cache_size: 100_000, + max_tokens_per_event: crate::crypto::nip59::DEFAULT_MAX_TOKENS_PER_EVENT, + encrypted_token_rate_limit_per_minute: 240, + encrypted_token_rate_limit_per_hour: 5000, + device_token_rate_limit_per_minute: 240, + device_token_rate_limit_per_hour: 5000, + max_concurrent_event_processing: 64, + global_unwrap_rate_limit_per_minute: 600, + global_unwrap_rate_limit_per_hour: 30_000, + }; + + let error = resolve_server_private_key(&mut config) + .expect_err("a group/world-readable key file must refuse to load"); + assert!(error.to_string().contains("group/other access"), "{error}"); + } + + #[cfg(unix)] + #[test] + fn resolve_server_private_key_reads_0600_key_file() { + let file = write_key_file_with_mode(0o600); + let mut config = config::ServerConfig { + private_key: Zeroizing::new(String::new()), + private_key_file: file.path().display().to_string(), + shutdown_timeout_secs: 10, + max_dedup_cache_size: 100_000, + dedup_state_path: PathBuf::new(), + dedup_retention_secs: crate::nostr::events::DEFAULT_DEDUP_RETENTION_SECS, + max_notification_age_secs: crate::nostr::events::DEFAULT_MAX_NOTIFICATION_AGE_SECS, + max_notification_future_skew_secs: + crate::nostr::events::DEFAULT_MAX_NOTIFICATION_FUTURE_SKEW_SECS, + max_rate_limit_cache_size: 100_000, + max_tokens_per_event: crate::crypto::nip59::DEFAULT_MAX_TOKENS_PER_EVENT, + encrypted_token_rate_limit_per_minute: 240, + encrypted_token_rate_limit_per_hour: 5000, + device_token_rate_limit_per_minute: 240, + device_token_rate_limit_per_hour: 5000, + max_concurrent_event_processing: 64, + global_unwrap_rate_limit_per_minute: 600, + global_unwrap_rate_limit_per_hour: 30_000, + }; + + assert_eq!( + resolve_server_private_key(&mut config).unwrap().as_str(), + "abc123" + ); + } + + // ---- #150 / #142: init_logging accepts both enum variants ---- + + #[test] + fn init_logging_builds_layer_for_each_format() { + // `init_logging` installs a global subscriber (a process-wide, one-shot + // side effect), so exercise the format-selection logic through + // `logging_filter` + the format match instead of calling `init`. Both + // enum variants must yield a filter without error. + for format in [config::LogFormat::Json, config::LogFormat::Pretty] { + let config = config::LoggingConfig { + level: "info".to_string(), + format, + }; + assert!(logging_filter(&config).is_ok()); + } + } } diff --git a/src/nostr/client.rs b/src/nostr/client.rs index f5738f8..4694787 100644 --- a/src/nostr/client.rs +++ b/src/nostr/client.rs @@ -517,6 +517,11 @@ impl RelayClient { } } + /// The configured relay URLs to advertise, as raw config strings. + /// + /// The `r`-tag values published in the kind-10050 event preserve the + /// operator's exact spelling; change-detection normalizes separately (see + /// [`Self::inbox_relay_event_is_current`]). fn configured_inbox_relays(&self) -> BTreeSet { self.config .clearnet @@ -543,19 +548,53 @@ impl RelayClient { .await .map_err(|e| Error::Nostr(format!("Failed to fetch existing kind 10050 event: {e}")))?; - Ok(events - .first() - .is_some_and(|event| inbox_relay_tags(event) == *relay_urls)) + // kind 10050 is replaceable: compare against the newest fetched event by + // `created_at`, not an arbitrary `events.first()` that can land on a + // stale reply from a slow/secondary relay. + let Some(newest) = events.iter().max_by_key(|event| event.created_at) else { + return Ok(false); + }; + + // Normalize both sides through `RelayUrl` before comparing so cosmetic + // differences (trailing slash, host case) between the config spelling + // and the published tag do not trigger a spurious republish. Config + // entries are already validated, so a parse failure there is a real + // error; unparseable tag values on the network side are skipped. + let configured = normalize_relay_urls(relay_urls.iter().map(String::as_str))?; + let published = normalize_relay_tags(newest); + + Ok(configured == published) } } -fn inbox_relay_tags(event: &Event) -> BTreeSet { +/// Normalize an iterator of config relay strings into a canonical set. +/// +/// Returns an error if any entry fails to parse; config URLs are validated at +/// startup, so a failure here is a real misconfiguration rather than +/// network-supplied noise. +fn normalize_relay_urls<'a, I>(urls: I) -> Result> +where + I: IntoIterator, +{ + urls.into_iter() + .map(|url| { + RelayUrl::parse(url) + .map_err(|e| Error::Nostr(format!("Invalid configured relay URL '{url}': {e}"))) + }) + .collect() +} + +/// Collect the `r`-tag relay URLs from an event, normalized through +/// [`RelayUrl`]. Unparseable tag values are skipped rather than failing the +/// comparison, since the event is network-supplied. +fn normalize_relay_tags(event: &Event) -> BTreeSet { event .tags .as_slice() .iter() .filter(|tag| tag.kind() == TagKind::Relay) - .filter_map(|tag| tag.content().map(str::to_owned)) + .filter_map(|tag| tag.content()) + .filter_map(|content| RelayUrl::parse(content).ok()) .collect() } @@ -625,6 +664,38 @@ fn validate_relay_config(config: &RelayConfig) -> Result<()> { )); } + // Validate the onion list symmetrically with clearnet: without this, a + // plaintext `ws://` or a misspelled `.onion` entry passed config validation + // and was added blindly, defeating the wss-only TLS enforcement and + // silently routing traffic over clearnet at connect time (only a `warn!`). + for url in &config.onion { + validate_onion_relay_url(url)?; + } + + Ok(()) +} + +/// Rejects an onion relay URL that does not parse as a `ws://`/`wss://` URL +/// with a `.onion` host. +/// +/// A plaintext `ws://` entry in `relays.onion` would defeat the wss-only TLS +/// enforcement, and a typo'd `.onion` URL would be silently dropped at connect +/// time — either way the deployment degrades to clearnet without a hard +/// failure. Parsing through nostr-sdk's [`RelayUrl`] (the same type the pool +/// uses) rejects both fast at startup with a named error. +fn validate_onion_relay_url(url: &str) -> Result<()> { + let parsed = RelayUrl::parse(url).map_err(|e| { + Error::Nostr(format!( + "Onion relay '{url}' is not a valid ws:// or wss:// URL: {e}" + )) + })?; + + if !parsed.is_onion() { + return Err(Error::Nostr(format!( + "Onion relay '{url}' must have a .onion host" + ))); + } + Ok(()) } @@ -1606,4 +1677,150 @@ mod tests { .contains("Failed to connect to any relay within 2 seconds") ); } + + // ---- #122: onion relay URL validation (feature-independent parser) ---- + + #[test] + fn test_validate_onion_relay_url_accepts_ws_and_wss_onion() { + assert!(validate_onion_relay_url("ws://abcabcabcabcabcd.onion").is_ok()); + assert!(validate_onion_relay_url("wss://abcabcabcabcabcd.onion").is_ok()); + // With an explicit port, too. + assert!(validate_onion_relay_url("ws://abcabcabcabcabcd.onion:8080").is_ok()); + } + + #[test] + fn test_validate_onion_relay_url_rejects_non_onion_host() { + // A clearnet host slipped into relays.onion must be rejected. + let error = validate_onion_relay_url("wss://relay.example.com").unwrap_err(); + assert!(error.to_string().contains(".onion"), "{error}"); + } + + #[test] + fn test_validate_onion_relay_url_rejects_missing_scheme() { + // A bare host (no ws://) must be rejected instead of silently dropped. + let error = validate_onion_relay_url("abcabcabcabcabcd.onion").unwrap_err(); + assert!( + error.to_string().contains("valid ws:// or wss:// URL"), + "{error}" + ); + } + + #[test] + fn test_validate_onion_relay_url_rejects_non_ws_scheme() { + // http:// is not a websocket scheme; RelayUrl::parse rejects it. + let error = validate_onion_relay_url("http://abcabcabcabcabcd.onion").unwrap_err(); + assert!( + error.to_string().contains("valid ws:// or wss:// URL"), + "{error}" + ); + } + + #[cfg(feature = "tor")] + #[test] + fn test_validate_relay_config_rejects_plaintext_ws_in_onion_list() { + // The concrete #122 footgun: a plaintext ws:// entry with a clearnet + // host in relays.onion. Only reachable when the tor feature is on + // (otherwise the "requires tor feature" error fires first). + let config = RelayConfig { + clearnet: vec![], + allow_unencrypted_clearnet_relays: false, + onion: vec!["ws://plaintext.example.com".to_string()], + reconnect_interval_secs: 5, + max_reconnect_attempts: 10, + connection_timeout_secs: 5, + }; + let error = validate_relay_config(&config).unwrap_err(); + assert!(error.to_string().contains(".onion"), "{error}"); + } + + #[cfg(feature = "tor")] + #[test] + fn test_validate_relay_config_accepts_valid_onion() { + let config = RelayConfig { + clearnet: vec![], + allow_unencrypted_clearnet_relays: false, + onion: vec!["ws://abcabcabcabcabcd.onion".to_string()], + reconnect_interval_secs: 5, + max_reconnect_attempts: 10, + connection_timeout_secs: 5, + }; + assert!(validate_relay_config(&config).is_ok()); + } + + // ---- #124: normalized inbox comparison + newest-event selection ---- + + fn kind_10050_event(keys: &Keys, created_at: Timestamp, relay_urls: &[&str]) -> Event { + let tags: Vec = relay_urls + .iter() + .map(|url| Tag::custom(TagKind::Relay, [*url])) + .collect(); + EventBuilder::new(Kind::Custom(10050), "") + .tags(tags) + .custom_created_at(created_at) + .sign_with_keys(keys) + .unwrap() + } + + #[test] + fn test_normalize_relay_tags_ignores_trailing_slash_and_case() { + let keys = Keys::generate(); + // The published event uses a trailing slash and mixed host case. + let event = kind_10050_event(&keys, Timestamp::now(), &["wss://Relay.Example.com/"]); + let published = normalize_relay_tags(&event); + // The config side uses the bare, lowercase form. + let configured = normalize_relay_urls(["wss://relay.example.com"]).unwrap(); + assert_eq!( + published, configured, + "cosmetic URL differences must normalize to the same set (no spurious republish)" + ); + } + + #[test] + fn test_normalize_relay_tags_skips_unparseable_tag_values() { + let keys = Keys::generate(); + let event = kind_10050_event( + &keys, + Timestamp::now(), + &["wss://relay.example.com", "not a url"], + ); + let published = normalize_relay_tags(&event); + // Only the parseable URL survives. + assert_eq!(published.len(), 1); + assert!(published.contains(&RelayUrl::parse("wss://relay.example.com").unwrap())); + } + + #[test] + fn test_normalize_relay_urls_errors_on_bad_config_entry() { + let error = normalize_relay_urls(["totally invalid"]).unwrap_err(); + assert!( + error.to_string().contains("Invalid configured relay URL"), + "{error}" + ); + } + + #[test] + fn test_newest_kind_10050_event_selected_by_created_at() { + // Verifies the max_by_key(created_at) selection logic: given two events, + // the newest one's normalized tag set is what change-detection compares + // against, regardless of fetch order. + let keys = Keys::generate(); + let older = kind_10050_event( + &keys, + Timestamp::from(1_000), + &["wss://old-relay.example.com"], + ); + let newer = kind_10050_event( + &keys, + Timestamp::from(2_000), + &["wss://new-relay.example.com"], + ); + + let events = [older.clone(), newer.clone()]; + let selected = events.iter().max_by_key(|e| e.created_at).unwrap(); + assert_eq!(selected.id, newer.id, "the newest event must be selected"); + + let published = normalize_relay_tags(selected); + let configured = normalize_relay_urls(["wss://new-relay.example.com"]).unwrap(); + assert_eq!(published, configured); + } } diff --git a/src/nostr/events.rs b/src/nostr/events.rs index c2422cb..e847b1f 100644 --- a/src/nostr/events.rs +++ b/src/nostr/events.rs @@ -380,12 +380,18 @@ pub struct EventProcessor { } /// Configuration for token rate limiting. +/// +/// The size fields are [`NonZeroUsize`] so a zero cache capacity or token +/// limit is unrepresentable here: production config rejects zeros at load +/// time with named-field errors instead of constructors silently substituting +/// defaults (`max_cache_size`) or rejecting every notification event +/// (`max_tokens_per_event`). #[derive(Debug, Clone, Copy)] pub struct TokenRateLimitConfig { /// Maximum entries in each rate limit cache. - pub max_cache_size: usize, + pub max_cache_size: NonZeroUsize, /// Maximum encrypted tokens accepted in a single event. - pub max_tokens_per_event: usize, + pub max_tokens_per_event: NonZeroUsize, /// Max encrypted token requests per minute. pub encrypted_token_per_minute: u32, /// Max encrypted token requests per hour. @@ -403,8 +409,10 @@ pub struct TokenRateLimitConfig { impl Default for TokenRateLimitConfig { fn default() -> Self { Self { - max_cache_size: DEFAULT_MAX_SIZE, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(DEFAULT_MAX_SIZE) + .expect("DEFAULT_MAX_SIZE is non-zero"), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT) + .expect("DEFAULT_MAX_TOKENS_PER_EVENT is non-zero"), encrypted_token_per_minute: DEFAULT_RATE_LIMIT_PER_MINUTE, encrypted_token_per_hour: DEFAULT_RATE_LIMIT_PER_HOUR, device_token_per_minute: DEFAULT_RATE_LIMIT_PER_MINUTE, @@ -424,7 +432,12 @@ pub struct ReplayProtectionConfig { /// When `dedup_state_path` is set, all terminal event IDs inside /// `dedup_retention` are kept so durable replay state covers the full relay /// lookback window instead of being capped by this LRU size. - pub max_dedup_cache_size: usize, + /// + /// `NonZeroUsize` so a zero capacity is unrepresentable here: production + /// config rejects `server.max_dedup_cache_size = 0` at load time with a + /// named-field error instead of the constructor silently substituting the + /// default. + pub max_dedup_cache_size: NonZeroUsize, /// Optional durable replay-state path. /// /// The file stores only public Nostr gift-wrap event IDs and the time they @@ -444,7 +457,8 @@ pub struct ReplayProtectionConfig { impl Default for ReplayProtectionConfig { fn default() -> Self { Self { - max_dedup_cache_size: DEFAULT_MAX_DEDUP_CACHE_SIZE, + max_dedup_cache_size: NonZeroUsize::new(DEFAULT_MAX_DEDUP_CACHE_SIZE) + .expect("DEFAULT_MAX_DEDUP_CACHE_SIZE is non-zero"), dedup_state_path: None, dedup_retention: DEDUP_WINDOW, max_notification_age: Duration::from_secs(DEFAULT_MAX_NOTIFICATION_AGE_SECS), @@ -510,7 +524,8 @@ impl EventProcessor { push_dispatcher, rate_limit_config, ReplayProtectionConfig { - max_dedup_cache_size, + max_dedup_cache_size: NonZeroUsize::new(max_dedup_cache_size) + .expect("test dedup cache size must be non-zero"), ..ReplayProtectionConfig::default() }, metrics, @@ -527,10 +542,9 @@ impl EventProcessor { replay_config: ReplayProtectionConfig, metrics: Option, ) -> Result { - let cache_size = NonZeroUsize::new(replay_config.max_dedup_cache_size).unwrap_or( - NonZeroUsize::new(DEFAULT_MAX_DEDUP_CACHE_SIZE) - .expect("DEFAULT_MAX_DEDUP_CACHE_SIZE is non-zero"), - ); + // `max_dedup_cache_size` is `NonZeroUsize`, so the previous silent + // zero-to-default substitution is unrepresentable. + let cache_size = replay_config.max_dedup_cache_size; let (seen_events, dedup_persistence) = if let Some(path) = replay_config.dedup_state_path { let seen = @@ -559,7 +573,7 @@ impl EventProcessor { global_unwrap_limiter: RateLimiter::new(RateLimitConfig { max_per_minute: rate_limit_config.global_unwrap_per_minute, max_per_hour: rate_limit_config.global_unwrap_per_hour, - max_entries: 1, + max_entries: NonZeroUsize::MIN, }), encrypted_token_limiter: RateLimiter::new(RateLimitConfig { max_per_minute: rate_limit_config.encrypted_token_per_minute, @@ -571,7 +585,7 @@ impl EventProcessor { max_per_hour: rate_limit_config.device_token_per_hour, max_entries: rate_limit_config.max_cache_size, }), - max_tokens_per_event: rate_limit_config.max_tokens_per_event, + max_tokens_per_event: rate_limit_config.max_tokens_per_event.get(), metrics, }) } @@ -1617,7 +1631,7 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let state_path = dir.path().join("dedup-state.tsv"); let replay_config = ReplayProtectionConfig { - max_dedup_cache_size: 2, + max_dedup_cache_size: NonZeroUsize::new(2).unwrap(), dedup_state_path: Some(state_path.clone()), ..ReplayProtectionConfig::default() }; @@ -2024,8 +2038,8 @@ mod tests { create_processor_with_shutdown_dispatcher_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 1, encrypted_token_per_hour: 1, device_token_per_minute: 1, @@ -2079,8 +2093,8 @@ mod tests { create_processor_with_shutdown_dispatcher_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 0, encrypted_token_per_hour: 100, device_token_per_minute: 100, @@ -2316,13 +2330,16 @@ mod tests { } #[tokio::test] - async fn test_cache_size_zero_uses_default() { + #[should_panic(expected = "test dedup cache size must be non-zero")] + async fn test_cache_size_zero_is_rejected() { + // A zero dedup cache size is no longer silently swapped for the default: + // `ReplayProtectionConfig::max_dedup_cache_size` is `NonZeroUsize`, so a + // zero is unrepresentable. Production config rejects + // `server.max_dedup_cache_size = 0` at load time before it can reach the + // constructor (see the config validation tests); this asserts the + // in-memory construction path cannot be built with a zero either. let server_keys = Keys::generate(); - let processor = create_processor_with_cache_size(&server_keys, 0); - - let event_id = EventId::all_zeros(); - processor.mark_seen(event_id).await; - assert!(processor.is_duplicate(&event_id).await); + let _ = create_processor_with_cache_size(&server_keys, 0); } #[tokio::test] @@ -2519,8 +2536,8 @@ mod tests { let (processor, metrics) = create_processor_with_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 100, encrypted_token_per_hour: 1000, device_token_per_minute: 1, @@ -2584,8 +2601,8 @@ mod tests { let processor = create_processor_with_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 100, // High limit for encrypted tokens encrypted_token_per_hour: 1000, device_token_per_minute: 2, // Only allow 2 per minute per device @@ -2647,8 +2664,8 @@ mod tests { let processor = create_processor_with_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 2, // Only allow 2 per minute per encrypted token encrypted_token_per_hour: 100, device_token_per_minute: 100, // High limit for device tokens @@ -2728,8 +2745,8 @@ mod tests { let processor = create_processor_with_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 1, encrypted_token_per_hour: 100, device_token_per_minute: 100, @@ -2797,8 +2814,8 @@ mod tests { let (processor, metrics) = create_processor_with_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 100, encrypted_token_per_hour: 1000, device_token_per_minute: 1, @@ -2916,8 +2933,8 @@ mod tests { let (processor, metrics) = create_processor_with_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), // The valid token's device budget is exhausted by the warmup, so // it is rate-limited; the other token is undecryptable. encrypted_token_per_minute: 100, @@ -2964,8 +2981,8 @@ mod tests { global_unwrap_per_hour: u32, ) -> TokenRateLimitConfig { TokenRateLimitConfig { - max_cache_size: 100, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(100).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), // Generous per-token limits so only the global limiter can shed. encrypted_token_per_minute: 100_000, encrypted_token_per_hour: 100_000, @@ -3117,8 +3134,8 @@ mod tests { let (processor, metrics) = create_processor_with_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 3, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(3).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 100, encrypted_token_per_hour: 1000, device_token_per_minute: 100, @@ -3165,8 +3182,8 @@ mod tests { let (processor, metrics) = create_processor_with_metrics_and_rate_limits( &server_keys, TokenRateLimitConfig { - max_cache_size: 3, - max_tokens_per_event: DEFAULT_MAX_TOKENS_PER_EVENT, + max_cache_size: NonZeroUsize::new(3).unwrap(), + max_tokens_per_event: NonZeroUsize::new(DEFAULT_MAX_TOKENS_PER_EVENT).unwrap(), encrypted_token_per_minute: 1, encrypted_token_per_hour: 100, device_token_per_minute: 100, diff --git a/src/rate_limiter.rs b/src/rate_limiter.rs index ee9b7a2..7166000 100644 --- a/src/rate_limiter.rs +++ b/src/rate_limiter.rs @@ -200,7 +200,12 @@ pub struct RateLimitConfig { /// for a least-recently-used stale entry to evict, then falls back to the /// least-recently-used entry that is still below its own rate limits; if no /// safe victim is found, the unknown key is rejected. - pub max_entries: usize, + /// + /// `NonZeroUsize` so a zero capacity is unrepresentable here: production + /// config rejects `server.max_rate_limit_cache_size = 0` at load time with + /// a named-field error instead of the constructor silently substituting a + /// default. + pub max_entries: NonZeroUsize, } impl Default for RateLimitConfig { @@ -208,7 +213,7 @@ impl Default for RateLimitConfig { Self { max_per_minute: DEFAULT_RATE_LIMIT_PER_MINUTE, max_per_hour: DEFAULT_RATE_LIMIT_PER_HOUR, - max_entries: DEFAULT_MAX_SIZE, + max_entries: NonZeroUsize::new(DEFAULT_MAX_SIZE).expect("DEFAULT_MAX_SIZE is non-zero"), } } } @@ -247,11 +252,8 @@ pub struct RateLimiter { impl RateLimiter { /// Creates a new rate limiter with the given configuration. pub fn new(config: RateLimitConfig) -> Self { - let size = NonZeroUsize::new(config.max_entries) - .unwrap_or(NonZeroUsize::new(DEFAULT_MAX_SIZE).expect("DEFAULT_MAX_SIZE is non-zero")); - Self { - entries: RwLock::new(LruCache::new(size)), + entries: RwLock::new(LruCache::new(config.max_entries)), max_per_minute: config.max_per_minute, max_per_hour: config.max_per_hour, next_hit_id: AtomicU64::new(0), @@ -440,12 +442,16 @@ fn remove_hit(hits: &mut VecDeque<(Instant, u64)>, hit_id: u64) { mod tests { use super::*; + fn entries(n: usize) -> NonZeroUsize { + NonZeroUsize::new(n).expect("test max_entries must be non-zero") + } + #[tokio::test] async fn test_allows_within_limits() { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); for _ in 0..10 { @@ -458,7 +464,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 5, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); for _ in 0..5 { @@ -479,7 +485,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 1, max_per_hour: 1, - max_entries: 100, + max_entries: entries(100), }); let reservation = limiter @@ -500,7 +506,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); let first = limiter @@ -520,7 +526,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 1, + max_entries: entries(1), }); let first = limiter @@ -543,7 +549,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 100, max_per_hour: 5, - max_entries: 100, + max_entries: entries(100), }); for _ in 0..5 { @@ -563,7 +569,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 5, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); for _ in 0..5 { @@ -584,7 +590,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 5, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); assert!(limiter.check_and_increment(&1u64).await.is_allowed()); @@ -615,7 +621,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 100, max_per_hour: 5, - max_entries: 100, + max_entries: entries(100), }); for _ in 0..5 { @@ -636,7 +642,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 100, max_per_hour: 5, - max_entries: 100, + max_entries: entries(100), }); assert!(limiter.check_and_increment(&1u64).await.is_allowed()); @@ -665,7 +671,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 2, max_per_hour: 10, - max_entries: 100, + max_entries: entries(100), }); // Key 1 hits limit @@ -686,7 +692,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); limiter.check_and_increment(&1u64).await; @@ -708,7 +714,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: CLEANUP_BATCH_SIZE + 1, + max_entries: entries(CLEANUP_BATCH_SIZE + 1), }); for key in 0..=CLEANUP_BATCH_SIZE as u64 { @@ -729,7 +735,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 3, + max_entries: entries(3), }); for key in 1..=3 { @@ -748,7 +754,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 1, max_per_hour: 100, - max_entries: 3, + max_entries: entries(3), }); for key in 1..=3 { @@ -765,7 +771,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: 3, + max_entries: entries(3), }); limiter.check_and_increment(&1u64).await; @@ -817,7 +823,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 10, max_per_hour: 100, - max_entries: CLEANUP_BATCH_SIZE + 1, + max_entries: entries(CLEANUP_BATCH_SIZE + 1), }); for key in 0..=CLEANUP_BATCH_SIZE as u64 { @@ -844,7 +850,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 1, max_per_hour: 100, - max_entries: 3, + max_entries: entries(3), }); limiter.check_and_increment(&1u64).await; @@ -865,7 +871,7 @@ mod tests { let limiter: RateLimiter = RateLimiter::new(RateLimitConfig { max_per_minute: 1, max_per_hour: 100, - max_entries: 2, + max_entries: entries(2), }); assert!(limiter.check_and_increment(&1u64).await.is_allowed()); @@ -913,7 +919,7 @@ mod tests { let limiter: RateLimiter<[u8; 32]> = RateLimiter::new(RateLimitConfig { max_per_minute: 5, max_per_hour: 100, - max_entries: 100, + max_entries: entries(100), }); let key1 = [1u8; 32];