WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 months ago

#52822 closed defect (bug) (fixed)

PHPMailer change in WordPress 5.7 breaks working sites

Reported by: tigertech Owned by: SergeyBiryukov
Milestone: 5.7.1 Priority: high
Severity: normal Version: 5.7
Component: Mail Keywords: has-patch needs-testing commit fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

In #52577 , the "Upgrade PHPMailer to version 6.3.0" change has this functionality:

Make the mail() and sendmail transports set the envelope sender the same way as SMTP does, i.e. use whatever From is set to, only falling back to the sendmail_from php.ini setting if From is unset.

This is a change in behavior that can break mail sending, because it changes WordPress behavior if the site has been using a valid envelope sender configured in the php.ini "sendmail_from" field. Some (many) plugins send messages from the WordPress "admin_email" address, so let's say that the site is properly configured like this:

php.ini "sendmail_from = user123@ hostingcompany.com"
WordPress admin_email address: example@ gmail.com

(Ignore the extra spaces I had to add after "@" to get trac to not hide them.)

If a plugin uses wp_mail() to send mail "From" the admin_email address (because it hasn't bothered to ask the user to specify a better one, which is sadly common), WordPress would previously send that as:

Return-Path: user123@ hostingcompany.com
From: example@ gmail.com

This would work: the "sending with a From header you don't own" is not recommended for DKIM/DMARC purposes, but at least the Return-Path is a valid address for the sender, so the SPF would likely match and it would be delivered.

Now, however, the same message would be sent as:

Return-Path: example@ gmail.com
From: example@ gmail.com

In other words, the valid PHP sendmail configuration is now ignored, and WordPress tries sending from a domain name that it is not authorized to send from. This looks like a forgery to receiving servers because neither DKIM nor SPF match, and the message is likely to be rejected. (I'm filing this bug report as a result of seeing an actual case of that happening in the wild.)

The justification for this code change is "This avoids errors from the mail() function if Sender is not set explicitly and php.ini is not configured", which is a good goal, but it's affecting more than that: it also changes how it works when that error is not happening.

The "Return-Path" envelope sender address that PHP is configured to use is important. Competent server operators have it set up to work reliably, and that shouldn't be throw away.

A better fix for this goal would be to check for errors from the mail() function, and retry it with the new code only if there actually was an error.

Change History (27)

#1 @SergeyBiryukov
5 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 5.7.1

#2 @galbaras
5 months ago

Workaround: Use filter instead of php.ini. See https://gist.github.com/elfacht/8147504 + replace $phpmailer->From on line 7 with the desired value.

Having said that, the logic of this phpMailer change defies me. If anyone bothers to set a php.ini value, surely they mean for it to be used. @tigertech you might want to argue your case by opening an issue on https://github.com/PHPMailer/PHPMailer/issues.

Version 0, edited 5 months ago by galbaras (next)

#3 @audrasjb
4 months ago

#52906 was marked as a duplicate.

#4 follow-up: @audrasjb
4 months ago

  • Keywords needs-patch 2nd-opinion added
  • Priority changed from normal to high

Hello, thanks for reporting this issue,

I think #52822 is indeed a top priority issue for 5.7.1.

Some hosting companies reached out to me to explain their concerns about the new PHPMailer version and all the potential issues that currently occur with WP 5.7. Some of them blocked the 5.7 update.

When we updated the PHPMailer external lib in 5.7, maybe we should have made a (somewhat big) change on how WordPress manage its default emails (and perhaps our best bet is to provide a setting to set a specific email address for WordPress emails).

By the way, that’s not a valid approach for a point release.

I’m not a specialist of PHPMailer, but for 5.7.1, I can see several options:

  • Find a way to override PHPMailer settings
  • Directly edit the PHPMailer lib (this is not something we want)
  • Rollback PHPMailer to its previous version (same as above!)

For more context, after a quick search, I think the issue comes from this upstream function: https://github.com/PHPMailer/PHPMailer/blob/master/src/PHPMailer.php#L1263

(the new default value of the third parameter – $auto = true – I can’t tell why but it looks like the issue is related to this change)

Last edited 4 months ago by audrasjb (previous) (diff)

#5 @audrasjb
4 months ago

For better context, here is an example where the usage of the setFrom function in WP Core should override the default PHPMailer settings: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable.php#L396

#6 @desrosj
4 months ago

Introduced in [50397]/#52577.

Chatted with @audrasjb about this briefly today in Slack and wanted to add a summary of that conversation here for transparency.

Option 2 is one I would definitely strongly oppose. Over the last 3-5 major releases we have made a number of changes that eased the burden of updating PHPMailer in Core by eliminating any direct library modifications allowing the source code to be copied verbatim (see #41750, #50377, #50379, #50176, #52369).

Option 3 also makes me nervous, and it's preferable to not roll back external libraries unless it's the last option.

I'm hoping to dive into this in the next few days to better understand it.

#7 in reply to: ↑ 4 @SergeyBiryukov
4 months ago

Replying to audrasjb:

I’m not a specialist of PHPMailer, but for 5.7.1, I can see several options:

  • Find a way to override PHPMailer settings
  • Directly edit the PHPMailer lib (this is not something we want)
  • Rollback PHPMailer to its previous version (same as above!)

For more context, after a quick search, I think the issue comes from this upstream function: https://github.com/PHPMailer/PHPMailer/blob/master/src/PHPMailer.php#L1263

(the new default value of the third parameter – $auto = true – I can’t tell why but it looks like the issue is related to this change)

Thanks! Just noting that the parameter is not new in PHPMailer 6.3.0, it was added 11 years ago in version 5.1, see the upstream commit and [17676] for core. The wp_mail() function in WordPress explicitly passes `false` there.

I think the issue comes from this commit:
https://github.com/PHPMailer/PHPMailer/commit/f9f5b8d21ed2b79fe2e660f2c0ae9c676e30210c

It should be possible to override the default settings using the phpmailer_init action, as suggested in comment:2, so I think the first option would be the preferred way here. It's also worth checking if any issues have been submitted upstream for this change.

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


4 months ago

#9 follow-up: @galbaras
4 months ago

@SergeyBiryukov Perhaps, just as an interim measure, WordPress can check for ini_get( 'sendmail_from' ) and, if set, pass it explicitly. This will simply restore things to the way they were before 5.7.

Once phpMailer reverts that change, or we find out why it was a good idea, we can figure out what to do next.

Either way, 5.7.1 is the time to deal with this issue.

#10 in reply to: ↑ 9 @tigertech
4 months ago

Replying to galbaras:

@SergeyBiryukov Perhaps, just as an interim measure, WordPress can check for ini_get( 'sendmail_from' ) and, if set, pass it explicitly. This will simply restore things to the way they were before 5.7.

That actually wouldn't fully restore it, because there's another common case being affected: even when there's nothing explicitly set in php.ini, the PHP mail() function without "-f" still may have generated a working value that's now being overridden.

For example, if the Linux username is "user123" and the server's hostname is "server456.hostingcompany.com", then a message sent using PHP mail() with no "-f" option will default to an envelope sender of "user123@ server456.hostingcompany.com", even if nothing is explicitly set in php.ini -- and many companies ensure that this default works by setting up SPF for "server456.hostingcompany.com" and setting up forwarding aliases to handle bounces that go to that address.

So this isn't really php.ini specific, although that's an easy example showing the problem. The problem is more generally that wp_mail() via PHPMailer used to call PHP's mail() function with no "-f" parameter, letting PHP and the mail server figure out what the envelope sender address should be (whether that's the php.ini setting, or the default set in the mail server, or whatever).

But now wp_mail() ends up always calling PHP mail() with a "-f" that's the same as the From address in the message, preventing PHP or the mail server from using whatever they would choose. To completely restore the original behavior, I think the change in PHPMailer that now adds the "-f" by default would have to be removed.

#11 follow-up: @galbaras
4 months ago

Actually, the -f flag is only used when !empty($this->Sender), which means that when there's no sender, the defaults should still work. Detecting the INI value and setting the Sender should resolve this problem, and in all other cases, things should behave as before.

Either way, this forum isn't about phpMailer. It's about WordPress, and changes to phpMailer are outside the scope of the discussion. Feel free to suggest your change on https://github.com/PHPMailer/PHPMailer/issues/new?assignees=&labels=&template=bug_report.md&title=.

Last edited 4 months ago by galbaras (previous) (diff)

#12 in reply to: ↑ 11 @tigertech
4 months ago

Replying to galbaras:

Actually, the -f flag is only used when !empty($this->Sender), which means that when there's no sender, the defaults should still work.

That unfortunately isn't the case because of line 1684 here:

        if ('' === $this->Sender) {
            $this->Sender = $this->From;
        }

That effectively makes "$this->Sender" never be empty in the check you're mentioning.

Either way, this forum isn't about phpMailer. It's about WordPress, and changes to phpMailer are outside the scope of the discussion.

My apologies that I wasn't clear. When I said that a complete fix would be to reverse the changes in PHPMailer that cause "-f" to now be passed from wp_mail() when it previously wasn't, I meant that such a change could be made in the copy shipped with WordPress. However, I'm aware that patching it again is an unpleasant option.

#13 follow-up: @galbaras
4 months ago

That effectively makes "$this->Sender" never be empty in the check you're mentioning.

Good point :(

In that case, unless there's a super good reason not to, the simplest short-term solution would be to modify phpMailer in WordPress, with the long-term solution of creating a GitHub ticket to change phpMailer itself.

Of course, if the phpMailer issue is resolved quickly and a release is issued before WP 5.7.1, this gets two birds with one stone :)

#14 in reply to: ↑ 13 @tigertech
4 months ago

Replying to galbaras:

Of course, if the phpMailer issue is resolved quickly and a release is issued before WP 5.7.1, this gets two birds with one stone :)

I opened https://github.com/PHPMailer/PHPMailer/issues/2298 there, so we'll see what happens. Thanks!

#15 @Synchro
4 months ago

Hi WP peeps! I'm the PHPMailer maintainer. I've responded to the report on GitHub, comments please!

#16 @ayeshrajans
4 months ago

Awesome, thanks a lot for the fix and new release. I created #52954 to upload a patch upgrading to PHPMailer 6.4.0.

#17 @audrasjb
4 months ago

  • Keywords needs-patch 2nd-opinion removed

Looks like the change was reverted upstream by our lovely friends from PHPMailer 🎉
https://github.com/PHPMailer/PHPMailer/pull/2300

Now we'll only need to update the PHPMailer external library, if it's available before we release 5.7.1.

#18 @ocean90
4 months ago

#52954 was marked as a duplicate.

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


4 months ago

#20 @ocean90
4 months ago

  • Keywords has-patch needs-testing added

I have tested ticket:52954:52954.patch on a server which had a broken mail sender and can confirm that it's working again after applying the patch.

Full changelog for PHPMailer v6.4.0: https://github.com/PHPMailer/PHPMailer/compare/v6.3.0...v6.4.0

#21 @Synchro
4 months ago

I'm always happy to keep WordPress people happy! (there are rather a lot of you!)

#22 @ocean90
4 months ago

  • Keywords commit added

#23 @SergeyBiryukov
4 months ago

  • Keywords commit removed
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#24 @ocean90
4 months ago

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

In 50628:

External Libraries: Upgrade PHPMailer from 6.3.0 to 6.4.0.

6.4.0 reverts a change that made the mail() and sendmail transports set the envelope sender if one isn't explicitly provided, as it was causing problems in specific PHP/server configurations.

Release post: https://github.com/PHPMailer/PHPMailer/releases/tag/v6.4.0
Changelog: https://github.com/PHPMailer/PHPMailer/compare/v6.3.0...v6.4.0

Props Synchro, tigertech, ayeshrajans, galbaras, audrasjb, SergeyBiryukov, desrosj, ocean90.
Fixes #52822.

#25 @ocean90
4 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs to be backported to the 5.7 branch.

#26 @SergeyBiryukov
4 months ago

  • Keywords commit added

Didn't mean to remove the commit keyword :)

#27 @SergeyBiryukov
4 months ago

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

In 50630:

External Libraries: Upgrade PHPMailer from 6.3.0 to 6.4.0.

6.4.0 reverts a change that made the mail() and sendmail transports set the envelope sender if one isn't explicitly provided, as it was causing problems in specific PHP/server configurations.

Release post: https://github.com/PHPMailer/PHPMailer/releases/tag/v6.4.0
Changelog: https://github.com/PHPMailer/PHPMailer/compare/v6.3.0...v6.4.0

Props Synchro, tigertech, ayeshrajans, galbaras, audrasjb, SergeyBiryukov, desrosj, ocean90.
Merges [50628] to the 5.7 branch.
Fixes #52822.

Note: See TracTickets for help on using tickets.