Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#38819 closed defect (bug) (fixed)

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

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 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)

38819.diff (5.4 KB) - added by dd32 9 years ago.
38819.2.diff (3.1 KB) - added by rachelbaker 9 years ago.
If author_user_agent is not set, attempt to fallback to $_SERVER['REMOTE_ADDR'] and eventually 127.0.0.1
38819.3.diff (5.7 KB) - added by joehoyle 9 years ago.
38819.4.diff (6.1 KB) - added by joehoyle 9 years ago.

Download all attachments as: .zip

Change History (15)

@dd32
9 years ago

#1 @jnylen0
9 years ago

  • Keywords needs-unit-tests added

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.

#2 @rachelbaker
9 years ago

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

@rachelbaker
9 years ago

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

#3 @rachelbaker
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

#5 @rachelbaker
9 years ago

  • Keywords has-patch commit added; needs-unit-tests removed

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


9 years ago

@joehoyle
9 years ago

#7 @joehoyle
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.

@joehoyle
9 years ago

#8 @rachelbaker
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:

  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?

#9 @rachelbaker
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

#10 @rachelbaker
9 years ago

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

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.

#11 @ramiy
9 years ago

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

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

Note: See TracTickets for help on using tickets.