WordPress.org

Make WordPress Core

#21570 closed defect (bug) (fixed)

is_email_address_unsafe() is too aggressive

Reported by: mdawaffe Owned by: nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: Users Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (4)

21570.diff (1.5 KB) - added by mdawaffe 21 months ago.
21570-ut.diff (1.0 KB) - added by mdawaffe 20 months ago.
21570-ut2.diff (786 bytes) - added by mdawaffe 20 months ago.
21570.2.diff (1.6 KB) - added by mdawaffe 18 months ago.

Download all attachments as: .zip

Change History (17)

mdawaffe21 months ago

comment:2 dd3220 months ago

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

comment:3 jamescollins20 months ago

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

comment:4 follow-up: mdawaffe20 months ago

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
Last edited 20 months ago by ocean90 (previous) (diff)

comment:5 in reply to: ↑ 4 ; follow-up: westi20 months ago

  • Milestone changed from Awaiting Review to 3.5

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.

mdawaffe20 months ago

comment:7 in reply to: ↑ 5 mdawaffe20 months ago

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.

mdawaffe20 months ago

comment:8 follow-up: mdawaffe20 months ago

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

attachment:21570-ut2.diff

Version 0, edited 20 months ago by mdawaffe (next)

comment:9 in reply to: ↑ 8 westi20 months ago

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]

comment:10 bpetty18 months ago

  • Keywords needs-unit-tests removed

comment:11 nacin18 months ago

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

Last edited 18 months ago by SergeyBiryukov (previous) (diff)

mdawaffe18 months ago

comment:13 nacin18 months ago

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

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.

Note: See TracTickets for help on using tickets.