Make WordPress Core

Opened 14 years ago

Closed 7 years ago

#14452 closed defect (bug) (fixed)

Duplicate check for comments: Inappropriate errorhandling for xmlrpc

Reported by: mrutz's profile mrutz Owned by: johnbillion's profile johnbillion
Milestone: 4.7 Priority: normal
Severity: normal Version: 3.0
Component: XML-RPC Keywords: has-patch commit has-unit-tests
Focuses: Cc:

Description

The duplicate check for comments just dies in case a dulicate is detected(see wp-includes/comment.php around line 550). This ends up with a html page delivered in any cases. When accessing through xmlrpc this leads to the problem that this html page is send back to the xmlrpc client instead of a proper xmlrpc formated message.

Attachments (3)

14452.diff (735 bytes) - added by websupporter 7 years ago.
14452.2.diff (793 bytes) - added by websupporter 7 years ago.
But this time it should be ok :)
14452.3.diff (2.1 KB) - added by markoheijnen 7 years ago.

Download all attachments as: .zip

Change History (16)

#1 @nacin
13 years ago

  • Component changed from Comments to XML-RPC
  • Keywords needs-patch 2nd-opinion added; xmlrpc comment duplicate removed
  • Milestone changed from Awaiting Review to 3.1

We make a DOING_AJAX check there. I would want to add in an XMLRPC check, but that seems lame. Maybe we can drop-in an action there, that XMLRPC can then intercept?

#2 @mrutz
13 years ago

Looking at the code I see that there is already the action 'comment_duplicate_trigger'. I don't know exactly, but I would assume that the XMLRPC code should attach to this and handle the duplicate appropriate. Or am I missing something?

If I'm completely wrong, I still would opt for the action as it would be prepared for future upcoming protocols too.

#3 @nacin
13 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from 3.1 to Future Release

Indeed, exactly. We'd want to do this for APP requests too.

Of note, there's also comment_flood_trigger that has the same wp_die().

This isn't a regression, so I'm moving this to future release.

#4 @solarissmoke
13 years ago

  • Keywords dev-feedback added

Looking at the code I see that there is already the action 'comment_duplicate_trigger'. I don't know exactly, but I would assume that the XMLRPC code should attach to this and handle the duplicate appropriate.

I might be missing something obvious, but I don't think this is possible. IXR_Server calls the wp_newComment() function when that method is invoked. Now, the only way to get a proper error message sent back to the client is to have wp_newComment return an IXR_Error object. Hooking into comment_duplicate_trigger doesn't help because it's an action and not a filter. So you can't use it to tell wp_newComment() what to return.

It seems to me that we need something like DOING_AJAX to get this to work. Or just replicate the check separately in the XML-RPC class (and APP, etc).

#5 @dd32
13 years ago

Hopefully implementing some of #16979 will allow the duplicate comment check to be handled properly for xml-rpc

#6 @markoheijnen
12 years ago

Shouldn't we change the function wp_allow_comment() to not use the function die()/wp_die(). Makes more sense that the function what calles wp_allow_comment() to do that.

#7 @mdgl
9 years ago

  • Keywords has-patch added; needs-patch removed

Patch uploaded to #18630 that follows markoheijnen's suggestion and would allow APIs such as XML-RPC, JSON and others to add comments safely and with reasonable error handling.

#8 @mdgl
9 years ago

  • Keywords needs-patch added; has-patch removed

Still needs patch to XML-RPC handler.

#9 @chriscct7
8 years ago

  • Keywords needs-unit-tests added; dev-feedback removed

@websupporter
7 years ago

#10 @websupporter
7 years ago

  • Keywords has-patch added; needs-patch removed

With https://core.trac.wordpress.org/ticket/36901 in core, wp_new_comment() returns the comment ID, false or a WP_Error. <strike>14452.diff takes this into account.</strike>

Ah sorry, my patch works on wp_trackback.phpnot the general XMLRPC. Sorry! But wp_new_comment() has changed. It should be easily changeable now :)

Last edited 7 years ago by websupporter (previous) (diff)

@websupporter
7 years ago

But this time it should be ok :)

@markoheijnen
7 years ago

#11 @markoheijnen
7 years ago

  • Keywords commit has-unit-tests added; needs-unit-tests removed

Uploaded a new patch with one unit tests testing for duplicated comment. That is the only error we can get as others aren't been checked for as wp_handle_comment_submission() is not being used.

Maybe we do need to check in the XML-RPC for an empty comment?

#12 @johnbillion
7 years ago

  • Milestone changed from Future Release to 4.7
  • Owner set to johnbillion
  • Status changed from new to reviewing

#13 @johnbillion
7 years ago

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

In 39045:

XML-RPC: Correctly handle empty and duplicate comments.

This prevents wp_die() being sent in response to an XML-RPC call that attempts to submit a duplicate comment, and correctly returns an error in response to an attempt to submit an empty comment.

Props markoheijnen, websupporter.
Fixes #14452, #38466.
See #36901

Note: See TracTickets for help on using tickets.