Bug 319953: Missing real email syntax check

r=glob a=LpSolit

diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index ddaa6eb6296ba5dc1f450e9f72a587bd221f0749..b4709212e303419c1b825c95f2b040b34c550fc8 100644 (file)
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -38,6 +38,8 @@ use Bugzilla::Error;
 use Bugzilla::Util;
 use Bugzilla::Group;
 
+use Email::Address;
+
 use base qw(Bugzilla::Object);
 
 ###############################
@@ -287,15 +289,11 @@ sub _check_cc_list {
       || ThrowUserError('flag_type_cc_list_invalid', { cc_list => $cc_list });
 
     my @addresses = split(/[,\s]+/, $cc_list);
-    # We do not call Util::validate_email_syntax because these
-    # addresses do not require to match 'emailregexp' and do not
-    # depend on 'emailsuffix'. So we limit ourselves to a simple
-    # sanity check:
-    # - match the syntax of a fully qualified email address;
-    # - do not contain any illegal character.
+    my $addr_spec = $Email::Address::addr_spec;
+    # We do not call check_email_syntax() because these addresses do not
+    # require to match 'emailregexp' and do not depend on 'emailsuffix'.
     foreach my $address (@addresses) {
-        ($address =~ /^[\w\.\+\-=]+@[\w\.\-]+\.[\w\-]+$/
-           && $address !~ /[\\\(\)<>&,;:"\[\] \t\r\n]/)
+        ($address !~ /\P{ASCII}/ && $address =~ /^$addr_spec$/)
           || ThrowUserError('illegal_email_address',
                             {addr => $address, default => 1});
     }

diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 373fc1ef3a5c72f84ed82c809c39c03bd2872696..23b08c63a1d42a7df29248887e14cba91707130f 100644 (file)
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -195,8 +195,7 @@ sub check_login_name_for_creation {
     my ($invocant, $name) = @_;
     $name = trim($name);
     $name || ThrowUserError('user_login_required');
-    validate_email_syntax($name)
-        || ThrowUserError('illegal_email_address', { addr => $name });
+    check_email_syntax($name);
 
     # Check the name if it's a new user, or if we're changing the name.
     if (!ref($invocant) || $invocant->login ne $name) {

diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index a04095647a6accdd7069c034299d3c64e031d2d8..bf8569839c91a9ca4625e2d57bae39e64ce144ef 100644 (file)
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -20,7 +20,7 @@ use base qw(Exporter);
                              format_time validate_date validate_time datetime_from
                              file_mod_time is_7bit_clean
                              bz_crypt generate_random_password
-                             validate_email_syntax clean_text
+                             validate_email_syntax check_email_syntax clean_text
                              get_text template_var disable_utf8
                              detect_encoding);
 
@@ -552,7 +552,13 @@ sub generate_random_password {
 sub validate_email_syntax {
     my ($addr) = @_;
     my $match = Bugzilla->params->{'emailregexp'};
-    my $ret = ($addr =~ /$match/ && $addr !~ /[\\\(\)<>&,;:"\[\] \t\r\n]/);
+    my $email = $addr . Bugzilla->params->{'emailsuffix'};
+    # This regexp follows RFC 2822 section 3.4.1.
+    my $addr_spec = $Email::Address::addr_spec;
+    # RFC 2822 section 2.1 specifies that email addresses must
+    # be made of US-ASCII characters only.
+    # Email::Address::addr_spec doesn't enforce this.
+    my $ret = ($addr =~ /$match/ && $email !~ /\P{ASCII}/ && $email =~ /^$addr_spec$/);
     if ($ret) {
         # We assume these checks to suffice to consider the address untainted.
         trick_taint($_[0]);
@@ -560,6 +566,15 @@ sub validate_email_syntax {
     return $ret ? 1 : 0;
 }
 
+sub check_email_syntax {
+    my ($addr) = @_;
+
+    unless (validate_email_syntax(@_)) {
+        my $email = $addr . Bugzilla->params->{'emailsuffix'};
+        ThrowUserError('illegal_email_address', { addr => $email });
+    }
+}
+
 sub validate_date {
     my ($date) = @_;
     my $date2;
@@ -763,6 +778,7 @@ Bugzilla::Util - Generic utility functions for bugzilla
 
   # Validation Functions
   validate_email_syntax($email);
+  check_email_syntax($email);
   validate_date($date);
 
   # DB-related functions
@@ -1069,6 +1085,12 @@ Do a syntax checking for a legal email address and returns 1 if
 the check is successful, else returns 0.
 Untaints C<$email> if successful.
 
+=item C<check_email_syntax($email)>
+
+Do a syntax checking for a legal email address and throws an error
+if the check fails.
+Untaints C<$email> if successful.
+
 =item C<validate_date($date)>
 
 Make sure the date has the correct format and returns 1 if

diff --git a/docs/en/xml/administration.xml b/docs/en/xml/administration.xml
index bdcdaeefb7db545b20acb787538a2508c863a76a..0ececabd2cfc441ef48b9dfad54e7616e87c8741 100644 (file)
--- a/docs/en/xml/administration.xml
+++ b/docs/en/xml/administration.xml
@@ -288,11 +288,11 @@
               <para>
                 Defines the regular expression used to validate email addresses
                 used for login names. The default attempts to match fully
-                qualified email addresses (i.e. 'user@example.com'). Some
-                Bugzilla installations allow only local user names (i.e 'user' 
-                instead of 'user@example.com'). In that case, the 
-                <command>emailsuffix</command> parameter should be used to define
-                the email domain.    
+                qualified email addresses (i.e. 'user@example.com') in a slightly
+                more restrictive way than what is allowed in RFC 2822.
+                Some Bugzilla installations allow only local user names (i.e 'user'
+                instead of 'user@example.com'). In that case, this parameter
+                should be used to define the email domain.
               </para>
             </listitem>
           </varlistentry>

diff --git a/t/007util.t b/t/007util.t
index e21ec28f8a00c73db128be4f9ceb7364a28ef486..a73f4dfa7498d18bc28b6684f5bd010edbbeeb5c 100644 (file)
--- a/t/007util.t
+++ b/t/007util.t
@@ -11,7 +11,7 @@
 
 use lib 't';
 use Support::Files;
-use Test::More tests => 15;
+use Test::More tests => 17;
 
 BEGIN { 
     use_ok(Bugzilla);
@@ -58,6 +58,16 @@ foreach my $input (keys %email_strings) {
        "email_filter('$input')");
 }
 
+# validate_email_syntax. We need to override some parameters.
+my $params = Bugzilla->params;
+$params->{emailregexp} = '.*';
+$params->{emailsuffix} = '';
+my $ascii_email = 'admin@company.com';
+# U+0430 returns the Cyrillic "а", which looks similar to the ASCII "a".
+my $utf8_email = "\N{U+0430}dmin\@company.com";
+ok(validate_email_syntax($ascii_email), 'correctly formatted ASCII-only email address is valid');
+ok(!validate_email_syntax($utf8_email), 'correctly formatted email address with non-ASCII characters is rejected');
+
 # diff_arrays():
 my @old_array = qw(alpha beta alpha gamma gamma beta alpha delta epsilon gamma);
 my @new_array = qw(alpha alpha beta gamma epsilon delta beta delta);

diff --git a/template/en/default/admin/params/auth.html.tmpl b/template/en/default/admin/params/auth.html.tmpl
index ceb85c98471f0a3194fc672f20858f0b8fe17694..96aba3c1d948cf598242d589b66d5e224e1786e1 100644 (file)
--- a/template/en/default/admin/params/auth.html.tmpl
+++ b/template/en/default/admin/params/auth.html.tmpl
@@ -93,10 +93,13 @@
                   "front page will require a login. No anonymous users will " _
                   "be permitted.",
 
-  emailregexp => "This defines the regexp to use for legal email addresses. The " _
-                 "default tries to match fully qualified email addresses. Another " _
-                 "popular value to put here is <tt>^[^@]+$</tt>, which means " _
-                 "'local usernames, no @ allowed.'",
+  emailregexp =>
+    "This defines the regular expression to use for legal email addresses. " _
+    "The default tries to match fully qualified email addresses. " _
+    "Use <tt>.*</tt> to accept any email address following the " _
+    "<a href='http://tools.ietf.org/html/rfc2822#section-3.4.1'>RFC 2822</a> " _
+    "specification. Another popular value to put here is <tt>^[^@]+$</tt>, " _
+    "which means 'local usernames, no @ allowed.'",
 
   emailregexpdesc => "This describes in English words what kinds of legal addresses " _
                      "are allowed by the <tt>emailregexp</tt> param.",

diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 506dca582c62e78e12aa9f2122179a0363a8c237..24d0392cad37c5dc328f731f0cd9900eb49c9e0b 100644 (file)
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -36,8 +36,7 @@
     [% ELSE %]
       [%+ Param('emailregexpdesc') FILTER html_light %]
     [% END %]
-    It must also not contain any of these special characters:
-    <tt>\ ( ) &amp; &lt; &gt; , ; : &quot; [ ]</tt>, or any whitespace.
+    It also must not contain any illegal characters.
 
   [% ELSIF error == "authres_unhandled" %]
     The result value of [% value FILTER html %] was not handled by

diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 1585003ec1767ea7196e76abca372a4f67068edc..8dbaa4ad408dc499293ca6bba3d4dad1a3109a0b 100644 (file)
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -841,9 +841,8 @@
     [% ELSE %]
       [%+ Param('emailregexpdesc') FILTER html_light %]
     [% END %]
-    It must also not contain any of these special characters:
-    <tt>\ ( ) &amp; &lt; &gt; , ; : &quot; [ ]</tt>, or any whitespace.
-    
+    It also must not contain any illegal characters.
+
   [% ELSIF error == "illegal_frequency" %]
     [% title = "Too Frequent" %]
     Unless you are an administrator, you may not create series which are 

diff --git a/token.cgi b/token.cgi
index 7c6b4e8d197d9fb88f7ffd02c7f1369d91cfaa2b..5f647edb343e5282fef1b0234508624021145fb5 100755 (executable)
--- a/token.cgi
+++ b/token.cgi
@@ -117,9 +117,7 @@ sub requestChangePassword {
     my $login_name = $cgi->param('loginname')
       or ThrowUserError("login_needed_for_password_change");
 
-    validate_email_syntax($login_name)
-      || ThrowUserError('illegal_email_address', {addr => $login_name});
-
+    check_email_syntax($login_name);
     my $user = Bugzilla::User->check($login_name);
 
     # Make sure the user account is active.

diff --git a/userprefs.cgi b/userprefs.cgi
index ac323c65e33a70d0661985f4adceed3e044c98f7..e527b148946ab03e0e016b16750f5a5fe296cace 100755 (executable)
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -111,8 +111,7 @@ sub SaveAccount {
             }
 
             # Before changing an email address, confirm one does not exist.
-            validate_email_syntax($new_login_name)
-              || ThrowUserError('illegal_email_address', {addr => $new_login_name});
+            check_email_syntax($new_login_name);
             is_available_username($new_login_name)
               || ThrowUserError("account_exists", {email => $new_login_name});