Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#45346 closed defect (bug) (fixed)

Varnish querystring.sort can break load-styles.php

Reported by: rabinio's profile rabin.io Owned by: azaozz's profile 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)

45346.diff (2.2 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Script Loader

Related: #22757, #26886.

#2 @rabin.io
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

#3 @rabin.io
5 years ago

Hi,

Is this issue getting any attention ?

#4 @ocean90
5 years ago

#47334 was marked as a duplicate.

#5 @azaozz
5 years ago

  • Milestone changed from Awaiting Review to 5.3

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.

Version 0, edited 5 years ago by azaozz (next)

#6 @rabin.io
5 years ago

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

@azaozz
5 years ago

#7 @azaozz
5 years ago

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 :)

#8 @rabin.io
5 years ago

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.

Last edited 5 years ago by rabin.io (previous) (diff)

#9 @azaozz
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.

#10 @azaozz
5 years ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

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.

#11 @azaozz
5 years ago

In 45457:

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

See #45346.

#12 @azaozz
5 years ago

In 45458:

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

See #45346.

Note: See TracTickets for help on using tickets.