WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 23 months ago

Last modified 17 months ago

#16956 closed defect (bug) (fixed)

Comments Being Pulled from Non-Existent Post Types

Reported by: sterlo Owned by: boonebgorges
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.1
Component: Posts, Post Types Keywords: has-patch 2nd-opinion needs-unit-tests
Focuses: Cc:

Description

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.

Attachments (12)

16956.diff (1.6 KB) - added by mitchoyoshitaka 5 years ago.
Patch v1
16956.2.diff (1.6 KB) - added by mitchoyoshitaka 4 years ago.
Patch v1 (fixed patchrot, 2013 May 9)
16956.3.diff (952 bytes) - added by DrewAPicture 4 years ago.
refresh following [24593]
16956.4.diff (1.7 KB) - added by imath 3 years ago.
16956.4.2.diff (1.7 KB) - added by imath 3 years ago.
capabilities.php (39.5 KB) - added by codeelite 2 years ago.
16956.5.diff (1.3 KB) - added by boonebgorges 2 years ago.
16956.6.diff (480 bytes) - added by nofearinc 2 years ago.
run wp_dashboard_recent_comments against active post types
16956.7.diff (1.1 KB) - added by SergeyBiryukov 2 years ago.
16956.8.diff (1.3 KB) - added by boonebgorges 2 years ago.
16956.9.diff (2.0 KB) - added by jorbin 23 months ago.
16956.10.patch (3.3 KB) - added by dlh 23 months ago.

Download all attachments as: .zip

Change History (69)

#1 @kawauso
6 years ago

  • Keywords dev-feedback added

#2 @dd32
6 years ago

  • Component changed from General to Warnings/Notices
  • Keywords dev-feedback removed
  • Type changed from enhancement to defect (bug)

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.

#3 @boonebgorges
5 years ago

  • Cc boonebgorges@… added

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?

#4 @nacin
5 years ago

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

Patch v1

#5 @mitchoyoshitaka
5 years ago

  • Cc mitcho@… added
  • Keywords has-patch added

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

#7 @SergeyBiryukov
5 years ago

#22719 was marked as a duplicate.

#8 @SergeyBiryukov
5 years ago

#22934 was marked as a duplicate.

#9 @ddavout
4 years ago

  • Component changed from Warnings/Notices to Post Types

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

#10 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 3.6

@mitchoyoshitaka
4 years ago

Patch v1 (fixed patchrot, 2013 May 9)

#11 @mitchoyoshitaka
4 years ago

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.

#12 follow-up: @sterlo
4 years ago

It's ALIVE!!!!

http://i.imgur.com/bhNML.gif

#13 in reply to: ↑ 12 @mitchoyoshitaka
4 years ago

Replying to sterlo:

It's ALIVE!!!!

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

#14 @nacin
4 years ago

  • Keywords 3.7-early added
  • Milestone changed from 3.6 to Future Release

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.

#15 @wonderboymusic
4 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#16 @SergeyBiryukov
4 years ago

#25173 was marked as a duplicate.

@DrewAPicture
4 years ago

refresh following [24593]

#17 @DrewAPicture
4 years ago

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

#18 @alexvorn2
4 years ago

  • Cc alexvornoffice@… added

#19 @alexvorn2
4 years ago

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

#20 @ocean90
4 years ago

#25324 was marked as a duplicate.

#21 @nacin
4 years ago

  • Milestone changed from 3.7 to 3.8

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

#22 @SergeyBiryukov
4 years ago

#26366 was marked as a duplicate.

#23 @nacin
4 years ago

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

I'm going to try this for 3.9.

#24 @bobbingwide
4 years ago

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.

#25 @SergeyBiryukov
3 years ago

#28215 was marked as a duplicate.

#26 @DrewAPicture
3 years ago

#28775 was marked as a duplicate.

#27 @DrewAPicture
3 years ago

#29043 was marked as a duplicate.

#28 @DrewAPicture
3 years ago

  • Keywords 4.1-early added; 3.9-early removed

#29 @DeBAAT
3 years ago

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.

#30 @imath
3 years ago

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.

Last edited 3 years ago by imath (previous) (diff)

@imath
3 years ago

@imath
3 years ago

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


3 years ago

#32 @bobbingwide
2 years ago

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

#33 @codeelite
2 years ago

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.

Last edited 2 years ago by codeelite (previous) (diff)

@boonebgorges
2 years ago

#34 @boonebgorges
2 years ago

  • Keywords 2nd-opinion added; commit 4.1-early removed
  • Milestone changed from Future Release to 4.3

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.

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


2 years ago

#36 @jorbin
2 years ago

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.

#37 @valendesigns
2 years ago

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?

#38 follow-up: @boonebgorges
2 years ago

  • Milestone changed from 4.3 to Future Release

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.

#39 @valendesigns
2 years ago

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.

#40 @valendesigns
2 years ago

  • Keywords needs-unit-tests added

#41 in reply to: ↑ 38 @jorbin
2 years ago

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.

#42 follow-up: @bobbingwide
2 years ago

Is this in the early 4.4 list yet?

#43 in reply to: ↑ 42 ; follow-up: @boonebgorges
2 years ago

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

run wp_dashboard_recent_comments against active post types

#44 follow-up: @nofearinc
2 years ago

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.

#45 @boonebgorges
2 years ago

  • Milestone changed from Future Release to 4.4
  • Owner set to boonebgorges
  • Status changed from new to reviewing

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.

#46 in reply to: ↑ 44 ; follow-up: @SergeyBiryukov
2 years ago

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.

#47 in reply to: ↑ 46 @boonebgorges
2 years ago

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.

@boonebgorges
2 years ago

#48 in reply to: ↑ 43 @bobbingwide
2 years ago

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.

#49 @bobbingwide
23 months ago

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/

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


23 months ago

@jorbin
23 months ago

#51 @jorbin
23 months ago

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.

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


23 months ago

@dlh
23 months ago

#53 @dlh
23 months ago

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.

#54 @boonebgorges
23 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

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


22 months ago

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


20 months ago

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


17 months ago

Note: See TracTickets for help on using tickets.