From 483f13b226d88c6c36248a5988550c641b2a4153 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 27 Jun 2017 19:51:03 -0700 Subject: [PATCH] resourceloader: Use "\n" instead of ";" as separator for scripts This fixes two bugs: * 1) When two modules are requested, and the first one ends with ";" inside a comment, the second module might become part of that comment. * 2) A request with script=only where the requested module content ends in a statement without ";", but has a comment after it that does ends with a semicolon, then in debug=false, mw.loader.state() would be appended directly after the semicolon-less statement because the check is performed before minification. Previously: script> foo() script> // bar(); states> mw.loader.state( {} ); Became (minified separately): script> foo() states> mw.loader.state({}); Became (concatenated) > foo()mw.loader.state(); Which is invalid code. Both of these are now fixed. Bug: T162719 Change-Id: Ic8114c46ce232f5869400eaa40d3027003550533 --- includes/resourceloader/ResourceLoader.php | 23 +++++++++++++++++-- .../resourceloader/ResourceLoaderModule.php | 16 +++++++------ tests/phpunit/includes/OutputPageTest.php | 2 +- .../ResourceLoaderModuleTest.php | 14 +++++++---- .../resourceloader/ResourceLoaderTest.php | 20 ++++++++-------- 5 files changed, 49 insertions(+), 26 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index c11fe5b618..855311667d 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -1100,7 +1100,12 @@ MESSAGE; $strContent = self::filter( $filter, $strContent ); } - $out .= $strContent; + if ( $context->getOnly() === 'scripts' ) { + // Use a linebreak between module scripts (T162719) + $out .= $this->ensureNewline( $strContent ); + } else { + $out .= $strContent; + } } catch ( Exception $e ) { $this->outputErrorAndLog( $e, 'Generating module package failed: {exception}' ); @@ -1128,7 +1133,8 @@ MESSAGE; if ( !$context->getDebug() ) { $stateScript = self::filter( 'minify-js', $stateScript ); } - $out .= $stateScript; + // Use a linebreak between module script and state script (T162719) + $out = $this->ensureNewline( $out ) . $stateScript; } } else { if ( count( $states ) ) { @@ -1140,6 +1146,19 @@ MESSAGE; return $out; } + /** + * Ensure the string is either empty or ends in a line break + * @param string $str + * @return string + */ + private function ensureNewline( $str ) { + $end = substr( $str, -1 ); + if ( $end === false || $end === "\n" ) { + return $str; + } + return $str . "\n"; + } + /** * Get names of modules that use a certain message. * diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 3ad6a84864..743b69b3fe 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -643,16 +643,18 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { $scripts = $this->getScriptURLsForDebug( $context ); } else { $scripts = $this->getScript( $context ); - // rtrim() because there are usually a few line breaks - // after the last ';'. A new line at EOF, a new line - // added by ResourceLoaderFileModule::readScriptFiles, etc. + // Make the script safe to concatenate by making sure there is at least one + // trailing new line at the end of the content. Previously, this looked for + // a semi-colon instead, but that breaks concatenation if the semicolon + // is inside a comment like "// foo();". Instead, simply use a + // line break as separator which matches JavaScript native logic for implicitly + // ending statements even if a semi-colon is missing. + // Bugs: T29054, T162719. if ( is_string( $scripts ) && strlen( $scripts ) - && substr( rtrim( $scripts ), -1 ) !== ';' + && substr( $scripts, -1 ) !== "\n" ) { - // Append semicolon to prevent weird bugs caused by files not - // terminating their statements right (T29054) - $scripts .= ";\n"; + $scripts .= "\n"; } } $content['scripts'] = $scripts; diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php index 32fa46887d..c3ed6dd8a6 100644 --- a/tests/phpunit/includes/OutputPageTest.php +++ b/tests/phpunit/includes/OutputPageTest.php @@ -293,7 +293,7 @@ class OutputPageTest extends MediaWikiTestCase { [ [ 'test.quux', ResourceLoaderModule::TYPE_SCRIPTS ], "" ], ]; diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php index 6057b9710b..c861b37033 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php @@ -98,11 +98,11 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { return [ [ "mw.foo()", - "mw.foo();\n", + "mw.foo()\n", ], [ "mw.foo();", - "mw.foo();", + "mw.foo();\n", ], [ "mw.foo();\n", @@ -110,15 +110,19 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { ], [ "mw.foo()\n", - "mw.foo()\n;\n", + "mw.foo()\n", ], [ "mw.foo()\n// mw.bar();", - "mw.foo()\n// mw.bar();", + "mw.foo()\n// mw.bar();\n", + ], + [ + "mw.foo()\n// mw.bar()", + "mw.foo()\n// mw.bar()\n", ], [ "mw.foo()// mw.bar();", - "mw.foo()// mw.bar();", + "mw.foo()// mw.bar();\n", ], ]; } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php index b7b24731ee..1422adc11b 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php @@ -623,10 +623,10 @@ mw.example(); 'modules' => [ 'foo' => 'foo()', ], - 'expected' => "foo();\n" . 'mw.loader.state( { + 'expected' => "foo()\n" . 'mw.loader.state( { "foo": "ready" } );', - 'minified' => "foo();" . 'mw.loader.state({"foo":"ready"});', + 'minified' => "foo()\n" . 'mw.loader.state({"foo":"ready"});', 'message' => 'Script without semi-colon', ], [ @@ -634,24 +634,22 @@ mw.example(); 'foo' => 'foo()', 'bar' => 'bar()', ], - 'expected' => "foo();\nbar();\n" . 'mw.loader.state( { + 'expected' => "foo()\nbar()\n" . 'mw.loader.state( { "foo": "ready", "bar": "ready" } );', - 'minified' => "foo();bar();" . 'mw.loader.state({"foo":"ready","bar":"ready"});', + 'minified' => "foo()\nbar()\n" . 'mw.loader.state({"foo":"ready","bar":"ready"});', 'message' => 'Two scripts without semi-colon', ], [ 'modules' => [ 'foo' => "foo()\n// bar();" ], - // FIXME: Invalid code (T162719) - 'expected' => "foo()\n// bar();" . 'mw.loader.state( { + 'expected' => "foo()\n// bar();\n" . 'mw.loader.state( { "foo": "ready" } );', - // FIXME: Invalid code (T162719) - 'minified' => "foo()" . 'mw.loader.state({"foo":"ready"});', - 'message' => 'Script with semi-colon in comment', + 'minified' => "foo()\n" . 'mw.loader.state({"foo":"ready"});', + 'message' => 'Script with semi-colon in comment (T162719)', ], ]; $ret = []; @@ -695,7 +693,7 @@ mw.example(); ); $response = $rl->makeModuleResponse( $context, $modules ); - $this->assertCount( 0, $rl->getErrors(), 'Error count' ); + $this->assertSame( [], $rl->getErrors(), 'Errors' ); $this->assertEquals( $expected, $response, $message ?: 'Response' ); } @@ -728,7 +726,7 @@ mw.example(); $this->assertCount( 1, $errors ); $this->assertRegExp( '/Ferry not found/', $errors[0] ); $this->assertEquals( - 'foo();bar();mw.loader.state( { + "foo();\nbar();\n" . 'mw.loader.state( { "ferry": "error", "foo": "ready", "bar": "ready" -- 2.20.1