WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 3 months ago

#43749 reopened enhancement

Update zxcvbn to 4.4.2

Reported by: desrosj Owned by: desrosj
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch needs-testing needs-refresh
Focuses: javascript Cc:
PR Number:

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 18 months ago.
43749.2.diff (811.0 KB) - added by desrosj 18 months ago.
43749.3.diff (813.2 KB) - added by omarreiss 18 months ago.
43749.4.diff (813.3 KB) - added by omarreiss 16 months ago.
43749.5.diff (813.3 KB) - added by omarreiss 16 months ago.
43749.6.diff (10.3 KB) - added by desrosj 16 months ago.

Change History (32)

#1 @desrosj
20 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
20 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
18 months ago

#3 @valchovski
18 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
18 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
18 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
18 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
18 months ago

#7 @desrosj
18 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
18 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
18 months ago

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

@omarreiss
18 months ago

#10 @omarreiss
18 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
17 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.


17 months ago

@omarreiss
16 months ago

@omarreiss
16 months ago

#13 @omarreiss
16 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
16 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
16 months ago

#15 follow-up: @netweb
16 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
16 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
16 months ago

  • Keywords commit added

Right you are @desrosj, patch looks good to me

#18 @pento
14 months ago

  • Milestone changed from 5.0 to 5.1

#19 @desrosj
12 months 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
12 months ago

  • Keywords commit removed

#21 @desrosj
12 months 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
12 months 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
12 months 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 months ago

  • Milestone changed from 5.1 to 5.2

Bumping to 5.2, as it requires further investigation.

#25 @desrosj
9 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.2 to Future Release

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.

This ticket was mentioned in Slack in #core by hareesh-pillai. View the logs.


3 months ago

Note: See TracTickets for help on using tickets.