Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#38477 closed defect (bug) (fixed)

Missing validation while posting comment via REST API

Reported by: mangeshp's profile mangeshp Owned by: rachelbaker's profile rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests dev-feedback
Focuses: Cc:


I am able to post invalid comment/data via REST API. Validations should be there while posting a comment like the validations are done while posting a comment from the comment form.

For example, I am able to post a@b.c as an author email successfully. Which is wrong. Also, the maximum allowed content length ( at least length of a comment_content, comment_author, comment_author_email, comment_author_url ) should be checked while posting the comment and proper error messages should be given in response to such errors.

For instance, the following request will create the successful comment,

curl -i -X POST -d '{"post":"1","content":"your comment content","author_name":"your name","author_email":"a@b.c"}' -H 'Accept: application/json' -H "Content-Type: application/json"

Attachments (4)

38477.diff (3.3 KB) - added by mangeshp 7 years ago.
Here is the patch for the proposed changes in this ticket
38477-1.diff (3.3 KB) - added by mangeshp 7 years ago.
New changes
38477.2.diff (3.6 KB) - added by mangeshp 7 years ago.
Updated changes as per the feedback given
38477.3.diff (10.9 KB) - added by rachelbaker 7 years ago.
Introduce wp_check_comment_data_max_lengths()

Download all attachments as: .zip

Change History (22)

7 years ago

Here is the patch for the proposed changes in this ticket

#1 @mangeshp
7 years ago

  • Keywords has-patch added

#2 @joehoyle
7 years ago

  • Milestone changed from Awaiting Review to 4.7

cc @rachelbaker

#3 follow-up: @dd32
7 years ago

38477.diff should use > rather than < to improve readability.

if ( strlen( $something ) > $max_length ) is more readable than if ( $max_length < strlen( $something ) )

#4 in reply to: ↑ 3 @mangeshp
7 years ago

Made the changes as per the suggestion and submitted the new patch.

Replying to dd32:

38477.diff should use > rather than < to improve readability.

if ( strlen( $something ) > $max_length ) is more readable than if ( $max_length < strlen( $something ) )

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

7 years ago

New changes

#6 @rachelbaker
7 years ago

  • Keywords needs-refresh added

@mangeshp Thank you for the patches.

a@b.c is a valid email according to the RFC. We already check is_email() with rest_validate_request_arg() so there is no need to duplicate the logic here.

In your patch it looks like you are only checking the lengths of values when a comment is created AND only if the require_name_email option is enabled. It would be better to move the string length checks into the prepare_item_for_database() method so we can check lengths on update actions as well.

@salcode was already working on a patch via Github here: that also included unit tests, but needed to be converted to a Trac patch.

#7 @mangeshp
7 years ago

@rachelbaker Thank you for the feedback.

I checked the pull request and also the feedback you have given on the GitHub repo.

I would like to point out two major things from the PR

Following is the sample code from PR:

return new WP_Error( 'comment_author_column_length', __( '<strong>ERROR</strong>: your name is too long.' ), array( 'status' => 400 ) );

You can see that comment_author_column_length is not following the same pattern as other parameters we are using in all other classes. It should be like this rest_comment_author_column_length. Also, HTML entities like <strong> should not be there in error messages. The error messages should be plain text in API.

It's better to move the string length checks into the prepare_item_for_database() and also the check should be done in create_item(), because the filter rest_preprocess_comment in prepare_item_for_database() may send the incorrect values in $prepared_comment. Which need to be checked once again.

Let me know your opinion the above points.

7 years ago

Updated changes as per the feedback given

#8 @mangeshp
7 years ago

Hi @rachelbaker,
Can you please take a look at this ticket #38506 (regarding the length of email ID). Would appreciate your thought on it.

#9 @jbpaul17
7 years ago

  • Keywords needs-refresh removed

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.

7 years ago

#11 @pento
7 years ago

I'm starting to think it's worth the effort of merging the sanity checking in wp_handle_comment_submission() and WP_REST_Comments_Controller. Between this and #38506, there's a bit too much logic duplication going on that I really don't want us to have to maintain.

#12 @rachelbaker
7 years ago

  • Owner set to rachelbaker
  • Status changed from new to assigned

7 years ago

Introduce wp_check_comment_data_max_lengths()

#13 @rachelbaker
7 years ago

  • Keywords has-unit-tests added

In 38477.3.diff I took @pento's advice and abstracted the max lengths check from wp_handle_comment_submission() into a new function wp_check_comment_data_max_lengths().

I use wp_check_comment_max_lengths() to check the string lengths of the comment content, author name, author url, and author email against the maximum size of their respective database columns when a comment is created and when a comment is being updated. I am not committed to the function name, or the return values of this function.

I also have unit tests for each scenario included in the patch.

@pento or @dd32 can you review this when you have a chance? I agree that there is more we can abstract from wp_handle_comment_submission() to reduce duplication within the REST API, this just seemed like an obvious starting place.

##Sidenote: In core we only check the maximum lengths of these fields when a comment is created, I will open a new ticket to also check on update.

#14 @rachelbaker
7 years ago

  • Keywords dev-feedback added

#15 @pento
7 years ago

Nice! This is the direction I was thinking.

For the REST API, is it valuable/possible to return all of the the fields that are too long? It'd probably be valuable to do the same for wp_handle_comment_submission(), but that's getting out of scope of this ticket.

#16 @rachelbaker
7 years ago

@pento It would be nice and I did think about that. Multiple maximum length fields - how often would that happen? I am not convinced it is worth the special handling.

#17 @pento
7 years ago

I agree that it'd be relatively rare. I'm inclined to not handle that special case, given that I'm not aware of it ever being reported against wp_handle_comment_submission() behaviour.

#18 @rachelbaker
7 years ago

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

In 39101:

REST API: Return an error when the length of a comment field is too long.

Introduces wp_check_comment_data_max_lengths() which allows both the REST API comments endpoints and wp_handle_comment_submission() to check the length of the comment content, author name, author url, and author email fields against their respective database columns.

Props rachelbaker, mangeshp, salcode, pento.
Fixes #38477.

Note: See TracTickets for help on using tickets.