WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#33699 closed defect (bug) (fixed)

Hidden password input fields should default to disabled="disabled"

Reported by: raamdev Owned by: ocean90
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Users Keywords: has-patch needs-testing
Focuses: ui, javascript, administration Cc:

Description

There have been several reports of WordPress v4.3 sending the "notice of password change" email every time a user's profile is updated. I'm aware of a similar issue where some plugins are improperly calling wp_update_user() with a user_pass field, but what I'm describing in this ticket is separate and unrelated.

Some browsers (reproduced sporadically with Firefox and Chrome) with the "Save Passwords" or "Auto Fill" options enabled are submitting the hidden pass1 input field on the Edit User page, with a password auto-filled from the browser, resulting in the updated user's password being reset whenever the "Update User" button is clicked, even if nothing on the users profile was changed.

Since WordPress v4.3 now hides the password reset input box behind the "Generate Password" button, the hidden password input field should default to having the HTML attribute disabled="disabled" so that even if it is automatically filled by the browser behind-the-scenes, it is never POSTd; i.e., if the field is not in view, disable it entirely, to avoid the potential for this to occur.

The JavaScript used to show the password input field whenever you click "Generate Password" in WP v4.3+, could then remove the disabled="disabled" attribute when it is in view, so that you can interact with it and so that the browser will POST when it is in view.

Attachments (7)

wp-v43-password-input.png (170.9 KB) - added by raamdev 4 years ago.
Hidden password input field should have disabled="disabled" attribute
33699.diff (1.2 KB) - added by adamsilverstein 4 years ago.
33699.2.diff (844 bytes) - added by flixos90 4 years ago.
Fixed JS disabling the password field on installation routine
33699.3.diff (1.6 KB) - added by flixos90 4 years ago.
fixed further JS issues during installation / lostpassword form
33699.4.diff (768 bytes) - added by adamsilverstein 4 years ago.
33699.5.diff (388 bytes) - added by ocean90 4 years ago.
33699.6.diff (476 bytes) - added by ocean90 4 years ago.

Download all attachments as: .zip

Change History (40)

@raamdev
4 years ago

Hidden password input field should have disabled="disabled" attribute

#1 @wonderboymusic
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#2 @adamsilverstein
4 years ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@raamdev: Thanks for the bug report.

I can reproduce a similar issue by going to the edit profile screen, then telling LastPass to autofill. I'm not sure how to reproduce with one of the browser password helpers you mentioned.

Can you give the steps to reproduce and/or try adding disabled="disabled" attribute to all the hidden fields, including the generated one, to see if it helps.

I'm not sure disabling the fields will change anything - currently all fields have the parameter autocomplete="off". This is the setting that is supposed to tell LastPass and other password managers "don't autofill this field".

Unfortunately, it looks like LastPass ignores these settings BY DEFAULT. To fix this issue you can set LastPass to honor the autofill parameter:
http://cl.ly/image/1H2m0E0A0O3E/Preferences_2015-09-29_15-54-48.jpg

I tested this and verified it works correctly, preventing LastPass from filling the hidden fields even when I try to manually 'autofill'.

Can you please give it a try and let me know if this resolves your issue?

Recommend wontfix pending feedback from reporter.

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

#3 follow-up: @JasWSInc
4 years ago

Can you give the steps to reproduce and/or try adding disabled="disabled" attribute to all the hidden fields, including the generated one, to see if it helps.

I can confirm that adding disabled="disabled" resolves this in Chrome, Firefox, and Opera. Adding disabled="disabled" seems the way to go! :-) Any field that is not being displayed should be disabled to avoid this conflict. That way any attempt to auto-fill a field (by whatever software; e.g., Chrome, LastPass, etc.) will have no adverse effect. Note that a field that is disabled (even if POSTd) will not be sent to the server. That's exactly what we need in this scenario.

#4 in reply to: ↑ 3 @adamsilverstein
4 years ago

Ok @JasWSInc thanks for the feedback and confirming this fixes the issue.

I will work up a patch with the disabled attribute applied. Happy to add that if it resolves the issue! I use chrome password manager but never seem to have this issue - i wonder if my settings are different somehow? any special steps to reproduce?

Replying to JasWSInc:

Can you give the steps to reproduce and/or try adding disabled="disabled" attribute to all the hidden fields, including the generated one, to see if it helps.

I can confirm that adding disabled="disabled" resolves this in Chrome, Firefox, and Opera. Adding disabled="disabled" seems the way to go! :-) Any field that is not being displayed should be disabled to avoid this conflict. That way any attempt to auto-fill a field (by whatever software; e.g., Chrome, LastPass, etc.) will have no adverse effect. Note that a field that is disabled (even if POSTd) will not be sent to the server. That's exactly what we need in this scenario.

#5 @adamsilverstein
4 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed
  • Milestone changed from Future Release to 4.4

In 33699.diff:

  • Disable all hidden input fields on initial load, when the 'Generate Password' button is visible and the fields are hidden.
  • Enable all inputs when 'Generate Password' button clicked.
  • Disable all inputs when 'Cancel' clicked.

Prevents an issue where password helpers would autofill the hidden password fields, inadvertently causing password changes when editing other fields on the profile page among other issues. This patch keeps the fields 'disabled' when they are hidden which should prevent submission even if they are filled.

@JasWSInc: Can you please check the patch to see if this resolves the issue you see?

Thanks!

#6 @JasWSInc
4 years ago

@adamsilverstein I applied your patch to trunk and ran a few tests. No problems. Looks like that does the trick!

#7 @JasWSInc
4 years ago

@adamsilverstein writes...

any special steps to reproduce?

Referencing my notes here also, where I detailed how I was able to reproduce this in the very beginning: https://github.com/websharks/s2member/issues/705#issuecomment-137253739

