Make WordPress Core

Opened 9 years ago

Closed 4 months ago

Last modified 4 months ago

#39714 closed enhancement (fixed)

Proposal: Use Full PHPMailer library

Reported by: ipstenu's profile Ipstenu Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Currently we're using a customized version of phpMailer that strips out some features. This is most evident when you compare class.smtp.php:

Ours: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-smtp.php#L465

Official: https://github.com/PHPMailer/PHPMailer/blob/v5.2.21/class.smtp.php#L465-L527

We're missing sections which plugin developers are using to support extended features (oauth, ntlm, etc). By leaving this out, we introduce a reason for them to include versions of PHPMailer which they then have to update in the case of security issues (such as the sort that predicated the 4.7.1 core release).

If we were to include the full library, it would be easier (and faster) for core to update in case of security issues, and it would provide more flexibility and security for plugins and (by extension) users of WordPress.

Attachments (3)

phpmailer.diff (748 bytes) - added by JeffMatson 9 years ago.
PHPMailer drop-in
phpmailer2.diff (525.3 KB) - added by JeffMatson 9 years ago.
Diff including new files.
39714-1.diff (4.8 KB) - added by Ipstenu 9 years ago.
Update PHPMailer to 5.2.2, no longer fork library

Download all attachments as: .zip

Change History (20)

#1 follow-up: @lukecavanagh
9 years ago

@Ipstenu

Seems like it is worth doing, with the number of SMTP or related plugins which did include their own version of PHPMailer.

#2 in reply to: ↑ 1 @Ipstenu
9 years ago

The majority of plugins included their own copy in order to include it period. It was merely an educational issue that caused that situation. The point of this ticket is that those who have a legit reason for the un-forked version of the library should not be excluded and put more at risk :)

#3 @dd32
9 years ago

I'm +1 for this. Merging a pristine copy of PHPMailer and not having to remove features for security updates is beneficial for all.

I can see arguments for keeping our current "slimmed down" version, as 99.99% of sites never use some of the functions we currently ship with (SMTP for example, or the SMTP authentication methods), but the fact it's not an official upstream version we can copy-paste irks me, and isn't good for security/file modification scanners.

#4 @Otto42
9 years ago

Seems like a good idea. We should investigate why these additional pieces were originally removed and figure out if that makes sense in the current circumstances.

If we can include upstream libraries unchanged, then we should. If we need to push things upstream to make that easier or better for all involved, then bonus points.

#5 @JeffMatson
9 years ago

I dropped in PHPMailer and things seem to be fine so far. Obviously, it would need a lot more testing. Diff attached.

At the moment, I left in the existing classes for backwards compatibility for anyone who might be calling them directly.

@JeffMatson
9 years ago

PHPMailer drop-in

@JeffMatson
9 years ago

Diff including new files.

@Ipstenu
9 years ago

Update PHPMailer to 5.2.2, no longer fork library

#6 @Ipstenu
9 years ago

I think the drop-in fix would be better. The second patch has _everything_ and we don't need all that :) Yet.

I simply updated the two files to the full version of the phpMailer source for 5.2.22 (the current stable)

#7 @Ipstenu
9 years ago

  • Keywords has-patch added

#8 @MattyRob
9 years ago

The reason these blocks of code are removed is because we don't include the necessary 'extras' used by PHPMailer to make these work.

For example the NTLM code block contains the following line:
require_once 'extras/ntlm_sasl_client.php';

This library file is not bundled with WordPress so this would result in an ugly looking failure.

In a similar way, the 'XOAUTH2' authentication method calls the $OAuth->getOauth64() function and as the class.phpmaileroauthgoogle.php library is also not included, this would fail.

To include these methods would require bundling these extra files from the PHPMailer package.

#9 @desrosj
3 years ago

  • Keywords needs-refresh added

It's been quite a bit of time. This one definitely will need a refresh and reevaluation with today's code base.

#10 @SirLouen
5 months ago

#50049 was marked as a duplicate.

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


5 months ago

This ticket was mentioned in PR #9279 on WordPress/wordpress-develop by @SergeyBiryukov.


5 months ago
#12

  • Keywords needs-refresh removed

#13 @SergeyBiryukov
5 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.9
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#14 follow-up: @SirLouen
5 months ago

  • Keywords dev-feedback added; needs-testing removed

@SergeyBiryukov do you need something here?

#15 in reply to: ↑ 14 @SergeyBiryukov
5 months ago

Replying to SirLouen:

do you need something here?

A review of PR 9279 would be great, specifically the PHPMailer parts not previously included in core.

It appears that the library no longer has an extras directory, so the issues mentioned in comment:8 are not relevant, but it would be helpful to make sure there are no errors when using the new files in a plugin.

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

#16 @SergeyBiryukov
4 months ago

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

In 60623:

External Libraries: Upgrade PHPMailer to version 6.10.0.

This is a feature and maintenance release introducing full support for RFC 6531 SMTPUTF8, meaning that plugin or theme developers are now free to use Unicode characters in email addresses, such as JøeÜser@example.com, without any complicated encoding schemes. Using this feature requires sending through a mail server that advertises support for SMTPUTF8. For full details see SMTPUTF8.md.

This commit also includes the parts of PHPMailer not previously bundled with core, specifically the DSNConfigurator, OAuth, and POP3 classes, so that plugin developers could use those extended features without including their own versions of the library.

Including the full library aims to make it easier (and faster) for core to update in case of security issues, and to provide more flexibility and security for plugins and (by extension) users of WordPress.

References:

Follow-up to [54937], [55557], [56484], [57137], [59246], [59481].

Props agulbra, Ipstenu, JeffMatson, lukecavanagh, dd32, Otto42, JeffMatson, MattyRob, desrosj, SirLouen, SergeyBiryukov.
Fixes #39714, #63811.

#17 @SirLouen
4 months ago

Massive upgrade @SergeyBiryukov
I have this in my list to do some additional testing.
I will be posting just for the record.

Note: See TracTickets for help on using tickets.