| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • chasemp | T121186 Implement results of enwiki Security review RfC | |||
| Open | None | T32574 Display a password strength bar | |||
| Declined | None | T211489 Security review of bjeavons/zxcvbn-php |
Event Timeline
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
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.
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.
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
zxcvbn-php had a stable 1.0 release recently (diff with 0.4, release notes) which included some fairly major changes to make it match the behavior of the original (JS) zxcvbn library (before it was more of a reimplementation of the idea than a proper PHP version of the same library - see bjeavons/zxcvbn-php#15 for more background).
It would be nice to use the 1.0 version, both because of the JS consistency (we probably won't use the JS library, but nice to have that option open), because the algorithm is probably superior, and because it is more modern (e.g. supports same PHP versions as MW master). Does that require a re-review?
Hi @Tgr - it does appear to need a re-review. We are putting in our backlog and unassigning until someone can pick it up. We're doing our best but have limited resource hours right now, so please let us know if you have any questions or concerns and we'll be in touch as we move forward. Thanks!
Is there any plan to use this if we re-review it? Is anyone likely to work on implementing it/using it?
Obviously if we do re-review, it needs prioritising etc.
The Security team is updating their readiness review SOP to reflect a new change that any request that has aged 90 days without being in a reviewable state will be declined. We do this to help keep our work area current, accurate and reflective of actual work. If the status of your project changes please re-tag us and we will get this work scheduled.
@Jcross: Please do not remove correct metadata as it makes it harder to find tasks. This is and was a security readiness review request (which has been declined). Thanks.