Make WordPress Core

Opened 5 years ago

Last modified 4 years ago

#47690 reviewing defect (bug)

remove_submenu_page() doesn't remove corresponding entry from $_wp_submenu_nopriv

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch needs-testing dev-feedback
Focuses: administration Cc:

Description

It can sometimes be desirable to give access to a submenu to a user that wouldn't normally have access to it.

Calling remove_submenu_page() and then calling add_submenu_page() to re-register the screen with a different user capability doesn't work completely because the entry that gets added to the $_wp_submenu_nopriv global by add_submenu_page() doesn't get removed by remove_submenu_page().

This means the menu item appears but access to the screen is denied when user_can_access_admin_page() is called, resulting in a Sorry, you are not allowed to access this page error.

Attachments (1)

47690.diff (2.3 KB) - added by donmhico 5 years ago.
Reverse the actions from add_submenu_page() and changed the parameter variable names to be consistent with add_submenu_page()

Download all attachments as: .zip

Change History (6)

@donmhico
5 years ago

Reverse the actions from add_submenu_page() and changed the parameter variable names to be consistent with add_submenu_page()

#1 @donmhico
5 years ago

  • Keywords has-patch added; needs-patch removed

On my attachment 47690.diff, you'll notice on line 1715

remove_all_actions( $hookname );

This is to reciprocate the code from line 1360 of the function add_submenu_page().

I'm not sure if remove_all_actions( $hookname ) is the best solution. If the $hookname from the code

$hookname = get_plugin_page_hookname( $menu_slug, $parent_slug );

is unique for the add_submenu_page() then I suppose there's no harm using remove_all_actions( $hookname ). If it's not then we do need to somehow hold the $function passed on add_submenu_page() and use remove_action( $hookname, $function ) instead.

It is important to remove the action because the submenu page will be accessible using the direct link with the function callback as the output even if the revised remove_submenu_page() is called.

Another thing is, I changed the variable names passed on remove_submenu_page() from $menu_slug and $submenu_slug to $parent_slug and $menu_slug to make it consistent with the variable names used on add_submenu_page().

References:
https://developer.wordpress.org/reference/functions/add_submenu_page/
https://codex.wordpress.org/Function_Reference/remove_all_actions
https://codex.wordpress.org/Function_Reference/remove_action.

Last edited 5 years ago by donmhico (previous) (diff)

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


5 years ago

#3 @johnbillion
5 years ago

  • Milestone changed from 5.3 to 5.4
  • Owner set to johnbillion
  • Status changed from new to reviewing

I haven't had time to review this but thanks for the patch @donmhico , I'll give it some eyes during 5.4.

#4 @johnbillion
4 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.4 to Future Release

@donmhico Can you explain the special case handling of tools.php in your patch please?

#5 @johnbillion
4 years ago

  • Keywords dev-feedback added
Note: See TracTickets for help on using tickets.