-
Notifications
You must be signed in to change notification settings - Fork 1
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
fixed telegram respond issue #645
Conversation
Reviewer's Guide by SourceryThis PR fixes issues with Telegram bot response handling by correcting handler registration attributes and improving webhook setup. The changes focus on properly initializing message, callback, and command handlers while also enhancing the webhook security implementation. Sequence diagram for Telegram bot webhook setupsequenceDiagram
participant Bot as Telegram Bot
participant TelegramAPI as Telegram API
participant Settings as Settings
Bot->>TelegramAPI: POST /setWebhook
Note right of Bot: Using secret token for security
TelegramAPI-->>Bot: Webhook registered
Note over Bot,TelegramAPI: Webhook setup with security token
Updated class diagram for Telegram bot handlersclassDiagram
class BaseTelegramMessageHandler {
+message
}
class BaseTelegramCommandHandler {
+command
}
class Bot {
+callback_handlers
+message_handlers
+command_handlers
+ready()
}
Bot --> BaseTelegramMessageHandler : uses
Bot --> BaseTelegramCommandHandler : uses
note for Bot "Registers handlers for messages, callbacks, and commands"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @alimaktabi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider validating the response from the webhook setup request to ensure it succeeded. A failed webhook registration could cause issues that are hard to debug.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
webhook_url = "https://api.unitap.app/api/telegram/wh/" | ||
telegram_api_url = ( | ||
f"https://api.telegram.org/bot{telebot_instance.token}/setWebhook" | ||
) | ||
|
||
# Register webhook with secret token for added security | ||
requests.post( | ||
res = requests.post( |
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.
issue (bug_risk): The webhook registration response should be checked for errors to ensure proper setup
Consider validating the response status code and handling potential errors to prevent silent failures in webhook setup
@@ -46,8 +46,6 @@ def telebot_respond(request): | |||
# if client_ip not in telegram_ips: |
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.
🚨 issue (security): IP validation should not be commented out as it reduces security defense in depth
While secret token validation provides security, IP validation adds an additional important security layer. Consider re-enabling it or documenting why it's disabled.
Summary by Sourcery
Fix the Telegram bot response issue by correctly registering message, callback, and command handlers, and enhance webhook registration by capturing the response from the requests.post call.
Bug Fixes:
Enhancements: