Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#38971 closed defect (bug) (fixed)

REST API: rest_comment_author_required should not allow empty value

Reported by: hnle's profile hnle 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 commit dev-reviewed
Focuses: rest-api Cc:

Description

in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-comments-controller.php#L515, it detect that "is the author param has set", but it allow empty value.

require_name_email option is true, and request data like that:

{"author_email":"foo@…","author_url":"","content":"comment","post":"2192","parent":"0"}

it return 400 with rest_comment_author_required, but with empty field like

{"author_email":"foo@…","author_name":"","author_url":"","content":"comment","post":"2192","parent":"0"}

it passed (201 Created).

Attachments (7)

disallow-empty.patch (1.5 KB) - added by hnle 8 years ago.
38971-test.patch (2.6 KB) - added by hnle 8 years ago.
UnitTest
38971-test2.patch (2.6 KB) - added by hnle 8 years ago.
broken…
38971.4.diff (11.8 KB) - added by jnylen0 8 years ago.
Combined patch; handle author updates; more tests
38971.5.diff (11.9 KB) - added by jnylen0 8 years ago.
Fix author updates (allow unsetting name and email)
38971.6.diff (11.1 KB) - added by rachelbaker 8 years ago.
38971.diff (11.0 KB) - added by flixos90 8 years ago.

Download all attachments as: .zip

Change History (37)

@hnle
8 years ago

#1 @jnylen0
8 years ago

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

Nice catch, @hnle. Can you add unit tests for these cases that fail before your patch and succeed after your patch?

@hnle
8 years ago

UnitTest

#2 @hnle
8 years ago

thanks @jnylen0, is it ok?

Last edited 8 years ago by hnle (previous) (diff)

@hnle
8 years ago

broken...

#3 @dd32
8 years ago

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

#4 @nacin
8 years ago

I presume the comment fails to be created even when you get back a 201? Just curious.

#5 @dd32
8 years ago

Nope; Creates the comment, shows up as Anonymous in wp-admin as a fallback.

#6 @jnylen0
8 years ago

This is also an issue with updating comments. Updated patch coming shortly.

@jnylen0
8 years ago

Combined patch; handle author updates; more tests

#7 @jnylen0
8 years ago

In 38971.4.diff:

  • Combine code and tests from @hnle into one patch.
  • Remove duplicate error messages.
  • Handle empty author data on comment update as well as create.
  • Test suite fixes: reset options and rest_allow_anonymous_comments after each test.
  • More tests.

#8 follow-up: @rachelbaker
8 years ago

@jnylen0 Thanks for hopping in here.

I don't think we should honor the require_name_email option on comment update, that option is intended for comment creation only. I intentionally left that logic out of https://github.com/WP-API/WP-API/pull/2861 The discussion setting reads:

Comment author must fill out name and email

After the comment is created, any user with permission to edit the comment in wp-admin (or via the XML-RPC endpoint) can remove an offensive comment author's name or their email (which would also prevent the associated gravatar from displaying). I don't think we should act any differently.

Version 0, edited 8 years ago by rachelbaker (next)

#9 in reply to: ↑ 8 @jnylen0
8 years ago

Replying to rachelbaker:

After the comment is created, any user with permission to edit the comment in wp-admin (or via the XML-RPC endpoint) can remove an offensive comment author's name or their email (which would also prevent the associated gravatar from displaying). I don't think we should act any differently. Executing this isn't straightforward due to how the schema validates email properties, but I will create a new ticket as this is a separate issue.

Hmm, ok, thanks for the background info. I'll update the patch here accordingly.

@jnylen0
8 years ago

Fix author updates (allow unsetting name and email)

#10 @jnylen0
8 years ago

I went ahead and fixed the issue with unsetting author email also in 38971.5.diff since the required logic and test logic are pretty closely intertwined with the original issue.

@rachelbaker
8 years ago

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


8 years ago

#12 @rachelbaker
8 years ago

Just a few minor changes in 38971.6.diff:

  • retains the rest_comment_author_data_required error code
  • changes the callback method name to check_comment_author_email() to be more explicit
  • minor docblock wording changes

#13 @rachelbaker
8 years ago

  • Keywords dev-feedback added

