From 39ee83f3885fc4ba0239ce085f11e8e20ffe97d3 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 21 Sep 2016 19:52:06 -0700 Subject: [PATCH] Move IP::isConfigured/TrustedProxy() to ProxyLookup service This creates a new ProxyLookup service to house the IP::isConfiguredProxy() and IP::isTrustedProxy() functions. The main purpose of this refactoring is to make the IP class entirely independent from MediaWiki, so it can be split into a separate library. Change-Id: I60434a5f3d99880352bc0f72349c33b7d029ae09 --- RELEASE-NOTES-1.28 | 3 + autoload.php | 1 + includes/Block.php | 6 +- includes/MediaWikiServices.php | 9 ++ includes/ProxyLookup.php | 85 +++++++++++++++++++ includes/ServiceWiring.php | 8 ++ includes/WebRequest.php | 8 +- includes/utils/IP.php | 47 ---------- .../includes/MediaWikiServicesTest.php | 3 +- tests/phpunit/includes/WebRequestTest.php | 6 +- 10 files changed, 121 insertions(+), 55 deletions(-) create mode 100644 includes/ProxyLookup.php diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28 index fd6c6137ee..a24f97a1f6 100644 --- a/RELEASE-NOTES-1.28 +++ b/RELEASE-NOTES-1.28 @@ -202,6 +202,9 @@ changes to languages because of Phabricator reports. Instead of --keep-uploads, use the same option to parserTests.php, but you must specify a directory with --upload-dir. * The 'jquery.arrowSteps' ResourceLoader module is now deprecated. +* IP::isConfiguredProxy() and IP::isTrustedProxy() were removed. Callers should + migrate to using the same functions on a ProxyLookup instance, obtainable from + MediaWikiServices. == Compatibility == diff --git a/autoload.php b/autoload.php index f5c334b37a..aa0f561a1b 100644 --- a/autoload.php +++ b/autoload.php @@ -1100,6 +1100,7 @@ $wgAutoloadLocalClasses = [ 'ProtectedPagesPager' => __DIR__ . '/includes/specials/SpecialProtectedpages.php', 'ProtectedTitlesPager' => __DIR__ . '/includes/specials/pagers/ProtectedTitlesPager.php', 'ProtectionForm' => __DIR__ . '/includes/ProtectionForm.php', + 'ProxyLookup' => __DIR__ . '/includes/ProxyLookup.php', 'PruneFileCache' => __DIR__ . '/maintenance/pruneFileCache.php', 'PublishStashedFileJob' => __DIR__ . '/includes/jobqueue/jobs/PublishStashedFileJob.php', 'PurgeAction' => __DIR__ . '/includes/actions/PurgeAction.php', diff --git a/includes/Block.php b/includes/Block.php index 19ba0a28fb..098d51c808 100644 --- a/includes/Block.php +++ b/includes/Block.php @@ -19,6 +19,9 @@ * * @file */ + +use MediaWiki\MediaWikiServices; + class Block { /** @var string */ public $mReason; @@ -1120,6 +1123,7 @@ class Block { } $conds = []; + $proxyLookup = MediaWikiServices::getInstance()->getProxyLookup(); foreach ( array_unique( $ipChain ) as $ipaddr ) { # Discard invalid IP addresses. Since XFF can be spoofed and we do not # necessarily trust the header given to us, make sure that we are only @@ -1130,7 +1134,7 @@ class Block { continue; } # Don't check trusted IPs (includes local squids which will be in every request) - if ( IP::isTrustedProxy( $ipaddr ) ) { + if ( $proxyLookup->isTrustedProxy( $ipaddr ) ) { continue; } # Check both the original IP (to check against single blocks), as well as build diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index f621867a50..b16044e12d 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -19,6 +19,7 @@ use MediaWiki\Services\ServiceContainer; use MediaWiki\Services\NoSuchServiceException; use MWException; use ObjectCache; +use ProxyLookup; use SearchEngine; use SearchEngineConfig; use SearchEngineFactory; @@ -529,6 +530,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'MediaHandlerFactory' ); } + /** + * @since 1.28 + * @return ProxyLookup + */ + public function getProxyLookup() { + return $this->getService( 'ProxyLookup' ); + } + /** * @since 1.28 * @return GenderCache diff --git a/includes/ProxyLookup.php b/includes/ProxyLookup.php new file mode 100644 index 0000000000..3a3243a5d6 --- /dev/null +++ b/includes/ProxyLookup.php @@ -0,0 +1,85 @@ +proxyServers = $proxyServers; + $this->proxyServersComplex = $proxyServersComplex; + } + + /** + * Checks if an IP matches a proxy we've configured + * + * @param string $ip + * @return bool + */ + public function isConfiguredProxy( $ip ) { + // Quick check of known singular proxy servers + if ( in_array( $ip, $this->proxyServers ) ) { + return true; + } + + // Check against addresses and CIDR nets in the complex list + if ( !$this->proxyIPSet ) { + $this->proxyIPSet = new IPSet( $this->proxyServersComplex ); + } + return $this->proxyIPSet->match( $ip ); + } + + /** + * Checks if an IP is a trusted proxy provider. + * Useful to tell if X-Forwarded-For data is possibly bogus. + * CDN cache servers for the site are whitelisted. + * + * @param string $ip + * @return bool + */ + public function isTrustedProxy( $ip ) { + $trusted = $this->isConfiguredProxy( $ip ); + Hooks::run( 'IsTrustedProxy', [ &$ip, &$trusted ] ); + return $trusted; + } +} diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 8c7d802d68..604491192a 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -164,6 +164,14 @@ return [ ); }, + 'ProxyLookup' => function( MediaWikiServices $services ) { + $mainConfig = $services->getMainConfig(); + return new ProxyLookup( + $mainConfig->get( 'SquidServers' ), + $mainConfig->get( 'SquidServersNoPurge' ) + ); + }, + 'LinkCache' => function( MediaWikiServices $services ) { return new LinkCache( $services->getTitleFormatter(), diff --git a/includes/WebRequest.php b/includes/WebRequest.php index a5ae4612c6..0065135ae0 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -23,6 +23,7 @@ * @file */ +use MediaWiki\MediaWikiServices; use MediaWiki\Session\Session; use MediaWiki\Session\SessionId; use MediaWiki\Session\SessionManager; @@ -1222,7 +1223,8 @@ HTML; # Append XFF $forwardedFor = $this->getHeader( 'X-Forwarded-For' ); if ( $forwardedFor !== false ) { - $isConfigured = IP::isConfiguredProxy( $ip ); + $proxyLookup = MediaWikiServices::getInstance()->getProxyLookup(); + $isConfigured = $proxyLookup->isConfiguredProxy( $ip ); $ipchain = array_map( 'trim', explode( ',', $forwardedFor ) ); $ipchain = array_reverse( $ipchain ); array_unshift( $ipchain, $ip ); @@ -1235,14 +1237,14 @@ HTML; foreach ( $ipchain as $i => $curIP ) { $curIP = IP::sanitizeIP( IP::canonicalize( $curIP ) ); if ( !$curIP || !isset( $ipchain[$i + 1] ) || $ipchain[$i + 1] === 'unknown' - || !IP::isTrustedProxy( $curIP ) + || !$proxyLookup->isTrustedProxy( $curIP ) ) { break; // IP is not valid/trusted or does not point to anything } if ( IP::isPublic( $ipchain[$i + 1] ) || $wgUsePrivateIPs || - IP::isConfiguredProxy( $curIP ) // bug 48919; treat IP as sane + $proxyLookup->isConfiguredProxy( $curIP ) // bug 48919; treat IP as sane ) { // Follow the next IP according to the proxy $nextIP = IP::canonicalize( $ipchain[$i + 1] ); diff --git a/includes/utils/IP.php b/includes/utils/IP.php index 8676baf49c..21203a47ce 100644 --- a/includes/utils/IP.php +++ b/includes/utils/IP.php @@ -723,53 +723,6 @@ class IP { return "$start/$bits"; } - /** - * Checks if an IP is a trusted proxy provider. - * Useful to tell if X-Forwarded-For data is possibly bogus. - * CDN cache servers for the site are whitelisted. - * @since 1.24 - * - * @param string $ip - * @return bool - */ - public static function isTrustedProxy( $ip ) { - $trusted = self::isConfiguredProxy( $ip ); - Hooks::run( 'IsTrustedProxy', [ &$ip, &$trusted ] ); - return $trusted; - } - - /** - * Checks if an IP matches a proxy we've configured - * @since 1.24 - * - * @param string $ip - * @return bool - */ - public static function isConfiguredProxy( $ip ) { - global $wgSquidServers, $wgSquidServersNoPurge; - - // Quick check of known singular proxy servers - $trusted = in_array( $ip, $wgSquidServers ); - - // Check against addresses and CIDR nets in the NoPurge list - if ( !$trusted ) { - if ( !self::$proxyIpSet ) { - self::$proxyIpSet = new IPSet( $wgSquidServersNoPurge ); - } - $trusted = self::$proxyIpSet->match( $ip ); - } - - return $trusted; - } - - /** - * Clears precomputed data used for proxy support. - * Use this only for unit tests. - */ - public static function clearCaches() { - self::$proxyIpSet = null; - } - /** * Returns the subnet of a given IP * diff --git a/tests/phpunit/includes/MediaWikiServicesTest.php b/tests/phpunit/includes/MediaWikiServicesTest.php index 41f516a189..a05e39d974 100644 --- a/tests/phpunit/includes/MediaWikiServicesTest.php +++ b/tests/phpunit/includes/MediaWikiServicesTest.php @@ -320,7 +320,8 @@ class MediaWikiServicesTest extends MediaWikiTestCase { '_MediaWikiTitleCodec' => [ '_MediaWikiTitleCodec', MediaWikiTitleCodec::class ], 'TitleFormatter' => [ 'TitleFormatter', TitleFormatter::class ], 'TitleParser' => [ 'TitleParser', TitleParser::class ], - 'VirtualRESTServiceClient' => [ 'VirtualRESTServiceClient', VirtualRESTServiceClient::class ] + 'VirtualRESTServiceClient' => [ 'VirtualRESTServiceClient', VirtualRESTServiceClient::class ], + 'ProxyLookup' => [ 'ProxyLookup', ProxyLookup::class ] ]; } diff --git a/tests/phpunit/includes/WebRequestTest.php b/tests/phpunit/includes/WebRequestTest.php index aade490860..041e7e3cd6 100644 --- a/tests/phpunit/includes/WebRequestTest.php +++ b/tests/phpunit/includes/WebRequestTest.php @@ -10,12 +10,10 @@ class WebRequestTest extends MediaWikiTestCase { parent::setUp(); $this->oldServer = $_SERVER; - IP::clearCaches(); } protected function tearDown() { $_SERVER = $this->oldServer; - IP::clearCaches(); parent::tearDown(); } @@ -367,7 +365,6 @@ class WebRequestTest extends MediaWikiTestCase { public function testGetIP( $expected, $input, $squid, $xffList, $private, $description ) { $_SERVER = $input; $this->setMwGlobals( [ - 'wgSquidServersNoPurge' => $squid, 'wgUsePrivateIPs' => $private, 'wgHooks' => [ 'IsTrustedProxy' => [ @@ -379,6 +376,8 @@ class WebRequestTest extends MediaWikiTestCase { ] ] ); + $this->setService( 'ProxyLookup', new ProxyLookup( [], $squid ) ); + $request = new WebRequest(); $result = $request->getIP(); $this->assertEquals( $expected, $result, $description ); @@ -564,6 +563,7 @@ class WebRequestTest extends MediaWikiTestCase { 'wgUsePrivateIPs' => false, 'wgHooks' => [], ] ); + $this->setService( 'ProxyLookup', new ProxyLookup( [], [] ) ); $request = new WebRequest(); # Next call throw an exception about lacking an IP -- 2.20.1