WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 weeks ago

#24169 accepted defect (bug)

WP_Customize_Manager loads the current user too early

Reported by: johnjamesjacoby Owned by: westonruter
Milestone: 4.3 Priority: normal
Severity: major Version: 3.4
Component: Customize Keywords: needs-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 (2)

24169.patch (2.6 KB) - added by johnjamesjacoby 2 years ago.
24169.php (262 bytes) - added by johnjamesjacoby 4 weeks ago.
Drop this into /wp-content/mu-plugins/

Download all attachments as: .zip

Change History (16)

@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) 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 2 years ago by johnjamesjacoby (previous) (next) (diff)

comment:4 @nacin23 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 @ericlewis19 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 @nacin16 months ago

  • Component changed from Themes to Appearance

comment:7 @grundyoso11 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 @celloexpressions5 months ago

  • Keywords needs-patch added; has-patch removed

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

comment:9 follow-up: @westonruter4 weeks ago

  • Keywords reporter-feedback added

Please advise if this is still an issue.

comment:10 in reply to: ↑ 9 ; follow-up: @johnjamesjacoby4 weeks ago

Replying to westonruter:

Please advise if this is still an issue.

Yes, it is.

comment:11 in reply to: ↑ 10 @westonruter4 weeks ago

  • Keywords reporter-feedback removed

Replying to johnjamesjacoby:

Replying to westonruter:

Please advise if this is still an issue.

Yes, it is.

And the specific steps to reproduce the issue is to activate BuddyPress and then access the Customizer?

@johnjamesjacoby4 weeks ago

Drop this into /wp-content/mu-plugins/

comment:12 @johnjamesjacoby4 weeks ago

Drop 24169.php into your mu-plugins directory:

  • Visit the front page of your installation (everything should appear normal)
  • Click around your site (everything should appear normal)
  • Login/logout, pretend like you're none-the-wiser
  • Attempt to visit the customizer using your favorite UX
  • Notice the 'too early!' message

comment:13 @westonruter4 weeks ago

  • Milestone changed from Future Release to 4.3

comment:14 @westonruter4 weeks ago

  • Owner set to westonruter
  • Status changed from new to accepted
Note: See TracTickets for help on using tickets.