valorin / pwned-validator

Super simple Laravel Validator for checking password via the Pwned Passwords service of Have I Been Pwned
MIT License
384 stars 24 forks source link

Undefined offset: 1 #3

Closed whoami15 closed 5 years ago

whoami15 commented 6 years ago

vendor\valorin\pwned-validator\src\Pwned.php63

valorin commented 6 years ago

Can you please provide more information?

That error should only occur if a malformed response was received. So either something is broken in the source data (I'll want to escalate - can you provide me with the password or hash prefix?) or there was a connection failure. It would be good to know what happened.

oliversarfas commented 5 years ago

I've managed to replicate this when running tests.

My validation is 'password' => 'required|pwned:250',

Password being provided from faker

user00265 commented 5 years ago

Actually, I have a better question -- you are querying an external source and expecting the return data to always be available... obviously things happen and sometimes data is not available, but your code does not fail gracefully. Is there a reason for this particular design?

valorin commented 5 years ago

Sorry for not replying sooner, I've been travelling for family reasons and haven't had much free time around it. I'll be back home this weekend and will take a deeper look at this!

The lack of graceful fail handling is a stupid oversight, and I really should know better! Troy talks a lot about Pwned Passwords having an insanely high uptime I figured it wasn't needed, but that doesn't count any of the other things that can get in the way to cause a failure. Totally my fault, sorry.

I'll add some error handling this weekend. 😀 (or you can submit a PR if you want)

whoami15 commented 5 years ago

Sorry for not replying sooner, I've been travelling for family reasons and haven't had much free time around it. I'll be back home this weekend and will take a deeper look at this!

The lack of graceful fail handling is a stupid oversight, and I really should know better! Troy talks a lot about Pwned Passwords having an insanely high uptime I figured it wasn't needed, but that doesn't count any of the other things that can get in the way to cause a failure. Totally my fault, sorry.

I'll add some error handling this weekend. 😀 (or you can submit a PR if you want)

Thank you!

user00265 commented 5 years ago

Sorry for not replying sooner, I've been travelling for family reasons and haven't had much free time around it. I'll be back home this weekend and will take a deeper look at this!

The lack of graceful fail handling is a stupid oversight, and I really should know better! Troy talks a lot about Pwned Passwords having an insanely high uptime I figured it wasn't needed, but that doesn't count any of the other things that can get in the way to cause a failure. Totally my fault, sorry.

I'll add some error handling this weekend. 😀 (or you can submit a PR if you want)

To be honest, the API was up during my testing, as I could query it using Postman from my desktop, but the server doesn't receive the proper response from their API or something. I've cloned the repo and I will look into what the issue actually is, and add some error handling as well (separately, of course).

Question is, should it pass the validation or return an error if it cannot receive a valid reply from the API?

valorin commented 5 years ago

Finally got to this...

I've made two changes: 8377583c93584b576e04b3f5883badc3dbe0a5ce & https://github.com/valorin/pwned-validator/commit/6479815ddaeb10c8bd1a7d43cbf21d1723bc3e32

Replacing the code that handles the pwned passwords response with this:

$hashes = explode("\n", trim($results));

return (new Collection($hashes))
    ->mapWithKeys(function ($value) {
        $pair = explode(':', trim($value), 2);

        return count($pair) === 2 && is_numeric($pair[1])
            ? [$pair[0] => $pair[1]]
            : [];
    });

I suspect it could be an extra newline on some pwned passwords responses, possibly... but either way, this should handle it.

It'll fail quietly, which I think is better than blocking a valid password with a weird error.

I'll bump the version in a few days if no one here has any suggestions or thinks more work is needed on this.

valorin commented 5 years ago

I just tagged 1.2.1 - should hit Packagist/Composer shortly, and fix this issue. Let me know if you see any other errors! 🙂