diff --git a/crates/plugin-telegram-bot/src/auth.rs b/crates/plugin-telegram-bot/src/auth.rs index 7c743f6..edc8414 100644 --- a/crates/plugin-telegram-bot/src/auth.rs +++ b/crates/plugin-telegram-bot/src/auth.rs @@ -44,12 +44,23 @@ pub struct PairingEntry { // ── Config-table read/write ──────────────────────────────────────────────────── -/// Reads the Telegram config from the `config` table. Returns `Default` when -/// the key is absent or unparseable (never fails the caller). +/// Reads the Telegram config from the `config` table. +/// +/// An **absent** key is an empty config — that is the state of a fresh install. +/// An **unparseable** one is an error, deliberately: this used to be +/// `unwrap_or_default()`, which turned a blob the current schema cannot read +/// into "no bindings, no pending codes" — and since every writer here saves the +/// whole blob back, the next pairing message would then overwrite the file with +/// that default and every binding on the box would be gone for good. Failing +/// loudly leaves the value intact for a human to look at. pub(crate) async fn load_config(config: &dyn ConfigApi) -> anyhow::Result { match config.get(CONFIG_KEY).await? { - Some(json) => Ok(serde_json::from_str(&json).unwrap_or_default()), - None => Ok(TelegramConfig::default()), + Some(json) => serde_json::from_str(&json) + .map_err(|e| anyhow::anyhow!( + "telegram: the stored `{CONFIG_KEY}` config is not readable ({e}) — \ + refusing to overwrite it; inspect the `config` table" + )), + None => Ok(TelegramConfig::default()), } } @@ -70,8 +81,26 @@ const PAIRING_TTL_HOURS: i64 = 24; /// Called when an unbound `chat_id` sends a message. Generates (or reuses) a /// pairing code, persists it to the config table, and replies with instructions. +/// +/// **Reads the store, not `shared.bindings`.** The cache is refreshed from a +/// lossy 64-slot broadcast (`ConfigKeyUpdated`), so it may hold a pending code +/// the store no longer has — a dropped event is enough. That cache is right for +/// the hot `chat_id → user_id` lookup on every inbound message; it is wrong +/// here, because the reader on the other side of the pairing (the web page and +/// the `telegram_pairing` tool) resolves the code against the **store**, and a +/// code handed out from a stale cache is one that can never bind: the user gets +/// their code and the web answers "invalid or expired". Pairing happens once +/// per person, so the extra read costs nothing. pub(crate) async fn handle_pairing(bot: &Bot, chat_id: ChatId, shared: &Arc) { - let mut cfg = shared.bindings.read().await.clone(); + let mut cfg = match load_config(&*shared.config).await { + Ok(c) => c, + Err(e) => { + error!(error = %e, "telegram: cannot read the config to issue a pairing code"); + bot.send_message(chat_id, "⚠️ Pairing is unavailable right now — please ask the admin to check the server.") + .await.ok(); + return; + } + }; // Prune expired codes. let cutoff = Utc::now() - chrono::Duration::hours(PAIRING_TTL_HOURS); @@ -94,14 +123,19 @@ pub(crate) async fn handle_pairing(bot: &Bot, chat_id: ChatId, shared: &Arc); + + #[async_trait::async_trait] + impl ConfigApi for FakeConfig { + async fn get(&self, _key: &str) -> anyhow::Result> { Ok(self.0.clone()) } + async fn set(&self, _key: &str, _value: &str) -> anyhow::Result<()> { Ok(()) } + } + + /// The distinction the silent `unwrap_or_default()` used to erase: an absent + /// key is a fresh install, an unreadable one must not present itself as an + /// empty config that the next write would then persist over the real one. + #[tokio::test] + async fn an_absent_key_is_empty_and_an_unreadable_one_is_an_error() { + let empty = load_config(&FakeConfig(None)).await.unwrap(); + assert!(empty.bindings.is_empty() && empty.pending_pairings.is_empty()); + + let err = load_config(&FakeConfig(Some("{ not json".into()))).await.unwrap_err(); + assert!(err.to_string().contains("not readable"), "got: {err}"); + + // A blob from a future/other schema is unreadable too — `bindings` must + // be an array of objects, and a wrong shape has to fail, not default. + assert!(load_config(&FakeConfig(Some(r#"{"bindings":"nope"}"#.into()))).await.is_err()); + } + #[test] fn unknown_code_fails_and_keeps_state() { let mut cfg = cfg_with_pairing("ABC123", 42); diff --git a/crates/plugin-telegram-bot/src/lib.rs b/crates/plugin-telegram-bot/src/lib.rs index 07053ab..791c80c 100644 --- a/crates/plugin-telegram-bot/src/lib.rs +++ b/crates/plugin-telegram-bot/src/lib.rs @@ -114,6 +114,11 @@ pub(crate) struct TgShared { pub(crate) location: Arc, // ── Pairing / bindings (config-table-backed, cached in memory) ── + /// Hot-path cache for the `chat_id → user_id` lookup every inbound message + /// does. Refreshed from the (lossy) `ConfigKeyUpdated` broadcast, so it is + /// eventually-consistent by construction: fine for a binding, where a + /// dropped event costs one message, and **not** fine for issuing a pairing + /// code, which reads the store directly (see `auth::handle_pairing`). pub(crate) bindings: RwLock, // ── Per-chat pending state ── @@ -243,7 +248,7 @@ impl Plugin for TelegramPlugin { let shared = self.shared() .ok_or_else(|| anyhow::anyhow!("telegram: the bot is not running — ask the admin to check the plugin"))? .clone(); - let mut cfg = auth::load_config(&*shared.config).await.unwrap_or_default(); + let mut cfg = auth::load_config(&*shared.config).await?; let chat_id = auth::apply_pairing_code(&mut cfg, code, user_id)?; auth::save_config(&*shared.config, &cfg).await?; ctx.user_config @@ -293,9 +298,11 @@ impl Plugin for TelegramPlugin { anyhow::bail!("telegram: token is empty — set it via the plugins API"); } - // Load bindings from the config table (or default if absent). - let telegram_config = auth::load_config(&*ctx.config).await - .unwrap_or_default(); + // Load bindings from the config table (empty if the key is absent). An + // unreadable blob fails the start on purpose — running with an empty + // cache would hand out pairing codes the store contradicts and let the + // first write bury the real bindings. + let telegram_config = auth::load_config(&*ctx.config).await?; info!( bindings = telegram_config.bindings.len(), pending = telegram_config.pending_pairings.len(), diff --git a/crates/plugin-telegram-bot/src/tools.rs b/crates/plugin-telegram-bot/src/tools.rs index 4683be5..3f42919 100644 --- a/crates/plugin-telegram-bot/src/tools.rs +++ b/crates/plugin-telegram-bot/src/tools.rs @@ -311,7 +311,7 @@ impl Tool for TelegramPairingTool { match action { "list" => { - let cfg = load_config(cfg_api).await.unwrap_or_default(); + let cfg = load_config(cfg_api).await?; if cfg.bindings.is_empty() { return Ok("No Telegram bindings.".to_string()); } @@ -327,7 +327,7 @@ impl Tool for TelegramPairingTool { .and_then(Value::as_i64) .ok_or_else(|| anyhow::anyhow!("telegram_pairing: `chat_id` required for unbind"))?; - let mut cfg = load_config(cfg_api).await.unwrap_or_default(); + let mut cfg = load_config(cfg_api).await?; let before = cfg.bindings.len(); cfg.bindings.retain(|b| b.chat_id != chat_id); if cfg.bindings.len() == before { @@ -338,7 +338,7 @@ impl Tool for TelegramPairingTool { } "bind" => { - let mut cfg = load_config(cfg_api).await.unwrap_or_default(); + let mut cfg = load_config(cfg_api).await?; // Resolve chat_id + user_id either from a pairing code or // from explicit arguments.