fix(prices,config): harden per-feed control + fail-closed gaps (adversarial review)

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

@@ -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());
+    }
 }

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<Self, AppError> {
         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<HashMap<String, f64>, 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<String, HashMap<String, f64>> = 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<String, HashMap<String, f64>> = 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<Vec<u8>, AppError> {
+    use futures::StreamExt;
+    let mut stream = resp.bytes_stream();
+    let mut buf: Vec<u8> = 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();

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!(

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");
 }