[5.7] Hash::check breaks all apps with legacy hash

[kamui545]

>>> Hash::check('s3cr3t', '$1$0590adc6$WVAjBIam8sJCgDieJGLey0');
RuntimeException with message 'This password does not use the Bcrypt algorithm.'

Use case: We have applications with sha1 AND bcrypt passwords.
On successful login, an event is triggered to update the hash stored in DB to the stronger hash algorithm.

Since the 5.7 update, all users with legacy passwords are not able to login anymore.

Please, reconsider this check in the Hash::check method or make it optional, it should be consistent with how password_verify() works.

Password hashing is not backward compatible nor future proof anymore...

Breaking changes introduced in: #24162 / #24178
Related issues: https://github.com/laravel/docs/issues/4509, #25468, #25458

[kamui545]

In the meantime, for those stuck and who still would like to upgrade, here's how:

Create app/Hashing/BcryptHasher.php:

<?php

namespace App\Hashing;

use Illuminate\Hashing\BcryptHasher as BaseHasher;

class BcryptHasher extends BaseHasher
{
    /**
     * Check the given plain value against a hash.
     *
     * @param  string  $value
     * @param  string  $hashedValue
     * @param  array  $options
     * @return bool
     */
    public function check($value, $hashedValue, array $options = [])
    {
        if (strlen($hashedValue) === 0) {
            return false;
        }

        return password_verify($value, $hashedValue);
    }
}

Then in your AppServiceProvider add:

app('hash')->extend('legacy-bcrypt', function () {
    return new BcryptHasher($this->app['config']['hashing.bcrypt'] ?? []);
});

And set 'driver' => 'legacy-bcrypt' in config/hashing.php

[crynobone]

I wonder why do we need to use Hash::check() for authentication verification if at the end we plan to support multiple hashing algorithm; bcrypt and argon1.

Shouldn't we respect whatever hashing used in the password as long as password_verify() can verify the password?

[crynobone]

This has been fixed in 5.7.4

[laurencei]

Thanks @crynobone !

Closing as this issue has been resolved.

[mnabialek]

@crynobone See me comment in #25677 why we should still use Hash::check() if we don't want to break existing apps that uses custom hasher.

[kamui545]

Can we re-open this now that the partial fix has been reverted?

[robinvdvleuten]

This also breaks your app if the user does not have a password set. This wasn't the case before Laravel 5.7.

[donovanhare]

#25468 has been merged, we await a new release...

[laurencei]

Closing as this issue has been resolved (again).