From c1b90ba5f8dc31e2a0c78b6d1f9fd39c656f2a11 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Thu, 6 Aug 2026 22:34:30 +0100 Subject: [PATCH] fix: stop Telegram handing out pairing codes the store never saw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pairing failed with "invalid or expired pairing code" on a code the bot had just sent. handle_pairing read the pending codes from the in-memory `shared.bindings` cache, which is refreshed from the ConfigKeyUpdated broadcast — a lossy 64-slot bus. One dropped event is enough for that cache to keep a pending entry the store no longer has; the "reuse an existing code for this chat" branch then hits, and that branch does not write. The user gets a code, and the web page — which resolves it against the store — cannot find it. Before the move to the config store, this path re-read the file on every message and could not drift. The cache stays where it earns its keep, the chat_id → user_id lookup on every inbound message, where a stale read costs one message. Issuing a code now reads the store. Two silent failures on the same path, each able to produce the same symptom while hiding its cause: handle_pairing sent the code even when the write had failed — it logged and carried on — so the error surfaced later, somewhere else, as a code that simply would not bind. It now says so in the chat and hands out nothing. load_config turned an unparseable blob into `unwrap_or_default()`: no bindings, no pending codes. Every writer here saves the whole blob back, so the next pairing message would have overwritten the real config with that default and taken every binding on the box with it. An absent key is still an empty config — that is a fresh install — but an unreadable one is now an error that callers propagate, including start(), which fails loudly rather than running on a cache it knows is wrong. --- crates/plugin-telegram-bot/src/auth.rs | 80 +++++++++++++++++++++---- crates/plugin-telegram-bot/src/lib.rs | 15 +++-- crates/plugin-telegram-bot/src/tools.rs | 6 +- 3 files changed, 84 insertions(+), 17 deletions(-) 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.