-
Notifications
You must be signed in to change notification settings - Fork 0
Security Blocklist + Rate Limiting Features #3
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR implements a blacklist and configurable interaction limit system for the WhatsApp bot. The changes allow administrators to block phone numbers and set per-event interaction limits to prevent abuse.
Key changes:
- New
blacklist_helpers.pymodule with caching logic for blocked numbers and interaction limits - Integration of blacklist checks at the beginning of all conversation handlers
- Replacement of hardcoded 450 interaction limit with dynamic, event-specific limits
- Initialization scripts for setting up blacklist configuration and default interaction limits
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| whatsapp_bot/app/utils/blacklist_helpers.py | New helper module implementing cached blacklist checking and configurable interaction limit retrieval |
| whatsapp_bot/app/handlers/SurveyMode.py | Added blacklist check and dynamic interaction limit enforcement |
| whatsapp_bot/app/handlers/ListenerMode.py | Added blacklist check and dynamic interaction limit enforcement with duplicate import |
| whatsapp_bot/app/handlers/FollowupMode.py | Added blacklist check and dynamic interaction limit enforcement |
| tools/initialize_survey_event.py | Added default interaction_limit field to event initialization |
| tools/initialize_listener_event.py | Minor formatting change |
| tools/blockednumbers.py | New tool for initializing blacklist configuration in Firestore |
| tools/2ndRoundDeliberation/initialize_listener_event.py | Added default interaction_limit field to event initialization |
| tools/2ndRoundDeliberation/initialize_followup_event.py | Added default interaction_limit field to event initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
justinstimatze
left a comment
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.
Would be nice to have a little test coverage on the blocklist logic if possible. I think this is ok but I'd like to have @juggler434 look before I approve.
| logger.info(f"Received message from {From} with body '{Body}' and media URL {MediaUrl0}") | ||
|
|
||
| # Normalize phone number | ||
| normalized_phone = From.replace("+", "").replace("-", "").replace(" ", "") |
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.
Feels like this snippet should probably be a utility function since it's repeated a few times.
juggler434
left a comment
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.
Overall, the logic looks sound. I would really like to see some unit test coverage in the repo though.
Add fallback handling for openai LLM failures (nonresponsive scenarios)
added a small unit test - they just validate the local caching and fallback logic using monkeypatches: but I feel more comfortable to always do real deployment checks (since its always less than 1-2 mins to deploy). |
What is the goal of this PR?
Key Changes
Testing