WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 2 months ago

#14601 closed enhancement (fixed)

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

Download all attachments as: .zip

Change History (42)

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

@wonderboymusic3 years ago

comment:4 @wonderboymusic3 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 @nacin2 years 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 @jeremyfelt2 years 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 @SergeyBiryukov8 months ago

#31012 was marked as a duplicate.

comment:15 @SergeyBiryukov8 months ago

  • Milestone changed from Future Release to 4.2

comment:16 @rachelbaker8 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 @slackbot8 months ago

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

comment:18 @slackbot8 months ago

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

comment:19 @rachelbaker7 months ago

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

Happy to pick this up.

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

comment:20 @johnbillion7 months ago

  • Keywords 3.2-early removed

comment:21 @slackbot6 months ago

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

comment:22 @DrewAPicture6 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 @slackbot3 months ago

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

@rachelbaker3 months ago

Filters given comment_author_IP and comment_agent values and includes unit tests

comment:24 @rachelbaker3 months ago

  • Keywords rest-api added

@rachelbaker3 months ago

Fixes file format

comment:25 @rachelbaker3 months 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: @boonebgorges3 months 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 @obenland3 months 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: @dd322 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.

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: @rmccue2 months 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 @dd322 months 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: @obenland2 months ago

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

comment:32 follow-up: @boonebgorges2 months 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 @rachelbaker2 months 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?

comment:34 @obenland2 months ago

I'm just trying to keep the ticket moving.
@dd32, @boonebgorges, could you come to a decision on this for 4.3?

comment:35 in reply to: ↑ 32 @dd322 months ago

Replying to boonebgorges:

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.

Based on that I think it should be safe to add the parameters, and a new function shouldn't be needed.

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

These two plugins have very few users, however should update their plugins to only pass the fields they expect (FYI @Shelob9 & @charlie73)

comment:36 @boonebgorges2 months ago

Thanks for the feedback, @dd32!

comment:37 @boonebgorges2 months ago

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

In 33021:

Allow 'comment_agent' and 'comment_author_IP' to be set via wp_new_comment().

Props mrutz, wonderboymusic, rachelbaker.
Fixes #14601.

Note: See TracTickets for help on using tickets.