Comments Being Pulled from Non-Existent Post Types

[sterlo]

Originally on: #10461

I'm running standard LAMP on the latest trunk.

Just viewing the dashboard with no plugins activated:

Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 918 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 919 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 Notice: Trying to get property of non-object in /Users/grok/Projects/Local Development/wordpress/trunk/wp-includes/capabilities.php on line 922 

It's not recognizing "$post_type->cap" as valid.

And...here's why - I added this (/wp-includes/capabilities.php):

 918 echo "POST TYPE: Y U NO OBJECT?\n";
 919 var_dump($post_type);

And got:

POST TYPE: Y U NO OBJECT?
NULL

So in this context...the post type is null and the code was not expecting that.

Opening the actual $post object:

stdClass Object
(
    [ID] => 60
    [post_author] => 1
    [post_date] => 2011-01-28 19:46:23
    [post_date_gmt] => 2011-01-28 19:46:23
    [post_content] => CONTENT!
    [post_title] => I have it all!
    [post_excerpt] => 
    [post_status] => publish
    [comment_status] => open
    [ping_status] => open
    [post_password] => 
    [post_name] => i-have-it-all
    [to_ping] => 
    [pinged] => 
    [post_modified] => 2011-01-28 19:46:28
    [post_modified_gmt] => 2011-01-28 19:46:28
    [post_content_filtered] => 
    [post_parent] => 0
    [guid] => http://dev.wordpress.local/?post_type=staff_listing&p=60
    [menu_order] => 0
    [post_type] => staff_listing
    [post_mime_type] => 
    [comment_count] => 6
    [ancestors] => Array
        (
        )

    [filter] => raw
)

I think the problem might be custom post types or custom taxonomies...

So my custom post type in another plugin is creating: "?post_type=staff_listing".
And this post does show "[post_type] => staff_listing". BUT the plugin that had created these comments...is de-activated.

Activating the plugin resolves this issue.

So, whats a viable solution? Telling a developer to clean up after the plugin (removing content just because of deactivation), OR having WordPress not pull data (e.g. comments) that are assigned to other data (e.g. post types) that don't exist?

Old Code: Give me all comments.

New Code: Give me all comments that are tied to existing objects.

[dd32]

Closed #17839 as duplicate.

Noticed in the recent comments widget:

( ! ) Notice: Trying to get property of non-object in C:\www\wordpress-commit\wp-includes\capabilities.php on line 929
Call Stack
#	Time	Memory	Function	Location
1	0.0005	360328	{main}( )	..\index.php:0
2	0.3960	24624984	wp_dashboard( )	..\index.php:63
3	0.3960	24625272	do_meta_boxes( string(9), string(6), string(0) )	..\dashboard.php:221
4	0.8141	25014760	call_user_func ( string(28), string(0), array(4) )	..\template.php:970
5	0.8141	25014776	wp_dashboard_recent_comments( string(0), array(4) )	..\template.php:0
6	0.8171	25271192	current_user_can( string(9), string(3) )	..\dashboard.php:632
7	0.8171	25271784	call_user_func_array ( array(2), array(2) )	..\capabilities.php:1047
8	0.8171	25272048	WP_User->has_cap( string(9), string(3) )	..\capabilities.php:0
9	0.8172	25272448	call_user_func_array ( string(12), array(3) )	..\capabilities.php:729
10	0.8172	25272736	map_meta_cap( string(9), string(1), string(3) )	..\capabilities.php:0

Turns out, that it's a comment on a custom post type that's no longer registered.

As a result $post_type = get_post_type_object( $post->post_type ); fails to return a valid post type object (however, get_post() can still return the cpt) which results in the code operating on a null object.

[boonebgorges]

Since the problem is related to map_meta_cap(), maybe it's best not to mess with the query but instead to solve the problem at the level of caps. Maybe when get_post_type_object( $post->post_type ) returns null, add 'do_not_allow' to $caps and bail?

[nacin]

Maybe when get_post_type_object( $post->post_type ) returns null, add 'do_not_allow' to $caps and bail?

At the very least, we should be checking for a null value there, yes.

[mitchoyoshitaka]

Patch v1

[mitchoyoshitaka]

Still an issue in trunk (3.5 alpha); attached patch based on boone and nacin's discussion. Patch fixes notices on my local trunk which were due to sterlo's initial situation (data from stray custom post types).

[SergeyBiryukov]

Related: #22312

[SergeyBiryukov]

#22719 was marked as a duplicate.

[SergeyBiryukov]

#22934 was marked as a duplicate.

[ddavout]

It is not so easy to apply a patch dating of 2 years ...

[mitchoyoshitaka]

Patch v1 (fixed patchrot, 2013 May 9)

[mitchoyoshitaka]

Replying to ddavout:

It is not so easy to apply a patch dating of 2 years ...

Not sure which patch you were trying (mine was not 2 years old) but it might not have applied cleanly. I went ahead and updated it against trunk.

[sterlo]

It's ALIVE!!!!

[mitchoyoshitaka]

Replying to sterlo:

It's ALIVE!!!!

Rumors of this ticket's death (or maybe my death) have been greatly exaggerated.

