Page MenuHomePhabricator

Using Skin objects in PreferencesGetLayout is unsafe when the hook runs in the resource loader
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):
From @Krinkle's review of our change that added the hook runner to the loader https://gerrit.wikimedia.org/r/c/mediawiki/core/+/851178/8#message-180f0802cf4776f04f6cc1551ba8d44e0f51b9e2

Add the following three lines to your LocalSettings.php file:

 $wgHooks['PreferencesGetLayout'][] = function ( &$useMobileLayout, $skin ) {
	$skin->isResponsive();
 };

What happens?:

fatal error.

BadMethodCallException: Sessions are disabled for load entry point
Backtrace:
#3 /w/includes/user/User.php(438): User->loadFromSession()
…
#10 /w/includes/skins/Skin.php(300): UserOptionsLookup->getBoolOption()
#12 /w/LocalSettings.php(29): SkinVector->isResponsive()
…
#16 /w/resources/Resources.php(2302): HookRunner->onPreferencesGetLayout()
…
#30 /w/load.php(39): wfLoadMain()

What should have happened instead?:
mediawiki running without going kablooey

Solutions (notes here focus on loader code):
Our callback in the loader currently does this:

$skinName = $context->getSkin();
$skinFactory = MediaWikiServices::getInstance()->getSkinFactory();
$skin = $skinFactory->makeSkin( $skinName );
Hooks::runner()->onPreferencesGetLayout( $useMobileLayout, $skin );

@Krinkle's suggestion:

I suggest changing the (new) PreferencesGetLayout hook to pass a string instead. It hasn't made it into a stable release yet so this should be easy to do still and ensures callers can never call into unsafe methods in the future which would be a train blocker waiting to happen as it may work in testing under certain conditions.

Mod tools initial idea:
We could pass an array containing the skin name (required) and then optionally Skin->isResponsive() and Skin->getOptions() as available. @Scardenasmolinar and I talked about this and were thinking of something like the following in the loader:

Hooks::runner()->onPreferencesGetLayout(
	$useMobileLayout, 
	[
	'name' => $context->getSkin(),
  	'responsive' => null,
  	'options' => null,
	]
);

or

Hooks::runner()->onPreferencesGetLayout(
	$useMobileLayout, 
	[ 'name' => $context->getSkin() ]
);

Having looked through the existing art, I also see that plenty of hooks have optional parameters that can have null passed in, eg.
@param string|null
I think the best practice would be for us to take this flat approach and simply pass in null values when the hook gets run in the loader. eg.

Hooks::runner()->onPreferencesGetLayout(
	$useMobileLayout,
	$context->getSkin(),
	null,
	null
);

Event Timeline

Change 857084 had a related patch set uploaded (by Scardenasmolinar; author: Scardenasmolinar):

[mediawiki/core@master] HookRunner - Change PreferencesGetLayoutHook params

https://gerrit.wikimedia.org/r/857084

Change 857085 had a related patch set uploaded (by Scardenasmolinar; author: Scardenasmolinar):

[mediawiki/skins/MinervaNeue@master] HookRunner - Change PreferencesGetLayoutHook params

https://gerrit.wikimedia.org/r/857085

Test wiki created on Patch demo by JSherman (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/052cd8359d/w

Change 857085 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] HookRunner - Change PreferencesGetLayoutHook params

https://gerrit.wikimedia.org/r/857085

@Scardenasmolinar this isn't exactly waiting for changes, but the core patch is waiting for comment.

I have also updated the MediaWiki documentation to reflect the changes made by these patches.