#38819 closed defect (bug) (fixed)
REST API: Limit what users can set `author_ip` in the Comments endpoint
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Comments | Keywords: | has-patch commit |
Focuses: | rest-api | Cc: |
Description
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.
Attachments (4)
Change History (15)
@
9 years ago
If author_user_agent
is not set, attempt to fallback to $_SERVER['REMOTE_ADDR']
and eventually 127.0.0.1
#3
@
9 years ago
@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.
This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-restapi by joehoyle. View the logs.
9 years ago
#7
@
9 years ago
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.
#8
@
9 years ago
@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:
- can set the
author_ip
property directly to a valid IP value - otherwise, fallback to the
$_SERVER['REMOTE_ADDR']
if present and a valid IP value - finally, fallback to
127.0.0.1
users withOUT the moderate_comments
capability:
- canNOT set the
author_ip
property directly, and instead receive aWP_Error
if they attempt to do so - the
author_ip
property is populated from$_SERVER['REMOTE_ADDR']
if present and a valid IP value - otherwise, fallback to
127.0.0.1
what do you think? does this sound sane to you?
#9
@
9 years ago
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
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.