Discrepancy with original Dropbox JS library about zxcvbn-php HOT 32 CLOSED

This won't fix the inconsistency between the zxcvbn PHP library and the JS library but I was able to get around the inconsistencies in my project by calling a node.js script from PHP rather than using the zxcvbn-php library. This would only help you if your server can run system commands and can install node.js.

Zxcvbn.php

class Zxcvbn {
    /**
     * Get password strength score from node.js version of zxcvbn
     * @param string $password Password to test
     * @param array $userData Relevant user data
     * @return array result from zxcvbn
     */
    public function checkStrength($password, $userData = [])
    {
        $cmd = 'node zxcvbn.js ' . escapeshellarg($password) . ' ' . escapeshellarg(join(' ' , $userData));
        $json = '';
        exec($cmd, $json);

        $data = json_decode($json[0], true);
        return $data;
    }
}

node install zxcvbn
zxcvbn.js

/**
 * Command line interface for zxvbn library
 * Usage:
 * npm zxvbn.js PASSWORD "USER_DATA1 USER_DATA2 ..."
 * 
 * Returns: Password score between 0 and 4
 */

var zxcvbn = require('zxcvbn');

var args = process.argv.slice(2);

var password = args[0];
var userData = args[1].split(" ");

var result = zxcvbn(password, userData);
console.log(JSON.stringify(result));

modified to sanitize shell arguments

from zxcvbn-php.

Well, I've started some work in a branch. https://github.com/mkopinsky/zxcvbn-php/blob/match-upstream/match-upstream.md is my notes on what needs to be done to bring zxcvbn-php into parity with the upstream/Dropbox library. If anyone has feedback or wants to pitch in and help, I'm totally open to both.

from zxcvbn-php.

@bcartfall I'm sorry to say this, but you should know that your code allows for arbitrary shell access.

from zxcvbn-php.

If this is truly a PHP port of zxcvbn and not just an "inspired by" library, the only long-term solution (imo) is to use the exact same algorithms, dictionaries, and unit tests as the JS/CoffeeScript library. Right now zxcvbn-php is structured somewhat differently from zxcvbn, so I fear that bring zxcvbn-php up to date in this way largely means a rewrite, at least of the core guts.

Because the public API is fairly limited, we might be able to do this without breaking the public API, but if people are relying on specific results, I don't think there's a way around that, if we want to be compatible with "upstream".

This is something I really wish I had time to take on...

from zxcvbn-php.

Thanks to @clamburger we have made a lot of progress. It's pretty close to being ready, probably within a week or two. It is up to @bjeavons as to whether it will be a PR into this repo or a new library that is released on packagist. It is a fairly substantial rewrite so I totally understand if he isn't willing to accept a PR, but at the same time, I think it would be nice if there were a single canonical zxcvbn implementation in PHP rather than having competing implementations.

Ben, what are your thoughts?

from zxcvbn-php.

Not to be annoying with another "+1", but I'm getting this also with correcthorsebattery. JS and PHP versions both report the same crack_time and entropy, but JS gives me a score of 3, and PHP gives me a score of 2. Comparing the final step of the algorithm, it seems to be quite different:

  guesses_to_score: (guesses) ->
    DELTA = 5
    if guesses < 1e3 + DELTA
      # risky password: "too guessable"
      0
    else if guesses < 1e6 + DELTA
      # modest protection from throttled online attacks: "very guessable"
      1
    else if guesses < 1e8 + DELTA
      # modest protection from unthrottled online attacks: "somewhat guessable"
      2
    else if guesses < 1e10 + DELTA
      # modest protection from offline attacks: "safely unguessable"
      # assuming a salted, slow hash function like bcrypt, scrypt, PBKDF2, argon, etc
      3
    else
      # strong protection from offline attacks under same scenario: "very unguessable"
      4

