Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#5427 closed enhancement (fixed)

WordPress should not include a file indicated by a URL query string that has not been specified in an add_submenu_page call

Reported by: johnbillion Owned by:
Milestone: 2.8.1 Priority: normal
Severity: normal Version: 2.3.1
Component: Menus Keywords: needs-patch add_submenu_page
Focuses: Cc:


Brought up on wp-hackers: http://comox.textdrive.com/pipermail/wp-hackers/2007-November/016405.html

It's possible to include any file within the plugins directory into the admin interface simply by passing the filename as the page parameter to any file within wp-admin.

Steps to reproduce:

  1. Login to your WordPress admin panel and visit the following URL: www.yourblog.com/wp-admin/edit.php?page=hello.php
  2. The file wp-content/plugins/hello.php will be include()-ed and will be in the scope of all the WordPress functions.

Try it with any file you have in your plugins directory. The activation status of a plugin is irrelevant as any file within the plugins directory can be included, including those in subdirectories (eg. akismet/akismet.php).

Only files that have been specified as the file paramemeter in add_submenu_page (or any of the wrapper functions) should be included via the page parameter in wp-admin.

Attachments (2)

5427.diff (1.5 KB) - added by DD32 8 years ago.
5427.2.diff (1.5 KB) - added by DD32 8 years ago.

Download all attachments as: .zip

Change History (15)

8 years ago

#1 @DD32
8 years ago

  • Keywords has-patch added; needs-patch removed

Attached a patch which checks to see if the page being loaded has been specified as a Top level page, or as a submenu page.

If a user attempts to direct it to a page which is not registered as being a menu, it die's with a message.

This could cause problems with plugins which link to pages via the admin pages which do not have a set submenu page. I doubt there are many plugins which would do that, And those that do, Should register a callback and include the file that way instead.

I was unsure where to put the validate function, If anyone thinks it should be commited, i'll add PHPDoc.

#2 @darkdragon
8 years ago

Is it possible to optimize the loops by using in_array() and/or array_search()?

It appears the top menu search is basically duplicating in_array(). I'm unsure about the second. It is okay, since you most likely won't be able to use in_array() unless you know the top level menu item. I think iterating through the top menu and then using in_array() to prevent looping on the sub menu will work.

Just a suggestion, however, it is unlikely to matter since most admin menus are not going to have 100s or even thousands of items. Might be premature optimization, but I think the coding part would justify the change. The testing part, I'm not so sure of.

#3 @DD32
8 years ago

It has to loop regardless AFAIK.

The key for the main menu's is in $menu[ <Keynumber> ][2], and for subpages, its in $submenu[ <MainKey> ][ <PageKey> ][2]

in_array() would only search the main array for the object existing as a value, and since its an array in each case, its impossible to tell. The only way to optimize that that i can think of, is to have a seperate array of files built when the pages are being added, it could then use in_array() to determine if its a valid page.

#4 @darkdragon
8 years ago

I did not know that. What is in [0] and [1]? It might be worth doing some clean up on the menu and submenu. There shouldn't be that much plugin breakage since most plugins should be using add_menu_page and add_submenu_page().

However, in the short term, I think your patch will solve the problem until which point the cleanup is performed. I remember some talk of doing that. This might give some motivation to doing something.

#5 @DD32
8 years ago

The $menu array has a structure like this:

  42 => 
      0 => string 'PHP Run' (length=7)
      1 => string 'administrator' (length=13)
      2 => string 'php-run/app.php' (length=15)
      3 => string 'PHP Run' (length=7)

the $submenu has a structure like this:

  'options-general.php' => 
      40 => 
          0 => string 'Miscellaneous' (length=13)
          1 => string 'manage_options' (length=14)
          2 => string 'options-misc.php' (length=16)
      41 => 
          0 => string 'DeviantArt' (length=10)
          1 => string 'administrator' (length=13)
          2 => string 'ddeviantart/dda_options.php' (length=27)
          3 => string 'dDeviantArt' (length=11)

[0] is the Link title, [1] is the permission required for it, [2] is the file that its been told to include, [3] is the Page title(Which is manually set for hte default pages when they're called directly).

It might be possible to simplify it down a bit, so that the submenu search only takes place for the file we're looking at right now.. have a look at the extra patch, There was a reason why i didnt do that before i think, But i cant seem to find a reason it wouldnt work.

8 years ago

#6 @darkdragon
8 years ago

Err, sorry, cool then. Just need a call to the commit team to get it in then. I think the optimization problem isn't a big deal, since there isn't any other PHP method for searching multidimensional arrays quickly. Also, it is unlikely that there will be enough pages added to any given administration menu that would have enough pages that would slow down the loops in any given degree.

#7 @darkdragon
8 years ago

I mean large time amount. Need to bump this down to 2.5, since it has a patch, but need to evaluate what the risk associated to committing the ticket is.

How many plugins link to pages which aren't part of any top or submenu?

#8 @darkdragon
8 years ago

  • Milestone changed from 2.6 to 2.5

#9 @DD32
8 years ago

How many plugins link to pages which aren't part of any top or submenu?

I'm not too sure.

none of the ones i use do, but there might be one, A fancy couple of greps through wp-plugins might lead to something.. I Guess i should do a checkout of the entire plugins list and have a look for some myself.

#10 @Denis-de-Bernardy
6 years ago

  • Component changed from Administration to Menus
  • Milestone changed from 2.9 to Future Release
  • Type changed from defect (bug) to enhancement

broken patch

#11 @Denis-de-Bernardy
6 years ago

  • Keywords needs-patch added; has-patch removed

#12 @johnbillion
6 years ago

r11595 and r11596 seem to address this issue I think

#13 @ryan
6 years ago

  • Milestone changed from Future Release to 2.8.1
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.