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

fix: applied rate limit to otp generation and verification #591

Merged
merged 2 commits into from
Sep 16, 2020

Conversation

Oxiang
Copy link
Contributor

@Oxiang Oxiang commented Sep 16, 2020

Problem

Prevent abuse of the endpoint responsible for sending OTPs by introducing rate limiting, using the express-rate-limit package already available in GoGovSG.

Solution

Adding rate limits for /otp endpoints.
For the rate limit, the maximum tries are set to 5 per minute. The numbers are derived from common user's usage. Failing to obtain the OTP up to 3 ~ 4 times in a minute is plausible for normal users. Anything that is 5 and above is likely to be intentional.

Copy link
Contributor

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

As discussed, rate limiting for verification is not needed

@Oxiang Oxiang requested a review from yong-jie September 16, 2020 05:48
@yong-jie
Copy link
Member

I tested on gitpod and made repeated OTP generate attempts. The client-side error that comes out when we are rate-limited is this. Might need to handle HTTP-status: 429 errors better to inform users about the rate-limit, but i'm not sure if it's within the scope of this current issue

image

Copy link
Member

@yong-jie yong-jie left a comment

Choose a reason for hiding this comment

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

lgtm. The issue on a more informative client-side error message is a separate task that can be addressed at another time.

@Oxiang Oxiang requested a review from LoneRifle September 16, 2020 06:14
@yong-jie
Copy link
Member

As discussed, rate limiting for verification is not needed

The changes requested have been addressed. I'll be dismissing this review so it doesnt block our merge 🙏

@yong-jie yong-jie dismissed LoneRifle’s stale review September 16, 2020 06:16

changes requested have been addressed

@yong-jie yong-jie merged commit 27a9f2f into develop Sep 16, 2020
@yong-jie yong-jie deleted the rate-limiter-update branch September 16, 2020 06:17
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.

3 participants