Make WordPress Core

Opened 14 years ago

Last modified 5 years ago

#16788 reopened defect (bug)

Ampersands in e-mail address become invalid

Reported by: jfarthing84's profile jfarthing84 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0.5
Component: Users Keywords: dev-feedback has-patch
Focuses: Cc:

Description

When an e-mail address contains an ampersand, WordPress improperly escapes the ampersand invalidating the e-mail address.

Example: h&f@… becomes h&amp@…

First of all, the proper HTML entity for "&" is &. Where did the extra amp come from?

Also, an ampersand is a valid character in an e-mail address and should not be escaped. Escaping it could be a completely different e-mail address.

I have not dug into the code to find out where this is happening but I'd assume in sanitize_email().

Attachments (1)

16788.diff (1.3 KB) - added by dkotter 10 years ago.

Download all attachments as: .zip

Change History (16)

#1 @nacin
14 years ago

Where is this occurring?

#2 @jfarthing84
14 years ago

Upon registration.

#3 @ericmann
14 years ago

I can verify this bug exists.

Testing on WordPress 3.2-bleeding.

  1. Attempted to register user with email t&est@eamann.com
  2. User was registered with no visible errors
  3. Registered user is listed with email t&ampest@eamann.com in WordPress admin
  4. Mailto mousover lists mailto:t&ampest@eamann.com as the link
  5. Editing the user lists t&est@eamann.com as the email address <- This is correct!
  6. Updating the user with t&est@eamann.com doesn't change any of the above behavior

#4 @ericmann
14 years ago

Also, the new user email sent to the admin states the following:

New user registration on your site WordPress Test Site:

Username: thisisatest

E-mail: t&amp;est@eamann.com

#5 @garyc40
14 years ago

Network Users table don't have this issue. This is because in class-wp-users-list-table.php, we sanitize_user_object() before outputting user details, while in class-wp-ms-users-list-table.php, we don't. That being said, I still think it's appropriate to sanitize user object in Network Users table as well before printing out.

When user object is sanitized, user_email filter is applied on the user's email. As a result, the email address is passed through sanitize_email(), resulting in t&amp;est@eamann.com. Now if you're in the admin panel, wp_filter_kses() will further mutilate the email address, resulting in t&amp;ampest@eamann.com. See this code.

#6 @garyc40
14 years ago

Actually, upon further investigation, sanitize_email() seems to be innocent.

Here's a more accurate recap of the process:

  • User data is sanitized before being saved to the database. pre_user_email filter is applied to user_email, which passes the email to wp_filter_kses(). Here, the email address is mutilated the first time. (t&est@test.com becomes t&amp;est@test.com)
  • When the user info is displayed in wp-admin/users.php, sanitize_user_object() is called, which in turn applies user_email filter to user_email. This results in wp_kses being called when is_admin(), thus, double-escape the email address (t&amp;est@test.com becomes t&amp;ampest@test.com).

#7 @SergeyBiryukov
13 years ago

  • Keywords needs-patch added

#9 @jkudish
12 years ago

  • Cc joachim.kudish@… added

#10 @anmari
11 years ago

Related: #11311 and #24354

@dkotter
10 years ago

#11 @dkotter
10 years ago

  • Keywords has-patch added; needs-patch removed

So that attached patch will stop ampersands from being converted in emails. In my testing, emails are then shown correctly on the front end, no encoding as mentioned above.

But I know there's lots of other suggestions we check for on emails #21537, as well as the other tickets mentioned above dealing with ampersands being encoded in other places when we might not want them to. So not sure if this patch is even worth it, but decided to throw it out there.

#12 @chriscct7
9 years ago

  • Severity changed from major to normal

#14 @garrett-eclipse
6 years ago

#38897 was marked as a duplicate.

#15 @garrett-eclipse
6 years ago

  • Status changed from new to reopened

I was going to re-open #38897 but then realized that this ticket pre-dates it and has an existing patch so will re-open this one to continue with this issue via this ticket.

#16 @SergeyBiryukov
6 years ago

  • Milestone set to Awaiting Review
Note: See TracTickets for help on using tickets.