Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#7450 closed defect (bug) (fixed)

Problem deleting comment activities

Reported by: Maniou Owned by: r-a-y
Milestone: 2.8.3 Priority: highest
Severity: critical Version: 2.8.0
Component: Activity Keywords: has-patch
Cc:

Description

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,

Attachments (2)

7450.01.patch (2.6 KB) - added by r-a-y 3 years ago.
7450.02.patch (2.7 KB) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (10)

#1 @r-a-y
3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 2.8.2
  • Version changed from 2.8.0 to 1.2

@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 );
Last edited 3 years ago by r-a-y (previous) (diff)

@r-a-y
3 years ago

#2 @r-a-y
3 years ago

  • Version changed from 1.2 to 2.8.0

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.

Last edited 3 years ago by r-a-y (previous) (diff)

@r-a-y
3 years ago

#3 @Maniou
3 years ago

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

#4 @boonebgorges
3 years ago

@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.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


3 years ago

#6 @mercime
3 years ago

  • Owner set to r-a-y
  • Status changed from new to assigned

Per June 7 dev chat

#7 @r-a-y
3 years ago

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

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)

#8 @r-a-y
3 years ago

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)

Note: See TracTickets for help on using tickets.