Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#19222 closed enhancement (fixed)

Undefined index and incorrect behaviour in WP_User::set_role().

Reported by: elyobo's profile elyobo Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: minor Version: 3.3
Component: Role/Capability Keywords: has-patch
Focuses: Cc:

Description

set_role in WP_User assumes that if the count of roles is one, then the only role present is at index 0; it seems that this is not always the case.

This is easily fixed by using current($this->roles) instead of $this->roles[0]; the behaviour is otherwise the same, but it will work without triggering a notice and will correctly short circuit the function when the only element is at a non-zero index.

A patch against the current SVN trunk is attached.

Attachments (1)

fix-user-set-role-with-non-zero-index.diff (486 bytes) - added by elyobo 12 years ago.
Patch against SVN trunk

Download all attachments as: .zip

Change History (10)

@elyobo
12 years ago

Patch against SVN trunk

#1 @scribu
12 years ago

  • Keywords reporter-feedback has-patch added

When is this not the case?

#2 @elyobo
12 years ago

The ordering is in place when I instantiate a new WP_User, e.g.

$id   = 1; // Some user ID
$role = 'some_role';
$user = new WP_User($id);
$user->set_role($role);

We do remove the standard wordpress roles, because we implement our own structure, so perhaps this is the cause. It seems that we should be able to do this without breaking core though .

Regardless of the cause, assuming the existence of array index zero is potentially buggy. In this case things still work, because the check in that line will fail (if permissive error reporting is used; ours is agressive, so this throws an exception) and it continues through the function to set the roles. In other areas the same logic may cause bigger problems.

#3 @scribu
12 years ago

  • Component changed from General to Role/Capability
  • Keywords reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

#4 @elyobo
12 years ago

If you're scheduling it for a future release rather than fixing it now, please note that there are three areas that assume the index as well that might be worth fixing at the same time.

  • wp-includes/capabilities.php line 724 (fixed in the attached patch)
  • wp-admin/admin-ajax.php lines 912 and 915

Cheers

#5 @deltafactory
12 years ago

  • Cc deltafactory added

I have observed it as well. Adding to cc to see how it all ends up.

#6 @glatze
12 years ago

I have the same problem when I set the role this way

wp_update_user(array('ID' => $user_id, 'role' => $role));

because wp_update_user() calls wp_insert_user() which calls set_role().

#7 @glatze
12 years ago

  • Cc glatze added

#8 @nacin
12 years ago

  • Milestone changed from Future Release to 3.5

Looks good. Let's make sure we eliminate this in ajax-actions as well, as mentioned in comment:4.

#9 @nacin
11 years ago

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

In [21866]:

Don't use hard-coded indexes when dealing with an array of roles. props elyobo. fixes #19222.

Note: See TracTickets for help on using tickets.