Make WordPress Core

Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#43177 closed defect (bug) (fixed)

REST API allows empty comments containing only whitespace

Reported by: jaswrks's profile jaswrks Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.1
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

I can POST the following to the REST API endpoint for comments and it slides through so long as it's not an empty string. Just adding a single space results in an empty comment.

{
	"post": 1,
	"content": " "
}

I suggest trim() empty check. Or, a more robust alternative: https://gist.github.com/jaswrks/d662f4ba8b379e7602c8f80f7b1bb82e

Attachments (2)

43177.patch (2.4 KB) - added by jaswrks 6 years ago.
Trim comment content before checking if empty.
43177.2.patch (2.5 KB) - added by jaswrks 6 years ago.
Take 2, use '' === trim(

Download all attachments as: .zip

Change History (17)

#1 @rmccue
6 years ago

For frontend submissions, this is usually handled by wp_handle_comment_submission, which trim()s author name, author URL, author email, and content. Likewise, wp_ajax_replyto_comment() trims the content.

However, XML-RPC does not trim the content, so it's possible to submit a whitespace-only string there.

Since creating a comment is an authenticated-only endpoint out-of-the-box, I think the current behaviour is fine; if an authenticated user really wants to submit empty content, they should be able to.

#2 @jaswrks
6 years ago

Thanks for the reconnaissance with respect to wp_handle_comment_submission() and the AJAX handler. I noticed this also and considered not opening this ticket when I reviewed tickets from the past that mentioned this.

However, I respectfully disagree that it should be possible to submit a whitespace-only comment via the REST API. The code, as it exists now, intends to prevent an empty comment from being submitted. The problem, as I see it, is not a matter of whether this should be allowed or not, because it's already the case that an empty comment should not be allowed. The problem is that the existing code fails to do this.

For example, I can submit a string that contains nothing but whitespace, and it accepts this, and then returns an object in the response with the content having been trimmed. So in fact it is an empty comment. The existing code just needs to trim before accepting the submission that really is just an empty string in the eyes of other core code.

#3 @dd32
6 years ago

FWIW, I think it makes sense to enforce trim()'ing the fields in both XML-RPC and the REST API, plus in wp_ajax_replyto_comment() if any of the values there aren't.

@jaswrks
6 years ago

Trim comment content before checking if empty.

#4 @jaswrks
6 years ago

My patch covers all three of the areas mentioned. Please let me know if it needs any further changes.

#5 @rmccue
6 years ago

I'd prefer '' === trim( $x ) over ! trim( $x ) just to be explicit, but I think it looks good otherwise.

@jaswrks
6 years ago

Take 2, use '' === trim(

#6 @jaswrks
6 years ago

@rmccue Thank you for reviewing :-)
Most recent patch uses '' === as you suggested.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-restapi by schlessera. View the logs.


6 years ago

#9 @schlessera
6 years ago

  • Keywords needs-unit-tests has-patch added
  • Milestone changed from Awaiting Review to Future Release

#10 @rachelbaker
6 years ago

  • Keywords needs-refresh added

In 43177.2.patch I don't see the need for the edit on ajax-actions.php since trim is already called on $comment-content see: https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/ajax-actions.php#L1137

#11 @TimothyBlynJacobs
3 years ago

#49903 was marked as a duplicate.

This ticket was mentioned in PR #657 on WordPress/wordpress-develop by TimothyBJacobs.


3 years ago
#12

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

#13 @TimothyBlynJacobs
3 years ago

  • Milestone changed from Future Release to 5.6

#14 @TimothyBlynJacobs
3 years ago

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

In 49303:

REST API, XML-RPC: Synchronise empty comment content checks.

The REST API and XML-RPC now uses the same detection methodology for empty comment content as wp_handle_comment_submission(). Specifically, comments now have their content trimmed and '0' is allowed.

Props jaswrks, rmccue, dd32, rachelbaker, Cawa-93, aduth, TimothyBlynJacobs.
Fixes #43177.

TimothyBJacobs commented on PR #657:


3 years ago
#15

Fixed in 1e030c4062f87b3881012d977a9adc3ade9c63b9.

Note: See TracTickets for help on using tickets.