WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 11 days ago

Last modified 8 hours ago

#42766 closed defect (bug) (fixed)

Issue in update password From admin side and login ith same password

Reported by: ronakganatra Owned by: adamsilverstein
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Users Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: ui, javascript, administration, performance Cc:

Description

Issue details:
-> login to admin area:
-> go to user tab
-> Click on your admin user
-> Go to password field click on generate password
-> Remove generated password
-> keep that password box empty or fill it with any number of space.
-> Click on update profile.
-> You will get a success message that your profile was successfully updated.
-> logout
-> But try using that same password or previous password you can't login.

Attachments (6)

42766.diff (1010 bytes) - added by 1naveengiri 3 years ago.
Good Catch @ronakganatra Adding fix patch for it.
42766.2.diff (539 bytes) - added by 1naveengiri 3 years ago.
@adamsilverstein trimming is better here, as you know spaces are not considered as empty. I have removed that error, your point seems meaningful to me here.
42766.patch (3.6 KB) - added by ronakganatra 3 years ago.
As pe my testing this code is working fine.
42766.3.diff (1.8 KB) - added by oolleegg55 3 years ago.
Add unit test
42766.4.diff (2.3 KB) - added by adamsilverstein 12 days ago.
42766.5.diff (2.3 KB) - added by adamsilverstein 11 days ago.

Download all attachments as: .zip

Change History (42)

#1 follow-up: @adamsilverstein
3 years ago

  • Keywords needs-patch good-first-bug added
  • Owner set to adamsilverstein
  • Status changed from new to assigned

Hi @ronakganatra - thanks for the bug report.

I ran thru these steps in mac/chrome dev channel and was able to reproduce by entering spaces into the field. if I leave the field blank, my password is unchanged. We probably shouldn't touch the password if your new password is all spaces, and when i tried logging in with the spaces as my password, it didn't work.

Last edited 3 years ago by adamsilverstein (previous) (diff)

@1naveengiri
3 years ago

Good Catch @ronakganatra Adding fix patch for it.

#2 @1naveengiri
3 years ago

  • Keywords has-patch added; needs-patch removed

#3 in reply to: ↑ 1 @ronakganatra
3 years ago

Thnaks for appreciation
Replying to adamsilverstein:

Hi @ronakganatra - thanks for the bug report.

I ran thru these steps in mac/chrome dev channel and was able to reproduce by entering spaces into the field. if I leave the field blank, my password is unchanged. We probably shouldn't touch the password if your new password is all spaces, and when i tried logging in with the spaces as my password, it didn't work.

#4 @adamsilverstein
3 years ago

  • Keywords needs-unit-tests added

@1naveengiri Thanks for the patch! this looks like it will resolve the issue, I will give it a test.

Did you consider checking for empty vs. trimming? i guess trimming is better, spaces at the beginning or end of passwords are likely copy/paste or user entry errors.

I'm not sure we need the error message, would users get that when submitting a blank password field? they might expect empty to indicate no change (and not an error).

Additionally, it would be good to add a unit test here validating that a blank password will not be saved.

#5 @adamsilverstein
3 years ago

  • Keywords needs-testing added

@1naveengiri
3 years ago

@adamsilverstein trimming is better here, as you know spaces are not considered as empty. I have removed that error, your point seems meaningful to me here.

#6 @1naveengiri
3 years ago

@adamsilverstein we already have a unit test for empty password check.
https://develop.svn.wordpress.org/trunk/tests/phpunit/tests/user.php
function test_edit_user_blank_pw()

#7 @ronakganatra
3 years ago

Perfect patch Naveen.

@ronakganatra
3 years ago

As pe my testing this code is working fine.

#8 @adamsilverstein
3 years ago

  • Keywords needs-testing removed

@ronakganatra thanks for testing.

@1naveengiri Ah right - thanks for tracking down the function test_edit_user_blank_pw test, which seems like it should test for this. However, in this case we were sending not a blank password but one consisting only of spaces, right?

We can expand the unit test to catch that case so the test fails before this patch, but passes after it.

#9 @ajensen
3 years ago

I was able to reproduce the bug when entering spaces into the field. After applying the patch however I was able to log in succesfully. Although it still lets me update my profile with blank and spaces only passwords, I can only login with my original password. I also tested trimming after applying the patch and found it to work succesfully.

@oolleegg55
3 years ago

Add unit test

#10 @oolleegg55
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@adamsilverstein I added a patch containing unit test.
I am confused a little with file 42766.patch. It contains code which doesn't regard this ticket in my opinion. I didn't use the file 42766.patch for new file 42766.3.diff.

This ticket was mentioned in Slack in #core by oolleegg55. View the logs.


3 years ago

#12 @afercia
3 years ago

Related: #42852 which aims to make the interaction a bit more transparent to users.

#13 @pento
21 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Version trunk deleted

#14 @adamsilverstein
12 months ago

  • Milestone changed from Future Release to 5.4

#15 @bookdude13
9 months ago

  • Keywords needs-testing added

