Page MenuHomePhabricator

Security review of bjeavons/zxcvbn-php
Closed, ResolvedPublic

Event Timeline

Tgr created this task.Dec 9 2018, 12:16 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 9 2018, 12:16 AM
Paladox added a subscriber: Paladox.Dec 9 2018, 3:53 AM
Reedy added a subscriber: Reedy.Dec 9 2018, 1:30 PM

Based on https://github.com/bjeavons/zxcvbn-php/pull/32 (seems a mismatch with the dropbox js version as detailed in https://github.com/bjeavons/zxcvbn-php/issues/15 ) the upstream might not be very active, and it has actually been forked... https://github.com/mkopinsky/zxcvbn-php and https://packagist.org/packages/mkopinsky/zxcvbn-php

Tgr added a comment.Dec 9 2018, 8:01 PM

Although the discussion there gives the impression that the bjeavons version actually does a better job. And the last commit to the Dropbox repo was over a year ago and the last functional change two years ago, so it's not like there is a constant flood if upstream improvements the library would be expected to keep up with.

The mismatch is a problem if we intend to run zxcvbn on the client side as well, but per T32574#2836570 we probably don't.

Reedy and Tgr, Please decide upon the specific component to be used and update.

sbassett added a subscriber: sbassett.EditedJan 22 2019, 10:56 PM

Pinging @Reedy and @Tgr per @charlotteportero 's comment above.

Tgr added a comment.Jan 22 2019, 11:18 PM

I don't have a strong preference. If we have any plans of running zxcvbn in Javascript we should go with the mkopinsky version, but we are unlikely to run it client-side because it would involve downloading large dictionaries, and a standalone node.js login service is science fiction at this point, so I don't think JS compatibility is something we need to be concerned about. Other than that, they both seem stable, not very active, both have some password quality issues and neither seems obviously superior to the other... the bjeavons version is more widely used which maybe means more eyes on security issues (which is what we are really concerned about here) so if I had to choose I'd go with that.

sbassett triaged this task as Medium priority.EditedJan 23 2019, 4:51 PM

Thanks, @Tgr. Unless @Reedy has strong objections, we'll go with the bjeavons repo and get this scheduled for a security review.

Reedy closed this task as Resolved.Apr 1 2019, 4:20 PM

This can go ahead.

As the fork hasn't diverged much (the API is the same), if we have a strong need we can switch to the other version down the road