backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

The user_login_authenticate_validate() function is prevented from execution if password is not entered #5658

Open alanmels opened 2 years ago

alanmels commented 2 years ago

Description of the bug

The correct notice is not output to user if the password filed is empty.

Steps To Reproduce

To reproduce the behavior:

  1. Go to admin/config/people/login and set either Limit login attempts by IP address or Limit login attempts by user to some low limit (eg: 1) for purpose of this test.
  2. Log out as admin and log back in as regular user.
  3. Try to login with the correct user/e-mail address and incorrect password couple times to get blocked until you see a message:

Sorry, too many failed login attempts from your IP address. This IP address is temporarily blocked. Try again later or request a new password.

Note, that it is the correct message.

  1. Now, enter the correct username/e-mail address and leave the password empty to see a different message, in fact two messages, the first one of which s correct, however the second one is not:

Password field is required. Sorry, no account with that email address found.

because the account actually exists, it's just blocked.

Additional information

The problem is caused by line https://github.com/backdrop/backdrop/blob/0394202d00a9b6c8d3b5e3c1625c24e0ed0f5f75/core/modules/user/user.module#L1813 that looks like:

if (strlen($name) && strlen($password)) {

effectively preventing the flood control check in case if the password field is not entered and thus causing the incorrect message.

IMO, there is no need to check password in this function, because of two reasons:

  1. Let the flood control check do its work and output the correct message;
  2. User login form has three validators, namely user_login_name_validate(), user_login_authenticate_validate(), user_login_final_validate() and all of them try to validate the password field, which is already an overkill, so I'd personally would leave the password field validation in only the user_login_name_validate() function which is executed first.
indigoxela commented 2 years ago

... effectively preventing the flood control check in case if the password field is not entered

That's the point, that's a bit unclear to me. If the password is not entered, I would expect that the "field required" message is there, of course. But flood control should strike even if the form submission was incomplete, anyway? :thinking:

The odd thing: if the username is correct and the password field is empty, I also get "Sorry, unrecognized username", which for sure isn't as intended.

Maybe the first step here is to settle on expected behavior in following scenario:

Username is correct, NO password given, flood limit already reached.

This message combination in above scenario is for sure a bug:

Password field is required.
Sorry, unrecognized username.
alanmels commented 2 years ago

I also believe that if password field is empty, then that should be enough to prevent further validation. However, if the current state is preserved (run through all three validation functions regardless if the first one fails), then the misleading notice should be prevented and the flood control system let to properly run it's validation.

ghost commented 2 years ago

I don't think flood protection should be involved at all unless there's a password entered... Flood protection is to prevent brute-force attacks, right? Well that's not an issue if the password's blank.