Harden checking of 'access callback' in menu system API
pwolanin - October 10, 2008 - 02:15
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | feature request |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
Description
SA-2008-63 saw a rash of contrib modules that were improperly updated to the new menu system. The fact that we use booleans for indicating access always allowed or never allow turns out to make it easier than it should to make an access bypass by doing a function call to user_access() (for example).
To harden this piece of the core APi going forward, chx proposed and I implemented a minor modification to the menu system to used defined strings rather than booleans.
#1
pretty simple patch.
#2
Could I have a bit more information? How are contrib modules getting this wrong?
#3
I'm not sure that we should make things a little bit harder when contrib modules can't do access control right. But that's my opinion...
#4
People are putting
<?php'access callback' => user_access('use PHP');
?>
which evaluates as TRUE.
afaik, it doesn't make any more work in contrib, just stops poorly upgraded D5 modules from having access bypasses when they completely ignore how the new menu system works.
#5
here's a patch that actually converts the few uses in core.
All tests pass with this patch: 6884 passes, 0 fails, 0 exceptions
@Dave Reid - this patch should make it harder for contrib to get it wrong, but not at all harder to get it right.
#6
looks like this needs work
#7
looks like the problem was just soe additional items in a test module that the prior patch misses
#8
#9
IMO, this is a result of people doing a rushed d5 to d6 upgrade. perhaps coder or deadwood contributed to the problem - i don't know. anyway, the problem is gone with d6 => d7 so i really think this is not very useful. IMO, this is babysitting. I don't feel strongly though.
#10
I'm not particularly fond of this patch, to be honest.
#11
The last submitted patch failed testing.
#12
submitting for re-testing after snafu this afternoon.
#13
The last submitted patch failed testing.