Make WordPress Core

Opened 11 years ago

Closed 7 years ago

Last modified 4 years ago

#24169 closed defect (bug) (wontfix)

WP_Customize_Manager loads the current user too early

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by:
Milestone: Priority: normal
Severity: major Version: 3.4
Component: Customize Keywords: needs-patch reporter-feedback
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 11 years ago.
24169.php (262 bytes) - added by johnjamesjacoby 9 years ago.
Drop this into /wp-content/mu-plugins/

Download all attachments as: .zip

Change History (30)

#2 follow-up: @nacin
11 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.

#3 in reply to: ↑ 2 @johnjamesjacoby
11 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 11 years ago by johnjamesjacoby (previous) (diff)

#4 @nacin
11 years 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.

#5 @ericlewis
10 years 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.

#6 @nacin
10 years ago

  • Component changed from Themes to Appearance

#7 @grundyoso
10 years 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.

#8 @celloexpressions
9 years ago

  • Keywords needs-patch added; has-patch removed

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

#9 follow-up: @westonruter
9 years ago

  • Keywords reporter-feedback added

Please advise if this is still an issue.

#10 in reply to: ↑ 9 ; follow-up: @johnjamesjacoby
9 years ago

Replying to westonruter:

Please advise if this is still an issue.

Yes, it is.

#11 in reply to: ↑ 10 @westonruter
9 years 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?

@johnjamesjacoby
9 years ago

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

#12 @johnjamesjacoby
9 years 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

#13 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.3

#14 @westonruter
9 years ago

  • Owner set to westonruter
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#16 @westonruter
9 years ago

  • Keywords reporter-feedback added

OK, the current user is getting set during the setup_theme action (and the WP_Customize_Manager::setup_theme() method) when this call is done:

                if ( ! current_user_can( 'customize' ) ) {
                        $this->wp_die( -1 );
                }

I don't know what can be done to change this securely from a Customizer perspective. Can the plugin logic be changed to just do it thing later at the init, wp_loaded, parse_request, or wp actions instead of at the set_current_user action?

#17 @ocean90
9 years ago

  • Milestone changed from 4.3 to Future Release

Moving this out of 4.3 until we have a final solution. It's worth to mention that #29783 will probably load the user early too, see ticket:29783:17.

This ticket was mentioned in Slack in #buddypress by basilakis. View the logs.


8 years ago

#19 @westonruter
8 years ago

  • Keywords close added
  • Owner westonruter deleted
  • Status changed from accepted to assigned

#20 @westonruter
8 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

#21 @johnjamesjacoby
8 years ago

Why is this a wontfix?

#22 @westonruter
8 years ago

@johnjamesjacoby Well, I added the close keyword 5 weeks ago and nobody objected. And comment #17 seemed to indicate that loading the user early is something that can't be eliminated.

#23 @johnjamesjacoby
8 years ago

Not knowing the solution doesn't invalidate an issue. I should have noticed the close tag, but ticket modifications should probably be associated with comments, too.

The bug still exists and the code is still defective in its current state. I think marking a valid issue as closed reduces discovery & the likelihood of others contributing to a true resolution.

I'd like to see this at least stay open, and ideally reprioritized.

Or… if we are going to adjust the load order to initialize the current user earlier, that's a good thing as far as this issue is concerned. The crux is it's currently unpredictable and inconsistent. The earlier the better, really.

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

#24 @westonruter
8 years ago

  • Keywords close removed
  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

I thought that that the resolution was that the user had to be loaded early regardless. I'll re-open.

#25 @caramiame
8 years ago

@johnjamesjacoby does this help at all? re: https://github.com/explodybits/hookr-plugin/issues/7

#26 @johnjamesjacoby
8 years ago

I thought that that the resolution was that the user had to be loaded early regardless. I'll re-open.

WordPress shouldn't load the user before plugins & themes have had a chance to get their hooks in.

I wonder if SHORTINIT might work here.

@johnjamesjacoby does this help at all?

Different issue, but somewhat related. Basically, the Hookr plugin is checking if the current user is logging in or out before BuddyPress has had a chance to load and hook itself in.

[The reason this happens is because functions like is_user_logged_in() and current_user_can() *can* be called at anytime, and WordPress will go get the current user data without complaining very much about it. Both BuddyPress & bbPress load most of their code early, but relay off of init and later into set_current_user. If set_current_user happens before init, it puts WordPress (and the authenticity of the current user's roles & caps) in an unpredictable state.]

#27 @johnjamesjacoby
7 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

#26511 (introducing get_user_locale()) now always loads the current user before init on every page load.

That change means this issue is no longer valid (or it was super valid enough to make all of WordPress work differently.)

Closing as wontfix again.

#28 @dlh
4 years ago

#43043 was marked as a duplicate.

Note: See TracTickets for help on using tickets.