WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#51613 closed defect (bug) (fixed)

Error thrown on login page

Reported by: ntsekouras Owned by: 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 8 months ago.
5163-login-ajax-error
51613.diff (561 bytes) - added by adamsilverstein 8 months ago.
password-reset.jpg (92.7 KB) - added by adamsilverstein 8 months ago.

Download all attachments as: .zip

Change History (29)

#1 @SergeyBiryukov
8 months ago

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

#2 @SergeyBiryukov
8 months 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.


8 months ago

  • Keywords has-patch added

Password generation should not load on the WordPress login page.

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

@sarahricker
8 months ago

5163-login-ajax-error

#4 @sarahricker
8 months 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
8 months 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 8 months ago by SergeyBiryukov (previous) (diff)

#6 @TimothyBlynJacobs
8 months ago

What happens with this change on the reset password screen?

#7 @adamsilverstein
8 months 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 8 months ago by adamsilverstein (previous) (diff)

#8 @sarahricker
8 months 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
8 months 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
8 months ago

  • Keywords dev-feedback added; reporter-feedback removed

#11 @adamsilverstein
8 months 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
8 months 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 8 months ago by adamsilverstein (previous) (diff)

#14 @adamsilverstein
8 months ago

  • Keywords reporter-feedback added

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


8 months ago

#16 @sarahricker
8 months 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
8 months ago

@sarahricker thanks for confirming.

Yea, we introduced that code in [49248] when working on #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.

Last edited 8 months ago by SergeyBiryukov (previous) (diff)

#18 @sarahricker
8 months 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 8 months ago by sarahricker (previous) (diff)

#19 @bookdude13
8 months 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
8 months ago

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

By removing the request it's okay.

#21 @ntsekouras
8 months ago

  • Keywords reporter-feedback removed

#22 @sarahricker
8 months 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.


8 months ago

#24 @helen
8 months 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.

#26 @prbot
8 months ago

hellofromtonya commented on PR #662:

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

Note: See TracTickets for help on using tickets.