r81524 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r81523‎ | r81524 | r81525 >
Date:16:39, 4 February 2011
Author:happy-melon
Status:ok (Comments)
Tags:
Comment:
Follow-up r64670 (bug22929): cleaner implementation of security for script (and potentially CSS) files. ResourceLoader *already* knows where each module has come from, so all we need to do is filter them in OutputPage according to the desired level of 'trustworthiness'.

TODO:
* Are there instances where we might want to restrict CSS as well as JS?
* Would a $wg config option and/or user preference and/or index.php GET parameter to limit inclusion be useful?
* Can we deprecate any of the existing $wg config options?
* What's going on with the duplicated code between OutputPage and SkinTemplate?
Modified paths:
  • /trunk/extensions/Gadgets/Gadgets_body.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderUserOptionsModule.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -124,9 +124,12 @@
125125
126126 var $mTemplateIds = array();
127127
128 - /** Initialized with a global value. Let us override it.
129 - * Should probably get deleted / rewritten ... */
130 - var $mAllowUserJs;
 128+ # What level of 'untrustworthiness' is allowed in CSS/JS modules loaded on this page?
 129+ # @see ResourceLoaderModule::$origin
 130+ # ResourceLoaderModule::ORIGIN_ALL is assumed unless overridden;
 131+ protected $mAllowedModules = array(
 132+ ResourceLoaderModule::TYPE_COMBINED => ResourceLoaderModule::ORIGIN_ALL,
 133+ );
