Fix IP::toHex for IPv4 addresses with a double/triple 0 block

Bug: T97897
Signed-off-by: Chad Horohoe <chadh@wikimedia.org>
Change-Id: I5c0a37be42ae2c5091ead487a6d19f6e0dd89b36

diff --git a/includes/utils/IP.php b/includes/utils/IP.php
index fd7030e..8abca5b 100644 (file)
--- a/includes/utils/IP.php
+++ b/includes/utils/IP.php
@@ -132,8 +132,9 @@ class IP {
 
        /**
         * Convert an IP into a verbose, uppercase, normalized form.
-        * IPv6 addresses in octet notation are expanded to 8 words.
-        * IPv4 addresses are just trimmed.
+        * Both IPv4 and IPv6 addresses are trimmed. Additionally,
+        * IPv6 addresses in octet notation are expanded to 8 words;
+        * IPv4 addresses have leading zeros, in each octet, removed.
         *
         * @param string $ip IP address in quad or octet form (CIDR or not).
         * @return string
@@ -143,8 +144,16 @@ class IP {
                if ( $ip === '' ) {
                        return null;
                }
-               if ( self::isIPv4( $ip ) || !self::isIPv6( $ip ) ) {
-                       return $ip; // nothing else to do for IPv4 addresses or invalid ones
+               /* If not an IP, just return trimmed value, since sanitizeIP() is called
+                * in a number of contexts where usernames are supplied as input.
+                */
+               if ( !self::isIPAddress( $ip ) ) {
+                       return $ip;
+               }
+               if ( self::isIPv4( $ip ) ) {
+                       // Remove leading 0's from octet representation of IPv4 address
+                       $ip = preg_replace( '/(?:^|(?<=\.))0+(?=[1-9]|0\.|0$)/', '', $ip );
+                       return $ip;
                }
                // Remove any whitespaces, convert to upper case
                $ip = strtoupper( $ip );
@@ -399,8 +408,9 @@ class IP {
                if ( self::isIPv6( $ip ) ) {
                        $n = 'v6-' . self::IPv6ToRawHex( $ip );
                } elseif ( self::isIPv4( $ip ) ) {
-                       // Bug 60035: an IP with leading 0's fails in ip2long sometimes (e.g. *.08)
-                       $ip = preg_replace( '/(?<=\.)0+(?=[1-9])/', '', $ip );
+                       // T62035/T97897: An IP with leading 0's fails in ip2long sometimes (e.g. *.08),
+                       // also double/triple 0 needs to be changed to just a single 0 for ip2long.
+                       $ip = self::sanitizeIP( $ip );
                        $n = ip2long( $ip );
                        if ( $n < 0 ) {
                                $n += pow( 2, 32 );

diff --git a/tests/phpunit/includes/utils/IPTest.php b/tests/phpunit/includes/utils/IPTest.php
index 04b8f48..369e38b 100644 (file)
--- a/tests/phpunit/includes/utils/IPTest.php
+++ b/tests/phpunit/includes/utils/IPTest.php
@@ -307,12 +307,34 @@ class IPTest extends PHPUnit_Framework_TestCase {
        }
 
        /**
-        * Improve IP::sanitizeIP() code coverage
-        * @todo Most probably incomplete
+        * @covers IP::sanitizeIP
+        * @dataProvider provideSanitizeIP
         */
-       public function testSanitizeIP() {
-               $this->assertNull( IP::sanitizeIP( '' ) );
-               $this->assertNull( IP::sanitizeIP( ' ' ) );
+       public function testSanitizeIP( $expected, $input ) {
+               $result = IP::sanitizeIP( $input );
+               $this->assertEquals( $expected, $result );
+       }
+
+       /**
+        * Provider for IP::testSanitizeIP()
+        */
+       public static function provideSanitizeIP() {
+               return array(
+                       array( '0.0.0.0', '0.0.0.0' ),
+                       array( '0.0.0.0', '00.00.00.00' ),
+                       array( '0.0.0.0', '000.000.000.000' ),
+                       array( '141.0.11.253', '141.000.011.253' ),
+                       array( '1.2.4.5', '1.2.4.5' ),
+                       array( '1.2.4.5', '01.02.04.05' ),
+                       array( '1.2.4.5', '001.002.004.005' ),
+                       array( '10.0.0.1', '010.0.000.1' ),
+                       array( '80.72.250.4', '080.072.250.04' ),
+                       array( 'Foo.1000.00', 'Foo.1000.00' ),
+                       array( 'Bar.01', 'Bar.01' ),
+                       array( 'Bar.010', 'Bar.010' ),
+                       array( null, '' ),
+                       array( null, ' ' )
+               );
        }
 
        /**
@@ -336,6 +358,7 @@ class IPTest extends PHPUnit_Framework_TestCase {
                        array( '80000000', '128.0.0.0' ),
                        array( 'DEADCAFE', '222.173.202.254' ),
                        array( 'FFFFFFFF', '255.255.255.255' ),
+                       array( '8D000BFD', '141.000.11.253' ),
                        array( false, 'IN.VA.LI.D' ),
                        array( 'v6-00000000000000000000000000000001', '::1' ),
                        array( 'v6-20010DB885A3000000008A2E03707334', '2001:0db8:85a3:0000:0000:8a2e:0370:7334' ),