From: Frédéric Buclin Date: Mon, 23 Jan 2012 16:13:37 +0000 (+0100) Subject: Bug 319953: Missing real email syntax check X-Git-Tag: bugzilla-4.3.1~121 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=96624a115fe60b8ebdbbecbc2b38a7566d4e4c59;p=thirdparty%2Fbugzilla.git Bug 319953: Missing real email syntax check r=glob a=LpSolit --- diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm index ddaa6eb629..b4709212e3 100644 --- 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 373fc1ef3a..23b08c63a1 100644 --- 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 a04095647a..bf8569839c 100644 --- 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 + +Do a syntax checking for a legal email address and throws an error +if the check fails. +Untaints C<$email> if successful. + =item C 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 bdcdaeefb7..0ececabd2c 100644 --- a/docs/en/xml/administration.xml +++ b/docs/en/xml/administration.xml @@ -288,11 +288,11 @@ 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 - emailsuffix 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. diff --git a/t/007util.t b/t/007util.t index e21ec28f8a..a73f4dfa74 100644 --- 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 ceb85c9847..96aba3c1d9 100644 --- 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 ^[^@]+$, 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 .* to accept any email address following the " _ + "RFC 2822 " _ + "specification. Another popular value to put here is ^[^@]+$, " _ + "which means 'local usernames, no @ allowed.'", emailregexpdesc => "This describes in English words what kinds of legal addresses " _ "are allowed by the emailregexp param.", diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl index 506dca582c..24d0392cad 100644 --- 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: - \ ( ) & < > , ; : " [ ], 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 1585003ec1..8dbaa4ad40 100644 --- 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: - \ ( ) & < > , ; : " [ ], 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 7c6b4e8d19..5f647edb34 100755 --- 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 ac323c65e3..e527b14894 100755 --- 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});