From 771a69b85e36b0f9951f83b261cdc39c51db8418 Mon Sep 17 00:00:00 2001 From: jwinterm Date: Thu, 25 Jun 2026 17:25:52 -0400 Subject: [PATCH] fix(prices,config): harden per-feed control + fail-closed gaps (adversarial review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a multi-lens adversarial review of the capabilities + prices work: - PRICES_ASSETS="" now means NONE, not all (real bug): from_env used env_opt, which collapses unset and present-but-empty into None, so the documented "empty = none" silently served all five feeds. Read via env::var so unset => all, present (even empty) => the listed subset or none. - PRICES_CURRENCY is now validated against a curated SUPPORTED_PRICE_CURRENCIES allowlist. Previously a typo (e.g. "usdd") booted, advertised prices:true, and served an empty feed forever (the provider returns empty quotes for an unknown currency) — a fail-open path inconsistent with the adjacent provider/asset checks. - PRICES_PROVIDER is lowercased at read, matching the asset/currency normalization (mixed-case no longer trips the allowlist). - validate() now requires an Electrum URL for an enabled BTC/LTC chain (hard error in prod, warn in dev), symmetric with the xmr/wow/grin secret checks. Keeps /capabilities honest: a chain it can't serve won't boot advertising itself as available. Tests: config regressions (currency, utxo-electrum); a PriceClient feed-subset selection unit test; the /prices integration test now populates the snapshot and asserts the served quotes + RFC-3339 timestamp (the prior PRICES_ASSETS env-var was inert under the harness); capabilities asserts network presence/null across all five chains. Dismissed after verification: coingecko json() was already switched to a streaming read_capped; fleet refresh-jitter (no API key, single-node, self-healing on 429); prices capability staying a bare bool (/prices carries the currency + per-asset map). --- src/config.rs | 109 ++++++++++++++++++++++++++++++++++++------ src/infra/prices.rs | 49 ++++++++++++++++--- tests/capabilities.rs | 16 +++++-- tests/prices.rs | 25 ++++++++-- 4 files changed, 171 insertions(+), 28 deletions(-) diff --git a/src/config.rs b/src/config.rs index 36879c8..c098bff 100644 --- a/src/config.rs +++ b/src/config.rs @@ -159,6 +159,14 @@ pub struct SecretConfig { /// `infra::prices` and a test there asserts it covers exactly this set. pub const SUPPORTED_PRICE_ASSETS: &[&str] = &["btc", "ltc", "xmr", "wow", "grin"]; +/// Fiat (and crypto-denominated) currencies the price feed may quote in. A +/// curated allowlist so a `PRICES_CURRENCY` typo fails closed at startup rather +/// than booting a feed that advertises `prices:true` but serves nothing (the +/// provider returns empty quotes for an unknown currency). +pub const SUPPORTED_PRICE_CURRENCIES: &[&str] = &[ + "usd", "eur", "gbp", "jpy", "cny", "aud", "cad", "chf", "btc", +]; + #[derive(Clone)] pub struct FeatureFlags { pub chains: ChainFlags, @@ -324,20 +332,24 @@ impl Config { grin: env_bool("FEATURE_GRIN", true), }, prices: env_bool("FEATURE_PRICES", true), - prices_provider: env_or("PRICES_PROVIDER", "coingecko"), + prices_provider: env_or("PRICES_PROVIDER", "coingecko").to_lowercase(), prices_interval_secs: env_parse("PRICES_FETCH_INTERVAL_SECS", 300u64)?, - // Unset = all supported feeds; an explicit list (even empty) - // narrows it, so operators can serve a subset or none. - prices_assets: match env_opt("PRICES_ASSETS") { - Some(s) => s + // Per-feed control. Distinguish UNSET from PRESENT-BUT-EMPTY: + // `env::var` (not `env_opt`, which collapses empty into None) so + // `PRICES_ASSETS=` means "none", not "all". + // unset => all supported feeds + // "btc,xmr" => that subset + // "" (or blanks) => none + prices_assets: match env::var("PRICES_ASSETS") { + Err(_) => SUPPORTED_PRICE_ASSETS + .iter() + .map(|s| s.to_string()) + .collect(), + Ok(s) => s .split(',') .map(|x| x.trim().to_lowercase()) .filter(|x| !x.is_empty()) .collect(), - None => SUPPORTED_PRICE_ASSETS - .iter() - .map(|s| s.to_string()) - .collect(), }, prices_currency: env_or("PRICES_CURRENCY", "usd").to_lowercase(), tips: env_bool("FEATURE_TIPS", false), @@ -501,9 +513,17 @@ impl Config { ); } - // Price feed: every configured asset must be one we can quote. Fail - // closed on a typo rather than silently dropping a feed. + // Price feed: provider must be one we implement, and every configured + // asset must be one we can quote. Fail closed on a typo rather than + // booting a feed that can never populate (which would still advertise + // prices:true and silently serve nothing). if self.features.prices { + if !matches!(self.features.prices_provider.as_str(), "coingecko") { + return Err(cfg_err(format!( + "PRICES_PROVIDER {:?} is not supported; supported: coingecko", + self.features.prices_provider + ))); + } for asset in &self.features.prices_assets { if !SUPPORTED_PRICE_ASSETS.contains(&asset.as_str()) { return Err(cfg_err(format!( @@ -512,10 +532,31 @@ impl Config { ))); } } + if !SUPPORTED_PRICE_CURRENCIES.contains(&self.features.prices_currency.as_str()) { + return Err(cfg_err(format!( + "PRICES_CURRENCY {:?} is not supported; supported: {}", + self.features.prices_currency, + SUPPORTED_PRICE_CURRENCIES.join(", ") + ))); + } } - // Per-enabled-chain infra secrets: hard error in production, warn in dev. + // Per-enabled-chain infra config: hard error in production, warn in dev. + // This keeps /capabilities honest — an enabled chain that can't be served + // (no node/secret) must not boot in prod advertising itself as available. let mut chain_warnings: Vec<&str> = Vec::new(); + if self.features.chains.btc + && self.chains.btc.electrum_primary.is_none() + && self.chains.btc.electrum_fallbacks.is_empty() + { + chain_warnings.push("BTC_ELECTRUM_URL"); + } + if self.features.chains.ltc + && self.chains.ltc.electrum_primary.is_none() + && self.chains.ltc.electrum_fallbacks.is_empty() + { + chain_warnings.push("LTC_ELECTRUM_URL"); + } if self.features.chains.xmr && self.chains.xmr.lws_admin_key.is_empty() { chain_warnings.push("XMR_LWS_ADMIN_KEY"); } @@ -528,12 +569,12 @@ impl Config { if !chain_warnings.is_empty() { if prod { return Err(cfg_err(format!( - "missing required secrets for enabled chains: {}", + "missing required config for enabled chains: {}", chain_warnings.join(", ") ))); } for w in &chain_warnings { - tracing::warn!("{w} is empty — set this before enabling that chain in production"); + tracing::warn!("{w} is unset — set this before enabling that chain in production"); } } @@ -694,4 +735,44 @@ mod tests { c.features.chains.xmr = true; // xmr.lws_admin_key is empty assert!(c.validate().is_err()); } + + #[test] + fn unsupported_price_provider_rejected() { + let mut c = valid(); + c.features.prices = true; + c.features.prices_provider = "binance".into(); + assert!(c.validate().is_err()); + } + + #[test] + fn unsupported_price_asset_rejected() { + let mut c = valid(); + c.features.prices = true; + c.features.prices_assets = vec!["doge".into()]; + assert!(c.validate().is_err()); + } + + #[test] + fn disabled_prices_skips_provider_validation() { + let mut c = valid(); + c.features.prices = false; + c.features.prices_provider = "binance".into(); + assert!(c.validate().is_ok()); + } + + #[test] + fn unsupported_price_currency_rejected() { + let mut c = valid(); + c.features.prices = true; + c.features.prices_currency = "usdd".into(); // typo + assert!(c.validate().is_err()); + } + + #[test] + fn enabled_utxo_chain_without_electrum_rejected_in_production() { + let mut c = valid(); + c.environment = "production".into(); + c.features.chains.btc = true; // btc has no electrum_primary/fallbacks + assert!(c.validate().is_err()); + } } diff --git a/src/infra/prices.rs b/src/infra/prices.rs index 9dd0fbb..36499c1 100644 --- a/src/infra/prices.rs +++ b/src/infra/prices.rs @@ -19,6 +19,10 @@ const REQUEST_TIMEOUT: Duration = Duration::from_secs(15); const COINGECKO_URL: &str = "https://api.coingecko.com/api/v3/simple/price"; /// Lower bound on the refresh interval, to stay within free-tier rate limits. const MIN_INTERVAL_SECS: u64 = 60; +/// Upper bound on the price response body. The real payload is a handful of +/// numbers; this caps memory if the (otherwise trusted) host misbehaves, matching +/// the bounded-read convention used by the chain clients. +const MAX_PRICE_BODY_BYTES: usize = 256 * 1024; /// Our asset symbols mapped to their CoinGecko coin ids. const COIN_IDS: &[(&str, &str)] = &[ @@ -61,8 +65,9 @@ pub struct PriceClient { impl PriceClient { /// Build a client for `provider` (e.g. `"coingecko"`), quoting `assets` in /// `currency` (e.g. `"usd"`). Assets not in [`COIN_IDS`] are dropped here - /// (config validation rejects them earlier). The provider is validated - /// lazily on first fetch. + /// (config validation rejects them earlier). The provider is enforced + /// fail-closed at startup by `Config::validate`; the `fetch` dispatch arm is + /// a belt-and-suspenders fallback. pub fn new(provider: &str, currency: &str, assets: &[String]) -> Result { let http = reqwest::Client::builder() .timeout(REQUEST_TIMEOUT) @@ -94,6 +99,11 @@ impl PriceClient { self.feeds.is_empty() } + #[cfg(test)] + fn feed_symbols(&self) -> Vec<&str> { + self.feeds.iter().map(|(s, _)| s.as_str()).collect() + } + /// Fetch the current price for every enabled feed. Assets the provider omits /// (or returns non-finite/negative) are skipped, not faked. pub async fn fetch(&self) -> Result, AppError> { @@ -132,11 +142,10 @@ impl PriceClient { ))); } // Shape: { "bitcoin": { "usd": 12345.6 }, ... }. The host is fixed and - // the payload is a handful of numbers, so `json()` (timeout-bounded) is - // sufficient here without the streaming cap the chain clients need. - let raw: HashMap> = resp - .json() - .await + // trusted, but the body is still read with a streaming size cap (as the + // chain clients do) so a misbehaving upstream can't force a large alloc. + let body = read_capped(resp, MAX_PRICE_BODY_BYTES).await?; + let raw: HashMap> = serde_json::from_slice(&body) .map_err(|_| AppError::NodeError("invalid price response".into()))?; let mut out = HashMap::new(); @@ -156,6 +165,24 @@ pub fn refresh_interval(configured_secs: u64) -> Duration { Duration::from_secs(configured_secs.max(MIN_INTERVAL_SECS)) } +/// Read a response body, failing if it exceeds `cap` bytes (content-length is +/// attacker-assertable, so the limit is enforced as bytes actually arrive). +async fn read_capped(resp: reqwest::Response, cap: usize) -> Result, AppError> { + use futures::StreamExt; + let mut stream = resp.bytes_stream(); + let mut buf: Vec = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|_| AppError::NodeError("price read failed".into()))?; + if buf.len().saturating_add(chunk.len()) > cap { + return Err(AppError::NodeError( + "price response exceeded size limit".into(), + )); + } + buf.extend_from_slice(&chunk); + } + Ok(buf) +} + #[cfg(test)] mod tests { use super::*; @@ -194,6 +221,14 @@ mod tests { assert_eq!(mapped, supported); } + #[test] + fn subset_whitelist_selects_exactly_those_feeds() { + let client = PriceClient::new("coingecko", "usd", &["btc".into(), "grin".into()]).unwrap(); + let mut syms = client.feed_symbols(); + syms.sort(); + assert_eq!(syms, vec!["btc", "grin"]); + } + #[test] fn unknown_and_empty_whitelists_yield_no_feeds() { let client = PriceClient::new("coingecko", "usd", &["doge".to_string()]).unwrap(); diff --git a/tests/capabilities.rs b/tests/capabilities.rs index fe8ccd7..921b863 100644 --- a/tests/capabilities.rs +++ b/tests/capabilities.rs @@ -19,9 +19,19 @@ async fn capabilities_is_public_and_well_shaped() { "chain {chain} reports enabled" ); } - // UTXO chains carry a network; CryptoNote/Grin don't. - assert!(body["chains"]["btc"]["network"].as_str().is_some()); - assert!(body["chains"]["xmr"]["network"].is_null()); + // UTXO chains carry a network; CryptoNote/Grin report null. + for chain in ["btc", "ltc"] { + assert!( + body["chains"][chain]["network"].as_str().is_some(), + "{chain} carries a network" + ); + } + for chain in ["xmr", "wow", "grin"] { + assert!( + body["chains"][chain]["network"].is_null(), + "{chain} network is null" + ); + } // Feature flags are present. for feat in ["grin_relay", "prices", "nostr_identity", "tips"] { assert!( diff --git a/tests/prices.rs b/tests/prices.rs index 6037465..01ca2ec 100644 --- a/tests/prices.rs +++ b/tests/prices.rs @@ -21,10 +21,9 @@ async fn prices_endpoint_respects_the_feature_flag() { assert_eq!(status, StatusCode::NOT_FOUND); assert_eq!(body["code"], "NOT_FOUND"); - // Enabled → 200 with the documented shape. No background task in the harness, - // so prices are empty and updated_at is null until the first refresh. + // Enabled, before any refresh → 200 with the documented empty shape (the + // background task lives in `main`, not the harness, so updated_at is null). std::env::set_var("FEATURE_PRICES", "true"); - std::env::set_var("PRICES_ASSETS", "btc,xmr"); let app = common::try_app() .await .expect("app builds with prices enabled"); @@ -34,6 +33,24 @@ async fn prices_endpoint_respects_the_feature_flag() { assert!(body["prices"].is_object(), "prices is a map"); assert!(body["updated_at"].is_null(), "not yet refreshed"); + // Enabled, with a populated snapshot → the handler serves the quotes and a + // non-null RFC 3339 timestamp. Drives the serving path the inert + // PRICES_ASSETS env-var never could; feed *selection* is unit-tested in + // infra::prices. + { + let mut snap = app.state.prices.write().await; + snap.prices.insert("btc".into(), 64000.0); + snap.prices.insert("xmr".into(), 150.0); + snap.updated_at = Some(chrono::Utc::now()); + } + let (status, body) = app.request("GET", "/api/v1/prices", None, None).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(body["prices"]["btc"], 64000.0); + assert_eq!(body["prices"]["xmr"], 150.0); + assert!( + body["updated_at"].as_str().is_some(), + "timestamp serialized" + ); + std::env::remove_var("FEATURE_PRICES"); - std::env::remove_var("PRICES_ASSETS"); }