is_email_address_unsafe() is too aggressive

[mdawaffe]

Adding "bar.com" as a banned email domain in Network Admin -> Settings causes "@foobar.com" email addresses to be blocked.

We should only check to see if the user's email address has the same domain as or is a subdomain of any banned email domain.

Attached.

See also #11775.

[SergeyBiryukov]

Related: #15706

[dd32]

Has this behaviour changed recently? or has it always been like this?

Changing the behaviour could cause back compat problems for someone who was blocking based on say, '.ru' to no longer work, correct?

I seem to want to suggest that instead maybe '@…' would be the suggested method if this isn't a recent regression

[jamescollins]

The is_email_address_unsafe() function doesn't look like it's had any changes (apart from whitespace/formattin changes) since it was copied over from WPMU in [12603]:

​http://core.trac.wordpress.org/browser/trunk/wp-includes/ms-functions.php?annotate=blame&rev=21628

[mdawaffe]

The form input is labelled "Banned Email Domains" and says "If you want to ban domains from site registrations. One domain per line.", so asking people to enter "@bar.com" is weird unless we change those strings. Plus, the option is named "banned_email_domains" :)

One "recent" change is [13447], which prevents the use of regular expressions, so the preg_match() should be removed unless we want to support crazy filters on pre_option_banned_email_domains. That changeset isn't too relevant to this ticket except that it prevents a possible solution: adding /\bfoo\.com/ to the list of Banned Email Domains.

I don't think that many people are including a ".bar.com" (as opposed to a "bar.com") entry since ".bar.com" does not block a registration from "mike@…", for example.

With the current code, a "bar.com" entry has the following behavior:

• mike@bar.com: blocked = expected
• mike@foo.bar.com: blocked = expected
• mike@foo-bar.com: blocked = not expected

And a ".bar.com" entry does:

• mike@bar.com: not blocked = expected? I don't know, see above.
• mike@foo.bar.com: blocked = expected
• mike@foo-bar.com: not blocked = expected

With my patch, the "bar.com" entry does:

• mike@bar.com: blocked = expected
• mike@foo.bar.com: blocked = expected
• mike@foo-bar.com: not blocked = expected

And the ".bar.com" entry does:

• mike@bar.com: not blocked = expected? I don't know, see above.
• mike@foo.bar.com: blocked = expected
• mike@foo-bar.com: not blocked = expected

[westi]

Replying to mdawaffe:

The form input is labelled "Banned Email Domains" and says "If you want to ban domains from site registrations. One domain per line.", so asking people to enter "@bar.com" is weird unless we change those strings. Plus, the option is named "banned_email_domains" :)

One "recent" change is [13447], which prevents the use of regular expressions, so the preg_match() should be removed unless we want to support crazy filters on pre_option_banned_email_domains. That changeset isn't too relevant to this ticket except that it prevents a possible solution: adding /\bfoo\.com/ to the list of Banned Email Domains.

I don't think that many people are including a ".bar.com" (as opposed to a "bar.com") entry since ".bar.com" does not block a registration from "mike@…", for example.

With the current code, a "bar.com" entry has the following behavior:

• mike@…: blocked = expected
• mike@…: blocked = expected
• mike@…: blocked = not expected

And a ".bar.com" entry does:

• mike@…: not blocked = expected? I don't know, see above.
• mike@…: blocked = expected
• mike@…: not blocked = expected

With my patch, the "bar.com" entry does:

• mike@…: blocked = expected
• mike@…: blocked = expected
• mike@…: not blocked = expected

And the ".bar.com" entry does:

• mike@…: not blocked = expected? I don't know, see above.
• mike@…: blocked = expected
• mike@…: not blocked = expected

These test cases look sane to me - we need to get them coded into the unit-tests.

I think not allowing for .ru type blocks is fine as that doesn't meet with what I would expect someone to use or the intent of the field itself.

[SergeyBiryukov]

Related: #21730

[mdawaffe]

Replying to westi:

These test cases look sane to me - we need to get them coded into the unit-tests.

I didn't really know what I was doing, but attachment:21570-ut.diff​.

[mdawaffe]

Nacin tells me the site_option juggling is pointless since the test framework rolls back after each method.

attachment:21570-ut2.diff​

[westi]

Replying to mdawaffe:

Nacin tells me the site_option juggling is pointless since the test framework rolls back after each method.

attachment:21570-ut2.diff​

Thanks ​[UT1007]

[nacin]

See ​[UT1094] for an additional test that now fails: blocking foo.co also blocks foo.com.

[mdawaffe]

attachment:21570.2.diff​ passes all unit tests.

[nacin]

In 22461:

Fix the matching in is_email_address_unsafe(), which was too aggressive.

We should only check to see if the user's email address has the same
domain as or is a subdomain of any banned email domain.

Add a filter.

props mdawaffe.
fixes #21570.