WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 11 hours 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: 4.3 Priority: normal
Severity: normal Version: 3.0.1
Component: Comments Keywords: rest-api 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 (5)

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 2 years ago.
14601.3.patch (5.8 KB) - added by rachelbaker 4 weeks ago.
Filters given comment_author_IP and comment_agent values and includes unit tests
14601.3.diff (5.8 KB) - added by rachelbaker 4 weeks ago.
Fixes file format

Download all attachments as: .zip

Change History (38)

@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 @nacin5 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

@wonderboymusic2 years ago

comment:11 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

Refreshed

comment:12 @nacin22 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 @jeremyfelt22 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 @SergeyBiryukov6 months ago

#31012 was marked as a duplicate.

comment:15 @SergeyBiryukov6 months ago

  • Milestone changed from Future Release to 4.2

comment:16 @rachelbaker6 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 @slackbot6 months ago

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

comment:18 @slackbot6 months ago

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

comment:19 @rachelbaker5 months ago

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

Happy to pick this up.

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

comment:20 @johnbillion5 months ago

  • Keywords 3.2-early removed

comment:21 @slackbot4 months ago

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

comment:22 @DrewAPicture4 months 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.

comment:23 @slackbot4 weeks ago

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

@rachelbaker4 weeks ago

Filters given comment_author_IP and comment_agent values and includes unit tests

comment:24 @rachelbaker4 weeks ago

  • Keywords rest-api added

@rachelbaker4 weeks ago

Fixes file format

comment:25 @rachelbaker4 weeks ago

  • Milestone changed from Future Release to 4.3

14601.3.diff updates @wonderboymusic's previous patch and filters the passed input for the $commentdata['comment_author_IP'] and $commentdata['comment_agent']. I also went ahead and added several unit tests.

As noted in my comment above for the WP REST API when creating comments the local $_SERVER data wouldn't contain the accurate IP or user agent values.

Milestoning this for 4.3 with my fingers crossed.

comment:26 follow-up: @boonebgorges4 weeks ago

I'd like to see an answer to nacin's comment regarding whether WP, or any plugins, are passing $_POST data (or some other unsanitized array) directly to wp_new_comment(). Can someone search the plugin directory to get a sense of what's out there in the wild?

comment:27 in reply to: ↑ 26 @obenland12 days ago

Replying to boonebgorges:

Can someone search the plugin directory to get a sense of what's out there in the wild?

@SergeyBiryukov, I believe you have local checkout, correct?

comment:28 follow-up: @dd327 days 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.

In a quick look through the wp-plugins github account I couldn't see any plugins using $_POST directly, but that's obviously not all of them.

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

@rachelbaker I'm curious as to this - other than a CLI use-case, REMOTE_ADDR should be set correctly, and if not, should be set in the bootstrap for the API.. same goes for the user agent..

What's your use-case exactly?

comment:29 in reply to: ↑ 28 ; follow-up: @rmccue7 days ago

Replying to dd32:

@rachelbaker I'm curious as to this - other than a CLI use-case, REMOTE_ADDR should be set correctly, and if not, should be set in the bootstrap for the API.. same goes for the user agent..

If you're programmatically creating comments on behalf of a user (e.g. custom commenting system backed by WP), and your requests to the API aren't on the client, the IP and UA will refer to the client making the request, not the original client.

comment:30 in reply to: ↑ 29 @dd326 days ago

Replying to rmccue:

Replying to dd32:

@rachelbaker I'm curious as to this - other than a CLI use-case, REMOTE_ADDR should be set correctly, and if not, should be set in the bootstrap for the API.. same goes for the user agent..

If you're programmatically creating comments on behalf of a user (e.g. custom commenting system backed by WP), and your requests to the API aren't on the client, the IP and UA will refer to the client making the request, not the original client.

That's a valid use-case for this, but I struggle to see how that's related to the rest api then, as the rest api comment endpoints shouldn't really allow that stuff to get set by default (IMHO)

comment:31 follow-up: @obenland5 days ago

@rachelbaker, we're one week away from beta, do you think this is going to make it into 4.3?

comment:32 @boonebgorges5 days ago

I reviewed all the plugins in the repo that use wp_new_comment() (there are about 100 of them). The vast majority pass a $commentdata array to wp_new_comment() that doesn't include either a comment_agent or comment_author_IP key. A small handful do include these values, but they are fetched from $_SERVER. The proposed change wouldn't affect these plugins at all.

I found just two plugins for which the proposed change may introduce security issues:

  • discussit-moderator sets up a custom xmlrpc method for comment creation, and blindly passes any data passed to the method along to wp_new_comment().
  • epoch, in what I believe is an AJAX endpoint, passes the $_POST array along to wp_new_comment() without sanitizing out the IP and user agent fields.

Based on this review, my guess is that the possibility for collateral damage here is very, very low. I'm fine going ahead with the change. That said, if there's any doubt, we could move the guts of wp_new_comment() to a new function, and turn wp_new_comment() into a wrapper that does some sanitization:

function wp_new_comment( $data ) {
    unset( $data['comment_agent'] );
    unset( $data['comment_author_IP'] );
    return _wp_new_comment( $data );
}

// we could probably use a better function name than this...
function _wp_new_comment( $data ) {
    // the existing wp_new_comment(), but 'comment_agent' and 'comment_author_IP' can be overridden
}

The REST API method could then call the new function instead of wp_new_comment().

That's a valid use-case for this, but I struggle to see how that's related to the rest api then, as the rest api comment endpoints shouldn't really allow that stuff to get set by default (IMHO)

I disagree with this. It's the client's responsibility to sanitize input before sending it to WP. The WP API (the PHP API wp_new_comment() as well as the REST API PUT /wp/v2/comments/) shouldn't make assumptions that can't be overridden by the client. It makes sense historically that wp_new_comment() would contain some security-related restrictions on accepted parameters, because it was designed with one client in mind, a client we have complete control over: wp-comments-post.php. But if it's going to be a more general service for arbitrary apps, it should be more flexible.

comment:33 in reply to: ↑ 31 @rachelbaker11 hours ago

Replying to obenland:

@rachelbaker, we're one week away from beta, do you think this is going to make it into 4.3?

What do you think about @boonebgorges's comment/feedback?

Note: See TracTickets for help on using tickets.