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

Implements a rate limit for password resets #13934

Merged
merged 4 commits into from Jul 17, 2019
Merged

Implements a rate limit for password resets #13934

merged 4 commits into from Jul 17, 2019

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jan 6, 2019

currently only prevents from requesting new password resets within one hour. Maybe we could even increase the time limit.

fixes #13813

@sgiehl sgiehl added the Needs Review PRs that need a code review label Jan 6, 2019
@fdellwing
Copy link
Contributor

I would maybe change the limit to 2 per hour? There are valid reasons to try it a second time.

@sgiehl
Copy link
Member Author

sgiehl commented Jan 7, 2019

Are there? If sending the email fails on Matomo side the request get's removed again. So a new request can be done until the email was sent.

@fdellwing
Copy link
Contributor

fdellwing commented Jan 7, 2019

Mail might get not deliviered on recieving end for some spam or missconfiguring reasons (e.g. greylisting, missing forwarding, wrong trusted networks, etc).

@sgiehl
Copy link
Member Author

sgiehl commented Jan 7, 2019

Then it would make sense to allow resending the last request once instead of allowing a complete new one...

@fdellwing
Copy link
Contributor

I currently don't see a difference, but that seems fine for me.

@sgiehl
Copy link
Member Author

sgiehl commented Jan 7, 2019

Requesting a new reset means filling out the form again and generating a new token for the reset. That means the email content is slightly another as the link differs

@fdellwing
Copy link
Contributor

Ok, it's fine for me to send the exact same mail again.

plugins/Login/lang/en.json Outdated Show resolved Hide resolved
@tsteur
Copy link
Member

tsteur commented Jan 7, 2019

I would have also allowed 2 or 3 per hour as it can be useful eg if mailbox is full etc but if it otherwise works within an hour again should be fine. Could have been also useful to block per IP but not needed (to avoid someone requesting forget password feature every hour for all users and basically blocking the feature, not really an issue though)

@sgiehl
Copy link
Member Author

sgiehl commented Jan 13, 2019

@tsteur Wondering what would be the userfriendliest and securest implementation.

  • Allowing to request a password reset only once an hour (e.g. an error will be shown when the last request was within the last 60 mins)
  • Limit the number of requests possible per day (or maybe month), without any "wait time" between the requests
  • Allowing to request a password reset only every 3-x hours, but show a message when another request is made within that time, that allows to trigger a resend of the last request (new token, but still the same password from the original request). Resending the request would also be limited to 2 or 3 times and afterwards an error would be shown to contact the admin

@tsteur
Copy link
Member

tsteur commented Jan 13, 2019

Ideally it would use the new brute force feature added in 3.8.0. We would add a new type column to the DB and methods to be able to take advantage of all the methods etc.

But it's a bit more work and #13813 is not even scheduled yet and not really important so would be too much work for now. I would probably simply allow 3 calls per hour for now or whatever is easiest.

@sgiehl
Copy link
Member Author

sgiehl commented Jan 14, 2019

@tsteur guess make sense to reuse that. This was just a quick approach to implement a simple rate limit. Maybe we should simply close this and implement it properly later?

@tsteur
Copy link
Member

tsteur commented Jan 14, 2019

No preference. Could also merge and just leave it like that. up to @Findus23 and @mattab

@Findus23
Copy link
Member

This PR fixes the most important issue (you can endlessly spam users), so I would say this is good for now. And I wouldn't create a too complex logic as I still think that one day the password reset should be replaced with the "normal" way (#11071) and then there wouldn't be a difference between resending a reset and sending a new reset.

@mattab
Copy link
Member

mattab commented Jan 14, 2019

just a quick feedback: reckon it should be possible to request again the password reset in the hour. For example if the mailserver was buggy and email didn't get sent, or any other problem. 3 times per hour sounds good and would not be considered spammy?

@tsteur tsteur added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Apr 11, 2019
@tsteur
Copy link
Member

tsteur commented Apr 11, 2019

@sgiehl I set it back to WIP. Lets allow 3 times per hour.

@mattab mattab added this to the 3.11.0 milestone May 21, 2019
@sgiehl sgiehl force-pushed the pwdresetratelimit branch 2 times, most recently from 9f2f6ee to 0bf3839 Compare June 23, 2019 21:26
@sgiehl
Copy link
Member Author

sgiehl commented Jun 23, 2019

Changed it so only 3 requests are allowed per hour.

@sgiehl sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Jun 23, 2019
@tsteur
Copy link
Member

tsteur commented Jun 24, 2019

Any chance to create an integration test for this @sgiehl ?

@sgiehl
Copy link
Member Author

sgiehl commented Jun 24, 2019

I've added some tests

@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jul 15, 2019
@tsteur tsteur merged commit ffd0987 into 3.x-dev Jul 17, 2019
@tsteur tsteur deleted the pwdresetratelimit branch July 17, 2019 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rate limit password resets
5 participants