Update zxcvbn to 4.4.2

[desrosj]

4.4.2 contains several bug fixes.

For a full list of changes, see ​https://github.com/dropbox/zxcvbn/compare/v4.4.1...v4.4.2.

[desrosj]

Of note, this library should have the dictionary within passed through a rot13 for PG-ness.

#31647 has a patch against the library's master branch for performing this modification: https://core.trac.wordpress.org/ticket/31647#comment:9

[netweb]

Thanks for the ticket @desrosj, for the moment this ticket is blocked by #43055

As the Zxcvbn library will be relocated to a different folder as part of this ticket waiting for #43055 to be merged will help us avoid having to create another patch for the #43055 ticket with Zxcvbn 4.4.2 and creating a patch twice here in this ticket.

It is expected that #43055 will be merged in coming days/week.

[valchovski]

Hey folks,

Contributing for the first time here at WordCamp Europe - updated the Zxcvbn library to 4.4.2 and added cache busting to avoid serving an older version.

See attachment 43749.diff​

[desrosj]

Hi @valchovski,

Thanks for the patch! Great to have you as a contributor.

The patch looks good to me, but it does require one change. The dictionary text should be passed through a rot13 cipher to scramble the words. Some of them are not PG, so they should be obscured. 31647:9 has some coffee scripts that will help perform the rot13 change.

Although it may require a new ticket, it would be nice to have that process included in the build process, if possible.

[valchovski]

Hi @desrosj!

Noted your comment. Should we create a new ticket for this? Just let me know how should we proceed and I'd love to help.

[netweb]

I've not tried this myself but it would be great if we could automate the ROT13'ing of zxcvbn

There are a few ROT13 packages available at ​https://www.npmjs.com/search?q=rot13

This ​https://www.npmjs.com/package/grunt-rot13 Grunt implementation uses ​https://www.npmjs.com/package/rot and might be able to automate the process

[desrosj]

In 43749.2.diff​:

• Add the grunt-rot13 and zxcvbn NPM libraries.
• Configure the zxcvbn library to copy from its node_module folder to the correct js folder on build.
• Set up initial rot13 settings.

Remaining tasks:

• The grunt rot13 task is currently ciphering the entire file. This needs to be updated so only the dictionary part of the file is ciphered.

Also of note, the zxcvbn library has always been in core as zxcvbn.min.js, but is not actually minimized. Minimizing the file, though only results in a 2KB difference, though (820KB compared to 822KB).

The NPM package also does not include a minimized version of the library. The file is copied from the package to the js without .min and is now minified via the uglify:dynamic task. If including the unminified version of the script is not desired, the unminified file should be removed after uglify runs.

[desrosj]

Also in the previous patch, the zxcvn file was removed from the src/js/_enqueues/vendor/ folder.

[netweb]

@desrosj 43749.2.diff​ looks good to me cc: @omarreiss

[omarreiss]

The grunt rot13 task is currently ciphering the entire file. This needs to be updated so only the dictionary part of the file is ciphered.

@desrosj I've uploaded a patch that should take care of that. I just created a custom task which takes care of just that. The only thing that gets ROT13'd now is the passwords string. I haven't checked yet if that is 100% in line with what we used to do.

grunt build and grunt build:js now include ROT13'ing and uglifying.

[desrosj]

Thanks for working on this, @omarreiss!

It seems that the file is not being fully PGified. It does change the first set of dictionary words, but not the others (male names, female names, english Wikipedia, etc.). Also, if you edit a user and type a password into the field, it says "Password strength unknown". I am thinking this may just be from these other lists not being ciphered.

Also, how do you feel about adding a new 822KB file to core? I am not sure it adds any benefit as it is just a handful of long dictionary strings. But, if we choose to keep it in the build, we should change the enqueue statement to remove .min when SCRIPT_DEBUG is enabled.

[omarreiss]

43749.5.diff should fix all the mentioned issues.

It seems that the file is not being fully PGified.

Only applied the transformation on the passwords list. Made it so that all the frequency lists are now transformed.

Also, if you edit a user and type a password into the field, it says "Password strength unknown".

The rot function was prepended without a semicolon at the end. That caused it to not be defined. Fixed now.

Also, how do you feel about adding a new 822KB file to core?

I agree. Made it like it was.

@desrosj could you maybe test?

[desrosj]

@omarreiss Thanks for the update! This is looking good. Checked the generated file and it is now appropriate for bedtime story time.

I am attaching an updated patch that has the following changes:

• Fixed indentation and alignment, replacing space indentation with tabs.
• Removing a commented out line in the rot13:zxcvbn task.
• Explicitly adding wp-includes/js/zxcvbn.min.js to the list of files to minify.

The resulting zxcvbn.min.js file in 43749.5.diff​ created the file, but did not minify it. This is actually the current behavior in core. Instead of adding the file as zxcvbn.js, minifying it, and then having to add an additional task to remove the.js file, the .min.js file can be specified in the src option for the uglify task. It does have to be listed _after_ the !**/*.min.js exclusion, though. Otherwise it will not be be processed because the exclusion rule comes after.

I tested the edit profile screen and the passwords reset screens. Both successfully returned expected results in the password strength meter!

@netweb do you mind giving 43749.6.diff​ a second round of testing/review? Otherwise, I think it is good to go.

[netweb]

Looking good, I see one issue, the following can be removed:

 //.min files that still need to be minified.
 'wp-includes/js/zxcvbn.min.js'

And instead, change the dest on line 1060 to dest: 'build/wp-includes/js/zxcvbn.js' (Remove the .min)

[desrosj]

@netweb In core today, there is no unminified version of the zxcvbn library. The change you suggested would result in an unminified version of the library being added to core at about ~850kb. I think that would be ok, but it would add nearly 1MB to the package.

But, just wanted to call that addition out to make sure the decision to add the unminified version of the library is clear.

[netweb]

Right you are @desrosj, patch looks good to me

[desrosj]

In 44354:

External Libraries: Update zxcvbn to 4.2.2.

Version 4.2.2 of the zxcvbn password strength library has several bug fixes. A full list of changes can be seen here: ​https://github.com/dropbox/zxcvbn/compare/v4.4.1...v4.4.2.

This commit also adds the library as a project dependency, making it easier to update in the future. Because the dictionary within the library contains non-PG language, a rot13:zxcvbn task has been added to Grunt to perform a ROT-13 cipher on the library. This task has been added to grunt build and grunt build:js.

Props omarreiss, netweb, desrosj.
Fixes #43749.

[desrosj]

In 44356:

External Libraries: zxcvbn library.

This reverts [44354] for more investigation into inconsistent password strength values that passed locally prior to commit.

Unprops omarreiss, netweb, desrosj.
See #43749.

[desrosj]

After committing this, some inconsistency in the password strength values was uncovered. These were not failing locally for me prior to committing, so it needs more investigation.

I am wondering if the ROT-13 transformation is somehow affecting the password strength.

[desrosj]

After testing this more locally, an un-ciphered version of the library returns results consistent with the expected values in the test suite. So something in the built version of the file is causing an issue.

[pento]

Bumping to 5.2, as it requires further investigation.

[desrosj]

The latest patch no longer applies to trunk. I have also not been able to return to this. Moving out of the 5.2 milestone.

[desrosj]

Not going to be able to work on this near term. Removing myself as owner so other interested contributors do not hesitate to take this on.