WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 19 months ago

#23578 new defect (bug)

URLs wrapped in <> parsed as HTML when wp_mail_content_type set to text/html

Reported by: iandunn Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.1
Component: Mail Keywords: has-patch
Focuses: Cc:

Description

[16285] wrapped the password-reset URL in the e-mail sent from retrieve_password() in greater-than/less-than signs in order to prevent it from breaking when line wrapped (see #14140). That has the side-effect of causing the URL to be parsed as HTML by the mail client when the message's content-type is set to text/html via the wp_mail_content_type filter. I don't see any other places in Core where this happens.

Using wp_mail_content_type to enable HTML e-mails is a common technique documented on the Codex and across the Web. Is it considered a bad practice to enable it globally -- as opposed to adding it before calling wp_mail(), then removing it after; or just setting the content type in the $headers param of wp_mail()? If so, we should document that on the Codex and possibly in wp_mail() where the filter is applied.

Even if it is, I don't think it'd hurt to have retrieve_password() check the content-type and behave accordingly. If it's "text/html", then create a proper link; otherwise wrap the URL in greater-than/less-than signs.

If everyone agrees on that approach, I'll submit a patch.

Attachments (1)

23578.diff (1.0 KB) - added by iandunn 19 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 @SergeyBiryukov2 years ago

Related/duplicate: #21095

comment:2 follow-up: @iandunn2 years ago

Thanks Sergey, I didn't come across that when I searched for an existing ticket. It sounds like the suggestion there was to revert [16285], and I agree that that's not a good idea.

I do still think this is a valid issue, though, because the current behavior leads to unexpected problems, and simply making retrieve_password() aware of the content-type before it constructs the message is an easy fix.

comment:3 in reply to: ↑ 2 @SergeyBiryukov2 years ago

Replying to iandunn:

simply making retrieve_password() aware of the content-type before it constructs the message is an easy fix.

Yes, that makes sense to me.

comment:4 @dd322 years ago

Using wp_mail_content_type to enable HTML e-mails is a common technique documented on the Codex and across the Web. Is it considered a bad practice to enable it globally -- as opposed to adding it before calling wp_mail(), then removing it after; or just setting the content type in the $headers param of wp_mail()? If so, we should document that on the Codex and possibly in wp_mail() where the filter is applied.

IMHO, Yes, If you enable it globally, you also have to take into account the fact that others use wp_mail, and expect to pass plain text content to it, and you must therefor also process the plain text accordingly with a filter or whatever is needed.

If a core change was made here, I'd be personally for detecting the HTML mimetype in wp_mail, and running any plain text input to htmlentities(), but that of course brings up questions of how to detect the content is HTML or plain text.

comment:5 @DaveWP1962 years ago

There is a problem with some webmail clients with the current message structure with the trailing ">" being treated as part of the URL. As a consequence, the user is unable to reset their password as their username has ">" appended in the reset request.

It would be probably better is the entire section was coded to send an HTML message reset request, or additional parameters were added to the 'retrieve_password_message' filter to pass the username and URL to a custom message generator so that themes can generate their own message in HTML.

comment:6 @iandunn2 years ago

#18493 already exists for changing all Core e-mails over to HTML; it hasn't had any activity for 5 months, so I don't think we can count on it being committed soon, but I'm hoping that it will be eventually.

Since retrieve_password() is the only Core function that wraps URLs in <>, then I think just making it aware of the content-type is a good solution until everything is converted over to HTML.

Last edited 2 years ago by iandunn (previous) (diff)

@iandunn19 months ago

comment:8 @iandunn19 months ago

  • Keywords has-patch added
Note: See TracTickets for help on using tickets.