search

emersion / go-smtp

📤 An SMTP client & server library written in Go
MIT License
1.72k stars 216 forks source link

Do not use HELLO cmd as fallback of EHLO when server responds with code 421 #260

Closed diogomr closed 5 months ago

diogomr commented 5 months ago

Change inspired by https://github.com/PHPMailer/PHPMailer/issues/2189

RFC1869, Section 4.5 (https://datatracker.ietf.org/doc/html/rfc1869#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.

A real case scenario of this issue:

220 smtp.hostedemail.com ESMTP
EHLO goodvouch.com
421 Service not available, closing transmission channel
HELO goodvouch.com

The error surfaced by client.Hello is EOF instead of 421 Service not available, closing transmission channel

diogomr commented 5 months ago

Had a bit of an hard time figuring how to best test this change, so please let me know if you prefer a different style.

Also feel free to edit anything directly if you don't like the style.

emersion commented 5 months ago

Hm, maybe we can tighten this even further and only attempt HELO if the server returns a 500 error ("command not recognized")? See https://datatracker.ietf.org/doc/html/rfc5321#section-3.2

Or do you think this won't work in some cases?

diogomr commented 5 months ago

Hm, maybe we can tighten this even further and only attempt HELO if the server returns a 500 error ("command not recognized")? See https://datatracker.ietf.org/doc/html/rfc5321#section-3.2

Or do you think this won't work in some cases?

Tbh my knowledge of SMTP is not very deep, but that sounds ok.

The test suite on client_test.go uses a 502 (502 EH?) to test the fallback to HELO. Do you have context on why the 502 was used?

emersion commented 5 months ago

Hm, reading section 4.2.4, seems like 502 means that the server knows that the command is defined in an RFC but it's not implemented. I suppose it should be fine to catch that one as well.