WordPress.org

Make WordPress Core

Opened 6 years ago

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

Download all attachments as: .zip

Change History (42)

#1 @scribu
6 years ago

  • Keywords has-patch added

#2 @Denis-de-Bernardy
6 years ago

  • Milestone changed from Awaiting Review to 3.1

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

@wonderboymusic
4 years ago

#4 @wonderboymusic
4 years ago

  • Milestone changed from Future Release to 3.6

Refreshed against trunk plus added the date thing

#5 follow-up: @ericlewis
3 years ago

Should we validate this data on the way in?

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

#7 follow-up: @DrewAPicture
3 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
3 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
3 years ago

  • Description modified (diff)

#10 @ocean90
3 years ago

  • Milestone changed from 3.6 to Future Release

#11 @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 3.7

Refreshed

#12 @nacin
3 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
3 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
19 months ago

#31012 was marked as a duplicate.

#15 @SergeyBiryukov
19 months ago

  • Milestone changed from Future Release to 4.2

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

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


19 months ago

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


19 months ago

#19 @rachelbaker
18 months ago

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

Happy to pick this up.

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

#20 @johnbillion
18 months ago

  • Keywords 3.2-early removed

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


17 months ago

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

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


14 months ago

@rachelbaker
14 months ago

Filters given comment_author_IP and comment_agent values and includes unit tests

#24 @rachelbaker
14 months ago

  • Keywords rest-api added

@rachelbaker
14 months ago

Fixes file format

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

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

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

#28 follow-up: @dd32
13 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?

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

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

#31 follow-up: @obenland
13 months 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
13 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.

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

#34 @obenland
13 months 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
13 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)

#36 @boonebgorges
13 months ago

Thanks for the feedback, @dd32!

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