131134
132135 /**
133136 * @EasterEgg I just love the name for this self documenting variable.
——@@ -212,15 +215,6 @@
213216 );
214217
215218 /**
216 - * Constructor
217 - * Initialise private variables
218 - */
219 - function __construct() {
220 - global $wgAllowUserJs;
221 - $this->mAllowUserJs = $wgAllowUserJs;
222 - }
223 -
224 - /**
225219 * Redirect to $url rather than displaying the normal page
226220 *
227221 * @param $url String: URL
——@@ -367,12 +361,33 @@
368362 }
369363
370364 /**
 365+ * Filter an array of modules to remove insufficiently trustworthy members
 366+ * @param $modules Array
 367+ * @return Array
 368+ */
 369+ protected function filterModules( $modules, $type = ResourceLoaderModule::TYPE_COMBINED ){
 370+ $resourceLoader = $this->getResourceLoader();
 371+ $filteredModules = array();
 372+ foreach( $modules as $val ){
 373+ $module = $resourceLoader->getModule( $val );
 374+ if( $module->getOrigin() <= $this->getAllowedModules( $type ) ) {
 375+ $filteredModules[] = $val;
 376+ }
 377+ }
 378+ return $filteredModules;
 379+ }
 380+
 381+ /**
371382 * Get the list of modules to include on this page
372383 *
 384+ * @param $filter Bool whether to filter out insufficiently trustworthy modules
373385 * @return Array of module names
374386 */
375 - public function getModules() {
376 - return array_values( array_unique( $this->mModules ) );
 387+ public function getModules( $filter = false, $param = 'mModules' ) {
 388+ $modules = array_values( array_unique( $this->$param ) );
 389+ return $filter
 390+ ? $this->filterModules( $modules )
 391+ : $modules;
377392 }
378393
379394 /**
——@@ -390,8 +405,8 @@
391406 * Get the list of module JS to include on this page
392407 * @return array of module names
393408 */
394 - public function getModuleScripts() {
395 - return array_values( array_unique( $this->mModuleScripts ) );
 409+ public function getModuleScripts( $filter = false ) {
 410+ return $this->getModules( $filter, 'mModuleScripts' );
396411 }
397412
398413 /**
——@@ -410,8 +425,8 @@
411426 *
412427 * @return Array of module names
413428 */
414 - public function getModuleStyles() {
415 - return array_values( array_unique( $this->mModuleStyles ) );
 429+ public function getModuleStyles( $filter = false ) {
 430+ return $this->getModules( $filter, 'mModuleStyles' );
416431 }
417432
418433 /**
——@@ -430,8 +445,8 @@
431446 *
432447 * @return Array of module names
433448 */
434 - public function getModuleMessages() {
435 - return array_values( array_unique( $this->mModuleMessages ) );
 449+ public function getModuleMessages( $filter = false ) {
 450+ return $this->getModules( $filter, 'mModuleMessages' );
436451 }
437452
438453 /**
——@@ -1065,22 +1080,61 @@
10661081 }
10671082
10681083 /**
1069 - * Remove user JavaScript from scripts to load
 1084+ * Do not allow scripts which can be modified by wiki users to load on this page;
 1085+ * only allow scripts bundled with, or generated by, the software.
10701086 */
10711087 public function disallowUserJs() {
1072 - $this->mAllowUserJs = false;
 1088+ $this->reduceAllowedModules(
 1089+ ResourceLoaderModule::TYPE_SCRIPTS,
 1090+ ResourceLoaderModule::ORIGIN_CORE_INDIVIDUAL
 1091+ );
10731092 }
10741093
10751094 /**
10761095 * Return whether user JavaScript is allowed for this page
1077 - *
 1096+ * @deprecated @since 1.18 Load modules with ResourceLoader, and origin and
 1097+ * trustworthiness is identified and enforced automagically.
10781098 * @return Boolean
10791099 */
10801100 public function isUserJsAllowed() {
1081 - return $this->mAllowUserJs;
 1101+ return $this->getAllowedModules( ResourceLoaderModule::TYPE_SCRIPTS ) >= ResourceLoaderModule::ORIGIN_USER_INDIVIDUAL;
10821102 }
10831103
10841104 /**
 1105+ * Show what level of JavaScript / CSS untrustworthiness is allowed on this page
 1106+ * @see ResourceLoaderModule::$origin
 1107+ * @param $type String ResourceLoaderModule TYPE_ constant
 1108+ * @return Int ResourceLoaderModule ORIGIN_ class constant
 1109+ */
 1110+ public function getAllowedModules( $type ){
 1111+ if( $type == ResourceLoaderModule::TYPE_COMBINED ){
 1112+ return min( array_values( $this->mAllowedModules ) );
 1113+ } else {
 1114+ return isset( $this->mAllowedModules[$type] )
 1115+ ? $this->mAllowedModules[$type]
 1116+ : ResourceLoaderModule::ORIGIN_ALL;
 1117+ }
 1118+ }
 1119+
 1120+ /**
 1121+ * Set the highest level of CSS/JS untrustworthiness allowed
 1122+ * @param $type String ResourceLoaderModule TYPE_ constant
 1123+ * @param $level Int ResourceLoaderModule class constant
 1124+ */
 1125+ public function setAllowedModules( $type, $level ){
 1126+ $this->mAllowedModules[$type] = $level;
 1127+ }
 1128+
 1129+ /**
 1130+ * As for setAllowedModules(), but don't inadvertantly make the page more accessible
 1131+ * @param $type String
 1132+ * @param $level Int ResourceLoaderModule class constant
 1133+ */
 1134+ public function reduceAllowedModules( $type, $level ){
 1135+ $this->mAllowedModules[$type] = min( $this->getAllowedModules($type), $level );
 1136+ }
 1137+
 1138+ /**
10851139 * Prepend $text to the body HTML
10861140 *
10871141 * @param $text String: HTML
——@@ -2347,7 +2401,7 @@
23482402 * TODO: Document
23492403 * @param $skin Skin
23502404 * @param $modules Array/string with the module name
2351 - * @param $only string May be styles, messages or scripts
 2405+ * @param $only String ResourceLoaderModule TYPE_ class constant
23522406 * @param $useESI boolean
23532407 * @return string html <script> and <style> tags
23542408 */
——@@ -2396,12 +2450,23 @@
23972451 $resourceLoader = $this->getResourceLoader();
23982452 foreach ( (array) $modules as $name ) {
23992453 $module = $resourceLoader->getModule( $name );
 2454+ # Check that we're allowed to include this module on this page
 2455+ if( ( $module->getOrigin() > $this->getAllowedModules( ResourceLoaderModule::TYPE_SCRIPTS )
 2456+ && $only == ResourceLoaderModule::TYPE_SCRIPTS )
 2457+ || ( $module->getOrigin() > $this->getAllowedModules( ResourceLoaderModule::TYPE_STYLES )
 2458+ && $only == ResourceLoaderModule::TYPE_STYLES )
 2459+ )
 2460+ {
 2461+ continue;
 2462+ }
 2463+
24002464 $group = $module->getGroup();
24012465 if ( !isset( $groups[$group] ) ) {
24022466 $groups[$group] = array();
24032467 }
24042468 $groups[$group][$name] = $module;
24052469 }
 2470+
24062471 $links = '';
24072472 foreach ( $groups as $group => $modules ) {
24082473 $query['modules'] = implode( '|', array_keys( $modules ) );
——@@ -2412,7 +2477,7 @@
24132478 // Support inlining of private modules if configured as such
24142479 if ( $group === 'private' && $wgResourceLoaderInlinePrivateModules ) {
24152480 $context = new ResourceLoaderContext( $resourceLoader, new FauxRequest( $query ) );
2416 - if ( $only == 'styles' ) {
 2481+ if ( $only == ResourceLoaderModule::TYPE_STYLES ) {
24172482 $links .= Html::inlineStyle(
24182483 $resourceLoader->makeModuleResponse( $context, $modules )
24192484 );
——@@ -2446,14 +2511,14 @@
24472512 $url = wfAppendQuery( $wgLoadScript, $query );
24482513 if ( $useESI && $wgResourceLoaderUseESI ) {
24492514 $esi = Xml::element( 'esi:include', array( 'src' => $url ) );
2450 - if ( $only == 'styles' ) {
 2515+ if ( $only == ResourceLoaderModule::TYPE_STYLES ) {
24512516 $links .= Html::inlineStyle( $esi );
24522517 } else {
24532518 $links .= Html::inlineScript( $esi );
24542519 }
24552520 } else {
24562521 // Automatically select style/script elements
2457 - if ( $only === 'styles' ) {
 2522+ if ( $only === ResourceLoaderModule::TYPE_STYLES ) {
24582523 $links .= Html::linkedStyle( wfAppendQuery( $wgLoadScript, $query ) ) . "\n";
24592524 } else {
24602525 $links .= Html::linkedScript( wfAppendQuery( $wgLoadScript, $query ) ) . "\n";
——@@ -2472,24 +2537,24 @@
24732538 * @return String: HTML fragment
24742539 */
24752540 function getHeadScripts( Skin $sk ) {
2476 - global $wgUser, $wgRequest, $wgUseSiteJs;
 2541+ global $wgUser, $wgRequest, $wgUseSiteJs, $wgAllowUserJs;
24772542
24782543 // Startup - this will immediately load jquery and mediawiki modules
2479 - $scripts = $this->makeResourceLoaderLink( $sk, 'startup', 'scripts', true );
 2544+ $scripts = $this->makeResourceLoaderLink( $sk, 'startup', ResourceLoaderModule::TYPE_SCRIPTS, true );
24802545
24812546 // Configuration -- This could be merged together with the load and go, but
24822547 // makeGlobalVariablesScript returns a whole script tag -- grumble grumble...
24832548 $scripts .= Skin::makeGlobalVariablesScript( $sk->getSkinName() ) . "\n";
24842549
24852550 // Script and Messages "only" requests
2486 - $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts(), 'scripts' );
2487 - $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleMessages(), 'messages' );
 2551+ $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleScripts( true ), ResourceLoaderModule::TYPE_SCRIPTS );
 2552+ $scripts .= $this->makeResourceLoaderLink( $sk, $this->getModuleMessages( true ), ResourceLoaderModule::TYPE_MESSAGES );
24882553
24892554 // Modules requests - let the client calculate dependencies and batch requests as it likes
2490 - if ( $this->getModules() ) {
 2555+ if ( $this->getModules( true ) ) {
24912556 $scripts .= Html::inlineScript(
24922557 ResourceLoader::makeLoaderConditionalScript(
2493 - Xml::encodeJsCall( 'mediaWiki.loader.load', array( $this->getModules() ) ) .
 2558+ Xml::encodeJsCall( 'mediaWiki.loader.load', array( $this->getModules( true ) ) ) .
24942559 Xml::encodeJsCall( 'mediaWiki.loader.go', array() )
24952560 )
24962561 ) . "\n";
——@@ -2500,25 +2565,25 @@
25012566
25022567 // Add site JS if enabled
25032568 if ( $wgUseSiteJs ) {
2504 - $scripts .= $this->makeResourceLoaderLink( $sk, 'site', 'scripts' );
 2569+ $scripts .= $this->makeResourceLoaderLink( $sk, 'site', ResourceLoaderModule::TYPE_SCRIPTS );
25052570 }
25062571
25072572 // Add user JS if enabled - trying to load user.options as a bundle if possible
25082573 $userOptionsAdded = false;
2509 - if ( $this->isUserJsAllowed() && $wgUser->isLoggedIn() ) {
 2574+ if ( $wgAllowUserJs && $wgUser->isLoggedIn() ) {
25102575 $action = $wgRequest->getVal( 'action', 'view' );
25112576 if( $this->mTitle && $this->mTitle->isJsSubpage() && $sk->userCanPreview( $action ) ) {
25122577 # XXX: additional security check/prompt?
25132578 $scripts .= Html::inlineScript( "\n" . $wgRequest->getText( 'wpTextbox1' ) . "\n" ) . "\n";
25142579 } else {
25152580 $scripts .= $this->makeResourceLoaderLink(
2516 - $sk, array( 'user', 'user.options' ), 'scripts'
 2581+ $sk, array( 'user', 'user.options' ), ResourceLoaderModule::TYPE_SCRIPTS
25172582 );
25182583 $userOptionsAdded = true;
25192584 }
25202585 }
25212586 if ( !$userOptionsAdded ) {
2522 - $scripts .= $this->makeResourceLoaderLink( $sk, 'user.options', 'scripts' );
 2587+ $scripts .= $this->makeResourceLoaderLink( $sk, 'user.options', ResourceLoaderModule::TYPE_SCRIPTS );
25232588 }
25242589
25252590 return $scripts;
——@@ -2713,7 +2778,7 @@
27142779 // dynamically added styles to override statically added styles from other modules. So the order
27152780 // has to be other, dynamic, site, user
27162781 // Add statically added styles for other modules
2717 - $ret .= $this->makeResourceLoaderLink( $sk, $styles['other'], 'styles' );
 2782+ $ret .= $this->makeResourceLoaderLink( $sk, $styles['other'], ResourceLoaderModule::TYPE_STYLES );
27182783 // Add normal styles added through addStyle()/addInlineStyle() here
27192784 $ret .= implode( "\n", $this->buildCssLinksArray() ) . $this->mInlineStyles;
27202785 // Add marker tag to mark the place where the client-side loader should inject dynamic styles
——@@ -2721,7 +2786,7 @@
27222787 $ret .= Html::element( 'meta', array( 'name' => 'ResourceLoaderDynamicStyles', 'content' => '' ) );
27232788 // Add site and user styles
27242789 $ret .= $this->makeResourceLoaderLink(
2725 - $sk, array_merge( $styles['site'], $styles['user'] ), 'styles'
 2790+ $sk, array_merge( $styles['site'], $styles['user'] ), ResourceLoaderModule::TYPE_STYLES
27262791 );
27272792 return $ret;
27282793 }
Index: trunk/phase3/includes/resourceloader/ResourceLoaderWikiModule.php
——@@ -32,6 +32,9 @@
3333 abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
3434
3535 /* Protected Members */
 36+
 37+ # Origin is user-supplied code
 38+ protected $origin = self::ORIGIN_USER_SITEWIDE;
3639
3740 // In-object cache for modified time
3841 protected $modifiedTime = array();
Index: trunk/phase3/includes/resourceloader/ResourceLoaderUserOptionsModule.php
——@@ -29,6 +29,8 @@
3030
3131 protected $modifiedTime = array();
3232
 33+ protected $origin = self::ORIGIN_CORE_INDIVIDUAL;
 34+
3335 /* Methods */
3436
3537 public function getModifiedTime( ResourceLoaderContext $context ) {
Index: trunk/phase3/includes/resourceloader/ResourceLoaderUserModule.php
——@@ -26,6 +26,7 @@
2727 class ResourceLoaderUserModule extends ResourceLoaderWikiModule {
2828
2929 /* Protected Methods */
 30+ protected $origin = self::ORIGIN_USER_INDIVIDUAL;
3031
3132 protected function getPages( ResourceLoaderContext $context ) {
3233 if ( $context->getUser() ) {
Index: trunk/phase3/includes/resourceloader/ResourceLoaderModule.php
——@@ -24,6 +24,34 @@
2525 * Abstraction for resource loader modules, with name registration and maxage functionality.
2626 */
2727 abstract class ResourceLoaderModule {
 28+
 29+ # Type of resource
 30+ const TYPE_SCRIPTS = 'scripts';
 31+ const TYPE_STYLES = 'styles';
 32+ const TYPE_MESSAGES = 'messages';
 33+ const TYPE_COMBINED = 'combined';
 34+
 35+ # sitewide core module like a skin file or jQuery component
 36+ const ORIGIN_CORE_SITEWIDE = 1;
 37+
 38+ # per-user module generated by the software
 39+ const ORIGIN_CORE_INDIVIDUAL = 2;
 40+
 41+ # sitewide module generated from user-editable files, like MediaWiki:Common.js, or
 42+ # modules accessible to multiple users, such as those generated by the Gadgets extension.
 43+ const ORIGIN_USER_SITEWIDE = 3;
 44+
 45+ # per-user module generated from user-editable files, like User:Me/Monobook.js
 46+ const ORIGIN_USER_INDIVIDUAL = 4;
 47+
 48+ # an access constant; make sure this is kept as the largest number in this group
 49+ const ORIGIN_ALL = 10;
 50+
 51+ # script and style modules form a hierarchy of trustworthiness, with core modules like
 52+ # skins and jQuery as most trustworthy, and user scripts as least trustworthy. We can
 53+ # limit the types of scripts and styles we allow to load on, say, sensitive special
 54+ # pages like Special:UserLogin and Special:Preferences
 55+ protected $origin = self::ORIGIN_CORE_SITEWIDE;
2856
2957 /* Protected Members */
3058
——@@ -57,6 +85,27 @@
5886 }
5987
6088 /**
 89+ * Get this module's origin. This is set when the module is registered
 90+ * with ResourceLoader::register()
 91+ *
 92+ * @return Int ResourceLoaderModule class constant, the subclass default
 93+ * if not set manuall
 94+ */
 95+ public function getOrigin() {
 96+ return $this->origin;
 97+ }
 98+
 99+ /**
 100+ * Set this module's origin. This is called by ResourceLodaer::register()
 101+ * when registering the module. Other code should not call this.
 102+ *
 103+ * @param $name Int origin
 104+ */
 105+ public function setOrigin( $origin ) {
 106+ $this->origin = $origin;
 107+ }
 108+
 109+ /**
61110 * Get whether CSS for this module should be flipped
62111 * @param $context ResourceLoaderContext
63112 */
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -201,6 +201,8 @@
202202 $tpl->setRef( 'usercss', $this->usercss );
203203
204204 $this->userjs = $this->userjsprev = false;
 205+ # FIXME: this is the only use of OutputPage::isUserJsAllowed() anywhere; can we
 206+ # get rid of it? For that matter, why is any of this here at all?
205207 $this->setupUserJs( $out->isUserJsAllowed() );
206208 $tpl->setRef( 'userjs', $this->userjs );
207209 $tpl->setRef( 'userjsprev', $this->userjsprev );
——@@ -1255,6 +1257,7 @@
12561258
12571259 /**
12581260 * @private
 1261+ * FIXME: why is this duplicated in/from OutputPage::getHeadScripts()??
12591262 */
12601263 function setupUserJs( $allowUserJs ) {
12611264 global $wgRequest, $wgJsMimeType;
Index: trunk/extensions/Gadgets/Gadgets_body.php
——@@ -106,19 +106,6 @@
107107 if ( !$wgUser->isLoggedIn() ) return true;
108108
109109 wfProfileIn( __METHOD__ );
110 - //disable all gadgets on critical special pages
111 - //NOTE: $out->isUserJsAllowed() is tempting, but always fals if $wgAllowUserJs is false.
112 - // That would disable gadgets on wikis without user JS. Introducing $out->isJsAllowed()
113 - // may work, but should that really apply also to MediaWiki:common.js? Even on the preference page?
114 - // See bug 22929 for discussion.
115 - $title = $out->getTitle();
116 - if ( $title->isSpecial( 'Preferences' )
117 - || $title->isSpecial( 'Resetpass' )
118 - || $title->isSpecial( 'Userlogin' ) )
119 - {
120 - wfProfileOut( __METHOD__ );
121 - return true;
122 - }
123110
124111 $gadgets = Gadget::loadList();
125112 if ( !$gadgets ) {
——@@ -164,6 +151,14 @@
165152 private static function applyScript( $page, $out ) {
166153 global $wgJsMimeType;
167154
 155+ # bug 22929: disable gadgets on sensitive pages. Scripts loaded through the
 156+ # ResourceLoader handle this in OutputPage::getModules()
 157+ # TODO: make this extension load everything via RL, then we don't need to worry
 158+ # about any of this.
 159+ if( $out->getAllowedModules( ResourceLoaderModule::TYPE_SCRIPTS ) < ResourceLoaderModule::ORIGIN_USER_SITEWIDE ){
 160+ return;
 161+ }
 162+
168163 $t = Title::makeTitleSafe( NS_MEDIAWIKI, $page );
169164 if ( !$t ) return;
170165

Follow-up revisions

RevisionCommit summaryAuthorDate
r81525Release notes for r81524happy-melon16:41, 4 February 2011
r81526user scripts & styles are lowercase skin; + using 'vector' as example instead.krinkle16:44, 4 February 2011
r84351Follow-up r81524: fix fatal when modules are deregisteredhappy-melon21:19, 19 March 2011
r85399Follow-up r85354: back out part of r81524 which somehow got caught up in the ...happy-melon23:53, 4 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r64670fix for bug 22929: disable gadgets on Special:Userlogindaniel21:00, 6 April 2010

Comments

#Comment by Krinkle (talk | contribs)   16:45, 4 February 2011

Follow-up: r81526.

#Comment by Nikerabbit (talk | contribs)   21:14, 19 March 2011

[19-Mar-2011 21:13:09] PHP Fatal error: Call to a member function getOrigin() on a non-object in /www/w/includes/OutputPage.php on line 373

When cached version of a page refers to module which no longer exists.

#Comment by Happy-melon (talk | contribs)   21:20, 19 March 2011

Should be fixed in r84351.

Status & tagging log