Move IP::isConfigured/TrustedProxy() to ProxyLookup service
authorKunal Mehta <legoktm@member.fsf.org>
Thu, 22 Sep 2016 02:52:06 +0000 (19:52 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 22 Sep 2016 03:02:09 +0000 (20:02 -0700)
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
autoload.php
includes/Block.php
includes/MediaWikiServices.php
includes/ProxyLookup.php [new file with mode: 0644]
includes/ServiceWiring.php
includes/WebRequest.php
includes/utils/IP.php
tests/phpunit/includes/MediaWikiServicesTest.php
tests/phpunit/includes/WebRequestTest.php

index fd6c613..a24f97a 100644 (file)
@@ -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 ==
 
index f5c334b..aa0f561 100644 (file)
@@ -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',
index 19ba0a2..098d51c 100644 (file)
@@ -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
index f621867..b16044e 100644 (file)
@@ -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 (file)
index 0000000..3a3243a
--- /dev/null
@@ -0,0 +1,85 @@
+<?php
+
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+use IPSet\IPSet;
+
+/**
+ * @since 1.28
+ */
+class ProxyLookup {
+
+       /**
+        * @var string[]
+        */
+       private $proxyServers;
+
+       /**
+        * @var string[]
+        */
+       private $proxyServersComplex;
+
+       /**
+        * @var IPSet|null
+        */
+       private $proxyIPSet;
+
+       /**
+        * @param string[] $proxyServers Simple list of IPs
+        * @param string[] $proxyServersComplex Complex list of IPs/ranges
+        */
+       public function __construct( $proxyServers, $proxyServersComplex ) {
+               $this->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;
+       }
+}
index 8c7d802..6044911 100644 (file)
@@ -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(),
index a5ae461..0065135 100644 (file)
@@ -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] );
index 8676baf..21203a4 100644 (file)
@@ -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
         *
index 41f516a..a05e39d 100644 (file)
@@ -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 ]
                ];
        }
 
index aade490..041e7e3 100644 (file)
@@ -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