WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 months 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: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Role/Capability Keywords: has-patch dev-feedback
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 (4)

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

Download all attachments as: .zip

Change History (34)

comment:1 @kevinB3 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 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.5.1

comment:3 @kevinB3 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 3 years ago by kevinB (previous) (diff)

comment:4 @johnbillion3 years ago

  • Cc johnbillion added

comment:5 @georgestephanis3 years ago

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

comment:6 @georgestephanis3 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?

@nacin3 years ago

comment:8 follow-up: @nacin3 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 @DrewAPicture2 years ago

  • Keywords has-patch needs-testing added

comment:10 @nacin2 years ago

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

comment:11 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:12 in reply to: ↑ 8 ; follow-up: @johnbillion2 years 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: @nacin2 years 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 @johnbillion2 years ago

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

comment:15 @nacin23 months ago

  • Milestone changed from 3.7 to 3.8

comment:16 @nacin21 months ago

  • Milestone changed from 3.8 to Future Release

Still unsure what the right fix is.

comment:17 in reply to: ↑ 13 @Solinx14 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 @bobbingwide9 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 @bobbingwide9 months ago

#29714 was marked as a duplicate.

@bobbingwide9 months ago

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

comment:20 @DrewAPicture9 months ago

#29714 was marked as a duplicate.

comment:21 @greenshady9 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 9 months ago by greenshady (previous) (diff)

comment:22 @jfarthing848 months ago

Just encountered this myself.

comment:24 @slackbot7 months ago

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

comment:25 @johnbillion7 months 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

comment:26 @gotVirtual6 months ago

so basically this is never going to get fixed?
thats about what i have taken away from this.

comment:27 @valendesigns5 months ago

@gotVirtual It's likely to not get fixed unless someone writes the unit tests to prove its viability and effectiveness. I would be happy to write unit tests if there was some sample code that went along with this ticket to demonstrate the issue.

comment:28 @DrewAPicture5 months ago

  • Keywords needs-refresh added
  • Milestone changed from 4.2 to Future Release

No meaningful activity in 6 weeks, we still need unit tests, and the patch needs a refresh. Punting to future release.

@joshlevinson3 months ago

Patch refresh

comment:29 @joshlevinson3 months ago

  • Keywords needs-refresh removed

@joshlevinson3 months ago

Unit test

comment:30 @joshlevinson3 months ago

  • Keywords dev-feedback added; needs-testing needs-unit-tests removed

I've added a test to cover this specific ticket. A couple notes:

  • The test doesn't provide full coverage for user_can_access_admin_page; it just verifies that 22895.3.diff works as intended in a straightforward way.
  • Since the admin menu functionality involves the use of globals and modifying those globals directly in various files (vs. function bodies), a lot of code from the menu "API" had to be brought within this test. I'm sure that once progress is made on #12718, this will change. Disclaimer: I'm a novice unit tester, so there may be a better way than how I managed to do this. I couldn't think of one.
Note: See TracTickets for help on using tickets.