Merge "ResourceLoader: Add page title to user script syntax error"

3 files changed, 16 insertions, 13 deletions

diff --git a/includes/ResourceLoader/Module.php b/includes/ResourceLoader/Module.php
index 5efcc6e0d1f7..087a3c2b5f86 100644
--- a/includes/ResourceLoader/Module.php
+++ b/includes/ResourceLoader/Module.php
@@ -993,7 +993,7 @@ abstract class Module implements LoggerAwareInterface {
 	/**
 	 * Validate a user-provided JavaScript blob.
 	 *
-	 * @param string $fileName
+	 * @param string $fileName Page title
 	 * @param string $contents JavaScript code
 	 * @return string JavaScript code, either the original content or a replacement
 	 *  that uses `mw.log.error()` to communicate a syntax error.
@@ -1003,10 +1003,14 @@ abstract class Module implements LoggerAwareInterface {
 			return $contents;
 		}
 		$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
-		// Cache potentially slow parsing of JavaScript code during the
-		// critical path. This happens lazily when responding to requests
-		// for modules=site, modules=user, and Gadgets.
+		// Cache potentially slow parsing of JavaScript code during the critical path.
+		// This happens during load.php requests for modules=site, modules=user, and Gadgets.
 		$error = $cache->getWithSetCallback(
+			// A content hash is included in the cache key so that this is immediately
+			// correct and re-computed after edits without relying on TTL or purges.
+			//
+			// We avoid accidental or abusive conflicts with other pages by including the
+			// wiki (makeKey vs makeGlobalKey) and page, because hashes are not unique.
 			$cache->makeKey(
 				'resourceloader-userjsparse',
 				self::USERJSPARSE_CACHE_VERSION,
@@ -1014,7 +1018,7 @@ abstract class Module implements LoggerAwareInterface {
 				$fileName
 			),
 			$cache::TTL_WEEK,
-			static function () use ( $contents, $fileName ) {
+			static function () use ( $contents ) {
 				try {
 					Peast::ES2016( $contents )->parse();
 				} catch ( PeastSyntaxException $e ) {
@@ -1028,12 +1032,11 @@ abstract class Module implements LoggerAwareInterface {
 		if ( $error ) {
 			// Send the error to the browser console client-side.
 			// By returning this as replacement for the actual script,
-			// we ensure user-provided scripts are safe to include in a batch
-			// request, without risk of a syntax error in this blob breaking
-			// the response itself.
+			// we ensure user-provided scripts are safe to serve to a browser,
+			// without breaking unrelated modules in the same response.
 			return 'mw.log.error(' .
 				json_encode(
-					'Parse error: ' . $error
+					"Parse error: $error in $fileName"
 				) .
 				');';
 		}
diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php
index e12d0b2cc3f2..6f3be0ea83c2 100644
--- a/tests/phpunit/ResourceLoaderTestCase.php
+++ b/tests/phpunit/ResourceLoaderTestCase.php
@@ -155,7 +155,7 @@ class ResourceLoaderTestModule extends Module {
 			// This enables the validation check that replaces invalid
 			// scripts with a warning message.
 			// Based on $wgResourceLoaderValidateJS
-			return $this->validateScriptFile( 'input', $this->script );
+			return $this->validateScriptFile( 'input.js', $this->script );
 		} else {
 			return $this->script;
 		}
diff --git a/tests/phpunit/includes/ResourceLoader/ModuleTest.php b/tests/phpunit/includes/ResourceLoader/ModuleTest.php
index 951e4f916bd0..b9d7bddf2e0a 100644
--- a/tests/phpunit/includes/ResourceLoader/ModuleTest.php
+++ b/tests/phpunit/includes/ResourceLoader/ModuleTest.php
@@ -156,17 +156,17 @@ class ModuleTest extends ResourceLoaderTestCase {
 
 		yield 'valid ES2017 async-await' => [
 			"var foo = async function(x) { return await x.fetch(); }",
-			'Parse error: Unexpected: function on line 1'
+			'Parse error: Unexpected: function on line 1 in input.js'
 		];
 
 		yield 'valid ES2018 spread in object literal' => [
 			"var x = {b: 2, c: 3}; var y = {a: 1, ...x};",
-			'Parse error: Unexpected: ... on line 1'
+			'Parse error: Unexpected: ... on line 1 in input.js'
 		];
 
 		yield 'SyntaxError' => [
 			"var a = 'this is';\n {\ninvalid",
-			'Parse error: Unclosed { on line 3'
+			'Parse error: Unclosed { on line 3 in input.js'
 		];
 
 		// If an implementation matches inputs using a regex with runaway backtracking,