[11.x] handle password_hash() failures better
#53821
Conversation
as of PHP 8.0.0, `password_hash()` no longer returns false on failure, instead a `ValueError` will be thrown if the password hashing algorithm is not valid, or an `Error` if the password hashing failed for an unknown error. I noticed the `ArgonHasher` already had a slightly updated check using error suppression, which was added in laravel#33856. however, I couldn't seem to get that to actually work, and for consistency I updated them both to use try/catch. added tests for all 3 implementations to see if they correctly throw a `RuntimeException` if they are not able to be created. I used invalid "cost" and "time" options to trigger this. not sure if there's a better way. https://www.php.net/manual/en/function.password-hash.php#:~:text=the%20salt%20generation.-,8.0.0,-password_hash()%20no
| 'time_cost' => $this->time($options), | ||
| 'threads' => $this->threads($options), | ||
| ]); | ||
| } catch (Error) { |
There was a problem hiding this comment.
Might it be helpful to add this caught Error as the previous parameter of the RuntimeException?
There was a problem hiding this comment.
yah, I was considering that. is there any security implication though, because it could expose info like which algo is being used?
There was a problem hiding this comment.
Isn't it already showing the hashing algo in the exception message?
In theory, these exceptions should not be passed along to the end-user when the app is set to production mode.
(I don't have a strong opinion on it either way, just a thought as I have fallen into the practice of forwarding exceptions when throwing a new one)
There was a problem hiding this comment.
It also catches a generic Error object, which could be caused by any number of things. At least this way you can see what specifically happened.
There was a problem hiding this comment.
it could also expose other info, like cost factor, etc.
you're right, it should never show to users in production.
I'd say give a PR a shot, and see what other people think.
as of PHP 8.0.0,
password_hash()no longer returns false on failure, instead aValueErrorwill be thrown if the password hashing algorithm is not valid, or anErrorif the password hashing failed for an unknown error.I noticed the
ArgonHasheralready had a slightly updated check using error suppression, which was added in #33856. however, I couldn't seem to get that to actually work, and for consistency I updated them both to use try/catch.added tests for all 3 implementations to see if they correctly throw a
RuntimeExceptionif they are not able to be created. I used invalid "cost" and "time" options to trigger this. not sure if there's a better way.https://www.php.net/manual/en/function.password-hash.php#:~:text=the%20salt%20generation.-,8.0.0,-password_hash()%20no