Don't trim whitespace when setting user password

I verified the same behavior exists in D8. If you sign up with the password " abc ", you can log back in later with either " abc " or the trimmed string "abc".

Updated issue summary.

If it does no harm for the user to have entered the spaces (that is, it doesn't prevent the user from logging in) is there a reason to expose this system quirk to the user?

As long as the trimmed spaces do not contribute to a greater password complexity rating, the user will see that " abc " is not complex enough and hopefully increase the complexity of their password. (In 7.x, they do contribute, so I've filed issue #2254235 and related it to this issue.)

Alternatively, a less technical explanation would be a warning such as "Leading and trailing spaces do not improve password strength."

As long as the trimmed spaces do not contribute to a greater password complexity rating, the user will see that " abc " is not complex enough and hopefully increase the complexity of their password. (In 7.x, they do contribute, so I've filed issue #2254235 and related it to this issue.)

It can still be misleading and give users a false sense of security. Suppose a user's password strength is already at the maximum according to the password-strength indicator*, but they add some spaces to further increase the strength of their password. Those spaces are not actually stored, against the expectation of the user.

There is another unusual behavior caused by this problem unrelated to password-strength indication: If a user attempts to save a password of all spaces, the password appears to save, yet their password remains unchanged.

* I assume the password-strength indicator has a maximum indication in D8 as it does in D7.

There is another unusual behavior caused by this problem unrelated to password-strength indication: If a user attempts to save a password of all spaces, the password appears to save, yet their password remains unchanged.

Yes this is definitely an issue and it has caught me out in the past when I wasted a bunch of time investigating it. It gives a very strange user experience and is also a security issue of sorts.

Confirmed issue is still valid in 8.3.x

Its probably better we don't trim whitespace: Password managers will record the whitespace. (confirmed with 1Password).

Taking the position of removing spaces means we are adding arbitrary password rules/requirements.

Perhaps spaces make a password harder to brute force because they are unexpected characters!

If a site is upgrading, it already has users which potentially have their passwords trimmed from spaces; however, the users do not necessarily know it is trimmed as their input on login is also trimmed each time. I think we need to maintain that behaviour for existing sites and only apply this to fresh installs.

If I understand correctly, the config/install/* will be installed when a site is first created, but existing sites will have NULL as the password_trim_spaces value and if NULL, we can set it to TRUE to maintain the existing behaviour.

Thoughts?

password_trim_spaces

We don't really do backwards compatibility flags in Drupal. I'd say go all in on removing trimming, or defer to next major version (9.x)

already has users which potentially have their passwords trimmed from spaces;

If they put spaces in it originally, then it is likely they will continue to put spaces in passwords both before and after the patch. This means there is no noticable diffference at all.

At worst, a password reset is required if they originally used spaces, but for some reason they know of this bug and stopped using spaces thereafter.

We should maintain the existing behavior. If not then this is a bug report. Anyway, we need functional tests for the new config and we also need upgrade path tests.

This is a bug as it violates people's reasonable expectations. If someone has included leading or trailing spaces in their password, they expect them to be used.

I'd venture to guess that most of the time when people include a leading or trailing space it is because they copy and pasted a password and didn't notice the space, but I have no data to support that hunch.

@hmendes, thank you for updating the Issue Summary. I find that really helpful.

First, a reroll. I did make one additional change, to add password_trim_spaces to user.schema.yml because \Drupal\Tests\user\Functional\UserCancelTest::testUserCancelWithoutPermission was failing. Testing locally and the patch doesn't work, setting password_trim-spaces to false and updating config, clear cache and the password was still trimmed. But then, this is my first patch touching javascript so the reroll could be wrong anyway.

This is working for me, although I did minimal testing. Still needs tests, plus and update test.

If some portion of site users won’t be able to log in, isn’t that a sort of backwards compatibility break? Ironically the behavior change would hurt the same users—those that cleverly lead or follow a pass phrase with spaces—that it tries to help.

To me, this would have to release in a major version, and it needs to be mentioned in release notes.

I think the issue summary is correct in saying the current functionality "probably saves many more headaches than it causes". I think there may be a way to preserve these benefits while addressing the concerns expressed here:

• Add validation that disallows leading/trailing whitespace at either side of a newly submitted password
• Add logic in the password strength meter to warn users that trailing/leading whitespace is disallowed. This prevents validation-frustration, and is a small addition to logic that has already been implemented.

Something like this could address the concerns without being as invasive. It also doesn't require the multiple instances of conditional logic necessary to apply different behavior depending on when the user was created. From what I can see, the changes in the patch do this nicely, but an unexpected use case would impact an incredibly crucial piece of a sites functionality.

Obviously I did not read the patch over carefully! It’s a new default, but opt-in for existing sites?

I prefer the approach suggested in #28 because people oft add white-spaces wrongly

I wrote a patch with the approach proposed in #28.
It still need tests.

Ops! I only checked JS lint.

Added test coverage. Test out leading whitespace. If that works, can use same pattern to test trailing whitespace.