Problem

During the site install, I created an admin account with a 256 char random password. The password was generated in Keepass, and copied/pasted into the form. I was able to log in when the install completed, perhaps with the help of the browsers' autocomplete. I created an author account with another long password the same way. I logged in successfully as the author and posted some dummy content.
However, a day later, I am unable to log in to either account. I notice that the maxlength attribute on the password field is 60, and if I remove that attribute and then submit the form, Drupal gives me an error message:

Password cannot be longer than 60 characters but is currently 256 characters long.

Edit: I just noticed that when changing a password, the maxlength of the password fields are 128. However, when logging in, the maxlength is 60.

Proposed resolution

Update the core user module and the new password form(s).

Remaining tasks

Edit a core module and a core template?

User interface changes

Display all of the password criteria, including maximum length and restricted characters, when a user is setting a password.

API changes

Consider setting the max password length to a larger value, 60 seems arbitrary and small. Maybe 1KB or so?

As a workaround, I changed the admin password directly in the database using user_hash_password().
For the record, this install is running in WAMP on Windows 7, and I believe that drush is unavailable.

Files: 
CommentFileSizeAuthor
#33 remove-login-block-password-maxlength-1777270-33.patch370 bytessmokris
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]
#24 UserLoginTest_refactored_1777270_23.patch8.6 KBaries
FAILED: [[SimpleTest]]: [MySQL] 50,744 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#22 UserLoginTest_refactored_1777270_22.patch6.71 KBaries
FAILED: [[SimpleTest]]: [MySQL] 50,632 pass(es), 27 fail(s), and 16 exception(s).
[ View ]
#20 testPasswordLength.patch1.73 KBaries
PASSED: [[SimpleTest]]: [MySQL] 49,382 pass(es).
[ View ]
#17 testPasswordLength.patch1.68 KBaries
PASSED: [[SimpleTest]]: [MySQL] 49,260 pass(es).
[ View ]
#16 testPasswordLength.patch1.68 KBaries
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#10 remove-login-block-password-maxlength-1777270-10.patch377 bytesDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 39,533 pass(es).
[ View ]
#7 remove-login-block-password-maxlength-1777270-7.patch395 bytesDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 46,194 pass(es).
[ View ]

Comments

danjro’s picture

I found one of the problems.

On line 1310 of modules/user/user.module:

<?php
  $form
['pass'] = array('#type' => 'password',
   
'#title' => t('Password'),
   
'#maxlength' => 60,
   
'#size' => 15,
   
'#required' => TRUE,
  );
?>

should probably be

<?php
  $form
['pass'] = array('#type' => 'password',
   
'#title' => t('Password'),
   
'#maxlength' => 128,
   
'#size' => 15,
   
'#required' => TRUE,
  );
?>

in order to match line 380 of modules/system/system.module:

<?php
  $types
['password'] = array(
   
'#input' => TRUE,
   
'#size' => 60,
   
'#maxlength' => 128,
   
'#process' => array('ajax_process_form'),
   
'#theme' => 'password',
   
'#theme_wrappers' => array('form_element'),
  );
?>

Or maybe one of these values should refer to the other, I'm new here.

danjro’s picture

Title:I am no longer able to log in to any of the user accounts that I created with long random passwords.» Maximum password length is 60 chars in user.module and 128 chars in system.module.
danjro’s picture

Assigned:Unassigned» danjro

I would like to learn how to fix this myself, properly, so I am assigning this to myself. However, this will be my first patch, and it may take me a day to figure it all out. I will ask for help on the #drupal irc channel if I need it. If someone beats me to the punch, that's totally cool too.

Devin Carlson’s picture

Version:7.15» 8.x-dev

Awesome ross9885, the more core developers the merrier!

Some notes from my inital look at the issue (repeating some of your observations):

  • User module stores passwords in the "pass" column which has a length limit of 128 characters. (user.install)
  • User module provides a block which allows users to enter their username and password in order to login. The block's password textfield has a maximum length restriction of 60 characters. This is incorrect and should be changed to 128. (user.module)
  • When installing Drupal, the password field has no validation check for the length of the password. #1344552: Password confirm field type should have support for #maxlength attribute is related but can be worked around by manually validating the length of the password in install_configure_form_validate() (install.core.inc)
  • When updating a user password on the user edit page, the password field has no validation check for the length of the password. #1344552: Password confirm field type should have support for #maxlength attribute is related but can be worked around by manually validating the length of the password in user_validate_current_pass() (user.module) which is called by AccountFormController (AccountFormController.php)

Also, patches are made against Drupal 8 and then backported to Drupal 7 in order to prevent regressions.

tim.plunkett’s picture

Priority:Major» Normal
Issue tags:+needs backport to D7

This is a nasty bug, but I don't think it should be blocking other development.

Really nice find, @ross9885! And great issue summary.

ssm2017 Binder’s picture

why not remove the size and maxlength because they are already defined in the default password form element declaration ?

this is ok to define a custom value for custom modules but i think that drupal should have the same value everywhere.

