WebRequest::getIP() cleanups.

* Always treat the first XFF IP from cache proxies as sane
  even if it is a private IP (useful for things like labs wmf).
* Make sure IP::canonicalize() gets called if the IP is selected
  from the XFF chain (this matches getRawIP()).
* Altered and expanded unit tests.

bug: 48919
Change-Id: I350aca72c7a96ba3ec727324800612fc84e0e7a4

diff --git a/includes/WebRequest.php b/includes/WebRequest.php
index 20ee5e5..72042a7 100644 (file)
--- a/includes/WebRequest.php
+++ b/includes/WebRequest.php
@@ -1098,19 +1098,21 @@ HTML;
                                array_unshift( $ipchain, $ip );
                        }
 
-                       # Step through XFF list and find the last address in the list which is a trusted server
-                       # Set $ip to the IP address given by that trusted server, unless the address is not sensible (e.g. private)
+                       # Step through XFF list and find the last address in the list which is a
+                       # trusted server. Set $ip to the IP address given by that trusted server,
+                       # unless the address is not sensible (e.g. private). However, prefer private
+                       # IP addresses over proxy servers controlled by this site (more sensible).
                        foreach ( $ipchain as $i => $curIP ) {
                                $curIP = IP::canonicalize( $curIP );
-                               if ( wfIsTrustedProxy( $curIP ) ) {
-                                       if ( isset( $ipchain[$i + 1] ) ) {
-                                               if ( $wgUsePrivateIPs || IP::isPublic( $ipchain[$i + 1] ) ) {
-                                                       $ip = $ipchain[$i + 1];
-                                               }
+                               if ( wfIsTrustedProxy( $curIP ) && isset( $ipchain[$i + 1] ) ) {
+                                       if ( wfIsConfiguredProxy( $curIP ) || // bug 48919
+                                               ( IP::isPublic( $ipchain[$i + 1] ) || $wgUsePrivateIPs )
+                                       ) {
+                                               $ip = IP::canonicalize( $ipchain[$i + 1] );
+                                               continue;
                                        }
-                               } else {
-                                       break;
                                }
+                               break;
                        }
                }
 

diff --git a/tests/phpunit/includes/WebRequestTest.php b/tests/phpunit/includes/WebRequestTest.php
index d382f6f..1c6b733 100644 (file)
--- a/tests/phpunit/includes/WebRequestTest.php
+++ b/tests/phpunit/includes/WebRequestTest.php
@@ -101,11 +101,19 @@ class WebRequestTest extends MediaWikiTestCase {
        /**
         * @dataProvider provideGetIP
         */
-       function testGetIP( $expected, $input, $squid, $private, $description ) {
+       function testGetIP( $expected, $input, $squid, $xffList, $private, $description ) {
                $_SERVER = $input;
                $this->setMwGlobals( array(
                        'wgSquidServersNoPurge' => $squid,
                        'wgUsePrivateIPs' => $private,
+                       'wgHooks' => array(
+                               'IsTrustedProxy' => array(
+                                       function( &$ip, &$trusted ) use ( $xffList ) {
+                                               $trusted = $trusted || in_array( $ip, $xffList );
+                                               return true;
+                                       }
+                               )
+                       )
                ) );
 
                $request = new WebRequest();
@@ -121,6 +129,7 @@ class WebRequestTest extends MediaWikiTestCase {
                                        'REMOTE_ADDR' => '127.0.0.1'
                                ),
                                array(),
+                               array(),
                                false,
                                'Simple IPv4'
                        ),
@@ -130,6 +139,7 @@ class WebRequestTest extends MediaWikiTestCase {
                                        'REMOTE_ADDR' => '::1'
                                ),
                                array(),
+                               array(),
                                false,
                                'Simple IPv6'
                        ),
@@ -140,6 +150,7 @@ class WebRequestTest extends MediaWikiTestCase {
                                        'HTTP_X_FORWARDED_FOR' => '12.0.0.3, 12.0.0.2'
                                ),
                                array( '12.0.0.1', '12.0.0.2' ),
+                               array(),
                                false,
                                'With X-Forwaded-For'
                        ),
@@ -150,6 +161,7 @@ class WebRequestTest extends MediaWikiTestCase {
                                        'HTTP_X_FORWARDED_FOR' => '12.0.0.3, 12.0.0.2'
                                ),
                                array(),
+                               array(),
                                false,
                                'With X-Forwaded-For and disallowed server'
                        ),
