Skip to:
Content

BuddyPress.org

Opened 6 months ago

Closed 5 months ago

#8236 closed defect (bug) (fixed)

`empty()` check for activity content after kses filtering

Reported by: oztaser Owned by: imath
Milestone: 6.0.0 Priority: normal
Severity: normal Version:
Component: Activity Keywords: has-patch
Cc:

Description

The BP_Activity_Activity()->save() method inserts an activity with empty content If the whole activity content has not allowed HTML elements. Because bp_activity_filter_kses returns an empty value.

I think we should check the activity content before running update/insert query.

Attachments (3)

8236.patch (569 bytes) - added by oztaser 6 months ago.
8236.1.patch (1.2 KB) - added by oztaser 6 months ago.
8236.2.patch (4.2 KB) - added by imath 5 months ago.

Download all attachments as: .zip

Change History (11)

@oztaser
6 months ago

#1 @imath
6 months ago

Hi @oztaser, thanks for your suggestion and patch, I believe we are using activities with no content to trace things like the last time the user connected to the site or when a group is created.

I’ll need to check. I believe this check exists earlier in the process when someone posts from the activity post form.

#2 @oztaser
6 months ago

Hi @imath, thank you for your response. I checked and I found last_activity type with no content as you mention. It looks like we can not add a control for all types. But an empty activity_update looks like a problem to me, at least. Because if users post just an iframe, an empty activity is inserted. There is an empty check for activity_update type on bp_activity_post_update function but the content is not filtered there. Maybe we can filter not allowed HTML elements before checking content. Of course, you think this is an issue. I fixed my problem by using bp_activity_before_save action.

I have another suggestion, we must pass a message parameter for bp_activity_missing_component and bp_activity_missing_type errors. Because It's a required parameter for WP_Error->add(). I am not quite sure about what message should return so I am not preparing the patch.

Thanks again.

#3 @imath
6 months ago

You're welcome :) what about adding a check to the activity type ? eg:

if ( 'activity_update' === $this->type && ! $this->content ) {}

@oztaser
6 months ago

#4 @oztaser
6 months ago

Hi again @imath , What do you think about 8236.1.patch?

#5 @imath
6 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 6.0.0

Hi! This is shaping great, I think I would do the check a bit later, just after the statement about component and type:

if ( empty( $this->component ) || empty( $this->type ) )

This way you are sure about the fact type is not empty before checking it’s not activity_update.

Thanks a lot for your work on it 👌

@imath
5 months ago

#6 @imath
5 months ago

  • Keywords reporter-feedback added

In 8236.2.patch :

  • I'm checking the content once we are sure the type is set as explained in my previous comment.
  • I'm introducing a filter bp_activity_type_requires_content to allow plugins to make their activity type's content required.
  • I'm adding unit tests.

@oztaser could you test the patch before the end of march to confirm it's fixing your issue so that we can eventually include this in the 6.0.0 first beta release?

#7 @oztaser
5 months ago

  • Keywords reporter-feedback removed

Hi @imath, your patch looks good and fixed the problem. Thanks a lot for your time!

#8 @imath
5 months ago

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

In 12604:

Make sure activities requiring content are not inserted empty into db

The activity type activity_update requires a content. If it is not set (eg: this can happen after sanitization), the activity won't be inserted into the database. Plugin developers can use the bp_activity_type_requires_content filter to force the activity type their plugin generated to also require content.

Props oztaser

Fixes #8236

Note: See TracTickets for help on using tickets.