CSSMin: Change behavior for missing files

We would previously return the path to the local file on the
filesystem, which is useless in all cases and possibly a security
issue in some. Now we return the URL at which the file would be
accessible had it existed.

Also reordered the code around that part to make the control flow
clearer and added a test.

Change-Id: I1d5befb2ea385ae4d316c5d8c5d1fc092b64c4ff

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index a2c63a1..fd5bca4 100644 (file)
--- a/includes/libs/CSSMin.php
+++ b/includes/libs/CSSMin.php
@@ -241,33 +241,28 @@ class CSSMin {
                        }
                }
 
-               // We drop the query part here and instead make the path relative to $remote
-               $url = "{$remote}/{$file}";
-               // Path to the actual file on the filesystem
-               $localFile = "{$local}/{$file}";
-
-               if ( $local !== false && file_exists( $localFile ) ) {
-
-                       // Add version parameter as a time-stamp in ISO 8601 format,
-                       // using Z for the timezone, meaning GMT
-                       $url .= '?' . gmdate( 'Y-m-d\TH:i:s\Z', round( filemtime( $localFile ), -2 ) );
-                       if ( $embed ) {
-                               $data = self::encodeImageAsDataURI( $localFile );
-                               if ( $data !== false ) {
-                                       return $data;
-                               } else {
-                                       return $url;
-                               }
-                       } else {
-                               // Assume that all paths are relative to $remote, and make them absolute
-                               return $url;
-                       }
-               } elseif ( $local === false ) {
+               if ( $local === false ) {
                        // Assume that all paths are relative to $remote, and make them absolute
-                       return $url . $query;
+                       return $remote . '/' . $url;
                } else {
-                       // $local is truthy, but the file is missing
-                       return $localFile;
+                       // We drop the query part here and instead make the path relative to $remote
+                       $url = "{$remote}/{$file}";
+                       // Path to the actual file on the filesystem
+                       $localFile = "{$local}/{$file}";
+                       if ( file_exists( $localFile ) ) {
+                               // Add version parameter as a time-stamp in ISO 8601 format,
+                               // using Z for the timezone, meaning GMT
+                               $url .= '?' . gmdate( 'Y-m-d\TH:i:s\Z', round( filemtime( $localFile ), -2 ) );
+                               if ( $embed ) {
+                                       $data = self::encodeImageAsDataURI( $localFile );
+                                       if ( $data !== false ) {
+                                               return $data;
+                                       }
+                               }
+                       }
+                       // If any of these conditions failed (file missing, we don't want to embed it
+                       // or it's not embeddable), return the URL (possibly with ?timestamp part)
+                       return $url;
                }
        }
 

diff --git a/tests/phpunit/includes/libs/CSSMinTest.php b/tests/phpunit/includes/libs/CSSMinTest.php
index 239cdf6..94ebe60 100644 (file)
--- a/tests/phpunit/includes/libs/CSSMinTest.php
+++ b/tests/phpunit/includes/libs/CSSMinTest.php
@@ -148,6 +148,11 @@ class CSSMinTest extends MediaWikiTestCase {
                                'foo { background: url(red.gif); }',
                                'foo { background: url(http://localhost/w/red.gif?timestamp); }',
                        ),
+                       array(
+                               'Regular file (missing)',
+                               'foo { background: url(theColorOfHerHair.gif); }',
+                               'foo { background: url(http://localhost/w/theColorOfHerHair.gif); }',
+                       ),
                        array(
                                'Remote URL',
                                'foo { background: url(http://example.org/w/foo.png); }',