WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

Last modified 4 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 4 months ago.
39380.1.diff (1.2 KB) - added by rachelbaker 4 months ago.
Slice the $data array after the wp_update_comment_data filter

Download all attachments as: .zip

Change History (12)

#1 @dshanske
4 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
4 months ago

#2 @dshanske
4 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
4 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
4 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
4 months ago

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

#6 @rachelbaker
4 months ago

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

@rachelbaker
4 months ago

Slice the $data array after the wp_update_comment_data filter

#7 @rachelbaker
4 months ago

  • Keywords commit added

#8 @dshanske
4 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
4 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
4 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.