[wp-trac] [WordPress Trac] #62940: wp_mail(): Address header parsing is not RFC-5322 complient and fails on quoted-string when including a "<", ">" or ", "

[WordPress Trac]

#62940: wp_mail(): Address header parsing is not RFC-5322 complient and fails on
quoted-string when including a "<", ">" or ","
-------------------------------------------------+-------------------------
 Reporter:  bhujagendra                          |       Owner:  (none)
     Type:  enhancement                          |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  Mail                                 |     Version:  2.1.1
 Severity:  normal                               |  Resolution:
 Keywords:  needs-patch needs-unit-tests has-    |     Focuses:
  test-info                                      |
-------------------------------------------------+-------------------------
Changes (by SirLouen):

 * keywords:   => needs-patch needs-unit-tests has-test-info
 * version:  trunk => 2.1.1
 * type:  defect (bug) => enhancement


Comment:

 We have to divide this into 3 bugs or enhancements (I would have created 3
 tickets, one for each one), and probably we could find an answer for each
 one individually instead of treating them as a single problem (although,
 by the end I think that there could be a single solution that could
 benefit all 3 scenarios).

 Also, these would have avoided this macro questions and answers that are
 ultra-tiring for most reviewers that will prefer to skip for the future in
 most cases.

 These problems have been here pretty much since the integration of
 PHPMailer in [4946]

 == Firstly, about parsing the various forms like `From: "test<stage"
 <test at example.com>`

 Replying to [ticket:62940 bhujagendra]:
 > The [https://www.php.net/manual/en/function.mail.php php mail()
 documentation] does not explicitly state that the `From` header needs to
 comply with RFC 2822. But it does say so for the `To` header. Furthermore,
 the current implementation does already support the `name-
 addr`-[https://datatracker.ietf.org/doc/html/rfc2822#section-3.4 syntax]
 (i.e. `[display-name] angle-addr`), which was not allowed in the preceding
 [https://datatracker.ietf.org/doc/html/rfc822#section-4.4.1 RFC 822].

 `mail` doesn't specify anything about the `from` because it doesn't have
 any specific limitations regarding the `headers` which are left to the
 final decision of the MTA (and you can pretty set whatever custom header
 you like).

 This doesn't mean that any development should or should not take care of
 this. In the case of `PHPMailer` it provides a way of taking care of the
 `From:` part from the headers.

 Remember that in `wp_mail` the `From:` parsing from Headers
 [https://github.com/WordPress/wordpress-
 develop/blob/d71f29ff0899c6eb29bc037a7c521a966405cb35/src/wp-
 includes/pluggable.php#L301-L319 you mention here] was declared legacy
 [5639] and ideally not to be manipulated from there, but from the hook
 `wp_mail_from` (similar fashion as using any other raw thing, instead of
 the provided functionality, which is causing some troubles in other areas)

 So basically, using `Headers` is a bad idea, and still PHPMailer will fail
 with such format if you directly use the hook:
 `PHPMailer Error: Invalid address:  (From): "test<stage"
 <test at example.com>`

 So technically, this should be a thing that
 [https://github.com/PHPMailer/PHPMailer/issues should be discussed
 upstream]. Plus, with just one From, without a Sender header, you cannot
 really bypass the `addFrom` method from `PHPMailer` if you want to
 actually send the email and it to be accepted by any MTA (with many
 additional problems I'm explaining after).

 == Second, the multiple `From:` issue:

 This is a little incremental to the first point, because now, we can
 actually introduce a Sender header, so we could play around `From:` a
 little more.

 Let's say we go into the `wp_mail_from` and we directly pass:

 {{{
 add_filter('wp_mail_from', function($original_email_address) {
      return 'test <test at example.com>, User <user at example.com>';
 });
 }}}

 What `PHPMailer` will reject is that `From` format. It might ultimately
 reject any of the forms you are thinking for the `From:`.

 Moreover, this is one of the least recommended practices,
 [https://serverfault.com/questions/841465/rejected-emails-messages-with-
 multiple-addresses-in-from-header-are-not-accept as many servers] will
 reject this kind of header.

 Although it's true that the specification permits this according to
 [https://datatracker.ietf.org/doc/html/rfc5322#section-3.6.2 RFC 5322], we
 need to add the Sender field in the header.

 So yes, we could add everything in the Custom Headers, but the idea moves
 towards handling everything without a full custom email in the `PHPMailer`
 fashion. Or you could do something like this:

 {{{
 add_action('phpmailer_init', function ($phpmailer) {
         $phpmailer->addCustomHeader( 'From', 'User <user at example.com>' );
         $phpmailer->addCustomHeader( 'Sender', 'test <sender at example.com>'
 );
 });

 add_filter('wp_mail_from', function() {
         return 'test at example.com';
 });

 add_filter('wp_mail_from_name', function() {
         return 'test';
 });
 }}}

 To achieve the same behaviour (although, if you set a `From:` in the
 format like in the first part, it will fail with the same PHPMailer error)

 == Third, about the `To:`, `CC:`… parsing

 Here, as in the first, I agree that the parsing is completely
 insufficient.

 But in this case, we don't really have an easy way to work around it. So I
 think is the main bug that should be sorted out. Although I have to agree
 with you that despite the Header part is legacy, sorting it out should
 also be done (and remove it from "legacy" because playing around with the
 Headers should be completely functional).

 Personally, I would approach this with a specific sanitization function,
 extracting it from `wp_mail`, and using such to parse each of the
 potential areas where an email address could be present compliant with RFC
 2822

 So I invite you to work on a possible fix (also unit tests will be 100%
 needed for this fix)

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/62940#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform