WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

#18658 closed defect (bug) (duplicate)

Allow apostrophe in email validation

Reported by: swinhoe Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2.1
Component: Users Keywords: has-patch
Focuses: Cc:

Description

When the is_email function checks the format of a supplied email, it errors when it finds an apostrophe in the local part of the email address.

Many valid emails use apostrophe's, and as such this should be accomodated in the validation.

Attachments (3)

18658-wp-login.diff (554 bytes) - added by theandystratton 2 years ago.
Patch for wp-login.php
18658-user.diff (521 bytes) - added by theandystratton 2 years ago.
Patch for wp-admin/includes/user.php
0001-Allow-email-addresses-containing-apostrophes.patch (1.9 KB) - added by holizz 21 months ago.
Unified patch of all three changes, against v3.4.1

Download all attachments as: .zip

Change History (20)

comment:2 swinhoe3 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Yes, sorry, related issue already logged. The files I was amending in the core are not referenced in the related ticket, but I assume it will resolve all validation errors relating to emails..

Thanks.

comment:3 SergeyBiryukov3 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Not sure if it's an exact duplicate. That ticket only covers one scenario, while this seems to be more general. Let's reopen for a more thorough review.

comment:4 westi3 years ago

There is another ticket of which this is probably a dupe which is to switch to using the filter_var function inside is_email - #16867

comment:5 swinhoe3 years ago

Im new here, so am not quite sure of how these things get prioritised.

What are the chances of this being addressed soon ? or is it worth me putting time into a plugin fix ?

theandystratton2 years ago

Patch for wp-login.php

theandystratton2 years ago

Patch for wp-admin/includes/user.php

comment:6 theandystratton2 years ago

I ran into this with a client a month ago, and finally got some time to investigate and create a patch.

Actually, I'm attaching two patches, one for registration, the other for the admin user add/edit functions. Allows apostrophes (effectively stripping slashes from the email address).

Should be safe since a double-quote invalidates is_email(). Hope this makes it in!

comment:7 SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

comment:8 kurtpayne2 years ago

  • Cc kpayne@… added

Related #17433

Regarding 18658-user.diff and 18658-wp-login.diff ... aren't all $_POST vars already stripped by wp_magic_quotes? I'm not sure these are necessary.

comment:9 theandystratton2 years ago

Typically in PHP I do something like this to get unescaped $_POST data:

$x = get_magic_quotes_gpc() ? stripslashes($_POST['x']) : $_POST['x'];

WP is stripping the superglobals with wp_magic_quotes() but only if magic quotes are on. After that, regardless of settings, it's adding slashes back via the call to add_magic_quotes

Regardless of your PHP settings, WP is protecting the data. It's secure and awesome, but I think this is just an outlier case that probably doesn't happen very often so it hasn't been a priority.

A slash in an email address is just ugly, but it's still valid and we should allow it...right? ;]

comment:10 theandystratton2 years ago

To confirm, I did run a series of tests with the email address "ken.o'brien@domain.com" which thew errors during registration and in add/edit user screens of the admin.

comment:11 holizz2 years ago

  • Component changed from Validation to Users
  • Type changed from enhancement to defect (bug)

This also needs to be fixed in send_confirmation_on_profile_email (wp-admin/includes/ms.php line 239 as of v3.3.2).

I'm also changing this from enhancement to bug (and from Validation to Users) because is_email already allows apostrophes in email addresses and is working correctly - the problem is that the user-related areas which call is_email aren't using stripslashes like they should be.

And now for some opinion, because this "feature" has been annoying me for years:

Regardless of your PHP settings, WP is protecting the data. It's secure and awesome, but I think this is just an outlier case that probably doesn't happen very often so it hasn't been a priority.

People who know what they're doing run all their SQL through $wpdb->prepare. People who don't know what they're doing shouldn't be putting their code on public-facing Web sites. Adding backslashes causes problems all the time, whether I've forgotten to remove them in my code, somebody else has forgotten in their plugin/theme, and bugs like this which prove that even WP core developers can't remember to use stripslashes. I think protecting neophytes who expect not to get owned is a small benefit for all the problems it causes.

comment:12 holizz2 years ago

  • Cc tom@… added

comment:13 theandystratton2 years ago

@holizz I feel you on both counts. I think the security is top concern – I think that there are just the two places we've noted where there's contextual unescaping (slash stripping, sounds sexy) that needs to occur.

On a personal note, I would love to see my patch accepted – if I have a chance I will try to submit another for send_confirmation_on_profile_email

holizz21 months ago

Unified patch of all three changes, against v3.4.1

comment:14 holizz21 months ago

  • Version changed from 3.2.1 to 3.4.1

comment:15 SergeyBiryukov21 months ago

  • Version changed from 3.4.1 to 3.2.1

Version number indicates when the bug was initially introduced/reported.

comment:16 SergeyBiryukov8 months ago

  • Milestone changed from Awaiting Review to 3.7

Moving for review along with #18039.

comment:17 nacin7 months ago

  • Milestone 3.7 deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

Duplicate of #18039.

Note: See TracTickets for help on using tickets.