🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Telegram topic/thread fallback can misdeliver replies to parent chat when thread ID is stale

Description

The new withTelegramThreadFallback logic retries Telegram sends without message_thread_id when Telegram returns "message thread not found".

message_thread_id is used in this codebase for:

• Telegram forum topics (scope: "forum"): routes replies into a specific topic within a group (chatId stays the same; message_thread_id selects the topic).
• Telegram DM topics (scope: "dm"): routes replies inside a private chat topic.

When the thread/topic ID is stale (deleted topic, wrong stored thread ID, etc.), the new fallback will resend the same content to the parent chat (no topic), which can cause unintended message delivery:

• In a forum group, dropping message_thread_id posts into the main chat/General context rather than the intended topic, potentially exposing content to a broader audience than intended.

Vulnerable behavior:

​// if thread not found -> retry without message_thread_id
return await send(removeMessageThreadIdParam(requestParams));

This is a behavior change from “fail closed” (no message delivered) to “fail open” (deliver to a different location) for forum-topic scoped sends.

Recommendation

Do not automatically fall back to a threadless send for contexts where message_thread_id is a routing/segregation boundary (forum topics).

Safer options:

1. Only allow this fallback for DM-topic sends (thread.scope === "dm"). For forum topics, rethrow and surface the error.
2. Alternatively, explicitly route to a safe/admin channel or create a new topic rather than posting to the parent chat.

Example (restrict fallback to DMs):

async function sendTelegramWithThreadFallback<T>(params: {
  operation: string;
  runtime: RuntimeEnv;
  requestParams?: Record<string, unknown>;
  allowThreadlessFallback?: boolean;
  send: (effectiveParams?: Record<string, unknown>) => Promise<T>;
}) {
  try {
    return await params.send(params.requestParams);
  } catch (err) {
    if (!params.allowThreadlessFallback) throw err;
    if (!hasMessageThreadIdParam(params.requestParams) || !isTelegramThreadNotFoundError(err)) throw err;
    return await params.send(removeMessageThreadIdParam(params.requestParams));
  }
}

​// call site (delivery.ts):
​// allowThreadlessFallback: thread?.scope === "dm"

This preserves the DM reliability improvement while preventing cross-topic misdelivery in forum/group contexts.

Analyzed PR: #30762 at commit 8cc1eeb

Greptile Summary

This PR introduces a retry mechanism for Telegram bot outbound sends (text and all media types) that automatically drops the stale message_thread_id when Telegram responds with "400: Bad Request: message thread not found". The implementation is clean and well-structured, with two new layered helpers (withTelegramThreadFallback and sendTelegramWithThreadFallback) and regression tests covering text and photo retry paths.

Key findings:

• False danger-level error logged on every successful retry — sendTelegramWithThreadFallback wraps each attempt in withTelegramApiErrorLogging, which logs the "thread not found" error via runtime.error(danger(...)) before withTelegramThreadFallback gets a chance to retry. For text sends the shouldLog guard only excludes parse/empty-text errors, so "thread not found" always passes through; for media sends there is no shouldLog at all. The message ultimately delivers successfully, but each successful retry emits a spurious danger-level error that can trigger false alerts in error monitoring.
• New tests do not cover the false-error side-effect — neither retry test asserts expect(runtime.error).not.toHaveBeenCalled(), so the spurious log goes undetected.
• The regex THREAD_NOT_FOUND_RE works correctly for real GrammyError instances because formatErrorMessage extracts err.message, which contains the full "(400: Bad Request: message thread not found)" string.

Confidence Score: 3/5

• The retry logic is functionally correct and messages are delivered, but the architecture causes false danger-level error logs on every successful thread-fallback retry, which can pollute error monitoring.
• The core retry logic is sound: the regex matches real GrammyError messages, hasMessageThreadIdParam and removeMessageThreadIdParam are correct, and the retry only triggers when appropriate. However, the nesting of withTelegramApiErrorLogging inside withTelegramThreadFallback's send callback means the "thread not found" error is always logged as a danger before the retry succeeds. This is a real behavioural issue (false error alerts), and the new tests do not detect it, warranting a score below fully safe.
• Pay close attention to src/telegram/bot/delivery.ts around the sendTelegramWithThreadFallback function, specifically how withTelegramApiErrorLogging and withTelegramThreadFallback are layered.

_{Last reviewed commit: 8cc1eeb}