REST API: logic error in comments post ID

[dd32]

The rest API currently requires that a comment from a non-privileged user be added to a post which the user can read (ie. not trashed, private, draft, etc).

However the checks miss a case where the user attempts to add a comment for a future post which does not yet exist (ie. max_post_id + 1). This should be prevented, to prevent a comment being added to a not-yet-created post (which would then inherit it).

I'm not sure I understand the logic behind allowing comment creation for a non-existent post_id if the user has moderate_comments cap though.. that doesn't appear to be something which we would need to support.

[jnylen0]

This is closely related to #38700. Currently, creating a comment with an invalid post ID fails. However, when updating a comment, this is caught in wp_update_comment which leads to a 500 error.

I'm not sure I understand the logic behind allowing comment creation for a non-existent post_id if the user has moderate_comments cap though

I agree, we shouldn't support this either.

I don't think this is a permissions issue, so it shouldn't be addressed in the permission_callback. 38700.5.diff​ fixes this for comment updates, and we should add this check in create_item also.

Finally I think 404 is a better status code here.

[dd32]

I don't think this is a permissions issue, so it shouldn't be addressed in the permission_callback

It can go both ways, the user doesn't have permission to add a comment to something that doesn't exist, but the user also should not be able to add a post to something that doesn't exist.

I'd err on the side of permissions myself.

[dd32]

38816.2.diff​ is possibly a more logical permissions-based patch.

[jnylen0]

Looks fine to me (I guess where the check lives doesn't matter as long as we are not sending a 401 for this).

[rachelbaker]

I'm not sure I understand the logic behind allowing comment creation for a non-existent post_id if the user has moderate_comments cap though

@jnylen0 @dd32 We support this with the REST API because wp_insert_comment() supports adding comments without an attached post_ID - and our endpoints are intended to mirror the underlying WordPress API functions. I am open to changing the cap check here, if you have other ideas.

[rachelbaker]

restores the ability to create a comment with a post_id of 0

[dd32]

We support this with the REST API because wp_insert_comment() supports adding comments without an attached post_ID - and our endpoints are intended to mirror the underlying WordPress API functions.

Yeah, I strongly disagree with that on this count at least. Even though the underlying API allows it, doesn't mean it should be possible. Some things just shouldn't be done through a generic endpoint, if a plugin wanted the ability to create those for some reason, why should it not have to add it's own endpoint for that specific use-case?

I also dislike the moderate_comments cap being re-used for things that it doesn't represent - It represents the ability to moderate comments.. it doesn't mean that the user is a higher privileged user who can create special content (AFAIK)

[jnylen0]

our endpoints are intended to mirror the underlying WordPress API functions

I think this is worth discussing a bit more.

wp_insert_comment doesn't call wp_filter_comment (or wp_slash), which we definitely need to do, so this is one fairly obvious way the API needs to be more restrictive than the underlying functions.

There are important validation checks for users that are not performed in wp_insert_user (#38739). We shouldn't skip those either.

I think we need to be really careful about exposing totally new functionality (in terms of "things an end user can do") via the API. If we're thinking about allowing something new, these seem like good questions to ask:

1. Is this going to break anything?
2. Is this a valuable feature?
3. Do we have time to think through questions 1 and 2 properly?

Creating comments with a post_id of 0 might enable interesting ways to store data within WP. I don't know if it would break things beyond what has already been discussed in this ticket.

Similarly, comment karma (#38821) might enable themes to sort/rank comments in interesting new ways. But allowing clients to set the karma value to any integer probably isn't the right way to go there.

[rachelbaker]

Replying to jnylen0:

wp_insert_comment doesn't call wp_filter_comment (or wp_slash), which we definitely need to do, so this is one fairly obvious way the API needs to be more restrictive than the underlying functions.

There are important validation checks for users that are not performed in wp_insert_user (#38739). We shouldn't skip those either.

Do these needed changes have tickets yet?

[jnylen0]

Replying to rachelbaker:

Do these needed changes have tickets yet?

Yes, both of those are fixed (comments in #38704; users in #38739). I brought them up as specific examples where we shouldn't mirror the underlying WP functions, rather we should mirror the existing functionality instead.

[rachelbaker]

In 39288:

REST API: On comment create, return an error if the post parameter does not relate to a valid WP_Post object.

Return a WP_Error object for attempts to create a comment without an empty or invalid post ID.

Props dd32, jnylen0, rachelbaker.
Fixes #38816.