Page MenuHomePhabricator

Parser code for special page transclusion replaces main context
Open, Needs TriagePublicBUG REPORT

Description

In Parser::braceSubstitution(), the block that handles transcluded special pages does this:

// Create a new context to execute the special page
$context = new RequestContext;
$context->setTitle( $title );
$context->setRequest( new FauxRequest( $pageArgs ) );
if ( $specialPage && $specialPage->maxIncludeCacheTime() === 0 ) {
	$context->setUser( $this->userFactory->newFromUserIdentity( $this->getUserIdentity() ) );
} else {
	// If this page is cached, then we better not be per user.
	$context->setUser( User::newFromName( '127.0.0.1', false ) );
}
$context->setLanguage( $this->mOptions->getUserLangObj() );
$ret = $this->specialPageFactory->capturePath( $title, $context, $this->getLinkRenderer() );

and then SpecialPage::capturePath() does this:

$main = RequestContext::getMain();
$ctx = [
	'title' => $main->getTitle(),
	'output' => $main->getOutput(),
	'request' => $main->getRequest(),
	'user' => $main->getUser(),
	'language' => $main->getLanguage(),
];

// Override
$main->setTitle( $title );
$main->setOutput( $context->getOutput() );
$main->setRequest( $context->getRequest() );
$main->setUser( $context->getUser() );
$main->setLanguage( $context->getLanguage() );

// The useful part
$ret = $this->executePath( $page, $context, true, $linkRenderer );

// Restore old globals and context
$main->setTitle( $ctx['title'] );
$main->setOutput( $ctx['output'] );
$main->setRequest( $ctx['request'] );
$main->setUser( $ctx['user'] );
$main->setLanguage( $ctx['language'] );

This was a hacky but reasonably safe thing to do in the past. But in a service container based world, where services can are created on the fly and stored for future invocations, and dependencies get passed in, all these things might get cached in various places. The one I ran into was MinervaHooks::onSpecialPageBeforeExecute() fetching the Vector.FeatureManager service, which is initialized using the request context; it ends up storing the FauxRequest created by the parser and so you get a weird failure of the useskinversion URL parameter on some pages.

That specific bug is very fringe and is not worth much attention, but the trick employed by the parser could be the source of other bugs and it might be worth thinking about how to move away from this pattern.

Related Objects

Event Timeline

Tgr added a subscriber: daniel.

@daniel I couldn't find the right tags but maybe this is interesting to you.

fetching the Vector.FeatureManager service, which is initialized using the request context;

This is why service wiring should not depend on the request or user settings.

@daniel I couldn't find the right tags but maybe this is interesting to you.

Yes, thanks!

@tstarling This seems relevant in the context of the discussion we had the other day about service wiring depending on the request.

and then SpecialPage::capturePath() does this:

In an ideal world, SpecialPageFactory::capturePath() should not need to mess with global state. It should just construct a fake RequestContext and call setContext() on the SpecialPage before executing it. That should(tm) work. And the Real Fix(c) is to make it work...

This is why service wiring should not depend on the request or user settings.

Ideally, but not having any dependency on user preferences (often used as feature flags to disable big chunks of code to reduce various risks associated with new features) or a message localizer seems very far from our current reality.

I can see three way to handle this:

  • Services do not depend the context; instead the context gets passed around basically everywhere as a function parameter.
  • Ability to set a faux service container when you use a faux context.
  • Providing the request context as a service, so other services can depend on that and always see the current value of the context.

The first is not realistic, at least in the short term; and even in the long term, I think it could prove pretty inconvenient. The third doesn't help when you'd want to initialize your service with something more granular, like the value of a user preference, or a query parameter. Having some ScopedCallback-style pattern which sets you up with a new context and a new service container seems the cleanest, but would probably have severe performance implications, and some services might expect to be called back in some teardown phase (e.g. a deferred queue) so not doing that might result to loss of consistency.

So none of those seem like great options...

ServiceWiring.php
* Note that, ideally, all information used to instantiate service objects should come
* from configuration. Information derived from the current request is acceptable, but
* only where there is no feasible alternative. It is preferred that such information
* (like the client IP, the acting user's identity, requested title, etc) be passed to
* the service object's methods as parameters. This makes the flow of information more
* obvious, and makes it easier to understand the behavior of services.

For example, to follow this pattern the SpecialPageFactory has a getPage function, but every caller has to call setContext on it to make sure for which context it is used/working. That also includes that the constructor cannot use the context, which is also sometimes violated (T297203). The service is not responsible for the context.

A similiar discussion happen on T249668: 'LinkRenderer' service violates to not inspect request/session

Change 958540 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] SpecialPageFactory: Deprecate accessing global context during transclusion

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

This pattern just caused the same issue again, and I'd love to get rid of it. We should find out if we even need it – do any special pages access the global context, rather than the provided one? This patch emits a deprecation warning when they do.

We should find out if we even need it – do any special pages access the global context, rather than the provided one?

I've been testing mostly with Special:RecentChanges, and… yes – it turns out there's a lot of low-level code in Linker, Message, and other places, where the current context isn't passed, and which ends up reaching into the global context. :(

Change 958540 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] SpecialPageFactory: Deprecate accessing global context during transclusion

Reason:

This requires a lot more work than I have time for right now.

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

Change 1006602 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] SpecialPageFactory::capturePath: Stop saving and restoring global state

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

We should find out if we even need it – do any special pages access the global context, rather than the provided one?