i m telling this because i'm building a custom module and i have overrided the form element maxlenght but this is still forced in some drupal's forms like the login form so i need to also use a form alter or preprocess instead of just altering the default form element.

Devin Carlson’s picture

Assigned:danjro» Devin Carlson
Status:Active» Needs review
Issue tags:+Novice
StatusFileSize
new395 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,194 pass(es).
[ View ]

A patch to implement ssm2017 Binder's suggestion.

The login block's password form item doesn't need to declare a maxlength since all password elements have an appropriate default maxlength of 128.

caelon’s picture

Status:Needs review» Reviewed & tested by the community

I have tested the patch and confirmed that it does not enforce 60 characters max on the login form.

Because this pointed out a larger problem that all password fields should be consistent, I did a grep to find all '#type' => 'password' instances assuming they would be the only ones that would have max sizes:

grep -r "'#type' => 'password'" *

I reviewed all nine core files found by that grep and none of them, post-patch, has a maxlength attribute so all are now consistently coded.

webchick’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Looks like this already got committed at some point down the line to 8.x. Moving down to 7.x, which should have that same test by caelon in #8 to ensure that there's only one instance.

Devin Carlson’s picture

Status:Patch (to be ported)» Needs review
StatusFileSize
new377 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,533 pass(es).
[ View ]

The patch from #1772584: Get rid of user_login_block() & user_login() in favour of user_login_form() fixed the issue and was committed three days ago by yourself. :P

See http://drupalcode.org/project/drupal.git/commit/b38996475dd09e5c3709397c...

A patch against D7, since the issue which fixed this in D8 will not be backported.

barraponto’s picture

Status:Needs review» Reviewed & tested by the community

Just ran ack --type-add php=.inc password -A 10 . --php | grep maxlength to doublecheck if we were missing anything, seems ok.

David_Rothstein’s picture

Looks good to me, but shouldn't we have an automated test here too?

This issue has the "Novice" tag, and it might actually be a good test for someone who is looking to get started with simpletests to work on. Basically, the goal would be to create a user with a password equal to the maximum allowed length and then make sure that user can log in (via both the user login block and the regular /user/login page) without any errors.

This test could go into Drupal 8 too, actually...

David_Rothstein’s picture

Title:Maximum password length is 60 chars in user.module and 128 chars in system.module.» Write tests for: Users with passwords over 60 characters cannot log in via the user login block
Version:7.x-dev» 8.x-dev
Category:bug» task
Status:Reviewed & tested by the community» Active
Issue tags:+Needs tests

Actually, come to think of it, since this is already fixed in Drupal 8 it will be a lot simpler if we just commit it to Drupal 7 now and do the tests as a followup.

So... committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/dafe7ae

David_Rothstein’s picture

Component:user system» user.module

@Devin Carlson, feel free to unassign yourself if you don't want to write those tests, by the way :)

nagwani’s picture

@David Carlson, anything I can help with. Happy to help. Thanks

aries’s picture

StatusFileSize
new1.68 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Here is patch for the test case.

aries’s picture

StatusFileSize
new1.68 KB
PASSED: [[SimpleTest]]: [MySQL] 49,260 pass(es).
[ View ]

I've left a mistake, the length of the valid and invalid case were the same. Apologies.

David_Rothstein’s picture

Status:Active» Needs review

Thanks! Setting to "needs review" so that the testbot will run it.

