WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 2 months ago

#43542 reviewing defect (bug)

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 14 months ago.
.diff (867 bytes) - added by bennemann 14 months ago.
Patch for preventing to set multiple headers
class-phpmailer.php (143.3 KB) - added by aryamaaru 13 months ago.
43542.2.diff (3.2 KB) - added by lbenicio 12 months ago.

Download all attachments as: .zip

Change History (11)

#1 @junktrunk
15 months ago

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

#2 @danieltj
15 months 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
14 months ago

#3 @studyboi
14 months 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
14 months ago

Patch for preventing to set multiple headers

#4 @bennemann
14 months ago

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

#5 @SergeyBiryukov
14 months ago

  • Description modified (diff)

@lbenicio
12 months ago

#6 @noisysocks
12 months ago

  • Keywords has-patch added; needs-patch removed

#7 @SergeyBiryukov
2 months ago

  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.