Problem/Motivation

Currently, User X can request a new password an infinite amount of times. This can be confirmed by the logs at admin/reports/dblog.
Update 8/15/13: The issue is still in the Drupal8 download as I have tested.

Proposed resolution

Create flood event and enforce it.
Create/modify tests in order so that this patch can pass. Since the flood protection takes action after a while the user will not be able to login after too many tries, and this the way it should work. The tests should let this pass.

Remaining tasks

Addressing concerns of @David_Rothstein in #73

I was going to say that looked like a good change, but then it occurred to me: Because the flood control is by IP address, we actually have no idea how many times the current user tried to reset their password recently (it could be zero).

Which actually means this: If many users at an organization all share the same IP address, then a malicious user at the same IP address (let's say a disgruntled employee situation) could trigger the flood control deliberately by continually requesting multiple passwords for their own account. Since there were no password reset requests for other accounts, those users will not have any login links in their e-mail to click on, nor will they be able to generate them. So they will be completely locked out of the site (assuming the attacker flooded the login form too), and the only way to get back in is to wait for someone with technical knowledge to go onto the server and fix things.

I have to ask, isn't the above scenario actually more dangerous than the simple "spam" scenario the patch is trying to protect against?

See one opinion on this tradeoff in #127 - tl;dr the benefit outweighs the risk (which is a bit of an edge-case).

User interface changes

TBD

API changes

TBD

Original report by Traverus

Currently, User X can request a new password an infinite amount of times. There should be a flood event created and enforced. I'll include the code that I used to implement my own solution to help get toward a patch for the problem.

As a for instance Bob doesn't like Alice, so he resets Alice's password 1230875109 times. This makes Alice upset because she has an equal amount of e-mails in her inbox.

Thanks!

function xyz_user_pass_form_validate($form, $form_state){
  if(!flood_is_allowed('request new password', 1, 86400, $form_state['values']['name'])){
    form_set_error('name', 'Reset password limit exceeded.  Please contact technical support for further assistance.');
	flood_register_event('request new password', 86400, $form_state['values']['name']);
  } else {
    flood_register_event('request new password', 86400, $form_state['values']['name']);
  }
}

hook_form_user_pass_alter($form){
  array_unshift($form['#validate'], 'xyz_user_pass_form_validate');
}

CommentFileSizeAuthor
#148 1681832-148-interdiff.txt1005 byteskim.pepper
#148 1681832-148.patch18.85 KBkim.pepper
#145 1681832-145-interdiff.txt5.25 KBkim.pepper
#145 1681832-145.patch18.85 KBkim.pepper
#142 interdiff-1681832-137-142.txt2.57 KBmcdruid
#142 1681832-142.patch15.86 KBmcdruid
#137 interdiff-1681832-136-137.txt1.78 KBmcdruid
#137 1681832-137.patch14.97 KBmcdruid
#136 interdiff-1681832-125-136.txt3.64 KBmcdruid
#136 1681832-136.patch13.62 KBmcdruid
#134 interdiff-1681832-125-134.txt3.62 KBmcdruid
#134 1681832-134.patch13.61 KBmcdruid
#125 1681832.patch11.72 KBdrumm
#123 1681832.patch11.72 KBdrumm
#117 1681832-117.patch11.74 KBpdenooijer
#117 interdiff-1681832-116-117.txt2.34 KBpdenooijer
#116 interdiff-1681832-114-116.txt4.39 KBmcdruid
#116 1681832-116.patch11.94 KBmcdruid
#114 interdiff-1681832-113-114.txt5.41 KBmcdruid
#114 1681832-114.patch10.27 KBmcdruid
#113 interdiff-1681832-111-113.txt6.59 KBmcdruid
#113 1681832-113.patch8.58 KBmcdruid
#111 interdiff-1681832-109-111.txt2.41 KBmcdruid
#111 1681832-111.patch8.64 KBmcdruid
#109 interdiff-1681832-107-109.txt2.1 KBmcdruid
#109 1681832-109.patch8.66 KBmcdruid
#107 interdiff-1681832-98-107.txt2.11 KBmcdruid
#107 1681832-107.patch8.64 KBmcdruid
#105 interdiff-1681832-98-105.txt1.51 KBmcdruid
#105 interdiff-1681832-103-105.txt1.44 KBmcdruid
#105 1681832-105.patch8.63 KBmcdruid
#103 interdiff-1681832-101-103.txt1.11 KBmcdruid
#103 1681832-103.patch9.05 KBmcdruid
#101 interdiff-1681832-98-101.txt614 bytesmcdruid
#101 1681832-101.patch8.91 KBmcdruid
#98 1681832-98-D8.patch8.93 KBmohit1604
#96 1681832-96-D8.patch8.95 KBmohit1604
#77 interdiff.txt2.8 KBk_zoltan
#77 1681832_77.patch9.33 KBk_zoltan
#70 interdiff.txt1.73 KBk_zoltan
#70 1681832_70.patch10.04 KBk_zoltan
#66 interdiff.txt2.82 KBk_zoltan
#66 1681832_66.patch9.86 KBk_zoltan
#58 drupal8.user-module.1681832-57-complete.patch9.85 KBZenDoodles
#58 drupal8.user-module.1681832-57-tests.patch5.6 KBZenDoodles
#55 drupal8.user-module.1681832-55.patch9.84 KBeshta
#48 drupal8.user-module.1681832-48.patch3.92 KBCyberschorsch
#45 drupal8.user-module.1681832-45.patch3.91 KBZenDoodles
#43 drupal8.user-module.1681832-43.patch4.03 KBdougvann
#41 drupal8.user-module.1681832-41.patch3.9 KBdougvann
#39 drupal8.user-module.1681832-39.patch3.9 KBdougvann
#36 drupal8.user-module.1681832-36.patch3.9 KBdougvann
#32 user_Passwort_reset_flood_1681832_32.patch4.02 KBk_zoltan
#26 user_Passwort_reset_flood_1681832_26.patch3.58 KBphiit
#24 user_Passwort_reset_flood_1681832_24.patch2.11 KBk_zoltan
#18 user_Passwort_reset_flood_1681832_18.patch2.11 KBCyberschorsch
#17 user_Passwort_reset_flood_1681832_17.patch2.12 KBCyberschorsch
#7 password_reset_flood_1681832_3.patch1.78 KBk_zoltan
#4 password_reset_flood_1681832_2.patch1.85 KBmihai_brb
#1 password_reset_flood_1681832_1.patch1.27 KBk_zoltan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

k_zoltan’s picture

Category: bug » task
Status: Active » Needs work
FileSize
1.27 KB

Attached first patch

Unable to test it now please have a look to.

Also very strange that this isn't done before

k_zoltan’s picture

Status: Needs work » Needs review
coltrane’s picture

Status: Needs review » Needs work

Nice find, I don't think there's a legitimate reason for not having flood control on this.

I assume this issue also exists in D8. We should confirm and fix there as well as D7.

As for the patch, it needs testing and I have this specific feedback:

+++ b/modules/user/user.pages.inc
@@ -55,6 +55,16 @@ function user_pass() {
+  $pass_reset_limit = variable_get('user_failed_pass_reset_limit', 50);

How about a limit of 20?

+++ b/modules/user/user.pages.inc
@@ -55,6 +55,16 @@ function user_pass() {
+    flood_register_event('request new password', $pass_reset_window, $name);

Should register the event in the submit handler in case a hook alter implementation on this form sets additional validation checks. Only want to register when the email goes out.

Additionally there's no call to flood_clear_event(), but perhaps there shouldn't be because the goal of this is issue is to protect against abuse of the password reset email functionality.

mihai_brb’s picture

Status: Needs work » Needs review
FileSize
1.85 KB

The patch was changed in order to register the event in the submit handler user_pass_submit function.
Also the password reset limit was lowered to 20 times.

TODO: Check if the problem exists in Drupal8.

mihai_brb’s picture

Status: Needs review » Active
Damien Tournoud’s picture

Status: Active » Needs work
+  flood_register_event('request new password', $pass_reset_window, $name);

This limits the password reset per user. This is not desirable. We should limit by IP address instead, by removing the $name parameter (here and in the flood_is_allowed() call).

k_zoltan’s picture

Status: Needs work » Active
FileSize
1.78 KB

Changed limits from per user to per IP, removed the $name variable since not needed any more.

k_zoltan’s picture

Status: Active » Needs review
Rob C’s picture

Status: Needs review » Needs work

The last submitted patch, password_reset_flood_1681832_3.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
YesCT’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

tagging
oh, and also moving to 8.x. We can backport it to 7.x. Means we'll need a new 8.x patch.

grwgreg’s picture

Issue summary: View changes

Updating the issue summary

grwgreg’s picture

Issue summary: View changes

Updating issue

grwgreg’s picture

I was able to reproduce this in drupal 8 and updated the issue summary. The flood functions from drupal 7 have been changed in drupal 8 to flood controllers so the code above needs to be updated for drupal 8.

grwgreg’s picture

Issue summary: View changes

Issue updated by grwgreg

grwgreg’s picture

Issue summary: View changes

.

grwgreg’s picture

Steps to reproduce:

  1. Install the latest Drupal 8.x release.
  2. Navigate to admin/people with the admin account.
  3. Create a new active user with an email that would be okay with receiving 20+ emails.
  4. Sign out of the admin account.
  5. Navigate to /user/password and fill out the form requesting a new password for the account created in step 3.
  6. Repeat step 5 20+ times.
  7. Sign back in and check admin/reports/dblog for multiple "Password reset instructions mailed to..." messages.
k_zoltan’s picture

I would really help in providing the Drupal 8 patch but after seeing how much things have been changed I'm a bit scared.

k_zoltan’s picture

Also maybe it would be a great thing to get the patch commited in Drupal7 because of the big difference between the d7 and the d8 versions on how this problem needs to be treated.

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

I added a flood check for Drupal 8. I am using the same limits as the user login for now which means 50 requests from the same ip is the current limit.

Cyberschorsch’s picture

Removed a whitespace ...

aiypz’s picture

HI,

This is my first attempt to help out here and my first use of a patch so I've described how I replicated this below (Just to make sure I've got it right).

I tested the patch and can confirm that on the 50+ attempts I get the following message: "Reset password limit exceeded. Please contact technical support for further assistance."
I can also see each reset request in the log file.

I hope this helps. Below is how I replicated it.

Thanks
Stuart

Steps to reproduce:

Install the latest Drupal 8.x release.
Applied patch above by navigating to root of drupal directory and used command: patch -p1 < user_Passwort_reset_flood_1681832_18.patch
Navigate to admin/people with the admin account.
Create a new active user with an email that would be okay with receiving 20+ emails.
Sign out of the admin account.
Navigate to /user/password and fill out the form requesting a new password for the account created in step 3.
Repeat step 5 50+ times.
Sign back in and check admin/reports/dblog for multiple "Password reset instructions mailed to..." messages.

k_zoltan’s picture

Status: Needs review » Reviewed & tested by the community

I think you did everything right.
The only remark is that after doing what you did
the tickets status should be changed to "reviewed and tested by the community".

But lets see what others with more experience say.

YesCT’s picture

alippai’s picture

Are you sure this is the right order for register() and isAllowed()? If we register not only the requests under the threshold the database is exposed to DDoS attack (unlimited writing of DB without delay or threshold).

k_zoltan’s picture

Status: Reviewed & tested by the community » Needs work

I looked at the current code in the user module, to see how it's done the same thing for the login

in the user.module file line 1339 it checks: if (!drupal_container()->get('flood')->isAllowed('user.failed_login_ip'
the register comes only after in the user_login_final_validate() function.

So It looks like it should be changed.

k_zoltan’s picture

Status: Needs work » Needs review
FileSize
2.11 KB

Here is the same patch as for the #18 just modified the order of the 2 functions

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

phiit’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.58 KB

First reroll I've done, hopefully everything is in order!

star-szr’s picture

Status: Needs review » Needs work

Discussed this with @phiit in IRC, needs another go at the reroll, perhaps applying the patch manually.

k_zoltan’s picture

Please write down what you discussed, whats the conclusion, what should be done further.

star-szr’s picture

We just discussed that these functions have moved around significantly so doing a reroll based on the instructions at http://drupal.org/patch/reroll would likely not be successful. I suggested since it's a small patch to apply the changes to the current codebase manually.

k_zoltan’s picture

I just talked with Gabor Hojtsy at the drupalaton camp and now I understand what you say. I will get the latest drupal 8 from git and try to find the function.

k_zoltan’s picture

Issue summary: View changes

Updated issue summary, moved the detail about what to do next from the proposed resolution to remaining tasks. took some words from the original report and used them for the proposed resolution.

k_zoltan’s picture

The forget password form and its validation has been moved into the UserPasswordForm class that is located at the following path:
core/modules/user/lib/Drupal/user/UserPasswordForm.php

This is why a reroll didn't falled but the patch is still useless.

k_zoltan’s picture

Here is a patch done with the help of @swentel and @aspilicious
This patch is only a flood protection by IP.
I will do the filtering for the username too.

k_zoltan’s picture

Status: Needs work » Needs review

I made a more deeper research and I think the patch is ok as is.

Status: Needs review » Needs work

The last submitted patch, user_Passwort_reset_flood_1681832_32.patch, failed testing.

star-szr’s picture

Issue tags: +Needs reroll

Thanks for working on this @k_zoltan!

Tagging for reroll and there is a bit of trailing whitespace in the patch that should be removed per http://drupal.org/coding-standards#indenting.

dougvann’s picture

Rerolled patch [only my 2nd time submitting a rerolled patch] ;-)

Also curious why we're not using trailing commas. It seems to me that line 77 should end in a comma in preparation for any additional lines that may be added in the future. Yes?

dougvann’s picture

Status: Needs work » Needs review

updating status...

Status: Needs review » Needs work

The last submitted patch, drupal8.user-module.1681832-36.patch, failed testing.

dougvann’s picture

Status: Needs work » Needs review
FileSize
3.9 KB

Corrected PHP Syntax error

Status: Needs review » Needs work

The last submitted patch, drupal8.user-module.1681832-39.patch, failed testing.

dougvann’s picture

Status: Needs work » Needs review
FileSize
3.9 KB

trying again

Status: Needs review » Needs work

The last submitted patch, drupal8.user-module.1681832-41.patch, failed testing.

dougvann’s picture

Status: Needs work » Needs review
FileSize
4.03 KB

4th time's a charm?

Status: Needs review » Needs work

The last submitted patch, drupal8.user-module.1681832-43.patch, failed testing.

ZenDoodles’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.91 KB

One more try...

Status: Needs review » Needs work

The last submitted patch, drupal8.user-module.1681832-45.patch, failed testing.

k_zoltan’s picture

I think we are doing something wrong.
I wondering do the tests work correctly?

k_zoltan’s picture

Issue summary: View changes

hope to clarify the issue summary by directing attention to the path to solution in #29

Cyberschorsch’s picture

Issue summary: View changes
FileSize
3.92 KB

Rerolled the patch

Cyberschorsch’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: drupal8.user-module.1681832-48.patch, failed testing.

k_zoltan’s picture

Great job @Cyberschorsch

BUT

These patches will always fail on testing, since the tests are there to prevent that any functionality is broken after the patching.

But this time it should break a feature. The feature of requesting a new password eternally aka. spamming the user with the new password request.

The test needs to be changed in order to get this done.

k_zoltan’s picture

eshta’s picture

Assigned: Unassigned » eshta

I'll work on tests for this one today.

eshta’s picture

So I've gotten it to a point where the tests all pass. There were missing imports. Seems like we should be adding an additional test, however, of this new functionality. I'm working on adding that before I push up the new patch.

eshta’s picture

Status: Needs work » Needs review
FileSize
9.84 KB

Adding a new patch for the following:

  • Fixed previous patch automated tests by adding imports for the FloodInterface and ConfigFactory
  • Updated a few deprecated function calls within the validation function.
  • Added a test case for flood triggering.
ZenDoodles’s picture

Status: Needs review » Needs work

The last submitted patch, 55: drupal8.user-module.1681832-55.patch, failed testing.

ZenDoodles’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to 7.x, +#SprintWeekend2014
FileSize
5.6 KB
9.85 KB

Rerolled and created a tests-only patch.

ZenDoodles’s picture

The last submitted patch, 58: drupal8.user-module.1681832-57-tests.patch, failed testing.

hrbel’s picture

Assigned: eshta » hrbel

I will review the change during sprint weekend

hrbel’s picture

Assigned: hrbel » Unassigned
eshta’s picture

Thanks ZenDoodles for catching this up! I still have a little time if there is more to do :-)

k_zoltan’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary as best as I understand the situation.

amateescu’s picture

Status: Needs review » Needs work

This patch looks mostly ok, here's a small nitpicky review :)

  1. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordForm.php
    @@ -108,19 +132,26 @@ public function buildForm(array $form, array &$form_state) {
    +      $this->setFormError('name', $form_state, $this->t('Reset password limit exceeded.  Please contact technical support for further assistance.'));
    
    +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,61 @@ public function testUserResetPasswordTextboxFilled() {
    +      $this->assertText(t('Reset password limit exceeded.  Please contact technical support for further assistance.'), 'Flood error message shown.');
    

    Double space before "Please".

  2. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,61 @@ public function testUserResetPasswordTextboxFilled() {
    +   * Test password reset flood control.
    ...
    +   * Make a password request.
    

    Very minor comment here, we must start with a third person singular present tense verb. In this case: "Tests ..." and "Makes ...".

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,61 @@ public function testUserResetPasswordTextboxFilled() {
    +   * @param $name
    ...
    +   * @param $valid_trigger
    ...
    +   * @param $flood_trigger
    ...
    +  public function assertPasswordReset($name, $valid_trigger = TRUE, $flood_trigger = NULL) {
    

    We're missing type hints for these parameters: "string", "bool" and "bool".

    Also $flood_trigger should default to FALSE (instead of NULL) in the function signature since it is actually a boolean.

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,61 @@ public function testUserResetPasswordTextboxFilled() {
    +    if (isset($flood_trigger)) {
    

    We're converting $flood_trigger to a boolean so we should remove the isset() usage here.

  5. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,61 @@ public function testUserResetPasswordTextboxFilled() {
    +  }
     }
    

    We generally put an empty line after the last method of a class.

k_zoltan’s picture

Status: Needs work » Needs review
FileSize
9.86 KB
2.82 KB

Fixes for the review above.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good to me now.

David_Rothstein’s picture

+      $this->setFormError('name', $form_state, $this->t('Reset password limit exceeded. Please contact technical support for further assistance.'));

I won't un-RTBC over a text change, but this definitely looks wrong. Who is "technical support"??? And we shouldn't say "please" in user interface text.

I think this needs to be a more helpful error message that explains to the user how they can actually log in (that's what the flood control message on the user login form does)... In this case, I guess it needs to tell people to check their e-mail and use one of the existing password reset links they find there?

k_zoltan’s picture

How about the following message "Sorry, there were to many failed password recovery attempts for this account. Check your e-mail and use one of the password reset instructions from there." I will do patch and interdiff later.

k_zoltan’s picture

FileSize
10.04 KB
1.73 KB

Updated patch as promised. Actually the same patch as #66 just changed the error message.

StuartJNCC’s picture

"to many" should be "too many".

This has become rather verbose. How about something like: "You have tried to reset your password too many times. Check your e-mail for instructions on how to proceed."

YesCT’s picture

Issue tags: +Novice

Novice for someone to do that change.
https://drupal.org/contributor-tasks/create-patch (that also has links to docs with one of the steps linking to how to do an interdiff)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

I was going to say that looked like a good change, but then it occurred to me: Because the flood control is by IP address, we actually have no idea how many times the current user tried to reset their password recently (it could be zero).

Which actually means this: If many users at an organization all share the same IP address, then a malicious user at the same IP address (let's say a disgruntled employee situation) could trigger the flood control deliberately by continually requesting multiple passwords for their own account. Since there were no password reset requests for other accounts, those users will not have any login links in their e-mail to click on, nor will they be able to generate them. So they will be completely locked out of the site (assuming the attacker flooded the login form too), and the only way to get back in is to wait for someone with technical knowledge to go onto the server and fix things.

I have to ask, isn't the above scenario actually more dangerous than the simple "spam" scenario the patch is trying to protect against?

sun’s picture

  1. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordForm.php
    @@ -108,19 +132,26 @@ public function buildForm(array $form, array &$form_state) {
       public function validateForm(array &$form, array &$form_state) {
    ...
    +    $flood_config = $this->configFactory->get('user.flood');
    +    if ($this->flood->isAllowed('user.password_request', $flood_config->get('ip_limit'), $flood_config->get('ip_window'))) {
    ...
         else {
    -      $this->setFormError('name', $form_state, $this->t('Sorry, %name is not recognized as a username or an e-mail address.', array('%name' => $name)));
    +      $this->setFormError('name', $form_state, $this->t('Sorry, there were to many failed password recovery attempts for this account. Check your e-mail and use one of the password reset instructions from there.'));
         }
    

    Since the flood protection is an all-or-nothing condition, I'd prefer to use an early-return in this method, instead of indenting the whole function body.

  2. +++ b/core/modules/user/lib/Drupal/user/Form/UserPasswordForm.php
    @@ -108,19 +132,26 @@ public function buildForm(array $form, array &$form_state) {
    +      $this->flood->register('user.password_request', $flood_config->get('ip_window'));
    

    Since the default IP limit is relatively high (50), I wonder whether we should additionally use a per-user limit (which defaults to 5)?

    So that a single IP is limited to 50 attempts, but in addition, a single given name or email (if found/valid) is limited to 5?

    Or is that overkill?

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,62 @@ public function testUserResetPasswordTextboxFilled() {
    +  public function testUserResetPasswordFloodControl() {
    

    It looks like this could be a DrupalUnitTestBase, which programmatically executes the form instead.

    In a DUTB test, you can mock a Request and pass a custom IP, which in turn allows you to additionally test that the form still works for a different IP.

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php
    @@ -143,4 +129,62 @@ public function testUserResetPasswordTextboxFilled() {
    +   * @param string $name
    +   *   The user name or email address to use for the request.
    +   * @param bool $valid_trigger
    +   *   Whether of not to expect the $name is valid.
    +   * @param bool $flood_trigger
    +   *   Whether or not to expect that the flood control mechanism will be
    +   *   triggered.
    +   */
    +  public function assertPasswordReset($name, $valid_trigger = TRUE, $flood_trigger = FALSE) {
    +    $this->drupalGet('user/password');
    

    An assertion method should not perform more operations than the assertion(s) itself.

    The drupalGet() and drupalPostForm() should not be part of the assertion method.

    In addition, a single assertion method should not have a Boolean switch to negate the expectation.

    Instead of such arguments, use separate assertion methods instead. The negated ones are typically use "No" in their method names; i.e.:

    assertValidPasswordReset()
    assertNoValidPasswordReset()
    assertPasswordFlood()
    assertNoPasswordFlood()

k_zoltan’s picture

Issue summary: View changes

I will start working on point 1, 3 and 4.
Point 2 should be discussed.

Updating Issue summary.

k_zoltan’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Novice

A few more simple issue summary updates

k_zoltan’s picture

FileSize
9.33 KB
2.8 KB

Done point 1 of the comment #74.

The interdiff looks so strange because the of the early return the indentation changed.

Just marked as need review for testing. Actually it needs work.

k_zoltan’s picture

Status: Needs work » Needs review

Just marked as need review for testing. Actually it needs work.

k_zoltan’s picture

Status: Needs review » Needs work
alimac’s picture

Issue tags: -, -#SprintWeekend2014 +SprintWeekend2014

Minor tag cleanup - please ignore.

k_zoltan’s picture

Issue tags: +SprintWeekend2015

Well then lets add this tag too. :)

StevenPatz queued 77: 1681832_77.patch for re-testing.

The last submitted patch, 77: 1681832_77.patch, failed testing.

deepakaryan1988’s picture

Removing sprint weekend tag!!
As suggested by @YesCT

star-szr’s picture

Issue tags: -password

:)

YesCT’s picture

@deepakaryan1988 The tag was added January 17, 2015 during the actual global sprint weekend. See https://groups.drupal.org/node/447258
Please leave the tag on issues that were actually worked on during that event.

I think you might have confused a message from May about the sprint weekend tag, that it should not be added to events from May. The tag is for issues worked on during https://groups.drupal.org/node/447258 by people attending sprint weekend sprints.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

Title: Password reset spammable (and shouldn't be) » Password reset form has no flood protection
Version: 8.2.x-dev » 8.3.x-dev
Category: Task » Bug report

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Rob C’s picture

Adding needs reroll tag to attract some attention + adding some related issues.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mohit1604’s picture

Assigned: Unassigned » mohit1604
mohit1604’s picture

Rerolling patch #77 :)

mohit1604’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.95 KB

Status: Needs review » Needs work

The last submitted patch, 96: 1681832-96-D8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mohit1604’s picture

Assigned: Unassigned » mohit1604
Status: Needs work » Needs review
FileSize
8.93 KB

Status: Needs review » Needs work

The last submitted patch, 98: 1681832-98-D8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

alexharries’s picture

I appreciate this is an old task and my post is very much too late to change anything, but perhaps the following approach might be better at preventing people from being locked out of their accounts:

- Each password reset is valid for 48 hours per account;
- We don't send password reset e-mails more-frequently than once every two hours;
- The token used in password reset e-mails in the same 48 hour period is the same.

In a bit more detail...

- When password reset is requested for a username, a token is created and saved in the database which is valid for 48 hours, and an e-mail with a link containing the token is sent to the user's e-mail address (so far, pretty straight-forward). We note the time that the e-mail was sent,

- If another password reset is requested for that username fewer than two hours since the last e-mail was sent, we don't send an e-mail. Instead, we show a message along the lines of, "We have sent you a password reset e-mail within the last two hours. Please check your junk/spam folder in your e-mail client. If you still haven't received your e-mail within another 2 hours/1 hour/30 minutes [don't specify the exact time here; there's no need for that level of detail], please fill out this password reset form again." - this step should not need write to the DB at all (to reduce overhead from write operations) - it can be entirely read-only.

- Cron can be used to clean up the expired tokens.

This approach will greatly reduce the chances of malicious password resets flooding the log tables or sending out many hundreds of e-mails, without allowing a user to become locked out of their account.

Just my $0.0002-worth though, and as mentioned before I appreciate this is way too late to change things now.

Alex

mcdruid’s picture

Assigned: mohit1604 » Unassigned
Status: Needs work » Needs review
Issue tags: +Security improvements
FileSize
8.91 KB
614 bytes

Updated patch with the codesniff fix (switch to short array syntax on one line).

Not sure if there are still other test failures which need to be addressed; we should see in due course.

Status: Needs review » Needs work

The last submitted patch, 101: 1681832-101.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Status: Needs work » Needs review
FileSize
9.05 KB
1.11 KB

Looks like the constructor of UserPasswordForm has changed and I think that was causing multiple test failures.

I've also changed another couple of codesniffer fixes (redundant Use's).

Status: Needs review » Needs work

The last submitted patch, 103: 1681832-103.patch, failed testing. View results

mcdruid’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
1.44 KB
1.51 KB

Couple of other things needed a tweak; I'll let the interdiff speak for itself.

Also including an interdiff showing changes from patch #98 which is where I started.

Tests pass locally now.

Status: Needs review » Needs work

The last submitted patch, 105: 1681832-105.patch, failed testing. View results

mcdruid’s picture

Status: Needs work » Needs review
FileSize
8.64 KB
2.11 KB

The tests that I was actually running passed locally :)

Looks like another test which is affected by the patch is failing because it's calling a non-existent method. This hopefully fixes that, but looks like we'll then hit failures within this test along the lines of:

Drupal\Core\Config\ImmutableConfigException: Can not set values on immutable configuration user.flood:ip_limit. Use \Drupal\Core\Config\ConfigFactoryInterface::getEditable() to retrieve a mutable configuration object in Drupal\Core\Config\ImmutableConfig->set() (line 27 of /path/to/drupal-8.x/core/lib/Drupal/Core/Config/ImmutableConfig.php).

...and perhaps some others.

Hopefully this is a little closer than when I started on it today though (interdiff based on that).

Status: Needs review » Needs work

The last submitted patch, 107: 1681832-107.patch, failed testing. View results

mcdruid’s picture

Status: Needs work » Needs review
FileSize
8.66 KB
2.1 KB

Setting to NR to update the tests, but this will still fail.

I've corrected a spelling mistake, and hopefully the error due to trying to change an immutable config.

Still a few more failures to work through though.

Status: Needs review » Needs work

The last submitted patch, 109: 1681832-109.patch, failed testing. View results

mcdruid’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysLisbon
FileSize
8.64 KB
2.41 KB

A few small tweaks were required to get tests to pass locally (including one which meant the actual change to the form validation was broken).

All passing locally now; fingers crossed the testbot agrees...

(adding DevDaysLisbon tag as that's where I am working on this :)

mcdruid’s picture

Status: Needs review » Needs work

Great, so the patch is passing tests again.

However, looking back at #74 and #75 there may be some more work to do,

Having a look at points 2, 3 and 4 from #74.

mcdruid’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
6.59 KB

Implemented the suggested refactoring of the test(s) - point 4 from [#74].

(I also lowered the number of resets we're expecting to succeed from 5 to 3 to make the test little lighter - I don't see any reason to do more iterations).

I think point 2 (a per user limit) seems like a decent suggestion.

Point 3 ("It looks like this could be a DrupalUnitTestBase") seems like a more significant change - I'm not going to tackle that just yet.

Back to NR to check the new test code.

mcdruid’s picture

Implemented per-user flood control following the same logic as core/modules/user/src/Form/UserLoginForm.php

Changed the wording of the messages slightly to match UserLoginForm more closely too.

Updated the existing tests to use the per-user flood control initially. This seems to be working.

However, this means there's no testing of the IP-based flood control at present.

The (last remaining) point 3 from [#74] was to change the test to be a UnitTest instead, one of the benefits of doing so being that a mocked request could pass custom IPs which would allow us to test the IP-based flood control properly. So that's probably the next todo.

pdenooijer’s picture

Status: Needs review » Needs work

More security is always better, so good work so far! I found some improvements.

Why change LanguageManagerInterface to LanguageManager, seems like this violates the dependency inversion principle.

This code could be replaced with a more descriptive function:

if ($flood_config->get('uid_only')) {
  // Register flood events based on the uid only, so they apply for any
  // IP address. This is the most secure option.
  $identifier = $account->id();
}
else {
  // The default identifier is a combination of uid and IP address. This
  // is less secure but more resistant to denial-of-service attacks that
  // could lock out all users with public user names.
  $identifier = $account->id() . '-' . $this->getRequest()->getClientIP();
}

with:

$identifier = $this->getFloodIdentifier($flood_config, $account);

and

/**
 * Get the flood identifier based on the flood config.
 *
 * @param \Drupal\Core\Config\ImmutableConfig $flood_config
 *   Flood config.
 * @param \Drupal\Core\Session\AccountInterface $account
 *   Current user.
 *
 * @return string
 *   The flood identifier based on the flood config.
 */
protected function getFloodIdentifier($flood_config, $account) {
  if ($flood_config->get('uid_only')) {
    // Register flood events based on the uid only, so they apply for any
    // IP address. This is the most secure option.
    return $account->id();
  }
  // The default identifier is a combination of uid and IP address. This
  // is less secure but more resistant to denial-of-service attacks that
  // could lock out all users with public user names.
  return $account->id() . '-' . $this->getRequest()->getClientIP();
}

I guess my naming can be improved as well, but it should :).

mcdruid’s picture

Status: Needs work » Needs review
FileSize
11.94 KB
4.39 KB

Thanks for the suggestions pdenooijer!

IIRC I had to change LanguageManager to get the patch working again, but I can take another look at that.

I think the suggestion of moving the user identifier logic into a helper function is a good one, but the code's pretty much copypasta from \Drupal\user\Form\UserLoginForm::validateAuthentication so I can see some benefit in staying consistent with that. Perhaps one for a followup issue?

In the meantime, I've tweaked the tests so that we do now test both the user- and IP-based flood control. The latter is done a little crudely by submitting multiple reset requests with random user names so that the IP limit is triggered.

Back to NR for testbot, but I will look at LanguageManager again.

pdenooijer’s picture

Status: Needs review » Needs work
FileSize
2.34 KB
11.74 KB

No problem! The LanguageManagerInterface was used pre patch, so that's why I was wondering why you changed it.

I added the following changes to your patch:

  • Improved the DI, used Interface again.
  • Removed the configFactory field, as it is already defined in the extended class.
  • Moved the flood field, as this is defined last.
  • Use the EntityTypeManager instead of the deprecated EntityManager.

The helper function idea could indeed be picked up in a follow up. Then the UserLoginForm could also be improved in the same iteration.

pdenooijer’s picture

Status: Needs work » Needs review

No idea why it changed to needs work...

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

davidwhthomas’s picture

Need to mention, this issue affects programmatic API submissions to /user/password?_format=json
because there appears to be no flood control in Drupal\user\Controller\UserAuthenticationController\resetPassword

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drumm’s picture

Issue tags: +affects drupal.org

Attached is a re-roll of #117 for 8.8.x, it looks like at least one line of code was fixed by a separate issue upstream already. https://git.drupalcode.org/project/drupal/commit/408c540ccf2#08e2732ea90...

I’m not as-confident in the re-rolling of the tests, which moved files, but still mostly cleanly applied.

Re #120 - to limit the scope of this issue, I think throttling API requests might want to be a separate issue. For my use case, I’d probably just block the API endpoint, and just this issue would help.

drumm’s picture

Status: Needs review » Needs work

The last submitted patch, 123: 1681832.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

drumm’s picture

Status: Needs work » Needs review
FileSize
11.72 KB

This cleans up the coding standards message, and some deprecation warnings.

drumm’s picture

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

The IS still mentions addressing concerns raised by @David_Rothstein in #73:

I was going to say that looked like a good change, but then it occurred to me: Because the flood control is by IP address, we actually have no idea how many times the current user tried to reset their password recently (it could be zero).

Which actually means this: If many users at an organization all share the same IP address, then a malicious user at the same IP address (let's say a disgruntled employee situation) could trigger the flood control deliberately by continually requesting multiple passwords for their own account. Since there were no password reset requests for other accounts, those users will not have any login links in their e-mail to click on, nor will they be able to generate them. So they will be completely locked out of the site (assuming the attacker flooded the login form too), and the only way to get back in is to wait for someone with technical knowledge to go onto the server and fix things.

I have to ask, isn't the above scenario actually more dangerous than the simple "spam" scenario the patch is trying to protect against?

So a malicious user who can send reset requests from a shared IP address could indeed trigger the IP-based flood control and prevent other users requesting password resets from that same IP address (within the time window). However, (as David mentioned) the users wouldn't be locked out unless the bad actor also triggered IP-based flood control on the login form. At this point, intervention may be required from "someone with technical knowledge".

I'm not actually sure how many people without "technical knowledge" would know that requesting a password reset / one-time login link was a way to circumvent the flood control blocking the login form :)

Also, as time passes the flood events will be cleared, so the problem goes away unless the malicious user maintains the DoS.

I, for one, think that the benefit of implementing flood control for password resets (with a fairly high default limit of 50 for the IP-based restriction) outweighs the potential downside outlined here, especially as this sort of DoS requires the bad actor to be able to send reset requests from the IP they are trying to get blocked.

The other "Remaining task" was implementing user-based flood control in addition to the IP-based. We added that in #114.

Note also that the D7 backport over in #3074666: [D7] Password reset form has no flood protection is RTBC and has been deployed to drupal.org

I'll edit the IS accordingly, and am going to mark this as RTBC.

mcdruid’s picture

pdenooijer’s picture

Nice :)!

klausi’s picture

The problem with this patch is that it never clears the flood events.

Example:
* User requests new password 5 times because the email gateway used does not deliver fast enough.
* The user is now blocked by flood control from requesting more password reset links
* The old emails finally arrive at the user and they log in, thereby updating their login timestamp in the database. That immediately means that all old password reset URLs in the emails are invalid now.
* The user forgets to set a password and logs out.
* They want to request a new password again, but are still blocked from flood control.

Proposed solution:
* When the password reset link is used then flood_clear_event() should be called with the identifier for the given user. That way the password reset form immediately works again when a user logs in and we avoid this lock-out scenario.

Let me know if you agree, then we should set this back to "needs work" and also cover this with a test case. Related issue: #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password

catch’s picture

Status: Reviewed & tested by the community » Needs work

Agreed with #130. Back to NW for that.

mcdruid’s picture

Ok, interesting point. We actually discussed this briefly over in the D7 backport issue: #3074666-7: [D7] Password reset form has no flood protection.

I think we may need to change the way we're doing the user based-flood control in terms of the identifier we use. At present this is copied from the login form implementation and by default the identifier is made up of both the uid and the IP address. The problem with clearing flood events based on that identifier is that we can't guarantee that the reset request and the login will come from the same IP.

If we want to clear the user-based flood events (and I agree that it'd be a good change to make), the simplest solution is probably to use only the uid as the identifier (which is a configurable option in the existing code, but not the default). That would mean we could derive the identifier reliably when a user logs in with the reset link, and could therefore clear the user-based flood events.

I'm happy to rework the patch along those lines.

pdenooijer’s picture

Other idea would be to keep the same behaviour, but instead change the way it is saved in the database. The uid should be saved for every entry separately beside the identifier, that can be a combination of multiple things (like uid and ip). This way when a user logs in successfully the old entries can easily be cleared.

mcdruid’s picture

Status: Needs work » Needs review
FileSize
13.61 KB
3.62 KB

Hmm, whilst I think adding the uid as an extra field in what's recorded by flood is a pretty decent idea, it's more obvious (to me) how that would work for user-based events as opposed to the flood events where we only record an IP. I'm thinking, for example of how the events are recorded in \Drupal\Core\Flood\MemoryBackend. I'm sure it's possible but perhaps a bid fiddly. It'd also need a change to the schema used by \Drupal\Core\Flood\DatabaseBackend. I don't know if there are any alternative implementations using e.g. redis or memcache, but if so those would need to be adapted for a change to the interface too.

I'm not actually certain it's worth jumping through those hoops as I don't think it'd be bad to use uid only for the user-based flood events.

What are we really trying to achieve here? Flood control on login prevents (or at least impedes) brute force attempts. The flood control on password resets is arguably more aimed at preventing bad actors being a nuisance by filling users' mail inboxes with hundreds of password reset e-mails. A determined bad actor could circumvent the flood control where the identifier is based on uid + IP by sending reset requests from lots of different IPs (e.g. using tor).

Changing (the default) to a uid-only identifier would better protect users from being flooded with reset e-mails. If we regard it as a downside that doing so makes it easier for a bad actor to lock a user out of being able to request password resets... realistically how much of a problem is that? Unless the bad actor has a way of forcing the user to actually be locked out of the site unless they can request a reset (along the lines of David Rothstein's original concern), I'd argue that the protection adding uid-only user-based flood control outweighs any potential downsides.

Here's a patch which implements uid-only identifiers and clearing of flood events when a password reset is successful. If existing tests pass, we can add more tests to verify that the clearing of the flood events is successful.

Status: Needs review » Needs work

The last submitted patch, 134: 1681832-134.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

Status: Needs work » Needs review
FileSize
13.62 KB
3.64 KB

Oops, that's what happens when you try to rush a patch out before you have to jump off a train :)

Let's try that again; interdiff is still against #125 as that's more meaningful.

mcdruid’s picture

Added a test which verifies that a successful password reset (login) clears flood events for a user, so they can request another password reset even if they were at the threshold for being blocked before using the last reset URL they requested, per @klausi's suggestion in #130.

If we're happy with this approach, we should backport it to #3074666: [D7] Password reset form has no flood protection.

mcdruid’s picture

@klausi does this address your concerns (in #130)?

If so, happy for this to go back to RTBC?

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

Moving this back to RTBC as I think we've addressed the concerns in #130.

Remaining question is whether we're happy using only the uid for the user-based flood control.

I'd argue it's okay, and that any site having trouble with it could adjust the thresholds (IP-based vs. user-based) if needs be.

pdenooijer’s picture

RTBC +1

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,11 +69,12 @@ class UserController extends ControllerBase {
    -  public function __construct(DateFormatterInterface $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data, LoggerInterface $logger) {
    +  public function __construct(DateFormatterInterface $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data, LoggerInterface $logger, FloodInterface $flood) {
         $this->dateFormatter = $date_formatter;
         $this->userStorage = $user_storage;
         $this->userData = $user_data;
         $this->logger = $logger;
    +    $this->flood = $flood;
    

    Can we do the BC dance here?

    e.g. change it to $flood = NULL and if we get passed NULL, fetch it from the singleton and trigger a deprecation error - there are classes in contrib that subclass this - see http://grep.xnddx.ru/node/29192493

    And yes, this isn't part of our API, but that doesn't mean we can't be nice about it so that the minor upgrade is a bit smoother for folks running said modules.

  2. +++ b/core/modules/user/src/Form/UserPasswordForm.php
    @@ -40,10 +49,16 @@ class UserPasswordForm extends FormBase {
    -  public function __construct(UserStorageInterface $user_storage, LanguageManagerInterface $language_manager) {
    +  public function __construct(UserStorageInterface $user_storage, LanguageManagerInterface $language_manager, ConfigFactory $config_factory, FloodInterface $flood) {
         $this->userStorage = $user_storage;
         $this->languageManager = $language_manager;
    +    $this->configFactory = $config_factory;
    +    $this->flood = $flood;
       }
     
       /**
    @@ -52,7 +67,9 @@ public function __construct(UserStorageInterface $user_storage, LanguageManagerI
    
    @@ -52,7 +67,9 @@ public function __construct(UserStorageInterface $user_storage, LanguageManagerI
       public static function create(ContainerInterface $container) {
         return new static(
           $container->get('entity_type.manager')->getStorage('user'),
    -      $container->get('language_manager')
    +      $container->get('language_manager'),
    +      $container->get('config.factory'),
    +      $container->get('flood')
    

    😿same here re BC - http://grep.xnddx.ru/node/368235 there are subclasses in contrib

mcdruid’s picture

Thanks!

This patch hopefully adds the BC dance for the new params in the two constructors.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC on the basis that I'm pretty sure we've done what @larowlan suggested.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @mcdruid - we just need to add a deprecation test to ensure the error is triggered.

See e.g. \Drupal\Tests\Core\Entity\ContentEntityFormTest for an example

I'm keeping a close eye on this as its on my wishlist for 8.8

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
18.85 KB
5.25 KB

Added deprecation tests.

andypost’s picture

Having 2 legacy tests in this case is fine, RTBC++

Status: Needs review » Needs work

The last submitted patch, 145: 1681832-145.patch, failed testing. View results

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
18.85 KB
1005 bytes

Fix test fail.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

Great! So I think we're back to RTBC hopefully.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Thanks, agree this is ready - we just need a change record.

Setting to NR for that.

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Added a basic change record. Let me know if it needs more info.

mcdruid’s picture

Thanks - CR looks good. I tweaked a couple of words as it mentioned log messages, which we're not actually doing here (they're just errors set on the form).

If/when #2983395: user module's flood controls should do better logging lands we might want to revisit this to see if we want to add logging and 403 responses, but I'd prefer to get the foundations in rather than block this one.

larowlan’s picture

  • larowlan committed dc7b06c on 8.8.x
    Issue #1681832 by mcdruid, k_zoltan, dougvann, ZenDoodles, Cyberschorsch...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.8.0 highlights

Committed dc7b06c and pushed to 8.8.x. Thanks!

Published the change record

Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.