Merge pull request #54 from akrabat/hop-count

Add hop count parameter

Author: akrabat

README.md

@@ -12,15 +12,17 @@ composer require akrabat/ip-address-middleware
 
 ## Configuration
 
-The constructor takes 4 parameters which can be used to configure this middleware.
+The constructor takes 5 parameters which can be used to configure this middleware.
 
 **Check proxy headers**
 
-Note that the proxy headers are only checked if the first parameter to the constructor is set to `true`. If it is set to `false`, then only `$_SERVER['REMOTE_ADDR']` is used.
+The proxy headers are only checked if the first parameter to the constructor is set to `true`. If it is set to `false`, then only `$_SERVER['REMOTE_ADDR']` is used.
 
 **Trusted Proxies**
 
-If you configure to check the proxy headers (first parameter is `true`), you have to provide an array of trusted proxies as the second parameter. When the array is empty, the proxy headers will always be evaluated which is not recommended. If the array is not empty, it must contain strings with IP addresses (wildcard `*` is allowed in any given part) or networks in CIDR-notation. One of them must match the `$_SERVER['REMOTE_ADDR']` variable in order to allow evaluating the proxy headers - otherwise the `REMOTE_ADDR` itself is returned.
+If you enable checking of the proxy headers (first parameter is `true`), you have to provide an array as the second parameter. This is the list of IP addresses (supporting wildcards) of your proxy servers. If the array is empty, the proxy headers will always be used and the selection is based on the hop count (parameter 5). 
+
+If the array is not empty, it must contain strings with IP addresses (wildcard `*` is allowed in any given part) or networks in CIDR-notation. One of them must match the `$_SERVER['REMOTE_ADDR']` variable in order to allow evaluating the proxy headers - otherwise the `REMOTE_ADDR` itself is returned. This list is not ordered and there is no requirement that any given proxy header includes all the listed proxies.
 
 **Attribute name**
 
@@ -56,6 +58,11 @@ If you use _CloudFlare_, then according to the [documentation][cloudflare] you s
 [nginx]: http://nginx.org/en/docs/http/ngx_http_realip_module.html
 [cloudflare]: https://support.cloudflare.com/hc/en-us/articles/200170986-How-does-Cloudflare-handle-HTTP-Request-headers-
 
+**hop count**
+
+Set this to the number of known proxies between ingress and the application. This is used to determine the number of
+proxies to check in the `X-Forwarded-For` header, and is generally used when the IP addresses of the proxies cannot
+be reliably determined. The default is 0.
 
 ## Security considerations
 
@@ -71,7 +78,7 @@ In Mezzio, copy `Mezzio/config/ip_address.global.php.dist` into your Mezzio Appl
 
 ## Usage
 
