WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#39915 closed defect (bug) (fixed)

is_email_address_unsafe() throws notice for invalid email addresses

Reported by: ocean90 Owned by: jeremyfelt
Milestone: 4.8 Priority: normal
Severity: normal Version: 3.5
Component: Users Keywords: has-patch has-unit-tests
Focuses: multisite Cc:

Description

If the banned_email_domains option is set, is_email_address_unsafe() tries to split an email address into two parts. If the provided email doesn't contain a domain a notice is thrown.

$ wp shell
wp> is_email_address_unsafe( 'foo' );
=> bool(false)
wp> update_site_option( 'banned_email_domains', [ 'example.org' ] );
=> bool(true)
wp> is_email_address_unsafe( 'foo' );
PHP Notice:  Undefined offset: 1 in /wp-includes/ms-functions.php on line 360
Notice: Undefined offset: 1 in /wp-includes/ms-functions.php on line 360
=> bool(false)

I noticed this because in wpmu_validate_user_signup() the is_email() check happens after is_email_address_unsafe().

For wpmu_validate_user_signup() we can move the is_email() check above is_email_address_unsafe().

Not sure about is_email_address_unsafe(). Should the check be skipped if the email is invalid? Should it return true or false? Should we treat it as a domain only?

Attachments (4)

39915.patch (1.1 KB) - added by ocean90 3 years ago.
39915.diff (3.4 KB) - added by jeremyfelt 2 years ago.
39915-email-unsafe.diff (538 bytes) - added by jeremyfelt 2 years ago.
39915.2.diff (1.4 KB) - added by jeremyfelt 2 years ago.

Download all attachments as: .zip

Change History (14)

@ocean90
3 years ago

#1 @johnjamesjacoby
3 years ago

  • Keywords commit added
  • Owner set to jeremyfelt
  • Status changed from new to assigned

Attached patch is adequate to address the debug notice.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 years ago

#3 @jnylen0
2 years ago

  • Keywords needs-unit-tests added; commit removed

Can we get a unit test for this case before commit, please?

#4 @jeremyfelt
2 years ago

It looks like we can add some more test conditions to is_email_address_unsafe(). We test a lot of addresses based on matching the list of banned domains, but we don't cover the case of an invalid email address.

Not sure about is_email_address_unsafe(). Should the check be skipped if the email is invalid? Should it return true or false? Should we treat it as a domain only?

I think if the email is invalid, then we should return false as "unsafe" in this case is whether or not it matches a banned list when a valid domain is provided.

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


2 years ago

@jeremyfelt
2 years ago

#6 @jeremyfelt
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Status changed from assigned to reviewing
  • Version set to 3.5

39915.diff adds some tests for wpmu_validate_user_signup() that replicate the issue. In addition to putting is_email() above is_email_address_unsafe(), those blocks are now combined. If we know that an email is invalid already, there's no reason to check if it's unsafe. If is_email() fails, there's also a good chance sanitize_email() has already returned an empty string.

It looks like is_email_address_unsafe() could benefit from a basic check for @ rather than a full is_email(). is_email() can be filtered to allow an email without @ (why?) and we'd run into a similar report one day. 39915-email-unsafe.diff addresses this as a separate issue.

This was introduced in [22461]. Before that commit it looks like bademail would generate a domain of ademail. :)

#7 @jeremyfelt
2 years ago

In 40594:

Multisite: Validate email before checking against banned domains.

Previously, an invalid email could result in an undefined index when attempting to determine the email domain.

Props ocean90.
See #39915.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


2 years ago

@jeremyfelt
2 years ago

#9 @jeremyfelt
2 years ago

39915.2.diff adds tests for 39915-email-unsafe.diff, which ensures @ at least exists in the email address being checked.

#10 @jeremyfelt
2 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 40595:

Multisite: Check only valid looking emails against banned domain list.

If an email address is missing an @, we can't assume enough to check it against a list of domain names.

Additional validation of email should happen in is_email() before being passed to is_email_address_unsafe().

Fixes #39915.

Note: See TracTickets for help on using tickets.