Skip to content

[9.x] Improve password checks - #42248

Merged
taylorotwell merged 4 commits into
laravel:9.xfrom
korkoshko:impove-password-checks
May 4, 2022
Merged

[9.x] Improve password checks#42248
taylorotwell merged 4 commits into
laravel:9.xfrom
korkoshko:impove-password-checks

Conversation

@korkoshko

Copy link
Copy Markdown
Contributor

It also fixes a possible collision when credentials contains only "password" values.

$provider->retrieveByCredentials(['password' => 'dayle', 'password2' => 'night']);

As a result of the code above, the first record will be retrieved from the table in the database.

@driesvints

driesvints commented May 4, 2022

Copy link
Copy Markdown
Member

This a major breaking change and security risk. You're trying to retrieve users by their unhashed passwords, something that should be avoided at all costs. Please hash your passwords.

@korkoshko

Copy link
Copy Markdown
Contributor Author

@driesvints Sorry, maybe I didn't express myself very well. After my changes to the retrieveByCredentials method all password values are excluded from credentials array. This method calling by method "attempt" in SessionGuard, which is used in the Auth facade.

@korkoshko

korkoshko commented May 4, 2022

Copy link
Copy Markdown
Contributor Author

@driesvints Now there is a bug when only two or more keys with the substring "password" can be passed to the "attempt" method as input:

Auth::attempt(['password' => 'dayle', 'password2' => 'night']);

As a result of this "bug", the first record (without filters) will be retrieved from the table, and this is not good : )

@korkoshko

Copy link
Copy Markdown
Contributor Author

@driesvints just look L108 and then L120 to understand the bug above.

@taylorotwell
taylorotwell merged commit 8542414 into laravel:9.x May 4, 2022
@driesvints

Copy link
Copy Markdown
Member

@korkoshko I finally see what you meant now. Sorry, misjudged this. Thanks for your PR 👍

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.

3 participants