fix SMTP dot-stuffing

[joshgoebel]

Mail does not do proper dot-stuffing and some MTAs will completely fail when trying to send email with un-stuffed periods

[joshgoebel]

The previous issue I created before I took a stab at a patch:

#669

[carsonreinke]

http://tools.ietf.org/html/rfc2821#section-4.5.2

Seems like to me Net::SMTP should be doing this operation, as it is the client, but it doesn't?

[joshgoebel]

You could make that argument - but I wanted to get the discussion started. I have no idea how to get a patch into Net:SMTP itself - and finding it hard to believe no one before me has already fixed this. Such a huge gaping hole.

Net:SMTP definitely does no dot-stuffing, I've looked.

[carsonreinke]

You can report the Net::SMTP problem here, as that is part of the stdlib.

[carsonreinke]

Wait, think I found where Net::SMTP already does the dot stuffing.

http://svn.ruby-lang.org/cgi-bin/viewvc.cgi/tags/v1_9_3_448/lib/net/protocol.rb?view=markup, Line 258:

    def write_message_0(src)
      prev = @written_bytes
      each_crlf_line(src) do |line|
        write0 line.sub(/\A\./, '..')
      end
      @written_bytes - prev
    end

[joshgoebel]

Interesting. I'll look at this again... maybe that code path isn't getting called... I'm definitely seeing production errors where dot-stuffing does not appear to be happening with 1.9.3_448 and Mail...

[joshgoebel]

Code to reproduce:

require 'mail'
require 'net/smtp'

Mail.defaults do
   # need to provide your own SMTP, auth info, etc.
  delivery_method :smtp, { :address              => "smtp.sendgrid.net", ... }
end

mail = Mail.new do
  to "someone@domain.com"
  from "youremail@you.com"
  body "look, a period with no endline\n."
end

mail.deliver!

The trick is the unterminated last line. each_crlf_line inside write_message_0 skips over it (I guess because it's not a crlf_line and then it falls to using_each_crlf_line to be dealt with here:

    def using_each_crlf_line
      @wbuf = ''
      yield
      if not @wbuf.empty?   # unterminated last line
        write0 @wbuf.chomp + "\r\n"
      elsif @written_bytes == 0   # empty src
        write0 "\r\n"
      end
      write0 ".\r\n"
      @wbuf = nil
    end

So it's an actual edge-case that's handled (# unterminated last line) but all the Ruby does is add a "\r\n"... so now our broken "." becomes a broken ".\r\n"... when it needs the same dot-stuffing logic applied to it as the rest of the message.

Ruby's SMTP docs doesn't say anything about msgstr always needing to be \n terminated, so I guess I should go ahead and file this upstream against Ruby SMTP?

[joshgoebel]

Filed a bug against Ruby: https://bugs.ruby-lang.org/issues/9627

[joshgoebel]

Since Ruby versions like 1.9.3 are still supported until 2015 (although only getting security updates) might it still be a good idea for Mail to try and help with this issue? Now that the exact issue is understood it seems the fix would be as simple as adding newline to any body that was missing one. Is that something we should add to Mail? Would just throwing on an extra new-line break email for anyone?

[carsonreinke]

I confirmed that issue, Gmail actually gives a 502 5.5.1 Unrecognized command. Probably because it is just sending a "." after the end of the message. Definitely a problem though.

[joshgoebel]

Yep. The first improper dot closes the DATA block and then the next "." is unrecognized. I've added a monkey patch to the Ruby bug report. The fix is really trivial.

[kennethkalmer]

Nice work, I saw this fix got released with 2.2.0, which means that apart from 1.9.3 not being supported from February 2015, there are still a few versions of Ruby to support for a while. I don't know how quickly folks will be upgrading to 2.2.0.

I think the fix @yyyc514 got applied to Ruby itself is the best place. Maybe this issue should just be closed off, and if someone finds it while searching for it they'll know they have two options:

• Upgrade to 2.2.0 or later
• Add a newline to the end of their message body

If this gets merged, I'm worried we'll have to add an expiry date to it...

@mikel wdyt?

[joshgoebel]

It went in before 2.2. It's in 2.1.5 for example. I think 2.1.3 is when it first popped up.

[jeremy]

The Ruby fix was backported to 2.1 and 2.0 stable as well, with a 1.9.3 backport pending - hopefully hits before 1.9.3 EOL. An upstream fix is appropriate, but it certainly wouldn't hurt to provide one ourselves either.

[joshgoebel]

Thanks.