From 8bd6922ab0b1f9a317dc1912d97257112bc90ece Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 30 May 2013 10:30:55 -0700 Subject: [PATCH] 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 --- includes/WebRequest.php | 20 +++--- tests/phpunit/includes/WebRequestTest.php | 82 +++++++++++++++++++++-- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 20ee5e584d..72042a7e49 100644 --- 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 d382f6f57c..1c6b7336d2 100644 --- 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)' + ), ); } -- 2.20.1