WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 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:

Description

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 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 nacin3 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.

comment:2 nacin3 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.

comment:3 nacin3 years ago

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

comment:4 westi3 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.

comment:5 westi3 years ago

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

comment:6 automattor3 years ago

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

comment:7 westi3 years ago

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

comment:8 westi3 years ago

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

comment:9 nacin3 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.

comment:10 nacin3 years ago

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

nacin3 years ago

comment:11 nacin3 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.

comment:12 westi3 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.

comment:13 nacin3 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.

comment:14 nacin3 years ago

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

comment:15 nacin3 years ago

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

Needs final, close review by ryan.

comment:16 nacin3 years ago

  • Keywords dev-feedback added

comment:17 ryan3 years ago

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

Looks good.

Note: See TracTickets for help on using tickets.