REST API: Limit what users can set `author_ip` in the Comments endpoint

[dd32]

The Comments endpoint currently requires the caller to set the author_ip field, including for unauthed anonymous comments.

The API should not allow an anonymous user to set the IP address of the comment.

Furthermore, the documentation suggests that the default IP of 127.0.0.1 will be used if not presented - This should default to $_SERVER['REMOTE_ADDR'] instead, and only authorized users should be able to override that.

I personally do not believe any cap should be able to override the field, and that it should be hard-coded to always use REMOTE_ADDR unless a plugin allows otherwise (or REMOTE_ADDR is unavailable in the environment), however, in following with the other API designs in the endpoint, it would make sense to limit it to users with the moderate_comments cap.

The attached patch is a movement towards this, but fails as I couldn't see how to make the defaults play nicely together with the cap check. Further checks probably are needed to prevent the field being edited as well.

[jnylen0]

Needs tests and missing an operator space: !empty → ! empty.

To resolve the default issue we should remove the default of 127.0.0.1 and add it as a fallback case if there is no other way to determine an IP.

[rachelbaker]

If author_user_agent is not set, attempt to fallback to $_SERVER['REMOTE_ADDR'] and eventually 127.0.0.1

[rachelbaker]

@jnylen0 Can you look over 38819.2.diff​ for me? I removed using the schema to default to 127.0.0.1 and instead try to fallback to the $_SERVER['REMOTE_ADDR'] first if it is a valid IP.

[joehoyle]

Added a refresh patch with some more tests and removed karma conflicting line. I also remove the empty check on $_SERVER['REMOTE_ADDR'] as this is going to cause a slight logic issue whereby an empty REMOTE_ADDR would allow setting the author IP to anything you wanted. I don't know if this value would ever be blank (maybe CLI) but I think it's better to not have the logic error and risk the PHP notice.

[rachelbaker]

@joehoyle I think this is close, but the more I look at this... ticket here is what I think should happen:

users with the moderate_comments capability:

1. can set the author_ip property directly to a valid IP value
2. otherwise, fallback to the $_SERVER['REMOTE_ADDR'] if present and a valid IP value
3. finally, fallback to 127.0.0.1

users withOUT the moderate_comments capability:

1. canNOT set the author_ip property directly, and instead receive a WP_Error if they attempt to do so
2. the author_ip property is populated from $_SERVER['REMOTE_ADDR'] if present and a valid IP value
3. otherwise, fallback to 127.0.0.1

what do you think? does this sound sane to you?

[rachelbaker]

I was thinking not returning an error anytime author_ip is set would be inconsistent (if moderate_comments fails). But after discussing with @joehoyle via Slack, 38819.4.diff​ looks to be a winner.

Slack discussion for reference: ​https://wordpress.slack.com/archives/core-restapi/p1479502640001805

[rachelbaker]

In 39302:

REST API: On Comment create, limit the ability to set the author_ip value directly.

Users without the moderate_comments capability can no longer set the author_ip property directly, and instead receive a WP_Error if they attempt to do so. Otherwise, the author_ip property is populated from $_SERVER['REMOTE_ADDR'] if present and a valid IP value. Finally, fallback to 127.0.0.1 as a last resort.

Props dd32, rachelbaker, joehoyle.
Fixes #38819.

[ramiy]

This changeset needs a minor updated, it's inconsistent with #38822.

See https://core.trac.wordpress.org/ticket/38822#comment:10