Project

General

Profile

Actions

Bug #16948

closed

bp-email-type doesn't properly deal with 'to' arrays

Added by Boone Gorges about 2 years ago. Updated about 2 years ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Category name:
Registration
Target version:
Start date:
2022-10-03
Due date:
% Done:

0%

Estimated time:
Deployment actions:

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?

Actions #1

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:
Actions #2

Updated by Boone Gorges about 2 years ago

This all looks good to me. Thanks, Ray!

Actions #3

Updated by Boone Gorges about 2 years ago

  • Status changed from Staged for Production Release to Resolved
Actions

Also available in: Atom PDF