vs

    public function score($entropy)
    {
        $seconds = $this->calcCrackTime($entropy);

        if ($seconds < pow(10, 2)) {
            return 0;
        }
        if ($seconds < pow(10, 4)) {
            return 1;
        }
        if ($seconds < pow(10, 6)) {
            return 2;
        }
        if ($seconds < pow(10, 8)) {
            return 3;
        }
        return 4;
    }

I'm assuming that guesses is not the same thing as $seconds, but I find it surprising that the final score is based on different measures in each library.

from zxcvbn-php.

We're running into issues with 'thisismypassword'. PHP version scores this as a zero because it recognizes 'this' 'is' 'my' and 'password' as separate words whereas the js version splits the words into 'thisis' and 'mypassword' which scores higher entropy. Similar score of 3 on js for that phrase and 0 on php. Not sure what the solution is for this plugin but for now we are calling the php version with an AJAX call and nixing the js version on our app.

from zxcvbn-php.

@mkopinsky your branch is looking pretty good. Do you have a feel for when it will be ready to be PR'd? Unfortunately, I'm not really qualified to judge the implementation, but would be happy to do what I can to get this in. The inconsistencies between these libs are proving more problematic than i had thought.

from zxcvbn-php.

@jamieburchell I’ve been using https://github.com/mkopinsky/zxcvbn-php/releases/tag/4.4.2 without issue. But if in doubt have a look through the tests - they replicate the Dropbox ones.

from zxcvbn-php.

The JS lib mentions matching in a "us_tv_and_film" dictionary which the PHP lib doesn't have. I haven't stayed up on developments to the JS lib so perhaps it has updated dictionaries and is a factor in the different score.

What outcome are you looking for with this issue?

from zxcvbn-php.

Preferably an update to the scoring. We were looking at the JS lib scoring, but no one in our team knows CoffeeScript so that makes it a bit harder to understand what's going on.

Although, honestly, the score on the PHP lib is more consistent with what we would expect.

At this point we just wanted verification on the why the inconsistency. We haven't made a decision as to which is more correct.

from zxcvbn-php.

Yeah, I'm surprised that the JS lib scores it so high. Honestly I'm unlikely to make the time to do this research so if it's important to you I encourage you or someone on your team to. It's not too time consuming to take the Coffescript code to non-minified JS in order to discover the internals. Stepping through the internals is another story tho 😉 and not something I want to undertake right now.

from zxcvbn-php.

I have the same problem like with geheim@geheim.
JS Score: 3
PHP Score: 1

I check first on client side with the JS script and than on the server side. So i need two consistent implementations.

from zxcvbn-php.

I can confirm previous model - client check first, then server side confirmation. example password "FordMustang68."

from zxcvbn-php.

This is making it difficult for us too, we need consistency between the client side check and the server side check. Having to do an AJAX request to the PHP library is just annoying.

from zxcvbn-php.

Just to add a data-point here: 12345 test abcdef test scores a 3 with the PHP version of the library, while the JS library gives it a 4. I am also more inclined to agree with the PHP rating of it.

My issue, and I'm sure a common one at that, is that I'm using the JS library to provide user feedback but am using the PHP one for validation of incoming requests. I guess the answer is to make some async calls to the server for information.

from zxcvbn-php.

inconsistency is sad : /

I'm seeing the opposite problem — zxcvbn-php giving high scores to bad passwords. For example, fooofooofooo gets a "3", despite having repeating letters, a repeating pattern, only two different characters, and an obvious base word. (the js version scores this as "1".)

Honestly I'm unlikely to make the time to do this research …

Is this still the state of things? Is this issue going to be addressed, or "it is what it is"?

thanks!

from zxcvbn-php.

No change in state for me but I'd welcome solution proposals from others. If someone wanted to take on the work I would like to first see a brief writeup of how to solve the inconsistency challenges. Then various users of this lib can discuss pros & cons before any codes is written and a PR made.

from zxcvbn-php.

