From fb234d9a5afdbdfa9497bccfc337d850d2448498 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 6 Mar 2018 16:23:06 -0800 Subject: [PATCH 1/1] resourceloader: Fix mw.loader to compute combined version in packed order The 'version' param was being computed based on the order of the modules list before we perform string compression. And this compression can change the order for its optimisation purposes. Specifically, when requesting modules like 'a', 'b.1', 'b.2' and 'c'. The version was computed based on a + b.1 + b.2 + c (standard sort order), whereas the optimised list is 'a,c|b.1,2', which expands to a + c + b.1 + b.2, which makes hash validation fail. Bug: T188076 Change-Id: I00d6985c054fecd88acf73041aa02878e83d62bc --- resources/src/mediawiki/mediawiki.js | 31 +++++++++++++---- tests/qunit/data/load.mock.php | 11 ++++++ .../mediawiki/mediawiki.loader.test.js | 34 +++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 7d8c3bc8e8..3fe276bbef 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -1523,17 +1523,28 @@ * * @private * @param {Object} moduleMap Module map - * @return {string} Module query string + * @return {Object} + * @return {string} return.str Module query string + * @return {Array} return.list List of module names in matching order */ function buildModulesString( moduleMap ) { var p, prefix, - arr = []; + str = [], + list = []; + + function restore( suffix ) { + return p + suffix; + } for ( prefix in moduleMap ) { p = prefix === '' ? '' : prefix + '.'; - arr.push( p + moduleMap[ prefix ].join( ',' ) ); + str.push( p + moduleMap[ prefix ].join( ',' ) ); + list.push.apply( list, moduleMap[ prefix ].map( restore ) ); } - return arr.join( '|' ); + return { + str: str.join( '|' ), + list: list + }; } /** @@ -1586,9 +1597,15 @@ */ function doRequest() { // Optimisation: Inherit (Object.create), not copy ($.extend) - var query = Object.create( currReqBase ); - query.modules = buildModulesString( moduleMap ); - query.version = getCombinedVersion( currReqModules ); + var query = Object.create( currReqBase ), + packed = buildModulesString( moduleMap ); + query.modules = packed.str; + // The packing logic can change the effective order, even if the input was + // sorted. As such, the call to getCombinedVersion() must use this + // effective order, instead of currReqModules, as otherwise the combined + // version will not match the hash expected by the server based on + // combining versions from the module query string in-order. (T188076) + query.version = getCombinedVersion( packed.list ); query = sortQuery( query ); addScript( sourceLoadScript + '?' + $.param( query ) ); } diff --git a/tests/qunit/data/load.mock.php b/tests/qunit/data/load.mock.php index 43ee0c7d07..2300949860 100644 --- a/tests/qunit/data/load.mock.php +++ b/tests/qunit/data/load.mock.php @@ -59,6 +59,15 @@ mw.loader.implement( 'testUrlInc.a', function () {} ); ", 'testUrlInc.b' => " mw.loader.implement( 'testUrlInc.b', function () {} ); +", + 'testUrlOrder' => " +mw.loader.implement( 'testUrlOrder', function () {} ); +", + 'testUrlOrder.a' => " +mw.loader.implement( 'testUrlOrder.a', function () {} ); +", + 'testUrlOrder.b' => " +mw.loader.implement( 'testUrlOrder.b', function () {} ); ", ]; @@ -70,6 +79,8 @@ $response = ''; if ( isset( $_GET['modules'] ) ) { if ( $_GET['modules'] === 'testUrlInc,testUrlIncDump|testUrlInc.a,b' ) { $modules = [ 'testUrlInc', 'testUrlIncDump', 'testUrlInc.a', 'testUrlInc.b' ]; + } elseif ( $_GET['modules'] === 'testUrlOrder,testUrlOrderDump|testUrlOrder.a,b' ) { + $modules = [ 'testUrlOrder', 'testUrlOrderDump', 'testUrlOrder.a', 'testUrlOrder.b' ]; } else { $modules = explode( ',', $_GET['modules'] ); } diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js index 6c988ef9f4..0b05ac1244 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.loader.test.js @@ -583,6 +583,40 @@ } ); } ); + // @covers mw.loader#batchRequest + // @covers mw.loader#buildModulesString + QUnit.test( 'Url composition (order of modules for version) – T188076', function ( assert ) { + mw.loader.register( [ + // [module, version, dependencies, group, source] + [ 'testUrlOrder', 'url', [], null, 'testloader' ], + [ 'testUrlOrder.a', '1', [], null, 'testloader' ], + [ 'testUrlOrder.b', '2', [], null, 'testloader' ], + [ 'testUrlOrderDump', 'dump', [], null, 'testloader' ] + ] ); + + return mw.loader.using( [ + 'testUrlOrderDump', + 'testUrlOrder.b', + 'testUrlOrder.a', + 'testUrlOrder' + ] ).then( function ( require ) { + assert.propEqual( + require( 'testUrlOrderDump' ).query, + { + modules: 'testUrlOrder,testUrlOrderDump|testUrlOrder.a,b', + // Expected: Combined in order after string packing + // $hash = hash( 'fnv132', 'urldump12' ); + // base_convert( $hash, 16, 36 ); // "1knqzan" + // Previously: Combined in order of before string packing + // $hash = hash( 'fnv132', 'url12dump' ); + // base_convert( $hash, 16, 36 ); // "11eo3in" + version: '1knqzan' + }, + 'Query parameters' + ); + } ); + } ); + QUnit.test( 'Broken indirect dependency', function ( assert ) { // don't emit an error event this.sandbox.stub( mw, 'track' ); -- 2.20.1