magniloquent icon indicating copy to clipboard operation
magniloquent copied to clipboard

autoHash - Doesn't Hash Password When Set To Same Plaintext

Open jimstump opened this issue 11 years ago • 1 comments

I noticed an issue where autoHash didn't appear to be hashing a User's password and it ended up being stored in plaintext in my database. I did some investigation and determined that the user tried to change their password to the same plaintext string. Here's a simple test case:

$user = User::find(1);
// This user currently has a different password, that is currently hashed in the database
var_dump($user->password);

// Then we change the password
$user->password = '12345';
$user->save();

// The password has been hashed and saved successfully
var_dump($user->password);
var_dump($user->errors());

// We then try to change the password again, but to the same plaintext string
$user->password = '12345';
$user->save();

// This time, the password is not autohashed and is stored in plaintext
// There are still no errors however
var_dump($user->password);
var_dump($user->errors());

This results in output something like:

string '$2y$10$txasGsFjds0Er/MXzwgi/ue0/FO02txUFMpxm9eLILJveYLYpPsD.' (length=60)
string '$2y$10$zSc9EKVoqbtnlcsCujdkfO8fFs3IJonRU5saUBbD5.2C9WdPiTteK' (length=60)
object(Illuminate\Support\MessageBag)[137]
  protected 'messages' => 
    array (size=0)
      empty
  protected 'format' => string ':message' (length=8)
string '12345' (length=5)
object(Illuminate\Support\MessageBag)[137]
  protected 'messages' => 
    array (size=0)
      empty
  protected 'format' => string ':message' (length=8)

I think this is due to having a combination of the isDirty check along with the Hash::check that was introduced with pull request #40. Wouldn't it be better to always hash the password if it is considered "dirty"? Is there another scenario or use case I'm missing?

For a work around right now, I've added a Hash::check in my controller that prevents the user from changing their password to the same string to prevent this behavior.

jimstump avatar Aug 10 '14 19:08 jimstump

I'm upgrading because of this bug. ( I wanted to upgrade anyways)

carcinocron avatar Jan 28 '15 16:01 carcinocron