Make WordPress Core

Opened 6 years ago

Last modified 3 years ago

#33972 new defect (bug)

static use of PHPMailer class results in stale state between calls to wp_mail()

Reported by: codebuddy Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3
Component: Mail Keywords: needs-patch
Focuses: Cc:

Description

I've just been chasing down a problem since upgrading to WordPress 4.3 with mails sent via wp_mail() being sent with:

Content-Transfer-Encoding: quoted-printable

Auto switching to quoted-printable was added in the PHPMailer class which was upgraded in 4.3. The issue is that the internal state in $phpmailer is not properly cleared between calls to wp_mail(). There is an attempt to do this via:

Empty out the values that may be set
$phpmailer->ClearAllRecipients();
$phpmailer->ClearAttachments();
$phpmailer->ClearCustomHeaders();
$phpmailer->ClearReplyTos();

But non of these methods reset the value of $this->Encoding. So if I make two calls to wp_mail() and the first one gets switched to quoted-printable the second one will be sent as quoted-printable even if it doesn't need to be.

Would it not be better to just create a new instance of PHPMailer for each invocation of wp_mail() and let the constructor take care of it?


Attachments (1)

33972.test.diff (939 bytes) - added by stephenharris 6 years ago.
Added test. This erroneously passes because of [33124].

Download all attachments as: .zip

Change History (6)

#1 @codebuddy
6 years ago

  • Component changed from General to Security

#2 @chriscct7
6 years ago

  • Component changed from Security to Mail
  • Keywords needs-patch added

#3 @stephenharris
6 years ago

I've written a test which (almost) replicates this. The test unexpectedly passes because of changes made to the mock PHPMailer class explicitly designed to suppress this behaviour (see #28909).

In my view we should be creating a new PHPMailer instance for each email, not trying to reset the instance for each e-mail (evidently PHPMailer doesn't allow us to do that).

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


6 years ago

@stephenharris
6 years ago

Added test. This erroneously passes because of [33124].

#5 @stephenharris
5 years ago

#37737 was marked as a duplicate.

Note: See TracTickets for help on using tickets.