WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#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 3 years ago.
21570-ut.diff (1.0 KB) - added by mdawaffe 3 years ago.
21570-ut2.diff (786 bytes) - added by mdawaffe 3 years ago.
21570.2.diff (1.6 KB) - added by mdawaffe 2 years ago.

Download all attachments as: .zip

Change History (17)

@mdawaffe3 years ago

comment:2 @dd323 years 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 @jamescollins3 years 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: @mdawaffe3 years 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@…: 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
Version 0, edited 3 years ago by mdawaffe (next)

comment:5 in reply to: ↑ 4 ; follow-up: @westi3 years 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.

@mdawaffe3 years ago

comment:7 in reply to: ↑ 5 @mdawaffe3 years 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.

@mdawaffe3 years ago

comment:8 follow-up: @mdawaffe3 years ago

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

attachment:21570-ut2.diff

Last edited 3 years ago by mdawaffe (previous) (diff)

comment:9 in reply to: ↑ 8 @westi3 years 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 @bpetty3 years ago

  • Keywords needs-unit-tests removed

comment:11 @nacin2 years ago

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

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

@mdawaffe2 years ago

comment:13 @nacin2 years 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.