SmtpClient.DataAsync Adds Extra CRLF

[nitinag]

SmtpClient currently adds an extra CRLF when sending messages to email servers.

Because of this bug, the delivered message contains an extra CRLF compared to the actual message when you do MimeMessage.WriteToAsync.

As per the RFC:

The mail data are terminated by a line containing only a period, that
is, the character sequence "<CRLF>.<CRLF>", where the first <CRLF> is
actually the terminator of the previous line (see Section 4.5.2).
This is the end of mail data indication. The first <CRLF> of this
terminating sequence is also the <CRLF> that ends the final line of
the data (message text) or, if there was no mail data, ends the DATA
command itself (the "no mail data" case does not conform to this
specification since it would require that neither the trace header
fields required by this specification nor the message header section
required by RFC 5322 [4] be transmitted).
https://tools.ietf.org/html/rfc5321#section-4.1.1.4

The EndData parameter in SmtpClient.cs (https://github.com/jstedfast/MailKit/blob/master/MailKit/Net/Smtp/SmtpClient.cs#L77) is currently:

static readonly byte[] EndData = Encoding.ASCII.GetBytes ("\r\n.\r\n");

It should be:

static readonly byte[] EndData = Encoding.ASCII.GetBytes (".\r\n");

The first CLRF should be from the end of the message itself.

[jstedfast]

This was a solution to fix messages that do not end with a new-line.

[nitinag]

Doesn't FormatOptions.EnsureNewLine already exist for that purpose?

[nitinag]

FYI, you always currently force FormatOptions.EnsureNewLine to true regardless of the user value of that setting at https://github.com/jstedfast/MailKit/blob/master/MailKit/Net/Smtp/SmtpClient.cs#L2055.

So, removing the extra CRLF on EndData would not seem to have any other impact than preventing the extra CRLF from being sent?

[jstedfast]

Yea, the EnsureNewLine thing on FormatOptions was added later and I didn't update the SmtpClient logic. Not sure if I just forgot or if I just wasn't 100% confident at the time that there weren't any corner cases where it didn't work.

Pretty sure it's a solved problem when you write to a Dos2UnixFilter or a Unix2DosFilter because the Flush() logic tracks what the last character written was. and makes sure to write a new-line sequence when flushed if it hasn't already ended with one.

I can probably remove it from SmtpClient, but I need time to verify that it won't break anything.

[jstedfast]

Changing EndData to ".\r\n" seems to work based on the unit tests I've got currently but I'm not 100% confident that there are no corner cases where MimeMessage.WriteTo() will not end with a newline sequence (even when FormatOptions.EnsureNewLine is enabled).

[jstedfast]

Now that SmtpClient.Send*() will always use the BDAT command if the CHUNKING extension is supported, I've updated the Send*() code to not force-enable EnsureNewLine in that case.

In other words, this is now a non-issue when the SMTP server supports the CHUNKING extension.

[nitinag]

I think changing EndData to ".\r\n", is the right way to go. With the forced CRLF on DATA commands, it really should be a non-issue.

However, defaulting EnsureNewLine to true is probably a good thing for most users. We'd prefer to have control and the option to disable it because we're controlling it in other ways and it would help us catch problems, but it should default to true. Most people wouldn't disable it and WriteTo would be consistent with what gets sent via SmtpClient.Send.

Most mail servers will still treat CHUNKING/BDAT like DATA in terms of requirements of ending messages in CRLF, so BDAT won't save many of those errors. It will ensure however that they'll at least get an error message back rather than a timeout waiting for a never coming CRLF that happens with DATA or the mail server may just add the CRLF itself.

For example, here are some interesting tidbits surrounding BDAT CRLF from my notes:

Preliminary support for RFC 3030 CHUNKING (BDAT) without BINARYMIME.
The Postfix SMTP server is the only program that knows the difference
between mail that was received with BDAT or DATA.

In both cases, the Postfix SMTP server will use smtpd_data_restrictions
and smtpd_end_of_data_restrictions, it will send one Milter DATA event
per mail transaction, and it will send one DATA command ending in
<CR><LF>.<CR><LF> to an smtpd_proxy_filter. There is no difference
in the Postfix queue file content.`
http://ftp.postfix.mirrors.pair.com/experimental/postfix-3.4-20180819-nonprod.RELEASE_NOTES

Interesting bug surrounding CRLF BDAT a while ago in Exim:
https://bugs.exim.org/show_bug.cgi?id=1974

From the CHUNKING RFC:

In particular, text messages sent with the BDAT command MUST be sent in
the canonical MIME format with lines delimited with a <CR><LF>.
https://tools.ietf.org/html/rfc3030#page-5

[jstedfast]

Good point about the SMTP server just adding the ending newline itself if it doesn't exist anyway.

I've re-added for EnsureNewLine.

[jstedfast]

I didn't add this change to the v2.3.0 release that I just made because this change is still somewhat risky but I wanted to get v2.3.0 released with all of the fixes I've done so far over the past 2 months.

The corner cases you found in MimeKit that didn't properly write a newline character have been fixed so I'm guessing it's probably safe now but I gotta see if I can figure out a way to add MimeKit unit tests that cover more potential cases to feel confident enough to make this change.

[nitinag]

Makes sense. If you put it into the post-release MyGet builds, I'm comfortable putting it into use and reporting back on any issues as well.

[jstedfast]

Not sure why I didn't think of this simple solution yesterday... but the above commit modifies the SmtpDataFilter to ensure that the output ends with a CRLF sequence, and if not, adds it in the Flush().

Now that the SmtpDataFilter ensures the data ends with CRLF, we can change EndData from \r\n.\r\n to .\r\n without any need to worry at all if there are any remaining corner-cases in MimeKit.

[nitinag]

That works as well.

Taking a look at the changes, I don't see SmtpDataFilter tracking a CR (would need to be tracked across buffers as well in case CR and LF were in two separate buffers). Thus, it does not seem to add the missing CRLF at the end of DATA when the stream ends with just LF (without the CR)? Though, this also shouldn't matter since the forced FormatOptions.EnsureNewLine will ensure CRLF is present.

[nitinag]

On further review, I notice that since it also runs through Unix2DosFilter filter before hitting SmtpDataFilter to ensure all LF are CRLF. Which is why you're not tracking CR in SmtpDataFilter?

[jstedfast]

Correct. No need to track CRLF vs LF in the SmtpDataFilter because all content that reaches the SmtpDataFilter has already been converted to CRLF from LF.