Make WordPress Core

Opened 16 years ago

Closed 16 years ago

Last modified 16 years ago

#8317 closed defect (bug) (fixed)

New installs: admin has user_level 0

Reported by: mr-pete's profile Mr Pete Owned by: jacobsantos's profile jacobsantos
Milestone: 2.7.1 Priority: normal
Severity: major Version: 2.5
Component: Upgrade/Install Keywords:
Focuses: Cc:

Description

On a fresh install of WordPress, the admin user begins with user_level == 0 (even thought level_10 works fine.)

This seems rather serious to me. It breaks any plugin, widget or theme feature that makes use of $user_level to identify administrators.

It's easy to check:

global $current_user;
get_currentuserinfo();
echo "level:".$user_level."<br />";
exit;

This was not a problem in 2.3.3; it is a problem from 2.5 onward at least through the current trunk.

All of my test systems have user level 10, so it somehow gets "fixed" after running a while. However, in the first week of my new plugin's release, I have had several user reports that my plugin is broken, and I traced it down to this issue. It wasn't my plugin, it was the fact that they had new installs of WordPress.

Attachments (2)

rolefix.patch (3.6 KB) - added by Mr Pete 16 years ago.
Proposed patch to fix
rolefix.diff (3.6 KB) - added by Mr Pete 16 years ago.
Same thing with *.diff ext'n

Download all attachments as: .zip

Change History (34)

#1 @DD32
16 years ago

  • Version set to 2.5

for a start (while it doesnt rule out this 'bug') you should probably switch to using capabilities instead for those who use finer grained control methods such as the Role manager plugin (ie. 'manage_options' or 'administrator' or 'publish_posts', etc) - thats assuming the versions of WP you're wanting to support support it (I have no idea when it was introduced)

#2 @DD32
16 years ago

  • Keywords reporter-feedback added

I cant reproduce it.

After globalising $user_level instad:

global $user_level;
get_currentuserinfo();
echo "level:".$user_level."<br />";

and running that code 2 page loads after a clean install, it appears to be working for me.

#3 @jacobsantos
16 years ago

Seems invalid to me.

It shouldn't affect the core of WordPress, since the core uses capabilities instead. The user_level has been deprecated since 2.0 or 2.1 I think. Ever since the new WP_Role class was added.

It will affect older plugins or plugins which use user_level instead of capabilities.

#4 @Mr Pete
16 years ago

My bad, yes it's globalized $user_level, not $current_user.

I easily replicated by downloading a fresh copy of any rev from 2.5 to 2.7 and installing from scratch.

It does not affect the core... it affects any plugin that uses $user_level.

The code AND documentation say that $user_level will someday be deprecated. But not now.

Perhaps the "fix" is to explicitly deprecate the old globals now :)

#5 @westi
16 years ago

  • Owner changed from anonymous to westi
  • Status changed from new to assigned

Please can you supply a simple example plugin which demostrates this issue as we can't reproduce it and the code looks fine to me too.

#6 @Mr Pete
16 years ago

Sure! How about the following. I saved as a php file in wp-content/plugins in yesterday's trunk download, in 2.5, and in 2.3.3. Works fine in 2.3.3, not after. Remember to use a clean new database... your existing tables probably already have some value for admin user_level :) Just activate the plugin and then look at the bottom of the page.

<?php
/*
Plugin Name: UserLevel Test
Plugin URI: http://blogs.icta.net/plugins/
Description: Generic simple test plugin
Version: 0.1
Author: Mr Pete
Author URI: http://blogs.icta.net/plugins/
*/


add_action('wp_footer', 'test_foot', 9999999999);
add_action('admin_footer', 'test_foot', 999999999);
function test_foot()
{
	global $user_level;
	get_currentuserinfo();
	
	echo "<div>User level is: $user_level<br/>User can level_10: ".(current_user_can('level_10') ? "yes" : "no")."</div>";
}
?>

#7 @Mr Pete
16 years ago

Idea: right now I'm testing on PHP 4.4.4 ... I'll try some other PHP's in a bit...

#8 follow-up: @ryan
16 years ago

  • Milestone 2.7 deleted
  • Resolution set to worksforme
  • Status changed from assigned to closed

Performed new install in clean DB with both PHP 4 and 5 and it works for me.

#9 in reply to: ↑ 8 ; follow-up: @Mr Pete
16 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Replying to ryan:

Performed new install in clean DB with both PHP 4 and 5 and it works for me.

Please recheck carefully. I've now rerun the test several times, using the current trunk.

Linux, php 5.2.6 -- works fine
Linux, php 4.4.9 -- FAILS as described above
Windows XP, php 5.2.1 -- works fine
Windows XP, php 4.4.4 -- FAILS as described above

