Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38820 closed defect (bug) (fixed)

REST API: Clients should not be allowed to set arbitrary comment_type's

Reported by: dd32's profile dd32 Owned by: rachelbaker's profile rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

The Comments API currently allows unauthed users to create comments with any comment_type, this includes 'comment' (which internally is sanitized to ''), 'pingback', and 'foobar'/'custom_internal_comment_type'.

The comments API should only allow a user to create a standard comment, anything other than that should be done through another specialised endpoint, or by a plugin enabling the extra comment_type ability.

Attachments (3)

38820.diff (2.6 KB) - added by dd32 7 years ago.
38820.2.diff (2.3 KB) - added by boonebgorges 7 years ago.
38820.3.diff (2.5 KB) - added by rachelbaker 7 years ago.
Only allow comments to be created with a comment type of comment, otherwise return error.

Download all attachments as: .zip

Change History (12)

@dd32
7 years ago

#1 follow-up: @dd32
7 years ago

I'll also add that I'm not sure a user with moderate_comments capability should be allowed to set this field either, but followed that inline with what the other fields in the API endpoint require. I'd be all for rejecting all requests which attempted to set it (unless a plugin had allowed it somehow).

#2 in reply to: ↑ 1 ; follow-up: @boonebgorges
7 years ago

Replying to dd32:

I'll also add that I'm not sure a user with moderate_comments capability should be allowed to set this field either, but followed that inline with what the other fields in the API endpoint require. I'd be all for rejecting all requests which attempted to set it (unless a plugin had allowed it somehow).

This seems right to me with respect to updates. I don't think there's precedent elsewhere in core for allowing 'moderate_comments' users, or anyone, to change comment types. And in fact, update_item() already disallows the changing of comment types. If this isn't about permissions (and I don't think it is) then the check probably belongs in create_item(). See 38820.2.diff.

Is the intent to support 'trackback' and 'pingback' creation via the core endpoint? @dd32 your patch hardcoded 'comment' only, but my patch includes all three core types.

@boonebgorges
7 years ago

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


7 years ago

#4 in reply to: ↑ 2 @dd32
7 years ago

Replying to boonebgorges:

Replying to dd32:

I'll also add that I'm not sure a user with moderate_comments capability should be allowed to set this field either, but followed that inline with what the other fields in the API endpoint require. I'd be all for rejecting all requests which attempted to set it (unless a plugin had allowed it somehow).

This seems right to me with respect to updates. I don't think there's precedent elsewhere in core for allowing 'moderate_comments' users, or anyone, to change comment types. And in fact, update_item() already disallows the changing of comment types. If this isn't about permissions (and I don't think it is) then the check probably belongs in create_item(). See 38820.2.diff.

Yep, it prevents you changing the comment type after-the-fact (which sounds correct to me). I put the checks into the comment creation permission check function as that's where permission checks in the rest api belong AFAIK.
My comment about moderate_comments was that currently the rest of the API allows a user with that cap from overriding most of the checks in place, so I went with that for this one too - although I think that should probably be reviewed too. I'm all for removing that check from my patch.

Is the intent to support 'trackback' and 'pingback' creation via the core endpoint? @dd32 your patch hardcoded 'comment' only, but my patch includes all three core types.

It currently allows that, but I don't think that's correct - a trackback/pingback shouldn't be able to be created via a comment creation api, that should only be created via the trackback/pingback entry points (and their validation/sanitization of the pingback/trackback).

#5 @rachelbaker
7 years ago

  • Owner set to rachelbaker
  • Status changed from new to reviewing

@rachelbaker
7 years ago

Only allow comments to be created with a comment type of comment, otherwise return error.

#6 @rachelbaker
7 years ago

In 38820.3.diff only a comment type of comment is allowed during comment create. Otherwise, a WP_Error is returned.

Also includes a test for comment being the default value in the schema for the type property.

#7 @rachelbaker
7 years ago

  • Keywords has-unit-tests added

#8 @rachelbaker
7 years ago

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

In 39290:

REST API: On comment create, return an error if the type property is set to anything other than comment.

Of the default comment_types, only comments are expected to be created via the REST API endpoint. Comments do not have registered types the way that Posts do, so we do not have a method to accurately check permissions for arbitrary comment types.

Props dd32, boonebgorges, rachelbaker.
Fixes #38820.

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


7 years ago

Note: See TracTickets for help on using tickets.