CSSMin: Don't match empty string as remappable url

The empty string being matched causes an undefined array index
notice in production, seen from various random gadgets, but spiked
after a change in MonoBook from last week that introduced a
broken background-image rule with empty string as url.

In browsers, that is actually interpreted as valid and "expands"
to the current url and re-fetches as Accept:image/*, silly, but
still broken. The broken icon was fixed in MonoBook, but we still
need to avoid trying to remap empty string as url.

Two changes:

1. Fix regex used by remap() to not match empty string.
   This was already fixed for the 'url()' case without the
   optional quotes, but with quotes, it was being matched as
   non-empty. This is now fixed by using '+' instead of '*'.
   Added tests to confirm they produce output, and PHPUnit
   is configured to also assert no Notices are emitted (which
   it converts to fatal exceptions).

2. Fix processUrlMatch() as sanity check to throw if the key
   is missing.

Bug: T191237
Change-Id: I0ada337b0b4ab73c80236367ff79c31bcd13aa7d

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index 3d1c8b8..1cbcbde 100644 (file)
--- a/includes/libs/CSSMin.php
+++ b/includes/libs/CSSMin.php
@@ -424,11 +424,11 @@ class CSSMin {
                        //   is only supported in PHP 5.6. Use a getter method for now.
                        $urlRegex = '(' .
                                // Unquoted url
-                               'url\(\s*(?P<file0>[^\'"][^\?\)]*?)(?P<query0>\?[^\)]*?|)\s*\)' .
+                               'url\(\s*(?P<file0>[^\'"][^\?\)]+?)(?P<query0>\?[^\)]*?|)\s*\)' .
                                // Single quoted url
-                               '|url\(\s*\'(?P<file1>[^\?\']*?)(?P<query1>\?[^\']*?|)\'\s*\)' .
+                               '|url\(\s*\'(?P<file1>[^\?\']+?)(?P<query1>\?[^\']*?|)\'\s*\)' .
                                // Double quoted url
-                               '|url\(\s*"(?P<file2>[^\?"]*?)(?P<query2>\?[^"]*?|)"\s*\)' .
+                               '|url\(\s*"(?P<file2>[^\?"]+?)(?P<query2>\?[^"]*?|)"\s*\)' .
                                ')';
                }
                return $urlRegex;
@@ -446,6 +446,9 @@ class CSSMin {
                                $match['file'] = $match['file1'];
                                $match['query'] = $match['query1'];
                        } else {
+                               if ( !isset( $match['file2'] ) || $match['file2'][1] === -1 ) {
+                                       throw new Exception( 'URL must be non-empty' );
+                               }
                                $match['file'] = $match['file2'];
                                $match['query'] = $match['query2'];
                        }
@@ -457,6 +460,9 @@ class CSSMin {
                                $match['file'] = $match['file1'];
                                $match['query'] = $match['query1'];
                        } else {
+                               if ( !isset( $match['file2'] ) || $match['file2'] === '' ) {
+                                       throw new Exception( 'URL must be non-empty' );
+                               }
                                $match['file'] = $match['file2'];
                                $match['query'] = $match['query2'];
                        }

diff --git a/tests/phpunit/includes/libs/CSSMinTest.php b/tests/phpunit/includes/libs/CSSMinTest.php
index 667eb0a..f2d5ef3 100644 (file)
--- a/tests/phpunit/includes/libs/CSSMinTest.php
+++ b/tests/phpunit/includes/libs/CSSMinTest.php
@@ -253,6 +253,36 @@ class CSSMinTest extends MediaWikiTestCase {
                ];
        }
 
+       /**
+        * Cases with empty url() for CSSMin::remap.
+        *
+        * Regression test for T191237.
+   *
+        * @dataProvider provideRemapEmptyUrl
+        * @covers CSSMin
+        */
+       public function testRemapEmptyUrl( $params, $expected ) {
+               $remapped = call_user_func_array( 'CSSMin::remap', $params );
+               $this->assertEquals( $expected, $remapped, 'Ignore empty url' );
+       }
+
+       public static function provideRemapEmptyUrl() {
+               return [
+                       'Empty' => [
+                               [ "background-image: url();", false, '/example', false ],
+                               "background-image: url();",
+                       ],
+                       'Single quote' => [
+                               [ "background-image: url('');", false, '/example', false ],
+                               "background-image: url('');",
+                       ],
+                       'Double quote' => [
+                               [ 'background-image: url("");', false, '/example', false ],
+                               'background-image: url("");',
+                       ],
+               ];
+       }
+
        /**
         * This tests the basic functionality of CSSMin::remap.
         *