This appears to be an issue with PHP4 vs PHP5 support. Perhaps some kind of object code?

#10 in reply to: ↑ 9 @Mr Pete
16 years ago

Linux, php 5.2.6 -- works fine
Linux, php 4.4.9 -- FAILS as described above
Windows XP, php 5.2.1 -- works fine
Windows XP, php 4.4.4 -- FAILS as described above

Sorry about the formatting. Wonder if this will work...

#11 @Mr Pete
16 years ago

 Linux, php 5.2.6 -- works fine
 Linux, php 4.4.9 -- FAILS as described above
 Windows XP, php 5.2.1 -- works fine
 Windows XP, php 4.4.4 -- FAILS as described above

Argh. Need to use preview!

#12 @ryan
16 years ago

It looks like WP_User::get_role_caps() isn't expanding the role into the full caps list (allcaps) during install. Thus WP_User::update_user_level_from_caps() doesn't see the levels. If someone wants to track this down further, go for it.

#13 @jacobsantos
16 years ago

  • Component changed from General to Upgrade
  • Keywords early added; reporter-feedback removed
  • Milestone set to 2.8
  • Owner changed from westi to jacobsantos
  • Priority changed from normal to low
  • Severity changed from normal to major
  • Status changed from reopened to new

I'll check this out. Thanks for finding the cause, should be easy now.

If it was important more people would have had this problem or reported it sooner.

#14 @jacobsantos
16 years ago

  • Keywords early removed

Probably should let developers decide that one.

#15 @Mr Pete
16 years ago

Re: lack of recognition...

I would have never guessed this was the issue. In my plugin, what was reported was certain users reported that crucial functions were not working correctly. That's all I knew. And, like you, I was unable to replicate the problem.

I suspect a lot of devs would just ignore the issue. After all, what can you do about a problem that not everyone sees, and you can't duplicate it.

I happen to have a prof'l software-breaking background, so I continued to go after it. And finally I found a repeatable test case.

I just searched the dozen plugins I am thinking of using, and googled for a couple of minutes (if anyone can grep the plugin svn this would be easy :)

Here are a few plugins most likely affected in some way by this: openwebanalytics, wp_shopping_cart, social_networking, dashboardopts, subscribe2, theme modern_blue.

#16 @Mr Pete
16 years ago

btw, ryan, thanks for debugging it to the next level!

#17 @DD32
16 years ago

I just tried to trace the bug, Utterly impossible, Seems like WP_User::$caps isnt being populated at all, and i couldnt work out why

If i was to replace:

http://trac.wordpress.org/browser/trunk/wp-includes/capabilities.php#L107

 $this->roles = get_option( $this->role_key );

with a var_export() a working role key, Everything would work as expected, So my thinking is that the WP_Roles class might not be picking up the role/capabilities list correctly.

#18 @Mr Pete
16 years ago

I'm on the hunt... a nice little puzzle!

Tracked down to this, so far. Not done yet...

During install, in wp-admin\include\upgrade.php line 68, set_role('administrator') ultimately calls get_role_caps() in capabilities.php.

PHP5: the result is an array with 'administrator' and all associated caps, including 'level_10'.

PHP4: the result is an array with ONLY 'administrator' and nothing else.

#19 @Mr Pete
16 years ago

