1073380 - Pass the hostname and port as two separate arguments to Email::Sender

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[:glob ✱]

we pass the value of the 'smtpserver' parameter as the 'host' in the Email::Sender::Transport::SMTP constructor:

    if ($method eq "SMTP") {
        $transport = Email::Sender::Transport::SMTP->new({
            host  => Bugzilla->params->{'smtpserver'},
            sasl_username => Bugzilla->params->{'smtp_username'},
            sasl_password => Bugzilla->params->{'smtp_password'},
            helo => $hostname,
            ssl => Bugzilla->params->{'smtp_ssl'},
            debug => Bugzilla->params->{'smtp_debug'} });
    }

Email::Sender::Transport::SMTP has a different parameter for the port:

    "host": the name of the host to connect to; defaults to "localhost"
    "port": port to connect to; defaults to 25 for non-SSL, 465 for SSL

this will break any site which isn't using port 25 to connect to their smtp server (see also bug 950363).

Comment 1

[Frédéric Buclin]

AFAICT, this is not true. IO::Socket::INET, which gets args from Email::Sender, says that you can optionally specify the port together with the hostname:

  PeerAddr    Remote host address          <hostname>[:<port>]

Comment 2

[David Lawrence [:dkl]]

Dylan, you have had some experience with testing SMTP recently. Can you can take a look at this one verify that it does work as host:port or if we need to split the param values on ':' and pass the port in separately?

Thanks
dkl

Comment 3

[Dylan Hardison [:dylan] (he/him)]

(In reply to David Lawrence [:dkl] from comment #2)
> Dylan, you have had some experience with testing SMTP recently. Can you can
> take a look at this one verify that it does work as host:port or if we need
> to split the param values on ':' and pass the port in separately?
> 
> Thanks
> dkl

The module uses Net::SMTP (or Net::SMTP::SSL).
Net::SMTP does sublcass IO::Socket::INET, but in its overloaded new(),
the PeerPort argument is given, and passed to IO::Socket::INET. However, IO::Socket::INET ignores the argument PeerPort if there is a port specified in the PeerAddr. 
It calls a private function (_sock_info()) with PeerAddr and PeerPort. PeerPort is only used if PeerAddr does not contain a port number.

This one liner demonstrates:

perl -MIO::Socket::INET -E 'say join(", ", IO::Socket::INET::_sock_info("hardison.net:22", 80))'

“Piet van Oostrum: I find this a nice feature but it is not according to the documentation. Or is it a BUG?
Larry Wall: Let's call it an accidental feature. :-)”
--Larry Wall

Now, I don't believe that Email::Sender::Transport::SMTP intends for us to know that we're using Net::SMTP or that Net::SMTP in turn uses IO::Socket::INET, so probably it is good to be explicit about this?

Comment 4

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #3)
> Now, I don't believe that Email::Sender::Transport::SMTP intends for us to
> know that we're using Net::SMTP or that Net::SMTP in turn uses
> IO::Socket::INET, so probably it is good to be explicit about this?

I'm not against being explicit. I'm just saying that I didn't regress anything as I use the hostname:port syntax myself.

From a UI point of view, there is nothing to change. The hostname:port syntax is fine. We just need to use a regexp on it in Mailer.pm to split it into hostname and port.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

I'll supply a patch to make the code more obvious today.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1073380-v1.patch

There should be no behavior change from this patch, but it makes the fact the smtpserver parameter can contain a port more obvious.

Note: This will not support IPv6-style addresses in the form of [2600:3c00:e001:1700::1]:25, but IO::Socket::INET doesn't support that either.

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 8505218 [details] [diff] [review]
bug-1073380-v1.patch

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

::: Bugzilla/Mailer.pm
@@ +127,4 @@
>      }
>  
>      if ($method eq "SMTP") {
> +        my ($host, $port) = split(/:/,  Bugzilla->params->{'smtpserver'}, 2);

please check that the $port value is not empty (i.e. "localhost:") and that it is a integer value (i.e. "localhost:abc") using detaint_natural.

Both of those cases gives me a nasty non-Bugzilla fatal error. We also get an error if the port number is not reachable that gives a stack trace with headers but not sure how to detect that before trying to send.

Comment 8

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #7)
> please check that the $port value is not empty (i.e. "localhost:") and that
> it is a integer value (i.e. "localhost:abc") using detaint_natural.
> 
> Both of those cases gives me a nasty non-Bugzilla fatal error. We also get
> an error if the port number is not reachable that gives a stack trace with
> headers but not sure how to detect that before trying to send.

Additional note, probably better to do these checks at the time the parameter is saved using a validator and now cause a crash later when a user is making a change.

dkl

Comment 9

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #7)
> please check that the $port value is not empty (i.e. "localhost:") and that
> it is a integer value (i.e. "localhost:abc") using detaint_natural.

Actually, no, this is not the right place to do this check. This must be done earlier when the admin sets the 'smtpserver' parameter. This is a separate issue and I propose this is fixed in another bug.

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Note, this is now required as the "accidental feature" has been corrected:

https://github.com/rjbs/Email-Sender/commit/99511418db56d1c83ac62f41d366bafee7759cff

Comment 11

[Frédéric Buclin]

So now this bug must be fixed for 5.0.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1073380-v2.patch

Now with 100% more parameter validation

Comment 13

[Frédéric Buclin]

Comment on attachment 8506655 [details] [diff] [review]
bug-1073380-v2.patch

Please focus on what this bug is about. The validation is already done in bug 1083737.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1073380-v2.1.patch

Now with 100% less validation.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 8506938 [details] [diff] [review]
bug-1073380-v2.1.patch

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

r=dkl

Comment 16

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   676ae49..226b92c  master -> master