REST API: rest_comment_author_required should not allow empty value

[hnle]

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

[jnylen0]

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

• ​WordPress PHPUnit testing docs
• Existing tests for comments endpoint

[hnle]

UnitTest

[hnle]

thanks @jnylen0, is it ok?

[hnle]

broken...

[dd32]

disallow-empty.patch​ + 38971-test2.patch​ Looks good to me.

[nacin]

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

[dd32]

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

[jnylen0]

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

[jnylen0]

Combined patch; handle author updates; more tests

[jnylen0]

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.

[rachelbaker]

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

[jnylen0]

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]

Fix author updates (allow unsetting name and email)

[jnylen0]

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]

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

[rachelbaker]

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

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

[rachelbaker]

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

[joehoyle]

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

[rachelbaker]

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

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?

[rachelbaker]

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

[rachelbaker]

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

[pento]

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

[rachelbaker]

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.

[rachelbaker]

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.

[rachelbaker]

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

[rachelbaker]

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.

[rachelbaker]

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.