WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#34905 closed defect (bug) (fixed)

zxcvbn() not defined (and making it impossible to edit an existing user)

Reported by: wp-plugin@… Owned by: adamsilverstein
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.3.1
Component: Users Keywords: commit has-screenshots
Focuses: javascript Cc:

Description (last modified by SergeyBiryukov)

This site is only using the Akismet plugin.

Uncaught ReferenceError: zxcvbn is not defined

wp.passwordStrength.meter @ load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:4
f @ load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:12
m.event.dispatch @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
r.handle @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4

2015-12-07 17:01:57.903 load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:4

Uncaught ReferenceError: zxcvbn is not defined

wp.passwordStrength.meter @ load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:4
f @ load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:12
m.event.dispatch @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
r.handle @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
m.event.trigger @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
e.event.trigger @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:8
(anonymous function) @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
m.extend.each @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:2
m.fn.m.each @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:2
m.fn.extend.trigger @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
(anonymous function) @ load-scripts.php?c=0&load[]=hoverIntent,common,admin-bar,password-strength-meter,underscore,wp-util…:12
m.event.dispatch @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4
r.handle @ load-scripts.php?c=0&load[]=jquery-core,jquery-migrate,utils,zxcvbn-async&ver=4.3.1:4

2015-12-07 17:01:57.938 load-scripts.php?c=0&l

Attachments (7)

34905.diff (3.7 KB) - added by peterwilsoncc 5 years ago.
34905.2.diff (3.7 KB) - added by adamsilverstein 5 years ago.
34905.3.diff (4.2 KB) - added by peterwilsoncc 4 years ago.
34905.4.diff (5.1 KB) - added by peterwilsoncc 4 years ago.
34905.5.diff (5.0 KB) - added by adamsilverstein 4 years ago.
34905.6.diff (4.1 KB) - added by peterwilsoncc 4 years ago.
34905.7.diff (5.1 KB) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (32)

#1 @peterwilsoncc
5 years ago

  • Keywords reporter-feedback added

Hi and welcome to trac.

Are you able to provide the following information to help recreate the bug

  • steps to recreate the issue
  • browser/browsers you are getting the error in

I will look into the problem but the info above will help if I am unable to recreate it locally.

Thanks

#2 @wp-plugin@…
5 years ago

Chrome ( on Mac OSX )

the user being edited has existed for a long time.

These error show up no matter which user I try to edit.

#3 follow-up: @peterwilsoncc
5 years ago

  • Focuses javascript added
  • Keywords needs-patch added; reporter-feedback removed

Able to recreate

  • In developer tools enable network throttling to emulate a poor network (2G)
  • Edit a user
  • Before the images/assets have loaded, click the generate password button
  • The generated password may remain blank if the JavaScript in the HTML footer is still loading
  • Start typing a password
  • zxcvbn is undefined and an error will be thrown in the browser console.

Using Chrome 47.0/OSX 10.11.1 the browser recovers once zxcvbn is available and the strength meter starts working.

PROPOSED SOLUTION: wp.passwordStrength.meter fails gracefully if zxcvbn is undefined.

  • returns -1 for strength unknown (new string required)
  • users need to accept the possibly weak password (string change/new string may be required)

#4 in reply to: ↑ 3 ; follow-up: @adamsilverstein
5 years ago

Replying to peterwilsoncc:

PROPOSED SOLUTION: wp.passwordStrength.meter fails gracefully if zxcvbn is undefined.

Could we have the Generate Password button load disabled, then enable when zxcvbn is ready? That would make it clear to users the button wasn't clickable. Also, I believe we already hide the generate password button if JavaScript is disabled.

#5 @SergeyBiryukov
5 years ago

  • Description modified (diff)

#6 in reply to: ↑ 4 @peterwilsoncc
5 years ago

Replying to adamsilverstein:

Could we have the Generate Password button load disabled, then enable when zxcvbn is ready? That would make it clear to users the button wasn't clickable. Also, I believe we already hide the generate password button if JavaScript is disabled.

Allowing passwords to be entered while zxcvbn loads improves performance. At 323K (gzipped), I think this important for users on low-speed mobile networks.

I'll trial both approaches to allow others to test.

@peterwilsoncc
5 years ago

#7 @peterwilsoncc
5 years ago

  • Keywords has-patch added; needs-patch removed

This slipped my mind way back when, here's a first pass at solving the race condition on slow networks.