@@ -160,29 +172,87 @@ class WebRequestTest extends MediaWikiTestCase {
                                        'HTTP_X_FORWARDED_FOR' => '12.0.0.3, 12.0.0.2'
                                ),
                                array( '12.0.0.1' ),
+                               array(),
                                false,
                                'With multiple X-Forwaded-For and only one allowed server'
                        ),
                        array(
-                               '12.0.0.2',
+                               '10.0.0.3',
                                array(
                                        'REMOTE_ADDR' => '12.0.0.2',
-                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.3, 12.0.0.2'
+                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.4, 10.0.0.3, 12.0.0.2'
                                ),
                                array( '12.0.0.1', '12.0.0.2' ),
+                               array(),
                                false,
-                               'With X-Forwaded-For and private IP'
+                               'With X-Forwaded-For and private IP (from cache proxy)'
                        ),
                        array(
-                               '10.0.0.3',
+                               '10.0.0.4',
                                array(
                                        'REMOTE_ADDR' => '12.0.0.2',
-                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.3, 12.0.0.2'
+                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.4, 10.0.0.3, 12.0.0.2'
+                               ),
+                               array( '12.0.0.1', '12.0.0.2', '10.0.0.3' ),
+                               array(),
+                               true,
+                               'With X-Forwaded-For and private IP (allowed)'
+                       ),
+                       array(
+                               '10.0.0.4',
+                               array(
+                                       'REMOTE_ADDR' => '12.0.0.2',
+                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.4, 10.0.0.3, 12.0.0.2'
                                ),
                                array( '12.0.0.1', '12.0.0.2' ),
+                               array( '10.0.0.3' ),
                                true,
                                'With X-Forwaded-For and private IP (allowed)'
                        ),
+                       array(
+                               '10.0.0.3',
+                               array(
+                                       'REMOTE_ADDR' => '12.0.0.2',
+                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.4, 10.0.0.3, 12.0.0.2'
+                               ),
+                               array( '12.0.0.1', '12.0.0.2' ),
+                               array( '10.0.0.3' ),
+                               false,
+                               'With X-Forwaded-For and private IP (disallowed)'
+                       ),
+                       array(
+                               '12.0.0.3',
+                               array(
+                                       'REMOTE_ADDR' => '12.0.0.1',
+                                       'HTTP_X_FORWARDED_FOR' => '12.0.0.3, 12.0.0.2'
+                               ),
+                               array(),
+                               array( '12.0.0.1', '12.0.0.2' ),
+                               false,
+                               'With X-Forwaded-For'
+                       ),
+                       array(
+                               '12.0.0.2',
+                               array(
+                                       'REMOTE_ADDR' => '12.0.0.1',
+                                       'HTTP_X_FORWARDED_FOR' => '12.0.0.3, 12.0.0.2'
+                               ),
+                               array(),
+                               array( '12.0.0.1' ),
+                               false,
+                               'With multiple X-Forwaded-For and only one allowed server'
+                       ),
+                       array(
+                               '12.0.0.2',
+                               array(
+                                       'REMOTE_ADDR' => '12.0.0.2',
+                                       'HTTP_X_FORWARDED_FOR' => '10.0.0.3, 12.0.0.2'
+                               ),
+                               array(),
+                               array( '12.0.0.2' ),
+                               false,
+                               'With X-Forwaded-For and private IP and hook (disallowed)'
+                       ),
                );
        }