Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only use dot-stuff workaround on old Ruby versions #1372

Merged
merged 3 commits into from
Feb 20, 2020
Merged

Only use dot-stuff workaround on old Ruby versions #1372

merged 3 commits into from
Feb 20, 2020

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Dec 28, 2019 •
edited
Loading

In #683 a workaround was introduced for broken dot-stuffing in Net::SMTP in older Ruby versions. However, the workaround is applied to all versions of Ruby, causing additional leading dots in the email being sent.

Also, this PR fixes a bug in the original workaround. We only need to do manual dot-stuffing if the last line of the email contains only a dot with no trailing newline. The tests imply that we also need to dot-stuff all lines with single dots, but that is wrong and also causes additional dots in the email being sent.

Steps to reproduce

Mail.new do |m|
  m.from "from@example.com"
  m.to   "to@example.com"
  m.body ".foo\n."
end.deliver!

Expected body (as displayed in email client)

.foo
.

Actual body

.foo
..

@c960657
Copy link
Contributor Author

c960657 commented Dec 30, 2019

Tested with Ruby 1.9.3, 2.0.0-p481, 2.3.3 and 2.7.0.

jeremy
jeremy approved these changes Feb 20, 2020
View reviewed changes
Copy link
Collaborator

@jeremy jeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @c960657 !

@jeremy jeremy merged commit 7481ec2 into mikel:master Feb 20, 2020
jeremy pushed a commit that referenced this pull request Feb 20, 2020
@c960657 @jeremy
Only use dot-stuff workaround on old Ruby versions (#1372)
c67129d 
* Dot stuff all lines, not only those containing a single period
* Only use dot-stuff workaround on old Ruby versions
@jeremy jeremy added this to the 2.7.2 milestone Feb 20, 2020
@ahorek ahorek mentioned this pull request Feb 20, 2020
Merged
@c960657 c960657 deleted the less-dot-stuffing branch February 21, 2020 07:20
@dalibor
Copy link
Contributor

dalibor commented Oct 30, 2020

@jeremy Can you please release a new mail gem version with this fix? It is an obscure bug that people are rarely finding and it is affecting few Ruby versions plus the original regex was not right causing even more issues. Thanks!

@dalibor
Copy link
Contributor

dalibor commented Oct 31, 2020

Hm, this issue/fix explains the additional dot for a line with a single dot and is probably not affecting as many as I thought.

There is another issue that I'm trying to figure out that is a missing dot. Any chance you have seen it?

I believe it happens with some buggy SMTP clients (or potentially antivirus programs or POP and IMAP in combination with) that most-likely removes the dot twice. This is very infrequent, but unfortunately it is happening.

I read about a workaround that suggests encoding . at beginning of each line with =2E that will prevent the issue. I've seen some libraries do that. Do you think we could extend Mail::Encodings::QuotedPrintable.encode with such a patch?

Also, I wonder why is Ruby's quoted-printable format 74 chars line length vs others splitting the lines at the max of 76 chars? That actually could help us by simplifying the fix to replace the first . with =2E, so with -1 + 3 we'll end-up with lines with max 76 chars so we don't have to split the line, but I'm not sure if we can rely on that the length is 74 chars?

@c960657 @jeremy Do you maybe have any thoughts about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremy jeremy jeremy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.7.2
Development

Successfully merging this pull request may close these issues.
3 participants
@c960657 @dalibor @jeremy