search

wpsharks / comment-mail

A WordPress plugin enabling email subscriptions for comments.
http://comment-mail.com
GNU General Public License v3.0
8 stars 3 forks source link

Remove quotes in From header #258

Open IvanRF opened 8 years ago

IvanRF commented 8 years ago

I started using Postman SMTP and I found this issue. The From is adding an extra quotation and the Headers have:

From: "'Name'" <mail@mail.com>

I found that Comment Mail adds double quotes for the name: utils-mail.php#L144.

In WP reference, examples don't use quotes: https://developer.wordpress.org/reference/functions/wp_mail/ $headers = 'From: My Name <myname@example.com>' . "\r\n";

raamdev commented 8 years ago

@IvanRF Thanks for the report. I agree, those double-quotes should not be there. I'm marking this as a bug and adding it to the Next Release milestone. Those double-quotes are part of the official RFC 2822 spec that wp_mail() says must be followed. See https://github.com/websharks/comment-mail/issues/258#issuecomment-222865263

jaswrks commented 8 years ago

The quotes do serve a valid purpose, and that is a part of the official email spec. They are not always required, but they do encapsulate special characters/delimiters that may appear in the name.

From: "'Name'" <mail@mail.com>

is perfectly valid. In fact, if the name contains single quotes then double quotes are needed. However, it looks like the single quotes being added to the name is unexpected. So this looks like a Postman + Comment Mail conflict of some kind.

kristineds commented 8 years ago

@raamdev I'm not able to reproduce this problem on a WP v4.5.2 clean install. Here are the steps that I followed.

Reedyseth commented 8 years ago

On mi side the quotes appeared, I tested with Comment Mail Pro v160213.

raamdev commented 8 years ago

@jaswsinc writes...

that is a part of the official email spec.

Would you mind pointing me to that official email spec? I'm reviewing RFC 2822, which the wp_mail() docs link to, and I'm not seeing anywhere that indicates enclosing the From name in quotes or double-quotes.

None of the "Valid Address Formats" examples on the wp_mail() page show quotes in use and since Comment Mail is a WordPress plugin, I think it's important to be compatible with wp_mail(), as other WP plugins that customize the behavior of wp_mail() (e.g., Postman SMTP) will be expecting whatever behavior the WordPress docs define as acceptable.

I'm not disagreeing with the common usage of the From name being encapsulated to prevent issues with odd characters—I just want to better understand what WordPress expects; maybe it's taking care of that issue for us somewhere?

raamdev commented 8 years ago

Found the portion of RFC 2822 that explains the usage of quotes:

   3.2.5. Quoted strings

   Strings of characters that include characters other than those
   allowed in atoms may be represented in a quoted string format, where
   the characters are surrounded by quote (DQUOTE, ASCII value 34)
   characters.

So I'm going to close this. Double-quotes, the way Comment Mail is adding them, should be fine and should be allowed. It sounds like Postman SMTP is adding its own single-quotes around the From name without checking if the From name is already quoted.

raamdev commented 8 years ago

I'm reopening this after some more thought about this issue.

The main problem here appears to be that Postman SMTP is adding single-quotes to the From name:

From: 'Name' <mail@mail.com>

Comment Mail then further customizes it by adding double-quotes, which results in a format that doesn't make sense (the single-quotes get included in the name that appears in the email client, e.g., 'Name'):

From: "'Name'" <mail@mail.com>

So who's at fault? Comment Mail or Postman SMTP?

My feeling is that if Comment Mail is passing off the mail to wp_mail(), it should leave out the double-quotes, because there's no telling what other plugin might be doing its own quoting (e.g., Postman SMTP).

If Comment Mail Pro is being used and the site owner is using the Comment Mail SMTP Configuration panel to configure an SMTP server, then it would make sense to me that Comment Mail should add the double-quotes (i.e., it should be assumed that no other plugin is going to be modifying things—running two SMTP plugins wouldn't make sense and would likely cause a conflict anyway).

Proposed Solution

I propose that we don't do the double-quoting by default (as seen here) in Comment Mail Lite, but that we add a filter that allows a site owner to tell Comment Mail to add the double-quotes if that's what they want (e.g., they're not using a plugin like Postman SMTP that adds quotes around the From name). That way if a site owner is using Comment Mail Lite and finds that they're running into issues with From names not being quoted, they can enable that functionality in Comment Mail.

In Comment Mail Pro, we do the same thing: off by default with the ability to override with a filter (again, we're assuming that the site owner is using another SMTP plugin that already provides the functionality of quoting the From name). However, if Comment Mail Pro SMTP Integration is enabled, we enable double-quote by default (and in that case, it could be disabled with the same filter if desired).

jaswrks commented 8 years ago

Here is where wp_mail() strips double quotes from the name. https://github.com/WordPress/WordPress/blob/9193013158d02ecd51ab8aa52d594aa789ac9836/wp-includes/pluggable.php#L259

I've never seen single quotes used to encapsulate a name, so I think Postman is wrong in their approach. However, single or double could be stripped just as well, so I suppose it doesn't matter.

The real problem with single quotes is that they are unexpected when parsing an email recipient; i.e.., because as far as I know that's not a part of any spec.

Here's where CM adds the double quotes when it builds the From: header and I don't see any reason to stop doing that, because it would be going against the email spec and asking for other problems in my view. Particularly since wp_mail() expects these and strips them when parsing as it should. https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/UtilsMail.php#L160

Also, looking closer at CM, I see that we ​also​ strip the double quotes like WordPress whenever CM parses the name. https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/UtilsString.php#L1413

So the problem here is that Postman is using the non-standard single quote encapsulation when they should be using double quotes per RFC2822.