is_email_address_unsafe() throws notice for invalid email addresses

[ocean90]

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?

[johnjamesjacoby]

Attached patch is adequate to address the debug notice.

[jnylen0]

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

[jeremyfelt]

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.

[jeremyfelt]

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. :)

[jeremyfelt]

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.

[jeremyfelt]

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

[jeremyfelt]

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.