Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#15326 closed enhancement (fixed)

Always check capabilites in admin pages

Reported by: westi Owned by: 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 5 years ago.

Download all attachments as: .zip

Change History (18)

#1 @nacin
5 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
5 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
5 years ago

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

#4 @westi
5 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
5 years ago

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

#6 @automattor
5 years ago

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

#7 @westi
5 years ago

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

#8 @westi
5 years ago

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

#9 @nacin
5 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
5 years ago

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

5 years ago

#11 @nacin
5 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
5 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
5 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
5 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
5 years ago

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

Needs final, close review by ryan.

#16 @nacin
5 years ago

  • Keywords dev-feedback added

#17 @ryan
5 years ago

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

Looks good.

Note: See TracTickets for help on using tickets.