WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 9 months ago

Last modified 9 months ago

#43542 closed defect (bug) (fixed)

Duplicate MIME-Version header

Reported by: junktrunk Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.9.4
Component: Mail Keywords: good-first-bug 2nd-opinion has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Issue: Duplicate "MIME-Version" header information can be sent via pluggable.php and class-phpmailer.php via plugins, causing the mail to fail to be delivered when using a service like AWS SMTP that rejects this duplicate header element.

I see that this issue has been reported in the past but closed as an invalid bug (e.g. #22492 from 5 years ago). But I argue that that may not be the correct conclusion.

Wordpress plugin developer guidance on the use of wp_mail() doesn't prohibit including MIME-Version in $headers , and does otherwise offer to "build the proper Mime header automagically". I have read an argument online that it may be proper for the plugin to build out the full header including the mandatory MIME-Version in the event the administrator has configured their site to use a different email service besides WP/PHP, though I haven't substantiated that fully.

Personally I think this should be a configurable option within any plugin, defaulted to off, if included at all. But in googling this issue, I see that this problem occurs in a large number of plugins, and for years, going unfixed, resulting in probably millions of unsent messages. For example, the following bug report for Email Subscription (the plugin I am working with that caused me to re-re-discover this problem) is from 2014, where it's still a problem today: https://github.com/Nyholm/Wordpress-Email-Subscription/issues/10

Regardless of the best component where this element should be added, the default installation of WP should have checks to avoid duplication that causes failures, especially given there is an easy fix. In pluggable.php, in the loop processing each of the $headers the plugin supplied, we can check to see whether any of those will be duplicated by class-phpmailer.php, omitting them if so:

        // Set custom headers
        if ( !empty( $headers ) ) {
                foreach ( (array) $headers as $name => $content ) {
                        // Only add custom header if not one that will be added automatically
                        // by wp-includes/class-phpmailer.php
                        if (('MIME-Version' !== $name) && ('X-Mailer' !== $name)) 
                                $phpmailer->addCustomHeader( sprintf( '%1$s: %2$s', $name, $content ) );
                }

(In my code snippet above I included X-Mailer too because that's also added automatically by class-phpmailer.php and doesn't make sense to duplicate, though it is not causing the same mail delivery failure when duplicated.)

Attachments (4)

43542.diff (729 bytes) - added by lbenicio 2 years ago.
.diff (867 bytes) - added by bennemann 2 years ago.
Patch for preventing to set multiple headers
class-phpmailer.php (143.3 KB) - added by aryamaaru 2 years ago.
43542.2.diff (3.2 KB) - added by lbenicio 2 years ago.

Download all attachments as: .zip

Change History (15)

#1 @junktrunk
2 years ago

I'm also happy to commit the fix if you agree this should be fixed.

#2 @danieltj
2 years ago

  • Keywords needs-patch good-first-bug reporter-feedback added

Welcome to Trac @junktrunk.

The best thing to do is to create a patch if you think you have a possible fix for this and upload to the ticket as either a .patch or .diff file (diffs and preferred). Then if someone has time they can test the patch for you and provide feedback on it before possible inclusion within core.

The old ticket references BuddyPress, are you running into this issue because of that plugin or is this directly related to core? If it is related BuddyPress, it'll be worth reaching out to the developers for that plugin rather than here. If it's not, carry on as you were! :)

Related to #22492.

@lbenicio
2 years ago

#3 @studyboi
2 years ago

  • Keywords 2nd-opinion added

To solve the problem of having the header be duplicated by having this function called twice, I added a fail-safe to prevent it. It checks whether a header has been created yet before executing the code to add one.

// Set custom headers
        if ( !empty( $headers ) ) {
                foreach ( (array) $headers as $name => $content ) {
                        // Only add custom header if not one that will be added automatically
                        // by wp-includes/class-phpmailer.php
                        if (('MIME-Version' !== $name) && ('X-Mailer' !== $name)){ 
                                if (get_header($name) == ""){
                                        $phpmailer->addCustomHeader( sprintf( '%1$s: %2$s', $name, $content ) );
                                }
                        }
                }

@bennemann
2 years ago

Patch for preventing to set multiple headers

#4 @bennemann
2 years ago

I just put the solution from @studyboi into a .diff

#5 @SergeyBiryukov
2 years ago

  • Description modified (diff)

@lbenicio
2 years ago

#6 @noisysocks
2 years ago

  • Keywords has-patch added; needs-patch removed

#7 @SergeyBiryukov
14 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by sergey. View the logs.


10 months ago

#9 @SergeyBiryukov
9 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 46115:

Mail: Avoid setting duplicate MIME-Version and X-Mailer headers in wp_mail(), they are added automatically by PHPMailer.

Props lbenicio, junktrunk, danieltj, studyboi, bennemann.
Fixes #43542.

#10 @SergeyBiryukov
9 months ago

In 46116:

Coding Standards: Fix WPCS issue in [46115].

See #43542.

#11 @SergeyBiryukov
9 months ago

In 46118:

Mail: Add a unit test to make sure wp_mail() does not duplicate the MIME-Version header added automatically by PHPMailer.

See #43542.

Note: See TracTickets for help on using tickets.