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

feat: add rate limiters for sending emails and login #1632

Conversation

adrianboros
Copy link
Contributor

Context

Changes

  • add rate limiters for sending verify and forgot pass emails
  • add rate limiters on login attempts

@adrianboros adrianboros linked an issue Sep 19, 2024 that may be closed by this pull request
@github-actions github-actions bot added package: wallet/backend Wallet backend implementations type: source Source changes labels Sep 19, 2024
@adrianboros adrianboros marked this pull request as ready for review September 19, 2024 19:53
@Tymmmy Tymmmy requested a review from sanducb September 21, 2024 17:38
@adrianboros
Copy link
Contributor Author

This rate limit implementation works as route middleware, which means we count the endpoint hit and NOT failed login attempts. We can hit the rate limit even with successful logins if we login/logout multiple times in a short amount of time.

Questions:

  • should we set these rate limits as env variables to be easier to update?
  • for login rate limit, should we count only failed attempts and not all route hits? in order to count just failed login attempts, we can move login route rate limit logic from a middleware to a service and use it in the authService.authorize function.

Copy link
Contributor

@lengyel-arpad85 lengyel-arpad85 left a comment

Choose a reason for hiding this comment

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

looks good,
I would add some tests as well, since we already have some for login and resend in packages/wallet/backend/tests/auth/controller.test.ts
extend this or add some for the rate limit

@lengyel-arpad85
Copy link
Contributor

This rate limit implementation works as route middleware, which means we count the endpoint hit and NOT failed login attempts. We can hit the rate limit even with successful logins if we login/logout multiple times in a short amount of time.

Questions:

  • should we set these rate limits as env variables to be easier to update?
  • for login rate limit, should we count only failed attempts and not all route hits? in order to count just failed login attempts, we can move login route rate limit logic from a middleware to a service and use it in the authService.authorize function.
  • yes I think having the rate limits in ENV variables is a good idea
  • if we are trying to prevent brute force attacks on login then current approach of route middleware is ok, I saw that you allow 3 maxAttempts before the pause, so that is ok from me

@github-actions github-actions bot added the type: test Improvements or additions to tests label Sep 25, 2024
docker/dev/.env.example Outdated Show resolved Hide resolved
@github-actions github-actions bot added the type: documentation Improvements or additions to documentation label Oct 3, 2024
dragosp1011
dragosp1011 previously approved these changes Oct 9, 2024
@adrianboros adrianboros merged commit 5084734 into main Nov 8, 2024
15 checks passed
@adrianboros adrianboros deleted the 1585-prevent-brute-force-attacks-support-rate-limiting-login-attempts branch November 8, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: wallet/backend Wallet backend implementations type: documentation Improvements or additions to documentation type: source Source changes type: test Improvements or additions to tests
Projects
None yet
4 participants