Bug #16948
closedbp-email-type doesn't properly deal with 'to' arrays
0%
Description
I experienced a fatal error in my PHP 8.x testing environment during the Commons registration process. Backtrace:
[03-Oct-2022 18:59:58 UTC] PHP Fatal error: Uncaught TypeError: strlen(): Argument #1 ($str) must be of type string, array given in /home/bgorges/sites/xcommons/wp-includes/formatting.php:3675 Stack trace: #0 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-core/classes/class-bp-email-recipient.php(74): sanitize_email() #1 /home/bgorges/sites/xcommons/wp-content/plugins/bp-email-type/bp-email-type.php(93): BP_Email_Recipient->__construct() #2 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(309): Ray_BP_Email_Type->check_user_option() #3 /home/bgorges/sites/xcommons/wp-includes/plugin.php(191): WP_Hook->apply_filters() #4 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-core/bp-core-functions.php(3652): apply_filters() #5 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-core/bp-core-filters.php(591): bp_send_email() #6 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(307): bp_core_activation_signup_user_notification() #7 /home/bgorges/sites/xcommons/wp-includes/plugin.php(191): WP_Hook->apply_filters() #8 /home/bgorges/sites/xcommons/wp-includes/ms-functions.php(1066): apply_filters() #9 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(307): wpmu_signup_user_notification() #10 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters() #11 /home/bgorges/sites/xcommons/wp-includes/plugin.php(476): WP_Hook->do_action() #12 /home/bgorges/sites/xcommons/wp-includes/ms-functions.php(892): do_action() #13 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-members/bp-members-functions.php(1891): wpmu_signup_user() #14 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-members/screens/register.php(263): bp_core_signup_user() #15 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(307): bp_core_screen_signup() #16 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters() #17 /home/bgorges/sites/xcommons/wp-includes/plugin.php(476): WP_Hook->do_action() #18 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-core/bp-core-dependency.php(388): do_action() #19 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(307): bp_screens() #20 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters() #21 /home/bgorges/sites/xcommons/wp-includes/plugin.php(476): WP_Hook->do_action() #22 /home/bgorges/sites/xcommons/wp-content/plugins/buddypress/bp-core/bp-core-dependency.php(445): do_action() #23 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(307): bp_template_redirect() #24 /home/bgorges/sites/xcommons/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters() #25 /home/bgorges/sites/xcommons/wp-includes/plugin.php(476): WP_Hook->do_action() #26 /home/bgorges/sites/xcommons/wp-includes/template-loader.php(13): do_action() #27 /home/bgorges/sites/xcommons/wp-blog-header.php(19): require_once('...') #28 /home/bgorges/sites/xcommons/index.php(17): require('...') #29 {main} thrown in /home/bgorges/sites/xcommons/wp-includes/formatting.php on line 3675
After some examination, I think that the fatal error is uncovering an existing bug in the bp-email-type plugin. Here's the logic:
1. During registration, BP sends activation keys to the new user like this: https://buddypress.trac.wordpress.org/browser/tags/10.4.0/src/bp-core/bp-core-filters.php?marks=591#L564 Note that the 'to' parameter is something like:
[ 0 => [ 'test@example.com' => 'salutation' ] ]
2. bp_send_email()
correctly handles 'to' arrays: https://buddypress.trac.wordpress.org/browser/tags/10.4.0/src/bp-core/classes/class-bp-email.php?marks=920#L902
3. But when bp-email-type hooks into the sending of the activation email (https://github.com/cuny-academic-commons/cac/blob/e991bdfea26176d9727fa03298870b09b1c33acb/wp-content/plugins/bp-email-type/bp-email-type.php#L35), it doesn't account for arrays: https://github.com/cuny-academic-commons/cac/blob/e991bdfea26176d9727fa03298870b09b1c33acb/wp-content/plugins/bp-email-type/bp-email-type.php#L92
4. BP_Email_Recipient
runs the value of $to
through sanitize_email()
, which in turn does a strlen
check. In PHP 7, this fails silently. In PHP 8, it throws a fatal error.
The PHP 7 behavior has never bothered us, since it just ends up hitting the bail clause here anyway https://github.com/cuny-academic-commons/cac/blob/e991bdfea26176d9727fa03298870b09b1c33acb/wp-content/plugins/bp-email-type/bp-email-type.php#L95 But it feels like a bug anyway. I think we should be looping through if 'to' is an array. Ray, does this seem right to you? If so, could you make the change and test?
Updated by Raymond Hoh about 2 years ago
- Status changed from New to Staged for Production Release
But it feels like a bug anyway. I think we should be looping through if 'to' is an array. Ray, does this seem right to you? If so, could you make the change and test?
Thanks Boone. Just tested this in PHP 8 and did the following: https://github.com/cuny-academic-commons/cac/commit/380cca3a262723c05cf74126c264893d7a199965. If the email is a registration email, bail from doing the user lookup since this isn't necessary. If $to is an array, I'm taking the first item from the array and passing the email address that way. I don't think many devs (including us) are using BP's email API to pass an array to send a mass email. Let me know what you think here.
I also encountered a few other PHP 8 issues when testing:- Don't use array format to set default 'Reply-To' email header: https://github.com/cuny-academic-commons/cac/commit/f4269f42b9004238c9e6b33e82580a8b4543453c. This is similar to the
bp-email-type
problem. - Set static method function calls correctly in
cac-non-cuny-signup
plugin: https://github.com/cuny-academic-commons/cac/commit/fb05376f38b0df5c8af512b8b80541e41f4cc1b9. This will be a big bug across the codebase for PHP 8, but we can cross that bridge when we bump our PHP version to 8 down the road.
Updated by Boone Gorges about 2 years ago
This all looks good to me. Thanks, Ray!
Updated by Boone Gorges about 2 years ago
- Status changed from Staged for Production Release to Resolved