WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 11 months ago

Last modified 11 months ago

#39380 closed defect (bug) (fixed)

wp_update_comment can cause database error with new filter

Reported by: dshanske Owned by: rachelbaker
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.6
Component: Comments Keywords: has-patch commit
Focuses: Cc:

Description (last modified by dshanske)

In 4.6, the comment data array was passed into the edit_comment action. In 4.7, the wp_update_comment_data filter was added to wp_update_comment.

However, wp_array_slice_assoc is called to filter keys out of the array before it is passed to both.

It should be called after and only to filter the data going into the database. By adding a key to the array at the filter you can cause a database error.

By removing extra keys from the data before it hits edit_comment, it also makes it impossible to pass data to be saved as metadata.

Attachments (2)

39380.patch (1.5 KB) - added by dshanske 11 months ago.
39380.1.diff (1.2 KB) - added by rachelbaker 11 months ago.
Slice the $data array after the wp_update_comment_data filter

Download all attachments as: .zip

Change History (12)

#1 @dshanske
11 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 4.7.1
  • Summary changed from wp_update_comment strips data from passed array to wp_update_comment can cause database error with new filter

@dshanske
11 months ago

#2 @dshanske
11 months ago

  • Keywords has-patch added

The patch submitted not only makes it so the database error cannot occur which is the more important issue, but preserves any data added to the comment array for the filter and hook.

#3 @rachelbaker
11 months ago

  • Milestone changed from 4.7.1 to 4.8
  • Type changed from defect (bug) to enhancement

@dshanske Prior to [38674] the sliced data was passed to the database update statement and then the edit_comment action, this doesn't look like a regression to me. Am I missing something?

I understand that you are approaching this with a specific use-case in mind (updating meta). However, I am not convinced other folks would expect the $data parameter in the edit_comment action to differ from the specific keys/values used to update the comment table.

#4 @dshanske
11 months ago

@rachelbaker the fact that the filter could add keys to the array and cause a database error when the update is run is still an issue. I tagged it for 4.7.1 for that reason. The slice still should come after the filter for that reason.

Should I split the ticket because I consider that a bug in the filter? I just figured I would address both.

#5 @rachelbaker
11 months ago

  • Milestone changed from 4.8 to 4.7.1
  • Type changed from enhancement to defect (bug)

#6 @rachelbaker
11 months ago

@dshanske Yeah, can you create a new ticket for the enhancement? And I will get the bugfix in 4.7.1

@rachelbaker
11 months ago

Slice the $data array after the wp_update_comment_data filter

#7 @rachelbaker
11 months ago

  • Keywords commit added

#8 @dshanske
11 months ago

@rachelbaker Will hold off on a new ticket as #36784 is tagged for 4.8 early and addresses the same use case.

#9 @rachelbaker
11 months ago

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

In 39640:

Comments: Fix placement of the wp_update_comment_data filter to safeguard filtered data from triggering a database error.

Introduced in [38674], the wp_update_comment_data filter took place after the $data was sliced and prepared for the database update statement. The location of the filter assumed the result of anyone applying it would not change the data type or make structural modifications or additions to the $data array. 😅

This moves the wp_update_comment_data filter to take place before the $data is sliced and prepared for the database update statement.

Props dshanske for initial patch.

Fixes #39380.

#10 @rachelbaker
11 months ago

In 39641:

Comments: Fix placement of the wp_update_comment_data filter to safeguard filtered data from triggering a database error.

Introduced in [38674], the wp_update_comment_data filter took place after the $data was sliced and prepared for the database update statement. The location of the filter assumed the result of anyone applying it would not change the data type or make structural modifications or additions to the $data array. 😅
This moves the wp_update_comment_data filter to take place before the $data is sliced and prepared for the database update statement.

Merges [39640] to the 4.7 branch.

Props dshanske for initial patch.
Fixes #39380.

Note: See TracTickets for help on using tickets.