WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 6 months ago

Last modified 6 months ago

#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:
PR Number:

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 6 months ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
12 months ago

  • Component changed from General to Script Loader

Related: #22757, #26886.

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

Hi,

Is this issue getting any attention ?

#4 @ocean90
6 months ago

#47334 was marked as a duplicate.

#5 @azaozz
6 months 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.

Last edited 6 months ago by azaozz (previous) (diff)

#6 @rabin.io
6 months ago

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

@azaozz
6 months ago

#7 @azaozz
6 months 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
6 months 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 6 months ago by rabin.io (previous) (diff)

#9 @azaozz
6 months 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
6 months 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
6 months ago

In 45457:

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

See #45346.

#12 @azaozz
6 months 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.