WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 11 days ago

#43749 reopened enhancement

Update zxcvbn to 4.4.2

Reported by: desrosj Owned by: desrosj
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch needs-testing
Focuses: javascript Cc:

Description

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.

Attachments (6)

43749.diff (1.5 MB) - added by valchovski 7 months ago.
43749.2.diff (811.0 KB) - added by desrosj 7 months ago.
43749.3.diff (813.2 KB) - added by omarreiss 7 months ago.
43749.4.diff (813.3 KB) - added by omarreiss 5 months ago.
43749.5.diff (813.3 KB) - added by omarreiss 5 months ago.
43749.6.diff (10.3 KB) - added by desrosj 5 months ago.

Change History (30)

#1 @desrosj
9 months ago

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

#2 @netweb
9 months ago

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

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
7 months ago

#3 @valchovski
7 months ago

  • Keywords has-patch added; needs-patch removed

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

#4 @desrosj
7 months ago

  • Keywords needs-refresh added

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.

#5 @valchovski
7 months ago

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.

#6 @netweb
7 months ago

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
7 months ago

#7 @desrosj
7 months ago

  • Keywords needs-testing added

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.

#8 @desrosj
7 months ago

  • Keywords needs-refresh removed

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

#9 @netweb
7 months ago

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

@omarreiss
7 months ago

#10 @omarreiss
7 months ago

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.

#11 @desrosj
6 months ago

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.

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


6 months ago

@omarreiss
5 months ago

@omarreiss
5 months ago

#13 @omarreiss
5 months ago

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?

#14 @desrosj
5 months ago

  • Milestone changed from Future Release to 5.0

@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.

@desrosj
5 months ago

#15 follow-up: @netweb
5 months ago

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)

#16 in reply to: ↑ 15 @desrosj
5 months ago

  • Keywords needs-testing removed

@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.

#17 @netweb
5 months ago

  • Keywords commit added

Right you are @desrosj, patch looks good to me

#18 @pento
3 months ago

  • Milestone changed from 5.0 to 5.1

#19 @desrosj
4 weeks ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

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.

#20 @desrosj
4 weeks ago

  • Keywords commit removed

#21 @desrosj
4 weeks ago

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.

#22 @desrosj
4 weeks ago

  • Keywords needs-testing added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#23 @desrosj
4 weeks ago

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.

#24 @pento
11 days ago

  • Milestone changed from 5.1 to 5.2

Bumping to 5.2, as it requires further investigation.

Note: See TracTickets for help on using tickets.