balintk’s picture

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -23,6 +23,45 @@ public static function getInfo() {
+      if ($password_length === 128) {
+        $this->assertNoText(t('User login'), 'Logged in with 127 character length password.');

Why does the assertion message say 127 characters here when the password was generated with 128 characters?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserLoginTest.phpundefined
@@ -23,6 +23,45 @@ public static function getInfo() {
+      if ($password_length === 129) {
+        $this->assertText(t('Log in'), 'Couldn\'t log in with 255 character long password.');

Similarly, I don't understand the assertion message. Why does it contain 255 characters? I could totally miss something here with those characters, but if that's the case maybe documenting the reason in a comment would be a good idea.
Also, it's just a small thing, but using single quotes and an escaped apostrophe in the string should be avoided (even though this is not inside of t()). I did a quick grep and I learned that it's more common to avoid this situation by using the non-shortened form "could not". Additionally, a super-minor remark: I would also make sure that the two assertion messages have nearly the same wording (i.e.: "length" vs. "long").

aries’s picture

Assigned:Devin Carlson» aries
StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 49,382 pass(es).
[ View ]

Thanks Bálint, I've accidentally left those numbers in. I've updated the test according your recommendations.

balintk’s picture

I was focusing on those assertion messages, but now I've given more thoughts to the approach of the patch.

Use assertFailedLogin()

There is already a method in the same class for making an unsuccessful login attempt, it would be better to use that.

Consider implementing setUp() method

Since almost all methods in this class need at least one user account to perform tests it would be interesting to consider implementing the setUp() method, creating user accounts there and making them accessible via class properties so that each test method in the class can utilize them.
One of the user accounts could be created with the maximum allowed numbers of characters in the password (other tests will pass just fine), so in the test method introduced by the patch this password could be simply changed when we want to test the login failure scenario.
If the setUp() method is a good idea, maybe refactoring the other methods to use the user object created by that could be a follow-up issue.

Test user login block as well

As @David_Rothstein said the test should cover the login attempts from a login block as well, not only logins from the regular user login page.

aries’s picture

StatusFileSize
new6.71 KB
FAILED: [[SimpleTest]]: [MySQL] 50,632 pass(es), 27 fail(s), and 16 exception(s).
[ View ]

Cheers Bálint for the review. This patch contains number of improvements what Bálint suggested:

  • UserLoginTest class refactored to use shared user accounts
  • additional test to cover the login block cases

Use of assertFailedLogin(). assertFailedLogin() expects wrong user credentials, where Drupal returns with Sorry, unrecognized username or password. Have you forgotten your password?' instead of Password cannot be longer than... which means the credentials were correct but the password has invalid length. That's why I handled this in different way.

Status:Needs review» Needs work

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

aries’s picture

StatusFileSize
new8.6 KB
FAILED: [[SimpleTest]]: [MySQL] 50,744 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Now only I fail left which looks like because a bug in the Blocks module. The testPasswordLengthBlock() should be fine, but the issue is a bit weird.

The login fails because the invalid password length, but in the block header instead t('User login') I get the username. With same block the error message and the form appears as it should. Check the raw response here: http://pastebin.com/97nAzeqW . With cURL the login via the UI works fine.

aries’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

smiletrl’s picture

I'm a little confused that have we committed the code to fix the problem described here?
If the answere is yes, so what we need to do now is to write tests for this scenario?

aries’s picture

@smiletrl: I noticed the issue I described in #21 while I was writing tests for log in. I need confirmation, is this a real issue?
To replicate it all you need is to apply the patch I attached and run the test via either Drush (drush test-run UserLoginTest) or UI.

smiletrl’s picture

@aries, yeah, this is an interesting problem as in #24. I get the same issue too.

I suggest do this test manually. Create such user in code directly and login via UI. I agree that there could be some problem with user-login form. Or I'm thinking because the length has been designed in db for 128, this new created user in code could lead to unexpected result from db.

<?php
     
'pass' => array(
       
'type' => 'varchar',
       
'length' => 128,
       
'not null' => TRUE,
       
'default' => '',
       
'description' => "User's password (hashed).",
      ),
?>

Maybe we could change the way we are testing? Try to create a user with 129 length pass, and get some error? But this page test returns right result:(

Maybe there's problem with drupalPost, like what you assumed. Anyway, let's test this manually, and see what happens:)

aries’s picture

@smiletrl: I've tested manually, I saved and put the response to pastebin.com. The link is in #24.

smiletrl’s picture

a)

John@JOHN-PC /d/apache/drupal82 (user_login_tests-1777270-29)
$ grep -r 'Password cannot be longer' *  

returns nothing. I'm not sure where does this scentence come from.

b) my manual test returns 'Sorry, unrecognized username or password. Have you forgotten your password?' when I try to log in with 129 length password.

c)I think we could add something like this in user.module

<?php
/**
 * Maximum length of user password text field.
 */
const USERPASS_MAX_LENGTH = 128;
?>

and do this for user_login_form and user_register_form

<?php
  $form
['pass'] = array(
   
'#type' => 'password',
   
'#title' => t('Password'),
   
'#size' => 60,
   
'#maxlength' => USERPASS_MAX_LENGTH// add constraint here.
   
'#description' => t('Enter the password that accompanies your username.'),
   
'#required' => TRUE,
  );
?>

This should solve our problem.
We may add some password validate process, like username does

<?php
 
if (drupal_strlen($name) > USERNAME_MAX_LENGTH) {
    return
t('The username %name is too long: it must be %max characters or less.', array('%name' => $name, '%max' => USERNAME_MAX_LENGTH));
  }
?>

But this validate looks redundant to me, since $form['name'] ['#maxlength'] has already defined the max length.

d) I think we need so something like this to place user_login_block.

<?php
   
// Add user login block.
   
$this->adminUser = $this->drupalCreateUser(array('administer blocks'));
   
$this->drupalLogin($this->adminUser);
   
$this->drupalPlaceBlock('user_login_block');
   
$this->drupalLogout($this->adminUser);
?>

But it seems this block is there without such an admin-user. Anyway, just a suggestion.

e) I can't explain why this test returns this wierd result. Perhaps it needs deep exploring code effort:(

aries’s picture

@smiletrl: your suggestions are already implemented.

aries’s picture

Issue summary:View changes

Added info about maxlength attribute.

smokris’s picture

Title:Write tests for: Users with passwords over 60 characters cannot log in via the user login block» Users with passwords over 60 characters cannot log in via the user login block
Version:8.x-dev» 6.x-dev
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new370 bytes
PASSED: [[SimpleTest]]: [MySQL] 190 pass(es).
[ View ]

This issue also affects Drupal 6.x (and is becoming more noticeable as more of our users are adopting xkcd-style password novellas).

Could we apply the attached backport to Drupal 6.x, then resume work on tests for 8.x?