WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#38971 closed defect (bug) (fixed)

REST API: rest_comment_author_required should not allow empty value

Reported by: hnle Owned by: 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 9 months ago.
38971-test.patch (2.6 KB) - added by hnle 9 months ago.
UnitTest
38971-test2.patch (2.6 KB) - added by hnle 9 months ago.
broken…
38971.4.diff (11.8 KB) - added by jnylen0 9 months ago.
Combined patch; handle author updates; more tests
38971.5.diff (11.9 KB) - added by jnylen0 9 months ago.
Fix author updates (allow unsetting name and email)
38971.6.diff (11.1 KB) - added by rachelbaker 9 months ago.
38971.diff (11.0 KB) - added by flixos90 9 months ago.

Download all attachments as: .zip

Change History (37)

#1 @jnylen0
9 months 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
9 months ago

UnitTest

#2 @hnle
9 months ago

thanks @jnylen0, is it ok?

Last edited 9 months ago by hnle (previous) (diff)

@hnle
9 months ago

broken...

#3 @dd32
9 months ago

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

#4 @nacin
9 months ago

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

#5 @dd32
9 months ago

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

#6 @jnylen0
9 months ago

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

@jnylen0
9 months ago

Combined patch; handle author updates; more tests

#7 @jnylen0
9 months 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
9 months 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. 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.

Last edited 9 months ago by rachelbaker (previous) (diff)

#9 in reply to: ↑ 8 @jnylen0
9 months 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
9 months ago

Fix author updates (allow unsetting name and email)

#10 @jnylen0
9 months 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
9 months ago

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


9 months ago

#12 @rachelbaker
9 months 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
9 months 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.


9 months ago

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


9 months ago

#16 @rachelbaker
9 months 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
9 months 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
9 months 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
9 months 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
9 months 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
9 months ago

#21 in reply to: ↑ 17 @flixos90
9 months 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
9 months 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
9 months 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
9 months ago

  • Keywords dev-reviewed added; dev-feedback removed

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

#25 @rachelbaker
9 months 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
9 months 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
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#28 @rachelbaker
9 months ago

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

#29 @rachelbaker
9 months 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
9 months 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.