I've been testing mostly with Special:RecentChanges, and… yes – it turns out there's a lot of low-level code in Linker, Message, and other places, where the current context isn't passed, and which ends up reaching into the global context. :(

I think we can lower the bar a bit. Reaching global context doesn't need to be completely prevented, it just needs to be less harmful and broken than saving and restoring global state.

If I understand correctly, after this change, if an includeable special page uses the global context for something instead of the passed context, then some parts of the rendering of the special page content will be done using the current user's context instead of the logged-out user context, and this rendering is cached and can be shown to other users later. Before this change, we have a different problem – some parts of the rendering other than the special page content were done using the logged-out user context instead of the current user's context.

This seems like a more risky situation. If we just render something using the wrong skin (or e.g. wrong language), like the cases we've seen so far, it's annoying but no big deal in the grand scheme of things. But the context is also used for privilege checks, so a bug of that kind could result in a data leak with this change, while without it it could only cause some missing data in the interface.

On the other hand, I haven't seen any bug reports of that kind, so this could be okay. Still, I'd like to be reassured that you've thought about it and you think it's okay (or that I'm wrong about this problem being possible).

Review of production includable special pages for main context dependence:

for w in $(expanddblist all); do
   echo '$spf = MediaWiki\MediaWikiServices::getInstance()->getSpecialPageFactory(); foreach ( $spf->getNames() as $name ) if ( $spf->getPage($name)->isIncludable() ) print "$name\n";' | mwscript eval.php --wiki=$w >> includable
done
sort includable | uniq
  • Allpages
    • OK
  • ApiHelp
    • OK
  • AppManagement
    • Likely non-functional either way; isIncludable should be removed
  • Contribute
    • OK
  • Contributions
    • Needs further review
    • Could use some more including() checks. For example, what's it doing calling $this->getSkin()->setRelevantUser()?
  • GlobalUsers
    • OK
  • Impact
    • There shouldn't really be any OOUI in an includable special page. It depends on the theme singleton which is global state. You're going to see the theme of the user who rendered the page.
  • IndexPages
    • OK
  • LanguageStats
    • Probably OK, although it would be good to suppress showPurgeForm, RebuildMessageGroupStatsJob
  • Listfiles
    • OK
  • Listusers
    • OK
  • ManageMentors
    • It's trying, but would be nice if handleAction() didn't throw PermissionsError
  • MessageGroupStats
    • Same comments as LanguageStats
  • Newimages
    • OK
  • NewMessages
    • I have some concerns about this LQT page. It should probably be made not includable.
  • Newpages
    • OK
  • PageAssessments
    • OK
  • PendingChanges
    • feed() is exposed by accident and needs to be suppressed. FeedUtils::formatDiffRow2() is careful to not leak private data even though it doesn't have the right state. But it is still using the wrong user for date formatting.
  • Prefixindex
    • T336504 is caused by an OOUI form being generated and thrown away during parse. Apparently it is only done to support a Translate extension hook handler that wants to know whether a checkbox is checked. Core checkboxes are done using WebRequest::getBool().
  • Recentchanges
    • checkLastModified() is fully broken either way.
    • action, peek parameters should be suppressed.
    • OOUI is conditional on including(), good.
    • ChangesList has context, and the hooks it calls are given sufficient context.
    • Public static methods with optional context like ChangesList::userCan() are a bit scary, could be deprecated. But apparently no actual private data leak.
    • PagerTools has context. HistoryTools hook is given a User.
    • Hooks FetchChangesList, ChangesListInitRows, ChangesListInsertArticleLink are given sufficient context to avoid leaking private data.
    • Hook handlers in Wikibase, FlaggedRevs, Thanks seem fine
    • Flow does various weird and scary things. Flow's modifyChangesListLine is problematic, it uses the FlowUser service which is the global context user. The current system does not fix this since the user is stored in a place that SpecialPageFactory doesn't know about.
    • MinervaNeue's FetchChangesList hook handler makes a whole new ChangesList, ignoring the passed context object and instead using Skin::getContext(). This is probably broken either way.
  • Recentchangeslinked
    • OK (no additional concerns compared with Recentchanges)
  • SearchWiki
    • This is used on Incubator to put a form on a page, although all instances seem to be commented out, so maybe there was a problem with it. Seems inadvisable, better to use a parser tag hook like what InputBox does. Uses OOUI.
  • Translations
    • Similar problems to Prefixindex
  • TranslationStats
    • OK
  • ValidationStatistics
    • It should be filtering hidden users out of getTopReviewers(). Broken either way.
  • Wantedpages
    • OK
  • Whatlinkshere
    • OK
    • Flow's WhatLinksHereProps seems OK

This seems like a more risky situation. If we just render something using the wrong skin (or e.g. wrong language), like the cases we've seen so far, it's annoying but no big deal in the grand scheme of things. But the context is also used for privilege checks, so a bug of that kind could result in a data leak with this change, while without it it could only cause some missing data in the interface.

It's fair to be concerned about this. Per the review above, most includable special pages do not have any privilege checks. On public wikis, metadata about current pages is generally provided to anonymous users. On private wikis, it's up to the editors to avoid putting private data on whitelist-read pages.

The exceptions are the ChangesList and Contributions pages which do need careful review.

Change #1006602 abandoned by Tim Starling:

[mediawiki/core@master] SpecialPageFactory::capturePath: Stop saving and restoring global state

Reason:

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