Have you seen https://github.com/Dreyer/php-zxcvbn ? I think its goal was to be an exact port of the JS library. I can say my goal was more to be "inspired by" than to be an exact match.

from zxcvbn-php.

It is a fairly substantial rewrite… but at the same time, I think it would be nice if there were a single canonical zxcvbn implementation

Agreed, especially since dropbox/zxcvbn already links here and describes it as a "port."
+1 for bringing this in!

from zxcvbn-php.

it would be nice if there were a single canonical zxcvbn implementation in PHP rather than having competing implementations

Completely agree. I haven't reviewed the other branch enough yet to have made up my mind.

@mkopinsky If it would come into this project would a single PR be appropriate? Without having full understanding of your changes hearing "substantial rewrite" makes me think incremental merges may be best, if even possible ...

from zxcvbn-php.

Splitting into multiple PRs would be pretty tough. If you look at the 3 PRs at https://github.com/mkopinsky/zxcvbn-php/pulls?utf8=%E2%9C%93&q= you can see the incremental progress that @clamburger and I made on this, but I don't think it would make sense to merge into here piece-meal.

Is there anything that we could do to help you review? I'm happy to chat through any questions offline if you'd like.

from zxcvbn-php.

I think there's a way that I can break up the changes to make it easier to review - I'll have a look at it when I get a chance.

from zxcvbn-php.

As this issue is over 2 years old, are we to assume that this library is unlikely to become an actual port of the original JS version?

I'm looking to use a PHP port to complement a progress metre on the front end and as far as I can tell, this library isn't fit for purpose if it doesn't implement the same checks and output as the original. It looks like https://github.com/mkopinsky/zxcvbn-php is still open and I'm assuming not production ready and https://github.com/Dreyer/php-zxcvbn also looks dead.

In the absence of a suitable PHP alternative, I guess the options are to use exec and the python or node versions?

from zxcvbn-php.

It looks like https://github.com/mkopinsky/zxcvbn-php is still open

My fork is production ready. I haven't done anything to mark it proactively, but it should be usable in production, and if there are issues I will do my best to resolve.

If anyone has suggestions for how to market I am all ears, or any of y'all are welcome to post about it in your favorite internet spaces. In particular, I'm curious what specifically make @jamieburchell think it's not ready, as that is something I should clearly fix.

from zxcvbn-php.

I'm curious what specifically make @jamieburchell think it's not ready, as that is something I should clearly fix.

I read through the issues here and saw that you were waiting on @bjeavons to review and merge pull requests from your repo, but that they had not had time/responded or decided how best to move forward. I therefore (wrongly it seems) assumed your changes were not production ready.

Also the "official" list of ports from the original Dropbox repo still cites this project as "the PHP port".

from zxcvbn-php.

@jamieburchell We are using the forked version in production, and it is making our life much easier. @bjeavons wrote the original zxcvbn-php to be inspired by zxcvbn, but not necessarily an exact match. I've opened a pull request with upstream to update the README to point to both libraries.

from zxcvbn-php.

My hope was that my changes would be merged into this repo, but when it became clear that wasn't going to be the case, I published my fork on packagist. Thank you for opening dropbox/zxcvbn#243 - I think it's better for someone from the "community" to do it than for the forker himself (myself).

from zxcvbn-php.

Thanks everyone for clarifying; much appreciated.

from zxcvbn-php.

+1 for https://github.com/mkopinsky/zxcvbn-php, all I did was remove and require with composer to switch over to @mkopinsky's package. It was a direct drop in for all the work I had done.

from zxcvbn-php.

+1 for https://github.com/mkopinsky/zxcvbn-php, all I did was remove and require with composer to switch over to @mkopinsky's package. It was a direct drop in for all the work I had done.

Switched to @mkopinsky too and the issue was resolved

from zxcvbn-php.

master branch now matches JS upstream, thanks to @mkopinsky' work!

from zxcvbn-php.