SmtpClient: Unconditionally escape leading dots #605
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -321,7 +321,7 @@ def receive_rcpt_to_response | |||
| end | |||
|
|
|||
| def escape_leading_dots(s) | |||
| s.gsub(/^\.([^.]|$)/) { "..#{$1}" } | |||
| s.gsub(/^\.(.|$)/) { "..#{$1}" } | |||
There was a problem hiding this comment.
Do you still need the $1 capture here?
There was a problem hiding this comment.
Yes, because we need to reinsert the character caught by the wildcard . inside the capture group in the case that the leading dot is not the only character on the line.
There was a problem hiding this comment.
I think it's equivalent to s.gsub(/^\./, '..')
There was a problem hiding this comment.
You're absolutely right. Going to amend the commit.
zachallaun
force-pushed
the
unconditional-smtp-escaping
branch
from
c8e2d70
to
4a7e833
Compare
sodabrew
added a commit
that referenced
this pull request
Jun 21, 2015
SmtpClient: Unconditionally escape leading dots
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug: emails sent using
:bodyand:headersdo not have leading dots properly escaped. This PR fixes that. See "Reasoning" below for more info.Breaking change
The
EM::Protocols::SmtpClient:bodyand:headersparameters now escape leading dots on new lines.Given the following message passed in as the
:bodyOld behavior: The message would be sent as-is. The recipient would see
New behavior: The leading dot in the second line would be escaped. The recipient would see
Required updates to code: If you knew about this issue and were working around it by manually escaping leading dots in data you passed to
:bodyor:headers, you need to stop doing so to prevent double escaping.Reasoning
cc/ @sodabrew
While discussing #600, I wrote the following:
Almost immediately after posting this, @davidbalbert and I realized that this decision was incorrect. Unfortunately, the PR had been merged before we could post. Sorry, @sodabrew!
The escaping mechanism for leading dots in the SMTP DATA command is unconditional. I.e. if a line starts with a dot, another dot should be inserted in front of it no matter what comes after. The current implementation escapes a leading dot only if it is not followed by another dot. This is a bug: it prevents a user from sending a message where a line starts with two leading dots (the recipient would see one), three leading dots (the recipient would see two), etc. This pull request fixes that behavior by unconditionally escaping any leading dot.
This is a small breaking change: If someone has been working around the lack of escaping in
:bodyand:headersby escaping the leading dots themselves, this change will double-escape their leading dots. Given thatSmtpClienthas not been updated in years and to our knowledge no one had posted about the issue of unescaped leading dots, we think it's much more likely that people are unaware of this issue and sending unescaped emails via:bodyand:headersrather than manually escaping their emails themselves.