WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 3 months ago

#24169 new defect (bug)

WP_Customize_Manager loads the current user too early

Reported by: johnjamesjacoby Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.4
Component: Appearance Keywords: has-patch
Focuses: Cc:

Description

When previewing a theme, neither the locale nor the functions.php of either parent/child themes have had the opportunity to load ahead of the current user. This causes theme previews to load without translations in multisite setups where the user chooses their own language, and also means any theme that modifies the current user via actions or filters never has the chance to hook in in time.

The problem is introduced when WP_Customize_Manager prematurely calls is_user_logged_in() and current_user_can() before $wp->init() has fired, on the 'setup_theme' action.

From what I can tell, these specific checks can be moved into a new method, hooked to init, without any consequence. They are only responsible for maybe calling wp_die() where appropriate, meaning any past or subsequent actions or execution are irrelevant anyways.

Patch attached fixes this issue by adding an init() method to the WP_Customize_Manager class, and moves the user checks into this method.

Attachments (1)

24169.patch (2.6 KB) - added by johnjamesjacoby 12 months ago.

Download all attachments as: .zip

Change History (7)

johnjamesjacoby12 months ago

comment:2 follow-up: nacin12 months ago

Could you explain a bit more what the issues are? "means any theme that modifies the current user via actions or filters never has the chance to hook in in time" scares the hell out of me. Translations, I get — I guess that would apply to error messages? #BB2309 seems to get to a larger issue, though.

comment:3 in reply to: ↑ 2 johnjamesjacoby12 months ago

Replying to nacin:

Could you explain a bit more what the issues are?

The issues are two fold, like you've mentioned:

  • Per-user language settings can't load the correct locale.
  • The user is loaded before either themes' functions.php can be loaded, resulting in anything hooked to user actions/filters ('set_current_user' for example) not executing, because the load order is reversed.

#BB2309 is referencing a _doing_it_wrong() added to the current versions of bbPress/BuddyPress; it alerts site admins when 'set_current_user' is fired before 'after_setup_theme'.

When the above happens, it puts sites at risk of critical on-the-fly procedures introduced by plugins being missed (like mapped roles and capabilities) related to #23016, because there's no other reliable time to hook into WP_Roles.

Version 2, edited 12 months ago by johnjamesjacoby (previous) (next) (diff)

comment:4 nacin9 months ago

  • Milestone changed from 3.6 to Future Release

So the problem with doing cap checks later on is that the theme has already been given a chance to load by this point. Even though we eventually die, the very act of including a theme's functions.php when the user is unable to switch_themes can be considered privilege escalation.

Unfortunately we *need* to do cap checks here before we actually load the theme. I'm happy to consider some adjustments for it to work better with the very valid concerns you mentioned. But for the moment, status quo prevails.

comment:5 ericlewis6 months ago

Should a theme's funtions.php file include user-modifying functionality? Use cases appreciated.

If locales should be considered required for user instantiation, we should add a wp_load_translations_early() to the WP_User constructor.

comment:6 nacin3 months ago

  • Component changed from Themes to Appearance
Note: See TracTickets for help on using tickets.