[How do you enable Wiki Formatting? I can't get bullets to work...]

End of current debug pass:

  • add_cap() appears to work ok in php4 and php5
  • However, by the time get_role_caps() is called, $role->capabilities is an empty array in php4 but ok in php5

Feel free to pick this up, anyone!

Debug hints:

  • Create a temp database, have phpMyAdmin open to quickly select all tables and drop... to reset before each debug run
  • Have httpd.conf open to swap between php4/5 as needed

(I'm using my WP Tuner tools, w/ a couple of minor mods to function during install)

#20 @Mr Pete
16 years ago

Solved it. Interesting bug! I learned a bit of new stuff about php. And... I believe this same pattern is a problem for some other code.

The issue: PHP5 automagically passes and returns objects by reference. PHP4 passes and returns object copies.
Documentation of the effect: http://www.php.net/manual/en/language.references.return.php

Note: Unlike parameter passing, here you have to use & in both places - to indicate that you return by-reference, not a copy as usual, and to indicate that reference binding, rather than usual assignment, should be done for $myValue.

Correct use of get_role() everywhere:

   $role =& get_role( 'administrator' );

Incorrect, and current, use of get_role everywhere:

   $role = get_role( 'administrator' );

I changed all use of get_role to =& in schema.php and capabilities.php and the problem was solved.

This is a VERY easy and simple fix. Any chance it can get bumped?

NOTE: I also searched the code base for other return-by-reference functions. There are MANY of these. I checked a few for correct calling syntax, and 90% were wrong. (get_categor* calls). Obviously (?) it is not a big deal if the caller does not intend to modify the object.

However, any situation where the object needs to be changed means a PHP4 incompatibility.

What a pain!

#21 @Mr Pete
16 years ago

  • Cc jacobsantos added
  • Priority changed from low to normal

#22 @Mr Pete
16 years ago

  • Cc jacobsantos removed

oops.

#23 @ryan
16 years ago

Let's just change "function &get_role( $role )" to "function get_role( $role )". Does doing that fix the problem for you?

#24 @Mr Pete
16 years ago

Ummm... that can't possibly fix it ;)

Look at schema.php -- It grabs a role object, then uses it to insert a bunch of capabilities.

In PHP5, it automatically grabs a reference to the original, and all is well. Doesn't matter whether you "&" or not.

In PHP4, unless you "&" both the declaration AND use, it grabs a COPY of the object. So, the inserted capabilities never make it into the original object instantiation!

In PHP4, declaring the function with "&" means it is capable of returning a reference. Using with "&" means it actually grabs a reference. Eliminate either one, or both, and you get a copy.

Thus, to code compatibly for PHP4 and PHP5, you have to use "&" in both declaration and use.

#25 @Mr Pete
16 years ago

(In PHP5, the harder thing is to get a copy of the original object... requires some tricky code to get a copy in a way that's compatible with both PHP4 and PHP5)

As I suggested above, if this is misunderstood in this case, (and I admit I never looked into this myself until now), I suspect this could easily be an issue elsewhere in the code base.

#26 @Mr Pete
16 years ago

Return-by-reference is a critical issue any time the caller needs to modify the referenced object.

Here's a complete list of core function declarations that return references. I marked them based on the code comments, using a quick inspection:

  • "AAA" claim to return array (reference)
  • "*" claim to be capable of returning an object (reference)
  • "xxx" return nothing or something else

AFAIK, only the eleven "*" functions might be a problem.

----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\capabilities.php' :
*** E:\tech\cvs\wp\temp\wp-includes\capabilities.php(213): 	function &get_role( $role ) {
Found 'function\s*\&' 1 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\category.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\category.php(39): function &get_categories( $args = '' ) {
*** E:\tech\cvs\wp\temp\wp-includes\category.php(76): function &get_category( $category, $output = OBJECT, $filter = 'raw' ) {
AAA E:\tech\cvs\wp\temp\wp-includes\category.php(277): function &get_tags( $args = '' ) {
*** E:\tech\cvs\wp\temp\wp-includes\category.php(309): function &get_tag( $tag, $output = OBJECT, $filter = 'raw' ) {
Found 'function\s*\&' 4 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\comment.php' :
*** E:\tech\cvs\wp\temp\wp-includes\comment.php(133): function &get_comment(&$comment, $output = OBJECT) {
AAA E:\tech\cvs\wp\temp\wp-includes\comment.php(485): function &separate_comments(&$comments) {
Found 'function\s*\&' 2 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\http.php' :
*** E:\tech\cvs\wp\temp\wp-includes\http.php(90): 	function &_getTransport( $args = array() ) {
*** E:\tech\cvs\wp\temp\wp-includes\http.php(138): 	function &_postTransport( $args = array() ) {
*** E:\tech\cvs\wp\temp\wp-includes\http.php(1063): function &_wp_http_get_object() {
Found 'function\s*\&' 3 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\post.php' :
*** E:\tech\cvs\wp\temp\wp-includes\post.php(116): function &get_children($args = '', $output = OBJECT) {
*** E:\tech\cvs\wp\temp\wp-includes\post.php(209): function &get_post(&$post, $output = OBJECT, $filter = 'raw') {
*** E:\tech\cvs\wp\temp\wp-includes\post.php(1891): function &get_page(&$page, $output = OBJECT, $filter = 'raw') {
AAA E:\tech\cvs\wp\temp\wp-includes\post.php(1979): function &get_page_children($page_id, $pages) {
AAA E:\tech\cvs\wp\temp\wp-includes\post.php(2054): function &get_pages($args = '') {
*** E:\tech\cvs\wp\temp\wp-includes\post.php(3446): function &wp_get_post_revision(&$post, $output = OBJECT, $filter = 'raw') {
Found 'function\s*\&' 6 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\query.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\query.php(59): function &query_posts($query) {
AAA E:\tech\cvs\wp\temp\wp-includes\query.php(1562): 	function &get_posts() {
AAA E:\tech\cvs\wp\temp\wp-includes\query.php(2499): 	function &query($query) {
Found 'function\s*\&' 3 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\taxonomy.php' :
E:\tech\cvs\wp\temp\wp-includes\taxonomy.php(299): function &get_term($term, $taxonomy, $output = OBJECT, $filter = 'raw') {
AAA E:\tech\cvs\wp\temp\wp-includes\taxonomy.php(589): function &get_terms($taxonomies, $args = '') {
AAA E:\tech\cvs\wp\temp\wp-includes\taxonomy.php(1807): function &get_object_term_cache($id, $taxonomy) {
AAA E:\tech\cvs\wp\temp\wp-includes\taxonomy.php(1957): function &_get_term_children($term_id, $terms, $taxonomy) {
Found 'function\s*\&' 4 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\EnchantSpell.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\EnchantSpell.php(20): 	function &checkWords($lang, $words) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\EnchantSpell.php(49): 	function &getSuggestions($lang, $word) {
Found 'function\s*\&' 2 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\GoogleSpell.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\GoogleSpell.php(18): 	function &checkWords($lang, $words) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\GoogleSpell.php(36): 	function &getSuggestions($lang, $word) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\GoogleSpell.php(53): 	function &_getMatches($lang, $str) {
Found 'function\s*\&' 3 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpell.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpell.php(18): 	function &checkWords($lang, $words) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpell.php(37): 	function &getSuggestions($lang, $word) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpell.php(49): 	function &_getPLink($lang) {
Found 'function\s*\&' 3 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpellShell.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpellShell.php(18): 	function &checkWords($lang, $words) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\PSpellShell.php(60): 	function &getSuggestions($lang, $word) {
Found 'function\s*\&' 2 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\SpellChecker.php' :
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\SpellChecker.php(26): 	function &loopback(/* args.. */) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\SpellChecker.php(37): 	function &checkWords($lang, $words) {
AAA E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\classes\SpellChecker.php(48): 	function &getSuggestions($lang, $word) {
Found 'function\s*\&' 3 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\includes\general.php' :
*** E:\tech\cvs\wp\temp\wp-includes\js\tinymce\plugins\spellchecker\includes\general.php(44): function &getLogger() {
Found 'function\s*\&' 1 time(s).
----------------------------------------
Find 'function\s*\&' in 'E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php' :
xxx E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php(308):     function &reverse()
xxx E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php(342):     function &reverse()
xxx E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php(364):     function &reverse()
xxx E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php(386):     function &reverse()
xxx E:\tech\cvs\wp\temp\wp-includes\Text\Diff.php(408):     function &reverse()
Found 'function\s*\&' 5 time(s).
Search complete, found 'function\s*\&' 42 time(s). (14 files.)

#27 @Mr Pete
16 years ago

I'm browsing down quickly. Obviously the first one is affected.

Next one I see that will act differently in PHP5 and PHP4:

taxonomy.php line 114, function _cat_row()

	$category = get_category( $category, OBJECT, 'display' );
...
	$category->count = number_format_i18n( $category->count );

And the same at line 274, function link_cat_row()

In PHP4, the original category object is not changed because $category is a copy. In PHP5, the original would be changed if number_format_i18n() is anything but a nop.

I don't know if this is a problem or not. If the original should not be modified, the code is wrong for PHP5.

Every function that accepts OBJECT as a return type acts differently in PHP5 and PHP4. I don't immediately see other issues in the code base than what I've outlined here, but then I only spent a few minutes.

Again, this most likely impacts non-core code (all those "OBJECT" returns are there for a purpose, even though not used in the core code base.)

Overall, I suspect the actual core code bugs related to this are rare.

I suspect this is a potential security hole. A badly coded plugin has direct access to original objects in PHP5, because the objects (and arrays!) are returned by reference in all of the above functions.

Should this be documented? I dunno... I'm just a noob.

#28 @ryan
16 years ago

We return by reference in places where we really don't need to. Obviously get_role() is a place where we need to. Can you create a patch that uses "& get_role()" everywhere?

@Mr Pete
16 years ago

Proposed patch to fix

#29 @Mr Pete
16 years ago

First time with SVN (I've dev'd CVS before) and first time w/ WP. Is this about right?

@Mr Pete
16 years ago

Same thing with *.diff ext'n

#30 @ryan
16 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [10200]) Explicitly return ref for the sake of PHP4. Fixes user_level being empty when installing on PHP4. Props Mr Pete. fixes #8317 for trunk

#31 @ryan
16 years ago

(In [10201]) Explicitly return ref for the sake of PHP4. Fixes user_level being empty when installing on PHP4. Props Mr Pete. fixes #8317 for 2.7

#32 @ryan
16 years ago

  • Milestone changed from 2.8 to 2.7.1
Note: See TracTickets for help on using tickets.