-
Notifications
You must be signed in to change notification settings - Fork 0
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
Develop #646
Develop #646
Conversation
fixed telegram respond issue
Reviewer's Guide by SourceryThis PR implements several improvements to the Telegram bot implementation, focusing on webhook security, handler registration, and code cleanup. The changes include proper handler attribute references, webhook response handling, and removal of debug logging. Sequence diagram for webhook registration processsequenceDiagram
participant Bot
participant TelegramAPI
Bot->>TelegramAPI: POST /setWebhook with secret_token
TelegramAPI-->>Bot: Response stored in 'res'
Updated class diagram for handler registrationclassDiagram
class BaseTelegramMessageHandler {
+message
}
class BaseTelegramCommandHandler {
+command
}
class Bot {
+callback_handlers
+message_handlers
+command_handlers
}
Bot --> BaseTelegramMessageHandler : registers
Bot --> BaseTelegramCommandHandler : registers
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:
- The webhook registration response (res) is captured but never checked. Consider adding error handling to verify the webhook was set up successfully.
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
Enhance the Telegram bot by improving webhook registration and refactoring handler registration to use more specific attributes.
Enhancements: