wp_insert_post() should not contain current_user_can() checks

[alexkingorg]

wp_insert_post() is a utility function, it should not have a reliance on user capabilities. There are only two places in this function where there is a current_user_can() check - for updating custom taxonomies and for setting post slugs. All other checks (can user publish posts, etc.) are properly handled outside of the utility function.

wp_insert_post() should be safe to use in code that is run without a user context, for example via CRON. With the current code, this is the case *except* for the custom taxonomy feature. This inconsistency can cause a BrilliantDeveloperTM to lose a good deal of time debugging why the same data being passed in is coming back with different results.

For 3.4 (please!), perhaps we can figure out a way to move the checks for user capabilities on taxonomies out of the utility function and into the controller/procedural code. I'm happy to author and submit a patch once an approach has been determined.

For other developers who run into this and need to work around it, either of these 2 options work:

1. call wp_set_post_terms() to add your taxonomies after calling wp_insert_post()
2. set up a "current user" in your script before calling wp_insert_post()

[scribu]

Yes, please.

[scribu]

Well, actually, wp_insert_post() also takes care of sanitization, which checks the 'unfiltered_html' capability.

[nacin]

I have never liked this.

As scribu points out, kses is also applied, and we also prevent contributors from specifying a post slug of a pending post.

[nacin]

Essentially requires a revert of [13217], and instead handling this in wp_write_post()/edit_post(), at least for the taxonomy bits.

Emphasizing needs-patch on this one. Someone will probably need to step up here.

[johnbillion]

Replying to nacin:

Emphasizing needs-patch on this one. Someone will probably need to step up here.

As you mentioned it, if someone is working on a patch for a given ticket is it a best practice to assign the ticket to yourself so others know it's being worked on?

[nacin]

Replying to johnbillion:

Replying to nacin:

Emphasizing needs-patch on this one. Someone will probably need to step up here.

As you mentioned it, if someone is working on a patch for a given ticket is it a best practice to assign the ticket to yourself so others know it's being worked on?

Not typically necessary. I would just go for it and post a patch. Owners are typically more useful for those in charge of major tasks for a release (and the bugs that inevitably follow) and for committers to keep track of what they are watching over.

If multiple patches end up on the ticket, that's not a bad thing. It's good to see multiple approaches.

[alexkingorg]

Here's the best solution I've come up with that:

1. maintains backward compatibility
2. doesn't open up new security holes
3. generally enables wp_insert_post() to be used programatically outside of a "current user" scope

I believe there are three places where user permissions are referenced (tax_input, kses, post_slug). In adding a $user param to the function we can handle 3 scenarios:

1. nothing passed in - continue working as we do today (assume current user)
2. $user/$user_ID passed in - perform checks/security on behalf of that user
3. false passed in - execute with no security checks (assume admin user)

What do you all think about this as a general approach?

[alexkingorg]

Add a $user param to wp_insert_post to provide user context

[scribu]

That's pretty neat. Not only can you disable the cap checks, but also change which user to check against. +1

[scribu]

With the current path, is it true that sanitize_post() still runs as the current user?

I would assume that's not trivial to fix, though.

[alexkingorg]

Replying to scribu:

With the current path, is it true that sanitize_post() still runs as the current user?

I would assume that's not trivial to fix, though.

Exactly. This will allow bypassing it in "programmatic" mode, but the work the sanitization is doing is too far down the chain. Getting it inline with this approach would take more refactoring than is likely to be considered reasonable.

[scribu]

Then I think the third parameter should be a boolean, $sanitize = true.

Otherwise, you'd get inconsistent results, with some cap check from the current user and others from the user you passed in.

[alexkingorg]

Patch with boolean attached for review.

[alexkingorg]

boolean option for sanitization

[ejdanderson]

I just ran into this, any chance it will make it into 3.5?

[kovshenin]

I'm -1 on this. I think everything that goes into the database, whether by a logged in user action, anonymous user or cron, should be sanitized. Every post should have an author, and post_author = 0 sucks. We ran into a similar situation when allowing anonymous users to create draft posts, so we created a new user with minimum required caps, and inserted all the new posts on behalf of that user with wp_set_current_user, just don't forget to wp_set_current_user back to 0 when you're done.

[dd32]

#24564 was marked as a duplicate.

[rmccue]

+1, this is ridiculous.

[alexkingorg]

Refreshed patch against trunk, handles situation where post_author is not set

[alexkingorg]

Updated patch attached, in the event that no post_author is passed in and there is no "current user" context, a WP_Error (or 0) is returned.

Also, unit tests: ​http://core.trac.wordpress.org/changeset/1326/tests

[wonderboymusic]

these are all marked 3.7-early

[wonderboymusic]

this needs better whitespace and probably a unit test

[alexkingorg]

Is there a problem with the unit tests 4 comments up?

5 comments up is a diff with nice happy whitespace...

[nacin]

I'm ALL for fixing this. I strongly agree with the new parameter, otherwise this would be susceptible to skipping sanitization when wp_insert_post() is called with $_POST.

