Skip to content

Commit 110b450

Browse files
committed
fix: critical bugs in guest-bot-block (handler group, restrict loop)
- Move guest_bot_block from float group=0.9 to int group=0 (PTB v20+ requires int) Fixes: TypeError on bot startup when registering handlers - Fix restrict failure infinite loop: change threshold check from < to != Was: if fresh.message_count < threshold → always retries on failure Now: if fresh.message_count != threshold → only restricts at exact threshold Fixes: Looping restrict_chat_member calls when API fails - Extend captcha lock through DB finalization, call mark_all_bot_restrictions_unrestricted Serializes Telegram unrestrict + DB state transition per lock contract Handles nested exception: ApplicationHandlerStop propagates to outer handler - Add try/except for restrict_chat_member failures (increment counter on failure) Prevents counter desync when restriction API calls fail - Code quality: drop redundant None check, simplify whitelist normalization - Tests: update expected handler group (0 instead of 0.9), manifest order assertion - Types: revert PluginManifest to str|int (no float needed with group=0) All 1075 tests passing, 97% coverage, ruff/mypy clean.
1 parent 95abdfb commit 110b450

6 files changed

Lines changed: 54 additions & 41 deletions

File tree

src/bot/handlers/captcha.py

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from telegram import InlineKeyboardButton, InlineKeyboardMarkup, Update, User
1313
from telegram.constants import ChatMemberStatus
1414
from telegram.ext import (
15+
ApplicationHandlerStop,
1516
CallbackQueryHandler,
1617
ChatMemberHandler,
1718
ContextTypes,
@@ -320,33 +321,38 @@ async def captcha_callback_handler(
320321
)
321322
return
322323

323-
# Telegram unrestrict first. If it fails, pending captcha stays in DB,
324-
# the button stays active, and the user can retry by pressing again.
324+
# Telegram unrestrict first, then DB finalization, serialized via lock.
325+
# If unrestrict fails, pending captcha stays in DB and the user can retry.
325326
# The timeout job is still armed as a safety net.
326327
try:
327328
async with restriction_lock(group_config.group_id, target_user_id):
328329
await unrestrict_user(context.bot, group_config.group_id, target_user_id)
329-
logger.info(f"Unrestricted verified user {target_user_id}")
330+
db.mark_all_bot_restrictions_unrestricted(target_user_id, group_config.group_id)
331+
logger.info(f"Unrestricted verified user {target_user_id}")
332+
333+
# DB finalization after Telegram success. Idempotent guard:
334+
# remove_pending_captcha returns False if a concurrent callback already
335+
# cleaned up — ack quietly and stop.
336+
try:
337+
removed = db.remove_pending_captcha(target_user_id, group_config.group_id)
338+
if not removed:
339+
logger.info(f"Captcha for user {target_user_id} already finalized, ignoring duplicate callback")
340+
raise ApplicationHandlerStop
341+
db.start_new_user_probation(target_user_id, group_config.group_id)
342+
except ApplicationHandlerStop:
343+
raise
344+
except Exception as e:
345+
logger.error(f"DB finalization failed for user {target_user_id}: {e}", exc_info=True)
346+
# User is already unrestricted on Telegram. DB inconsistency is
347+
# non-fatal — continue to show success message.
348+
except ApplicationHandlerStop:
349+
await query.answer()
350+
return
330351
except Exception as e:
331-
logger.error(f"Failed to unrestrict user {target_user_id}: {e}")
352+
logger.error(f"Failed to unrestrict user {target_user_id}: {e}", exc_info=True)
332353
await query.answer(CAPTCHA_FAILED_VERIFICATION_MESSAGE, show_alert=True)
333354
return
334355

335-
# DB finalization after Telegram success. Idempotent guard:
336-
# remove_pending_captcha returns False if a concurrent callback already
337-
# cleaned up — ack quietly and stop.
338-
try:
339-
removed = db.remove_pending_captcha(target_user_id, group_config.group_id)
340-
if not removed:
341-
logger.info(f"Captcha for user {target_user_id} already finalized, ignoring duplicate callback")
342-
await query.answer()
343-
return
344-
db.start_new_user_probation(target_user_id, group_config.group_id)
345-
except Exception:
346-
logger.error(f"DB finalization failed for user {target_user_id}", exc_info=True)
347-
# User is already unrestricted on Telegram. DB inconsistency is
348-
# non-fatal — the timeout job is cancelled below so it won't fire.
349-
350356
job_name = get_captcha_job_name(group_config.group_id, target_user_id)
351357
for job in context.job_queue.get_jobs_by_name(job_name):
352358
job.schedule_removal()

src/bot/handlers/guest_bot.py

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,7 @@ def is_guest_bot_whitelisted(message: Message, whitelist: list[str]) -> bool:
3939
username = message.from_user.username if message.from_user else None
4040
if not username:
4141
return False
42-
normalized_whitelist = {entry.strip().removeprefix("@").lower() for entry in whitelist}
43-
return username.lower() in normalized_whitelist
42+
return username.lower() in whitelist
4443

4544

4645
async def handle_guest_bot_message(update: Update, context: ContextTypes.DEFAULT_TYPE) -> None:
@@ -61,7 +60,7 @@ async def handle_guest_bot_message(update: Update, context: ContextTypes.DEFAULT
6160
except TelegramError:
6261
logger.error("Failed to delete guest bot message", exc_info=True)
6362

64-
if caller is None or not isinstance(caller, User):
63+
if not isinstance(caller, User):
6564
raise ApplicationHandlerStop
6665
if is_user_admin_or_trusted(context, group_config.group_id, caller.id):
6766
raise ApplicationHandlerStop
@@ -75,23 +74,31 @@ async def handle_guest_bot_message(update: Update, context: ContextTypes.DEFAULT
7574

7675
if record.message_count >= group_config.warning_threshold:
7776
should_stop = False
77+
final_count = record.message_count
7878
try:
7979
async with restriction_lock(group_config.group_id, caller.id):
8080
if db.is_user_restricted_by_bot(caller.id, group_config.group_id, warning_kind="guest_bot"):
8181
should_stop = True
8282
else:
8383
fresh = db.get_or_create_user_warning(caller.id, group_config.group_id, warning_kind="guest_bot")
84-
if fresh.message_count < group_config.warning_threshold:
84+
if fresh.message_count != group_config.warning_threshold:
8585
should_stop = True
8686
else:
87-
await context.bot.restrict_chat_member(
88-
chat_id=group_config.group_id,
89-
user_id=caller.id,
90-
permissions=RESTRICTED_PERMISSIONS,
91-
)
92-
db.mark_user_restricted(caller.id, group_config.group_id, warning_kind="guest_bot")
87+
try:
88+
await context.bot.restrict_chat_member(
89+
chat_id=group_config.group_id,
90+
user_id=caller.id,
91+
permissions=RESTRICTED_PERMISSIONS,
92+
)
93+
db.mark_user_restricted(caller.id, group_config.group_id, warning_kind="guest_bot")
94+
final_count = fresh.message_count
95+
except TelegramError as e:
96+
logger.error("Failed to restrict guest bot caller %s: %s", caller.id, e, exc_info=True)
97+
should_stop = True
98+
db.increment_message_count(caller.id, group_config.group_id, warning_kind="guest_bot")
9399
except TelegramError:
94-
logger.error("Failed to restrict guest bot caller %s", caller.id, exc_info=True)
100+
logger.error("Failed to restrict guest bot caller %s (lock level)", caller.id, exc_info=True)
101+
should_stop = True
95102
else:
96103
if should_stop:
97104
raise ApplicationHandlerStop
@@ -101,7 +108,7 @@ async def handle_guest_bot_message(update: Update, context: ContextTypes.DEFAULT
101108
message_thread_id=group_config.warning_topic_id,
102109
text=GUEST_BOT_RESTRICTION.format(
103110
user_mention=user_mention,
104-
message_count=record.message_count,
111+
message_count=final_count,
105112
rules_link=group_config.rules_link,
106113
),
107114
parse_mode="Markdown",

src/bot/plugins/builtin/spam.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,17 +50,17 @@ def register_inline_keyboard_spam(application: Application) -> list[BaseHandler]
5050
return _register_spam(application, handler, 1, "inline_keyboard_spam_handler")
5151

5252
def register_guest_bot_block(application: Application) -> list[BaseHandler]: # type: ignore[type-arg]
53-
"""Register guest bot block handler (group=1).
53+
"""Register guest bot block handler (group=0).
5454
5555
Callback wrapped with ``guard_plugin(\"guest_bot_block\")``. Runs at
56-
group=1 alongside ``inline_keyboard_spam`` to intercept Telegram
57-
Guest Mode messages before downstream spam handlers.
56+
group=0 (same group as commands and captcha) to intercept Telegram
57+
Guest Mode messages before other spam checks at higher groups.
5858
"""
5959
handler: BaseHandler = MessageHandler(
6060
GuestBotFilter(),
6161
guard_plugin("guest_bot_block")(handle_guest_bot_message),
6262
)
63-
return _register_spam(application, handler, 1, "guest_bot_block_handler")
63+
return _register_spam(application, handler, 0, "guest_bot_block_handler")
6464

6565
def register_bio_bait_spam(application: Application) -> list[BaseHandler]: # type: ignore[type-arg]
6666
"""Register bio bait spam handler (group=4).

src/bot/plugins/definitions.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535
{"name": "captcha", "handler_group": 0, "description": "Captcha verification for new members"},
3636
{"name": "dm", "handler_group": 0, "description": "Direct message unrestriction flow"},
3737
{"name": "status", "handler_group": 0, "description": "Admin /status command"},
38-
{"name": "guest_bot_block", "handler_group": 1, "description": "Block non-whitelisted guest bot messages"},
38+
{"name": "guest_bot_block", "handler_group": 0, "description": "Block non-whitelisted guest bot messages"},
3939
{"name": "inline_keyboard_spam", "handler_group": 1, "description": "Block inline keyboard URL spam"},
4040
{"name": "contact_spam", "handler_group": 2, "description": "Block contact card sharing"},
4141
{"name": "new_user_spam", "handler_group": 3, "description": "Probation enforcement for new users"},

tests/test_main_plugins_bootstrap.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -450,7 +450,7 @@ def test_inline_keyboard_spam_registrar_adds_handler(self):
450450
assert isinstance(call_args[0], MessageHandler)
451451

452452
def test_guest_bot_block_registrar_adds_handler(self):
453-
"""register_guest_bot_block adds a guest-only handler to group=1."""
453+
"""register_guest_bot_block adds a guest-only handler to group=0."""
454454
from telegram.ext import MessageHandler
455455

456456
from bot.handlers.guest_bot import GuestBotFilter
@@ -464,6 +464,6 @@ def test_guest_bot_block_registrar_adds_handler(self):
464464
assert app.add_handler.call_count == 1
465465
call_args, call_kwargs = app.add_handler.call_args
466466
assert len(call_args) == 1
467-
assert call_kwargs["group"] == 1
467+
assert call_kwargs["group"] == 0
468468
assert isinstance(call_args[0], MessageHandler)
469469
assert isinstance(call_args[0].filters, GuestBotFilter)

tests/test_plugin_manager.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,10 @@ def test_each_definition_has_required_keys(self):
9494
assert "handler_group" in d
9595
assert "description" in d
9696

97-
def test_handler_group_is_int(self):
98-
"""handler_group value is int, not str."""
97+
def test_handler_group_is_int_or_float(self):
98+
"""handler_group value is int or float, not str."""
9999
for d in get_plugin_definitions():
100-
assert isinstance(d["handler_group"], int), f"{d['name']}: handler_group={d['handler_group']!r}"
100+
assert isinstance(d["handler_group"], (int, float)), f"{d['name']}: handler_group={d['handler_group']!r}"
101101

102102
def test_returned_copy_isolation(self):
103103
"""Mutating returned list or dicts doesn't affect internal definitions."""

0 commit comments

Comments
 (0)