Hi, all,
Some of you may be aware that my wiki experienced a security breach this afternoon. The error was a minor lapse in assigning $wgGroupPermissions['user'][] = 'centralauth-merge'. This seems like an inocuous error to start with, but it had the following impact:
1. The value $wgGroupPermissions['user'][0] was assigned to 'centralauth-merge'. 2. In getting a user's rights, we use array_keys( array_filter( $wgGroupPermissions[$group] ) ), which added a right 0 to the list. 3. PHP has a nasty little habit of letting 0 == 'some string'. Witness the result:
$f = array( 'foo', 'bar', 'baz', 0, 'anne' ); print in_array( 'blah', $f );
1
4. User::isAllowed returned true for all rights if a user was in the 'user' group, because it uses in_array.
I spoke to the people in ##php, and supposedly, this is expected behaviour. They suggested we use in_array with 'true' as the final parameter, which uses ===.
I've done this in r40946. For those working on a revision BEFORE this one, PLEASE be careful. Double-check that you've assigned group permissions with $wgGroupPermissions['user']['right'] = true, rather than the (wrong) version I've used.
Thanks,
On Wed, Sep 17, 2008 at 3:27 AM, Andrew Garrett andrew@epstone.net wrote:
I spoke to the people in ##php, and supposedly, this is expected behaviour. They suggested we use in_array with 'true' as the final parameter, which uses ===.
There's usually no point in speaking with the people in ##php about things like this. The entire language is broken by design.
I've done this in r40946. For those working on a revision BEFORE this one, PLEASE be careful. Double-check that you've assigned group permissions with $wgGroupPermissions['user']['right'] = true, rather than the (wrong) version I've used.
Maybe it would be better to handle this case as "do what I mean" instead of dropping the rights assignment. I.e., check the array for numeric keys with string values, and add those on to the array properly.
Aryeh Gregor wrote:
Andrew Garrett wrote:
I spoke to the people in ##php, and supposedly, this is expected behaviour. They suggested we use in_array with 'true' as the final parameter, which uses ===.
There's usually no point in speaking with the people in ##php about things like this. The entire language is broken by design.
I've found ##php to be really useful if you feel like ranting and want an audience who will have almost, but not quite, no clue about what you're talking about. For complex design decisions, you're better off asking in #mediawiki.
I've done this in r40946. For those working on a revision BEFORE this one, PLEASE be careful. Double-check that you've assigned group permissions with $wgGroupPermissions['user']['right'] = true, rather than the (wrong) version I've used.
Maybe it would be better to handle this case as "do what I mean" instead of dropping the rights assignment. I.e., check the array for numeric keys with string values, and add those on to the array properly.
Or throw an exception, that would be a reasonable response to configuration errors.
-- Tim Starling
On Thu, 18 Sep 2008 23:32:11 +1200, Tim Starling wrote:
Aryeh Gregor wrote:
Maybe it would be better to handle this case as "do what I mean" instead of dropping the rights assignment. I.e., check the array for numeric keys with string values, and add those on to the array properly.
Or throw an exception, that would be a reasonable response to configuration errors.
-- Tim Starling
Do what I mean does seem a bit fragile, since we could only guess what they mean, and hope that it keeps working without regular testing, even if new enhancements are added. I think sanity checking the structure and throwing an exception on any error would be the right thing to do.
wikitech-l@lists.wikimedia.org