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

fix: remove username in $validFields by default #498

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 1, 2022

The default configuration does not support username as login id.
There is no form input for username, and no validation rules for it.

But Shield accepts username in the login form (attackers can send any field),
and sends it to the database without validation.

This is not an exploitable vulnerability, but it is undesirable. Unvalidated user input should not be sent to the database.

Screenshot 2022-11-01 17 28 07

The default configration does not validate username.
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems logical. Please wait for a review from @datamweb

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kenjis I already saw someone looking for a method to support both username and email for login.
By default, Shield accepts only one field for login.
But my idea was that with changes in the front end, for example, with the help of Vue.js, the user can identify the input whether it is an email or a username.
Of course, this requires changes in the default validations rules (required_without / required_with and permit_empty.

I have not done this in practice.

Now my question is, did I think right or not?

If it is true, doesn't this PR create a limitation?

@kenjis
Copy link
Member Author

kenjis commented Nov 1, 2022

Now my question is, did I think right or not?

You think right. If you configure correctly, you can send either email or username as a login id.

If it is true, doesn't this PR create a limitation?

No, this PR just changes the default configuration.
If you set $validFields and proper validation rules, you can use email or username.

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I did not see any problem in the registration process.

And login with username is Ok.
(Of course, it should be as follows)

    public array $validFields = [
        'email',
        'username',
    ];
Data of Table "auth_logins":

+----+------------+--------------------+----------+------------+---------+--------------------+---------+
| id | ip_address | user_agent         | id_type  | identifier | user_id | date               | success |
+----+------------+--------------------+----------+------------+---------+--------------------+---------+
| 26 | ::1        | Mozilla/5.0 (Wi... | username | Pooya      | 3       | 2022-11-01 17:1... | 1       |
+----+------------+--------------------+----------+------------+---------+--------------------+---------+

@kenjis kenjis marked this pull request as draft November 2, 2022 01:36
@kenjis kenjis marked this pull request as ready for review November 2, 2022 01:40
@kenjis kenjis added the bug Something isn't working label Nov 2, 2022
@kenjis kenjis merged commit 9d0b0c9 into codeigniter4:develop Nov 2, 2022
@kenjis kenjis deleted the fix-default-validFields branch November 2, 2022 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants