[wp-trac] [WordPress Trac] #14601: wp_new_comment method doesn't allow passed in values for IP and user-agent

[WordPress Trac]

#14601: wp_new_comment method doesn't allow passed in values for IP and user-agent
--------------------------------+--------------------------
 Reporter:  mrutz               |       Owner:  rachelbaker
     Type:  enhancement         |      Status:  accepted
 Priority:  normal              |   Milestone:  4.3
Component:  Comments            |     Version:  3.0.1
 Severity:  normal              |  Resolution:
 Keywords:  rest-api has-patch  |     Focuses:
--------------------------------+--------------------------

Comment (by 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.

 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.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/14601#comment:32>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform