Skip to:
Content

BuddyPress.org

Opened 4 months ago

Closed 3 months ago

Last modified 3 months ago

#9127 closed defect (bug) (fixed)

Critical error when updating xProfile fields through REST API using PHP 8+

Reported by: niftythree's profile niftythree Owned by: espellcaste's profile espellcaste
Milestone: 14.0.0 Priority: low
Severity: minor Version: 12.4.0
Component: REST API Keywords: has-unit-tests needs-testing-info has-dev-note has-patch commit
Cc:

Description

Hello,

When attempting to update an xProfile field through the REST API with value(s) that have an apostrophe in them, the REST API succeeds at making the change but then returns "There has been a critical error on this website." when using PHP 8 versions. Upon downgrading to PHP 7.4, the are no issues. We're using the latest WordPress and BuddyPress versions with the Legacy template.

Output in error log:

PHP Fatal error:  Uncaught TypeError: implode(): Argument #2 ($array) must be of type ?array, bool given in /wp-content/plugins/buddypress/bp-xprofile/bp-xprofile-template.php:986
Stack trace:
#0 /wp-content/plugins/buddypress/bp-xprofile/bp-xprofile-template.php(986): implode()
#1 /wp-content/plugins/buddypress/bp-xprofile/classes/class-bp-rest-xprofile-fields-endpoint.php(860): bp_unserialize_profile_field()
#2 /wp-content/plugins/buddypress/bp-xprofile/classes/class-bp-rest-xprofile-data-endpoint.php(437): BP_REST_XProfile_Fields_Endpoint->get_profile_field_rendered_value()
#3 /wp-content/plugins/buddypress/bp-xprofile/classes/class-bp-rest-xprofile-data-endpoint.php(253): BP_REST_XProfile_Data_Endpoint->prepare_item_for_response()
#4 /wp-includes/rest-api/class-wp-rest-server.php(1193): BP_REST_XProfile_Data_Endpoint->update_item()
#5 /wp-includes/rest-api/class-wp-rest-server.php(1041): WP_REST_Server->respond_to_request()
#6 /wp-includes/rest-api/class-wp-rest-server.php(431): WP_REST_Server->dispatch()
#7 /wp-includes/rest-api.php(424): WP_REST_Server->serve_request()
#8 /wp-includes/class-wp-hook.php(324): rest_api_loaded()
#9 /wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters()
#10 /wp-includes/plugin.php(565): WP_Hook->do_action()
#11 /wp-includes/class-wp.php(418): do_action_ref_array()
#12 /wp-includes/class-wp.php(813): WP->parse_request()
#13 /wp-includes/functions.php(1336): WP->main()
#14 /wp-blog-header.php(16): wp()
#15 /index.php(17): require('/website/12345.we...')
#16 {main} thrown in /wp-content/plugins/buddypress/bp-xprofile/bp-xprofile-template.php on line 986

Also getting: PHP Warning: Attempt to read property "display_name" on bool in /wp-content/plugins/buddypress/bp-messages/classes/class-bp-rest-messages-endpoint.php on line 879.

Thanks.

Change History (13)

#1 @espellcaste
4 months ago

  • Milestone changed from Awaiting Review to 14.0.0

Could you share the REST request performed here? Might be helpful to debug this, although I think I know what it is.

#2 @niftythree
4 months ago

Hi @espellcaste,

Thanks for your reply. We are performing the following request in Postman:

POST /wp-json/buddypress/v1/xprofile/<fieldid>/data/<userid>

{
    "context" : "edit",
    "value" : "I don\\'t travel often"
}

#3 @espellcaste
3 months ago

  • Keywords has-unit-tests needs-testing-info has-dev-note added
  • Priority changed from normal to low
  • Severity changed from normal to minor
  • Version set to 12.4.0

@niftythree Sorry it took me this long to test this. But so far, I'm unable to replicate the issue.

I tested passing the value as such: "I don't travel often" and as suggested by your example: "I don
't travel often". Tested on PHP 8.1 and 8.2.

I don't get the error logged or the PHP warning.

Also getting: PHP Warning: Attempt to read property "display_name" on bool in /wp-content/plugins/buddypress/bp-messages/classes/class-bp-rest-messages-endpoint.php on line 879

The fact that you are getting this warning from the messages endpoint indicates to me there is something else going on in your application that might be affecting your test, but not sure what is based on the information provided.

Can you share more information that might help? Maybe the BuddyPress version, if you are using other plugins, etc. Did you test with the endpoint only and can confirm if it works correctly?

For context, I created a unit test to confirm this is working properly here: https://github.com/buddypress/BP-REST/pull/497

#4 @niftythree
3 months ago

Hi @espellcaste,

No problem, thanks for your reply. Sorry, we should have added we're trying to update a checkbox, but this also happens for the multi-select box too, for example. Updating a text box is OK.

PHP version: 8.1
Theme: Twenty Twenty-Four
BuddyPress Template: Legacy
Plugins: The only plugins we had running were BuddyPress and JSON Basic Authentication (https://github.com/WP-API/Basic-Auth)

Here's some extra information that could assist with the critical error:
We tested this in a clean environment, with a new server, new application and with the latest WordPress (6.5) and BuddyPress (12.4.0). We created a new group and a new field within this group with "Checkboxes" and options ordered in Custom, one being "I don't travel often".

Thanks.

#5 @espellcaste
3 months ago

Thanks! That helped! Confirmed that unserialize doesn't work well when there is quoted strings inside it.

The issue is coming from our bp_unserialize_profile_field function.

This does not only happen when updating a field, but by seeing a field too.

The root of the issue seems to be this line.

It is removing the slash of the quoted string and the PHP unserialize function can't handle the new string properly.

@imath I'd love your take on a possible solution here.

#6 @espellcaste
3 months ago

I added some unit tests to confirm this is a real issue: https://github.com/buddypress/BP-REST/pull/497

Last edited 3 months ago by espellcaste (previous) (diff)

#7 @imath
3 months ago

Hi @espellcaste

Here's my opinion about it!

This very early stripslashes() is there since 1.0. I checked history if this wasn't made on purpose to fix a bug.

We have a lot of filters stripping slashes, so we are probably doing this twice. I'd see what happens if we remove this very early stripslashes() <- probably the nicest solution ;)

The other possibilities I see would be:

  • to do this before saving is_string( $value ) ? wptexturize( $value ) : $value; but I don't think we should do this.
  • to avoid doing this very early stripslashes()

eg: $this->value = is_serialized( $profiledata->value ) ? $profiledata->value : stripslashes( $profiledata->value ); <- probably the less risky!

This ticket was mentioned in PR #268 on buddypress/buddypress by renatonascalves.


3 months ago
#8

  • Keywords has-patch added

#9 @espellcaste
3 months ago

So, while testing this, the last suggestion is the better, and safer, one. Removing stripslashes all together was affecting non-serializable fields, like this example.

#10 @espellcaste
3 months ago

I tested this locally too. Both via the BP REST API and a local site (browser/UI) too.

#11 @imath
3 months ago

  • Keywords commit added

Awesome work @espellcaste on the ticket! Let's go with the safer approach 👍

#12 @espellcaste
3 months ago

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

In 13791:

Strip slashes from a BP_XProfile_ProfileData field value only if the field is serialized.

This change makes sure that XProfile fields with serialized data correctly keeps the string with apostrophes.

Props imath and niftythree
See https://github.com/buddypress/BP-REST/pull/497
Closes https://github.com/buddypress/buddypress/pull/268
Fixes #9127

#13 @espellcaste
3 months ago

The PHP warning mentioned was fixed at #9133. :)

Note: See TracTickets for help on using tickets.