Make WordPress Core

Opened 12 years ago

Closed 22 months ago

Last modified 22 months ago

#19847 closed defect (bug) (fixed)

wp_mail $from_name field is removing an extra character from the name

Reported by: hakanca's profile hakanca Owned by: desrosj's profile desrosj
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.3
Component: Mail Keywords: has-patch dev-feedback has-unit-tests
Focuses: Cc:

Description

If the headers are sent as a string to wp_mail, parsing of $from_name field removes one required character at the end of the name. Hence causing a name field of "Hakan" to show up as "Haka" in the received email.

Line 257 of pluggable.php is causing this error.

$from_name = substr( $content, 0, strpos( $content, '<' ) - 1 );

Removing "-1" from substr function call solves the problem.

$from_name = substr( $content, 0, strpos( $content, '<' ));

Attachments (2)

19847.patch (572 bytes) - added by SergeyBiryukov 12 years ago.
19847.2.diff (567 bytes) - added by MikeHansenMe 12 years ago.
same patch edits now on new lines.

Download all attachments as: .zip

Change History (19)

#1 follow-up: @kawauso
12 years ago

  • Keywords reporter-feedback added

Are you passing in the From value without a space?

i.e. Hakan<hakan@…>

If not, can you provide an example to reproduce?

Also, please see patching for Windows and *nix

Last edited 12 years ago by kawauso (previous) (diff)

#2 in reply to: ↑ 1 @hakanca
12 years ago

  • Keywords reporter-feedback removed

It doesn't really matter if we have a space there or not. strpos returns the numeric position of the found string; in our case "<" character. substr returns as many characters specified by its length parameter; in our case the number we get from strpos. Both functions start counting from "0" unless we specify otherwise; which is our case. So in the following cases:

Hakan<hakan@…> --> strpos will return 5; the index position of "<" in the original string starting from "0". So substr will return "Hakan" since we are returning 5 characters from the beginning.

If, as you asked, we have a space between the name and email like below:

Hakan <hakan@…> --> then strpos will return 6, and substr will also include 6 characters and return "Hakan " -including the space-. Since we are already trimming the space in the returned string 2 lines later in the code with

$from_name = trim( $from_name );

There is no need to subtract 1 from the length of the string, since we never run into problem of accidentally including that "<" character. I think that was the false logic behind adding that -1 to substr call in the first place.

By removing that "-1" we are actually making the code more flexible, by accommodating both versions. (i.e. with or without space) And to the best of my knowledge, PHP has no problem sending the email in either case.(Tested on my server)

As for making patches to the Wordpress core, I am not an expert PHP coder per se. I'd rather leave it to people who know what they are doing. :)


Replying to kawauso:

Are you passing in the From value without a space?

i.e. Hakan<hakan@…>

If not, can you provide an example to reproduce?

Also, please see patching for Windows and *nix

#3 @MikeHansenMe
12 years ago

  • Cc mdhansen@… added

Unable to reproduce the problem. Has it been fixed elsewhere?

@MikeHansenMe
12 years ago

same patch edits now on new lines.

#4 @SergeyBiryukov
12 years ago

  • Keywords 3.6-early added
  • Milestone changed from Awaiting Review to Future Release

To reproduce, leave out the space between the name and the e-mail address:

$headers = 'From: My Name<myname@mydomain.com>' . "\r\n";
wp_mail( 'test@test.com', 'subject', 'message', $headers );

The resulting name will be cut:

From: My Nam <myname@mydomain.com>

Considerations in comment:2 are valid.

#5 @bpetty
11 years ago

  • Keywords 3.6-early removed
  • Milestone changed from Future Release to 3.6

#6 @kovshenin
11 years ago

19847.2.diff applies cleanly and seems reasonable.

#7 follow-up: @MattyRob
11 years ago

RFC emails standards demand that there is a space in an email address between the "User name" and the <local-part@domain-part> of an email address.

If emails are passed without the space then it is being done wrong. I'm just asking then if we should really be 'fixing' this?

It kind of feels that we should but the standards are there for a reason aren't they?

#8 @mordauk
11 years ago

  • Cc pippin@… added

#9 in reply to: ↑ 7 ; follow-up: @DrewAPicture
11 years ago

  • Keywords dev-feedback added

Replying to MattyRob:

RFC emails standards demand that there is a space in an email address between the "User name" and the <local-part@domain-part> of an email address.

Can you provide a reference link?

If emails are passed without the space then it is being done wrong. I'm just asking then if we should really be 'fixing' this?

If it's considered the standard, I'm kind of with you that maybe this is just a case of _doing_it_wrong() and not something that needs "fixing"

#10 in reply to: ↑ 9 @MattyRob
11 years ago

Replying to DrewAPicture:

Replying to MattyRob:

RFC emails standards demand that there is a space in an email address between the "User name" and the <local-part@domain-part> of an email address.

Can you provide a reference link?

http://tools.ietf.org/html/rfc5322#section-3.4

I think this is the document I was referring to.

The pertinent sections seem to build on each other so:

name-addr = [display-name] angle-addr

name-addr is what we are defining here with display name being the define human readable name and angle-addr being the email address in angled brackets. The definition for angle-addr is:

[CFWS] "<" addr-spec ">" [CFWS]

Where adds-spec is:

local-part "@" domain

I think I've also read somewhere that a FWS must be included between the display-name and angle-addr but I can't find that right now. I'll post again if I stumble upon it.

#11 @ryan
11 years ago

  • Milestone changed from 3.6 to Future Release

#12 @iseulde
11 years ago

  • Component changed from General to Mail

#13 @chriscct7
9 years ago

  • Version changed from 3.3.1 to 3.3

This ticket was mentioned in PR #2968 on WordPress/wordpress-develop by desrosj.


23 months ago
#14

  • Keywords has-unit-tests added

#15 @desrosj
23 months ago

  • Milestone set to 6.1

Found this one while looking through the list of tickets missing a milestone.

I wrote a unit test with a few assertions to confirm that this is still happening, and looks like it is.

I've created a PR that includes both 19847.2.diff and the unit test, and looks like it does the trick. The -1 is not actually required anyway since the trim() call in the following lines will also remove the space (if present).

Looking at how the headers are passed to PHPMailer, each one is expanded using explode() before performing some basic trimming and adjusting to be in the proper format before getting passed. For the from name and from email, setFrom() within PHPMailer expects them to be passed as separate parameters in order to perform some of its own validation.

With this in mind, we could add a __doing_it_wrong(), but I'm not sure it's really necessary since PHPMailer will reconstruct the header correctly with the space later on.

#16 @desrosj
22 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 53900:

Mail: Prevent the last character of names in “From” headers from being trimmed.

When extracting the email and name from a “From” header, the last character of the name is incorrectly trimmed when a space is not included between the name and the opening <.

Though the space is required for the header to be compliant with RFC5322 (see https://www.rfc-editor.org/rfc/rfc5322#section-3.4), the absence of a space can be ignored here. PHPMailer accepts the name and email as separate parameters and constructs the header correctly later on.

Props hakanca, mikehansenme, SergeyBiryukov, kovshenin, mattyrob, drewapicture, desrosj.
Fixes #19847.

Note: See TracTickets for help on using tickets.