WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 weeks ago

#14601 accepted enhancement

wp_new_comment method doesn't allow passed in values for IP and user-agent

Reported by: mrutz Owned by: rachelbaker
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0.1
Component: Comments Keywords: has-patch
Focuses: Cc:

Description (last modified by ocean90)

In a scenario where you have a client that receives comments from the internet and pre-processes those comments before feeding them into wordpress through xmlrpc the ip and user-agent of the commenting internet user gets lost because there is no way of passing those values into the wp_new_comment function.

$_SERVER['REMOTE_ADDR'] and $_SERVER['HTTP_USER_AGENT'] are hard-coded, which in the above mentioned scenario will always have the IP and user-agent from the client that feeds the comments into wp through xmlrpc.

The attached patch will used passed in values and only fall back to $_SERVER['REMOTE_ADDR'] and $_SERVER['HTTP_USER_AGENT'] if not passed in.

Attachments (3)

comments_ip_agent.patch (1.3 KB) - added by mrutz 5 years ago.
14601.diff (1.5 KB) - added by wonderboymusic 2 years ago.
14601.2.diff (1.4 KB) - added by wonderboymusic 21 months ago.

Download all attachments as: .zip

Change History (25)

@mrutz5 years ago

comment:1 @scribu5 years ago

  • Keywords has-patch added

comment:2 @Denis-de-Bernardy5 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:3 @nacin4 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

It also doesn't allow you to modify the date of the comment.

@wonderboymusic2 years ago

comment:4 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.6

Refreshed against trunk plus added the date thing

comment:5 follow-up: @ericlewis2 years ago

Should we validate this data on the way in?

comment:6 in reply to: ↑ 5 @DrewAPicture2 years ago

14601.diff works as expected for me.

Replying to ericlewis:

Should we validate this data on the way in?

I suppose we could use FILTER_VALIDATE_IP to validate on the way in.

Last edited 2 years ago by DrewAPicture (previous) (diff)

comment:7 follow-up: @DrewAPicture2 years ago

Yeah, it's 5:30 am, and I wasn't thinking. I meant to say in comment:6 that we could probably use filter_var() and FILTER_VALIDATE_IP to check $commentdata['comment_author_IP'] on the way in. Something like

if ( ! isset( $commentdata['comment_author_IP'] ) && filter_var( $commentdata['comment_author_IP'], FILTER_VALIDATE_IP ) )

comment:8 in reply to: ↑ 7 @ocean902 years ago

Replying to DrewAPicture:

we could probably use filter_var() and FILTER_VALIDATE_IP

If it's available yes, but comment:7:ticket:16867.

comment:9 @ocean902 years ago

  • Description modified (diff)

comment:10 @ocean902 years ago

  • Milestone changed from 3.6 to Future Release

@wonderboymusic21 months ago

comment:11 @wonderboymusic21 months ago

  • Milestone changed from Future Release to 3.7

Refreshed

comment:12 @nacin20 months ago

Is there a situation where $commentdata is actually just $_POST data? Is it unreasonable for a plugin to have done that? We need to make sure users can't control these values.

comment:13 @jeremyfelt19 months ago

  • Milestone changed from 3.7 to Future Release

Punting to the future. More discussion should probably happen around the data coming in and how it should be handled.

comment:14 @SergeyBiryukov3 months ago

#31012 was marked as a duplicate.

comment:15 @SergeyBiryukov3 months ago

  • Milestone changed from Future Release to 4.2

comment:16 @rachelbaker3 months ago

To create comments with the WP REST API, we would like to use wp_new_comment(). This would allow us to take advantage of the filters, actions, and notifications already baked into wp_new_comment().

For API use the REMOTE_ADDR attribute would not be reliable or populated, and will trigger a PHP Notice of Undefined Index.

comment:17 @slackbot3 months ago

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

comment:18 @slackbot3 months ago

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

comment:19 @rachelbaker3 months ago

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

Happy to pick this up.

Last edited 3 months ago by rachelbaker (previous) (diff)

comment:20 @johnbillion2 months ago

  • Keywords 3.2-early removed

comment:21 @slackbot7 weeks ago

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

comment:22 @DrewAPicture7 weeks ago

  • Milestone changed from 4.2 to Future Release

Per discussion with @rachelbaker a few days ago, the suggestion was simply to check for the values coming and in and provide fallbacks. Needs a core developer to decide if passing an IP and/or user-agent should be possible in this context. See the JSON REST API use-case for context.

Pushing to future release for now.

Note: See TracTickets for help on using tickets.