Make WordPress Core

Opened 14 years ago

Closed 9 years ago

#14601 closed enhancement (fixed)

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

Reported by: mrutz's profile mrutz Owned by: rachelbaker's profile 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 14 years ago.
14601.diff (1.5 KB) - added by wonderboymusic 11 years ago.
14601.2.diff (1.4 KB) - added by wonderboymusic 11 years ago.
14601.3.patch (5.8 KB) - added by rachelbaker 9 years ago.
Filters given comment_author_IP and comment_agent values and includes unit tests
14601.3.diff (5.8 KB) - added by rachelbaker 9 years ago.
Fixes file format

Download all attachments as: .zip

Change History (42)

#1 @scribu
14 years ago

  • Keywords has-patch added

#2 @Denis-de-Bernardy
14 years ago

  • Milestone changed from Awaiting Review to 3.1

#3 @nacin
13 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.

#4 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.6

Refreshed against trunk plus added the date thing

#5 follow-up: @ericlewis
11 years ago

Should we validate this data on the way in?

#6 in reply to: ↑ 5 @DrewAPicture
11 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 11 years ago by DrewAPicture (previous) (diff)

#7 follow-up: @DrewAPicture
11 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 ) )

#8 in reply to: ↑ 7 @ocean90
11 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.

#9 @ocean90
11 years ago

  • Description modified (diff)

#10 @ocean90
11 years ago

  • Milestone changed from 3.6 to Future Release

#11 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

Refreshed

#12 @nacin
11 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.

#13 @jeremyfelt
11 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.

#14 @SergeyBiryukov
9 years ago

#31012 was marked as a duplicate.

#15 @SergeyBiryukov
9 years ago

  • Milestone changed from Future Release to 4.2

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

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 by rachelbaker. View the logs.


9 years ago

#19 @rachelbaker
9 years ago

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

Happy to pick this up.

Last edited 9 years ago by rachelbaker (previous) (diff)

#20 @johnbillion
9 years ago

  • Keywords 3.2-early removed

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


9 years ago

#22 @DrewAPicture
9 years 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.

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


9 years ago

@rachelbaker
9 years ago

Filters given comment_author_IP and comment_agent values and includes unit tests

#24 @rachelbaker
9 years ago

  • Keywords rest-api added

@rachelbaker
9 years ago

Fixes file format

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

#26 follow-up: @boonebgorges
9 years 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?

#27 in reply to: ↑ 26 @obenland
9 years 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?

#28 follow-up: @dd32
9 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.

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?

#29 in reply to: ↑ 28 ; follow-up: @rmccue
9 years 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.

#30 in reply to: ↑ 29 @dd32
9 years 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)

#31 follow-up: @obenland
9 years ago

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

#32 follow-up: @boonebgorges
9 years 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.

#33 in reply to: ↑ 31 @rachelbaker
9 years 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?

#34 @obenland
9 years ago

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

#35 in reply to: ↑ 32 @dd32
9 years 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)

#36 @boonebgorges
9 years ago

Thanks for the feedback, @dd32!

#37 @boonebgorges
9 years 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.