Extract the first IP from HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP and HTTP_CF_CONNECTING_IP and HTTP_X_FORWARDED_HOST when there is more than one IP (#10404)

* Extract the first IP from HTTP_X_FORWARDED_FOR when there is more than one
Fixes #10342

* Fetch the first IP from a list of IPs

* Return the first non empty IP

3 files changed, 25 insertions, 19 deletions

diff --git a/config/global.ini.php b/config/global.ini.php
index edf9b867a2..1cdf1cb302 100644
--- a/config/global.ini.php
+++ b/config/global.ini.php
@@ -443,6 +443,7 @@ multisites_refresh_after_seconds = 300
 assume_secure_protocol = 0
 
 ; List of proxy headers for client IP addresses
+; Piwik will determine the user IP by extracting the first IP address found in this proxy header.
 ;
 ; CloudFlare (CF-Connecting-IP)
 ;proxy_client_headers[] = HTTP_CF_CONNECTING_IP
diff --git a/core/IP.php b/core/IP.php
index 919e548353..9f584b1761 100644
--- a/core/IP.php
+++ b/core/IP.php
@@ -86,7 +86,7 @@ class IP
                 // this may be buggy if someone has proxy IPs and proxy host headers configured as
                 // `$_SERVER[$proxyHeader]` could be eg $_SERVER['HTTP_X_FORWARDED_HOST'] and
                 // include an actual host name, not an IP
-                $proxyIp = self::getLastIpFromList($_SERVER[$proxyHeader], $proxyIps);
+                $proxyIp = self::getFirstIpFromList($_SERVER[$proxyHeader], $proxyIps);
                 if (strlen($proxyIp) && stripos($proxyIp, 'unknown') === false) {
                     return $proxyIp;
                 }
@@ -103,13 +103,16 @@ class IP
      * @param array $excludedIps Optional list of excluded IP addresses (or IP address ranges).
      * @return string Last (non-excluded) IP address in the list or an empty string if all given IPs are excluded.
      */
-    public static function getLastIpFromList($csv, $excludedIps = null)
+    public static function getFirstIpFromList($csv, $excludedIps = null)
     {
         $p = strrpos($csv, ',');
         if ($p !== false) {
             $elements = explode(',', $csv);
-            for ($i = count($elements); $i--;) {
-                $element = trim(Common::sanitizeInputValue($elements[$i]));
+            foreach ($elements as $ipString) {
+                $element = trim(Common::sanitizeInputValue($ipString));
+                if(empty($element)) {
+                    continue;
+                }
                 $ip = \Piwik\Network\IP::fromStringIP(IPUtils::sanitizeIp($element));
                 if (empty($excludedIps) || (!in_array($element, $excludedIps) && !$ip->isInRanges($excludedIps))) {
                     return $element;
diff --git a/tests/PHPUnit/Unit/IPTest.php b/tests/PHPUnit/Unit/IPTest.php
index 139687f619..0b6ca8b026 100644
--- a/tests/PHPUnit/Unit/IPTest.php
+++ b/tests/PHPUnit/Unit/IPTest.php
@@ -74,7 +74,7 @@ class IPTest extends \PHPUnit_Framework_TestCase
             array('localhost inside LAN', array('127.0.0.1', '', null, null, '127.0.0.1')),
             array('outside LAN, no proxy', array('128.252.135.4', '', null, null, '128.252.135.4')),
             array('outside LAN, no (trusted) proxy', array('128.252.135.4', '137.18.2.13, 128.252.135.4', '', null, '128.252.135.4')),
-            array('outside LAN, one trusted proxy', array('192.168.1.10', '137.18.2.13, 128.252.135.4, 192.168.1.10', 'HTTP_X_FORWARDED_FOR', null, '128.252.135.4')),
+            array('outside LAN, one trusted proxy', array('137.18.2.13', '137.18.2.13, 128.252.135.4, 192.168.1.10', 'HTTP_X_FORWARDED_FOR', null, '128.252.135.4')),
             array('outside LAN, proxy', array('192.168.1.10', '128.252.135.4, 192.168.1.10', 'HTTP_X_FORWARDED_FOR', null, '128.252.135.4')),
             array('outside LAN, misconfigured proxy', array('192.168.1.10', '128.252.135.4, 192.168.1.10, 192.168.1.10', 'HTTP_X_FORWARDED_FOR', null, '128.252.135.4')),
             array('outside LAN, multiple proxies', array('192.168.1.10', '128.252.135.4, 192.168.1.20, 192.168.1.10', 'HTTP_X_FORWARDED_FOR', '192.168.1.*', '128.252.135.4')),
@@ -138,14 +138,14 @@ class IPTest extends \PHPUnit_Framework_TestCase
         $_SERVER['REMOTE_ADDR'] = '1.1.1.1';
 
         $_SERVER['HTTP_X_FORWARDED_FOR'] = $ip;
-        $this->assertEquals($ip, IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')));
+        $this->assertEquals($ip, IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')), 'case 1');
 
         $_SERVER['HTTP_X_FORWARDED_FOR'] = '1.2.3.4, ' . $ip;
-        $this->assertEquals($ip, IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')));
+        $this->assertEquals('1.2.3.4', IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')), 'case 2');
 
         // misconfiguration
         $_SERVER['HTTP_X_FORWARDED_FOR'] = $ip . ', 1.1.1.1';
-        $this->assertEquals($ip, IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')));
+        $this->assertEquals($ip, IP::getNonProxyIpFromHeader('1.1.1.1', array('HTTP_X_FORWARDED_FOR')), 'case 3');
     }
 
     /**
@@ -163,35 +163,37 @@ class IPTest extends \PHPUnit_Framework_TestCase
     }
 
     /**
-     * Dataprovider for testGetLastIpFromList
+     * Dataprovider for testGetFirstIpFromList
      */
-    public function getLastIpFromListTestData()
+    public function getFirstIpFromListTestData()
     {
         return array(
             array('', ''),
             array('127.0.0.1', '127.0.0.1'),
             array(' 127.0.0.1 ', '127.0.0.1'),
-            array(' 192.168.1.1, 127.0.0.1', '127.0.0.1'),
-            array('192.168.1.1 ,127.0.0.1 ', '127.0.0.1'),
-            array('192.168.1.1,', ''),
+            array(' 192.168.1.1, 127.0.0.1', '192.168.1.1'),
+            array('192.168.1.1 ,127.0.0.1 ', '192.168.1.1'),
+            array('2001:db8:cafe::17 , 192.168.1.1', '2001:db8:cafe::17'),
+            array('192.168.1.1,', '192.168.1.1'),
+            array(',192.168.1.1,', '192.168.1.1'),
         );
     }
 
     /**
-     * @dataProvider getLastIpFromListTestData
+     * @dataProvider getFirstIpFromListTestData
      */
-    public function testGetLastIpFromList($csv, $expected)
+    public function testGetFirstIpFromList($csv, $expected)
     {
         // without excluded IPs
-        $this->assertEquals($expected, IP::getLastIpFromList($csv));
+        $this->assertEquals($expected, IP::getFirstIpFromList($csv));
 
         // with excluded Ips
-        $this->assertEquals($expected, IP::getLastIpFromList($csv . ', 10.10.10.10', array('10.10.10.10')));
+        $this->assertEquals($expected, IP::getFirstIpFromList($csv . ', 10.10.10.10', array('10.10.10.10')));
     }
 
-    public function testGetLastIpFromList_shouldReturnAnEmptyString_IfMultipleIpsAreGivenButAllAreExcluded()
+    public function testGetFirstIpFromList_shouldReturnAnEmptyString_IfMultipleIpsAreGivenButAllAreExcluded()
     {
         // with excluded Ips
-        $this->assertEquals('', IP::getLastIpFromList('10.10.10.10, 10.10.10.10', array('10.10.10.10')));
+        $this->assertEquals('', IP::getFirstIpFromList('10.10.10.10, 10.10.10.10', array('10.10.10.10')));
     }
 }