In 34905.diff:

  • wp.passwordStrength.meter fails gracefully if zxcvbn is undefined.
  • Password strength indicator warns password strength is unknown
  • user needs to accept the use of a potentially week password
  • If the user enters a new password prior to the library loading:
    • no password is generated
    • password strength indicator updates once the library is ready

To test, open the network tab in Chrome dev tools, disable caching and throttle the network speed to 2G.

#8 @peterwilsoncc
5 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

#9 @ocean90
5 years ago

Maybe related to #36584.

#10 @peterwilsoncc
5 years ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-passwords by peterwilsoncc. View the logs.


5 years ago

#12 @peterwilsoncc
5 years ago

#36584 was marked as a duplicate.

#13 @adamsilverstein
5 years ago

  • Owner changed from peterwilsoncc to adamsilverstein

Hi @peterwilsoncc; thanks for the patch.

I tested this using the steps you described and was able to reproduce the issue and verify it is fixed nicely by your patch. I think your approach is sound, however I only tested the patch in Chrome; before this is committed, I think we should perform some additional testing across browsers/devices (including ie8 :( )

Here is a screencast showing the console errors when interacting before the patch:

http://cl.ly/122c1y1x2w3O

Here is a screencast showing the process after the patch is applied; the field still works and there is no error:

http://cl.ly/123E1h3M0S3E

#14 @adamsilverstein
5 years ago

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

34905.2.diff Add a bit of whitespace as per WordPress JS coding standards.

#15 @peterwilsoncc
4 years ago

In 34905.3.diff

Shuffles the functionality for showing/hiding the checkbox for confirming weak passwords. In previous editions of the patch the weak password check would not update following the JavaScript loading.

Tested in the following with network link conditioner enabled.

Chrome/OSX
FF/OSX

(Modern.ie)
IE8/Win 7
IE11/Win 7

(Browser Stack)
IE9/Win 7
IE10/Win 7
IE10/Win 8
IE11/Win8.1
Edge 12/Win10
Edge 13/Win10
FF46/Win10

#16 @peterwilsoncc
4 years ago

In 34905.4.diff, uses a function call to show or hide the weak password checkbox rather than triggering an event. Checked against IE8.

#17 @markjaquith
4 years ago

I think this is a good approach. As before, JS just errored, I'm not too worried about the new -1 return value. On any reasonably fast connection, you'll never encounter this. And I like that we're saying "potentially weak" and requiring confirmation when the strength is unknown.

#18 @adamsilverstein
4 years ago

@peterwilsoncc This patch looks good.

Here is a screencast of the patch in action on chrome, over a throttled 2G connection:

http://cl.ly/0l172S1x1R0T

Lets get this committed in beta so more people can test it out.

#19 @adamsilverstein
4 years ago

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

34905.5.diff:

  • Add a period at the end of comment block
  • Add space before comment blocks

#20 @pento
4 years ago

I think the JS coding standards handbook page is wrong.

"...should always be preceded by a blank line." should really be "...should always be preceded by a blank line, except for at the start of an indented block."

#21 @peterwilsoncc
4 years ago

34905.5.diff wasn't applying cleanly, 34905.6.diff is same but refreshed.

I agree with @pento, the line break hinders comprehension at the start of the control structure. The coding standards include examples both with and without a preceding empty line. What fun.

#22 @netweb
4 years ago

If you were to make any JS coding standards decisions, it would be great if said decision was somewhat based on the ESLint rule that applies to lines around comments: http://eslint.org/docs/rules/lines-around-comment

To which I'm thinking based on 34905.7.diff would be both of the following:

  • "beforeBlockComment": false disallows an empty line before block comments
  • "beforeLineComment": true requires an empty line before line comments

There's lots of ESLint rule configs to decide on for #31823

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

#23 follow-up: @pento
4 years ago

It seems that the appropriate ESLint rules would be:

beforeBlockComment: true
beforeLineComment: true
allowBlockStart: true

Anyway, 34905.7.diff is good to commit.

#24 @peterwilsoncc
4 years ago

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

In 37940:

Users: Check zxcvbn is defined before calling.

Prevents JavaScript errors by checking zxcvbn is defined before calling.

Changes wp.passwordStrength.meter() to return -1 if the strength of the password is unknown.

On the user profile screen, generatePassword() checks if the user has entered the password before setting the value of the password input box.

Props peterwilsoncc, adamsilverstein.
Fixes #34905.

#25 in reply to: ↑ 23 @netweb
4 years ago

Replying to pento:

It seems that the appropriate ESLint rules would be:

beforeBlockComment: true
beforeLineComment: true
allowBlockStart: true

Thanks @pento :+1

Note: See TracTickets for help on using tickets.