WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 13 months ago

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

Download all attachments as: .zip

Change History (37)

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

UnitTest

#2 @hnle
13 months ago

thanks @jnylen0, is it ok?

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

@hnle
13 months ago

broken...

#3 @dd32
13 months ago

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

#4 @nacin
13 months ago

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

#5 @dd32
13 months ago

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

#6 @jnylen0
13 months ago

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

@jnylen0
13 months ago

Combined patch; handle author updates; more tests

#7 @jnylen0
13 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
13 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 13 months ago by rachelbaker (previous) (diff)

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

Fix author updates (allow unsetting name and email)

#10 @jnylen0
13 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.

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


13 months ago

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


13 months ago

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


13 months ago

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

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

  • Keywords dev-reviewed added; dev-feedback removed

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

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

#28 @rachelbaker
13 months ago

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

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