#45346 closed defect (bug) (fixed)
Varnish querystring.sort can break load-styles.php
Reported by: | rabin.io | Owned by: | azaozz |
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | 4.9.8 |
Component: | Script Loader | Keywords: | |
Focuses: | Cc: |
Description
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 ?
Attachments (1)
Change History (13)
#2
@
6 years ago
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
#5
@
5 years ago
- Milestone changed from Awaiting Review to 5.3
We need to maintain the order of loading of the scripts too. 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.
#7
@
5 years ago
In 45346.diff:
- Make the
load
array associative so we can prevent sorting. - Use
ksort()
inload-scripts.php
andload-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 :)
#8
@
5 years ago
I will take some time for me to validate this, as I don't have this setup in my dev environment. so if any one else can test it, that will help.
thx.
#9
@
5 years ago
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.
Related: #22757, #26886.