r73076 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r73075‎ | r73076 | r73077 >
Date:18:15, 15 September 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Fixed really bad stuff going on with running hooks at the file level. Now, ResourceLoader::initialize must be called before you can use it durring page rendering. ResourceLoader::respond calls it automatically. ResourceLoader::initialize only has side-effects when run the first time.
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoader.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
@@ -2294,6 +2294,8 @@
22952295 foreach ( (array) $modules as $name ) {
22962296 $moduleGroups[strpos( $name, 'user' ) === 0 ? 'user' : null][] = $name;
22972297 }
 2298+ // Make sure ResourceLoader is ready for use
 2299+ ResourceLoader::initialize();
22982300 $links = '';
22992301 foreach ( $moduleGroups as $group => $modules ) {
23002302 if ( count( $modules ) ) {
Index: trunk/phase3/includes/ResourceLoader.php
@@ -29,6 +29,7 @@
3030
3131 // @var array list of module name/ResourceLoaderModule object pairs
3232 protected static $modules = array();
 33+ protected static $initialized = false;
3334
3435 /* Protected Static Methods */
3536
@@ -84,6 +85,17 @@
8586
8687 /* Static Methods */
8788
 89+ public static function initialize() {
 90+ global $IP;
 91+
 92+ if ( !self::$initialized ) {
 93+ // Do this first just in case someone accidentally adds a call to ResourceLoader::initialize in their hook
 94+ self::$initialized = true;
 95+ self::register( include( "$IP/resources/Resources.php" ) );
 96+ wfRunHooks( 'ResourceLoaderRegisterModules' );
 97+ }
 98+ }
 99+
88100 /**
89101 * Registers a module with the ResourceLoader system.
90102 *
@@ -194,6 +206,9 @@
195207 global $wgResourceLoaderVersionedClientMaxage, $wgResourceLoaderVersionedServerMaxage;
196208 global $wgResourceLoaderUnversionedServerMaxage, $wgResourceLoaderUnversionedClientMaxage;
197209
 210+ // Register modules
 211+ self::initialize();
 212+
198213 // Split requested modules into two groups, modules and missing
199214 $modules = array();
200215 $missing = array();
@@ -323,7 +338,4 @@
324339 }
325340 }
326341 }
327 -}
328 -
329 -ResourceLoader::register( include( "$IP/resources/Resources.php" ) );
330 -wfRunHooks( 'ResourceLoaderRegisterModules' );
\ No newline at end of file
 342+}
\ No newline at end of file

Comments

#Comment by Tim Starling (talk | contribs)   01:57, 17 September 2010

Are you aware that you just fixed a register_globals arbitrary inclusion vulnerability?

#Comment by Trevor Parscal (WMF) (talk | contribs)   16:58, 17 September 2010

I think you still have a better eye for this stuff than I do; I will admit that I wasn't thinking "hey, this will fix this issue" as I did it. However, it was never our plan to keep this vulnerability. Notice that in r72965 the bottom of the file looked like this:

// FIXME: Temp hack
require_once "$IP/resources/Resources.php";

Once I got the hook thing working in r73035, the next logical step was to un-hack it.

I will keep an eye out for this kind of vulnerability in the future. Thanks for the tip.

Status & tagging log