Routine user error can lead to plaintext passwords in the database

Confirmed that the patch in #5 applies cleanly and prevents user passwords from being stored in the watchdog table if a user accidentally enters their username in the password field.

Case:
User accidentally enters password in User Login form username field.

Result before patch:
Users with "access site reports" permission are able to see password on Recent log messages page.

Result after patch:
IP address from which failed login attempt was made is displayed on Recent log messages page.

Cases where a valid username and incorrect password are entered remain unchanged (watchdog message = "Login attempt failed for [username]") as do successful logins.

Looks good to me. Committed/pushed to 8.x. Moving back to 7.x for backport.

Hm, wondering if this should have a test?

+++ b/core/modules/user/user.moduleundefined
@@ -1373,7 +1373,14 @@ function user_login_final_validate($form, &$form_state) {
+        // If the username entered is not a valid user,
+        // only store the IP address.

The comment should be formatted to use as much of 80 characters as possible.

Also, I can see that the code is doing that, would be more useful to comment why. Because someone might consider to remove this again if not seeing the reason. Especially without having tests for it ;)

I agree with #8. I only saw this in the commit log and tried very hard to see how a password would end up in the DB with the previous code.
A comment would be great. Tests, as well. Re-titling for that.

Updated issue summary.

Poor forgotten issue is sad.

I hate to point this out so long after being committed but IP address is already stored on the watchdog. Also, as a site administrator, username was helpful in helping people trouble shoot logging in. But between tech savviness and proxies, IP addresses are pretty worthless for this message.

Thanks for the tests Berdir - and I like the new comment. Much better explanation of why.

@neclimdul:

IP address is already stored on the watchdog

Where else is the IP address written to watchdog for failed logins? I can confirm that it is only written once and that it comes from the original patch in this issue.

username was helpful in helping people trouble shoot logging in

I can see where this might be useful, but if a user is entering a valid username, it will still appear in watchdog. If you see that their IP is being stored instead, you know that they're entering an invalid user name, so that's a great place to start troubleshooting.

One other thing that was discussed when I first reported this to the security team, but that hasn't been brought up yet in this thread is that Viewing the database logs only requires "access site reports" permission, which is a low-level administrative permission; that is, not one of the ones listed at http://drupal.org/security-advisory-policy. (Just one more reason to keep this is - along with Berdir's improvements.)

reroll

ended up exporting a reversed patch.

Should this issue still be "needs review" , since the patch was already committed?

This issue is still needs review because there have been follow up issues reported since the last commit. The follow up issues were: 1) needs a test and 2) a more descriptive comment. These things were done in #15 but that patch no longer applies. I updated the comment to be clearer and rerolled the patch.

The patch in #23 no longer applies. I rerolled it.

+++ b/core/modules/user/tests/src/Functional/UserLoginTest.php
@@ -29,6 +35,44 @@ public function testLoginCacheTagsAndDestination() {
+    \Drupal::config('user.flood')
+      ->set('ip_limit', 10)
+      // Set a high per-user limit out so that it is not relevant in the test.
+      ->set('user_limit', 4000)
+      ->save();

Looks like this part of the test isn't working. Looks like that was also the case in the previous version of the patch though.

This issue was committed to Drupal 8.x and re-opened to add tests. Between then and now that work was done in #2983395: user module's flood controls should do better logging. If further work is needed regarding the UserLogin form it would be best to create a new issue.

Closing this as fixed and changing the version to when the other issue was fixed.