Varnish querystring.sort can break load-styles.php

[rabin.io]

If one use Varnish with this snippet

 # Sort the querystring parameters, so different orders of the same produce a single cache object.
  if (req.url ~ "\?") {
    set req.url = querystring.sort(req.url);
  }

And if load-styles.php get an array for the load parameter and this parameter is sorted, it can break the page by not loading all the style elements.

e.g:
this url

/load-styles.php?c=0&dir=rtl&load[]=dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets&load[]=,site-icon,l10n,buttons&ver=4.9.8

the array will look like this after sorting

    [load] => Array
        (
            [0] => dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets
            [1] => ,site-icon,l10n,buttons
        )

and the result will be,

Array
(
    [0] => 
    [1] => site-icon
    [2] => l10n
    [3] => buttonsdashicons <-------- this is the problem
    [4] => admin-bar
    [5] => common
    [6] => forms
    [7] => admin-menu
    [8] => dashboard
    [9] => list-tables
    [10] => edit
    [11] => revisions
    [12] => media
    [13] => themes
    [14] => about
    [15] => nav-menus
    [16] => wp-pointer
    [17] => widgets
)

the fix is very simple,

diff --git a/wp-admin/load-styles.php b/wp-admin/load-styles.php
index de20881..f45cfe2 100644
--- a/wp-admin/load-styles.php
+++ b/wp-admin/load-styles.php
@@ -20,7 +20,8 @@ require( ABSPATH . WPINC . '/version.php' );
 
 $load = $_GET['load'];
 if ( is_array( $load ) ) {
-       $load = implode( '', $load );
+       $load = array_map( function ($item) { return trim($item, ','); }, $load );
+       $load = implode( ',', $load );
 }
 $load = preg_replace( '/[^a-z0-9,_-]+/i', '', $load );
 $load = array_unique( explode( ',', $load ) );

but is it the right solution ?

[SergeyBiryukov]

Related: #22757, #26886.

[rabin.io]

Just a thought, right now WP pass the parameters with empty index

load[]=XXXXX&load[]=YYYY

what if when you split the string you assigned/pass then with index's
this way the sorting won't affect the order of the load array

load[1]=XXXXX&load[2]=YYY

[rabin.io]

Hi,

Is this issue getting any attention ?

[ocean90]

#47334 was marked as a duplicate.

[azaozz]

How about making this an associative array. That should prevent the sorting:

/load-styles.php?c=0&dir=rtl&load[chunk_0]=dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets&load[chunk_1]=,site-icon,l10n,buttons&ver=4.9.8

Which will produce:

[load] => Array (
      [chunk_0] => dashicons,admin-bar,common,forms,admin-menu,dashboard,list-tables,edit,revisions,media,themes,about,nav-menus,wp-pointer,widgets
      [chunk_1] => ,site-icon,l10n,buttons
)

Can even do a ksort() in load-scripts.php for a good measure.

Patch coming up.

[rabin.io]

yaiiiii... after 6 month of waiting, some attention :)

[azaozz]

In 45346.diff:

• Make the load array associative so we can prevent sorting.
• Use ksort() in load-scripts.php and load-styles.php.

@rabinio please test.

after 6 month of waiting...

Thinking it would have helped if there was a "testable" patch, especially with some unit tests. Of course you're welcome to add unit tests now if this patch works well :)

[rabin.io]

It will take some time for me to validate this, as I don't have this setup in my dev environment right now, so if any one else can test it, that will help.

thx.

[azaozz]

Note: as discussed in https://core.trac.wordpress.org/ticket/26886#comment:39 the sorting function here set req.url = querystring.sort(req.url); should not sort (return early) if the query string contains an array, i.e. if more than one param name has [ and ] in it.

[azaozz]

In 45456:

Script loader: prevent sorting of the load array in the query string when passing the script handles to load-scripts.php and load-styles.php.

Fixes #45346 #26886.

[azaozz]

In 45457:

Fix previous WPCS errors in script-loader.php triggered by [45456].

See #45346.

[azaozz]

In 45458:

Fix unit tests after [45456]. The query string is a bit different but the results are the same.

See #45346.