Problem deleting comment activities

[Maniou]

Hello,

First i'm not a dev and my english is not perfect but i'm going to explain the best i can the problem since the last maj 2.8.1.

The problem : when i'm trying to detele a comment activity that doesn't exist, it's deleting from the database all the comments with type = "activity_comment".

On the activity feed when I press the "delete" button if this activity have been already (for example another browser instance) it's going to delete all the activity_comment from the database (just before an error output).

When deleting an activity that doesn't exist, this is what happen :

File : /buddypress/bp-templates/bp-legacy/buddypress-functions.php
Function : bp_legacy_theme_delete_activity_comment()

$_POST['id'] = my id that doesn't exist

$comment = new BP_Activity_Activity( $_POST['id'] );

$comment output is equal to :

BP_Activity_Activity Object
(
    [id] => 0
    [item_id] => 
    [secondary_item_id] => 
    [user_id] => 
    [primary_link] => 
    [component] => 
    [type] => 
    [action] => 
    [content] => 
    [date_recorded] => 
    [hide_sitewide] => 0
    [mptt_left] => 
    [mptt_right] => 
    [is_spam] => 
    [errors] => WP_Error Object
        (
            [errors] => Array
                (
                )

            [error_data] => Array
                (
                )

        )

    [error_type] => bool
)

There is no check on $comment->id = 0 in order to exit.

I have to add this code to fix the problem, but i guess it's not the best way to do it :

	if ($comment->id == 0)
		exit( '-1<div id="message" class="error bp-ajax-message"><p>' . __( 'Comment doesn\'t exist.', 'buddypress' ) . '</p></div>' );

Also, i don't know if it's related but i didn't find any reference to "wp_ajax_delete_activity_comment" in the 2.8.1 version.

Thank you,

[r-a-y]

@Maniou - Thanks for reporting this important bug.

I can confirm the problem. It's due to bp_activity_delete_children() (and less importantly, bp_activity_delete_comment()) not doing any empty checks against the passed $activity_id.

This doesn't appear to be a regression from v2.8.0, but I think it's important enough to fix in a 2.8.x release.

I've attached a patch that should fix the problem.

In the meantime before this fix is added, you could add this hotfix to your theme's functions.php or ​wp-content/plugins/bp-custom.php:

// Hotfix for activity comment deletion bug - #7450
add_filter( 'bp_activity_delete_comment_pre', function( $retval, $activity_id ) {
    if ( empty( $activity_id ) ) {
        return false;
    }

    return $retval;
}, 10, 2 );

[r-a-y]

Actually, this bug is related to v2.8.0 due to r11312 (see #7394).

r11312 changed non-existent activity ID lookups to always be set to activity ID 0. So if you did something like this:

$a = new BP_Activity_Activity( 999999 );

$a->id now equals 0 instead of 999999, which is logistically correct, but activity ID 0 triggers the bug that Maniou reported above.

02.patch adds some logic to bp_activity_delete_comment() to check the validity of the passed $comment_id before continuing.

[Maniou]

Thanks @r-a-y for your reactivity and all your work on this great plugin !

[boonebgorges]

@Maniou For someone who is "not a dev", this is an excellent bug report. Thank you :-D

@r-a-y Good find - thanks for tracking it down. The approach in 7450.02.patch​ looks good to me.

[mercime]

Per June 7 dev chat

[r-a-y]

In 11583:

Activity: Fix activity comment deletion when comment ID equals 0.

In #7394, non-existent activity ID lookups now default to zero, instead of
the ID that was passed. For example, $a = new BP_Activity_Activity( 99 ),
$a->id now equals 0 instead of 99.

This is logistically correct; however, this raised a bug with
bp_activity_delete_comment() when passing 0 as the comment ID, which
could potentially delete all activity comments in the database.

Props Maniou.

Fixes #7450 (trunk)

[r-a-y]

In 11584:

Activity: Fix activity comment deletion when comment ID equals 0.

In #7394, non-existent activity ID lookups now default to zero, instead of
the ID that was passed. For example, $a = new BP_Activity_Activity( 99 ),
$a->id now equals 0 instead of 99.

This is logistically correct; however, this raised a bug with
bp_activity_delete_comment() when passing 0 as the comment ID, which
could potentially delete all activity comments in the database.

Props Maniou.

Fixes #7450 (2.8-branch)