Bug 1853138/Bug 1886954 - Switch to Email::Sender and Email::Address:…

…:XS (#103) * Bug 1853138 - Switch to Email::Address::XS * Bug 1886954 - Port bug 502625 to Harmony - Replace Email::Send with Email::Sender Co-authored-by: Frédéric Buclin <LpSolit@gmail.com> Co-authored-by: Dylan William Hardison <dylan@hardison.net> Co-authored-by: Dave Miller <justdave@bugzilla.org>
bugzilla · Mar 22, 2024 · 2913530 · 2913530
1 parent f3626a8
commit 2913530
Show file tree

Hide file tree

Showing 17 changed files with 110 additions and 126 deletions.
diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm
@@ -150,18 +150,19 @@ sub update_params {
   }
 
   # Old mail_delivery_method choices contained no uppercase characters
-  if (exists $param->{'mail_delivery_method'}
-    && $param->{'mail_delivery_method'} !~ /[A-Z]/)
-  {
-    my $method      = $param->{'mail_delivery_method'};
-    my %translation = (
-      'sendmail' => 'Sendmail',
-      'smtp'     => 'SMTP',
-      'qmail'    => 'Qmail',
-      'testfile' => 'Test',
-      'none'     => 'None'
-    );
-    $param->{'mail_delivery_method'} = $translation{$method};
+  my $mta = $param->{'mail_delivery_method'};
+  if ($mta) {
+      if ($mta !~ /[A-Z]/) {
+          my %translation = (
+              'sendmail' => 'Sendmail',
+              'smtp'     => 'SMTP',
+              'qmail'    => 'Qmail',
+              'testfile' => 'Test',
+              'none'     => 'None');
+          $param->{'mail_delivery_method'} = $translation{$mta};
+      }
+      # This will force the parameter to be reset to its default value.
+      delete $param->{'mail_delivery_method'} if $param->{'mail_delivery_method'} eq 'Qmail';
   }
 
   # Convert the old "ssl" parameter to the new "ssl_redirect" parameter.

diff --git a/Bugzilla/Config/Common.pm b/Bugzilla/Config/Common.pm
@@ -11,7 +11,7 @@ use 5.10.1;
 use strict;
 use warnings;
 
-use Email::Address;
+use Email::Address::XS;
 use Socket;
 
 use Bugzilla::Util;
@@ -78,7 +78,8 @@ sub check_regexp {
 
 sub check_email {
   my ($value) = @_;
-  if ($value !~ $Email::Address::mailbox) {
+  my ($address) = Email::Address::XS->parse($value);
+  if (!$address->is_valid) {
     return "must be a valid email address.";
   }
   return "";

diff --git a/Bugzilla/Config/MTA.pm b/Bugzilla/Config/MTA.pm
@@ -37,28 +37,15 @@ use warnings;
 
 use Bugzilla::Config::Common;
 
-# Return::Value 1.666002 pollutes the error log with warnings about this
-# deprecated module. We have to set NO_CLUCK = 1 before loading Email::Send
-# to disable these warnings.
-BEGIN {
-  $Return::Value::NO_CLUCK = 1;
-}
-use Email::Send;
-
 our $sortkey = 1200;
 
 sub get_param_list {
   my $class      = shift;
   my @param_list = (
     {
-      name => 'mail_delivery_method',
-      type => 's',
-
-      # Bugzilla is not ready yet to send mails to newsgroups, and 'IO'
-      # is of no use for now as we already have our own 'Test' mode.
-      choices => [
-        grep { $_ ne 'NNTP' && $_ ne 'IO' } Email::Send->new()->all_mailers(), 'None'
-      ],
+      name    => 'mail_delivery_method',
+      type    => 's',
+      choices => ['Sendmail', 'SMTP', 'Test', 'None'],
       default => 'Sendmail',
       checker => \&check_mail_delivery_method
     },

diff --git a/Bugzilla/Hook.pm b/Bugzilla/Hook.pm
@@ -1099,9 +1099,6 @@ Params:
 
 =item C<email> - The C<Email::MIME> object that's about to be sent.
 
-=item C<mailer_args> - An arrayref that's passed as C<mailer_args> to
-L<Email::Send/new>.
-
 =back
 
 =head2 object_before_create

diff --git a/Bugzilla/Mailer.pm b/Bugzilla/Mailer.pm
@@ -22,20 +22,16 @@ use Bugzilla::Util;
 
 use Date::Format qw(time2str);
 
-use Email::Address;
+use Email::Address::XS;
 use Email::MIME;
 use Encode qw(encode);
 use Encode::MIME::Header;
 use List::MoreUtils qw(none);
 use Try::Tiny;
 
-# Return::Value 1.666002 pollutes the error log with warnings about this
-# deprecated module. We have to set NO_CLUCK = 1 before loading Email::Send
-# to disable these warnings.
-BEGIN {
-  $Return::Value::NO_CLUCK = 1;
-}
-use Email::Send;
+use Email::Sender::Simple qw(sendmail);
+use Email::Sender::Transport::SMTP;
+use Bugzilla::Sender::Transport::Sendmail;
 use Sys::Hostname;
 use Bugzilla::Version qw(vers_cmp);
 
@@ -119,22 +115,14 @@ sub MessageToMTA {
 
   my $from = $email->header('From');
 
-  my ($hostname, @args);
-  my $mailer_class = $method;
+  my $hostname;
+  my $transport;
   if ($method eq "Sendmail") {
-    $mailer_class = 'Bugzilla::Send::Sendmail';
     if (ON_WINDOWS) {
-      $Email::Send::Sendmail::SENDMAIL = SENDMAIL_EXE;
+      $transport = Bugzilla::Sender::Transport::Sendmail->new({ sendmail => SENDMAIL_EXE });
     }
-    push @args, "-i";
-
-    # We want to make sure that we pass *only* an email address.
-    if ($from) {
-      my ($email_obj) = Email::Address->parse($from);
-      if ($email_obj) {
-        my $from_email = $email_obj->address;
-        push(@args, "-f$from_email") if $from_email;
-      }
+    else {
+      $transport = Bugzilla::Sender::Transport::Sendmail->new();
     }
   }
   else {
@@ -160,22 +148,26 @@ sub MessageToMTA {
   }
 
   if ($method eq "SMTP") {
-    push @args,
-      Host     => Bugzilla->params->{"smtpserver"},
-      username => Bugzilla->params->{"smtp_username"},
-      password => Bugzilla->params->{"smtp_password"},
-      Hello    => $hostname,
-      Debug    => Bugzilla->params->{'smtp_debug'};
+    my ($host, $port) = split(/:/, Bugzilla->params->{'smtpserver'}, 2);
+    $transport = Email::Sender::Transport::SMTP->new({
+      host => $host,
+      defined($port) ? (port => $port) : (),
+      sasl_username => Bugzilla->params->{'smtp_username'},
+      sasl_password => Bugzilla->params->{'smtp_password'},
+      helo          => $hostname,
+      ssl           => Bugzilla->params->{'smtp_ssl'},
+      debug         => Bugzilla->params->{'smtp_debug'}
+    });
   }
 
-  Bugzilla::Hook::process('mailer_before_send',
-    {email => $email, mailer_args => \@args});
+  Bugzilla::Hook::process('mailer_before_send', {email => $email});
 
   try {
     my $to         = $email->header('to') or die qq{Unable to find "To:" address\n};
-    my @recipients = Email::Address->parse($to);
+    my @recipients = Email::Address::XS->parse($to);
     die qq{Unable to parse "To:" address - $to\n} unless @recipients;
     die qq{Did not expect more than one "To:" address in $to\n} if @recipients > 1;
+    die qq{Invalid address in "To:" address - $to\n} if !$recipients[0]->is_valid;
     my $recipient = $recipients[0];
     my $badhosts  = Bugzilla::Bloomfilter->lookup("badhosts");
     if ($badhosts && $badhosts->test($recipient->host)) {
@@ -230,11 +222,12 @@ sub MessageToMTA {
     close TESTFILE;
   }
   else {
-    # This is useful for both Sendmail and Qmail, so we put it out here.
+    # This is useful for Sendmail, so we put it out here.
     local $ENV{PATH} = SENDMAIL_PATH;
-    my $mailer = Email::Send->new({mailer => $mailer_class, mailer_args => \@args});
-    my $retval = $mailer->send($email);
-    ThrowCodeError('mail_send_error', {msg => $retval, mail => $email}) if !$retval;
+    eval { sendmail($email, {transport => $transport}) };
+    if ($@) {
+      ThrowCodeError('mail_send_error', {msg => $@->message, mail => $email});
+    }
   }
 
   # insert into email_rates

diff --git a/Bugzilla/Migrate/Gnats.pm b/Bugzilla/Migrate/Gnats.pm
@@ -17,7 +17,7 @@ use Bugzilla::Constants;
 use Bugzilla::Install::Util qw(indicate_progress);
 use Bugzilla::Util qw(format_time trim generate_random_password);
 
-use Email::Address;
+use Email::Address::XS;
 use Email::MIME;
 use File::Basename;
 use IO::File;
@@ -396,10 +396,10 @@ sub _get_gnats_field_data {
   if ($originator !~ Bugzilla->params->{emailregexp}) {
 
     # We use the raw header sometimes, because it looks like "From: user"
-    # which Email::Address won't parse but we can still use.
+    # which Email::Address::XS won't parse but we can still use.
     my $address = $email->header('From');
-    my ($parsed) = Email::Address->parse($address);
-    if ($parsed) {
+    my ($parsed) = Email::Address::XS->parse($address);
+    if ($parsed->is_valid) {
       $address = $parsed->address;
     }
     if ($address) {

diff --git a/Bugzilla/Send/Sendmail.pm → Bugzilla/Sender/Transport/Sendmail.pm b/Bugzilla/Send/Sendmail.pm → Bugzilla/Sender/Transport/Sendmail.pm
@@ -5,60 +5,55 @@
 # This Source Code Form is "Incompatible With Secondary Licenses", as
 # defined by the Mozilla Public License, v. 2.0.
 
-package Bugzilla::Send::Sendmail;
+package Bugzilla::Sender::Transport::Sendmail;
+
 use 5.10.1;
 use strict;
 use warnings;
 
-use base qw(Email::Send::Sendmail);
-
-use Return::Value;
-use Symbol qw(gensym);
+use parent qw(Email::Sender::Transport::Sendmail);
 
-sub send {
-  my ($class, $message, @args) = @_;
-  my $mailer = $class->_find_sendmail;
+use Email::Sender::Failure;
 
-  return failure "Couldn't find 'sendmail' executable in your PATH"
-    . " and Email::Send::Sendmail::SENDMAIL is not set"
-    unless $mailer;
+sub send_email {
+  my ($self, $email, $envelope) = @_;
 
-  return failure "Found $mailer but cannot execute it" unless -x $mailer;
+  my $pipe = $self->_sendmail_pipe($envelope);
 
-  local $SIG{'CHLD'} = 'DEFAULT';
+  my $string = $email->as_string;
+  $string =~ s/\x0D\x0A/\x0A/g unless $^O eq 'MSWin32';
 
-  my $pipe = gensym;
+  print $pipe $string
+    or Email::Sender::Failure->throw("couldn't send message to sendmail: $!");
 
-  open($pipe, '|-', "$mailer -t -oi @args")
-    || return failure "Error executing $mailer: $!";
-  print($pipe $message->as_string)
-    || return failure "Error printing via pipe to $mailer: $!";
   unless (close $pipe) {
-    return failure "error when closing pipe to $mailer: $!" if $!;
+    Email::Sender::Failure->throw("error when closing pipe to sendmail: $!") if $!;
     my ($error_message, $is_transient) = _map_exitcode($? >> 8);
     if (Bugzilla->get_param_with_override('use_mailer_queue')) {
 
       # Return success for errors which are fatal so Bugzilla knows to
       # remove them from the queue
       if ($is_transient) {
-        return failure "error when closing pipe to $mailer: $error_message";
+        Email::Sender::Failure->throw(
+          "error when closing pipe to sendmail: $error_message");
       }
       else {
-        warn "error when closing pipe to $mailer: $error_message\n";
-        return success;
+        warn "error when closing pipe to sendmail: $error_message\n";
+        return $self->success;
       }
     }
     else {
-      return failure "error when closing pipe to $mailer: $error_message";
+      Email::Sender::Failure->throw(
+        "error when closing pipe to sendmail: $error_message");
     }
   }
-  return success;
+  return $self->success;
 }
 
 sub _map_exitcode {
 
   # Returns (error message, is_transient)
-  # from the sendmail source (sendmail/sysexit.h)
+  # from the sendmail source (sendmail/sysexits.h)
   my $code = shift;
   if ($code == 64) {
     return ("Command line usage error (EX_USAGE)", 1);
@@ -112,3 +107,10 @@ sub _map_exitcode {
 
 1;
 
+=head1 B<Methods in need of POD>
+
+=over
+
+=item send_email
+
+=back
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
@@ -38,7 +38,7 @@ use Date::Parse;
 use DateTime::TimeZone;
 use DateTime;
 use Digest;
-use Email::Address;
+use Email::Address::XS;
 use Encode qw(encode decode resolve_alias);
 use Encode::Guess;
 use English qw(-no_match_vars $EGID);
@@ -227,13 +227,19 @@ sub html_light_quote {
 sub email_filter {
   my ($toencode) = @_;
   if (!Bugzilla->user->id) {
-    my @emails = Email::Address->parse($toencode);
-    if (scalar @emails) {
-      my @hosts = map { quotemeta($_->host) } @emails;
-      my $hosts_re = join('|', @hosts);
-      $toencode =~ s/\@(?:$hosts_re)//g;
-      return $toencode;
+    my $email_re = qr/([a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,})/;
+    my @hosts;
+    while ($toencode =~ /$email_re/g) {
+      my @emails = Email::Address::XS->parse($1);
+      if (scalar @emails) {
+        my @these_hosts = map { quotemeta($_->host) } @emails;
+        push @hosts, @these_hosts;
+      }
     }
+    my $hosts_re = join('|', @hosts);
+
+    $toencode =~ s/\@(?:$hosts_re)//g;
+    return $toencode;
   }
   return $toencode;
 }
@@ -740,15 +746,10 @@ sub validate_email_syntax {
   my $match  = Bugzilla->params->{'emailregexp'};
   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 $address = Email::Address::XS->parse_bare_address($email);
   if ( $addr =~ /$match/
+    && $address->is_valid
     && $email !~ /\P{ASCII}/
-    && $email =~ /^$addr_spec$/
     && length($email) <= 127)
   {
     return 1;

diff --git a/Dockerfile b/Dockerfile
@@ -1,4 +1,4 @@
-FROM bugzilla/bugzilla-perl-slim:20240320.1
+FROM bugzilla/bugzilla-perl-slim:20240322.1
 
 ENV DEBIAN_FRONTEND noninteractive
 

diff --git a/Makefile.PL b/Makefile.PL
@@ -57,8 +57,9 @@ my %requires = (
   'Devel::NYTProf'             => '6.04',
   'Digest::SHA'                => '5.47',
   'EV'                         => '4.0',
+  'Email::Address::XS'         => '1.05',
   'Email::MIME'                => '1.904',
-  'Email::Send'                => '1.911',
+  'Email::Sender'              => '2.600',
   'FFI::Platypus'              => 0,
   'Future'                     => '0.34',
   'HTML::Escape'               => '1.10',

diff --git a/RELEASE_BLOCKERS.md b/RELEASE_BLOCKERS.md
@@ -35,6 +35,8 @@ their own bugfixes to those modules, but that’s not something we want to do
 upstream.  Version 5.0 rewrote the email code to use currently-supported Perl
 modules.  That needs to be ported into Harmony.
 
+**[COMPLETED]**
+
 # Postgresql Compatibility
 
 We suspect, but don’t know for certain, that BMO may have moved to using