-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Gateway Telegram polling path bypasses security controls (fixes #1747) #1791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5ae1c95
155d42a
8a11f68
241f95e
cd23388
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,37 +181,10 @@ async def start(self) -> None: | |
| ) | ||
|
|
||
| async def handle_message(update: Update, context: ContextTypes.DEFAULT_TYPE): | ||
| # Handle text OR audio messages | ||
| message_text = None | ||
|
|
||
| if update.message: | ||
| # Check for voice/audio first | ||
| if update.message.voice or update.message.audio: | ||
| message_text = await self._transcribe_audio(update) | ||
| elif update.message.text: | ||
| message_text = update.message.text | ||
|
|
||
| if not update.message or not message_text: | ||
| return | ||
|
|
||
| message = self._convert_update_to_message(update, override_text=message_text) | ||
|
|
||
| # Add channel type for pairing system | ||
| message._channel_type = "telegram" | ||
|
|
||
| self.fire_message_received(message) | ||
|
|
||
| # Check if channel is allowed | ||
| if not self.config.is_channel_allowed(message.channel.channel_id if message.channel else ""): | ||
| return | ||
|
|
||
| # Handle unknown users with pairing system | ||
| user_id = message.sender.user_id if message.sender else "" | ||
| is_explicitly_allowed = bool(self.config.allowed_users) and self.config.is_user_allowed(user_id) | ||
| if not is_explicitly_allowed: | ||
| user_allowed = await UnknownUserHandler.handle(message, self._bot_context) | ||
| if not user_allowed: | ||
| return | ||
| # Use shared security pipeline for consistent enforcement | ||
| message = await process_inbound_telegram_message(update, self) | ||
| if not message: | ||
| return # Message was dropped by security checks | ||
|
|
||
| for handler in self._message_handlers: | ||
| try: | ||
|
|
@@ -252,7 +225,7 @@ async def _tg_unreact(emoji, **kw): | |
| update.message.from_user.username or update.message.from_user.first_name or "" | ||
| ) if update.message.from_user else "" | ||
| try: | ||
| message_text = await self._debouncer.debounce(user_id, message_text) | ||
| message_text = await self._debouncer.debounce(user_id, message.content) | ||
|
|
||
| # Show typing indicator with renewal during long operation | ||
| if self.config.typing_indicator: | ||
|
|
@@ -848,3 +821,114 @@ async def reply(self, chat_id: str, text: str) -> None: | |
| except Exception as e: | ||
| logger.error(f"Failed to send reply: {e}") | ||
|
|
||
|
|
||
| async def process_inbound_telegram_message( | ||
| update, # Telegram Update | ||
| bot: TelegramBot, | ||
| gateway_context: Optional[Dict] = None | ||
| ) -> Optional[BotMessage]: | ||
| """ | ||
| Shared security pipeline for processing inbound Telegram messages. | ||
|
|
||
| Used by both standalone bot (TelegramBot.handle_message) and | ||
| gateway polling (_start_telegram_bot_polling) to ensure consistent | ||
| access control enforcement. | ||
|
|
||
| Args: | ||
| update: Telegram Update object | ||
| bot: TelegramBot instance with config and security settings | ||
| gateway_context: Optional dict with gateway-specific context | ||
|
|
||
| Returns: | ||
| BotMessage if message passes all security checks, None if dropped | ||
| """ | ||
| if not update.message: | ||
| return None | ||
|
|
||
| # Extract message text (including audio transcription) | ||
| message_text = None | ||
| if update.message.voice or update.message.audio: | ||
| message_text = await bot._transcribe_audio(update) | ||
| elif update.message.text: | ||
| message_text = update.message.text | ||
|
|
||
| if not message_text: | ||
| return None | ||
|
|
||
| # Convert to BotMessage for consistent processing | ||
| message = bot._convert_update_to_message(update, override_text=message_text) | ||
|
|
||
| # Set channel type for pairing system | ||
| message._channel_type = "telegram" | ||
|
|
||
| # Fire message received event | ||
| bot.fire_message_received(message) | ||
|
|
||
| # 1. Channel allowlist check | ||
| channel_id = message.channel.channel_id if message.channel else "" | ||
| if not bot.config.is_channel_allowed(channel_id): | ||
| logger.debug(f"Message dropped: channel {channel_id} not in allowed_channels") | ||
| return None | ||
|
|
||
| # 2. User allowlist and pairing check | ||
| user_id = message.sender.user_id if message.sender else "" | ||
| is_explicitly_allowed = bot.config.is_user_allowed(user_id) | ||
|
|
||
| if not is_explicitly_allowed: | ||
| # Check if bot context is available for pairing system | ||
|
Comment on lines
+873
to
+878
|
||
| if not hasattr(bot, '_bot_context') or bot._bot_context is None: | ||
| # For gateway mode, we need to create bot context on demand | ||
| if not hasattr(bot, '_pairing_store'): | ||
| from ..gateway.pairing import PairingStore | ||
| bot._pairing_store = PairingStore() | ||
|
|
||
| bot._bot_context = BotContext( | ||
| config=bot.config, | ||
| pairing_store=bot._pairing_store, | ||
| adapter=bot | ||
| ) | ||
|
|
||
| user_allowed = await UnknownUserHandler.handle(message, bot._bot_context) | ||
| if not user_allowed: | ||
| logger.debug(f"Message dropped: user {user_id} not allowed by pairing system") | ||
| return None | ||
|
Comment on lines
+873
to
+894
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. allowed_users bypassed via pairing In process_inbound_telegram_message(), users not in allowed_users can still be permitted if UnknownUserHandler.handle() returns True (e.g., already paired or policy allows), which can allow unauthorized users to reach _session.chat() in gateway polling mode. Agent Prompt
Comment on lines
+873
to
+894
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3. Empty allowlist drops all process_inbound_telegram_message() treats users as “not explicitly allowed” when allowed_users is empty and then invokes UnknownUserHandler.handle(); with BotConfig.unknown_user_policy defaulting to "deny", this drops all inbound messages instead of allowing all (the documented empty-allowlist behavior). This will also make the new test_empty_allowlists_allow_all fail and contradicts the gateway warning that empty allowed_users “accepts messages from everyone.” Agent Prompt
Comment on lines
+873
to
+894
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old Concrete failure: a standalone bot deployed with |
||
|
|
||
| # 3. Group policy enforcement | ||
| if message.channel and message.channel.channel_type not in ("dm", "private"): | ||
| # This is a group/channel message, check group policies | ||
| group_policy = getattr(bot.config, 'group_policy', 'mention_only') | ||
| mention_required = getattr(bot.config, 'mention_required', True) | ||
|
|
||
| if group_policy == "command_only": | ||
| if message.message_type != MessageType.COMMAND: | ||
| logger.debug(f"Message dropped: non-command in command_only group {channel_id}") | ||
| return None | ||
| elif group_policy == "mention_only": | ||
| # Check if bot was mentioned in the message | ||
|
Comment on lines
+899
to
+907
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 4. Gateway ignores group_policy Gateway extracts group_policy from gateway.yaml but does not pass it into BotConfig; the new shared pipeline then reads bot.config.group_policy and defaults to "mention_only", so gateway Telegram will silently ignore respond_all/command_only and enforce mention-only behavior. This is a security/configuration correctness break because command_only becomes more permissive (mentions allowed) than configured. Agent Prompt
|
||
| bot_username = bot._bot_user.username.lower() if bot._bot_user and bot._bot_user.username else "" | ||
| mention_handle = f"@{bot_username}" if bot_username else "" | ||
| bot_mentioned = ( | ||
| mention_handle and mention_handle in message.content.lower() | ||
| ) or message.message_type == MessageType.COMMAND # Commands are always allowed | ||
|
|
||
| if not bot_mentioned: | ||
| logger.debug(f"Message dropped: bot not mentioned in group {channel_id}") | ||
| return None | ||
| elif group_policy == "respond_all": | ||
| # Allow all group messages | ||
| pass | ||
| elif mention_required: | ||
| # Fallback for backward compatibility when group_policy is not set | ||
| bot_username = bot._bot_user.username.lower() if bot._bot_user and bot._bot_user.username else "" | ||
| mention_handle = f"@{bot_username}" if bot_username else "" | ||
| bot_mentioned = ( | ||
| mention_handle and mention_handle in message.content.lower() | ||
| ) or message.message_type == MessageType.COMMAND # Commands are always allowed | ||
|
coderabbitai[bot] marked this conversation as resolved.
greptile-apps[bot] marked this conversation as resolved.
|
||
|
|
||
| if not bot_mentioned: | ||
| logger.debug(f"Message dropped: bot not mentioned in group {channel_id}") | ||
| return None | ||
|
Comment on lines
+899
to
+930
|
||
|
|
||
|
Comment on lines
+896
to
+931
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Group policy command_only bypass The shared pipeline’s group gating in process_inbound_telegram_message() does not correctly implement BotConfig.group_policy semantics: it applies mention-gating whenever mention_required is true and fails to properly enforce command_only (and can also mis-gate respond_all). This can cause incorrect allow/deny behavior, including responding to non-command group messages when policy should suppress replies or dropping messages when respond_all should allow them. Agent Prompt
|
||
| # All security checks passed | ||
| logger.debug(f"Message security checks passed for user {user_id} in channel {channel_id}") | ||
| return message | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gateway_contextparameter is declared in the function signature and documented in the docstring, but is never read anywhere in the function body. Dead parameters add noise and invite callers to pass data that is silently ignored.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!