rcmail.php contains (famous) strpos mistakes

[Eddict]

the current code in rcmail.php is incorrect at 3 lines if $username contains @ character at 1st position (user [@domain.com], without the brackets, is often used as a 'catch-all account'). with the current code $username [@domain.com] is modified into [@domain.com\@domain.com] (without the brackets and the backslash. i need to add the extra characters, because email addresses are not allowed).

the PHP strpos function is often used wrong, because 'This function may return Boolean FALSE, but may also return a non-Boolean value which evaluates to FALSE' (see ​PHP Manual)

in the first line the !strpos() should be replaced by strpos() === false

in two other lines the if (strpos()) should be replaced by if (strpos() !== false)

    // Check if we need to add domain
    if (!empty($config['username_domain']) && !strpos($username, '@')) {
      if (is_array($config['username_domain']) && isset($config['username_domain'][$host]))
        $username .= '@'.$config['username_domain'][$host];
      else if (is_string($config['username_domain']))
        $username .= '@'.$config['username_domain'];
    }
    // try to resolve email address from virtuser table
    if (strpos($username, '@'))
      if ($virtuser = rcube_user::email2user($username))
        $username = $virtuser;
    // lowercase username if it's an e-mail address (#1484473)
    if (strpos($username, '@'))
      $username = mb_strtolower($username);

should be

    // Check if we need to add domain
    if (!empty($config['username_domain']) && (strpos($username, '@') === false)) {
      if (is_array($config['username_domain']) && isset($config['username_domain'][$host]))
        $username .= '@'.$config['username_domain'][$host];
      else if (is_string($config['username_domain']))
        $username .= '@'.$config['username_domain'];
    }
    // try to resolve email address from virtuser table
    if (strpos($username, '@') !== false)
      if ($virtuser = rcube_user::email2user($username))
        $username = $virtuser;
    // lowercase username if it's an e-mail address (#1484473)
    if (strpos($username, '@') !== false)
      $username = mb_strtolower($username);

[alec]

This is not a mistake. What server allows catch-all accounts creation? I thought that catch-all is always an alias for the real account. Login using alias name shouldn't be allowed.

[Eddict]

hi alec,

maybe you're right, maybe not, i don't want to start a discussion about the validity/behaviour of such an account. for more info see ​Wikipedia.

the way the strpos function used in rcmail.php is still wrong.
it wants to look for the presence/absence of a @ character in $username and it gives an invalid result ("not present") on those 3 lines if the @ character is the 1st character in $username.

regards Eddict

[alec]

It's not wrong if we want to check presence of @ or check if it's on the first position. ;)

[Eddict]

it's only not wrong if you want to check the presence of @ on any position, except the 1st one. well... sounds a little bit strange to me.

there are plenty of articles available which warn about this common mistake. (no explicit type checking)
​one example

[Eddict]

some webhosters, including mine !, use the [@domain.com] account as the catch-all account.
so it is still a valid accountname although it's an invalid email address.

but it's never used as an emailaddress, because it's a catch-all account...

[toddtrann]

In my reading of RFC5322, [@domain.com] is not a valid e-mail address, therefore I think the current use of strpos() is OK as it is. I think this ticket should be closed.

But here is also some interesting discussion on the same topic:
​http://blog.dominicsayers.com/2009/02/23/confusedrfc2822com/
Where he says that [""@domain.com] is valid. Still does not contradict the above.

[Eddict]

Replying to toddtrann:

In my reading of RFC5322, [@domain.com] is not a valid e-mail address, therefore I think the current use of strpos() is OK as it is. I think this ticket should be closed.

But here is also some interesting discussion on the same topic:
​http://blog.dominicsayers.com/2009/02/23/confusedrfc2822com/
Where he says that [""@domain.com] is valid. Still does not contradict the above.

despite if it's valid or not valid,(again);
the way the strpos function used in rcmail.php is still wrong. it wants to look for the presence/absence of a @ character in $username and it gives an invalid result ("not present") on those 3 lines if the @ character is the 1st character in $username.

in other scripts within roundcube it's used correctly, so why not solve this issue and also get rid of the different methods. kill two birds with one stone..

regards Eddict

[alec]

It is not wrong!!!!! We know what we're doing. Question is, should we allow "@domain.tld" as username or not?

[alec]

... and if an answer is "yes", we should check that maybe we need some more changes in other places.

[alec]

Fixed in [c16fab16]. Only first strpos() was used wrong.