But, I'm a bit — well, very — concerned about avoiding sanitize_post() all together. We (and possibly plugins) use the various filters there to handle other safety measures, including sanitizing the post_type, post_status, post_mime_type, comment_status, ping_status, and guid fields. We're talking avoiding XSS via these fields and such. This is isn't a situation where stuff is "sometimes" invalid (i.e. kses), but always invalid.

That said, I think this is salvageable. Rather than skipping sanitize_post(), let's call kses_remove_filters() before, and kses_init() after. Thoughts?

Finally, wp-19373-20130728-refresh.diff​ seems to introduce a new error condition. This is in light of kovshenin's feedback. But I think it is tangental to the issue here. There is nothing actually wrong with post_author = 0, especially for custom post types. If it doesn't work in core, maybe it should. (I would not be surprised if you're able to find a trac comment from me saying it shouldn't be allowed because X.)

Point is, there's no reason to change this here. Even without this patch, it's possible to insert post_author = 0 when there is no current user. Let's address the desire to avoid post_author = 0 in a new ticket?

[nacin]

19373.diff​ is what I'm proposing. Largely untested. Probably requires unit tests to be updated. If there is a good reason to avoid sanitize_post() entirely, please make your case.

If there is anything short of immediate consensus, it will be well worth waiting until 3.8 to make sure we get this right.

[nacin]

Oh, uh, if we're going to add the $sanitize parameter, maybe it wouldn't be a terrible idea to also avoid calling wp_unslash() in that case. :-)

[nacin]

Well, not quite what I was going for... But due to wp_insert_post_data, well, you'll see the patch.

[MikeSchinkel]

Replying to nacin:

19373.diff​ is what I'm proposing.

Does an explicit $sanitize parameter make sense, or instead an $args parameter so that future unforeseen needs don't require adding yet another position parameter?

[nacin]

Replying to MikeSchinkel:

Replying to nacin:

19373.diff​ is what I'm proposing.

Does an explicit $sanitize parameter make sense, or instead an $args parameter so that future unforeseen needs don't require adding yet another position parameter?

Because it is a bool, we can always convert it to $args later. We *could* convert $wp_error to an array of args now.

[nacin]

Moving to 3.8 pending discussion.

[MikeSchinkel]

Replying to nacin:

Because it is a bool, we can always convert it to $args later.

Good point.

We *could* convert $wp_error to an array of args now.

Yes. That's a much better choice.

[nacin]

How far should ! $sanitize go? Should wp_insert_post_empty_content also be false and the empty_content WP_Error be skipped?

[danielbachhuber]

I dig 19373.2.diff, but it doesn't really solve my problem when coming at this from a slightly different angle: creating new nav menu items without a user context.

wp_update_nav_menu_item() uses the tax_input argument to associate a new menu item with the menu term id. Without a user context, the capability check fails and the new menu item isn't associated with the menu.

If the proposed patch were to go in, I don't think calling wp_insert_post() inside of wp_update_nav_menu_item() with $sanitize = false is the right way to go. It'd solve my immediate problem, but isn't a global solution.

I don't have a good suggestion otherwise at this point, other than:

• Sanitization should probably be treated separately from capability checks.
• Capability checks should always be implemented higher up the stack, and probably are for the needs of wp_insert_post(). I don't think there are many people depending on the capability check around tax_input. However, intentionally removing the capability check could unintentionally open permissions holes in third-party code

I created #27113 for my use case though.

[nacin]

In 27556:

Explicitly assign menu term relationship rather than piggybacking on wp_insert_post() with the tax_input argument.

That argument currently depends on user context (see #19373).

Adds unit test for properly updating orphaned menu items.

props danielbachhuber.
fixes #27113.

[ocean90]

#30454 was marked as a duplicate.

[boonebgorges]

See #30284.

[johnbillion]

#30389 was marked as a duplicate.

[ocean90]

#35233 was marked as a duplicate.

[sebastian.pisula]

I add patch for WordPress 4.5

[ocean90]

#37119 was marked as a duplicate.

[SergeyBiryukov]

Related: #29999

[ocean90]

#41744 was marked as a duplicate.

[kevinwhoffman]

I just wasted hours trying to figure out why terms were not being assigned when wp_insert_post() was called via WP Cron. After finding this ticket, I discovered that a user capability check is preventing terms from being assigned because there is no user in WP Cron.

For those looking to assign terms from a custom taxonomy to a post via WP Cron, you will have to call wp_set_post_terms() after wp_insert_post() as mentioned in the original report. Here is a snippet that allows you to assign terms using the same post array that you would normally pass to wp_insert_post():

$post_id = $this->save( $post_array );

foreach( $post_array['tax_input'] as $taxonomy => $terms ) {
        wp_set_post_terms( $post_id, $terms, $taxonomy  );
}

[tanner m]

Looks like this ticket hasn't been updated in a while. I can confirm that it is still an issue. Can we continue the conversation and get this fixed?

[perrelet]

wo. this really is quite a big deal team. +1 for numerous hours wasted.

If the gusto really isn't there to fix, a prominent notice on the wp_insert_post feels only fair.

There's a 12 year old hole in the floor charlie 💥💥💥