WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#19222 closed enhancement (fixed)

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

Reported by: elyobo Owned by: 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 8 years ago.
Patch against SVN trunk

Download all attachments as: .zip

Change History (10)

@elyobo
8 years ago

Patch against SVN trunk

#1 @scribu
8 years ago

  • Keywords reporter-feedback has-patch added

When is this not the case?

#2 @elyobo
8 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
8 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
8 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
8 years ago

  • Cc deltafactory added

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

#6 @glatze
8 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
8 years ago

  • Cc glatze added

#8 @nacin
7 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
7 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.