[nacin]

I like this patch, but this needs to be done carefully. Also, this should probably be paired with #13905. And the issues in #14530 should be considered.

Also — post types didn't always need to exist. There is some old code on WP.org that used the post_type field back from before there was a way to register them. Maybe we do nothing, but it's something to at least think about.

[wonderboymusic]

these are all marked 3.7-early

[SergeyBiryukov]

#25173 was marked as a duplicate.

[DrewAPicture]

refresh following [24593]

[DrewAPicture]

16956.3.diff​ is a refresh following [24593], which took care of the more visible notices (such as for comments).

[alexvorn2]

maybe it would be better not to show comments from non-existent post types?

[ocean90]

#25324 was marked as a duplicate.

[nacin]

A do_ not_allow patch needs a lot of testing and soaking.

[SergeyBiryukov]

#26366 was marked as a duplicate.

[nacin]

I'm going to try this for 3.9.

[bobbingwide]

A simple alternative workaround for the original problem is to delete any comment which causes this message to appear.

Note: I haven't checked if this has already been reported but...
Whatever solution is eventually chosen for this ticket, almost exactly the same logic should apply for the tests against the post_status object ( $status_obj ) which are performed immediately after.
$status_obj can also be null if a post_status that was previously valid is no longer registered.

[SergeyBiryukov]

#28215 was marked as a duplicate.

[DrewAPicture]

#28775 was marked as a duplicate.

[DrewAPicture]

#29043 was marked as a duplicate.

[DeBAAT]

Sorry I reported my ticket as a duplicate of this one.
And also sorry to see that this defect has not been solved for over three years now.
The patch file attached to ticket #29043 should solve all (14!) issues still left in 4.0 trunk regarding using null-objects, whether they are post, post_type or status_obj.

[imath]

Hi,

had the opportunity to test 16956.3.diff and noticed after applying it that in the comments WP List Table, trying to get the permalink of the parent post was also throwing a notice. So i suggest 16956.4.2.diff to avoid it.

[bobbingwide]

How many duplicates need to be raised before someone actually fixes this bug?
Can this be assigned to 4.3-early?

[codeelite]

New to this, not sure how to add those .diff files to attachment..sorry,
I currently got lots of notice in wp-admin dashboard (run on WP 4.2.2), the problem was the previous plugin is installing some custom post types, and when I deactivate this plugin I got lots of notice, such as..

Trying to get property of non-object in /wp-includes/capabilities.php in line 1159.. etc

so.. I added simple check to capabilities.php - function map_meta_cap()

post_type_exists($post->post_type)

and all notice are gone. Not sure if this change will take affect into another WP features, but for now all is good, can't see any warning or notice in my dashboard.

[boonebgorges]

Circling back around to this. Forcing 'do_not_allow' in these cases seems problematic, as it means that even administrators will not be able to manage the content in question. As suggested by nacin on #13905 https://core.trac.wordpress.org/ticket/13905#comment:10, it seems safer to fall back on some sane defaults. 16956.5.diff​ accomplishes this by falling back on get_post_type_object( 'post' ). This seems pretty safe to me, but a second opinion would be helpful.

[jorbin]

This needs a 2nd opinion and some activity in the next week, otherwise it is going to be punted from 4.3.

I have some concerns that this could lead to unexpected capability escalation. I would also really like to see some automated tests.

[valendesigns]

I can review the patch and take a stab at unit test in the next day or two. Does anyone have sample code to share that can quickly reproduce the issue?

[boonebgorges]

I have some concerns that this could lead to unexpected capability escalation

Are your concerns related to a general squeamishness about cap mapping, or are you imagining specific scenarios where escalation could occur? I'm struggling to describe a situation where meaningful cap escalation could take place. There is perhaps a concern that a plugin registers a post type 'foo' and provides custom logic for, eg, 'edit_foo'; when the plugin is then deactivated, the WP interface will fall back on 'edit_post'; and while currently current_user_can( 'edit_post' ) will always return false in these cases, with my proposed fix it will obey the general logic for 'edit_post'. I can imagine cases where this might be problematic, but I'm also not sure how much WP can be responsible for it, given that caps are registered and processed at runtime.

I personally don't feel comfortable moving forward with this during beta, so I'm moving it out of the milestone.

[valendesigns]

Let's revisit this early 4.4 and at that time write unit tests, so we're not making assumptions about capability escalation or other possible side effects of the proposed change.

[jorbin]

Replying to boonebgorges:

I have some concerns that this could lead to unexpected capability escalation

Are your concerns related to a general squeamishness about cap mapping, or are you imagining specific scenarios where escalation could occur? I'm struggling to describe a situation where meaningful cap escalation could take place. There is perhaps a concern that a plugin registers a post type 'foo' and provides custom logic for, eg, 'edit_foo'; when the plugin is then deactivated, the WP interface will fall back on 'edit_post'; and while currently current_user_can( 'edit_post' ) will always return false in these cases, with my proposed fix it will obey the general logic for 'edit_post'. I can imagine cases where this might be problematic, but I'm also not sure how much WP can be responsible for it, given that caps are registered and processed at runtime.

