Skip to content

Commit

Permalink
Do not use HELLO as fallback of EHLO when server responds with 421
Browse files Browse the repository at this point in the history
Change inspired by PHPMailer/PHPMailer#2189

RFC 1869 section 4.5 states that the server will return the code
421 if the SMTP server is no longer available

This change fixes an issue where the actual error response from a
failed EHLO was not surfaced due to always being overridden by the
HELLO response.
  • Loading branch information
diogomr authored and emersion committed Apr 18, 2024
1 parent 8fc2197 commit 5e727ac
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 5 deletions.
21 changes: 16 additions & 5 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,23 @@ func (c *Client) greet() error {

// hello runs a hello exchange if needed.
func (c *Client) hello() error {
if !c.didHello {
c.didHello = true
if err := c.greet(); err != nil {
c.helloError = err
} else if err := c.ehlo(); err != nil {
if c.didHello {
return c.helloError
}

c.didHello = true
if err := c.greet(); err != nil {
c.helloError = err
return c.helloError
}

if err := c.ehlo(); err != nil {
var smtpError *SMTPError
if errors.As(err, &smtpError) && (smtpError.Code == 500 || smtpError.Code == 502) {
// The server doesn't support EHLO, fallback to HELO
c.helloError = c.helo()
} else {
c.helloError = err
}
}
return c.helloError
Expand Down
34 changes: 34 additions & 0 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"bytes"
"crypto/tls"
"crypto/x509"
"errors"
"io"
"net"
"net/textproto"
Expand Down Expand Up @@ -509,6 +510,39 @@ var helloClient = []string{
"NOOP\n",
}

var shuttingDownServerHello = `220 hello world
421 Service not available, closing transmission channel
`

func TestHello_421Response(t *testing.T) {
server := strings.Join(strings.Split(shuttingDownServerHello, "\n"), "\r\n")
client := "EHLO customhost\r\n"
var cmdbuf bytes.Buffer
bcmdbuf := bufio.NewWriter(&cmdbuf)
var fake faker
fake.ReadWriter = bufio.NewReadWriter(bufio.NewReader(strings.NewReader(server)), bcmdbuf)
c := NewClient(fake)
defer c.Close()
c.serverName = "fake.host"
c.localName = "customhost"

err := c.Hello("customhost")
if err == nil {
t.Errorf("Expected Hello to fail")
}

var smtpError *SMTPError
if !errors.As(err, &smtpError) || smtpError.Code != 421 || smtpError.Message != "Service not available, closing transmission channel" {
t.Errorf("Expected error 421, got %v", err)
}

bcmdbuf.Flush()
actualcmds := cmdbuf.String()
if client != actualcmds {
t.Errorf("Got:\n%s\nExpected:\n%s", actualcmds, client)
}
}

var sendMailServer = `220 hello world
502 EH?
250 mx.google.com at your service
Expand Down

0 comments on commit 5e727ac

Please sign in to comment.