-In Slim 3:
+In Slim:
 
 ```php
 $checkProxyHeaders = true; // Note: Never trust the IP address for security processes!

src/IpAddress.php

@@ -50,6 +50,13 @@ class IpAddress implements MiddlewareInterface
      */
     protected $attributeName = 'ip_address';
 
+    /**
+     * Number of hops that can be considered safe. Set to a positive number to enable.
+     *
+     * @var int
+     */
+    protected $hopCount = 0;
+
     /**
      * List of proxy headers inspected for the client IP address
      *
@@ -67,15 +74,17 @@ class IpAddress implements MiddlewareInterface
      * Constructor
      *
      * @param bool $checkProxyHeaders Whether to use proxy headers to determine client IP
-     * @param array $trustedProxies   List of IP addresses of trusted proxies
+     * @param ?array $trustedProxies  Unordered list of IP addresses of trusted proxies
      * @param string $attributeName   Name of attribute added to ServerRequest object
      * @param array $headersToInspect List of headers to inspect
+     * @param int $hopCount           Number of hops that can be considered safe. Set to a positive number to enable
      */
     public function __construct(
         $checkProxyHeaders = false,
         ?array $trustedProxies = null,
         $attributeName = null,
-        array $headersToInspect = []
+        array $headersToInspect = [],
+        int $hopCount = 0
     ) {
         if ($checkProxyHeaders && $trustedProxies === null) {
             throw new \InvalidArgumentException('Use of the forward headers requires an array for trusted proxies.');
@@ -105,6 +114,8 @@ public function __construct(
         if (!empty($headersToInspect)) {
             $this->headersToInspect = $headersToInspect;
         }
+
+        $this->hopCount = $hopCount;
     }
 
     private function parseWildcard(string $ipAddress): array
@@ -211,7 +222,8 @@ protected function determineClientIpAddress($request): ?string
                         $header,
                         $headerValue,
                         $ipAddress,
-                        $trustedProxies
+                        $trustedProxies,
+                        $this->hopCount
                     );
                     break;
                 }
@@ -224,8 +236,9 @@ protected function determineClientIpAddress($request): ?string
     public function getIpAddressFromHeader(
         string $headerName,
         string $headerValue,
-        string $ipAddress,
-        array $trustedProxies
+        string $thisIpAddress,
+        array $trustedProxies,
+        int $hopCount
     ) {
         if (strtolower($headerName) == 'forwarded') {
             // The Forwarded header is different, so we need to extract the for= values. Note that we perform a
@@ -237,13 +250,13 @@ public function getIpAddressFromHeader(
             foreach ($ipList as $ip) {
                 $ip = $this->extractIpAddress($ip);
                 if (!filter_var($ip, FILTER_VALIDATE_IP)) {
-                    return $ipAddress;
+                    return $thisIpAddress;
                 }
             }
         } else {
             $ipList = explode(',', $headerValue);
         }
-        $ipList[] = $ipAddress;
+        $ipList[] = $thisIpAddress;
 
         // Remove port from each item in the list
         $ipList = array_map(function ($ip) {
@@ -253,20 +266,28 @@ public function getIpAddressFromHeader(
         // Ensure all IPs are valid and return $ipAddress if not
         foreach ($ipList as $ip) {
             if (!filter_var($ip, FILTER_VALIDATE_IP)) {
-                return $ipAddress;
+                return $thisIpAddress;
             }
         }
 
         // walk list from right to left removing known proxy IP addresses.
         $ipList = array_reverse($ipList);
+        $count = 0;
         foreach ($ipList as $ip) {
-            $ip = trim($ip);
-            if (!empty($ip) && !$this->isTrustedProxy($ip, $trustedProxies)) {
+            $count++;
+            if (!$this->isTrustedProxy($ip, $trustedProxies)) {
+                if ($count <= $hopCount) {
+                    continue;
+                }
                 return $ip;
+//            } else {
+//                if ($count <= $hopCount) {
+//                    continue;
+//                }
             }
         }
 
-        return $ipAddress;
+        return $thisIpAddress;
     }
 
     protected function isTrustedProxy(string $ipAddress, array $trustedProxies): bool

tests/IpAddressTest.php

@@ -433,4 +433,56 @@ public function testThatATrustedProxiesInWrongPlaceIsIgnored()
 
         $this->assertSame('192.168.1.2', $ipAddress);
     }
+
+    /**
+     * If the hop count is set, then it is used to determine the IP address to return
+     */
+    public function testHopCountIsUsedWhenNoTrustedProxiesAreDefined()
+    {
+        $middleware = new IPAddress(true, [], null, [], 3);
+        $env = [
+            'REMOTE_ADDR' => '192.168.1.1',
+            'HTTP_X_FORWARDED_FOR' => '192.168.1.5, 192.168.1.4, 192.168.1.3, 192.168.1.2'
+        ];
+        $ipAddress = $this->simpleRequest($middleware, $env);
+
+        // With three trusted hops, the 4th IP address should be found
+        $this->assertSame('192.168.1.4', $ipAddress);
+    }
+
+    /**
+     * With the hop count set, the IP address returned is the first IP address after the hop count even
+     * if there are non-trusted IP addresses before it in the list.
+     */
+    public function testHopCountOverridesTrustedProxies()
+    {
+        $middleware = new IPAddress(true, ['192.168.1.2', '192.168.1.1'], null, [], 3);
+        $env = [
+            'REMOTE_ADDR' => '192.168.1.1',
+            'HTTP_X_FORWARDED_FOR' => '192.168.1.5, 192.168.1.4, 192.168.1.3, 192.168.1.2'
+        ];
+        $ipAddress = $this->simpleRequest($middleware, $env);
+
+        // With three trusted hops, the 4th IP address should be found even though the third IP address
+        // is not a trusted proxy
+        $this->assertSame('192.168.1.4', $ipAddress);
+    }
+
+    /**
+     * With the hop count is set, if the IP address at the hop count is a trusted proxy, then
+     * select the first IP address after it that is not a trusted proxy
+     */
+    public function testHopCountDoesNotReturnATrustedProxy()
+    {
+        $middleware = new IPAddress(true, ['192.168.1.2', '192.168.1.1'], null, [], 1);
+        $env = [
+            'REMOTE_ADDR' => '192.168.1.1',
+            'HTTP_X_FORWARDED_FOR' => '192.168.1.5, 192.168.1.4, 192.168.1.3, 192.168.1.2'
+        ];
+        $ipAddress = $this->simpleRequest($middleware, $env);
+
+        // With 1 trusted hop, the second IP address would be found, except that it is a trusted proxy
+        // itself, so the third IP address should be found
+        $this->assertSame('192.168.1.3', $ipAddress);
+    }
 }