Passwords with no length still don't allow for updating the profile (disabled button), but the issue remains for passwords just made of spaces. You can update the password to " " successfully, but then not log in with either " " or the old password.

Like @ajensen, after the patch I still see the following:

  • When creating a user with two spaces as a password (and weak checkbox checked) I can click Add User, although I get the empty password error message which stops me from setting it
  • When updating a user with two spaces as a password (and weak checkbox checked) I can click Update Profile and it will appear to be successful. However, when logging in I cannot use the two space password. The old password (the one before " ") works to log in.

Right now I think we need the profile update page to not show a success if the password ends up trimmed (and thus not updated). A more descriptive error message would be nice as well, since the password technically isn't empty for the user.

Note that I haven't checked out the unit test, since my current Windows environment isn't set up for that atm :)

#16 @SergeyBiryukov
8 months ago

  • Component changed from Administration to Users

#17 @SergeyBiryukov
8 months ago

#49433 was marked as a duplicate.

#18 @bookdude13
8 months ago

  • Keywords needs-design-feedback added

Design questions on this:

  • Do we want a password of only spaces/space-like characters (e.g. tabs maybe if pasted in) to be allowed? Or should we forbid passwords being set if they would be "empty" after being trimmed?
  • If the second case, then when creating the user, should the feedback to the user change at all, since they did enter a non-empty password and the error message would be "password empty"?
  • Should the updating user page be able to update (e.g. click the button) if the only thing updated is the password, and it's an invalid password (e.g. will be trimmed to nothing, not updated)?

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


8 months ago

#20 @nrqsnchz
8 months ago

  • Keywords needs-design-feedback removed

Discussed today during design triage meeting in Slack: https://wordpress.slack.com/archives/C02S78ZAL/p1581961322460700

Do we want a password of only spaces/space-like characters (e.g. tabs maybe if pasted in) to be allowed?

Not sure if this can be answered by the design team. This seems more like a security or compliance issue.

If the second case, then when creating the user, should the feedback to the user change at all, since they did enter a non-empty password and the error message would be "password empty"?

If the password is rejected because it contains empty spaces, and these are forbidden, then yes, the error messages should be clear and indicate why the password is being rejected.

Should the updating user page be able to update (e.g. click the button) if the only thing updated is the password, and it's an invalid password

I don't think so. If an error has occurred, this should be communicated to the user instead of falsely making them believe the operation was a success.

#21 @afercia
8 months ago

Related, for the UI part: #42749 and #42852.

#22 @adamsilverstein
8 months ago

  • Milestone changed from 5.4 to 5.5

Punting this to 5.5 as we aren't quite ready with a solution here.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


3 months ago

#25 @whyisjake
3 months ago

  • Keywords needs-design added
  • Milestone changed from 5.5 to 5.6

#26 @aristath
8 weeks ago

@noudwordpress, @hansjovisyoast and myself tried tackling this one today.
After reading the comments above, we started wondering if the problem is the fact that the user can set a whilespace-only (spaces, tabs etc) password, or the fact that after setting it they can't login with it.
After a lot of backtracing and debugging we came to the conclusion that allowing whitespace-only characters as a password would introduce a lot more issues than it would solve. It is possible to allow them, but not without weakening security or implementing "hacky" solutions.

Do we want a password of only spaces/space-like characters (e.g. tabs maybe if pasted in) to be allowed?

According to our findings, though technically possible, it would be inadvisable to allow whitespaces-only passwords.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


6 weeks ago

#28 @paaljoachim
6 weeks ago

We looked at this during Monday's Design triage and agreed on that this ticket does not need design. I am thinking that the password field needs to limit what can be entered.

#29 @hellofromTonya
2 weeks ago

  • Keywords needs-design removed

Per @paaljoachim comment above, this ticket does not need design.

We looked at this during Monday's Design triage and agreed on that this ticket does not need design.

#30 @adamsilverstein
2 weeks ago

This is pretty close, although after the last patch some additional concerns were raised. I think we can address these by adding the same 'trim' logic for passwords that are all spaces to the JavaScript side, so we can make the action button disabled (just like when the field is blank). I'll plan to get back to this soon unless someone else beats me to it :)

#32 @adamsilverstein
12 days ago

  • Keywords commit added; needs-testing removed

In 42766.4.diff:

  • Disable submit button when trimmed password empty.

The Submit button now remains disabled if the password field contains only spaces.

#33 @adamsilverstein
11 days ago

42766.5.diff - fixes for php linter.

#34 @adamsilverstein
11 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 49118:

Users: prevent saving empty passwords, trim space from password ends on save.

Fix an issue where users could save a password with only spaces, or spaces at the beginning or end of their password, preventing them from logging in.

Props ronakganatra, 1naveengiri, ajensen, oolleegg55, bookdude13, nrqsnchz, aristath.
Fixes #42766.

#35 @SergeyBiryukov
10 days ago

In 49126:

Tests: Use assertSame() in test_edit_user_blank_password(), for consistency with other assertions.

Follow-up to [49118].

See #42766, #38266.

Note: See TracTickets for help on using tickets.