Skip to content
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

hotfix - twilio ddos fix #9237

Merged
merged 4 commits into from
Jun 30, 2021
Merged

hotfix - twilio ddos fix #9237

merged 4 commits into from
Jun 30, 2021

Conversation

owocki
Copy link
Contributor

@owocki owocki commented Jun 30, 2021

Description

some butthead is going in and ddosing our twilio account with twilio verify requests, which cost like 1c each... and in aggregate costing us hundreds of $$$$ per round.

this PR adds a rate limit to the twilio endpoint call so that it cant be done anymore.

ideally we'd extend this even further and block ppl who abuse the system as such.. but i dont have cycles to code that today

Refers/Fixes

https://app.datadoghq.com/logs?query=request_user_sms&index=
https://www.twilio.com/console/verify/services

Testing

did not test, but its a simple addition of a rate limit so hope its ok

@owocki
Copy link
Contributor Author

owocki commented Jun 30, 2021

note engineering team: - i did not get a chance to test as im on a laptoip without docker privisioned and in between calls. can you quickly test before shipping?

Copy link
Contributor

@gdixon gdixon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

Screenshot 2021-06-30 at 18 11 08

Do we want to add any error messages to the frontend?

@owocki
Copy link
Contributor Author

owocki commented Jun 30, 2021

a graceful failure eg "Youre ratelimited -Please try again in 1 minute" would prob be nice

@gdixon
Copy link
Contributor

gdixon commented Jun 30, 2021

It looks like the reason we were getting so many requests to the whitepaper was because it was set as the response to ratelimited requests - I've changed that and added the error response to the front-end for sms-verification 👍

Screenshot 2021-06-30 at 19 02 47

@octavioamu octavioamu merged commit 9f22593 into stable Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants