319953 - Missing real email syntax check

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Frédéric Buclin]

Actually, we don't check that login_name + emailsuffix = valid email syntax!

Util::validate_email_syntax() only validates login names and completely ignores 'emailsuffix' meaning that we could result in invalid email addresses, see also bug 278414 comment 2.

We should rename the actual Util::validate_email_syntax() into Util::validate_login() and add a real Util::validate_email_syntax() which makes sure that appending the emailsuffix results in a real and well formed email address (with only one '@', one dot after the '@', etc...).

Comment 1

[Marc Schumann [:Wurblzap]]

Right.
The definition of "well formed email address" should be left to a regexp, though. Who knows whether people use their Bugzilla with flat Linux logins, for example.

Comment 2

[Frédéric Buclin]

For the record: $Email::Address::addr_spec returns a regexp to validate email addresses, per RFC 2822. This would be better than our self-made regexp.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

Comment 4

[Frédéric Buclin]

Attachment: patch, v1.1

I added a test in 007util.t to make sure we correctly catch invalid email addresses. I also fixed validate_email_syntax() to catch non-ASCII characters, which are illegal in email addresses, and check_email_syntax() which wasn't returning an untaint email address on success.

Comment 5

[:glob ✱]

Comment on attachment 585381 [details] [diff] [review]
patch, v1.1

Review of attachment 585381 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/admin/params/auth.html.tmpl
@@ +113,5 @@
> +    "<a href='http://tools.ietf.org/html/rfc2822#section-3.4.1'>RFC 2822</a>. " _
> +    "As RFC 2822 can be a bit too permissive, the default tries to match " _
> +    "fully qualified email addresses in a slightly more restrictive way. " _
> +    "Another popular value to put here is <tt>^[^@]+$</tt>, which means " _
> +    "'local usernames, no @ allowed.'",

remove the sentence "as rfc 2822 can be a bit too permissive, the default tries to match fully qualified email addresses in a slightly more restrictive way."  it doesn't add any real value here; if you want to make a note of it, please do so in the documentation.

::: template/en/default/global/code-error.html.tmpl
@@ +46,4 @@
>        [%+ Param('emailregexpdesc') FILTER html_light %]
>      [% END %]
>      It must also not contain any of these special characters:
> +    <tt>\ ( ) &lt; &gt; , ; : &quot; [ ]</tt>, or any whitespace.

my concerns about the error message on bug 714472 are echoed here.

::: template/en/default/global/user-error.html.tmpl
@@ +875,5 @@
>        [%+ Param('emailregexpdesc') FILTER html_light %]
>      [% END %]
>      It must also not contain any of these special characters:
> +    <tt>\ ( ) &lt; &gt; , ; : &quot; [ ]</tt>, or any whitespace.
> +

.. and here.

::: t/007util.t
@@ +77,5 @@
> +my $params = Bugzilla->params;
> +$params->{emailregexp} = '.*';
> +$params->{emailsuffix} = '';
> +my $ascii_email = '[e-mail]';
> +my $utf8_email  = 'а[e-mail]';

i don't see any utf8 characters in $utf8_email.

Comment 6

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #5)
> remove the sentence "as rfc 2822 can be a bit too permissive, the default
> tries to match fully qualified email addresses in a slightly more
> restrictive way."  it doesn't add any real value here

If we don't add this sentence, how do you justify that the default is not .*? I think this sentence makes sense to admins, unless you can come with a better justification.


> my concerns about the error message on bug 714472 are echoed here.

If we follow what rjbs suggested, we would drop most of the error message, which could be even more confusing.


> i don't see any utf8 characters in $utf8_email.

That's the goal! :) The UTF8 one looks exactly the same as the non-UTF8 one, but the "a" is not a real "a" in the UTF8 one.

Comment 7

[:glob ✱]

(In reply to Frédéric Buclin from comment #6)
> > remove the sentence "as rfc 2822 can be a bit too permissive, the default
> > tries to match fully qualified email addresses in a slightly more
> > restrictive way."  it doesn't add any real value here
> 
> If we don't add this sentence, how do you justify that the default is not
> .*? I think this sentence makes sense to admins, unless you can come with a
> better justification.

i don't argue that it doesn't make sense, just that it's overly verbose, and doesn't add enough value to bloat the always-visible description.

> > my concerns about the error message on bug 714472 are echoed here.
> 
> If we follow what rjbs suggested, we would drop most of the error message,
> which could be even more confusing.

i disagree.  if i'm creating a user as an administrator, and i copy and paste a name which happens to have unicode characters, the error message you're proposing appears to list all conditions the validation check failed, except in this case it doesn't contain the actual reason!  we either need to list _all_ reasons, or return an message such as rjbs' suggestion.

> > i don't see any utf8 characters in $utf8_email.
> 
> That's the goal! :) The UTF8 one looks exactly the same as the non-UTF8 one,
> but the "a" is not a real "a" in the UTF8 one.

this needs to be much clearer :)  i suggest:

my $utf8_email = "\N{U+430}dmin\@example.com";

Comment 8

[Frédéric Buclin]

Attachment: patch, v2

I addressed all your comments.

Comment 9

[:glob ✱]

Comment on attachment 590577 [details] [diff] [review]
patch, v2

Review of attachment 590577 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

please fix the error message on commit.

::: template/en/default/global/code-error.html.tmpl
@@ +36,4 @@
>      [% ELSE %]
>        [%+ Param('emailregexpdesc') FILTER html_light %]
>      [% END %]
> +    It also must not contain any illegal character.

"It also must not contain any illegal characters."

::: template/en/default/global/user-error.html.tmpl
@@ +864,4 @@
>      [% ELSE %]
>        [%+ Param('emailregexpdesc') FILTER html_light %]
>      [% END %]
> +    It also must not contain any illegal character.

"It also must not contain any illegal characters."

Comment 10

[Frédéric Buclin]

Thanks :)

Comment 11

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified token.cgi
modified userprefs.cgi
modified Bugzilla/FlagType.pm
modified Bugzilla/User.pm
modified Bugzilla/Util.pm
modified docs/en/xml/administration.xml
modified t/007util.t
modified template/en/default/admin/params/auth.html.tmpl
modified template/en/default/global/code-error.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 8087.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 590577 [details] [diff] [review]
patch, v2

Review of attachment 590577 [details] [diff] [review]:
-----------------------------------------------------------------

Just wanted to say that I like this patch. :-)

::: t/007util.t
@@ +60,5 @@
>  
> +# validate_email_syntax. We need to override some parameters.
> +my $params = Bugzilla->params;
> +$params->{emailregexp} = '.*';
> +$params->{emailsuffix} = '';

Nit: As a note for the future, it would be nice to put this into a block and use "local" here.

Comment 13

[Bill Barry]

This patch breaks my company's bugzilla instance (sanity check and unable to create a user I am asked to add).

We use several invalid users as holding grounds for bugs by assigning them to users like "review". All real users have a full email address in their login name field and we cannot use the emailsuffix parameter because they aren't all the same domain. My boss likes that the holding users are not email addresses because they show up differently in outlook (as black text instead of underlined mailto links). 

Does the email regex parameter mean anything now?

Comment 14

[Frédéric Buclin]

(In reply to Bill Barry from comment #13)
> Does the email regex parameter mean anything now?

It does. It lets you restrict even more the kind of email addresses you accept in your installation. But it doesn't let you relax the email syntax defined in RFC 2822.

Comment 16

[Matt McHenry]

I agree with Bill: this should've been included in the release notes.

Our installation is configured to allow either 'jdoe' or '[e-mail]' as a user ID: the MTA is configured to auto-append the '@company.com' if it's missing.  External users ([e-mail]) also work, meaning emailsuffix is empty.

Further, user creation is restricted to IDs w/o an '@' in them -- that way, internal users can create their own logins, but external users have to have an admin create their account for them.

If I upgrade our install to 4.4, I can change createemailregexp to allow only '@company.com' users to create accounts, and we can live with the longer login IDs for internal users.  But, will all existing users have to change their login IDs?  The limited testing I've done suggests not (I was able to log in & add to CC list), but I want to confirm that before I unleash this upgrade.

Another way of phrasing the question, I think, is: are there any calls to validate_email_syntax for existing users, or does that only happen when creating new users?

Comment 17

[Matt McHenry]

I've tested a user w/o a domain in his login ID in the following cases: assignee, reporter, cc list member, QA contact, commenter.  All appear to work fine, so I think I'll go ahead with the upgrade.  I'll report back here if any problems arise.