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

Add lost password limits #974

Merged
merged 10 commits into from Oct 19, 2018

Conversation

Projects
None yet
2 participants
@brettshumaker
Copy link
Contributor

brettshumaker commented Sep 27, 2018

This adds rate limits to lost password requests using similar logic to our login limiter. It generalizes a few of the existing functions used in limiting login attempts so the logic could be applied to multiple contexts. We're limiting lost password requests to 3 attempts. I've also added two tests related to the new functionality.

Steps to Test

  1. Check out PR.
  2. Go to /wp-login.php?action=lostpassword
  3. Fill out and submit the form.
  4. Verify that an error message is returned after the third attempt.
  5. Verify that no password reset email is received after the third attempt.

@mjangda mjangda changed the title Update/add lost password limits Add lost password limits Oct 18, 2018

mjangda and others added some commits Sep 11, 2018

Add rate limits to lost password requests
Extends the login limit functionality and applies it to lostpassword
requests
Updates to wpcom_vip_login_limit_lost_password()
- Moved contents of the `wpcom_vip_login_limiter()` to `wpcom_vip_limiter()`
- Added `wpcom_vip_limiter()` as a generic utility to track/cache user and IP|user to a specific cache group
- Rename `wpcom_vip_login_limit_lost_password()` to `wpcom_vip_lost_password_limit()`
- Tweak `wpcom_vip_lost_password_limit()` to add the error if the threshold has been reached.
- Changed `wpcom_vip_login_is_limited()` to accept a cache group to see if the username is limited in that context, and return an error based on the cache group if necessary.
Rename `wpcom_vip_login_is_limited()`
Renamed `wpcom_vip_login_is_limited()`  to `wpcom_vip_username_is_limited()` to make this more generic.
Added `password_reset_limit_exceeded` hook.
Added a `password_reset_limit_exceeded` hook to match the `login_limit_exceeded` hook.
Updated the thresholds and timeouts
Discussed setting the threshold to 3 attempts, and setting the timeout to 30 minutes. I've kept the restricted username threshold at 2 and timeout at 1 hour. For an unrestricted username, the threshold is now at 3 and the IP|username lockout is 30 minutes, while the IP lockout is still at 1 hour.
Adding tests for lost password limiter
- Added a test to make sure we're not altering existing errors during the `lostpassword_post` hook.
- Added a test to make sure our `WP_Error` is returned once we've hit our lost password threshold and not before.
Make sure $threshold2 is always defined.
I had introduced a scenario where `$threshold2` could be undefined. I've fixed that scenario and now all tests are passing.

@mjangda mjangda force-pushed the update/add-lost-password-limits branch from f5ead91 to f8a3676 Oct 19, 2018

@mjangda
Copy link
Member

mjangda left a comment

I pushed some minor tweaks to rename a function and move some strings to constants.

This looks great and tests well; nice work @brettshumaker!

@mjangda mjangda merged commit 420da80 into master Oct 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mjangda mjangda deleted the update/add-lost-password-limits branch Oct 19, 2018

@mjangda

This comment has been minimized.

Copy link
Member

mjangda commented Oct 19, 2018

r120355-deploy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment