codeigniter4 / shield

Authentication and Authorization for CodeIgniter 4
https://shield.codeigniter.com
MIT License
366 stars 133 forks source link

Bug: NothingPersonalValidator strip_explode too sensible #385

Closed tomatlscomm closed 2 years ago

tomatlscomm commented 2 years ago

PHP Version

7.4

CodeIgniter4 Version

4.2.4

Shield Version

1.0.0-beta.2

Which operating systems have you tested for this bug?

macOS

Which server did you use?

apache

Database

MySQL 5.6

Did you customize Shield?

No

What happened?

The function NothingPersonalValidator->strip_explode() is too sensible for exploding the user password. Indeed, it explode any part separated for example with an underscore or a "-". This can produce the search of only one letter in the username and/or email adresse as part of a personal information. The user is then prompted wrongly that his password cannot contain any personal information contained is his username or email.

Steps to Reproduce

Try to register with : email : xxx@gmail.com password : G-test#1234

In this case, NothingPersonalValidator will fail cause "G" is considered as part of "xxx@gmail.com"

Expected Output

I think the NothingPersonalValidator->strip_explode() function should check if the searched string is not too short and then try to make a match only if the searching string contain 3 or more caracters.

Anything else?

No response

datamweb commented 2 years ago

Hello @tomatlscomm , Thank you very much for your report. I confirm this issue.

Screenshot 2022-08-16 180439

tomatlscomm commented 2 years ago

What i suggest to fix :

https://github.com/codeigniter4/shield/blob/d632fa1e18e96cb28f6a80000346b85b4aa54836/src/Authentication/Passwords/NothingPersonalValidator.php#L115-L119

Replace

if (empty($haystack) || in_array($haystack, $trivial, true)) {

by

if (empty($haystack) || in_array($haystack, $trivial, true) || strlen($haystack)<3) {
MGatner commented 2 years ago

I think that is a fine solution @tomatlscomm. We will want to make sure it works with existing tests and has its own new test. Would you be willing to open a Pull Request?

tomatlscomm commented 2 years ago

@MGatner Sure