Missing validation while posting comment via REST API

[mangeshp]

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" http://your.host/wp-json/wp/v2/comments

[mangeshp]

Here is the patch for the proposed changes in this ticket

[joehoyle]

cc @rachelbaker

[dd32]

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

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

[mangeshp]

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 ) )

[mangeshp]

New changes

[rachelbaker]

Related GH issue: ​https://github.com/WP-API/WP-API/issues/2785
Related GH PR: ​https://github.com/WP-API/WP-API/pull/2858

[rachelbaker]

@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: ​https://github.com/WP-API/WP-API/pull/2858 that also included unit tests, but needed to be converted to a Trac patch.

[mangeshp]

@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.

[mangeshp]

Updated changes as per the feedback given

[mangeshp]

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

[pento]

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.

[rachelbaker]

Introduce wp_check_comment_data_max_lengths()

[rachelbaker]

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.

[pento]

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.

[rachelbaker]

@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.

[pento]

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.

[rachelbaker]

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.