From: Timo Tijhof <krinklemail@gmail.com>
Date: Sat, 24 Aug 2019 18:45:27 +0000 (+0100)
Subject: resourceloader: Add tests for disallowing access to private modules
X-Git-Tag: 1.34.0-rc.0~600^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=e1bf44cd21441b212aab8672397e821d7aa5e106;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Add tests for disallowing access to private modules

* Add a test to confirm that the ResourceLoader::respond() logic
  works as intended.

* Remove the client code for preventing it from being loaded.
  This can never happen in production unless there is a bug.
  Instead of optimising to avoid a pointless request that only
  happens when the software is broken, instead optimise for when
  the software is not broken by just letting it happen. The server
  already handles it just fine.

  This was originally added in 2015 with 1dd73903726 to reduce
  logspam, but that was instead fixed in 6d6b037e122 by making the
  log message debug-only (because it's not a software problem,
  it's a client-error, e.g. a broken user script or a third
  party trying out different things on the load.php entry point).

  Removing this makes the client a bit smaller, too :)

Change-Id: Ic5420d9329a73514f4fc27baa46ae58d94addafb
---

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 9892b15705..0785225d2c 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -661,8 +661,9 @@ class ResourceLoader implements LoggerAwareInterface {
 				// Do not allow private modules to be loaded from the web.
 				// This is a security issue, see T36907.
 				if ( $module->getGroup() === 'private' ) {
+					// Not a serious error, just means something is trying to access it (T101806)
 					$this->logger->debug( "Request for private module '$name' denied" );
-					$this->errors[] = "Cannot show private module \"$name\"";
+					$this->errors[] = "Cannot build private module \"$name\"";
 					continue;
 				}
 				$modules[$name] = $module;
diff --git a/resources/src/startup/mediawiki.js b/resources/src/startup/mediawiki.js
index ad05c6f6c0..ab7f3a074e 100644
--- a/resources/src/startup/mediawiki.js
+++ b/resources/src/startup/mediawiki.js
@@ -1211,15 +1211,9 @@
 
 				dependencies.forEach( function ( module ) {
 					// Only queue modules that are still in the initial 'registered' state
-					// (not ones already loading, ready or error).
+					// (e.g. not ones already loading or loaded etc.).
 					if ( registry[ module ].state === 'registered' && queue.indexOf( module ) === -1 ) {
-						// Private modules must be embedded in the page. Don't bother queuing
-						// these as the server will deny them anyway (T101806).
-						if ( registry[ module ].group === 'private' ) {
-							setAndPropagate( module, 'error' );
-						} else {
-							queue.push( module );
-						}
+						queue.push( module );
 					}
 				} );
 
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
index 86c2e9f59b..ac4a1ca1bb 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
@@ -1095,6 +1095,32 @@ END
 		$rl->respond( $context );
 	}
 
+	/**
+	 * Refuse requests for private modules.
+	 *
+	 * @covers ResourceLoader::respond
+	 */
+	public function testRespondErrorPrivate() {
+		$rl = $this->getMockBuilder( EmptyResourceLoader::class )
+			->setMethods( [
+				'measureResponseTime',
+				'tryRespondNotModified',
+				'sendResponseHeaders',
+			] )
+			->getMock();
+		$rl->register( [
+			'foo' => [ 'class' => ResourceLoaderTestModule::class ],
+			'bar' => [ 'class' => ResourceLoaderTestModule::class, 'group' => 'private' ],
+		] );
+		$context = $this->getResourceLoaderContext(
+			[ 'modules' => 'foo|bar', 'only' => null ],
+			$rl
+		);
+
+		$this->expectOutputRegex( '/^\/\*.+Cannot build private module/s' );
+		$rl->respond( $context );
+	}
+
 	/**
 	 * @covers ResourceLoader::respond
 	 */