jenkins-bot has submitted this change and it was merged.

Change subject: Move IP::isConfigured/TrustedProxy() to ProxyLookup service
......................................................................


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
---
M RELEASE-NOTES-1.28
M autoload.php
M includes/Block.php
M includes/MediaWikiServices.php
A includes/ProxyLookup.php
M includes/ServiceWiring.php
M includes/WebRequest.php
M includes/utils/IP.php
M tests/phpunit/includes/MediaWikiServicesTest.php
M tests/phpunit/includes/WebRequestTest.php
10 files changed, 121 insertions(+), 55 deletions(-)

Approvals:
  Aaron Schulz: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28
index fd6c613..a24f97a 100644
--- a/RELEASE-NOTES-1.28
+++ b/RELEASE-NOTES-1.28
@@ -202,6 +202,9 @@
   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 f5c334b..aa0f561 100644
--- a/autoload.php
+++ b/autoload.php
@@ -1100,6 +1100,7 @@
        '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 19ba0a2..098d51c 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 @@
                }
 
                $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 @@
                                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 f621867..b16044e 100644
--- a/includes/MediaWikiServices.php
+++ b/includes/MediaWikiServices.php
@@ -19,6 +19,7 @@
 use MediaWiki\Services\NoSuchServiceException;
 use MWException;
 use ObjectCache;
+use ProxyLookup;
 use SearchEngine;
 use SearchEngineConfig;
 use SearchEngineFactory;
@@ -531,6 +532,14 @@
 
        /**
         * @since 1.28
+        * @return ProxyLookup
+        */
+       public function getProxyLookup() {
+               return $this->getService( 'ProxyLookup' );
+       }
+
+       /**
+        * @since 1.28
         * @return GenderCache
         */
        public function getGenderCache() {
diff --git a/includes/ProxyLookup.php b/includes/ProxyLookup.php
new file mode 100644
index 0000000..3a3243a
--- /dev/null
+++ b/includes/ProxyLookup.php
@@ -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;
+       }
+}
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index 8c7d802..6044911 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -164,6 +164,14 @@
                );
        },
 
+       '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 a5ae461..0065135 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 @@
                # 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 @@
                        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 8676baf..21203a4 100644
--- a/includes/utils/IP.php
+++ b/includes/utils/IP.php
@@ -724,53 +724,6 @@
        }
 
        /**
-        * 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
         *
         * @param string $ip
diff --git a/tests/phpunit/includes/MediaWikiServicesTest.php 
b/tests/phpunit/includes/MediaWikiServicesTest.php
index 41f516a..a05e39d 100644
--- a/tests/phpunit/includes/MediaWikiServicesTest.php
+++ b/tests/phpunit/includes/MediaWikiServicesTest.php
@@ -320,7 +320,8 @@
                        '_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 aade490..041e7e3 100644
--- a/tests/phpunit/includes/WebRequestTest.php
+++ b/tests/phpunit/includes/WebRequestTest.php
@@ -10,12 +10,10 @@
                parent::setUp();
 
                $this->oldServer = $_SERVER;
-               IP::clearCaches();
        }
 
        protected function tearDown() {
                $_SERVER = $this->oldServer;
-               IP::clearCaches();
 
                parent::tearDown();
        }
@@ -367,7 +365,6 @@
        public function testGetIP( $expected, $input, $squid, $xffList, 
$private, $description ) {
                $_SERVER = $input;
                $this->setMwGlobals( [
-                       'wgSquidServersNoPurge' => $squid,
                        'wgUsePrivateIPs' => $private,
                        'wgHooks' => [
                                'IsTrustedProxy' => [
@@ -378,6 +375,8 @@
                                ]
                        ]
                ] );
+
+               $this->setService( 'ProxyLookup', new ProxyLookup( [], $squid ) 
);
 
                $request = new WebRequest();
                $result = $request->getIP();
@@ -564,6 +563,7 @@
                        'wgUsePrivateIPs' => false,
                        'wgHooks' => [],
                ] );
+               $this->setService( 'ProxyLookup', new ProxyLookup( [], [] ) );
 
                $request = new WebRequest();
                # Next call throw an exception about lacking an IP

-- 
To view, visit https://gerrit.wikimedia.org/r/312162
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I60434a5f3d99880352bc0f72349c33b7d029ae09
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Legoktm <legoktm.wikipe...@gmail.com>
Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Legoktm <legoktm.wikipe...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to