WordPress.org

Make WordPress Core

Opened 8 years ago

Last modified 3 months ago

#19373 new enhancement

wp_insert_post() should not contain current_user_can() checks

Reported by: alexkingorg Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.0
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

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()

Attachments (7)

patch.diff (1.8 KB) - added by alexkingorg 8 years ago.
Add a $user param to wp_insert_post to provide user context
patch-boolean.diff (1.6 KB) - added by alexkingorg 8 years ago.
boolean option for sanitization
wp-19373-20130728-refresh.diff (3.0 KB) - added by alexkingorg 6 years ago.
Refreshed patch against trunk, handles situation where post_author is not set
19373.diff (1.8 KB) - added by nacin 6 years ago.
19373.2.diff (2.6 KB) - added by nacin 6 years ago.
Well, not quite what I was going for... But due to wp_insert_post_data, well, you'll see the patch.
19373-unittests.diff (1.9 KB) - added by boonebgorges 5 years ago.
See #30284.
35233.patch (603 bytes) - added by sebastian.pisula 4 years ago.

Download all attachments as: .zip

Change History (55)

#1 @scribu
8 years ago

Yes, please.

#2 @scribu
8 years ago

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

#3 @johnbillion
8 years ago

  • Cc johnbillion@… added

#4 @sillybean
8 years ago

  • Cc steph@… added

#5 @mikeschinkel
8 years ago

  • Cc mikeschinkel@… added

#6 @nacin
8 years ago

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.

#7 @ashfame
8 years ago

  • Cc ashishsainiashfame@… added

#8 follow-up: @nacin
8 years ago

  • Keywords 3.4-early needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement
  • Version changed from 3.3 to 3.0

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.

#9 in reply to: ↑ 8 ; follow-up: @johnbillion
8 years ago

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?

#10 in reply to: ↑ 9 @nacin
8 years ago

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.

#11 @alexkingorg
8 years ago

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
8 years ago

Add a $user param to wp_insert_post to provide user context

#12 @scribu
8 years ago

  • Keywords has-patch added; needs-patch removed

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

#13 follow-up: @scribu
8 years ago

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.

#14 in reply to: ↑ 13 @alexkingorg
8 years ago

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.

#15 @scribu
8 years ago

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.

#16 @alexkingorg
8 years ago

Patch with boolean attached for review.

@alexkingorg
8 years ago

boolean option for sanitization

#17 @ejdanderson
7 years ago

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

#18 @wonderboymusic
6 years ago

  • Milestone changed from Future Release to 3.6

#19 @alex-ye
6 years ago

  • Cc nashwan.doaqan@… added

#20 @kovshenin
6 years ago

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.

#21 @mordauk
6 years ago

  • Cc pippin@… added

#22 @sunnyratilal
6 years ago

  • Cc sunnyratilal5@… added

#23 @ryan
6 years ago

  • Milestone changed from 3.6 to Future Release

#24 @dd32
6 years ago

#24564 was marked as a duplicate.

#25 @rmccue
6 years ago

+1, this is ridiculous.

@alexkingorg
6 years ago

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

#26 @alexkingorg
6 years ago

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

#27 @duck_
6 years ago

  • Keywords 3.7-early added; 3.4-early removed
  • Milestone changed from Future Release to 3.7

#28 @wonderboymusic
6 years ago

these are all marked 3.7-early

#29 @wonderboymusic
6 years ago

this needs better whitespace and probably a unit test

#30 @alexkingorg
6 years ago

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

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

#31 @nacin
6 years ago

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
6 years ago

#32 follow-up: @nacin
6 years ago

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.

#33 @nacin
6 years ago

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
6 years ago

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

#34 in reply to: ↑ 32 ; follow-up: @MikeSchinkel
6 years ago

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?

#35 in reply to: ↑ 34 ; follow-up: @nacin
6 years ago

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.

#36 @nacin
6 years ago

  • Keywords 3.8-early added; 3.7-early removed
  • Milestone changed from 3.7 to Future Release

Moving to 3.8 pending discussion.

#37 in reply to: ↑ 35 @MikeSchinkel
6 years ago

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.

#38 @nacin
6 years ago

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

#39 @danielbachhuber
5 years ago

  • Keywords 3.8-early removed

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.

#40 @nacin
5 years ago

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.

#41 @ocean90
5 years ago

#30454 was marked as a duplicate.

#42 @johnbillion
5 years ago

#30389 was marked as a duplicate.

#43 @ocean90
4 years ago

#35233 was marked as a duplicate.

#44 @sebastian.pisula
4 years ago

I add patch for WordPress 4.5

#45 @ocean90
3 years ago

#37119 was marked as a duplicate.

#47 @ocean90
23 months ago

#41744 was marked as a duplicate.

#48 @kevinwhoffman
3 months ago

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 = wp_insert_post( $post_array );

foreach( $post_array['tax_input'] as $taxonomy => $terms ) {
        wp_set_post_terms( $post_id, $terms, $taxonomy  );
}
Last edited 3 months ago by kevinwhoffman (previous) (diff)
Note: See TracTickets for help on using tickets.