Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Fix deprecation warning when comparing a password against a NULL database password #44986

Merged
merged 1 commit into from Nov 17, 2022

Conversation

MaxGiting
Copy link
Contributor

@MaxGiting MaxGiting commented Nov 17, 2022

This used to be dealt with via a strlen(null) check added in this commit
a36f1fe
@GrahamCampbell I'd appreciate if you could cast your eye over this change.

strlen(null) used to return 0 as shown in this PHP docs comment, however since PHP 8.0, passing null to strlen() has been deprecated.

I think this check has always been a null check as passing an empty string as the second parameter to password_verify() has always been permitted and throws no level of warning or error.

To be on the safe side I have also added tests for when $hashedValue is passed in as an empty string.

We're are seeing a lot of deprecation notices as in our system it is possible for users to have a NULL password in the database until they have activated their account and setup a password.

@driesvints
Copy link
Member

A password in the database should never be null. Just generate passwords for all users.

@JayBizzle
Copy link
Contributor

A password in the database should never be null. Just generate passwords for all users.

In that case, should passwords in the database ever be empty strings?

@JayBizzle
Copy link
Contributor

JayBizzle commented Nov 17, 2022

We have a system that allows users to invite other users. When the user is invited, an account is created with a NULL password, and the user creates their own password when they accept the invitation.

If passwords in the database should never be NULL, surely they should never be empty strings, but there is an existing check to make sure the password in the database is not an empty string?

@driesvints
Copy link
Member

By default, a password is not nullable in Laravel:

https://github.com/laravel/laravel/blob/9.x/database/migrations/2014_10_12_000000_create_users_table.php#L21

I can't speak for empty strings.

@MaxGiting
Copy link
Contributor Author

I believe this commit a36f1fe was added to fix errors if null was passed in as the hashed password.
The commit message was "Fixed errors for empty password hashes". No errors would have been thrown from passing an empty string in as the second parameter to password_verify() whereas passing null does.

Would be good to hear from @GrahamCampbell to know if that was the case.

@taylorotwell taylorotwell merged commit 939302c into laravel:9.x Nov 17, 2022
@taylorotwell
Copy link
Member

I don't understand why your tests were using assertTrue. Should they not be assertFalse? I have changed them.

@MaxGiting
Copy link
Contributor Author

MaxGiting commented Nov 17, 2022

Yes they should have been assertFalse

@taylorotwell
Copy link
Member

Thanks - just checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants