WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 weeks ago

#22895 accepted defect (bug)

user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages

Reported by: kevinB Owned by: johnbillion
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.5
Component: Role/Capability Keywords: has-patch needs-testing needs-unit-tests
Focuses: Cc:

Description

Utilization of the new separation edit_posts /create_posts capability separation reveals a flaw in admin menu privilege checking.

The issue occurs when:

  1. For any post type other the "post", the user has $type->cap->edit_posts but not $type->cap->create_posts
  1. User also does not have a manage_terms capability for any associated taxonomies

In that situation, access to "edit.php?post_type=whatever" fails unless the user has the "edit_posts" cap for the "post" type.

This occurs because:

  1. wp-admin/includes/menu.php removes solitary submenus that have the same destination as the parent
  1. get_admin_page_parent() returns nullstring if there is no $submenu item
  1. user_can_access_admin_page() performs a type-sensitive capability check only if get_admin_page_parent() returns an existing $submenu key.

For now, my plugin workaround is to hook into 'admin_menu' and add a dummy submenu with nullstring caption.

Attachments (2)

22895.diff (645 bytes) - added by nacin 2 years ago.
22895-2.diff (1.5 KB) - added by bobbingwide 3 months ago.
Don't update the global $pagenow. Use a copy in $pagetest.

Download all attachments as: .zip

Change History (27)

comment:1 @kevinB2 years ago

  • Summary changed from user_can_admin_menu() is Type-Insensitive for Users who can't create pages to user_can_admin_menu() is Type-Insensitive for Users who Can't Create Pages

comment:2 @nacin2 years ago

  • Milestone changed from Awaiting Review to 3.5.1

comment:3 @kevinB2 years ago

This will be obvious to some, but note that this issue only occurs when:

  • $type_obj->cap->create_posts is defined to != $type_obj->cap->edit_posts
Last edited 2 years ago by kevinB (previous) (diff)

comment:4 @johnbillion2 years ago

  • Cc johnbillion added

comment:5 @georgestephanis2 years ago

Blocking off some time on Saturday to wrangle this if noone else beats me to it.

comment:6 @georgestephanis2 years ago

I'm trying to get the capabilities set up properly on a post type to duplicate this ... any chance you can paste me your register_post_type capabilities that you're using?

@nacin2 years ago

comment:8 follow-up: @nacin2 years ago

  • Milestone changed from 3.5.1 to 3.6

22895.diff is a hack that allows user_can_access_admin_page() to respect edit.php?post_type= pages. It still does not respect edit-tags.php?taxonomy= pages, though, and neither does get_admin_page_parent(). Overall, the whole thing is a mess and needs to be rewritten. Fun. Even #12718 only touches the API, rather than the inner guts...

Since this is a new feature, and I'm not really sure of the possible breakage that could be caused by 22895.diff (one problem with the admin menu code is it is completely unmaintainable), I'm going to move this ticket to 3.6. So, 3.5 has create_posts with a caveat: It only works if the user has edit_posts, such as when the post type leverages edit_posts (like attachments do). Not the best, but also not something to go messing with a point release. Think of it as version 0.1.

comment:9 @DrewAPicture22 months ago

  • Keywords has-patch needs-testing added

comment:10 @nacin20 months ago

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

comment:11 @wonderboymusic19 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:12 in reply to: ↑ 8 ; follow-up: @johnbillion18 months ago

Replying to nacin:

So, 3.5 has create_posts with a caveat: It only works if the user has edit_posts, such as when the post type leverages edit_posts (like attachments do).

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

comment:13 in reply to: ↑ 12 ; follow-up: @nacin17 months ago

Replying to johnbillion:

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

So 22895.diff would be the wrong fix? Patch welcome for sure.

comment:14 @johnbillion17 months ago

At the moment, I'm not sure. I'll find some time to look at it.

comment:15 @nacin17 months ago

  • Milestone changed from 3.7 to 3.8

comment:16 @nacin15 months ago

  • Milestone changed from 3.8 to Future Release

Still unsure what the right fix is.

comment:17 in reply to: ↑ 13 @Solinx8 months ago

Replying to nacin:

Replying to johnbillion:

I've just run into this problem. A user who doesn't have the create_posts capability for a custom post type can't see the edit menu for that post type unless they also have the general edit_posts capability. This is caused by $parent in user_can_access_admin_page() being empty when it shouldn't.

So 22895.diff would be the wrong fix? Patch welcome for sure.

I've also run into this exact issue as described by johnbillion.

To be honest I found the code dealing with menu rights to be quite a mess. A rewrite or at least some documentation wouldn’t hurt, but obviously both take quite a bit of time. In the meantime there are several possible solutions to this problem.

  1. Address $parent being empty - quick fix
  2. Add $typenow to $pagenow - quick fix
  3. Consider changes that are (potentially) more impactful and time consuming

  1. Address $parent being empty

To address the problem by setting a $parent value the following code could be added to get_admin_page_parent() at line 1525:

	if ( !empty($typenow) ) {
		foreach ( $menu as $menuitem ) {
			if ( $menuitem[2] === "$pagenow?post_type=$typenow" ) {
				return $parent_file = "$pagenow?post_type=$typenow";
			}
		}
	}

Besides in user_can_access_admin_page(), get_admin_page_parent() is also used to determine page title and page hook.

The above code does not influence the former. I have not tested whether it influences the latter, but if it does, it would be for the better. Plugin and theme authors would not expect the hook of the post_type edit page to change (which I think it does upon becoming a parent menu item after removing the create_posts capability).

The con to this solution is that the page being visited in reality has no parent page. There is essentially nothing wrong with $parent being empty in user_can_access_admin_page(), because there is no parent. For this reason I prefer solution 2 and did not look into the above points any further.


  1. Add $typenow to $pagenow

The initial solution I came up with closely resembles what you did and is also located at the same place in user_can_access_admin_page():

if ($pagenow === 'edit.php' && isset($_REQUEST['post_type']))
		$curpage .= '?post_type=' . $_REQUEST['post_type'];	
	else
		$curpage = $pagenow;

Our solutions effectively differ at only one point. I also check whether the current page is an 'edit.php' page. My reason for adding this check was to limit any possible side effects - this issue is limited to edit.php pages.

The con to this solution is that it feels like a hack and does not help improve the quality of the code structure. That said, I strongly recommend implementing this solution as a short term fix to this particular issue.


  1. Consider changes that are (potentially) more impactful and time consuming

As noted, the $parent value being empty in user_can_access_admin_page() is actually as it should be. If you consider that, there are a few problems with user_can_access_admin_page(), where the second solution is a fix targeted to this specific case.

a.) Admin pages with an empty $parent value are passed by blacklists targeted at submenu items.

Unless submenu items can be created without a parent menu item there is no sense to this. The specific issue of this ticket is resolved if the checks for submenu items are removed. It is the Posts submenu item that causes the permission denied message, because that is the only ‘edit.php’ submenu without GET strings attached in the keys.

I do not know what the full impact of removing the submenu blacklists will be. This will have to be studied.

b.) The permission blacklists used in case $parent is empty include GET strings in the array keys, while the checks are made without GET strings

Fixing this inconsistency for this specific case is what solution 2 is aimed at. The code is not quite future proof, but that may not be a concern if a rewrite is planned anyhow.

c.) A blacklist system is used to check whether a user has access to a certain page.

Blacklisting is generally a bad idea. Any rewrite attempt should use whitelists instead.

The use of blacklists is not limited to the situation where $parent is empty. The user_can_access_admin_page() function defaults to true if no matches can be found - which effectively means the whole function is designed as a blacklist system.

Each of these options need further attention if they are to be pursued, and could then just as well be the start of a rewrite.


In sum, I would for now recommend implementing solution 2, or your earlier code, in the upcoming update. It may not be perfect, but it does solve a production issue and can be implemented now without any apparent negative effects.

I hope there is still time to make this part of 4.0.

Cheers,

  1. van Dam

comment:18 @bobbingwide3 months ago

Just been talking to johnbillion at #wpldn contributor day.
Apparently #29714 is a duplicate of this defect.
Interestingly the workaround that I documented doesn't work
but I have applied my patch to 4.1-beta2 and it works for me.

He also suggested that I should point out that Nacin's patch would lead to the same side effects that I was getting with the first version of the workaround. Rather than change $pagenow it should be copied to another variable and then the tests be performed using the copy.

So, the challenge that now remains is to create some form of unit test from my source code.

comment:19 @bobbingwide3 months ago

#29714 was marked as a duplicate.

@bobbingwide3 months ago

Don't update the global $pagenow. Use a copy in $pagetest.

comment:20 @DrewAPicture3 months ago

#29714 was marked as a duplicate.

comment:21 @greenshady3 months ago

I believe I ran into a similar issue, but I'm not certain if it's the same yet. I wanted to make sure I didn't need to open another ticket first.

I have two post types. The first has a normal, top-level menu. The second is a sub-menu of the first.

This is how it's set up:

- Type A
--- Type A (all items)
--- Type B (all items)

Users with the cap to view Type B get the access denied message because they don't have the cap to view Type A.

My workaround has been to check if the user has the cap to view Type A. If not, make Type B the top-level menu.

Last edited 3 months ago by greenshady (previous) (diff)

comment:22 @jfarthing842 months ago

Just encountered this myself.

comment:24 @slackbot3 weeks ago

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

comment:25 @johnbillion3 weeks ago

  • Keywords needs-unit-tests added; 3.7-early removed
  • Milestone changed from Future Release to 4.2
  • Owner set to johnbillion
  • Status changed from new to accepted
Note: See TracTickets for help on using tickets.