Make WordPress Core

Opened 13 years ago

Last modified 6 years ago

#17433 new defect (bug)

localhost is not accepted as email domain

Reported by: sanvila's profile sanvila Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.2
Component: Formatting Keywords: has-patch is-email
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Hi. Tried to install WordPress on a Debian machine not connected to Internet, only for testing purposes, so when the setup procedure asked me for an email address, I used mylogin@localhost. The setup procedure, however, rejected this as "invalid".

I think the bug is exactly in wp-includes/formatting.php, where it says:

        // Assume the domain will have at least two subs
        if ( 2 > count( $subs ) ) {
                return apply_filters( 'is_email', false, $email, 'domain_no_periods' );
        }

So: Could you please special-case "localhost" in is_email() so that it's allowed as email domain?

I guess the probability of someone using @localhost for email "by mistake" is extremely low, so this change will unlikely harm the average user.

Thanks.

Attachments (4)

17433.diff (3.2 KB) - added by kurtpayne 13 years ago.
17433-2.diff (4.8 KB) - added by kurtpayne 13 years ago.
Use filter_var to validate / sanitize e-mail addresses
17433-3.diff (3.0 KB) - added by salcode 9 years ago.
whitelist localhost domain for email addresses
17433-4.diff (2.9 KB) - added by salcode 9 years ago.
Updated to remove redundant check for single domain

Download all attachments as: .zip

Change History (21)

#1 @sivel
13 years ago

  • Keywords reporter-feedback close added

Why not just use email@localhost.localdomain ? That should work without issue.

Last edited 13 years ago by sivel (previous) (diff)

#2 @sanvila
13 years ago

There are several reasons:

a) localhost is shorter and I expected it to work.

b) My MTA is usually not configured to accept mail for localhost.localdomain. I have never had a need for that in 15 years.

What's the problem with supporting localhost since it's correct?

BTW: The default value for DB_HOST in wp-config-sample.php is localhost. It would be somewhat contradictory that localhost is not even accepted for email!

#3 @toscho
13 years ago

  • Cc info@… added

PHP 5.2 is now a requirement, so we should use the filter functions which allow me@localhost. See the plugin Extend Email Checks for an example.

Last edited 13 years ago by toscho (previous) (diff)

#4 @SergeyBiryukov
13 years ago

  • Keywords needs-patch added; reporter-feedback close removed

#5 follow-up: @kurtpayne
13 years ago

  • Cc kurtpayne added
  • Keywords has-patch added; needs-patch removed

This patch will allow dotless e-mail domains as long as the server can resolve the domain via a DNS lookup. It should allow "localhost" and other development domains, but prevent "fakedomain" (unless fakedomain resolves on your network).

If DNS times out on an invalid dotless domain, there may be a delay of up to 2 seconds. This should be encountered rarely, but, due to the stacking of sanitize_email() and is_email() may be encountered twice in a row.

Thoughts about this approach?

@kurtpayne
13 years ago

#6 in reply to: ↑ 5 @westi
13 years ago

Replying to kurtpayne:

This patch will allow dotless e-mail domains as long as the server can resolve the domain via a DNS lookup. It should allow "localhost" and other development domains, but prevent "fakedomain" (unless fakedomain resolves on your network).

If DNS times out on an invalid dotless domain, there may be a delay of up to 2 seconds. This should be encountered rarely, but, due to the stacking of sanitize_email() and is_email() may be encountered twice in a row.

Thoughts about this approach?

We don't want to add dns lookups to every call to is_email as it could slow down a site unnecessary and lead to other issues.

In general we should probably consider moving all of our validation filtering like this to use the filter_var stuff in PHP 5.2 now it is available to us.

@kurtpayne
13 years ago

Use filter_var to validate / sanitize e-mail addresses

#7 @kurtpayne
13 years ago

@westi I was ready to disagree and defend "user@localhost" as a valid address, but I dug into this a bit more and found this post on stackoverflow.com which changed my mind. The php developers who wrote the e-mail filter don't allow short domains because only FQDNs are allowed in SMTP servers according to RFC 5321.

Submitting patch 17433-2.diff to switch to email code to filter_var() as you and @toscho suggested.

#8 @pauldewouters
11 years ago

  • Cc pauldewouters@… added

#9 @nacin
10 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

Switching to filter_var() is a major design decision. We can move faster than PHP versions; filter_var() has had a lot of bugs over the years that don't get fixed until later versions of PHP if at all; etc. We need to study this carefully. And, yes, OMG it needs unit tests, and lots of them. The worst thing that we could do moving to filter_var() is regressing in other areas, as unlikely it may be here.

Simply whitelisting 'localhost' seems like a good in-the-meantime solution.

#10 @SergeyBiryukov
10 years ago

  • Description modified (diff)

#11 @nofearinc
10 years ago

What RFC or specification is being followed for email addresses? We could probably agree on some rules and add 50 more tests to the formatting phpunit group of is_email validations, and start from there with any refactoring.

Another good question is whether any framework or CMS out there is doing it better, so that we could get some inspiration from there?

Related: #17491, #21730, #25108, #30039

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


10 years ago

@salcode
9 years ago

whitelist localhost domain for email addresses

#13 @salcode
9 years ago

I've added patch 17433-3.diff, which whitelists localhost as suggested above by Nacin.

This modifies is_email() (as listed in the original description) and sanitize_email(), which was also had to be modified.

@salcode
9 years ago

Updated to remove redundant check for single domain

#14 @salcode
9 years ago

I think we can remove the following check from sanitize_email()

	// Assume the domain will have at least two subs, whitelist localhost
	if ( 2 > count( $subs ) && 'localhost' !== strtolower( $domain ) ) {
		/** This filter is documented in wp-includes/formatting.php */
		return apply_filters( 'sanitize_email', '', $email, 'domain_no_periods' );
	}

because it is redundant with this check later in the function

	// If there aren't 2 or more valid subs, whitelist localhost
	if ( 2 > count( $new_subs ) && 'localhost' !== strtolower( $domain ) ) {
		/** This filter is documented in wp-includes/formatting.php */
		return apply_filters( 'sanitize_email', '', $email, 'domain_no_valid_subs' );
	}

I've updated this patch with 17433-4.diff

#15 @ericmann
9 years ago

  • Keywords needs-unit-tests removed

#16 @miqrogroove
9 years ago

  • Keywords is-email added

#17 @ocean90
6 years ago

#43108 was marked as a duplicate.

Note: See TracTickets for help on using tickets.