So it's a bit tricky to reproduce. You need to have an installation of WordPress to test with that already has a stored password (in your browser) for the user profile that you're editing. In other words, the browser needs to make an attempt to auto-fill the password field on the user profile editing page.

Even then, when I'm testing this in Chrome with the hidden password field it returns inconsistent results. Approx. 1 in 10 attempts results in Chrome attempting to auto-fill the hidden input field. Anyway, your patch makes that a moot point now. Good!

#8 follow-up: @raamdev
4 years ago

I'm not sure disabling the fields will change anything - currently all fields have the parameter autocomplete="off". This is the setting that is supposed to tell LastPass and other password managers "don't autofill this field".

@adamsilverstein Disabling the fields (via the disabled="disabled" attribute) ensures that the browser does not send the field in the POST at all, even if that field somehow contains a value.

I haven't been able to reproduce the issue that I described with any regularity, but what's happening, as @JasWSInc mentioned, is that the browser is sometimes sending the pass1 and/or pass2 field with the POST, even when the field is hidden, and when the browser (or perhaps an extension) does not obey the autocomplete="off" setting and fills in the hidden field with a value this results in a WordPress Password Reset inadvertently occurring when updating the users profile.

Since there's no reason to send the field with the POST when the field is not visible, setting disabled="disabled" is what we want.

I reviewed your patch and it looks good to me. :-) Thank you!

#9 in reply to: ↑ 8 @adamsilverstein
4 years ago

Super, thanks for clarifying. Appreciate the bug report and testing.

#10 @adamsilverstein
4 years ago

#34108 was marked as a duplicate.

#11 @adamsilverstein
4 years ago

  • Milestone changed from 4.4 to 4.3.2

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


4 years ago

#13 @adamsilverstein
4 years ago

Related: #33441

The patch on this ticket resolves the issues raised in #33441.

#14 @adamsilverstein
4 years ago

  • Keywords commit added
  • Milestone changed from 4.3.2 to 4.4

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


4 years ago

#16 @ocean90
4 years ago

#33441 was marked as a duplicate.

#17 @ocean90
4 years ago

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

In 35603:

Passwords: Disable hidden input fields on profile/user edit page.

Prevents an issue where password helpers would autofill the hidden password fields and inadvertently causing password changes when editing other fields on the profile page.

Props adamsilverstein.
Fixes #33699.

#18 @flixos90
4 years ago

  • Keywords needs-patch added; has-patch dev-feedback commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

On the installation screen the password input field is now disabled since it is initially opened already.

@flixos90
4 years ago

Fixed JS disabling the password field on installation routine

#19 @flixos90
4 years ago

  • Keywords has-patch added; needs-patch removed

The patch above fixes the issue by checking the password field's name attribute ('admin_password' on the installation screen, but different in a user's profile in backend).

@flixos90
4 years ago

fixed further JS issues during installation / lostpassword form

#20 @flixos90
4 years ago

  • Keywords needs-testing added

The above patch fixes the previous one which did not work properly on the password reset form.

Furthermore the JS files needed during installation are only enqueued in the steps where they're necessary to prevent JS errors.

#21 @adamsilverstein
4 years ago

@flixos90 Good catch. I think this affects the password reset screen as well where the password is initially shown.

I like your suggestion of breaking out the script if needed, however lets leave that for another ticket.

In 33699.4.diff :

  • Only disable text password field if it is hidden, corrects a bug where the visible password field would be disabled on the password reset and WordPress install screens since r35603.
  • Verified passes JSHint

#22 @flixos90
4 years ago

@adamsilverstein sounds good. I created the ticket at #34700, including a patch like the fix from above.

#23 @ocean90
4 years ago

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

In 35649:

Passwords: Only disable hidden password fields if they are really hidden.

Makes the password field on install and for password resets editable again. Both fields were accidentally set to disabled in [35603].

Props adamsilverstein, flixos90.
Fixes #33699.

#24 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[35603] and [35649] broke creating a new user via Add New User screen.

Notice: Undefined index: user_pass in wp-includes/user-functions.php on line 1295

The password is no longer passed to edit_user() and wp_insert_user().

#25 @ocean90
4 years ago

#34750 was marked as a duplicate.

@ocean90
4 years ago

#26 @wonderboymusic
4 years ago

  • Owner changed from adamsilverstein to ocean90
  • Status changed from reopened to assigned

your journey

@ocean90
4 years ago

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


4 years ago

#28 @adamsilverstein
4 years ago

33699.6.diff resolves the underlying issue where we were inadvertently submitting the fields disabled and their value was never passed to PHP. This patch ensures the fields get enabled before the submission occurs. I verified that I no longer get a PHP notice when adding a new user.

Once possible issue would be that if validation fails (for example, omitting the email address), the fields would be enabled again. I don't think the password managers would then attempt to autofill these fields, because I think they only check for password fields on page load; however I am unable to reproduce the original issue consistently.

@raamdev or @JasWSInc can you check @ocean90's patch to see if you see any of the original issue recurring after applying this patch: if possible, please test the edit profile screen and try submitting the form with a validation error (for example, change the email to something thats not an email). submit, get the error, correct and resubmit; then verify the password wasn’t inadvertently changed.

Thanks!

#29 @ocean90
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Anyone using a password manager, testing highly appreciated.

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


4 years ago

#31 @ocean90
4 years ago

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

In 35733:

Passwords: Re-enable password fields before submitting the form.

Avoids an PHP undefined notice when creating new users.

Fixes #33699.

#32 @swissspidy
4 years ago

#34815 was marked as a duplicate.

#33 @swissspidy
4 years ago

#33992 was marked as a duplicate.

Note: See TracTickets for help on using tickets.