-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 Numeric usernames can start with a space issue #4489
Conversation
Tested and this patch works correctly for me. All good. |
This does not work when a non-breaking space or a double byte whitespace is added at the beginning or end of the username. |
Infact, at the moment, we have the code on different places to sanitize, validate the username, so I am not sure where should we put the code:
We need to determine what's the best place to put the extra code. Also, we need to determine if the username contains these invalid characters, we will just sanitize it (just remove it, then continue saving process - like in code #1 above) or if it contains invalid characters, we will just throw error ? Also, what are the allowed characters for username in Joomla ? |
This is the code I would use in the check() method to take care of non-breaking spaces as well as double byte whitespaces:
To test with a double byte space, here are 2 of them, one before and one after the word "test" |
I even wonder if it would not be interesting to define our own JTrim() method to always take care of these... |
We could add the new method in JFilterOutput
Then, instead of trim, we would use when necessary JFilterOutput::trim() |
In Joomla, do we have any specific requirement about username? What are allowed characters? Without a clear requirement, I think it will be difficult for us to write code to validate it. |
Except for this issue of the spaces, the regex is OK |
Yes we have many rules but as J-M says the issue is just the spaces JLIB_DATABASE_ERROR_VALID_AZ09="Please enter a valid username. No space at On 9 October 2014 10:50, infograf768 [email protected] wrote:
Brian Teeman |
Thanks @infograf768 and @brianteeman for information about the spaces:
Not sure what's the best choice here. I think we need input from other developers before working further on it. |
Not sure we have to do in JFilterInput as we want the user to know if an error was made when entering the name, username and mail. |
@infograf768: I see. However, as you see, before the username is validated in check method of JTableUser class, it was sanitized in JFilterInput already (some of special characters if entered are removed and users don't know about this remove). I agree that the normal space can be entered by mistake and users should know / be warned about it. However, I don't think users really want to use "non-breaking spaces as well as double byte whitespaces" in their usernames. So if we sanitize it before storing into database (using JFilterInput), it should be acceptable. It is just my thought, not sure how others think about it. |
I am not sure we use "USERNAME" in JFilterInput::getInstance()->clean() in core
After some discussion with Michael, here is a PR to add in the clean() method a "TRIM" instance |
Code to change in both check() methods in user and table would be
|
@infograf768 We do use filter USERNAME on our core code. Thanks for your PR with TRIM filter added. I think I will update this pull request with your proposed code after your PR #4492 merged? |
Please test PR #4492 |
I like the proposal. I would just do a minor change. Instead of calling
|
@Bakual Agree. That's what I want to do when I update this PR. |
Replace php trim method with JFilterInput TRIM filter to deal with non-breaking and multibyte spaces
OK. Just a minor thing. Should we pass "TRIM" or "trim" to the second parameter of clean method ? They both work, but what's the prefered style? In Joomla core code, it seems when we use a filter, we always pass it in lowercase ("int" instead of "INT", "cmd" instead of "CMD"). I can update the pull request if necessary. |
I have seen both used in core. |
OK. Then I think we can leave it as how it is now. Just need one more tester. |
btw: the patches also solved the "multibreak + username" issue in the front-end registration form. |
Fix Numeric usernames can start with a space issue
Thanks folks! |
Thanks All :). |
This pull request fix the issue #4481 Numeric usernames can start with a space. Basically, the username is validated using this line of code below in JTableUser class:
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/table/user.php#L189
In this if command, the code trim($this->username) != $this->username doesn't work as expected.
In this case, when username only contains space and numeric characters, trim($this->username) != $this->username still return false (it seems because PHP "thinks" that " 123" and "123" are both number, so it is the same and the command return false instead of true as expected).
To fix this issue, we simply change != operator to !== and It works well.
How to test: