Make WordPress Core

Opened 14 years ago

Closed 14 years ago

#15326 closed enhancement (fixed)

Always check capabilites in admin pages

Reported by: westi's profile westi Owned by: ryan's profile ryan
Milestone: 3.1 Priority: high
Severity: normal Version: 3.1
Component: Security Keywords: dev-feedback
Focuses: Cc:


WP_List_Table introduces a check_permissions() function which hides away the capabilities check inside the list table class so that it is easy to write a generic AJAX handler.

We should still have current_user_can() checks in the normal admin pages as it makes it easier to review for security holes.

Still doing it in the table classes is good defence in depth.

Attachments (1)

15326.diff (9.9 KB) - added by nacin 14 years ago.

Download all attachments as: .zip

Change History (18)

#1 @nacin
14 years ago

[16222] never hit the ticket.

wpdavis on IRC mentioned that he was getting 'Cheatin', uh?' on edit-tags.php. I couldn't track it down for a while, but sure enough, $tax isn't actually set at that point, or if it is, then it's because of a variable collision. Fixing now.

#2 @nacin
14 years ago

Also, what if we had check_permissions() instead just return a cap to check? Then we can just call current_user_can( $wp_list_table->cap() ). That cuts out redundant cycles and also makes it *very* clear what's going on. (It's arguably confusing why we're doing it twice.) We're also no longer really just using strings for capabilities, given the cap objects for taxonomies and post types.

#3 @nacin
14 years ago

(In [16253]) Set $tax as the taxonomy object, and kill off our colliding global from menu construction. see #15326.

#4 @westi
14 years ago

I don't like the current_user_can( $wp_list_table->cap()) because it still hides away the required cap in a lot of cases.

#5 @westi
14 years ago

(In [16776]) Restore some more cap checks for clarity. See #15326.

#6 @automattor
14 years ago

(In [16779]) Revert brokenness. See #15326.

#7 @westi
14 years ago

(In [16965]) Move post_type var setup back into edit.php and reinstate the cap check. See #15326

#8 @westi
14 years ago

(In [16970]) More caps back. See #15326

#9 @nacin
14 years ago

(In [16990]) Remove check_permissions() calls outside of AJAX context. Also only check for switch_themes in check_permissions() for the themes table. see #15326.

#10 @nacin
14 years ago

(In [16991]) Move check_permissions() out of ajax_response(). see #15326.

14 years ago

#11 @nacin
14 years ago

Noticed some issues with how we were still doing things.

We need to change wp_die() in every check_permissions() method to die('-1').

15326.diff kills off check_permissions() entirely.

The new method is called ajax_user_can(). You return current_user_can() through it. In admin-ajax, we then die with -1 if appropriate.

Renaming it allows us to ensure that no one is using it incorrectly, as we'd be returning true/false rather than dying.

Conservative alternative: die with -1 in each handler. No need to rename the function.

#12 @westi
14 years ago

I really like ajax_user_can - I've been wanting to rename the function and this makes the checks even more obvious.

I hate hiding the dies/wp_dies in a function.

#13 @nacin
14 years ago

(In [16992]) Replace check_permissions() with ajax_user_can(). New method returns true/false to current_user_can(), which we then handle in admin ajax. see #15326.

#14 @nacin
14 years ago

Ticket is complete. I just want some sanity checks on [16990] and [16992] to make sure I didn't miss anything.

#15 @nacin
14 years ago

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

Needs final, close review by ryan.

#16 @nacin
14 years ago

  • Keywords dev-feedback added

#17 @ryan
14 years ago

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

Looks good.

Note: See TracTickets for help on using tickets.