Access - strict boolean checking [#472160]

After working a for long time with Drupal 6 exclusively, I caught myself writing the following menu item on Drupal 5.

$items[] = array(
  'path' => 'admin/settings/foo',
  'access' => array('administer foo'),
  // ...
);

As a non-empty array evaluates as TRUE, the code above grants access to all users. A whole new pitfall for those of us who worked with D6! :)

I've attached a patch to _menu_build that only accepts boolean TRUE to prevent such issues.

The hook_menu docblock tells us

"access": A boolean value that determines whether the user has access rights to this menu item. Usually determined by a call to user_access(). If omitted and "callback" is also absent, the access rights of the parent menu item will be used instead.

As this is expected to be a boolean value, the patch is not an API change (it is an implementation change).

Comment	File	Size	Author
#3	menu-D5.patch	1.23 KB	chx
	do_boolean_menu_access_5.patch	669 bytes	heine

Comments

Comment #1

chx commented 25 May 2009 at 13:33

And what about computed menu items? They might have a non-bool access. I would say that the security hardening should check against the specific problem which I admit is a valid concern:

// Protect against D6 style access
if (is_array($item['access']) && count($item['access']) == 1 && isset($item['access'][0]) && is_string($item['access'][0])) {
  $item['access'] = FALSE;
}

Comment #2

heine commented 25 May 2009 at 15:17

Right, while such computed items violate the API contract, even user module does it (logout), so it is probably widespread.

Comment #3

chx commented 25 May 2009 at 16:37

Status	File	Size
new	menu-D5.patch	1.23 KB

We discussed this with Heine and maybe we need a CHANGELOG and for sure need a new handbook page detailing this change. I can imagine a case where this breaks: someone calls a hook which lists something and uses that directly in access. Ie. if there is a module that implements my info hook then there is access to this page. However, it's really an edge case you need lists of strings and exactly one string. If you use say machinereadable name => humanreadable name, we are ok. If there is a structure returned, we are ok again.