Make WordPress Core

Opened 2 years 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: Customize Keywords: needs-patch
Focuses: Cc:


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

Download all attachments as: .zip

Change History (9)

@johnjamesjacoby2 years ago

comment:2 follow-up: @nacin2 years 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 @johnjamesjacoby2 years 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) from those functions.php files not executing, because the load order is reversed and the hooks are added after the action has already fired.

#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' has fired.

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.

Last edited 2 years ago by johnjamesjacoby (previous) (diff)

comment:4 @nacin21 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 @ericlewis17 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 @nacin14 months ago

  • Component changed from Themes to Appearance

comment:7 @grundyoso9 months ago

I've been trying to resolve an issue that I traced down to being related to this one. It exhibited itself in debug_log like this:

Notice: bbp_setup_current_user was called incorrectly. The current user is being initialized without using $wp->init(). Please see Debugging in WordPress for more information. (This message was added in version 2.3.)

Through some stack tracing and variable duping, I tracked the issue down to the WooDojo plugin. This plugin in particular loads bundles of custom widgets and they don't seem to protect against the infamous "after_setup_theme" state before trying to access the current user. So it exhibits itself as a BBPress plugin issue when in fact WooDojo is making the mistake of making sure the theme is setup before trying to mess with the current user. At any rate, it was a simple fix in line 122 of the ./plugins/woodojo/classes/base.class.php file from this:

add_action( 'plugins_loaded', array( &$this, 'init_component_loaders' ) );

to this:

add_action( 'after_setup_theme', array( &$this, 'init_component_loaders' ) );

I'm somewhat of a nooB, so take this with a grain of salt... just hope it helps save someone some time.

comment:8 @celloexpressions3 months ago

  • Keywords needs-patch added; has-patch removed

Per nacin, this needs a new approach if it's still an issue.

Note: See TracTickets for help on using tickets.