Mostly general squermishness and paranoia. I think my worry is something like:

Developer creates a CPT with custom capabilities and maps it so that only users who's name starts with the letter A can manage comments (since A names are awesome). A user with a name starting with B has the ability to deactivate plugins and deactivates the plugin containing this CPT. With this change, they would now be able manage comments even though they don't have an awesome A name.

Over paranoid? Very possible. At a minimum, I think we should throw a doing_it_wrong with some instructions so that developers are less likely to accidentally escalate privileges here.

[bobbingwide]

Is this in the early 4.4 list yet?

[boonebgorges]

Replying to bobbingwide:

Is this in the early 4.4 list yet?

I'd be very glad to get this into 4.4, but I want to make sure we respond adequately to jorbin's concerns. In particular:

At a minimum, I think we should throw a doing_it_wrong with some instructions so that developers are less likely to accidentally escalate privileges here.

I'm not sure how this would work. The problem here occurs when a CPT is no longer registered, which usually means that the plugin is no longer active. So we can't throw a _doing_it_wrong() here. Yet surely we don't want to discourage cap mapping in general. I think this is more of an education issue: plugins ought to have deactivation routines that clean up any sensitive content or privileges.

[nofearinc]

run wp_dashboard_recent_comments against active post types

[nofearinc]

The problem seems to be pretty persistent and reproducible when wp_dashboard_recent_comments is invoked in the dashboard. Attached is a safer (in my opinion) workaround that doesn't touch the capabilities and runs the comment query against the available post types (get_post_types()).

I have seen that particular issue numerous times on the same screen, always after deactivating a plugin registering another post type.

[boonebgorges]

nofearinc - Ha, we'd all gotten so caught up in the capabilities issue that we overlooked this. You are correct that it's much safer. The problem of cap checks against comments belonging to non-existent post types still exists, but maybe it doesn't surface anywhere in core except for this spot.

[SergeyBiryukov]

Replying to nofearinc:

Attached is a safer (in my opinion) workaround that doesn't touch the capabilities and runs the comment query against the available post types (get_post_types()).

16956.7.diff​ uses the same fix on Comments screen.

Note that this approach means that orphaned comments won't be displayed anywhere in the admin.

[boonebgorges]

Replying to SergeyBiryukov:

Note that this approach means that orphaned comments won't be displayed anywhere in the admin.

Yeah. In contrast, the current state of affairs is that they're *displayed*, but they can't be edited, because the cap checks always fail - except in the case of a super admin on multisite, when they always pass :-/. Hiding them altogether seems like an improvement.

Just to throw out another possible caps-based solution that might be a compromise. When mapping caps for 'edit_comment' etc, if the post type object is not found, map onto 'edit_others_posts'. This is more restrictive than simply mapping to 'edit_posts', but still allows the content to be managed by highly privileged users. See 16956.8.diff​.

[bobbingwide]

Replying to boonebgorges:

I think this is more of an education issue: plugins ought to have deactivation routines that clean up any sensitive content or privileges.

There are times when you have to deactivate plugins temporarily and they should not be deleting data when this happens.

I like @nofearinc's approach.
Suggest raising a separate defect for Sergey's fix for the Comments screen.

[bobbingwide]

I recently accidentally discovered another workaround, which is to override the post type registration with your own version. So if the plugin that originally registered the post type is deactivated, the post type is still registered by your own plugin.

​http://herbmiller.me/2015/09/07/h2gd-part-48-super-side-effect-of-oik-types-provides-workaround-for-trac-16956/

[jorbin]

Sergey made a point that "since r34038, [orphaned comments] can be edited or removed" via the dashboard. With that in mind, I think Boone's approach is the right one. It still could lead to unintented information disclosure. To hopefully help mitigate this from being an issue, I've updated attachment:16956.8.diff​ to include a call to _doing_it_wrong when we switch the permission in attachment:16956.9.diff​

This still needs unit tests as well before it's ready for commit.

[dlh]

attachment:16956.10.patch​ attempts a unit test. The patch also assigns edit_others_posts to $caps[] instead of $cap (although I might have misunderstood the use of $cap) and fixes a couple typos.

[boonebgorges]

In 34091:

Fail gracefully when checking mapped cap against unregistered post type.

Post type objects are reponsible for mapping their capabilities to core caps.
As a result, when the post type is no longer registered, the caps are no
longer mapped. This causes problems when a post is left in the database after
the post type is no longer present, and WP does an 'edit_post' or other cap
check against it: a PHP notice is thrown, and the cap check always fails.

As a more graceful fallback, we map all post-type-dependent caps onto
'edit_others_posts', which allows highly privileged users to be able to
access orphaned content (such as comments belonging to disabled post types),
while minimizing the possibility of unintended privilege escalation.

We also add a _doing_it_wrong() notice, so that developers and site
administrators are aware that the cap mapping is failing in the absence of
the registered post type.

Props mitchoyoshitaka, DrewAPicture, imath, codeelite, boonebgorges, nofearinc, SergeyBiryukov, jorbin, dlh.
Fixes #16956.