Fixes for r88883, r89197:

* Modified WebRequest::findIE6Extension() to fix the performance issue and the hash parsing issue I noted on CR
* In FindIE6ExtensionTest, fixed all the assertEquals() calls, I had the expected and actual around the wrong way
* Added a couple of extra tests for cases that seemed important during the rewrite.

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 209b1b4..1e0b98d 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -814,44 +814,52 @@ class WebRequest {
         * insecure.
         *
         * The criteria for finding an extension are as follows:
-        * * a possible extension is a dot followed by one or more characters not in <>\"/:|?.#
-        * * if we find a possible extension followed by the end of the string or a #, that's our extension
-        * * if we find a possible extension followed by a ?, that's our extension
-        * ** UNLESS it's exe, dll or cgi, in which case we ignore it and continue searching for another possible extension
-        * * if we find a possible extension followed by a dot or another illegal character, we ignore it and continue searching
+        * - a possible extension is a dot followed by one or more characters not 
+        *   in <>\"/:|?.#
+        * - if we find a possible extension followed by the end of the string or 
+        *   a #, that's our extension
+        * - if we find a possible extension followed by a ?, that's our extension
+        *    - UNLESS it's exe, dll or cgi, in which case we ignore it and continue 
+        *      searching for another possible extension
+        * - if we find a possible extension followed by a dot or another illegal 
+        *   character, we ignore it and continue searching
         * 
         * @param $url string URL
         * @return mixed Detected extension (string), or false if none found
         */
        public static function findIE6Extension( $url ) {
-               $remaining = $url;
-               while ( $remaining !== '' ) {
+               $pos = 0;
+               $hashPos = strpos( $url, '#' );
+               if ( $hashPos !== false ) {
+                       $urlLength = $hashPos;
+               } else {
+                       $urlLength = strlen( $url );
+               }
+               $remainingLength = $urlLength;
+               while ( $remainingLength > 0 ) {
                        // Skip ahead to the next dot
-                       $pos = strcspn( $remaining, '.' );
-                       if ( $pos === strlen( $remaining ) || $remaining[$pos] === '#' ) {
+                       $pos += strcspn( $url, '.', $pos, $remainingLength );
+                       if ( $pos >= $urlLength ) {
                                // End of string, we're done
                                return false;
                        }
                        
                        // We found a dot. Skip past it
-                       $remaining = substr( $remaining, $pos + 1 );
+                       $pos++;
+                       $remainingLength = $urlLength - $pos;
+
                        // Check for illegal characters in our prospective extension,
-                       // or for another dot, or for a hash sign
-                       $pos = strcspn( $remaining, "<>\\\"/:|?*.#" );
-                       if ( $pos === strlen( $remaining ) ) {
-                               // No illegal character or next dot or hash
+                       // or for another dot
+                       $nextPos = $pos + strcspn( $url, "<>\\\"/:|?*.", $pos, $remainingLength );
+                       if ( $nextPos >= $urlLength ) {
+                               // No illegal character or next dot
                                // We have our extension
-                               return $remaining;
-                       }
-                       if ( $remaining[$pos] === '#' ) {
-                               // We've found a legal extension followed by a hash
-                               // Trim the hash and everything after it, then return that
-                               return substr( $remaining, 0, $pos );
+                               return substr( $url, $pos, $urlLength - $pos );
                        }
-                       if ( $remaining[$pos] === '?' ) {
+                       if ( $url[$nextPos] === '?' ) {
                                // We've found a legal extension followed by a question mark
                                // If the extension is NOT exe, dll or cgi, return it
-                               $extension = substr( $remaining, 0, $pos );
+                               $extension = substr( $url, $pos, $nextPos - $pos );
                                if ( strcasecmp( $extension, 'exe' ) && strcasecmp( $extension, 'dll' ) &&
                                        strcasecmp( $extension, 'cgi' ) )
                                {
@@ -861,7 +869,8 @@ class WebRequest {
                        }
                        // We found an illegal character or another dot
                        // Skip to that character and continue the loop
-                       $remaining = substr( $remaining, $pos );
+                       $pos = $nextPos + 1;
+                       $remainingLength = $urlLength - $pos;
                }
                return false;
        }

diff --git a/tests/phpunit/includes/FindIE6ExtensionTest.php b/tests/phpunit/includes/FindIE6ExtensionTest.php
index d1d5341..534f098 100644 (file)
--- a/tests/phpunit/includes/FindIE6ExtensionTest.php
+++ b/tests/phpunit/includes/FindIE6ExtensionTest.php
@@ -6,97 +6,113 @@
 class FindIE6ExtensionTest extends MediaWikiTestCase {
        function testSimple() {
                $this->assertEquals( 
-                       WebRequest::findIE6Extension( 'x.y' ),
                        'y',
+                       WebRequest::findIE6Extension( 'x.y' ),
                        'Simple extension'
                );
        }
 
        function testSimpleNoExt() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'x' ),
                        '',
+                       WebRequest::findIE6Extension( 'x' ),
                        'No extension'
                );
        }
 
        function testEmpty() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( '' ),
                        '',
+                       WebRequest::findIE6Extension( '' ),
                        'Empty string'
                );
        }
 
        function testQuestionMark() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( '?' ),
                        '',
+                       WebRequest::findIE6Extension( '?' ),
                        'Question mark only'
                );
        }
 
        function testExtQuestionMark() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( '.x?' ),
                        'x',
+                       WebRequest::findIE6Extension( '.x?' ),
                        'Extension then question mark'
                );
        }
 
        function testQuestionMarkExt() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( '?.x' ),
                        'x',
+                       WebRequest::findIE6Extension( '?.x' ),
                        'Question mark then extension'
                );
        }
 
        function testInvalidChar() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( '.x*' ),
                        '',
+                       WebRequest::findIE6Extension( '.x*' ),
                        'Extension with invalid character'
                );
        }
 
+       function testInvalidCharThenExtension() {
+               $this->assertEquals(
+                       'x',
+                       WebRequest::findIE6Extension( '*.x' ),
+                       'Invalid character followed by an extension'
+               );
+       }
+
        function testMultipleQuestionMarks() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'a?b?.c?.d?e?f' ),
                        'c',
+                       WebRequest::findIE6Extension( 'a?b?.c?.d?e?f' ),
                        'Multiple question marks'
                );
        }
 
        function testExeException() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'a?b?.exe?.d?.e' ),
                        'd',
+                       WebRequest::findIE6Extension( 'a?b?.exe?.d?.e' ),
                        '.exe exception'
                );
        }
 
        function testExeException2() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'a?b?.exe' ),
                        'exe',
+                       WebRequest::findIE6Extension( 'a?b?.exe' ),
                        '.exe exception 2'
                );
        }
 
        function testHash() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'a#b.c' ),
                        '',
+                       WebRequest::findIE6Extension( 'a#b.c' ),
                        'Hash character preceding extension'
                );
        }
 
        function testHash2() {
                $this->assertEquals(
-                       WebRequest::findIE6Extension( 'a?#b.c' ),
                        '',
+                       WebRequest::findIE6Extension( 'a?#b.c' ),
                        'Hash character preceding extension 2'
                );
        }
+
+       function testDotAtEnd() {
+               $this->assertEquals(
+                       '',
+                       WebRequest::findIE6Extension( '.' ),
+                       'Dot at end of string'
+               );
+       }
 }