Fix Numeric usernames can start with a space issue

[joomdonation]

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

[purplebeanie]

Tested and this patch works correctly for me.

All good.

[infograf768]

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?

[joomdonation]

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 ?

[infograf768]

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

    public function check()
    {
        // Set user id to null istead of 0, if needed
        if ($this->id === 0)
        {
            $this->id = null;
        }

        // Validate user information
        if (trim($this->name) == '' || trim($this->name, chr(0xC2).chr(0xA0)) == ''
            || trim($this->name, chr(0xE3).chr(0x80).chr(0x80)) == '')
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_PLEASE_ENTER_YOUR_NAME'));

            return false;
        }

        if (trim($this->username) == '' || trim($this->username, chr(0xC2).chr(0xA0)) == ''
            || trim($this->username, chr(0xE3).chr(0x80).chr(0x80)) == '')
        {
            $this->setError(JText::_('JLIB_DATABASE_ERROR_PLEASE_ENTER_A_USER_NAME'));

            return false;
        }

        if (preg_match('#[<>"\'%;()&\\\\]|\\.\\./#', $this->username) || strlen(utf8_decode($this->username)) < 2
            || trim($this->username, chr(0xC2).chr(0xA0)) !== $this->username || trim($this->username) !== $this->username
            || trim($this->username, chr(0xE3).chr(0x80).chr(0x80)) !== $this->username)
        {
            $this->setError(JText::sprintf('JLIB_DATABASE_ERROR_VALID_AZ09', 2));

            return false;
        }

        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　

[infograf768]

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

[infograf768]

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

    /**
     * Alternative to trim
     * Also trims non-breaking spaces and double-byte 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()

[joomdonation]

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.

[infograf768]

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

[brianteeman]

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/

[joomdonation]

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.

[infograf768]

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.

[joomdonation]

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

[infograf768]

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
It will also be very useful elsewhere in core

[infograf768]

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;
        }

[joomdonation]

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

[infograf768]

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

[Bakual]

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') == '')
{
    ...
}

[joomdonation]

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

[infograf768]

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

[joomdonation]

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.

[infograf768]

I have seen both used in core.

[joomdonation]

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

[pe7er]

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

[pe7er]

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!

[infograf768]

Thanks folks!

[joomdonation]

Thanks All :).