#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)
Change History (37)
#1
@
8 years ago
- Keywords has-patch needs-unit-tests added
- Milestone changed from Awaiting Review to 4.7
#3
@
8 years ago
- Keywords has-unit-tests commit added; needs-unit-tests removed
disallow-empty.patch + 38971-test2.patch Looks good to me.
#4
@
8 years ago
I presume the comment fails to be created even when you get back a 201? Just curious.
#7
@
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:
↓ 9
@
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.
#9
in reply to:
↑ 8
@
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.
#10
@
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.
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
8 years ago
#12
@
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
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
@
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:
↓ 21
@
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
@
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
@
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
@
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
#21
in reply to:
↑ 17
@
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 thesanitize_callback
now the sanitize callback can return a WP_Error. Nothing should default tovalidate_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
@
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
@
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
@
8 years ago
- Keywords dev-reviewed added; dev-feedback removed
Agreed, I'm fine with it being on sanitize_callback
.
Nice catch, @hnle. Can you add unit tests for these cases that fail before your patch and succeed after your patch?