From 5385f56e96bd76eb6aa2c46b6d4de6a8649aa85d Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 4 Apr 2018 21:07:33 +0100
Subject: [PATCH] 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
---
 includes/libs/CSSMin.php                   | 12 ++++++---
 tests/phpunit/includes/libs/CSSMinTest.php | 30 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index 3d1c8b800d..1cbcbde1e0 100644
--- 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 667eb0ace6..f2d5ef379b 100644
--- 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.
 	 *
-- 
2.20.1