Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#51613 closed defect (bug) (fixed)

Error thrown on login page

Reported by: ntsekouras's profile ntsekouras Owned by: sarahricker's profile sarahricker
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Login and Registration Keywords: has-patch has-screenshots commit
Focuses: Cc:

Description

Currently when you visit the login page, an error is thrown in console.

The root for this might be in some recent changes (3 days ago) in password generation in core.

The request is coming from here: https://github.com/WordPress/WordPress/blob/c575f66422d8288dbe478c7717df9f5685be0f80/wp-admin/js/user-profile.js#L150

That file changed 3 days ago in this commit here: (https://github.com/WordPress/WordPress/commit/daa977c495e73bb2b85a517e464c16fd4f2d3233).

Attachments (3)

Screen Shot 2020-10-25 at 8.48.41 PM.png (16.5 KB) - added by sarahricker 3 years ago.
5163-login-ajax-error
51613.diff (561 bytes) - added by adamsilverstein 3 years ago.
password-reset.jpg (92.7 KB) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (29)

#1 @SergeyBiryukov
3 years ago

  • Component changed from General to Login and Registration
  • Milestone changed from Awaiting Review to 5.6
  • Version set to trunk

#2 @SergeyBiryukov
3 years ago

  • Keywords reporter-feedback added

Hi there, welcome back to WordPress Trac! Thanks for the report.

When vising the login screen, I see a couple of jQuery Migrate warnings, but no errors:

jquery-migrate.js?ver=3.3.1:100 JQMIGRATE: jQuery.fn.click() event shorthand is deprecated
migrateWarn @ jquery-migrate.js?ver=3.3.1:100
jQuery.fn.<computed> @ jquery-migrate.js?ver=3.3.1:658
(anonymous) @ user-profile.js?ver=5.6-beta1-20201023.222339:275

jquery-migrate.js?ver=3.3.1:100 JQMIGRATE: jQuery.fn.change() event shorthand is deprecated
migrateWarn @ jquery-migrate.js?ver=3.3.1:100
jQuery.fn.<computed> @ jquery-migrate.js?ver=3.3.1:658
bindPasswordForm @ user-profile.js?ver=5.6-beta1-20201023.222339:111
(anonymous) @ user-profile.js?ver=5.6-beta1-20201023.222339:378

Could you share some more details about the error you're seeing?

This ticket was mentioned in PR #661 on WordPress/wordpress-develop by sarahricker.


3 years ago
#3

  • Keywords has-patch added

Password generation should not load on the WordPress login page.

Trac ticket: https://core.trac.wordpress.org/ticket/51613

@sarahricker
3 years ago

5163-login-ajax-error

#4 @sarahricker
3 years ago

  • Keywords needs-testing has-screenshots added
  • Owner set to sarahricker
  • Status changed from new to assigned

Hey @SergeyBiryukov! I took a quick look at this. It was the ajax error in above screenshot. Resolved by preventing wp.ajax.post( 'generate-password' ) from running on the wp-login.php page, including when redirected to the page, as there is nothing to generate there.

Note: Ability to generate passwords still runs on user profile and when on the login page for password resets.

See Github PR: https://github.com/WordPress/wordpress-develop/pull/661

#5 @hellofromTonya
3 years ago

Pinging @adamsilverstein to get his attention on this ticket too. This problem may have been introduced in Ticket #42852 in changeset [49248].

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

#6 @TimothyBlynJacobs
3 years ago

What happens with this change on the reset password screen?

#7 @adamsilverstein
3 years ago

Thanks for reporting @ntsekouras and working on this @sarahricker. I will take a look. Odd that the src/js/_enqueues/admin/user-profile.js file is being pulled into the login screen at all!

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

#8 @sarahricker
3 years ago

Yep I believe the reset password screen relies on the password generation script to suggest a first password? It does not however use the button to suggest additional strong passwords, yet the ajax is still called and errors.

  • Preventing that part from loading on wp-login.php as the patch does fixes it.
  • Adding the button to randomize other strong passwords like the user profile page does may also fix it -- but I would consider that a feature for next release if that is suggested. Not a bad idea though, if I do say so myself!

#9 @adamsilverstein
3 years ago

the reset password screen

Ah, that makes more sense. I will test there as well. Trying to determine best condition to use to avoid the error.

#10 @hellofromTonya
3 years ago

  • Keywords dev-feedback added; reporter-feedback removed

#11 @adamsilverstein
3 years ago

I confirmed the use of user-profile.js on the password reset screen, which is served via the same PHP page as login with a URL domain/wp-login.php?action=rp. As you point out, that screen prefills the password (on the PHP side), I think the JS here mostly activates the password meter and handles the 'weak password' checkbox and disabling the submit button when required.

In my testing, I was able to remove the pre-setting of the password that was causing the error. This had no effect on the editing/setting passwords on the user profile screen, or after resetting WordPress (wp db reset) on the New Site setup screen (where you also create a user).

Patch incoming.

#13 @adamsilverstein
3 years ago

In 51613.diff removes the preloading of the password in JS entirely; we already do this in PHP where needed. In my testing this fixed the reported console error and password generation continued to work as expected.

@sarahricker and @ntsekouras can you confirm this resolves the console error you were seeing and also if possible test the user profile, password reset and new site install screens to verify password generation (plus show/hide and cancel when present) still works as expected?

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

#14 @adamsilverstein
3 years ago

  • Keywords reporter-feedback added

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


3 years ago

#16 @sarahricker
3 years ago

Hey @adamsilverstein Yes that was my experience as well! Your patch also fixes the problem in a cleaner way, however do you know the history of that code you're removing? Wouldn't want to break something else unknown!

51613.diff Passed all concerns I'm aware of:

  • user profile > generate password
  • wp-login.php?action=rp
  • wp-login.php

#17 @adamsilverstein
3 years ago

@sarahricker thanks for confirming.

Yea, we introduced that code in https://core.trac.wordpress.org/changeset/49248 when working on https://core.trac.wordpress.org/ticket/42852. @bookdude13 write the original patch with that code, perhaps they can weigh in with any feedback about the origin of the block I am proposing we remove. In my testing it is not required.

Version 0, edited 3 years ago by adamsilverstein (next)

#18 @sarahricker
3 years ago

  • Keywords 2nd-opinion added; needs-testing removed

Awesome, thanks @adamsilverstein! If @bookdude13 signs off on https://github.com/WordPress/wordpress-develop/pull/662 then I'm good with moving forward on this one on my end.

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

#19 @bookdude13
3 years ago

@adamsilverstein @sarahricker I haven't been active for a while, so not sure what's changed with the files involved in the patch since I made it. I removed that chunk in the 42852.4 patch, then re-added to the final patch because there was a noticeable delay when populating the password field in the profile page. If that delay isn't there anymore, then this should be fine.

#20 @ntsekouras
3 years ago

Hey @adamsilverstein and @sarahricker - thanks for working on this!

By removing the request it's okay.

#21 @ntsekouras
3 years ago

  • Keywords reporter-feedback removed

#22 @sarahricker
3 years ago

  • Keywords commit added; dev-feedback 2nd-opinion removed

Awesome, thanks @ntsekouras! I think this is ready for commit then, @SergeyBiryukov @hellofromTonya

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


3 years ago

#24 @helen
3 years ago

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

In 49337:

Login: Avoid AJAX error on login screen.

This has to do with the password generator, which does not need to generate and cache passwords in JS as that's already done in PHP.

Props adamsilverstein, sarahricker.
Fixes #51613.

hellofromtonya commented on PR #662:


3 years ago
#26

Trac ticket closed with changeset https://core.trac.wordpress.org/changeset/49337 merge.

Note: See TracTickets for help on using tickets.