? Success

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
9 Oct 2014

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:

  1. Try to register for an account using "Numeric usernames start with a space", for example " 123" (space + 123). You will see that Joomla still allows you to register using that username (unexpected behavior)

  2. Apply this pull request

  3. Try to register again. You will see that the username like that will be rejected

avatar joomdonation joomdonation - open - 9 Oct 2014
avatar jissues-bot jissues-bot - change - 9 Oct 2014
Labels Added: ?
avatar purplebeanie
purplebeanie - comment - 9 Oct 2014

Tested and this patch works correctly for me.

All good.

avatar purplebeanie purplebeanie - test_item - 9 Oct 2014 - Tested successfully
avatar infograf768
infograf768 - comment - 9 Oct 2014

This does not work when a non-breaking space or a double byte whitespace is added at the beginning or end of the username.
It is not a new issue as it is also present when the username is not a figure.
Shall we update the check() method to take care of these cases?

avatar joomdonation
joomdonation - comment - 9 Oct 2014

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:

  1. First, the username is sanitized https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/filter/input.php#L260 (some invalid characters are sanitized/removed from this line of code)

  2. Then it is validated in this line of code https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/form/rule/username.php#L36

  3. Finally, it is validated again in JTableUser class https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/table/user.php#L189

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 ?

avatar infograf768
infograf768 - comment - 9 Oct 2014

This is the code I would use in the check() method to take care of non-breaking spaces as well as double byte whitespaces:


        if ((trim($this->email) == "") || !JMailHelper::isEmailAddress($this->email)
            || trim($this->email, chr(0xC2).chr(0xA0)) == ''
            || trim($this->email, chr(0xE3).chr(0x80).chr(0x80)) == '')
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_VALID_MAIL'));

            return false;
        }

To test with a double byte space, here are 2 of them, one before and one after the word "test"
 test 

avatar infograf768
infograf768 - comment - 9 Oct 2014

I even wonder if it would not be interesting to define our own JTrim() method to always take care of these...

avatar infograf768
infograf768 - comment - 9 Oct 2014

We could add the new method in JFilterOutput
we would have something like:

    /**
     * Alternative to trim
     * Also trims non-breaking spaces and double-bytes whitespaces
     *
     * @param   string  $str  String to be processed
     *
     * @return  string  Trimmed string
     *
     * @see     http://php.net/manual/en/function.trim.php
     * @since   3.4.0
     */
    public static function trim($string)
    {
        $str = trim($string);
        $str = trim($str, chr(0xE3).chr(0x80).chr(0x80));
        $str = trim($str, chr(0xC2).chr(0xA0));

        return $str;
    }

Then, instead of trim, we would use when necessary JFilterOutput::trim()

avatar joomdonation
joomdonation - comment - 9 Oct 2014

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.

avatar infograf768
infograf768 - comment - 9 Oct 2014

Except for this issue of the spaces, the regex is OK

avatar brianteeman
brianteeman - comment - 9 Oct 2014

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
beginning or end, at least %d characters and must not
contain the following characters: < > \ "QQ" ' % ; ( ) &"

On 9 October 2014 10:50, infograf768 notifications@github.com wrote:

Except for this issue of the spaces, the regex is OK


Reply to this email directly or view it on GitHub
#4489 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar joomdonation
joomdonation - comment - 9 Oct 2014

Thanks @infograf768 and @brianteeman for information about the spaces:

  1. If we just want to solve the issue with spaces in username, I think we can simply add code to JFilterInput class https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/filter/input.php#L260 to remove these space characters from username

  2. If we want to re-use that code to validate name and email as well, maybe use @infograf768 propose will be good.

Not sure what's the best choice here. I think we need input from other developers before working further on it.

avatar infograf768
infograf768 - comment - 9 Oct 2014

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.

avatar joomdonation
joomdonation - comment - 9 Oct 2014

@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.

avatar infograf768
infograf768 - comment - 9 Oct 2014

I am not sure we use "USERNAME" in JFilterInput::getInstance()->clean() in core
I read there

     *                           USERNAME:  Do not use (use an application specific filter),

After some discussion with Michael, here is a PR to add in the clean() method a "TRIM" instance
#4492
Then we can use the new code instead of trim in Table class as well as in user

avatar infograf768
infograf768 - comment - 9 Oct 2014

Code to change in both check() methods in user and table would be

// Validate user information
        if (JFilterInput::getInstance()->clean((string) $this->name, 'trim') == '')
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_PLEASE_ENTER_YOUR_NAME'));

            return false;
        }

        if (JFilterInput::getInstance()->clean((string) $this->username, 'trim') == '')
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_PLEASE_ENTER_A_USER_NAME'));

            return false;
        }

        if (preg_match('#[<>"\'%;()&\\\\]|\\.\\./#', $this->username) || strlen(utf8_decode($this->username)) < 2
            || JFilterInput::getInstance()->clean((string) $this->username, 'trim') !== $this->username)
        {
            $this->setError(JText::sprintf('JLIB_DATABASE_ERROR_VALID_AZ09', 2));

            return false;
        }

        if ((JFilterInput::getInstance()->clean((string) $this->email, 'trim') == "") || !JMailHelper::isEmailAddress($this->email))
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_VALID_MAIL'));

            return false;
        }
avatar joomdonation
joomdonation - comment - 9 Oct 2014

@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?

avatar infograf768
infograf768 - comment - 9 Oct 2014

Please test PR #4492
We need 2 tests to get it in

avatar Bakual
Bakual - comment - 9 Oct 2014

I like the proposal. I would just do a minor change. Instead of calling JFilterInput::getInstance()->clean((string) $this->..., 'trim') multiple times you can store the instance in a variable and use it from there. Like this:

$jfilter = JFilterInput::getInstance();

if ($jfilter->clean((string) $this->name, 'trim') == '')
{
    ...
}
avatar joomdonation
joomdonation - comment - 9 Oct 2014

@Bakual Agree. That's what I want to do when I update this PR.

avatar JerryChr JerryChr - test_item - 9 Oct 2014 - Tested successfully
avatar infograf768
infograf768 - comment - 10 Oct 2014

#4492 is now merged. You can use it. :)

avatar infograf768
infograf768 - comment - 10 Oct 2014

@test
#4492 is now merged.
This PR is now fine.
One more tester

avatar joomdonation
joomdonation - comment - 10 Oct 2014

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.

avatar infograf768
infograf768 - comment - 10 Oct 2014

I have seen both used in core.

avatar joomdonation
joomdonation - comment - 10 Oct 2014

OK. Then I think we can leave it as how it is now. Just need one more tester.

avatar pe7er
pe7er - comment - 10 Oct 2014

@test
I have just added patch #4492 + #4489
It solved the "space + numerical username" & "non-breakable space + username" issue.

avatar pe7er
pe7er - comment - 10 Oct 2014

btw: the patches also solved the "multibreak + username" issue in the front-end registration form.
The back-end edit form in User Manager does not allow multibreak/non-breakable space/space anymore. Nice!

avatar infograf768 infograf768 - close - 10 Oct 2014
avatar infograf768 infograf768 - change - 10 Oct 2014
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2014-10-10 07:32:50
avatar infograf768
infograf768 - comment - 10 Oct 2014

Thanks folks!

avatar joomdonation
joomdonation - comment - 10 Oct 2014

Thanks All :).

Add a Comment

Login with GitHub to post a comment