+1 for commit of 38971.6.diff. Needs dev-reviewed

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


8 years ago

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


8 years ago

#16 @rachelbaker
8 years ago

  • Summary changed from rest_comment_author_required should not allow empty value to REST API: rest_comment_author_required should not allow empty value

#17 follow-up: @joehoyle
8 years ago

Is validate_callback' => null, // skip built-in validation of 'email'. needed? The validation should actually be hooked via the sanitize_callback now the sanitize callback can return a WP_Error. Nothing should default to validate_callback, see https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api.php#L858.

#18 @rachelbaker
8 years ago

@joehoyle but because the field type is email (which has built-in schema validation) the validation callback still runs, so if you try to pass '' for author_email you get a rest_invalid_param error.

#19 @joehoyle
8 years ago

@rachelbaker hmm ok - I'm going to pull this down and see what the deal is, because my understanding is that the validation should only happen via the sanitize_callback.

#20 @rachelbaker
8 years ago

@joehoyle This is caused by WP_REST_Server->dispatch() calling WP_REST_Request->has_valid_params().
See: https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-server.php#L865

@flixos90
8 years ago

#21 in reply to: ↑ 17 @flixos90
8 years ago

Minor test change in 38971.diff: The tearDown() method of the test class has been removed since it's not necessary as database transactions as well as filters are reset automatically.

Replying to joehoyle:

Is validate_callback' => null, // skip built-in validation of 'email'. needed? The validation should actually be hooked via the sanitize_callback now the sanitize callback can return a WP_Error. Nothing should default to validate_callback, see https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api.php#L858.

I have a hard time understanding this: The function check_comment_author_email() does validation only, why should it be handled via sanitize_callback? It's possible of course, but I don't think it logically makes sense. If we wanna use sanitize_callback for validation as well, what would be the point of validate_callback?

#22 @rachelbaker
8 years ago

  • Keywords commit dev-feedback removed

Removed the commit keyword added by @dd32 above now that the patch has gone through several iterations.

#23 @rachelbaker
8 years ago

  • Keywords commit dev-feedback added

38971.diff Looks ready for commit. We are using the sanitize_callback() because we are kind of combining the sanitization and validation logic here (see casting to value as a string).

#24 @pento
8 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Agreed, I'm fine with it being on sanitize_callback.

#25 @rachelbaker
8 years ago

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

In 39440:

REST API: Fix handling of some orderby parameters for the Posts controller.

  • 'orderby' => 'include' requires an array of post_ids via the include collection param.

'orderby' => 'id' and 'orderby' => 'slug' need map the correct WP_Query equivalents.

Props flixos90, hnle, dd32, rachelbaker, joehoyle, pento.

Fixes #38971.

#26 @rachelbaker
8 years ago

In 39441:

REST API: Fix handling of some orderby parameters for the Posts controller.

  • 'orderby' => 'include' requires an array of post_ids via the include collection param.
  • 'orderby' => 'id' and 'orderby' => 'slug' need map the correct WP_Query equivalents.

Merges [39440] to the 4.7 branch.

Props flixos90, hnle, dd32, rachelbaker, joehoyle, pento.
Fixes #38971 for 4.7.

#27 @rachelbaker
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#28 @rachelbaker
8 years ago

Commits [39440] and [39441] were intended to close #38985 :(

#29 @rachelbaker
8 years ago

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

In 39444:

REST API: Fix bug where comment author and author email could be an empty string when creating a comment.

If the require_name_email option is true, creating a comment with an empty string for the author name or email should not be accepted. Both values can be an empty string on update.

Props flixos90, hnle, dd32, rachelbaker, jnylen0, ChopinBach, joehoyle, pento.

Fixes #38971.

#30 @rachelbaker
8 years ago

In 39446:

REST API: Fix bug where comment author and author email could be an empty string when creating a comment.

If the require_name_email option is true, creating a comment with an empty string for the author name or email should not be accepted. Both values can be an empty string on update.

Merges [39444] into the 4.7 branch.
Props flixos90, hnle, dd32, rachelbaker, jnylen0, ChopinBach, joehoyle, pento.

Fixes #38971 for 4.7.

Note: See TracTickets for help on using tickets.